From ed7c347ddb262ce9faa7edd879b057005ea22045 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 24 Jul 2017 15:37:56 +0200 Subject: [PATCH] Allow to push updates of account.config file Pushing updates of account.config files was disabled while accounts were stored in both ReviewDb and NoteDb to prevent that the accounts go out of sync between ReviewDb and NoteDb. Now since accounts only live in NoteDb we can allow pushing updates of account.config files. However pushing updates to an account.config file is still rejected if: - it cannot be parsed - the preferred email is invalid - the own account is deactivated The same checks are done on submit if updates of account.config files have been pushed for review. Pushing an account.config file with an invalid preferred email is not rejected if the preferred email didn't change. This means if an account already had an invalid preferred email updates of account.config can still be pushed without being forced to fix the preferred email. However if the preferred email is changed it is verified that is valid. When validating the account.config file on submit we don't have the old file version available and hence we can't check if the preferred email has changed. This is why in this case we always enforce the preferred email to be valid. Change-Id: Ie08e50ea92fa7101c5cfcdca78632e8fd52cb26b Signed-off-by: Edwin Kempin --- .../acceptance/api/accounts/AccountIT.java | 395 +++++++++++++++++- .../server/config/GerritGlobalModule.java | 4 +- .../git/validators/AccountValidator.java | 91 ++++ .../git/validators/CommitValidators.java | 49 +-- .../git/validators/MergeValidators.java | 44 +- 5 files changed, 527 insertions(+), 56 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/git/validators/AccountValidator.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 85908c984d..b7d368a9f9 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -77,6 +77,7 @@ import com.google.gerrit.gpg.Fingerprint; import com.google.gerrit.gpg.PublicKeyStore; import com.google.gerrit.gpg.testutil.TestKey; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.account.AccountConfig; @@ -812,14 +813,14 @@ public class AccountIT extends AbstractDaemonTest { } @Test - public void pushAccountConfigToUserBranchForReviewIsRejectedOnSubmit() throws Exception { - String userRefName = RefNames.refsUsers(admin.id); + public void pushAccountConfigToUserBranchForReviewAndSubmit() throws Exception { + String userRef = RefNames.refsUsers(admin.id); TestRepository allUsersRepo = cloneProject(allUsers); - fetch(allUsersRepo, userRefName + ":userRef"); + fetch(allUsersRepo, userRef + ":userRef"); allUsersRepo.reset("userRef"); Config ac = getAccountConfig(allUsersRepo); - ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_STATUS, "OOO"); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_STATUS, "out-of-office"); PushOneCommit.Result r = pushFactory @@ -830,17 +831,203 @@ public class AccountIT extends AbstractDaemonTest { "Update account config", AccountConfig.ACCOUNT_CONFIG, ac.toText()) - .to(MagicBranch.NEW_CHANGE + userRefName); + .to(MagicBranch.NEW_CHANGE + userRef); r.assertOkStatus(); accountIndexedCounter.assertNoReindex(); - assertThat(r.getChange().change().getDest().get()).isEqualTo(userRefName); + assertThat(r.getChange().change().getDest().get()).isEqualTo(userRef); + + gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve()); + gApi.changes().id(r.getChangeId()).current().submit(); + accountIndexedCounter.assertReindexOf(admin); + + AccountInfo info = gApi.accounts().self().get(); + assertThat(info.email).isEqualTo(admin.email); + assertThat(info.name).isEqualTo(admin.fullName); + assertThat(info.status).isEqualTo("out-of-office"); + } + + @Test + public void pushAccountConfigWithPrefEmailThatDoesNotExistAsExtIdToUserBranchForReviewAndSubmit() + throws Exception { + TestAccount foo = accountCreator.create(name("foo")); + String userRef = RefNames.refsUsers(foo.id); + accountIndexedCounter.clear(); + + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRef + ":userRef"); + allUsersRepo.reset("userRef"); + + String email = "some.email@example.com"; + Config ac = getAccountConfig(allUsersRepo); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_PREFERRED_EMAIL, email); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(MagicBranch.NEW_CHANGE + userRef); + r.assertOkStatus(); + accountIndexedCounter.assertNoReindex(); + assertThat(r.getChange().change().getDest().get()).isEqualTo(userRef); + + setApiUser(foo); + gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve()); + gApi.changes().id(r.getChangeId()).current().submit(); + + accountIndexedCounter.assertReindexOf(foo); + + AccountInfo info = gApi.accounts().self().get(); + assertThat(info.email).isEqualTo(email); + assertThat(info.name).isEqualTo(foo.fullName); + } + + @Test + public void pushAccountConfigToUserBranchForReviewIsRejectedOnSubmitIfConfigIsInvalid() + throws Exception { + String userRef = RefNames.refsUsers(admin.id); + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRef + ":userRef"); + allUsersRepo.reset("userRef"); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + "invalid config") + .to(MagicBranch.NEW_CHANGE + userRef); + r.assertOkStatus(); + accountIndexedCounter.assertNoReindex(); + assertThat(r.getChange().change().getDest().get()).isEqualTo(userRef); + gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve()); exception.expect(ResourceConflictException.class); exception.expectMessage( - String.format("update of %s not allowed", AccountConfig.ACCOUNT_CONFIG)); + String.format( + "invalid account configuration: commit '%s' has an invalid '%s' file for account '%s':" + + " Invalid config file %s in commit %s", + r.getCommit().name(), + AccountConfig.ACCOUNT_CONFIG, + admin.id, + AccountConfig.ACCOUNT_CONFIG, + r.getCommit().name())); gApi.changes().id(r.getChangeId()).current().submit(); } + @Test + public void pushAccountConfigToUserBranchForReviewIsRejectedOnSubmitIfPreferredEmailIsInvalid() + throws Exception { + String userRef = RefNames.refsUsers(admin.id); + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRef + ":userRef"); + allUsersRepo.reset("userRef"); + + String noEmail = "no.email"; + Config ac = getAccountConfig(allUsersRepo); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_PREFERRED_EMAIL, noEmail); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(MagicBranch.NEW_CHANGE + userRef); + r.assertOkStatus(); + accountIndexedCounter.assertNoReindex(); + assertThat(r.getChange().change().getDest().get()).isEqualTo(userRef); + + gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve()); + exception.expect(ResourceConflictException.class); + exception.expectMessage( + String.format( + "invalid account configuration: invalid preferred email '%s' for account '%s'", + noEmail, admin.id)); + gApi.changes().id(r.getChangeId()).current().submit(); + } + + @Test + public void pushAccountConfigToUserBranchForReviewIsRejectedOnSubmitIfOwnAccountIsDeactivated() + throws Exception { + String userRef = RefNames.refsUsers(admin.id); + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRef + ":userRef"); + allUsersRepo.reset("userRef"); + + Config ac = getAccountConfig(allUsersRepo); + ac.setBoolean(AccountConfig.ACCOUNT, null, AccountConfig.KEY_ACTIVE, false); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(MagicBranch.NEW_CHANGE + userRef); + r.assertOkStatus(); + accountIndexedCounter.assertNoReindex(); + assertThat(r.getChange().change().getDest().get()).isEqualTo(userRef); + + gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve()); + exception.expect(ResourceConflictException.class); + exception.expectMessage("invalid account configuration: cannot deactivate own account"); + gApi.changes().id(r.getChangeId()).current().submit(); + } + + @Test + public void pushAccountConfigToUserBranchForReviewDeactivateOtherAccount() throws Exception { + TestAccount foo = accountCreator.create(name("foo")); + assertThat(gApi.accounts().id(foo.id.get()).getActive()).isTrue(); + String userRef = RefNames.refsUsers(foo.id); + accountIndexedCounter.clear(); + + AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators")); + grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID()); + grantLabel("Code-Review", -2, 2, allUsers, userRef, false, adminGroup.getGroupUUID(), false); + grant(allUsers, userRef, Permission.SUBMIT, false, adminGroup.getGroupUUID()); + + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRef + ":userRef"); + allUsersRepo.reset("userRef"); + + Config ac = getAccountConfig(allUsersRepo); + ac.setBoolean(AccountConfig.ACCOUNT, null, AccountConfig.KEY_ACTIVE, false); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(MagicBranch.NEW_CHANGE + userRef); + r.assertOkStatus(); + accountIndexedCounter.assertNoReindex(); + assertThat(r.getChange().change().getDest().get()).isEqualTo(userRef); + + gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve()); + gApi.changes().id(r.getChangeId()).current().submit(); + accountIndexedCounter.assertReindexOf(foo); + + assertThat(gApi.accounts().id(foo.id.get()).getActive()).isFalse(); + } + @Test public void pushWatchConfigToUserBranch() throws Exception { TestRepository allUsersRepo = cloneProject(allUsers); @@ -883,13 +1070,70 @@ public class AccountIT extends AbstractDaemonTest { } @Test - public void pushAccountConfigToUserBranchIsRejected() throws Exception { + public void pushAccountConfigToUserBranch() throws Exception { TestRepository allUsersRepo = cloneProject(allUsers); fetch(allUsersRepo, RefNames.refsUsers(admin.id) + ":userRef"); allUsersRepo.reset("userRef"); Config ac = getAccountConfig(allUsersRepo); - ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_STATUS, "OOO"); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_STATUS, "out-of-office"); + + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(RefNames.REFS_USERS_SELF) + .assertOkStatus(); + accountIndexedCounter.assertReindexOf(admin); + + AccountInfo info = gApi.accounts().self().get(); + assertThat(info.email).isEqualTo(admin.email); + assertThat(info.name).isEqualTo(admin.fullName); + assertThat(info.status).isEqualTo("out-of-office"); + } + + @Test + public void pushAccountConfigToUserBranchIsRejectedIfConfigIsInvalid() throws Exception { + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, RefNames.refsUsers(admin.id) + ":userRef"); + allUsersRepo.reset("userRef"); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + "invalid config") + .to(RefNames.REFS_USERS_SELF); + r.assertErrorStatus("invalid account configuration"); + r.assertMessage( + String.format( + "commit '%s' has an invalid '%s' file for account '%s':" + + " Invalid config file %s in commit %s", + r.getCommit().name(), + AccountConfig.ACCOUNT_CONFIG, + admin.id, + AccountConfig.ACCOUNT_CONFIG, + r.getCommit().name())); + accountIndexedCounter.assertNoReindex(); + } + + @Test + public void pushAccountConfigToUserBranchIsRejectedIfPreferredEmailIsInvalid() throws Exception { + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, RefNames.refsUsers(admin.id) + ":userRef"); + allUsersRepo.reset("userRef"); + + String noEmail = "no.email"; + Config ac = getAccountConfig(allUsersRepo); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_PREFERRED_EMAIL, noEmail); PushOneCommit.Result r = pushFactory @@ -901,7 +1145,138 @@ public class AccountIT extends AbstractDaemonTest { AccountConfig.ACCOUNT_CONFIG, ac.toText()) .to(RefNames.REFS_USERS_SELF); - r.assertErrorStatus("account update not allowed"); + r.assertErrorStatus("invalid account configuration"); + r.assertMessage( + String.format("invalid preferred email '%s' for account '%s'", noEmail, admin.id)); + accountIndexedCounter.assertNoReindex(); + } + + @Test + public void pushAccountConfigToUserBranchInvalidPreferredEmailButNotChanged() throws Exception { + TestAccount foo = accountCreator.create(name("foo")); + String userRef = RefNames.refsUsers(foo.id); + + String noEmail = "no.email"; + accountsUpdate.create().update(foo.id, a -> a.setPreferredEmail(noEmail)); + accountIndexedCounter.clear(); + + AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators")); + grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID()); + + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRef + ":userRef"); + allUsersRepo.reset("userRef"); + + String status = "in vacation"; + Config ac = getAccountConfig(allUsersRepo); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_STATUS, status); + + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(userRef) + .assertOkStatus(); + accountIndexedCounter.assertReindexOf(foo); + + AccountInfo info = gApi.accounts().id(foo.id.get()).get(); + assertThat(info.email).isEqualTo(noEmail); + assertThat(info.name).isEqualTo(foo.fullName); + assertThat(info.status).isEqualTo(status); + } + + @Test + public void pushAccountConfigToUserBranchIfPreferredEmailDoesNotExistAsExtId() throws Exception { + TestAccount foo = accountCreator.create(name("foo")); + String userRef = RefNames.refsUsers(foo.id); + accountIndexedCounter.clear(); + + AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators")); + grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID()); + + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRef + ":userRef"); + allUsersRepo.reset("userRef"); + + String email = "some.email@example.com"; + Config ac = getAccountConfig(allUsersRepo); + ac.setString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_PREFERRED_EMAIL, email); + + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(userRef) + .assertOkStatus(); + accountIndexedCounter.assertReindexOf(foo); + + AccountInfo info = gApi.accounts().id(foo.id.get()).get(); + assertThat(info.email).isEqualTo(email); + assertThat(info.name).isEqualTo(foo.fullName); + } + + @Test + public void pushAccountConfigToUserBranchIsRejectedIfOwnAccountIsDeactivated() throws Exception { + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, RefNames.refsUsers(admin.id) + ":userRef"); + allUsersRepo.reset("userRef"); + + Config ac = getAccountConfig(allUsersRepo); + ac.setBoolean(AccountConfig.ACCOUNT, null, AccountConfig.KEY_ACTIVE, false); + + PushOneCommit.Result r = + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(RefNames.REFS_USERS_SELF); + r.assertErrorStatus("invalid account configuration"); + r.assertMessage("cannot deactivate own account"); + accountIndexedCounter.assertNoReindex(); + } + + @Test + public void pushAccountConfigToUserBranchDeactivateOtherAccount() throws Exception { + TestAccount foo = accountCreator.create(name("foo")); + assertThat(gApi.accounts().id(foo.id.get()).getActive()).isTrue(); + String userRef = RefNames.refsUsers(foo.id); + accountIndexedCounter.clear(); + + AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators")); + grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID()); + + TestRepository allUsersRepo = cloneProject(allUsers); + fetch(allUsersRepo, userRef + ":userRef"); + allUsersRepo.reset("userRef"); + + Config ac = getAccountConfig(allUsersRepo); + ac.setBoolean(AccountConfig.ACCOUNT, null, AccountConfig.KEY_ACTIVE, false); + + pushFactory + .create( + db, + admin.getIdent(), + allUsersRepo, + "Update account config", + AccountConfig.ACCOUNT_CONFIG, + ac.toText()) + .to(userRef) + .assertOkStatus(); + accountIndexedCounter.assertReindexOf(foo); + + assertThat(gApi.accounts().id(foo.id.get()).getActive()).isFalse(); } @Test diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index e58ab74917..6be83b39bb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -127,7 +127,7 @@ import com.google.gerrit.server.git.strategy.SubmitStrategy; import com.google.gerrit.server.git.validators.CommitValidationListener; import com.google.gerrit.server.git.validators.MergeValidationListener; import com.google.gerrit.server.git.validators.MergeValidators; -import com.google.gerrit.server.git.validators.MergeValidators.AccountValidator; +import com.google.gerrit.server.git.validators.MergeValidators.AccountMergeValidator; import com.google.gerrit.server.git.validators.MergeValidators.ProjectConfigValidator; import com.google.gerrit.server.git.validators.OnSubmitValidationListener; import com.google.gerrit.server.git.validators.OnSubmitValidators; @@ -393,7 +393,7 @@ public class GerritGlobalModule extends FactoryModule { bind(AnonymousUser.class); factory(AbandonOp.Factory.class); - factory(AccountValidator.Factory.class); + factory(AccountMergeValidator.Factory.class); factory(RefOperationValidators.Factory.class); factory(OnSubmitValidators.Factory.class); factory(MergeValidators.Factory.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/AccountValidator.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/AccountValidator.java new file mode 100644 index 0000000000..24a50cc9cb --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/AccountValidator.java @@ -0,0 +1,91 @@ +// Copyright (C) 2017 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.git.validators; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.AccountConfig; +import com.google.gerrit.server.mail.send.OutgoingEmailValidator; +import com.google.inject.Provider; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import javax.inject.Inject; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.revwalk.RevWalk; + +public class AccountValidator { + + private final Provider self; + private final OutgoingEmailValidator emailValidator; + + @Inject + public AccountValidator(Provider self, OutgoingEmailValidator emailValidator) { + this.self = self; + this.emailValidator = emailValidator; + } + + public List validate( + Account.Id accountId, RevWalk rw, @Nullable ObjectId oldId, ObjectId newId) + throws IOException { + Account oldAccount = null; + if (oldId != null && !ObjectId.zeroId().equals(oldId)) { + try { + oldAccount = loadAccount(accountId, rw, oldId); + } catch (ConfigInvalidException e) { + // ignore, maybe the new commit is repairing it now + } + } + + Account newAccount; + try { + newAccount = loadAccount(accountId, rw, newId); + } catch (ConfigInvalidException e) { + return ImmutableList.of( + String.format( + "commit '%s' has an invalid '%s' file for account '%s': %s", + newId.name(), AccountConfig.ACCOUNT_CONFIG, accountId.get(), e.getMessage())); + } + + List messages = new ArrayList<>(); + if (accountId.equals(self.get().getAccountId()) && !newAccount.isActive()) { + messages.add("cannot deactivate own account"); + } + + if (newAccount.getPreferredEmail() != null + && (oldAccount == null + || !newAccount.getPreferredEmail().equals(oldAccount.getPreferredEmail()))) { + if (!emailValidator.isValid(newAccount.getPreferredEmail())) { + messages.add( + String.format( + "invalid preferred email '%s' for account '%s'", + newAccount.getPreferredEmail(), accountId.get())); + } + } + + return ImmutableList.copyOf(messages); + } + + private Account loadAccount(Account.Id accountId, RevWalk rw, ObjectId commit) + throws IOException, ConfigInvalidException { + rw.reset(); + AccountConfig accountConfig = new AccountConfig(null, accountId); + accountConfig.load(rw, commit); + return accountConfig.getAccount(); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index 41381e8024..b38440508b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -32,7 +32,6 @@ import com.google.gerrit.reviewdb.client.Branch; 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.AccountConfig; import com.google.gerrit.server.account.WatchConfig; import com.google.gerrit.server.account.externalids.ExternalIdsConsistencyChecker; import com.google.gerrit.server.config.AllUsersName; @@ -58,11 +57,9 @@ import java.net.URL; import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Objects; import java.util.regex.Pattern; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.notes.NoteMap; @@ -70,7 +67,6 @@ import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.FooterLine; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; -import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.util.SystemReader; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -88,6 +84,7 @@ public class CommitValidators { private final DynamicSet pluginValidators; private final AllUsersName allUsers; private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker; + private final AccountValidator accountValidator; private final String installCommitMsgHookCommand; private final ProjectCache projectCache; @@ -99,12 +96,14 @@ public class CommitValidators { DynamicSet pluginValidators, AllUsersName allUsers, ExternalIdsConsistencyChecker externalIdsConsistencyChecker, + AccountValidator accountValidator, ProjectCache projectCache) { this.gerritIdent = gerritIdent; this.canonicalWebUrl = canonicalWebUrl; this.pluginValidators = pluginValidators; this.allUsers = allUsers; this.externalIdsConsistencyChecker = externalIdsConsistencyChecker; + this.accountValidator = accountValidator; this.installCommitMsgHookCommand = cfg != null ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null; this.projectCache = projectCache; @@ -133,7 +132,7 @@ public class CommitValidators { new BannedCommitsValidator(rejectCommits), new PluginCommitValidationListener(pluginValidators), new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker), - new AccountValidator(allUsers))); + new AccountCommitValidator(allUsers, accountValidator))); } public CommitValidators forGerritCommits( @@ -158,7 +157,7 @@ public class CommitValidators { new ConfigValidator(branch, user, rw, allUsers), new PluginCommitValidationListener(pluginValidators), new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker), - new AccountValidator(allUsers))); + new AccountCommitValidator(allUsers, accountValidator))); } public CommitValidators forMergedCommits(PermissionBackend.ForRef perm, IdentifiedUser user) { @@ -709,11 +708,13 @@ public class CommitValidators { } /** Rejects updates to 'account.config' in user branches. */ - public static class AccountValidator implements CommitValidationListener { + public static class AccountCommitValidator implements CommitValidationListener { private final AllUsersName allUsers; + private final AccountValidator accountValidator; - public AccountValidator(AllUsersName allUsers) { + public AccountCommitValidator(AllUsersName allUsers, AccountValidator accountValidator) { this.allUsers = allUsers; + this.accountValidator = accountValidator; } @Override @@ -725,7 +726,7 @@ public class CommitValidators { if (receiveEvent.command.getRefName().startsWith(MagicBranch.NEW_CHANGE)) { // no validation on push for review, will be checked on submit by - // MergeValidators.AccountValidator + // MergeValidators.AccountMergeValidator return Collections.emptyList(); } @@ -735,15 +736,19 @@ public class CommitValidators { } try { - ObjectId newBlobId = getAccountConfigBlobId(receiveEvent.revWalk, receiveEvent.commit); - - ObjectId oldId = receiveEvent.command.getOldId(); - ObjectId oldBlobId = - !ObjectId.zeroId().equals(oldId) - ? getAccountConfigBlobId(receiveEvent.revWalk, oldId) - : null; - if (!Objects.equals(oldBlobId, newBlobId)) { - throw new CommitValidationException("account update not allowed"); + List errorMessages = + accountValidator.validate( + accountId, + receiveEvent.revWalk, + receiveEvent.command.getOldId(), + receiveEvent.commit); + if (!errorMessages.isEmpty()) { + throw new CommitValidationException( + "invalid account configuration", + errorMessages + .stream() + .map(m -> new CommitValidationMessage(m, true)) + .collect(toList())); } } catch (IOException e) { String m = String.format("Validating update for account %s failed", accountId.get()); @@ -752,14 +757,6 @@ public class CommitValidators { } return Collections.emptyList(); } - - private ObjectId getAccountConfigBlobId(RevWalk rw, ObjectId id) throws IOException { - RevCommit commit = rw.parseCommit(id); - try (TreeWalk tw = - TreeWalk.forPath(rw.getObjectReader(), AccountConfig.ACCOUNT_CONFIG, commit.getTree())) { - return tw != null ? tw.getObjectId(0) : null; - } - } } private static CommitValidationMessage invalidEmail( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java index af9f6d52ea..fd524b4af1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.git.validators; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.gerrit.extensions.api.projects.ProjectConfigEntryType; import com.google.gerrit.extensions.registration.DynamicMap; @@ -47,6 +48,7 @@ import java.io.IOException; import java.util.List; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevWalk; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -55,7 +57,7 @@ public class MergeValidators { private final DynamicSet mergeValidationListeners; private final ProjectConfigValidator.Factory projectConfigValidatorFactory; - private final AccountValidator.Factory accountValidatorFactory; + private final AccountMergeValidator.Factory accountValidatorFactory; public interface Factory { MergeValidators create(); @@ -65,7 +67,7 @@ public class MergeValidators { MergeValidators( DynamicSet mergeValidationListeners, ProjectConfigValidator.Factory projectConfigValidatorFactory, - AccountValidator.Factory accountValidatorFactory) { + AccountMergeValidator.Factory accountValidatorFactory) { this.mergeValidationListeners = mergeValidationListeners; this.projectConfigValidatorFactory = projectConfigValidatorFactory; this.accountValidatorFactory = accountValidatorFactory; @@ -222,23 +224,26 @@ public class MergeValidators { } } - public static class AccountValidator implements MergeValidationListener { + public static class AccountMergeValidator implements MergeValidationListener { public interface Factory { - AccountValidator create(); + AccountMergeValidator create(); } private final Provider dbProvider; private final AllUsersName allUsersName; private final ChangeData.Factory changeDataFactory; + private final AccountValidator accountValidator; @Inject - public AccountValidator( + public AccountMergeValidator( Provider dbProvider, AllUsersName allUsersName, - ChangeData.Factory changeDataFactory) { + ChangeData.Factory changeDataFactory, + AccountValidator accountValidator) { this.dbProvider = dbProvider; this.allUsersName = allUsersName; this.changeDataFactory = changeDataFactory; + this.accountValidator = accountValidator; } @Override @@ -250,30 +255,33 @@ public class MergeValidators { PatchSet.Id patchSetId, IdentifiedUser caller) throws MergeValidationException { - if (!allUsersName.equals(destProject.getProject().getNameKey()) - || Account.Id.fromRef(destBranch.get()) == null) { + Account.Id accountId = Account.Id.fromRef(destBranch.get()); + if (!allUsersName.equals(destProject.getProject().getNameKey()) || accountId == null) { return; } - if (commit.getParentCount() > 1) { - // for merge commits we cannot ensure that the 'account.config' file is not modified, since - // for merge commits file modifications that come in through the merge don't appear in the - // file list that is returned by ChangeData#currentFilePaths() - throw new MergeValidationException("cannot submit merge commit to user branch"); - } - ChangeData cd = changeDataFactory.create( dbProvider.get(), destProject.getProject().getNameKey(), patchSetId.getParentKey()); try { - if (cd.currentFilePaths().contains(AccountConfig.ACCOUNT_CONFIG)) { - throw new MergeValidationException( - String.format("update of %s not allowed", AccountConfig.ACCOUNT_CONFIG)); + if (!cd.currentFilePaths().contains(AccountConfig.ACCOUNT_CONFIG)) { + return; } } catch (OrmException e) { log.error("Cannot validate account update", e); throw new MergeValidationException("account validation unavailable"); } + + try (RevWalk rw = new RevWalk(repo)) { + List errorMessages = accountValidator.validate(accountId, rw, null, commit); + if (!errorMessages.isEmpty()) { + throw new MergeValidationException( + "invalid account configuration: " + Joiner.on("; ").join(errorMessages)); + } + } catch (IOException e) { + log.error("Cannot validate account update", e); + throw new MergeValidationException("account validation unavailable"); + } } } }