From c5ee0fe7b49979a2ddf83aeb5138938c4cb837fc Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 29 Dec 2017 15:52:43 +0100 Subject: [PATCH 1/7] 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; } } From 3e4dd8f2780235b28cb1c5b6e803f48b0eb1d737 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 2 Jan 2018 08:30:45 +0100 Subject: [PATCH 2/7] AccountsUpdate: Remove methods to delete account Gerrit doesn't support deletion of accounts. AccountsUpdate only supported the deletion of user branches to do a rollback if there was an error during account creation. Since the account creation is now done atomically we no longer need to manually rollback the creation of user branches. Hence the delete methods from AccountsUpdate can be removed. The last remaining callers of these methods were a test and a schema migration in which the needed functionality can be inlined. Change-Id: I58d591dba50aa70d5423f1d79b8e9af47bf55281 Signed-off-by: Edwin Kempin --- .../gerrit/server/account/AccountsUpdate.java | 74 ------------------- .../gerrit/server/schema/Schema_147.java | 35 ++++++--- .../server/change/ConsistencyCheckerIT.java | 27 ++++++- 3 files changed, 46 insertions(+), 90 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java index ee3cf87f61..1e033ee1e1 100644 --- a/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/java/com/google/gerrit/server/account/AccountsUpdate.java @@ -24,8 +24,6 @@ import com.google.common.collect.Lists; import com.google.common.util.concurrent.Runnables; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.externalids.ExternalIdNotes; @@ -51,11 +49,7 @@ import java.util.Optional; import java.util.function.Consumer; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.BatchRefUpdate; -import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; -import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.RefUpdate; -import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.lib.Repository; /** @@ -446,74 +440,6 @@ public class AccountsUpdate { }); } - /** - * Deletes the account. - * - * @param account the account that should be deleted - * @throws IOException if deleting the user branch fails due to an IO error - * @throws OrmException if deleting the user branch fails - * @throws ConfigInvalidException - */ - public void delete(Account account) throws IOException, OrmException, ConfigInvalidException { - deleteByKey(account.getId()); - } - - /** - * Deletes the account. - * - * @param accountId the ID of the account that should be deleted - * @throws IOException if deleting the user branch fails due to an IO error - * @throws OrmException if deleting the user branch fails - * @throws ConfigInvalidException - */ - public void deleteByKey(Account.Id accountId) - throws IOException, OrmException, ConfigInvalidException { - deleteAccount(accountId); - } - - private Account deleteAccount(Account.Id accountId) - throws IOException, OrmException, ConfigInvalidException { - return retryHelper.execute( - ActionType.ACCOUNT_UPDATE, - () -> { - deleteUserBranch(accountId); - return null; - }); - } - - private void deleteUserBranch(Account.Id accountId) throws IOException { - try (Repository repo = repoManager.openRepository(allUsersName)) { - deleteUserBranch(repo, allUsersName, gitRefUpdated, currentUser, authorIdent, accountId); - } - } - - public static void deleteUserBranch( - Repository repo, - Project.NameKey project, - GitReferenceUpdated gitRefUpdated, - @Nullable IdentifiedUser user, - PersonIdent refLogIdent, - Account.Id accountId) - throws IOException { - String refName = RefNames.refsUsers(accountId); - Ref ref = repo.exactRef(refName); - if (ref == null) { - return; - } - - RefUpdate ru = repo.updateRef(refName); - ru.setExpectedOldObjectId(ref.getObjectId()); - ru.setNewObjectId(ObjectId.zeroId()); - ru.setForceUpdate(true); - ru.setRefLogIdent(refLogIdent); - ru.setRefLogMessage("Delete Account", true); - Result result = ru.delete(); - if (result != Result.FORCED) { - throw new IOException(String.format("Failed to delete ref %s: %s", refName, result.name())); - } - gitRefUpdated.fire(project, ru, user != null ? user.getAccount() : null); - } - private AccountConfig read(Repository allUsersRepo, Account.Id accountId) throws IOException, ConfigInvalidException { AccountConfig accountConfig = new AccountConfig(emailValidator, accountId); diff --git a/java/com/google/gerrit/server/schema/Schema_147.java b/java/com/google/gerrit/server/schema/Schema_147.java index 29ae7d5a19..fd85463af2 100644 --- a/java/com/google/gerrit/server/schema/Schema_147.java +++ b/java/com/google/gerrit/server/schema/Schema_147.java @@ -19,10 +19,7 @@ import static java.util.stream.Collectors.toSet; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.GerritPersonIdent; -import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -34,25 +31,23 @@ import java.sql.Statement; import java.util.HashSet; import java.util.Objects; import java.util.Set; -import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.lib.Repository; /** Delete user branches for which no account exists. */ public class Schema_147 extends SchemaVersion { private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; - private final PersonIdent serverIdent; @Inject Schema_147( - Provider prior, - GitRepositoryManager repoManager, - AllUsersName allUsersName, - @GerritPersonIdent PersonIdent serverIdent) { + Provider prior, GitRepositoryManager repoManager, AllUsersName allUsersName) { super(prior); this.repoManager = repoManager; this.allUsersName = allUsersName; - this.serverIdent = serverIdent; } @Override @@ -69,8 +64,7 @@ public class Schema_147 extends SchemaVersion { .collect(toSet()); accountIdsFromUserBranches.removeAll(accountIdsFromReviewDb); for (Account.Id accountId : accountIdsFromUserBranches) { - AccountsUpdate.deleteUserBranch( - repo, allUsersName, GitReferenceUpdated.DISABLED, null, serverIdent, accountId); + deleteUserBranch(repo, accountId); } } catch (IOException e) { throw new OrmException("Failed to delete user branches for non-existing accounts.", e); @@ -87,4 +81,21 @@ public class Schema_147 extends SchemaVersion { return ids; } } + + private void deleteUserBranch(Repository allUsersRepo, Account.Id accountId) throws IOException { + String refName = RefNames.refsUsers(accountId); + Ref ref = allUsersRepo.exactRef(refName); + if (ref == null) { + return; + } + + RefUpdate ru = allUsersRepo.updateRef(refName); + ru.setExpectedOldObjectId(ref.getObjectId()); + ru.setNewObjectId(ObjectId.zeroId()); + ru.setForceUpdate(true); + Result result = ru.delete(); + if (result != Result.FORCED) { + throw new IOException(String.format("Failed to delete ref %s: %s", refName, result.name())); + } + } } diff --git a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index f1385dd39c..f8b765247c 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -41,7 +41,6 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.Sequences; -import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ConsistencyChecker; import com.google.gerrit.server.change.PatchSetInserter; @@ -67,7 +66,10 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.RefUpdate.Result; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Before; import org.junit.Test; @@ -90,8 +92,6 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { @Inject private Sequences sequences; - @Inject private AccountsUpdate.Server accountsUpdate; - private RevCommit tip; private Account.Id adminId; private ConsistencyChecker checker; @@ -126,7 +126,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { public void missingOwner() throws Exception { TestAccount owner = accountCreator.create("missing"); ChangeNotes notes = insertChange(owner); - accountsUpdate.create().deleteByKey(owner.getId()); + deleteUserBranch(owner.getId()); assertProblems(notes, null, problem("Missing change owner: " + owner.getId())); } @@ -958,4 +958,23 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { private void assertNoProblems(ChangeNotes notes, @Nullable FixInput fix) throws Exception { assertThat(checker.check(notes, fix).problems()).isEmpty(); } + + private void deleteUserBranch(Account.Id accountId) throws IOException { + try (Repository repo = repoManager.openRepository(allUsers)) { + String refName = RefNames.refsUsers(accountId); + Ref ref = repo.exactRef(refName); + if (ref == null) { + return; + } + + RefUpdate ru = repo.updateRef(refName); + ru.setExpectedOldObjectId(ref.getObjectId()); + ru.setNewObjectId(ObjectId.zeroId()); + ru.setForceUpdate(true); + Result result = ru.delete(); + if (result != Result.FORCED) { + throw new IOException(String.format("Failed to delete ref %s: %s", refName, result.name())); + } + } + } } From aa1490039fce3d713cb152ba6e8893e8d068df1c Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 2 Jan 2018 11:14:16 +0100 Subject: [PATCH 3/7] AccountManager: Make unlinking ext IDs atomic Change-Id: I4f80207c001baf3006fcfbd11612957dc125bc48 Signed-off-by: Edwin Kempin --- .../gerrit/server/account/AccountManager.java | 34 +++++++------------ 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java index c460b0ec34..1b88e45771 100644 --- a/java/com/google/gerrit/server/account/AccountManager.java +++ b/java/com/google/gerrit/server/account/AccountManager.java @@ -460,27 +460,17 @@ public class AccountManager { } } - externalIdsUpdateFactory.create().delete(extIds); - - if (extIds.stream().anyMatch(e -> e.email() != null)) { - accountsUpdateFactory - .create() - .update( - "Clear Preferred Email on Unlinking External ID\n" - + "\n" - + "The preferred email is cleared because the corresponding external ID\n" - + "was removed.", - from, - (a, u) -> { - if (a.getPreferredEmail() != null) { - for (ExternalId extId : extIds) { - if (a.getPreferredEmail().equals(extId.email())) { - u.setPreferredEmail(null); - break; - } - } - } - }); - } + accountsUpdateFactory + .create() + .update( + "Unlink External ID" + (extIds.size() > 1 ? "s" : ""), + from, + (a, u) -> { + u.deleteExternalIds(extIds); + if (a.getPreferredEmail() != null + && extIds.stream().anyMatch(e -> a.getPreferredEmail().equals(e.email()))) { + u.setPreferredEmail(null); + } + }); } } From 624eb5fc76583a89e74a418909f17d56a9eb835b Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 2 Jan 2018 11:27:25 +0100 Subject: [PATCH 4/7] AccountManager: Use AccountsUpdate to delete ext IDs on update link All account updates (including external ID updates) should be done through AccountsUpdate. We plan to get rid of ExternalIdsUpdate. Updating the link does several external ID updates that could be done in a single transaction but this would prevent us from reusing the existing link and update methods. For now we just want to remove the usage of ExternalIdsUpdate, updating the link atomically can be implemented later if needed. Change-Id: Iac5cd92205628df855ce4c57efdb4b2fc288269c Signed-off-by: Edwin Kempin --- .../google/gerrit/server/account/AccountManager.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java index 1b88e45771..aac515d75e 100644 --- a/java/com/google/gerrit/server/account/AccountManager.java +++ b/java/com/google/gerrit/server/account/AccountManager.java @@ -35,7 +35,6 @@ import com.google.gerrit.server.account.AccountsUpdate.AccountUpdater; import com.google.gerrit.server.account.externalids.DuplicateExternalIdKeyException; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.account.externalids.ExternalIds; -import com.google.gerrit.server.account.externalids.ExternalIdsUpdate; import com.google.gerrit.server.auth.NoSuchUserException; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.group.db.GroupsUpdate; @@ -74,7 +73,6 @@ public class AccountManager { private final ProjectCache projectCache; private final AtomicBoolean awaitsFirstAccountCheck; private final ExternalIds externalIds; - private final ExternalIdsUpdate.Server externalIdsUpdateFactory; private final GroupsUpdate.Factory groupsUpdateFactory; private final boolean autoUpdateAccountActiveStatus; private final SetInactiveFlag setInactiveFlag; @@ -92,7 +90,6 @@ public class AccountManager { SshKeyCache sshKeyCache, ProjectCache projectCache, ExternalIds externalIds, - ExternalIdsUpdate.Server externalIdsUpdateFactory, GroupsUpdate.Factory groupsUpdateFactory, SetInactiveFlag setInactiveFlag) { this.schema = schema; @@ -107,7 +104,6 @@ public class AccountManager { this.awaitsFirstAccountCheck = new AtomicBoolean(cfg.getBoolean("capability", "makeFirstUserAdmin", true)); this.externalIds = externalIds; - this.externalIdsUpdateFactory = externalIdsUpdateFactory; this.groupsUpdateFactory = groupsUpdateFactory; this.autoUpdateAccountActiveStatus = cfg.getBoolean("auth", "autoUpdateAccountActiveStatus", false); @@ -415,7 +411,12 @@ public class AccountManager { .filter(e -> e.key().equals(who.getExternalIdKey())) .findAny() .isPresent())) { - externalIdsUpdateFactory.create().delete(filteredExtIdsByScheme); + accountsUpdateFactory + .create() + .update( + "Delete External IDs on Update Link", + to, + u -> u.deleteExternalIds(filteredExtIdsByScheme)); } return link(to, who); } From aac423526b79ffecc122887876e54833d54bac76 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 2 Jan 2018 13:43:06 +0100 Subject: [PATCH 5/7] Use AccountsUpdate to delete/update GPG keys All account updates (including external ID updates) should be done through AccountsUpdate. We plan to get rid of ExternalIdsUpdate (or at least reduce its functionality to a minimum). Change-Id: I108f14281185eda93e096995320f92f5106ae47c Signed-off-by: Edwin Kempin --- .../gerrit/gpg/server/DeleteGpgKey.java | 24 ++++++---- .../com/google/gerrit/gpg/server/GpgKeys.java | 22 +++++---- .../google/gerrit/gpg/server/PostGpgKeys.java | 47 ++++++++++--------- 3 files changed, 53 insertions(+), 40 deletions(-) diff --git a/java/com/google/gerrit/gpg/server/DeleteGpgKey.java b/java/com/google/gerrit/gpg/server/DeleteGpgKey.java index b9d89ee54f..6d132c8bad 100644 --- a/java/com/google/gerrit/gpg/server/DeleteGpgKey.java +++ b/java/com/google/gerrit/gpg/server/DeleteGpgKey.java @@ -24,8 +24,9 @@ import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.gpg.PublicKeyStore; import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.externalids.ExternalId; -import com.google.gerrit.server.account.externalids.ExternalIdsUpdate; +import com.google.gerrit.server.account.externalids.ExternalIds; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -41,16 +42,19 @@ public class DeleteGpgKey implements RestModifyView { private final Provider serverIdent; private final Provider storeProvider; - private final ExternalIdsUpdate.User externalIdsUpdateFactory; + private final AccountsUpdate.User accountsUpdateFactory; + private final ExternalIds externalIds; @Inject DeleteGpgKey( @GerritPersonIdent Provider serverIdent, Provider storeProvider, - ExternalIdsUpdate.User externalIdsUpdateFactory) { + AccountsUpdate.User accountsUpdateFactory, + ExternalIds externalIds) { this.serverIdent = serverIdent; this.storeProvider = storeProvider; - this.externalIdsUpdateFactory = externalIdsUpdateFactory; + this.accountsUpdateFactory = accountsUpdateFactory; + this.externalIds = externalIds; } @Override @@ -58,12 +62,16 @@ public class DeleteGpgKey implements RestModifyView { throws ResourceConflictException, PGPException, OrmException, IOException, ConfigInvalidException { PGPPublicKey key = rsrc.getKeyRing().getPublicKey(); - externalIdsUpdateFactory - .create() - .delete( - rsrc.getUser().getAccountId(), + ExternalId extId = + externalIds.get( ExternalId.Key.create( SCHEME_GPGKEY, BaseEncoding.base16().encode(key.getFingerprint()))); + accountsUpdateFactory + .create() + .update( + "Delete GPG Key via API", + rsrc.getUser().getAccountId(), + u -> u.deleteExternalId(extId)); try (PublicKeyStore store = storeProvider.get()) { store.remove(rsrc.getKeyRing().getPublicKey().getFingerprint()); diff --git a/java/com/google/gerrit/gpg/server/GpgKeys.java b/java/com/google/gerrit/gpg/server/GpgKeys.java index 63c0476f0d..c4e35e0c8e 100644 --- a/java/com/google/gerrit/gpg/server/GpgKeys.java +++ b/java/com/google/gerrit/gpg/server/GpgKeys.java @@ -92,7 +92,8 @@ public class GpgKeys implements ChildCollection { throws ResourceNotFoundException, PGPException, OrmException, IOException { checkVisible(self, parent); - byte[] fp = parseFingerprint(id.get(), getGpgExtIds(parent)); + ExternalId gpgKeyExtId = findGpgKey(id.get(), getGpgExtIds(parent)); + byte[] fp = parseFingerprint(gpgKeyExtId); try (PublicKeyStore store = storeProvider.get()) { long keyId = keyId(fp); for (PGPPublicKeyRing keyRing : store.get(keyId)) { @@ -106,30 +107,34 @@ public class GpgKeys implements ChildCollection { throw new ResourceNotFoundException(id); } - static byte[] parseFingerprint(String str, Iterable existingExtIds) + static ExternalId findGpgKey(String str, Iterable existingExtIds) throws ResourceNotFoundException { str = CharMatcher.whitespace().removeFrom(str).toUpperCase(); if ((str.length() != 8 && str.length() != 40) || !CharMatcher.anyOf("0123456789ABCDEF").matchesAllOf(str)) { throw new ResourceNotFoundException(str); } - byte[] fp = null; + ExternalId gpgKeyExtId = null; for (ExternalId extId : existingExtIds) { String fpStr = extId.key().id(); if (!fpStr.endsWith(str)) { continue; - } else if (fp != null) { + } else if (gpgKeyExtId != null) { throw new ResourceNotFoundException("Multiple keys found for " + str); } - fp = BaseEncoding.base16().decode(fpStr); + gpgKeyExtId = extId; if (str.length() == 40) { break; } } - if (fp == null) { + if (gpgKeyExtId == null) { throw new ResourceNotFoundException(str); } - return fp; + return gpgKeyExtId; + } + + static byte[] parseFingerprint(ExternalId gpgKeyExtId) { + return BaseEncoding.base16().decode(gpgKeyExtId.key().id()); } @Override @@ -145,8 +150,7 @@ public class GpgKeys implements ChildCollection { Map keys = new HashMap<>(); try (PublicKeyStore store = storeProvider.get()) { for (ExternalId extId : getGpgExtIds(rsrc)) { - String fpStr = extId.key().id(); - byte[] fp = BaseEncoding.base16().decode(fpStr); + byte[] fp = parseFingerprint(extId); boolean found = false; for (PGPPublicKeyRing keyRing : store.get(keyId(fp))) { if (Arrays.equals(keyRing.getPublicKey().getFingerprint(), fp)) { diff --git a/java/com/google/gerrit/gpg/server/PostGpgKeys.java b/java/com/google/gerrit/gpg/server/PostGpgKeys.java index d8ed855b3c..4996e0e05a 100644 --- a/java/com/google/gerrit/gpg/server/PostGpgKeys.java +++ b/java/com/google/gerrit/gpg/server/PostGpgKeys.java @@ -18,14 +18,12 @@ import static com.google.gerrit.gpg.PublicKeyStore.keyIdToString; import static com.google.gerrit.gpg.PublicKeyStore.keyToString; import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GPGKEY; import static java.nio.charset.StandardCharsets.UTF_8; -import static java.util.stream.Collectors.toList; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.collect.Sets; import com.google.common.io.BaseEncoding; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.extensions.api.accounts.GpgKeysInput; @@ -45,9 +43,9 @@ import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountResource; import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.account.externalids.ExternalIds; -import com.google.gerrit.server.account.externalids.ExternalIdsUpdate; import com.google.gerrit.server.mail.send.AddKeySender; import com.google.gerrit.server.query.account.InternalAccountQuery; import com.google.gwtorm.server.OrmException; @@ -61,7 +59,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.Set; import org.bouncycastle.bcpg.ArmoredInputStream; import org.bouncycastle.openpgp.PGPException; import org.bouncycastle.openpgp.PGPPublicKey; @@ -85,7 +82,7 @@ public class PostGpgKeys implements RestModifyView accountQueryProvider; private final ExternalIds externalIds; - private final ExternalIdsUpdate.User externalIdsUpdateFactory; + private final AccountsUpdate.User accountsUpdateFactory; @Inject PostGpgKeys( @@ -96,7 +93,7 @@ public class PostGpgKeys implements RestModifyView accountQueryProvider, ExternalIds externalIds, - ExternalIdsUpdate.User externalIdsUpdateFactory) { + AccountsUpdate.User accountsUpdateFactory) { this.serverIdent = serverIdent; this.self = self; this.storeProvider = storeProvider; @@ -104,7 +101,7 @@ public class PostGpgKeys implements RestModifyView existingExtIds = externalIds.byAccount(rsrc.getUser().getAccountId(), SCHEME_GPGKEY); try (PublicKeyStore store = storeProvider.get()) { - Set toRemove = readKeysToRemove(input, existingExtIds); - List newKeys = readKeysToAdd(input, toRemove); + Map toRemove = readKeysToRemove(input, existingExtIds); + Collection fingerprintsToRemove = toRemove.values(); + List newKeys = readKeysToAdd(input, fingerprintsToRemove); List newExtIds = new ArrayList<>(existingExtIds.size()); for (PGPPublicKeyRing keyRing : newKeys) { @@ -133,26 +131,29 @@ public class PostGpgKeys implements RestModifyView extIdKeysToRemove = - toRemove.stream().map(fp -> toExtIdKey(fp.get())).collect(toList()); - externalIdsUpdateFactory + accountsUpdateFactory .create() - .replace(rsrc.getUser().getAccountId(), extIdKeysToRemove, newExtIds); - return toJson(newKeys, toRemove, store, rsrc.getUser()); + .update( + "Update GPG Keys via API", + rsrc.getUser().getAccountId(), + u -> u.replaceExternalIds(toRemove.keySet(), newExtIds)); + return toJson(newKeys, fingerprintsToRemove, store, rsrc.getUser()); } } - private Set readKeysToRemove( + private Map readKeysToRemove( GpgKeysInput input, Collection existingExtIds) { if (input.delete == null || input.delete.isEmpty()) { - return ImmutableSet.of(); + return ImmutableMap.of(); } - Set fingerprints = Sets.newHashSetWithExpectedSize(input.delete.size()); + Map fingerprints = + Maps.newHashMapWithExpectedSize(input.delete.size()); for (String id : input.delete) { try { - fingerprints.add(new Fingerprint(GpgKeys.parseFingerprint(id, existingExtIds))); + ExternalId gpgKeyExtId = GpgKeys.findGpgKey(id, existingExtIds); + fingerprints.put(gpgKeyExtId, new Fingerprint(GpgKeys.parseFingerprint(gpgKeyExtId))); } catch (ResourceNotFoundException e) { // Skip removal. } @@ -160,7 +161,7 @@ public class PostGpgKeys implements RestModifyView readKeysToAdd(GpgKeysInput input, Set toRemove) + private List readKeysToAdd(GpgKeysInput input, Collection toRemove) throws BadRequestException, IOException { if (input.add == null || input.add.isEmpty()) { return ImmutableList.of(); @@ -188,7 +189,7 @@ public class PostGpgKeys implements RestModifyView keyRings, Set toRemove) + AccountResource rsrc, List keyRings, Collection toRemove) throws BadRequestException, ResourceConflictException, PGPException, IOException { try (PublicKeyStore store = storeProvider.get()) { List addedKeys = new ArrayList<>(); @@ -269,7 +270,7 @@ public class PostGpgKeys implements RestModifyView toJson( Collection keys, - Set deleted, + Collection deleted, PublicKeyStore store, IdentifiedUser user) throws IOException { From e6673fa817184ba1deeb91097e79799060026201 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 2 Jan 2018 14:19:26 +0100 Subject: [PATCH 6/7] Remove ExternalIdsUpdate All account updates (including external ID updates) are done through AccountsUpdate now, hence ExternalIdsUpdate is no longer needed. The test for retrying external ID updates are no longer needed since ExternalIdsUpdate is deleted. The same kind of tests already exist for account updates (see AccountIT#retryOnLockFailure and AccountIT#failAfterRetryerGivesUp). Change-Id: I8946f0de5c70a5db9bbb5b8d3e073549044b2ddc Signed-off-by: Edwin Kempin --- .../externalids/ExternalIdsUpdate.java | 460 ------------------ .../acceptance/rest/account/ExternalIdIT.java | 95 ---- 2 files changed, 555 deletions(-) delete mode 100644 java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java b/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java deleted file mode 100644 index 028fd8d146..0000000000 --- a/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java +++ /dev/null @@ -1,460 +0,0 @@ -// Copyright (C) 2016 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.externalids; - -import static com.google.common.base.Preconditions.checkNotNull; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.util.concurrent.Runnables; -import com.google.gerrit.common.Nullable; -import com.google.gerrit.metrics.Counter0; -import com.google.gerrit.metrics.Description; -import com.google.gerrit.metrics.MetricMaker; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.server.account.AccountCache; -import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.MetaDataUpdate; -import com.google.gerrit.server.update.RetryHelper; -import com.google.gerrit.server.update.RetryHelper.ActionType; -import com.google.gwtorm.server.OrmDuplicateKeyException; -import com.google.gwtorm.server.OrmException; -import com.google.inject.Inject; -import com.google.inject.Provider; -import com.google.inject.Singleton; -import java.io.IOException; -import java.util.Collection; -import java.util.Collections; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.Repository; - -/** - * Updates externalIds in ReviewDb and NoteDb. - * - *

In NoteDb external IDs are stored in the All-Users repository in a Git Notes branch called - * refs/meta/external-ids where the sha1 of the external ID is used as note name. Each note content - * is a git config file that contains an external ID. It has exactly one externalId subsection with - * an accountId and optionally email and password: - * - * 

- * [externalId "username:jdoe"]
- *   accountId = 1003407
- *   email = jdoe@example.com
- *   password = bcrypt:4:LCbmSBDivK/hhGVQMfkDpA==:XcWn0pKYSVU/UJgOvhidkEtmqCp6oKB7
- *
- * - * For NoteDb each method call results in one commit on refs/meta/external-ids branch. - * - *

On updating external IDs this class takes care to evict affected accounts from the account - * cache and thus triggers reindex for them. - */ -public class ExternalIdsUpdate { - /** - * Factory to create an ExternalIdsUpdate instance for updating external IDs by the Gerrit server. - * - *

The Gerrit server identity will be used as author and committer for all commits that update - * the external IDs. - */ - @Singleton - public static class Server { - private final GitRepositoryManager repoManager; - private final Provider metaDataUpdateServerFactory; - private final AccountCache accountCache; - private final AllUsersName allUsersName; - private final MetricMaker metricMaker; - private final ExternalIds externalIds; - private final ExternalIdCache externalIdCache; - private final RetryHelper retryHelper; - - @Inject - public Server( - GitRepositoryManager repoManager, - Provider metaDataUpdateServerFactory, - AccountCache accountCache, - AllUsersName allUsersName, - MetricMaker metricMaker, - ExternalIds externalIds, - ExternalIdCache externalIdCache, - RetryHelper retryHelper) { - this.repoManager = repoManager; - this.metaDataUpdateServerFactory = metaDataUpdateServerFactory; - this.accountCache = accountCache; - this.allUsersName = allUsersName; - this.metricMaker = metricMaker; - this.externalIds = externalIds; - this.externalIdCache = externalIdCache; - this.retryHelper = retryHelper; - } - - public ExternalIdsUpdate create() { - return new ExternalIdsUpdate( - repoManager, - () -> metaDataUpdateServerFactory.get().create(allUsersName), - accountCache, - allUsersName, - metricMaker, - externalIds, - externalIdCache, - retryHelper); - } - } - - /** - * Factory to create an ExternalIdsUpdate instance for updating external IDs by the Gerrit server. - * - *

Using this class no reindex will be performed for the affected accounts and they will also - * not be evicted from the account cache. - * - *

The Gerrit server identity will be used as author and committer for all commits that update - * the external IDs. - */ - @Singleton - public static class ServerNoReindex { - private final GitRepositoryManager repoManager; - private final Provider metaDataUpdateServerFactory; - private final AllUsersName allUsersName; - private final MetricMaker metricMaker; - private final ExternalIds externalIds; - private final ExternalIdCache externalIdCache; - private final RetryHelper retryHelper; - - @Inject - public ServerNoReindex( - GitRepositoryManager repoManager, - Provider metaDataUpdateServerFactory, - AllUsersName allUsersName, - MetricMaker metricMaker, - ExternalIds externalIds, - ExternalIdCache externalIdCache, - RetryHelper retryHelper) { - this.repoManager = repoManager; - this.metaDataUpdateServerFactory = metaDataUpdateServerFactory; - this.allUsersName = allUsersName; - this.metricMaker = metricMaker; - this.externalIds = externalIds; - this.externalIdCache = externalIdCache; - this.retryHelper = retryHelper; - } - - public ExternalIdsUpdate create() { - return new ExternalIdsUpdate( - repoManager, - () -> metaDataUpdateServerFactory.get().create(allUsersName), - null, - allUsersName, - metricMaker, - externalIds, - externalIdCache, - retryHelper); - } - } - - /** - * Factory to create an ExternalIdsUpdate instance for updating external IDs by the current user. - * - *

The identity of the current user will be used as author for all commits that update the - * external IDs. The Gerrit server identity will be used as committer. - */ - @Singleton - public static class User { - private final GitRepositoryManager repoManager; - private final Provider metaDataUpdateUserFactory; - private final AccountCache accountCache; - private final AllUsersName allUsersName; - private final MetricMaker metricMaker; - private final ExternalIds externalIds; - private final ExternalIdCache externalIdCache; - private final RetryHelper retryHelper; - - @Inject - public User( - GitRepositoryManager repoManager, - Provider metaDataUpdateUserFactory, - AccountCache accountCache, - AllUsersName allUsersName, - MetricMaker metricMaker, - ExternalIds externalIds, - ExternalIdCache externalIdCache, - RetryHelper retryHelper) { - this.repoManager = repoManager; - this.metaDataUpdateUserFactory = metaDataUpdateUserFactory; - this.accountCache = accountCache; - this.allUsersName = allUsersName; - this.metricMaker = metricMaker; - this.externalIds = externalIds; - this.externalIdCache = externalIdCache; - this.retryHelper = retryHelper; - } - - public ExternalIdsUpdate create() { - return new ExternalIdsUpdate( - repoManager, - () -> metaDataUpdateUserFactory.get().create(allUsersName), - accountCache, - allUsersName, - metricMaker, - externalIds, - externalIdCache, - retryHelper); - } - } - - private final GitRepositoryManager repoManager; - private final MetaDataUpdateFactory metaDataUpdateFactory; - @Nullable private final AccountCache accountCache; - private final AllUsersName allUsersName; - private final ExternalIds externalIds; - private final ExternalIdCache externalIdCache; - private final RetryHelper retryHelper; - private final Runnable afterReadRevision; - private final Counter0 updateCount; - - private ExternalIdsUpdate( - GitRepositoryManager repoManager, - MetaDataUpdateFactory metaDataUpdateFactory, - @Nullable AccountCache accountCache, - AllUsersName allUsersName, - MetricMaker metricMaker, - ExternalIds externalIds, - ExternalIdCache externalIdCache, - RetryHelper retryHelper) { - this( - repoManager, - metaDataUpdateFactory, - accountCache, - allUsersName, - metricMaker, - externalIds, - externalIdCache, - retryHelper, - Runnables.doNothing()); - } - - @VisibleForTesting - public ExternalIdsUpdate( - GitRepositoryManager repoManager, - MetaDataUpdateFactory metaDataUpdateFactory, - @Nullable AccountCache accountCache, - AllUsersName allUsersName, - MetricMaker metricMaker, - ExternalIds externalIds, - ExternalIdCache externalIdCache, - RetryHelper retryHelper, - Runnable afterReadRevision) { - this.repoManager = checkNotNull(repoManager, "repoManager"); - this.metaDataUpdateFactory = checkNotNull(metaDataUpdateFactory, "metaDataUpdateFactory"); - this.accountCache = accountCache; - this.allUsersName = checkNotNull(allUsersName, "allUsersName"); - this.externalIds = checkNotNull(externalIds, "externalIds"); - this.externalIdCache = checkNotNull(externalIdCache, "externalIdCache"); - this.retryHelper = checkNotNull(retryHelper, "retryHelper"); - this.afterReadRevision = checkNotNull(afterReadRevision, "afterReadRevision"); - this.updateCount = - metricMaker.newCounter( - "notedb/external_id_update_count", - new Description("Total number of external ID updates.").setRate().setUnit("updates")); - } - - /** - * Inserts a new external ID. - * - *

If the external ID already exists, the insert fails with {@link OrmDuplicateKeyException}. - */ - public void insert(ExternalId extId) throws IOException, ConfigInvalidException, OrmException { - insert(Collections.singleton(extId)); - } - - /** - * Inserts new external IDs. - * - *

If any of the external ID already exists, the insert fails with {@link - * OrmDuplicateKeyException}. - */ - public void insert(Collection extIds) - throws IOException, ConfigInvalidException, OrmException { - updateNoteMap(n -> n.insert(extIds)); - } - - /** - * Inserts or updates an external ID. - * - *

If the external ID already exists, it is overwritten, otherwise it is inserted. - */ - public void upsert(ExternalId extId) throws IOException, ConfigInvalidException, OrmException { - upsert(Collections.singleton(extId)); - } - - /** - * Inserts or updates external IDs. - * - *

If any of the external IDs already exists, it is overwritten. New external IDs are inserted. - */ - public void upsert(Collection extIds) - throws IOException, ConfigInvalidException, OrmException { - updateNoteMap(n -> n.upsert(extIds)); - } - - /** - * Deletes an external ID. - * - * @throws IllegalStateException is thrown if there is an existing external ID that has the same - * key, but otherwise doesn't match the specified external ID. - */ - public void delete(ExternalId extId) throws IOException, ConfigInvalidException, OrmException { - delete(Collections.singleton(extId)); - } - - /** - * Deletes external IDs. - * - * @throws IllegalStateException is thrown if there is an existing external ID that has the same - * key as any of the external IDs that should be deleted, but otherwise doesn't match the that - * external ID. - */ - public void delete(Collection extIds) - throws IOException, ConfigInvalidException, OrmException { - updateNoteMap(n -> n.delete(extIds)); - } - - /** - * Delete an external ID by key. - * - * @throws IllegalStateException is thrown if the external ID does not belong to the specified - * account. - */ - public void delete(Account.Id accountId, ExternalId.Key extIdKey) - throws IOException, ConfigInvalidException, OrmException { - delete(accountId, Collections.singleton(extIdKey)); - } - - /** - * Delete external IDs by external ID key. - * - * @throws IllegalStateException is thrown if any of the external IDs does not belong to the - * specified account. - */ - public void delete(Account.Id accountId, Collection extIdKeys) - throws IOException, ConfigInvalidException, OrmException { - updateNoteMap(n -> n.delete(accountId, extIdKeys)); - } - - /** - * Delete external IDs by external ID key. - * - *

The external IDs are deleted regardless of which account they belong to. - */ - public void deleteByKeys(Collection extIdKeys) - throws IOException, ConfigInvalidException, OrmException { - updateNoteMap(n -> n.deleteByKeys(extIdKeys)); - } - - /** Deletes all external IDs of the specified account. */ - public void deleteAll(Account.Id accountId) - throws IOException, ConfigInvalidException, OrmException { - delete(externalIds.byAccount(accountId)); - } - - /** - * Replaces external IDs for an account by external ID keys. - * - *

Deletion of external IDs is done before adding the new external IDs. This means if an - * external ID key is specified for deletion and an external ID with the same key is specified to - * be added, the old external ID with that key is deleted first and then the new external ID is - * added (so the external ID for that key is replaced). - * - * @throws IllegalStateException is thrown if any of the specified external IDs does not belong to - * the specified account. - */ - public void replace( - Account.Id accountId, Collection toDelete, Collection toAdd) - throws IOException, ConfigInvalidException, OrmException { - updateNoteMap(n -> n.replace(accountId, toDelete, toAdd)); - } - - /** - * Replaces external IDs for an account by external ID keys. - * - *

Deletion of external IDs is done before adding the new external IDs. This means if an - * external ID key is specified for deletion and an external ID with the same key is specified to - * be added, the old external ID with that key is deleted first and then the new external ID is - * added (so the external ID for that key is replaced). - * - *

The external IDs are replaced regardless of which account they belong to. - */ - public void replaceByKeys(Collection toDelete, Collection toAdd) - throws IOException, ConfigInvalidException, OrmException { - updateNoteMap(n -> n.replaceByKeys(toDelete, toAdd)); - } - - /** - * Replaces an external ID. - * - * @throws IllegalStateException is thrown if the specified external IDs belong to different - * accounts. - */ - public void replace(ExternalId toDelete, ExternalId toAdd) - throws IOException, ConfigInvalidException, OrmException { - replace(Collections.singleton(toDelete), Collections.singleton(toAdd)); - } - - /** - * Replaces external IDs. - * - *

Deletion of external IDs is done before adding the new external IDs. This means if an - * external ID is specified for deletion and an external ID with the same key is specified to be - * added, the old external ID with that key is deleted first and then the new external ID is added - * (so the external ID for that key is replaced). - * - * @throws IllegalStateException is thrown if the specified external IDs belong to different - * accounts. - */ - public void replace(Collection toDelete, Collection toAdd) - throws IOException, ConfigInvalidException, OrmException { - updateNoteMap(n -> n.replace(toDelete, toAdd)); - } - - private void updateNoteMap(ExternalIdUpdater updater) - throws IOException, ConfigInvalidException, OrmException { - retryHelper.execute( - ActionType.ACCOUNT_UPDATE, - () -> { - try (Repository repo = repoManager.openRepository(allUsersName)) { - ExternalIdNotes extIdNotes = - new ExternalIdNotes(externalIdCache, accountCache, repo) - .setAfterReadRevision(afterReadRevision) - .load(); - updater.update(extIdNotes); - try (MetaDataUpdate metaDataUpdate = metaDataUpdateFactory.create()) { - extIdNotes.commit(metaDataUpdate); - } - extIdNotes.updateCaches(); - updateCount.increment(); - return null; - } - }); - } - - @FunctionalInterface - private static interface ExternalIdUpdater { - void update(ExternalIdNotes extIdsNotes) - throws IOException, ConfigInvalidException, OrmException; - } - - @VisibleForTesting - @FunctionalInterface - public static interface MetaDataUpdateFactory { - MetaDataUpdate create() throws IOException; - } -} diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java index 9ccc138196..2aa8d58e20 100644 --- a/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java @@ -26,7 +26,6 @@ import static java.util.stream.Collectors.toList; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import static org.eclipse.jgit.lib.Constants.OBJ_TREE; -import com.github.rholder.retry.StopStrategies; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -41,20 +40,15 @@ import com.google.gerrit.extensions.api.config.ConsistencyCheckInput.CheckAccoun import com.google.gerrit.extensions.common.AccountExternalIdInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; -import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.account.AccountsUpdate; -import com.google.gerrit.server.account.externalids.DisabledExternalIdCache; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.account.externalids.ExternalIdNotes; import com.google.gerrit.server.account.externalids.ExternalIdReader; import com.google.gerrit.server.account.externalids.ExternalIds; -import com.google.gerrit.server.account.externalids.ExternalIdsUpdate; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; -import com.google.gerrit.server.git.LockFailureException; import com.google.gerrit.server.git.MetaDataUpdate; -import com.google.gerrit.server.update.RetryHelper; import com.google.gson.reflect.TypeToken; import com.google.gwtorm.server.OrmDuplicateKeyException; import com.google.inject.Inject; @@ -67,8 +61,6 @@ import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Set; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; import org.eclipse.jgit.api.errors.TransportException; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; @@ -92,8 +84,6 @@ public class ExternalIdIT extends AbstractDaemonTest { @Inject private AccountsUpdate.Server accountsUpdate; @Inject private ExternalIds externalIds; @Inject private ExternalIdReader externalIdReader; - @Inject private MetricMaker metricMaker; - @Inject private RetryHelper.Metrics retryMetrics; @Inject private ExternalIdNotes.Factory externalIdNotesFactory; @Test @@ -714,91 +704,6 @@ public class ExternalIdIT extends AbstractDaemonTest { return scheme + ":foo" + ++i.value; } - @Test - public void retryOnLockFailure() throws Exception { - ExternalId.Key fooId = ExternalId.Key.create("foo", "foo"); - ExternalId.Key barId = ExternalId.Key.create("bar", "bar"); - - final AtomicBoolean doneBgUpdate = new AtomicBoolean(false); - ExternalIdsUpdate update = - new ExternalIdsUpdate( - repoManager, - () -> metaDataUpdateFactory.create(allUsers), - accountCache, - allUsers, - metricMaker, - externalIds, - new DisabledExternalIdCache(), - new RetryHelper( - cfg, - retryMetrics, - null, - null, - null, - r -> r.withBlockStrategy(noSleepBlockStrategy)), - () -> { - if (!doneBgUpdate.getAndSet(true)) { - try { - insertExtId(ExternalId.create(barId, admin.id)); - } catch (Exception e) { - // Ignore, the successful insertion of the external ID is asserted later - } - } - }); - assertThat(doneBgUpdate.get()).isFalse(); - update.insert(ExternalId.create(fooId, admin.id)); - assertThat(doneBgUpdate.get()).isTrue(); - - assertThat(externalIds.get(fooId)).isNotNull(); - assertThat(externalIds.get(barId)).isNotNull(); - } - - @Test - public void failAfterRetryerGivesUp() throws Exception { - ExternalId.Key[] extIdsKeys = { - ExternalId.Key.create("foo", "foo"), - ExternalId.Key.create("bar", "bar"), - ExternalId.Key.create("baz", "baz") - }; - final AtomicInteger bgCounter = new AtomicInteger(0); - ExternalIdsUpdate update = - new ExternalIdsUpdate( - repoManager, - () -> metaDataUpdateFactory.create(allUsers), - accountCache, - allUsers, - metricMaker, - externalIds, - new DisabledExternalIdCache(), - new RetryHelper( - cfg, - retryMetrics, - null, - null, - null, - r -> - r.withStopStrategy(StopStrategies.stopAfterAttempt(extIdsKeys.length)) - .withBlockStrategy(noSleepBlockStrategy)), - () -> { - try { - insertExtId(ExternalId.create(extIdsKeys[bgCounter.getAndAdd(1)], admin.id)); - } catch (Exception e) { - // Ignore, the successful insertion of the external ID is asserted later - } - }); - assertThat(bgCounter.get()).isEqualTo(0); - try { - update.insert(ExternalId.create(ExternalId.Key.create("abc", "abc"), admin.id)); - fail("expected LockFailureException"); - } catch (LockFailureException e) { - // Ignore, expected - } - assertThat(bgCounter.get()).isEqualTo(extIdsKeys.length); - for (ExternalId.Key extIdKey : extIdsKeys) { - assertThat(externalIds.get(extIdKey)).isNotNull(); - } - } - @Test public void readExternalIdWithAccountIdThatCanBeExpressedInKiB() throws Exception { ExternalId.Key extIdKey = ExternalId.Key.parse("foo:bar"); From f3306ea1f0f24b04035e27e4d36430c04dedcade Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 3 Jan 2018 08:39:05 +0100 Subject: [PATCH 7/7] AccountConfig: Remove unneeded validation of preferred email AccountConfig was validating the preferred email and generated a validation error if it was invalid, however the validation errors were always ignored and never accessed. Instead the validation of the preferred email is done in AccountValidator. Here the validation only results into an error if the preferred email was changed to an invalid email, but not if the email already was invalid and stayed unchanged. This is something that AccountConfig can't do since it doesn't know about the previous account state. Change-Id: I656a4f405bda44c1a4194439061dc73c3c0bea44 Signed-off-by: Edwin Kempin --- .../gerrit/server/account/AccountConfig.java | 38 +------------------ .../gerrit/server/account/Accounts.java | 10 +---- .../gerrit/server/account/AccountsUpdate.java | 20 +--------- .../git/validators/AccountValidator.java | 2 +- .../gerrit/server/schema/Schema_154.java | 2 +- .../acceptance/api/accounts/AccountIT.java | 5 --- .../account/AbstractQueryAccountsTest.java | 2 +- 7 files changed, 8 insertions(+), 71 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountConfig.java b/java/com/google/gerrit/server/account/AccountConfig.java index 1283270d59..830f2798f1 100644 --- a/java/com/google/gerrit/server/account/AccountConfig.java +++ b/java/com/google/gerrit/server/account/AccountConfig.java @@ -18,20 +18,14 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import com.google.common.base.Strings; -import com.google.common.collect.ImmutableList; -import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.git.MetaDataUpdate; -import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.git.VersionedMetaData; -import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.gwtorm.server.OrmDuplicateKeyException; import java.io.IOException; import java.sql.Timestamp; -import java.util.ArrayList; -import java.util.List; import java.util.Optional; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; @@ -67,7 +61,7 @@ import org.eclipse.jgit.revwalk.RevSort; * account. The first commit may be an empty commit (if no properties were set and 'account.config' * doesn't exist). */ -public class AccountConfig extends VersionedMetaData implements ValidationError.Sink { +public class AccountConfig extends VersionedMetaData { public static final String ACCOUNT_CONFIG = "account.config"; public static final String ACCOUNT = "account"; public static final String KEY_ACTIVE = "active"; @@ -75,17 +69,14 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. public static final String KEY_PREFERRED_EMAIL = "preferredEmail"; public static final String KEY_STATUS = "status"; - @Nullable private final OutgoingEmailValidator emailValidator; private final Account.Id accountId; private final String ref; private Optional loadedAccount; private Optional accountUpdate = Optional.empty(); private Timestamp registeredOn; - private List validationErrors; - public AccountConfig(@Nullable OutgoingEmailValidator emailValidator, Account.Id accountId) { - this.emailValidator = emailValidator; + public AccountConfig(Account.Id accountId) { this.accountId = checkNotNull(accountId); this.ref = RefNames.refsUsers(accountId); } @@ -182,11 +173,6 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. String preferredEmail = get(cfg, KEY_PREFERRED_EMAIL); account.setPreferredEmail(preferredEmail); - if (emailValidator != null && !emailValidator.isValid(preferredEmail)) { - error( - new ValidationError( - ACCOUNT_CONFIG, String.format("Invalid preferred email: %s", preferredEmail))); - } account.setStatus(get(cfg, KEY_STATUS)); account.setMetaId(metaId); @@ -296,24 +282,4 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. private void checkLoaded() { checkState(loadedAccount != null, "Account %s not loaded yet", accountId.get()); } - - /** - * Get the validation errors, if any were discovered during load. - * - * @return list of errors; empty list if there are no errors. - */ - public List getValidationErrors() { - if (validationErrors != null) { - return ImmutableList.copyOf(validationErrors); - } - return ImmutableList.of(); - } - - @Override - public void error(ValidationError error) { - if (validationErrors == null) { - validationErrors = new ArrayList<>(4); - } - validationErrors.add(error); - } } diff --git a/java/com/google/gerrit/server/account/Accounts.java b/java/com/google/gerrit/server/account/Accounts.java index 7b04610c5a..5292a2f8ec 100644 --- a/java/com/google/gerrit/server/account/Accounts.java +++ b/java/com/google/gerrit/server/account/Accounts.java @@ -23,7 +23,6 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -46,16 +45,11 @@ public class Accounts { private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; - private final OutgoingEmailValidator emailValidator; @Inject - Accounts( - GitRepositoryManager repoManager, - AllUsersName allUsersName, - OutgoingEmailValidator emailValidator) { + Accounts(GitRepositoryManager repoManager, AllUsersName allUsersName) { this.repoManager = repoManager; this.allUsersName = allUsersName; - this.emailValidator = emailValidator; } @Nullable @@ -138,7 +132,7 @@ public class Accounts { private Optional read(Repository allUsersRepository, Account.Id accountId) throws IOException, ConfigInvalidException { - AccountConfig accountConfig = new AccountConfig(emailValidator, accountId); + AccountConfig accountConfig = new AccountConfig(accountId); accountConfig.load(allUsersRepository); return accountConfig.getLoadedAccount(); } diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java index 1e033ee1e1..1b9f30cbd1 100644 --- a/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/java/com/google/gerrit/server/account/AccountsUpdate.java @@ -33,7 +33,6 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.index.change.ReindexAfterRefUpdate; -import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.gerrit.server.update.RefUpdateUtil; import com.google.gerrit.server.update.RetryHelper; import com.google.gerrit.server.update.RetryHelper.ActionType; @@ -113,7 +112,6 @@ public class AccountsUpdate { private final GitRepositoryManager repoManager; private final GitReferenceUpdated gitRefUpdated; private final AllUsersName allUsersName; - private final OutgoingEmailValidator emailValidator; private final Provider serverIdentProvider; private final Provider metaDataUpdateInternalFactory; private final RetryHelper retryHelper; @@ -124,7 +122,6 @@ public class AccountsUpdate { GitRepositoryManager repoManager, GitReferenceUpdated gitRefUpdated, AllUsersName allUsersName, - OutgoingEmailValidator emailValidator, @GerritPersonIdent Provider serverIdentProvider, Provider metaDataUpdateInternalFactory, RetryHelper retryHelper, @@ -132,7 +129,6 @@ public class AccountsUpdate { this.repoManager = repoManager; this.gitRefUpdated = gitRefUpdated; this.allUsersName = allUsersName; - this.emailValidator = emailValidator; this.serverIdentProvider = serverIdentProvider; this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory; this.retryHelper = retryHelper; @@ -146,7 +142,6 @@ public class AccountsUpdate { gitRefUpdated, null, allUsersName, - emailValidator, metaDataUpdateInternalFactory, retryHelper, extIdNotesFactory, @@ -169,7 +164,6 @@ public class AccountsUpdate { private final GitRepositoryManager repoManager; private final GitReferenceUpdated gitRefUpdated; private final AllUsersName allUsersName; - private final OutgoingEmailValidator emailValidator; private final Provider serverIdentProvider; private final Provider metaDataUpdateInternalFactory; private final RetryHelper retryHelper; @@ -180,7 +174,6 @@ public class AccountsUpdate { GitRepositoryManager repoManager, GitReferenceUpdated gitRefUpdated, AllUsersName allUsersName, - OutgoingEmailValidator emailValidator, @GerritPersonIdent Provider serverIdentProvider, Provider metaDataUpdateInternalFactory, RetryHelper retryHelper, @@ -188,7 +181,6 @@ public class AccountsUpdate { this.repoManager = repoManager; this.gitRefUpdated = gitRefUpdated; this.allUsersName = allUsersName; - this.emailValidator = emailValidator; this.serverIdentProvider = serverIdentProvider; this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory; this.retryHelper = retryHelper; @@ -202,7 +194,6 @@ public class AccountsUpdate { gitRefUpdated, null, allUsersName, - emailValidator, metaDataUpdateInternalFactory, retryHelper, extIdNotesFactory, @@ -222,7 +213,6 @@ public class AccountsUpdate { private final GitRepositoryManager repoManager; private final GitReferenceUpdated gitRefUpdated; private final AllUsersName allUsersName; - private final OutgoingEmailValidator emailValidator; private final Provider serverIdentProvider; private final Provider identifiedUser; private final Provider metaDataUpdateInternalFactory; @@ -234,7 +224,6 @@ public class AccountsUpdate { GitRepositoryManager repoManager, GitReferenceUpdated gitRefUpdated, AllUsersName allUsersName, - OutgoingEmailValidator emailValidator, @GerritPersonIdent Provider serverIdentProvider, Provider identifiedUser, Provider metaDataUpdateInternalFactory, @@ -244,7 +233,6 @@ public class AccountsUpdate { this.gitRefUpdated = gitRefUpdated; this.allUsersName = allUsersName; this.serverIdentProvider = serverIdentProvider; - this.emailValidator = emailValidator; this.identifiedUser = identifiedUser; this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory; this.retryHelper = retryHelper; @@ -260,7 +248,6 @@ public class AccountsUpdate { gitRefUpdated, user, allUsersName, - emailValidator, metaDataUpdateInternalFactory, retryHelper, extIdNotesFactory, @@ -277,7 +264,6 @@ public class AccountsUpdate { private final GitReferenceUpdated gitRefUpdated; @Nullable private final IdentifiedUser currentUser; private final AllUsersName allUsersName; - private final OutgoingEmailValidator emailValidator; private final Provider metaDataUpdateInternalFactory; private final RetryHelper retryHelper; private final ExternalIdNotesLoader extIdNotesLoader; @@ -290,7 +276,6 @@ public class AccountsUpdate { GitReferenceUpdated gitRefUpdated, @Nullable IdentifiedUser currentUser, AllUsersName allUsersName, - OutgoingEmailValidator emailValidator, Provider metaDataUpdateInternalFactory, RetryHelper retryHelper, ExternalIdNotesLoader extIdNotesLoader, @@ -301,7 +286,6 @@ public class AccountsUpdate { gitRefUpdated, currentUser, allUsersName, - emailValidator, metaDataUpdateInternalFactory, retryHelper, extIdNotesLoader, @@ -316,7 +300,6 @@ public class AccountsUpdate { GitReferenceUpdated gitRefUpdated, @Nullable IdentifiedUser currentUser, AllUsersName allUsersName, - OutgoingEmailValidator emailValidator, Provider metaDataUpdateInternalFactory, RetryHelper retryHelper, ExternalIdNotesLoader extIdNotesLoader, @@ -327,7 +310,6 @@ public class AccountsUpdate { this.gitRefUpdated = checkNotNull(gitRefUpdated, "gitRefUpdated"); this.currentUser = currentUser; this.allUsersName = checkNotNull(allUsersName, "allUsersName"); - this.emailValidator = checkNotNull(emailValidator, "emailValidator"); this.metaDataUpdateInternalFactory = checkNotNull(metaDataUpdateInternalFactory, "metaDataUpdateInternalFactory"); this.retryHelper = checkNotNull(retryHelper, "retryHelper"); @@ -442,7 +424,7 @@ public class AccountsUpdate { private AccountConfig read(Repository allUsersRepo, Account.Id accountId) throws IOException, ConfigInvalidException { - AccountConfig accountConfig = new AccountConfig(emailValidator, accountId); + AccountConfig accountConfig = new AccountConfig(accountId); accountConfig.load(allUsersRepo); afterReadRevision.run(); diff --git a/java/com/google/gerrit/server/git/validators/AccountValidator.java b/java/com/google/gerrit/server/git/validators/AccountValidator.java index 0f894134a2..282e215322 100644 --- a/java/com/google/gerrit/server/git/validators/AccountValidator.java +++ b/java/com/google/gerrit/server/git/validators/AccountValidator.java @@ -90,7 +90,7 @@ public class AccountValidator { private Optional loadAccount(Account.Id accountId, RevWalk rw, ObjectId commit) throws IOException, ConfigInvalidException { rw.reset(); - AccountConfig accountConfig = new AccountConfig(null, accountId); + AccountConfig accountConfig = new AccountConfig(accountId); accountConfig.load(rw, commit); return accountConfig.getLoadedAccount(); } diff --git a/java/com/google/gerrit/server/schema/Schema_154.java b/java/com/google/gerrit/server/schema/Schema_154.java index 8e05d38506..a89f520102 100644 --- a/java/com/google/gerrit/server/schema/Schema_154.java +++ b/java/com/google/gerrit/server/schema/Schema_154.java @@ -139,7 +139,7 @@ public class Schema_154 extends SchemaVersion { PersonIdent ident = serverIdent.get(); md.getCommitBuilder().setAuthor(ident); md.getCommitBuilder().setCommitter(ident); - AccountConfig accountConfig = new AccountConfig(null, account.getId()); + AccountConfig accountConfig = new AccountConfig(account.getId()); accountConfig.load(allUsersRepo); accountConfig.setAccount(account); accountConfig.commit(md); diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 4b34ade644..4217317878 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -104,7 +104,6 @@ import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.index.account.AccountIndexer; import com.google.gerrit.server.index.account.StalenessChecker; import com.google.gerrit.server.mail.Address; -import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl; import com.google.gerrit.server.project.RefPattern; import com.google.gerrit.server.query.account.InternalAccountQuery; @@ -191,8 +190,6 @@ public class AccountIT extends AbstractDaemonTest { @Inject private AccountIndexer accountIndexer; - @Inject private OutgoingEmailValidator emailValidator; - @Inject private GitReferenceUpdated gitReferenceUpdated; @Inject private RetryHelper.Metrics retryMetrics; @@ -2013,7 +2010,6 @@ public class AccountIT extends AbstractDaemonTest { gitReferenceUpdated, null, allUsers, - emailValidator, metaDataUpdateInternalFactory, new RetryHelper( cfg, @@ -2062,7 +2058,6 @@ public class AccountIT extends AbstractDaemonTest { gitReferenceUpdated, null, allUsers, - emailValidator, metaDataUpdateInternalFactory, new RetryHelper( cfg, diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java index 77c139e60d..42073930db 100644 --- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java +++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java @@ -416,7 +416,7 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { PersonIdent ident = serverIdent.get(); md.getCommitBuilder().setAuthor(ident); md.getCommitBuilder().setCommitter(ident); - AccountConfig accountConfig = new AccountConfig(null, accountId); + AccountConfig accountConfig = new AccountConfig(accountId); accountConfig.load(repo); accountConfig.setAccountUpdate(InternalAccountUpdate.builder().setFullName(newName).build()); accountConfig.commit(md);