Merge "Revert: Do not check the account visibility of reviewers/CCs"
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
index 11999ab..3f51872 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
@@ -14,6 +14,7 @@
package com.google.gerrit.extensions.api.changes;
+import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
import com.google.gerrit.extensions.client.Comment;
@@ -154,7 +155,11 @@
}
public ReviewInput reviewer(String reviewer) {
- return reviewer(reviewer, REVIEWER, false);
+ return reviewer(reviewer, REVIEWER, /* confirmed= */ false);
+ }
+
+ public ReviewInput cc(String cc) {
+ return reviewer(cc, CC, /* confirmed= */ false);
}
public ReviewInput reviewer(String reviewer, ReviewerState state, boolean confirmed) {
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 8824d56..65eb332 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -559,6 +559,11 @@
return searchImpl(input, searchers, this::canSeePredicate, AccountResolver::allVisible);
}
+ public Result resolveIncludeInactiveIgnoreVisibility(String input)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::allVisible);
+ }
+
public Result resolveIgnoreVisibility(String input) throws ConfigInvalidException, IOException {
return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::isActive);
}
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index edaca70..75d4253 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -275,14 +275,29 @@
Iterables.transform(ccs, Account.Id::toString));
}
+ public ChangeInserter setReviewersAndCcsIgnoreVisibility(
+ Iterable<Account.Id> reviewers, Iterable<Account.Id> ccs) {
+ return setReviewersAndCcsAsStrings(
+ Iterables.transform(reviewers, Account.Id::toString),
+ Iterables.transform(ccs, Account.Id::toString),
+ /* skipVisibilityCheck= */ true);
+ }
+
public ChangeInserter setReviewersAndCcsAsStrings(
Iterable<String> reviewers, Iterable<String> ccs) {
+ return setReviewersAndCcsAsStrings(reviewers, ccs, /* skipVisibilityCheck= */ false);
+ }
+
+ private ChangeInserter setReviewersAndCcsAsStrings(
+ Iterable<String> reviewers, Iterable<String> ccs, boolean skipVisibilityCheck) {
reviewerInputs =
Streams.concat(
Streams.stream(reviewers)
.distinct()
- .map(id -> newReviewerInput(id, ReviewerState.REVIEWER)),
- Streams.stream(ccs).distinct().map(id -> newReviewerInput(id, ReviewerState.CC)))
+ .map(id -> newReviewerInput(id, ReviewerState.REVIEWER, skipVisibilityCheck)),
+ Streams.stream(ccs)
+ .distinct()
+ .map(id -> newReviewerInput(id, ReviewerState.CC, skipVisibilityCheck)))
.collect(toImmutableList());
return this;
}
@@ -595,7 +610,8 @@
}
}
- private static InternalReviewerInput newReviewerInput(String reviewer, ReviewerState state) {
+ private static InternalReviewerInput newReviewerInput(
+ String reviewer, ReviewerState state, boolean skipVisibilityCheck) {
// Disable individual emails when adding reviewers, as all reviewers will receive the single
// bulk new change email.
InternalReviewerInput input =
@@ -608,6 +624,8 @@
// not worth complicating the ChangeInserter interface further at this time.
input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE;
+ input.skipVisibilityCheck = skipVisibilityCheck;
+
return input;
}
diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index 9580565..d2dfdfe 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java
@@ -113,6 +113,9 @@
* resolving to an account/group/email.
*/
public FailureBehavior otherFailureBehavior = FailureBehavior.FAIL;
+
+ /** Whether the visibility check for the reviewer account should be skipped. */
+ public boolean skipVisibilityCheck = false;
}
public static InternalReviewerInput newReviewerInput(
@@ -262,7 +265,13 @@
IdentifiedUser reviewerUser;
boolean exactMatchFound = false;
try {
- reviewerUser = accountResolver.resolveIncludeInactive(input.reviewer).asUniqueUser();
+ if (input instanceof InternalReviewerInput
+ && ((InternalReviewerInput) input).skipVisibilityCheck) {
+ reviewerUser =
+ accountResolver.resolveIncludeInactiveIgnoreVisibility(input.reviewer).asUniqueUser();
+ } else {
+ reviewerUser = accountResolver.resolveIncludeInactive(input.reviewer).asUniqueUser();
+ }
if (input.reviewer.equalsIgnoreCase(reviewerUser.getName())
|| input.reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) {
exactMatchFound = true;
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index fa46bf4..f90daff 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -286,7 +286,7 @@
reviewers.remove(user.getAccountId());
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.getAccountId());
- ins.setReviewersAndCcs(reviewers, ccs);
+ ins.setReviewersAndCcsIgnoreVisibility(reviewers, ccs);
ins.setRevertOf(notes.getChangeId());
ins.setWorkInProgress(input.workInProgress);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
index 90ca047..f3d3420 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -26,6 +26,7 @@
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
@@ -49,6 +50,7 @@
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;
@@ -58,6 +60,7 @@
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;
@@ -424,6 +427,46 @@
}
@Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void revertWithNonVisibleUsers() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount reverter = user;
+ TestAccount changeOwner = admin; // must be admin, since admin cloned testRepo
+ TestAccount reviewer = accountCreator.user2();
+ TestAccount cc =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the reverter can neither see the changeOwner, the reviewer nor the cc.
+ requestScopeOperations.setApiUser(reverter.id());
+ assertThatAccountIsNotVisible(changeOwner, reviewer, cc);
+
+ // Create the change.
+ requestScopeOperations.setApiUser(changeOwner.id());
+ PushOneCommit.Result r = createChange();
+
+ // 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();
+
+ // Revert the change.
+ requestScopeOperations.setApiUser(reverter.id());
+ String revertChangeId = gApi.changes().id(r.getChangeId()).revert().get().id;
+
+ // Revert doesn't check the reviewer/CC visibility. Since the reverter can see the reverted
+ // change, they can also see its reviewers/CCs. This means preserving them on the revert change
+ // doesn't expose their account existence and it's OK to keep them even if their accounts are
+ // not visible to the reverter.
+ assertReviewers(revertChangeId, changeOwner, reviewer);
+ assertCcs(revertChangeId, cc);
+ }
+
+ @Test
@TestProjectInput(createEmptyCommit = false)
public void revertInitialCommit() throws Exception {
PushOneCommit.Result r = createChange();
@@ -1446,6 +1489,36 @@
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"
