Fix: Require proper permission to search users by secondary email
We currently allow probing for secondary emails e.g. via
/accounts/secondary_email/detail. But reading secondary emails should
actually require MODIFY_ACCOUNT permission.
Change-Id: Ic0f36127a3d8134371fe11baf7a8cb57f8e369f1
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 103013c..68f5a85 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -27,12 +27,16 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.index.Schema;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.config.AnonymousCowardName;
+import com.google.gerrit.server.permissions.GlobalPermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.account.InternalAccountQuery;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -437,7 +441,15 @@
// up with a reasonable result list.
// TODO(dborowitz): This doesn't match the documentation; consider whether it's possible to be
// more strict here.
- return accountQueryProvider.get().enforceVisibility(true).byDefault(input).stream();
+ boolean canSeeSecondaryEmails = false;
+ try {
+ permissionBackend.user(self.get()).check(GlobalPermission.MODIFY_ACCOUNT);
+ canSeeSecondaryEmails = true;
+ } catch (AuthException | PermissionBackendException e) {
+ // remains false
+ }
+ return accountQueryProvider.get().enforceVisibility(true)
+ .byDefault(input, canSeeSecondaryEmails).stream();
}
@Override
@@ -473,6 +485,7 @@
private final Provider<InternalAccountQuery> accountQueryProvider;
private final Realm realm;
private final String anonymousCowardName;
+ private final PermissionBackend permissionBackend;
@Inject
AccountResolver(
@@ -482,15 +495,17 @@
IdentifiedUser.GenericFactory userFactory,
Provider<CurrentUser> self,
Provider<InternalAccountQuery> accountQueryProvider,
+ PermissionBackend permissionBackend,
Realm realm,
@AnonymousCowardName String anonymousCowardName) {
- this.realm = realm;
this.accountCache = accountCache;
+ this.emails = emails;
this.accountControlFactory = accountControlFactory;
this.userFactory = userFactory;
this.self = self;
this.accountQueryProvider = accountQueryProvider;
- this.emails = emails;
+ this.permissionBackend = permissionBackend;
+ this.realm = realm;
this.anonymousCowardName = anonymousCowardName;
}
diff --git a/java/com/google/gerrit/server/query/account/InternalAccountQuery.java b/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
index 091edca..b7f8b45 100644
--- a/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
+++ b/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
@@ -54,8 +54,8 @@
super(queryProcessor, indexes, indexConfig);
}
- public List<AccountState> byDefault(String query) {
- return query(AccountPredicates.defaultPredicate(schema(), true, query));
+ public List<AccountState> byDefault(String query, boolean canSeeSecondaryEmails) {
+ return query(AccountPredicates.defaultPredicate(schema(), canSeeSecondaryEmails, query));
}
public List<AccountState> byExternalId(String scheme, String id) {
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index d54574a..8c7d892 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -1797,7 +1797,7 @@
}
assertThat(accountCache.get(admin.id())).isEmpty();
- assertThat(accountQueryProvider.get().byDefault(admin.id().toString())).isEmpty();
+ assertThat(accountQueryProvider.get().byDefault(admin.id().toString(), true)).isEmpty();
}
@Test
@@ -2187,7 +2187,7 @@
@Test
public void internalQueryFindActiveAndInactiveAccounts() throws Exception {
String name = name("foo");
- assertThat(accountQueryProvider.get().byDefault(name)).isEmpty();
+ assertThat(accountQueryProvider.get().byDefault(name, true)).isEmpty();
TestAccount foo1 = accountCreator.create(name + "-1");
assertThat(gApi.accounts().id(foo1.username()).getActive()).isTrue();
@@ -2196,7 +2196,7 @@
gApi.accounts().id(foo2.username()).setActive(false);
assertThat(gApi.accounts().id(foo2.id().get()).getActive()).isFalse();
- assertThat(accountQueryProvider.get().byDefault(name)).hasSize(2);
+ assertThat(accountQueryProvider.get().byDefault(name, true)).hasSize(2);
}
@Test
@@ -3014,6 +3014,20 @@
}
}
+ @Test
+ public void searchForSecondaryEmailRequiresModifyAccountPermission() throws Exception {
+ String email = "preferred@example.com";
+ TestAccount foo = accountCreator.create(name("foo"), email, "Foo", null);
+ String secondaryEmail = "secondary@example.com";
+ EmailInput input = newEmailInput(secondaryEmail);
+ gApi.accounts().id(foo.id().get()).addEmail(input);
+
+ requestScopeOperations.setApiUser(user.id());
+ assertThrows(ResourceNotFoundException.class, () -> gApi.accounts().id("secondary"));
+ requestScopeOperations.setApiUser(admin.id());
+ assertThat(gApi.accounts().id("secondary").get()._accountId).isEqualTo(foo.id().get());
+ }
+
private void createDraft(PushOneCommit.Result r, String path, String message) throws Exception {
DraftInput in = new DraftInput();
in.path = path;
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java b/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
index b999abd..84f95f7 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
@@ -58,4 +58,21 @@
AccountDetailInfo info = newGson().fromJson(r.getReader(), AccountDetailInfo.class);
assertThat(info.tags).containsExactly(AccountInfo.Tag.SERVICE_USER);
}
+
+ @Test
+ public void searchForSecondaryEmailRequiresModifyAccountPermission() throws Exception {
+ Account.Id id =
+ accountOperations
+ .newAccount()
+ .preferredEmail("preferred@email")
+ .addSecondaryEmail("secondary@email")
+ .create();
+ RestResponse r = userRestSession.get("/accounts/secondary/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());
+ }
}
diff --git a/javatests/com/google/gerrit/server/account/AccountResolverTest.java b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
index 5f14d28..5e49aaf 100644
--- a/javatests/com/google/gerrit/server/account/AccountResolverTest.java
+++ b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
@@ -352,7 +352,7 @@
}
private static AccountResolver newAccountResolver() {
- return new AccountResolver(null, null, null, null, null, null, null, "Anonymous Name");
+ return new AccountResolver(null, null, null, null, null, null, null, null, "Anonymous Name");
}
private AccountState newAccount(int id) {
