From c5ee0fe7b49979a2ddf83aeb5138938c4cb837fc Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 29 Dec 2017 15:52:43 +0100 Subject: [PATCH] AccountManager: Create account and username atomically So far the account was created first and then in a second transaction the external ID for the username was added. This meant that the account creation needed to be rolled back if the creation of the external ID for the username failed. With this change the account and the external ID for the username are now created within the same transaction. ChangeUserName is now only used by PutUsername and hence it is merged into this class. Change-Id: I8f06f792b02331d10b943a52c6c890830087caec Signed-off-by: Edwin Kempin --- .../gerrit/server/account/AccountManager.java | 106 ++++++----------- .../account/AccountUserNameException.java | 4 + .../gerrit/server/account/ChangeUserName.java | 112 ------------------ .../account/externalids/ExternalId.java | 7 ++ .../server/config/GerritGlobalModule.java | 2 - .../server/restapi/account/PutUsername.java | 62 +++++++--- 6 files changed, 92 insertions(+), 201 deletions(-) delete mode 100644 java/com/google/gerrit/server/account/ChangeUserName.java diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java index 3f87e5f6e6..c460b0ec34 100644 --- a/java/com/google/gerrit/server/account/AccountManager.java +++ b/java/com/google/gerrit/server/account/AccountManager.java @@ -14,6 +14,9 @@ package com.google.gerrit.server.account; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME; + import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -21,7 +24,6 @@ import com.google.common.collect.Sets; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.Permission; -import com.google.gerrit.common.errors.NameAlreadyUsedException; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.client.AccountFieldName; import com.google.gerrit.reviewdb.client.Account; @@ -39,6 +41,7 @@ import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.group.db.GroupsUpdate; import com.google.gerrit.server.group.db.InternalGroupUpdate; import com.google.gerrit.server.project.ProjectCache; +import com.google.gerrit.server.ssh.SshKeyCache; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; @@ -67,7 +70,7 @@ public class AccountManager { private final AccountCache byIdCache; private final Realm realm; private final IdentifiedUser.GenericFactory userFactory; - private final ChangeUserName.Factory changeUserNameFactory; + private final SshKeyCache sshKeyCache; private final ProjectCache projectCache; private final AtomicBoolean awaitsFirstAccountCheck; private final ExternalIds externalIds; @@ -86,7 +89,7 @@ public class AccountManager { AccountCache byIdCache, Realm accountMapper, IdentifiedUser.GenericFactory userFactory, - ChangeUserName.Factory changeUserNameFactory, + SshKeyCache sshKeyCache, ProjectCache projectCache, ExternalIds externalIds, ExternalIdsUpdate.Server externalIdsUpdateFactory, @@ -99,7 +102,7 @@ public class AccountManager { this.byIdCache = byIdCache; this.realm = accountMapper; this.userFactory = userFactory; - this.changeUserNameFactory = changeUserNameFactory; + this.sshKeyCache = sshKeyCache; this.projectCache = projectCache; this.awaitsFirstAccountCheck = new AtomicBoolean(cfg.getBoolean("capability", "makeFirstUserAdmin", true)); @@ -262,6 +265,8 @@ public class AccountManager { ExternalId extId = ExternalId.createWithEmail(who.getExternalIdKey(), newId, who.getEmailAddress()); + ExternalId userNameExtId = + !Strings.isNullOrEmpty(who.getUserName()) ? createUsername(newId, who.getUserName()) : null; boolean isFirstAccount = awaitsFirstAccountCheck.getAndSet(false) && !accounts.hasAnyAccount(); @@ -273,10 +278,14 @@ public class AccountManager { .insert( "Create Account on First Login", newId, - u -> - u.setFullName(who.getDisplayName()) - .setPreferredEmail(extId.email()) - .addExternalId(extId)); + u -> { + u.setFullName(who.getDisplayName()) + .setPreferredEmail(extId.email()) + .addExternalId(extId); + if (userNameExtId != null) { + u.addExternalId(userNameExtId); + } + }); } catch (DuplicateExternalIdKeyException e) { throw new AccountException( "Cannot assign external ID \"" @@ -291,6 +300,10 @@ public class AccountManager { awaitsFirstAccountCheck.set(isFirstAccount); } + if (userNameExtId != null) { + sshKeyCache.evict(who.getUserName()); + } + IdentifiedUser user = userFactory.create(newId); if (isFirstAccount) { @@ -309,37 +322,23 @@ public class AccountManager { addGroupMember(db, adminGroupUuid, user); } - if (who.getUserName() != null) { - // Only set if the name hasn't been used yet, but was given to us. - // - try { - changeUserNameFactory.create("Set Username on Login", user, who.getUserName()).call(); - } catch (NameAlreadyUsedException e) { - String message = - "Cannot assign user name \"" - + who.getUserName() - + "\" to account " - + newId - + "; name already in use."; - handleSettingUserNameFailure(account, extId, message, e, false); - } catch (InvalidUserNameException e) { - String message = - "Cannot assign user name \"" - + who.getUserName() - + "\" to account " - + newId - + "; name does not conform."; - handleSettingUserNameFailure(account, extId, message, e, false); - } catch (OrmException e) { - String message = "Cannot assign user name"; - handleSettingUserNameFailure(account, extId, message, e, true); - } - } - realm.onCreateAccount(who, account); return new AuthResult(newId, extId.key(), true); } + private ExternalId createUsername(Account.Id accountId, String username) + throws AccountUserNameException { + checkArgument(!Strings.isNullOrEmpty(username)); + + if (!ExternalId.isValidUsername(username)) { + throw new AccountUserNameException( + String.format( + "Cannot assign user name \"%s\" to account %s; name does not conform.", + username, accountId)); + } + return ExternalId.create(SCHEME_USERNAME, username, accountId); + } + private void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, IdentifiedUser user) throws OrmException, IOException, ConfigInvalidException, AccountException { // The user initiated this request by logging in. -> Attribute all modifications to that user. @@ -356,43 +355,6 @@ public class AccountManager { } } - /** - * This method handles an exception that occurred during the setting of the user name for a newly - * created account. If the realm does not allow the user to set a user name manually this method - * deletes the newly created account and throws an {@link AccountUserNameException}. In any case - * the error message is logged. - * - * @param account the newly created account - * @param extId the newly created external id - * @param errorMessage the error message - * @param e the exception that occurred during the setting of the user name for the new account - * @param logException flag that decides whether the exception should be included into the log - * @throws AccountUserNameException thrown if the realm does not allow the user to manually set - * the user name - * @throws OrmException thrown if cleaning the database failed - */ - private void handleSettingUserNameFailure( - Account account, ExternalId extId, String errorMessage, Exception e, boolean logException) - throws AccountUserNameException, OrmException, IOException, ConfigInvalidException { - if (logException) { - log.error(errorMessage, e); - } else { - log.error(errorMessage); - } - if (!realm.allowsEdit(AccountFieldName.USER_NAME)) { - // setting the given user name has failed, but the realm does not - // allow the user to manually set a user name, - // this means we would end with an account without user name - // (without 'username:' external ID), - // such an account cannot be used for uploading changes, - // this is why the best we can do here is to fail early and cleanup - // the database - accountsUpdateFactory.create().delete(account); - externalIdsUpdateFactory.create().delete(extId); - throw new AccountUserNameException(errorMessage, e); - } - } - /** * Link another authentication identity to an existing account. * diff --git a/java/com/google/gerrit/server/account/AccountUserNameException.java b/java/com/google/gerrit/server/account/AccountUserNameException.java index f1a2555bad..a1f1df23f4 100644 --- a/java/com/google/gerrit/server/account/AccountUserNameException.java +++ b/java/com/google/gerrit/server/account/AccountUserNameException.java @@ -21,6 +21,10 @@ package com.google.gerrit.server.account; public class AccountUserNameException extends AccountException { private static final long serialVersionUID = 1L; + public AccountUserNameException(String message) { + super(message); + } + public AccountUserNameException(String message, Throwable why) { super(message, why); } diff --git a/java/com/google/gerrit/server/account/ChangeUserName.java b/java/com/google/gerrit/server/account/ChangeUserName.java deleted file mode 100644 index b332b75339..0000000000 --- a/java/com/google/gerrit/server/account/ChangeUserName.java +++ /dev/null @@ -1,112 +0,0 @@ -// Copyright (C) 2009 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.server.account; - -import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME; - -import com.google.gerrit.common.Nullable; -import com.google.gerrit.common.errors.NameAlreadyUsedException; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.account.externalids.ExternalId; -import com.google.gerrit.server.account.externalids.ExternalIds; -import com.google.gerrit.server.ssh.SshKeyCache; -import com.google.gwtjsonrpc.common.VoidResult; -import com.google.gwtorm.server.OrmDuplicateKeyException; -import com.google.gwtorm.server.OrmException; -import com.google.inject.Inject; -import com.google.inject.assistedinject.Assisted; -import java.io.IOException; -import java.util.concurrent.Callable; -import java.util.regex.Pattern; -import org.eclipse.jgit.errors.ConfigInvalidException; - -/** Operation to change the username of an account. */ -public class ChangeUserName implements Callable { - public static final String USERNAME_CANNOT_BE_CHANGED = "Username cannot be changed."; - - private static final Pattern USER_NAME_PATTERN = Pattern.compile(Account.USER_NAME_PATTERN); - - /** Generic factory to change any user's username. */ - public interface Factory { - ChangeUserName create( - @Assisted("message") String message, - IdentifiedUser user, - @Assisted("newUsername") String newUsername); - } - - private final SshKeyCache sshKeyCache; - private final ExternalIds externalIds; - private final AccountsUpdate.Server accountsUpdate; - - private final String message; - private final IdentifiedUser user; - private final String newUsername; - - @Inject - ChangeUserName( - SshKeyCache sshKeyCache, - ExternalIds externalIds, - AccountsUpdate.Server accountsUpdate, - @Assisted("message") String message, - @Assisted IdentifiedUser user, - @Nullable @Assisted("newUsername") String newUsername) { - this.sshKeyCache = sshKeyCache; - this.externalIds = externalIds; - this.accountsUpdate = accountsUpdate; - this.message = message; - this.user = user; - this.newUsername = newUsername; - } - - @Override - public VoidResult call() - throws OrmException, NameAlreadyUsedException, InvalidUserNameException, IOException, - ConfigInvalidException { - if (!externalIds.byAccount(user.getAccountId(), SCHEME_USERNAME).isEmpty()) { - throw new IllegalStateException(USERNAME_CANNOT_BE_CHANGED); - } - - if (newUsername != null && !newUsername.isEmpty()) { - if (!USER_NAME_PATTERN.matcher(newUsername).matches()) { - throw new InvalidUserNameException(); - } - - ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, newUsername); - try { - accountsUpdate - .create() - .update( - message, - user.getAccountId(), - u -> u.addExternalId(ExternalId.create(key, user.getAccountId(), null, null))); - } catch (OrmDuplicateKeyException dupeErr) { - // If we are using this identity, don't report the exception. - // - ExternalId other = externalIds.get(key); - if (other != null && other.accountId().equals(user.getAccountId())) { - return VoidResult.INSTANCE; - } - - // Otherwise, someone else has this identity. - // - throw new NameAlreadyUsedException(newUsername); - } - } - - sshKeyCache.evict(newUsername); - return VoidResult.INSTANCE; - } -} diff --git a/java/com/google/gerrit/server/account/externalids/ExternalId.java b/java/com/google/gerrit/server/account/externalids/ExternalId.java index 1ac8f3dd6d..8be709221b 100644 --- a/java/com/google/gerrit/server/account/externalids/ExternalId.java +++ b/java/com/google/gerrit/server/account/externalids/ExternalId.java @@ -33,6 +33,7 @@ import java.io.Serializable; import java.util.Collection; import java.util.Objects; import java.util.Set; +import java.util.regex.Pattern; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; @@ -40,6 +41,12 @@ import org.eclipse.jgit.lib.ObjectId; @AutoValue public abstract class ExternalId implements Serializable { + private static final Pattern USER_NAME_PATTERN = Pattern.compile(Account.USER_NAME_PATTERN); + + public static boolean isValidUsername(String username) { + return USER_NAME_PATTERN.matcher(username).matches(); + } + private static final long serialVersionUID = 1L; private static final String EXTERNAL_ID_SECTION = "externalId"; diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index 108f89ca4a..74e19b6ddd 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -86,7 +86,6 @@ import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.account.AccountResolver; import com.google.gerrit.server.account.AccountVisibilityProvider; import com.google.gerrit.server.account.CapabilityCollection; -import com.google.gerrit.server.account.ChangeUserName; import com.google.gerrit.server.account.EmailExpander; import com.google.gerrit.server.account.GroupCacheImpl; import com.google.gerrit.server.account.GroupControl; @@ -419,7 +418,6 @@ public class GerritGlobalModule extends FactoryModule { factory(VersionedAuthorizedKeys.Factory.class); bind(AccountManager.class); - factory(ChangeUserName.Factory.class); bind(new TypeLiteral>() {}) .toProvider(CommentLinkProvider.class) diff --git a/java/com/google/gerrit/server/restapi/account/PutUsername.java b/java/com/google/gerrit/server/restapi/account/PutUsername.java index 646fd44787..fc40152798 100644 --- a/java/com/google/gerrit/server/restapi/account/PutUsername.java +++ b/java/com/google/gerrit/server/restapi/account/PutUsername.java @@ -14,7 +14,9 @@ package com.google.gerrit.server.restapi.account; -import com.google.gerrit.common.errors.NameAlreadyUsedException; +import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME; + +import com.google.common.base.Strings; import com.google.gerrit.extensions.api.accounts.UsernameInput; import com.google.gerrit.extensions.client.AccountFieldName; import com.google.gerrit.extensions.restapi.AuthException; @@ -22,14 +24,18 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AccountResource; -import com.google.gerrit.server.account.ChangeUserName; -import com.google.gerrit.server.account.InvalidUserNameException; +import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.Realm; +import com.google.gerrit.server.account.externalids.ExternalId; +import com.google.gerrit.server.account.externalids.ExternalIds; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.ssh.SshKeyCache; +import com.google.gwtorm.server.OrmDuplicateKeyException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -40,19 +46,25 @@ import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PutUsername implements RestModifyView { private final Provider self; - private final ChangeUserName.Factory changeUserNameFactory; private final PermissionBackend permissionBackend; + private final ExternalIds externalIds; + private final AccountsUpdate.Server accountsUpdate; + private final SshKeyCache sshKeyCache; private final Realm realm; @Inject PutUsername( Provider self, - ChangeUserName.Factory changeUserNameFactory, PermissionBackend permissionBackend, + ExternalIds externalIds, + AccountsUpdate.Server accountsUpdate, + SshKeyCache sshKeyCache, Realm realm) { this.self = self; - this.changeUserNameFactory = changeUserNameFactory; this.permissionBackend = permissionBackend; + this.externalIds = externalIds; + this.accountsUpdate = accountsUpdate; + this.sshKeyCache = sshKeyCache; this.realm = realm; } @@ -73,19 +85,39 @@ public class PutUsername implements RestModifyView u.addExternalId(ExternalId.create(key, accountId, null, null))); + } catch (OrmDuplicateKeyException dupeErr) { + // If we are using this identity, don't report the exception. + ExternalId other = externalIds.get(key); + if (other != null && other.accountId().equals(accountId)) { + return input.username; + } + + // Otherwise, someone else has this identity. throw new ResourceConflictException("username already used"); } + sshKeyCache.evict(input.username); return input.username; } }