Cherry-Pick: Do not fail if non-visible users are involved
If a change is cherry-picked, Gerrit automatically adds the change owner
and the reviewers of the cherry-picked change as reviewers on the
cherry-pick change. CCs of the cherry-picked change are automatically
added as CCs on the cherry-pick change.
Same as for revert (see change I8264d96f7) the visibility check for
these explicit reviewers/CCs should be skipped.
So far cherry-pick failed if any of the accounts that are added as
reviewers/CCs on the cherry-pick change are not visible to the caller.
Failing in this case is unnecessary since the user doing the cherry-pick
already knows about the existence of the reviewer/CC accounts (see
below) and hence we can just skip the account visibility check for them
during cherry-pick.
Cherry-picking a change is only possible if the calling user can see the
change that is being cherry-picked. If a user can see the change, they
can also see the change owner and all its reviewers/CCs regardless of
whether these accounts are visible. This means the user doing the
cherry-pick knows that Gerrit accounts exists for all users the are
either change owners, reviewer or CC on the cherry-picked change. This
means we can preserve them as reviewers/CCs on the cherry-pick change,
even if their accounts are not visible to the user doing the cherry-pick
(as it doesn't expose the existence of accounts that the user didn't
already know before).
In addition cherry-pick also implicitly CCs the author and committer if
they are forged. Here the situation is a bit different. It's possible
that there are no matching accounts for the author and committer, hence
from being able to see the author and committer information on the
cherry-picked change one cannot deduce that corresponding Gerrit
accounts exists. Hence we can only CC them on the cherry-pick change if
they are visible to the user doing the cherry-pick, as otherwise the
account existence would be revealed. If the author/committer accounts
are not visible we silently drop CCing them now so that the cherry-pick
can still succeed in this case. We do the same when pushing commits with
forged authors/committers so that doing local cherry-picks can also
succeed if the accounts of the forged authors/committers are not
visible.
Bug: Issue 16274
Bug: Google b/232285749
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I1531d1f95b572b89998a82d503c3a3bb23f8712d
Release-Notes: skip
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 1177734..c5d319f 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -32,6 +32,7 @@
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static com.google.gerrit.server.project.testing.TestLabels.label;
import static com.google.gerrit.server.project.testing.TestLabels.value;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toList;
@@ -89,6 +90,7 @@
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ProjectWatchInfo;
+import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -98,6 +100,7 @@
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.server.GerritPersonIdent;
@@ -1100,6 +1103,42 @@
gApi.changes().id(id).current().review(ReviewInput.recommend());
}
+ protected void assertThatAccountIsNotVisible(TestAccount... testAccounts) {
+ for (TestAccount testAccount : testAccounts) {
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(testAccount.id().get()).get());
+ }
+ }
+
+ protected void assertReviewers(String changeId, TestAccount... expectedReviewers)
+ throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).containsKey(ReviewerState.REVIEWER);
+ List<Integer> reviewers =
+ reviewerMap.get(ReviewerState.REVIEWER).stream().map(a -> a._accountId).collect(toList());
+ assertThat(reviewers)
+ .containsExactlyElementsIn(
+ Arrays.stream(expectedReviewers).map(a -> a.id().get()).collect(toList()));
+ }
+
+ protected void assertCcs(String changeId, TestAccount... expectedCcs) throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).containsKey(ReviewerState.CC);
+ List<Integer> ccs =
+ reviewerMap.get(ReviewerState.CC).stream().map(a -> a._accountId).collect(toList());
+ assertThat(ccs)
+ .containsExactlyElementsIn(
+ Arrays.stream(expectedCcs).map(a -> a.id().get()).collect(toList()));
+ }
+
+ protected void assertNoCcs(String changeId) throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).doesNotContainKey(ReviewerState.CC);
+ }
+
protected void assertSubmittedTogether(String chId, String... expected) throws Exception {
assertSubmittedTogether(chId, ImmutableSet.of(), expected);
}
diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index 299256b..f3ad4f7 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java
@@ -105,7 +105,10 @@
FAIL,
// Only not found failures cause the operation to fail, all other failures are ignored.
- IGNORE_EXCEPT_NOT_FOUND;
+ IGNORE_EXCEPT_NOT_FOUND,
+
+ // All failures are ignored.
+ IGNORE_ALL;
}
private enum FailureType {
@@ -155,7 +158,7 @@
in.reviewer = accountId.toString();
in.state = CC;
in.notify = notify;
- in.otherFailureBehavior = FailureBehavior.IGNORE_EXCEPT_NOT_FOUND;
+ in.otherFailureBehavior = FailureBehavior.IGNORE_ALL;
return Optional.of(in);
}
@@ -595,8 +598,9 @@
(input instanceof InternalReviewerInput)
? ((InternalReviewerInput) input).otherFailureBehavior
: FailureBehavior.FAIL;
- return failureType == FailureType.OTHER
- && behavior == FailureBehavior.IGNORE_EXCEPT_NOT_FOUND;
+ return behavior == FailureBehavior.IGNORE_ALL
+ || (failureType == FailureType.OTHER
+ && behavior == FailureBehavior.IGNORE_EXCEPT_NOT_FOUND);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index 00c48dc..d0113e5 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -527,7 +527,7 @@
reviewers.remove(user.get().getAccountId());
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.get().getAccountId());
- ins.setReviewersAndCcs(reviewers, ccs);
+ ins.setReviewersAndCcsIgnoreVisibility(reviewers, ccs);
}
// If there is a base, and the base is not merged, the groups will be overridden by the base's
// groups.
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
index f3d3420..93ac038 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -50,7 +50,6 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.validators.CommitValidationException;
@@ -60,7 +59,6 @@
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.inject.Inject;
import java.util.ArrayList;
-import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
@@ -1489,36 +1487,6 @@
return result;
}
- private void assertThatAccountIsNotVisible(TestAccount... testAccounts) {
- for (TestAccount testAccount : testAccounts) {
- assertThrows(
- ResourceNotFoundException.class, () -> gApi.accounts().id(testAccount.id().get()).get());
- }
- }
-
- private void assertReviewers(String changeId, TestAccount... expectedReviewers)
- throws RestApiException {
- Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
- gApi.changes().id(changeId).get().reviewers;
- assertThat(reviewerMap).containsKey(ReviewerState.REVIEWER);
- List<Integer> reviewers =
- reviewerMap.get(ReviewerState.REVIEWER).stream().map(a -> a._accountId).collect(toList());
- assertThat(reviewers)
- .containsExactlyElementsIn(
- Arrays.stream(expectedReviewers).map(a -> a.id().get()).collect(toList()));
- }
-
- private void assertCcs(String changeId, TestAccount... expectedCcs) throws RestApiException {
- Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
- gApi.changes().id(changeId).get().reviewers;
- assertThat(reviewerMap).containsKey(ReviewerState.CC);
- List<Integer> ccs =
- reviewerMap.get(ReviewerState.CC).stream().map(a -> a._accountId).collect(toList());
- assertThat(ccs)
- .containsExactlyElementsIn(
- Arrays.stream(expectedCcs).map(a -> a.id().get()).collect(toList()));
- }
-
private void addPureRevertSubmitRule() throws Exception {
modifySubmitRules(
"submit_rule(submit(R)) :- \n"
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 2c80333..8d64cf7 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -1009,6 +1009,84 @@
}
@Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void cherryPickWithNonVisibleUsers() throws Exception {
+ // Create a target branch for the cherry-pick.
+ createBranch(BranchNameKey.create(project, "stable"));
+
+ // Define readable names for the users we use in this test.
+ TestAccount cherryPicker = user;
+ TestAccount changeOwner = admin;
+ TestAccount reviewer = accountCreator.user2();
+ TestAccount cc =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+ TestAccount authorCommitter =
+ accountCreator.create("user4", "user4@example.com", "User4", /* displayName= */ null);
+
+ // Check that the cherry-picker can neither see the changeOwner, the reviewer, the cc nor the
+ // authorCommitter.
+ requestScopeOperations.setApiUser(cherryPicker.id());
+ assertThatAccountIsNotVisible(changeOwner, reviewer, cc, authorCommitter);
+
+ // Create the change with authorCommitter as the author and the committer.
+ requestScopeOperations.setApiUser(changeOwner.id());
+ PushOneCommit push = pushFactory.create(authorCommitter.newIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ // Check that authorCommitter was set as the author and committer.
+ ChangeInfo changeInfo = gApi.changes().id(r.getChangeId()).get();
+ CommitInfo commit = changeInfo.revisions.get(changeInfo.currentRevision).commit;
+ assertThat(commit.author.email).isEqualTo(authorCommitter.email());
+ assertThat(commit.committer.email).isEqualTo(authorCommitter.email());
+
+ // Pushing a commit with a forged author/committer adds the author/committer as a CC.
+ assertCcs(r.getChangeId(), authorCommitter);
+
+ // Remove the author/committer as a CC because because otherwise there are two signals for CCing
+ // authorCommitter on the cherry-pick change: once because they are author and committer and
+ // once because they are a CC. For authorCommitter we only want to test the first signal here
+ // (the second signal is covered by adding an explicit CC below).
+ gApi.changes().id(r.getChangeId()).reviewer(authorCommitter.email()).remove();
+ assertNoCcs(r.getChangeId());
+
+ // Add reviewer and cc.
+ ReviewInput reviewerInput = ReviewInput.approve();
+ reviewerInput.reviewer(reviewer.email());
+ reviewerInput.cc(cc.email());
+ gApi.changes().id(r.getChangeId()).current().review(reviewerInput);
+
+ // Approve and submit the change.
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // Cherry-pick the change.
+ requestScopeOperations.setApiUser(cherryPicker.id());
+ CherryPickInput cherryPickInput = new CherryPickInput();
+ cherryPickInput.message = "Cherry-pick to stable branch";
+ cherryPickInput.destination = "stable";
+ cherryPickInput.keepReviewers = true;
+ String cherryPickChangeId =
+ gApi.changes().id(r.getChangeId()).current().cherryPick(cherryPickInput).get().id;
+
+ // Cherry-pick doesn't check the visibility of explicit reviewers/CCs. Since the cherry-picker
+ // can see the cherry-picked change, they can also see its reviewers/CCs. This means preserving
+ // them on the cherry-pick change doesn't expose their account existence and it's OK to keep
+ // them even if their accounts are not visible to the cherry-picker.
+ // In contrast to this for implicit CCs that are added for the author/committer the account
+ // visibility is checked, but if their accounts are not visible the CC is silently dropped (so
+ // that the cherry-pick request can still succeed). Since in this case authorCommitter is not
+ // visible, we expect that CCing them is being dropped and hence authorCommitter is not returned
+ // as a CC here. The reason that the visibility for author/committer must be checked is that
+ // author/committer may not match a Gerrit account (if they are forged). This means by seeing
+ // the author/committer on the cherry-picked change, it's not possible to deduce that these
+ // Gerrit accounts exists, but if they would be added as a CC on the cherry-pick change even if
+ // they are not visible the account existence would be exposed.
+ assertReviewers(cherryPickChangeId, changeOwner, reviewer);
+ assertCcs(cherryPickChangeId, cc);
+ }
+
+ @Test
public void cherryPickToMergedChangeRevision() throws Exception {
createBranch(BranchNameKey.create(project, "foo"));
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index cd1d911..a27218f 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -128,6 +128,7 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Stream;
import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
@@ -1154,7 +1155,7 @@
.add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
.message(PushOneCommit.SUBJECT)
.create();
- // Push commit as "Admnistrator".
+ // Push commit as "Administrator".
pushHead(testRepo, "refs/for/master");
String changeId = GitUtil.getChangeId(testRepo, c).get();
@@ -1168,6 +1169,53 @@
}
@Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void pushForMasterWithNonVisibleForgedAuthorAndCommitter() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount uploader = user; // cannot use admin since admin can see all users
+ TestAccount author = accountCreator.user2();
+ TestAccount committer =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the uploader can neither see the author nor the committer.
+ requestScopeOperations.setApiUser(uploader.id());
+ assertThatAccountIsNotVisible(author, committer);
+
+ // Allow the uploader to forge author and committer.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.FORGE_AUTHOR).ref("refs/heads/master").group(REGISTERED_USERS))
+ .add(allow(Permission.FORGE_COMMITTER).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ // Clone the repo as uploader so that the push is done by the uplaoder.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project, uploader);
+
+ // Create a commit with different forged author and committer.
+ RevCommit c =
+ testRepo
+ .branch("HEAD")
+ .commit()
+ .insertChangeId()
+ .author(author.newIdent())
+ .committer(committer.newIdent())
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
+ .message(PushOneCommit.SUBJECT)
+ .create();
+
+ PushResult r = pushHead(testRepo, "refs/for/master");
+ RemoteRefUpdate refUpdate = r.getRemoteUpdate("refs/for/master");
+ assertThat(refUpdate.getStatus()).isEqualTo(RemoteRefUpdate.Status.OK);
+
+ String changeId = GitUtil.getChangeId(testRepo, c).get();
+ assertThat(getOwnerEmail(changeId)).isEqualTo(uploader.email());
+
+ // author and committer have not been CCed because their accounts are not visible
+ assertThat(getReviewerEmails(changeId, ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
public void pushNewPatchSetForMasterWithForgedAuthorAndCommitter() throws Exception {
TestAccount user2 = accountCreator.user2();
// First patch set has author and committer matching change owner.
@@ -1192,6 +1240,55 @@
.containsExactly(user.getNameEmail(), user2.getNameEmail());
}
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void pushNewPatchSetForMasterWithNonVisibleForgedAuthorAndCommitter() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount uploader = user; // cannot use admin since admin can see all users
+ TestAccount author = accountCreator.user2();
+ TestAccount committer =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the uploader can neither see the author nor the committer.
+ requestScopeOperations.setApiUser(uploader.id());
+ assertThatAccountIsNotVisible(author, committer);
+
+ // Allow the uploader to forge author and committer.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.FORGE_AUTHOR).ref("refs/heads/master").group(REGISTERED_USERS))
+ .add(allow(Permission.FORGE_COMMITTER).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ // Clone the repo as uploader so that the push is done by the uplaoder.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project, uploader);
+
+ // First patch set has author and committer matching uploader.
+ PushOneCommit push = pushFactory.create(uploader.newIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(uploader.email());
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.REVIEWER)).isEmpty();
+
+ testRepo
+ .amendRef("HEAD")
+ .author(author.newIdent())
+ .committer(committer.newIdent())
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT + "2")
+ .create();
+
+ PushResult r2 = pushHead(testRepo, "refs/for/master");
+ RemoteRefUpdate refUpdate = r2.getRemoteUpdate("refs/for/master");
+ assertThat(refUpdate.getStatus()).isEqualTo(RemoteRefUpdate.Status.OK);
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(uploader.email());
+
+ // author and committer have not been CCed because their accounts are not visible
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.CC)).isEmpty();
+ }
+
/**
* There was a bug that allowed a user with Forge Committer Identity access right to upload a
* commit and put *votes on behalf of another user* on it. This test checks that this is not
