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"); + } } } }