Check permissions when resolving accounts by secondary emails
The ByEmail searcher in AccountResolver resolved secondary emails
without checking whether the user has the Modify Account capability that
is required for being able to see secondary emails of other users.
Due to this it was possible to:
* Resolve a non-visible secondary email through the accounts API, e.g.:
GET /accounts/<seondary-email>/detail
* Query changes by a non-visible secondary email, e.g query changes that
are owned/uploaded/reviewed by the account that owns the secondary
email.
The visibility of the resolved accounts is checked in both cases, so
resolving a non-visible secondary email only worked when the account
owning it was visible.
A partial fix for the same issue had already been done by change
Ic0f36127a, but this fix missed to check the permission in the ByEmail
searcher (the ByEmail searcher is only used if the input contains '@',
hence the tests that were added by the fix change didn't catch this).
With this change AccountResolver resolves secondary emails for other
accounts only if the user has the Modify Account capability. Secondary
emails of the own account are always visible and hence always resolved.
Release-Notes: Check permissions when resolving accounts by secondary emails
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I799bf1c57eb84549cba6d7ff462c04cef90fc63b
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 703693d..632f0c8 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
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..3220615 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,180 @@
assertThat(result3).hasSize(1);
}
+ @Test
+ public void accountNotFoundWhenQueryingByNonVisibleSecondaryEmail() 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 querying for changes owned/uploaded by the secondary email of the owner that is
+ // not visible to user fails.
+ assertThat(
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().query("owner:" + secondaryOwnerEmail).get()))
+ .hasMessageThat()
+ .isEqualTo(String.format("Account '%s' not found", secondaryOwnerEmail));
+ assertThat(
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().query("uploader:" + secondaryOwnerEmail).get()))
+ .hasMessageThat()
+ .isEqualTo(String.format("Account '%s' not found", secondaryOwnerEmail));
+
+ // Verify that querying for changes reviewed by the secondary email of the reviewer doesn't
+ // match the change. Note this is not failing if the specified account doesn't exist or is not
+ // visible because it falls back to searching with the reviewerByEmailPredicate in this case
+ // which is for matching reviewers by email that have no account.
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get()).isEmpty();
+
+ // Verify that querying for changes by label + user with the secondary email of the reviewer
+ // that is not visible to user fails.
+ assertThat(
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ gApi.changes()
+ .query("label:Code-Review+1,user=" + secondaryReviewerEmail)
+ .get()))
+ .hasMessageThat()
+ .isEqualTo(String.format("Account '%s' not found", secondaryReviewerEmail));
+ }
+
+ @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 +565,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 {
