Merge changes I022c6a13,I799bf1c5
* changes:
Skip account visibility checks when querying changes
Check permissions when resolving accounts by secondary emails
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 703693d..2670e52 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -43,6 +43,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TreeSet;
@@ -421,13 +422,45 @@
private class ByEmail extends StringSearcher {
@Override
+ public boolean requiresContextUser() {
+ return true;
+ }
+
+ @Override
protected boolean matches(String input) {
return input.contains("@");
}
@Override
- public Stream<AccountState> search(String input) throws IOException {
- return toAccountStates(emails.getAccountFor(input));
+ public Stream<AccountState> search(String input, CurrentUser asUser) throws IOException {
+ boolean canSeeSecondaryEmails = false;
+ try {
+ if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
+ canSeeSecondaryEmails = true;
+ }
+ } catch (PermissionBackendException e) {
+ // remains false
+ }
+
+ if (canSeeSecondaryEmails) {
+ return toAccountStates(emails.getAccountFor(input));
+ }
+
+ // User cannot see secondary emails, hence search by preferred email only.
+ Stream<AccountState> accountStateStream =
+ accountQueryProvider.get().byPreferredEmail(input).stream();
+
+ // Users can always see their own secondary emails. Hence if any email of the user matches,
+ // include the user into the result.
+ if (asUser.isIdentifiedUser()
+ && asUser.asIdentifiedUser().state().externalIds().stream()
+ .map(ExternalId::email)
+ .filter(Objects::nonNull)
+ .anyMatch(email -> email.equals(input))) {
+ return Streams.concat(accountStateStream, Stream.of(asUser.asIdentifiedUser().state()));
+ }
+
+ return accountStateStream;
}
@Override
@@ -671,6 +704,18 @@
input, searchers, self.get(), this::allVisiblePredicate, AccountResolver::isActive);
}
+ public Result resolveAsUserIgnoreVisibility(CurrentUser asUser, String input)
+ throws ConfigInvalidException, IOException {
+ return resolveAsUserIgnoreVisibility(asUser, input, AccountResolver::isActive);
+ }
+
+ public Result resolveAsUserIgnoreVisibility(
+ CurrentUser asUser, String input, Predicate<AccountState> accountActivityPredicate)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(
+ input, searchers, asUser, this::allVisiblePredicate, accountActivityPredicate);
+ }
+
/**
* Resolves all accounts matching the input string by name or email.
*
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index f6fc8db..56a0bbd 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -24,6 +24,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Enums;
import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
@@ -230,6 +231,7 @@
public static final Account.Id OWNER_ACCOUNT_ID = Account.id(0);
public static final Account.Id NON_UPLOADER_ACCOUNT_ID = Account.id(-1);
public static final Account.Id NON_CONTRIBUTOR_ACCOUNT_ID = Account.id(-2);
+ public static final Account.Id NON_EXISTING_ACCOUNT_ID = Account.id(-1000);
public static final String OPERATOR_MERGED_BEFORE = "mergedbefore";
public static final String OPERATOR_MERGED_AFTER = "mergedafter";
@@ -1043,7 +1045,7 @@
} else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
} else {
- accounts = parseAccount(value);
+ accounts = parseAccountIgnoreVisibility(value);
}
} else if (key.equalsIgnoreCase(ARG_ID_GROUP)) {
group = parseGroup(value).getUUID();
@@ -1071,17 +1073,17 @@
if (accounts != null || group != null) {
throw new QueryParseException("more than one user/group specified (" + value + ")");
}
- try {
- if (value.equals(ARG_ID_OWNER)) {
- accounts = Collections.singleton(OWNER_ACCOUNT_ID);
- } else if (value.equals(ARG_ID_NON_UPLOADER)) {
- accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
- } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
- accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
- } else {
- accounts = parseAccount(value);
- }
- } catch (QueryParseException qpex) {
+ if (value.equals(ARG_ID_OWNER)) {
+ accounts = Collections.singleton(OWNER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_UPLOADER)) {
+ accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
+ accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
+ } else {
+ accounts = parseAccountIgnoreVisibility(value);
+ }
+
+ if (accounts.contains(NON_EXISTING_ACCOUNT_ID)) {
// If it doesn't match an account, see if it matches a group
// (accounts get precedence)
try {
@@ -1227,7 +1229,7 @@
@Operator
public Predicate<ChangeData> owner(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return owner(parseAccount(who, (AccountState s) -> true));
+ return owner(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> owner(Set<Account.Id> who) {
@@ -1240,7 +1242,7 @@
private Predicate<ChangeData> ownerDefaultField(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- Set<Account.Id> accounts = parseAccount(who);
+ Set<Account.Id> accounts = parseAccountIgnoreVisibility(who);
if (accounts.size() > MAX_ACCOUNTS_PER_DEFAULT_FIELD) {
return Predicate.any();
}
@@ -1251,7 +1253,7 @@
public Predicate<ChangeData> uploader(String who)
throws QueryParseException, IOException, ConfigInvalidException {
checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploader");
- return uploader(parseAccount(who, (AccountState s) -> true));
+ return uploader(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> uploader(Set<Account.Id> who) {
@@ -1266,7 +1268,7 @@
public Predicate<ChangeData> attention(String who)
throws QueryParseException, IOException, ConfigInvalidException {
checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
- return attention(parseAccount(who, (AccountState s) -> true));
+ return attention(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> attention(Set<Account.Id> who) {
@@ -1403,7 +1405,7 @@
@Operator
public Predicate<ChangeData> commentby(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return commentby(parseAccount(who));
+ return commentby(parseAccountIgnoreVisibility(who));
}
private Predicate<ChangeData> commentby(Set<Account.Id> who) {
@@ -1417,7 +1419,7 @@
@Operator
public Predicate<ChangeData> from(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- Set<Account.Id> ownerIds = parseAccount(who);
+ Set<Account.Id> ownerIds = parseAccountIgnoreVisibility(who);
return Predicate.or(owner(ownerIds), commentby(ownerIds));
}
@@ -1469,7 +1471,7 @@
@Operator
public Predicate<ChangeData> reviewedby(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return ChangePredicates.reviewedBy(parseAccount(who));
+ return ChangePredicates.reviewedBy(parseAccountIgnoreVisibility(who));
}
@Operator
@@ -1722,18 +1724,42 @@
}
}
- private Set<Account.Id> parseAccount(
- String who, java.util.function.Predicate<AccountState> activityFilter)
- throws QueryParseException, IOException, ConfigInvalidException {
+ private Set<Account.Id> parseAccountIgnoreVisibility(String who)
+ throws QueryRequiresAuthException, IOException, ConfigInvalidException {
try {
return args.accountResolver
- .resolveAsUser(args.getUser(), who, activityFilter)
+ .resolveAsUserIgnoreVisibility(args.getUser(), who)
.asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
}
- throw new QueryParseException(e.getMessage(), e);
+ return ImmutableSet.of(NON_EXISTING_ACCOUNT_ID);
+ }
+ }
+
+ private Set<Account.Id> parseAccountIgnoreVisibility(
+ String who, java.util.function.Predicate<AccountState> activityFilter)
+ throws QueryRequiresAuthException, IOException, ConfigInvalidException {
+ try {
+ return args.accountResolver
+ .resolveAsUserIgnoreVisibility(args.getUser(), who, activityFilter)
+ .asNonEmptyIdSet();
+ } catch (UnresolvableAccountException e) {
+ // Thrown if no account was found.
+
+ // Users can always see their own account. This means if self was being resolved and there was
+ // no match the user wasn't logged it and the request was done anonymously.
+ if (e.isSelf()) {
+ throw new QueryRequiresAuthException(e.getMessage(), e);
+ }
+
+ // If no account is found, we don't want to fail with an error as this would allow users to
+ // probe the existence of accounts (error -> account doesn't exist, empty result -> account
+ // exists but didn't take part in any visible changes). Hence, we return a special account ID
+ // (NON_EXISTING_ACCOUNT_ID) that doesn't match any account so the query can be normally
+ // executed
+ return ImmutableSet.of(NON_EXISTING_ACCOUNT_ID);
}
}
@@ -1790,7 +1816,7 @@
Predicate<ChangeData> reviewerPredicate = null;
try {
- Set<Account.Id> accounts = parseAccount(who);
+ Set<Account.Id> accounts = parseAccountIgnoreVisibility(who);
if (!forDefaultField || accounts.size() <= MAX_ACCOUNTS_PER_DEFAULT_FIELD) {
reviewerPredicate =
Predicate.or(
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 2c1739d..a9a0b70 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -2899,8 +2899,12 @@
requestScopeOperations.setApiUser(user.id());
assertThrows(ResourceNotFoundException.class, () -> gApi.accounts().id("secondary"));
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id("secondary@example.com"));
requestScopeOperations.setApiUser(admin.id());
assertThat(gApi.accounts().id("secondary").get()._accountId).isEqualTo(foo.id().get());
+ assertThat(gApi.accounts().id("secondary@example.com").get()._accountId)
+ .isEqualTo(foo.id().get());
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
index 3bfb573..43c8684 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
@@ -23,16 +24,20 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.UseClockStep;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.entities.AccessSection;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
@@ -40,9 +45,11 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.restapi.change.QueryChanges;
+import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.Arrays;
@@ -53,6 +60,7 @@
public class QueryChangesIT extends AbstractDaemonTest {
@Inject private AccountOperations accountOperations;
+ @Inject private ChangeOperations changeOperations;
@Inject private ProjectOperations projectOperations;
@Inject private Provider<QueryChanges> queryChangesProvider;
@Inject private RequestScopeOperations requestScopeOperations;
@@ -364,6 +372,282 @@
assertThat(result3).hasSize(1);
}
+ /**
+ * This test verifies that querying by a non-visible account doesn't fail.
+ *
+ * <p>Change queries only return changes that are visible to the calling user. If a non-visible
+ * account participated in such a change the existence of this account is known to everyone who
+ * can see the change. Hence it's OK to that the account visibility check is skipped when querying
+ * changes by non-visible accounts. If the account is visible through any visible change these
+ * changes are returned, otherwise the result is empty (see
+ * emptyResultWhenQueryingByNonVisibleAccountAndMatchingChangesAreNotVisible()), same as for
+ * non-existing accounts (see test emptyResultWhenQueryingByNonExistingAccount()).
+ */
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void changesCanBeQueriesByNonVisibleAccounts() throws Exception {
+ String ownerEmail = "owner@example.com";
+ Account.Id nonVisibleOwner = accountOperations.newAccount().preferredEmail(ownerEmail).create();
+
+ String reviewerEmail = "reviewer@example.com";
+ Account.Id nonVisibleReviewer =
+ accountOperations.newAccount().preferredEmail(reviewerEmail).create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(nonVisibleOwner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(nonVisibleReviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that user cannot see the other accounts.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleOwner.get()).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleReviewer.get()).get());
+
+ // Verify that the change is also found if user queries for changes owned/uploaded by
+ // nonVisibleOwner.
+ assertThat(gApi.changes().query("owner:" + ownerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + ownerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is also found if user queries for changes reviewed by
+ // nonVisibleReviewer.
+ assertThat(gApi.changes().query("reviewer:" + reviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + reviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
+ /**
+ * This test verifies that an empty result is returned for a query by a non-existing account.
+ *
+ * <p>Such queries must not return an error so that users cannot probe whether an account exists.
+ * Since we return an empty result for non-visible accounts if there are no matched changes or non
+ * of the matched changes is visible, users could conclude the existence of a account if we would
+ * return an error for non-existing accounts.
+ */
+ @Test
+ public void emptyResultWhenQueryingByNonExistingAccount() throws Exception {
+ assertThat(gApi.changes().query("owner:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("reviewer:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=non-existing@example.com").get())
+ .isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void emptyResultWhenQueryingByNonVisibleAccountAndMatchingChangesAreNotVisible()
+ throws Exception {
+ String ownerEmail = "owner@example.com";
+ Account.Id nonVisibleOwner = accountOperations.newAccount().preferredEmail(ownerEmail).create();
+
+ String reviewerEmail = "reviewer@example.com";
+ Account.Id nonVisibleReviewer =
+ accountOperations.newAccount().preferredEmail(reviewerEmail).create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(nonVisibleOwner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(nonVisibleReviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ // Block read permission so that the change is not visible.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user cannot see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get()).isEmpty();
+
+ // Verify that user cannot see the other accounts.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleOwner.get()).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleReviewer.get()).get());
+
+ // Verify that the change is also found if user queries for changes owned/uploaded by
+ // nonVisibleOwner.
+ assertThat(gApi.changes().query("owner:" + ownerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:" + ownerEmail).get()).isEmpty();
+
+ // Verify that the change is also found if user queries for changes reviewed by
+ // nonVisibleReviewer.
+ assertThat(gApi.changes().query("reviewer:" + reviewerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + reviewerEmail).get()).isEmpty();
+ }
+
+ @Test
+ public void emptyResultWhenQueryingByNonVisibleSecondaryEmail() throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that user cannot see the other accounts by their secondary email.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(secondaryOwnerEmail).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(secondaryReviewerEmail).get());
+
+ // Verify that the change is not found if user queries for changes owned/uploaded by the
+ // secondary email of the owner that is not visible to user.
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get()).isEmpty();
+
+ // Verify that the change is not found if user queries for changes reviewed by the secondary
+ // email of the reviewer that is not visible to user.
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .isEmpty();
+ }
+
+ @Test
+ public void changesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability()
+ throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability(GlobalCapability.MODIFY_ACCOUNT).group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the other accounts by their secondary email.
+ assertThat(gApi.accounts().id(secondaryOwnerEmail).get()._accountId).isEqualTo(owner.get());
+ assertThat(gApi.accounts().id(secondaryReviewerEmail).get()._accountId)
+ .isEqualTo(reviewer.get());
+
+ // Verify that the change is found if user queries for changes owned/uploaded by the secondary
+ // email.
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is found if user queries for changes reviewed by the secondary email.
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
+ @Test
+ public void changesFoundWhenQueryingByOwnSecondaryEmail() throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ // Verify that the change is found if owner queries for changes owned/uploaded by their
+ // secondary email.
+ requestScopeOperations.setApiUser(owner);
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is found if reviewer queries for changes reviewed by their secondary
+ // email.
+ requestScopeOperations.setApiUser(reviewer);
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
private static void assertNoChangeHasMoreChangesSet(List<ChangeInfo> results) {
for (ChangeInfo info : results) {
assertThat(info._moreChanges).isNull();
@@ -383,4 +667,9 @@
}
}
}
+
+ private static Correspondence<ChangeInfo, Change.Id> hasChangeId() {
+ return NullAwareCorrespondence.transforming(
+ changeInfo -> Change.id(changeInfo._number), "hasChangeId");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java b/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
index c89e11a..b21f97d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
@@ -91,16 +91,26 @@
Account.Id id =
accountOperations
.newAccount()
- .preferredEmail("preferred@email")
- .addSecondaryEmail("secondary@email")
+ .preferredEmail("preferred@eexample.com")
+ .addSecondaryEmail("secondary@example.com")
.create();
+
RestResponse r = userRestSession.get("/accounts/secondary/detail/");
r.assertStatus(404);
+
+ r = userRestSession.get("/accounts/secondary@example.com/detail/");
+ r.assertStatus(404);
+
// The admin has MODIFY_ACCOUNT permission and can see the user.
r = adminRestSession.get("/accounts/secondary/detail/");
r.assertStatus(200);
AccountDetailInfo info = newGson().fromJson(r.getReader(), AccountDetailInfo.class);
assertThat(info._accountId).isEqualTo(id.get());
+
+ r = adminRestSession.get("/accounts/secondary@example.com/detail/");
+ r.assertStatus(200);
+ info = newGson().fromJson(r.getReader(), AccountDetailInfo.class);
+ assertThat(info._accountId).isEqualTo(id.get());
}
private static class CustomAccountTagProvider implements AccountTagProvider {
diff --git a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
index cf1eee0..432a6c6 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
@@ -450,8 +450,18 @@
.to("refs/for/master");
r.assertOkStatus();
- // assert no email notifications for user
- assertThat(sender.getMessages()).isEmpty();
+ // Assert email notification for user.
+ // The non-visible account participated in a change that is visible to user, hence through this
+ // change user can see the non-visible account.
+ // Even if watching by the non-visible account was not possible, user could just watch all
+ // changes that are visible to them and then filter them by the non-visible account locally.
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ Message m = messages.get(0);
+ assertThat(m.rcpt()).containsExactly(user.getNameEmail());
+ assertThat(m.body()).contains("Change subject: subject\n");
+ assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+ sender.clear();
}
@Test
