Merge "Require 'Modify Account' to access another user's secondary emails"

Documentation/access-control.txt

@@ -1299,7 +1299,8 @@ Implies the following capabilities:
 
 Allow to link:cmd-set-account.html[modify accounts over the ssh prompt].
 This capability allows the granted group members to modify any user account
-setting.
+setting. In addition this capability is required to view secondary emails
+of other accounts.
 
 [[capability_priority]]
 === Priority
@@ -1386,6 +1387,10 @@ Allow viewing all accounts for purposes of auto-completion, regardless
 of link:config-gerrit.html#accounts.visibility[accounts.visibility]
 setting.
 
+This capability allows to view all accounts but not all account data.
+E.g. secondary emails of all accounts can only be viewed with the
+link:#capability_modifyAccount[Modify Account] capability.
+
 
 [[capability_viewCaches]]
 === View Caches

Documentation/rest-api-accounts.txt

@@ -63,7 +63,9 @@ for each account.
 
 [[all-emails]]
 --
-* `ALL_EMAILS`: Includes all registered emails.
+* `ALL_EMAILS`: Includes all registered emails. Requires the caller
+to have the link:access-control.html#capability_modifyAccount[Modify
+Account] global capability.
 --
 
 [[suggest-account]]
@@ -79,6 +81,10 @@ link:#all-emails[all emails] are always returned.
   GET /accounts/?suggest&q=John HTTP/1.0
 ----
 
+Secondary emails are only included if the calling user has the
+link:access-control.html#capability_modifyAccount[Modify Account]
+capability.
+
 .Response
 ----
   HTTP/1.1 200 OK
@@ -2159,7 +2165,10 @@ ALL_EMAILS] for account queries.
 |`secondary_emails`|optional|
 A list of the secondary email addresses of the user. +
 Only set for account queries when the link:#all-emails[ALL_EMAILS]
-option is set.
+option or the link:#suggest-account[suggest] parameter is set. +
+Secondary emails are only included if the calling user has the
+link:access-control.html#capability_modifyAccount[Modify Account], and
+hence is allowed to see secondary emails of other users.
 |`username`        |optional|The username of the user. +
 Only set if detailed account information is requested. +
 See option link:rest-api-changes.html#detailed-accounts[

java/com/google/gerrit/extensions/api/accounts/Accounts.java

@@ -137,6 +137,7 @@ public interface Accounts {
     private String query;
     private int limit;
     private int start;
+    private boolean suggest;
     private EnumSet<ListAccountsOption> options = EnumSet.noneOf(ListAccountsOption.class);
 
     /** Execute query and return a list of accounts. */
@@ -166,6 +167,11 @@ public interface Accounts {
       return this;
     }
 
+    public QueryRequest withSuggest(boolean suggest) {
+      this.suggest = suggest;
+      return this;
+    }
+
     public QueryRequest withOption(ListAccountsOption options) {
       this.options.add(options);
       return this;
@@ -193,6 +199,10 @@ public interface Accounts {
       return start;
     }
 
+    public boolean getSuggest() {
+      return suggest;
+    }
+
     public EnumSet<ListAccountsOption> getOptions() {
       return options;
     }

java/com/google/gerrit/server/api/accounts/AccountApiImpl.java

@@ -382,8 +382,12 @@ public class AccountApiImpl implements AccountApi {
   }
 
   @Override
-  public List<EmailInfo> getEmails() {
+  public List<EmailInfo> getEmails() throws RestApiException {
-    return getEmails.apply(account);
+    try {
+      return getEmails.apply(account);
+    } catch (Exception e) {
+      throw asRestApiException("Cannot get emails", e);
+    }
   }
 
   @Override

java/com/google/gerrit/server/api/accounts/AccountsImpl.java

@@ -156,6 +156,7 @@ public class AccountsImpl implements Accounts {
       myQueryAccounts.setQuery(r.getQuery());
       myQueryAccounts.setLimit(r.getLimit());
       myQueryAccounts.setStart(r.getStart());
+      myQueryAccounts.setSuggest(r.getSuggest());
       for (ListAccountsOption option : r.getOptions()) {
         myQueryAccounts.addOption(option);
       }

java/com/google/gerrit/server/query/account/AccountPredicates.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.query.account;
 import com.google.common.collect.Lists;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.Schema;
 import com.google.gerrit.index.query.IndexPredicate;
 import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.index.query.QueryBuilder;
@@ -35,14 +36,26 @@ public class AccountPredicates {
     return Predicate.and(p, isActive());
   }
 
-  public static Predicate<AccountState> defaultPredicate(String query) {
+  public static Predicate<AccountState> defaultPredicate(
+      Schema<AccountState> schema, boolean canSeeSecondaryEmails, String query) {
     // Adapt the capacity of this list when adding more default predicates.
     List<Predicate<AccountState>> preds = Lists.newArrayListWithCapacity(3);
     Integer id = Ints.tryParse(query);
     if (id != null) {
       preds.add(id(new Account.Id(id)));
     }
-    preds.add(equalsName(query));
+    if (canSeeSecondaryEmails) {
+      preds.add(equalsNameIcludingSecondaryEmails(query));
+    } else {
+      if (schema.hasField(AccountField.NAME_PART_NO_SECONDARY_EMAIL)) {
+        preds.add(equalsName(query));
+      } else {
+        preds.add(AccountPredicates.fullName(query));
+        if (schema.hasField(AccountField.PREFERRED_EMAIL)) {
+          preds.add(AccountPredicates.preferredEmail(query));
+        }
+      }
+    }
     preds.add(username(query));
     // Adapt the capacity of the "predicates" list when adding more default
     // predicates.
@@ -54,7 +67,7 @@ public class AccountPredicates {
         AccountField.ID, AccountQueryBuilder.FIELD_ACCOUNT, accountId.toString());
   }
 
-  public static Predicate<AccountState> email(String email) {
+  public static Predicate<AccountState> emailIncludingSecondaryEmails(String email) {
     return new AccountPredicate(
         AccountField.EMAIL, AccountQueryBuilder.FIELD_EMAIL, email.toLowerCase());
   }
@@ -71,12 +84,19 @@ public class AccountPredicates {
         AccountField.PREFERRED_EMAIL_EXACT, AccountQueryBuilder.FIELD_PREFERRED_EMAIL_EXACT, email);
   }
 
-  public static Predicate<AccountState> equalsName(String name) {
+  public static Predicate<AccountState> equalsNameIcludingSecondaryEmails(String name) {
     return new AccountPredicate(
         AccountField.NAME_PART, AccountQueryBuilder.FIELD_NAME, name.toLowerCase());
   }
 
-  public static Predicate<AccountState> externalId(String externalId) {
+  public static Predicate<AccountState> equalsName(String name) {
+    return new AccountPredicate(
+        AccountField.NAME_PART_NO_SECONDARY_EMAIL,
+        AccountQueryBuilder.FIELD_NAME,
+        name.toLowerCase());
+  }
+
+  public static Predicate<AccountState> externalIdIncludingSecondaryEmails(String externalId) {
     return new AccountPredicate(AccountField.EXTERNAL_ID, externalId);
   }
 

java/com/google/gerrit/server/query/account/AccountQueryBuilder.java

@@ -18,6 +18,8 @@ import com.google.common.base.Splitter;
 import com.google.common.collect.Lists;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.common.errors.NotSignedInException;
+import com.google.gerrit.index.Index;
+import com.google.gerrit.index.Schema;
 import com.google.gerrit.index.query.LimitPredicate;
 import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.index.query.QueryBuilder;
@@ -26,12 +28,21 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.index.account.AccountField;
+import com.google.gerrit.server.index.account.AccountIndexCollection;
+import com.google.gerrit.server.permissions.GlobalPermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.ProvisionException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /** Parses a query string meant to be applied to account objects. */
 public class AccountQueryBuilder extends QueryBuilder<AccountState> {
+  private static final Logger log = LoggerFactory.getLogger(AccountQueryBuilder.class);
+
   public static final String FIELD_ACCOUNT = "account";
   public static final String FIELD_EMAIL = "email";
   public static final String FIELD_LIMIT = "limit";
@@ -46,10 +57,17 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
 
   public static class Arguments {
     private final Provider<CurrentUser> self;
+    private final AccountIndexCollection indexes;
+    private final PermissionBackend permissionBackend;
 
     @Inject
-    public Arguments(Provider<CurrentUser> self) {
+    public Arguments(
+        Provider<CurrentUser> self,
+        AccountIndexCollection indexes,
+        PermissionBackend permissionBackend) {
       this.self = self;
+      this.indexes = indexes;
+      this.permissionBackend = permissionBackend;
     }
 
     IdentifiedUser getIdentifiedUser() throws QueryParseException {
@@ -71,6 +89,11 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
         throw new QueryParseException(NotSignedInException.MESSAGE, e);
       }
     }
+
+    Schema<AccountState> schema() {
+      Index<?, AccountState> index = indexes != null ? indexes.getSearchIndex() : null;
+      return index != null ? index.getSchema() : null;
+    }
   }
 
   private final Arguments args;
@@ -82,8 +105,17 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
   }
 
   @Operator
-  public Predicate<AccountState> email(String email) {
+  public Predicate<AccountState> email(String email)
-    return AccountPredicates.email(email);
+      throws PermissionBackendException, QueryParseException {
+    if (canSeeSecondaryEmails()) {
+      return AccountPredicates.emailIncludingSecondaryEmails(email);
+    }
+
+    if (args.schema().hasField(AccountField.PREFERRED_EMAIL)) {
+      return AccountPredicates.preferredEmail(email);
+    }
+
+    throw new QueryParseException("'email' operator is not supported by account index version");
   }
 
   @Operator
@@ -107,8 +139,17 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
   }
 
   @Operator
-  public Predicate<AccountState> name(String name) {
+  public Predicate<AccountState> name(String name)
-    return AccountPredicates.equalsName(name);
+      throws PermissionBackendException, QueryParseException {
+    if (canSeeSecondaryEmails()) {
+      return AccountPredicates.equalsNameIcludingSecondaryEmails(name);
+    }
+
+    if (args.schema().hasField(AccountField.NAME_PART_NO_SECONDARY_EMAIL)) {
+      return AccountPredicates.equalsName(name);
+    }
+
+    return AccountPredicates.fullName(name);
   }
 
   @Operator
@@ -124,7 +165,8 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
 
   @Override
   protected Predicate<AccountState> defaultField(String query) {
-    Predicate<AccountState> defaultPredicate = AccountPredicates.defaultPredicate(query);
+    Predicate<AccountState> defaultPredicate =
+        AccountPredicates.defaultPredicate(args.schema(), checkedCanSeeSecondaryEmails(), query);
     if ("self".equalsIgnoreCase(query) || "me".equalsIgnoreCase(query)) {
       try {
         return Predicate.or(defaultPredicate, AccountPredicates.id(self()));
@@ -138,4 +180,20 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
   private Account.Id self() throws QueryParseException {
     return args.getIdentifiedUser().getAccountId();
   }
+
+  private boolean canSeeSecondaryEmails() throws PermissionBackendException, QueryParseException {
+    return args.permissionBackend.user(args.getUser()).test(GlobalPermission.MODIFY_ACCOUNT);
+  }
+
+  private boolean checkedCanSeeSecondaryEmails() {
+    try {
+      return canSeeSecondaryEmails();
+    } catch (PermissionBackendException e) {
+      log.error("Permission check failed", e);
+      return false;
+    } catch (QueryParseException e) {
+      // User is not signed in.
+      return false;
+    }
+  }
 }

java/com/google/gerrit/server/query/account/InternalAccountQuery.java

@@ -83,7 +83,7 @@ public class InternalAccountQuery extends InternalQuery<AccountState> {
   }
 
   public List<AccountState> byDefault(String query) throws OrmException {
-    return query(AccountPredicates.defaultPredicate(query));
+    return query(AccountPredicates.defaultPredicate(schema(), true, query));
   }
 
   public List<AccountState> byExternalId(String scheme, String id) throws OrmException {
@@ -91,7 +91,7 @@ public class InternalAccountQuery extends InternalQuery<AccountState> {
   }
 
   public List<AccountState> byExternalId(ExternalId.Key externalId) throws OrmException {
-    return query(AccountPredicates.externalId(externalId.toString()));
+    return query(AccountPredicates.externalIdIncludingSecondaryEmails(externalId.toString()));
   }
 
   public AccountState oneByExternalId(String externalId) throws OrmException {

java/com/google/gerrit/server/restapi/account/GetEmails.java

@@ -15,8 +15,15 @@
 package com.google.gerrit.server.restapi.account;
 
 import com.google.gerrit.extensions.common.EmailInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.AccountResource;
+import com.google.gerrit.server.permissions.GlobalPermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -25,9 +32,22 @@ import java.util.List;
 
 @Singleton
 public class GetEmails implements RestReadView<AccountResource> {
+  private final Provider<CurrentUser> self;
+  private final PermissionBackend permissionBackend;
+
+  @Inject
+  GetEmails(Provider<CurrentUser> self, PermissionBackend permissionBackend) {
+    this.self = self;
+    this.permissionBackend = permissionBackend;
+  }
 
   @Override
-  public List<EmailInfo> apply(AccountResource rsrc) {
+  public List<EmailInfo> apply(AccountResource rsrc)
+      throws AuthException, PermissionBackendException {
+    if (self.get() != rsrc.getUser()) {
+      permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT);
+    }
+
     List<EmailInfo> emails = new ArrayList<>();
     for (String email : rsrc.getUser().getEmailAddresses()) {
       if (email != null) {

java/com/google/gerrit/server/restapi/account/QueryAccounts.java

@@ -21,22 +21,28 @@ import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.AccountVisibility;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
+import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
 import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.index.query.QueryParseException;
 import com.google.gerrit.index.query.QueryResult;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.AccountDirectory.FillOptions;
 import com.google.gerrit.server.account.AccountInfoComparator;
 import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.config.GerritServerConfig;
+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.AccountPredicates;
 import com.google.gerrit.server.query.account.AccountQueryBuilder;
 import com.google.gerrit.server.query.account.AccountQueryProcessor;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
+import com.google.inject.Provider;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.LinkedHashMap;
@@ -49,6 +55,8 @@ import org.kohsuke.args4j.Option;
 public class QueryAccounts implements RestReadView<TopLevelResource> {
   private static final int MAX_SUGGEST_RESULTS = 100;
 
+  private final Provider<CurrentUser> self;
+  private final PermissionBackend permissionBackend;
   private final AccountLoader.Factory accountLoaderFactory;
   private final AccountQueryBuilder queryBuilder;
   private final AccountQueryProcessor queryProcessor;
@@ -117,10 +125,14 @@ public class QueryAccounts implements RestReadView<TopLevelResource> {
 
   @Inject
   QueryAccounts(
+      Provider<CurrentUser> self,
+      PermissionBackend permissionBackend,
       AccountLoader.Factory accountLoaderFactory,
       AccountQueryBuilder queryBuilder,
       AccountQueryProcessor queryProcessor,
       @GerritServerConfig Config cfg) {
+    this.self = self;
+    this.permissionBackend = permissionBackend;
     this.accountLoaderFactory = accountLoaderFactory;
     this.queryBuilder = queryBuilder;
     this.queryProcessor = queryProcessor;
@@ -143,7 +155,7 @@ public class QueryAccounts implements RestReadView<TopLevelResource> {
 
   @Override
   public List<AccountInfo> apply(TopLevelResource rsrc)
-      throws OrmException, BadRequestException, MethodNotAllowedException {
+      throws OrmException, RestApiException, PermissionBackendException {
     if (Strings.isNullOrEmpty(query)) {
       throw new BadRequestException("missing query field");
     }
@@ -156,14 +168,21 @@ public class QueryAccounts implements RestReadView<TopLevelResource> {
     if (options.contains(ListAccountsOption.DETAILS)) {
       fillOptions.addAll(AccountLoader.DETAILED_OPTIONS);
     }
+    boolean modifyAccountCapabilityChecked = false;
     if (options.contains(ListAccountsOption.ALL_EMAILS)) {
+      permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT);
+      modifyAccountCapabilityChecked = true;
       fillOptions.add(FillOptions.EMAIL);
       fillOptions.add(FillOptions.SECONDARY_EMAILS);
     }
     if (suggest) {
       fillOptions.addAll(AccountLoader.DETAILED_OPTIONS);
       fillOptions.add(FillOptions.EMAIL);
-      fillOptions.add(FillOptions.SECONDARY_EMAILS);
+
+      if (modifyAccountCapabilityChecked
+          || permissionBackend.user(self).test(GlobalPermission.MODIFY_ACCOUNT)) {
+        fillOptions.add(FillOptions.SECONDARY_EMAILS);
+      }
     }
     accountLoader = accountLoaderFactory.create(fillOptions);
 

java/com/google/gerrit/server/restapi/change/ReviewersUtil.java

@@ -36,6 +36,7 @@ import com.google.gerrit.metrics.MetricMaker;
 import com.google.gerrit.metrics.Timer0;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.AccountControl;
 import com.google.gerrit.server.account.AccountDirectory.FillOptions;
 import com.google.gerrit.server.account.AccountLoader;
@@ -44,6 +45,9 @@ import com.google.gerrit.server.account.GroupMembers;
 import com.google.gerrit.server.index.account.AccountField;
 import com.google.gerrit.server.index.account.AccountIndexCollection;
 import com.google.gerrit.server.notedb.ChangeNotes;
+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.project.NoSuchProjectException;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.query.account.AccountPredicates;
@@ -51,6 +55,7 @@ import com.google.gerrit.server.query.account.AccountQueryBuilder;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.ResultSet;
 import com.google.inject.Inject;
+import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.ArrayList;
@@ -109,7 +114,7 @@ public class ReviewersUtil {
   // give the ranking algorithm a good set of candidates it can work with
   private static final int CANDIDATE_LIST_MULTIPLIER = 2;
 
-  private final AccountLoader accountLoader;
+  private final AccountLoader.Factory accountLoaderFactory;
   private final AccountQueryBuilder accountQueryBuilder;
   private final GroupBackend groupBackend;
   private final GroupMembers groupMembers;
@@ -118,6 +123,8 @@ public class ReviewersUtil {
   private final AccountIndexCollection accountIndexes;
   private final IndexConfig indexConfig;
   private final AccountControl.Factory accountControlFactory;
+  private final Provider<CurrentUser> self;
+  private final PermissionBackend permissionBackend;
 
   @Inject
   ReviewersUtil(
@@ -129,10 +136,10 @@ public class ReviewersUtil {
       Metrics metrics,
       AccountIndexCollection accountIndexes,
       IndexConfig indexConfig,
-      AccountControl.Factory accountControlFactory) {
+      AccountControl.Factory accountControlFactory,
-    Set<FillOptions> fillOptions = EnumSet.of(FillOptions.SECONDARY_EMAILS);
+      Provider<CurrentUser> self,
-    fillOptions.addAll(AccountLoader.DETAILED_OPTIONS);
+      PermissionBackend permissionBackend) {
-    this.accountLoader = accountLoaderFactory.create(fillOptions);
+    this.accountLoaderFactory = accountLoaderFactory;
     this.accountQueryBuilder = accountQueryBuilder;
     this.groupBackend = groupBackend;
     this.groupMembers = groupMembers;
@@ -141,6 +148,8 @@ public class ReviewersUtil {
     this.accountIndexes = accountIndexes;
     this.indexConfig = indexConfig;
     this.accountControlFactory = accountControlFactory;
+    this.self = self;
+    this.permissionBackend = permissionBackend;
   }
 
   public interface VisibilityControl {
@@ -153,7 +162,7 @@ public class ReviewersUtil {
       ProjectState projectState,
       VisibilityControl visibilityControl,
       boolean excludeGroups)
-      throws IOException, OrmException, ConfigInvalidException {
+      throws IOException, OrmException, ConfigInvalidException, PermissionBackendException {
     String query = suggestReviewers.getQuery();
     int limit = suggestReviewers.getLimit();
 
@@ -242,7 +251,14 @@ public class ReviewersUtil {
   }
 
   private List<SuggestedReviewerInfo> loadAccounts(List<Account.Id> accountIds)
-      throws OrmException {
+      throws OrmException, PermissionBackendException {
+    Set<FillOptions> fillOptions =
+        permissionBackend.user(self).test(GlobalPermission.MODIFY_ACCOUNT)
+            ? EnumSet.of(FillOptions.SECONDARY_EMAILS)
+            : EnumSet.noneOf(FillOptions.class);
+    fillOptions.addAll(AccountLoader.DETAILED_OPTIONS);
+    AccountLoader accountLoader = accountLoaderFactory.create(fillOptions);
+
     try (Timer0.Context ctx = metrics.loadAccountsLatency.start()) {
       List<SuggestedReviewerInfo> reviewer =
           accountIds

java/com/google/gerrit/server/restapi/change/SuggestChangeReviewers.java

@@ -26,6 +26,7 @@ import com.google.gerrit.server.IdentifiedUser.GenericFactory;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.restapi.change.ReviewersUtil.VisibilityControl;
 import com.google.gwtorm.server.OrmException;
@@ -66,7 +67,8 @@ public class SuggestChangeReviewers extends SuggestReviewers
 
   @Override
   public List<SuggestedReviewerInfo> apply(ChangeResource rsrc)
-      throws AuthException, BadRequestException, OrmException, IOException, ConfigInvalidException {
+      throws AuthException, BadRequestException, OrmException, IOException, ConfigInvalidException,
+          PermissionBackendException {
     if (!self.get().isIdentifiedUser()) {
       throw new AuthException("Authentication required");
     }

javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -713,14 +713,24 @@ public class AccountIT extends AbstractDaemonTest {
   }
 
   @Test
-  public void getEmailsOfOtherAccount() throws Exception {
+  public void cannotGetEmailsOfOtherAccountWithoutModifyAccount() throws Exception {
     String email = "preferred2@example.com";
-    String secondaryEmail = "secondary2@example.com";
+    TestAccount foo = accountCreator.create(name("foo"), email, "Foo");
+
+    setApiUser(user);
+    exception.expect(AuthException.class);
+    exception.expectMessage("modify account not permitted");
+    gApi.accounts().id(foo.id.get()).getEmails();
+  }
+
+  @Test
+  public void getEmailsOfOtherAccount() throws Exception {
+    String email = "preferred3@example.com";
+    String secondaryEmail = "secondary3@example.com";
     TestAccount foo = accountCreator.create(name("foo"), email, "Foo");
     EmailInput input = newEmailInput(secondaryEmail);
     gApi.accounts().id(foo.id.hashCode()).addEmail(input);
 
-    setApiUser(user);
     assertThat(
             gApi.accounts()
                 .id(foo.id.get())

javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java

@@ -24,6 +24,7 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.common.data.GlobalCapability;
+import com.google.gerrit.extensions.api.accounts.EmailInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.common.GroupInfo;
@@ -413,6 +414,60 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
     assertReviewers(suggestReviewers(changeId, name), ImmutableList.of(foo1), ImmutableList.of());
   }
 
+  @Test
+  public void suggestBySecondaryEmailWithModifyAccount() throws Exception {
+    String secondaryEmail = "foo.secondary@example.com";
+    TestAccount foo = createAccountWithSecondaryEmail("foo", secondaryEmail);
+
+    List<SuggestedReviewerInfo> reviewers =
+        suggestReviewers(createChange().getChangeId(), secondaryEmail, 4);
+    assertReviewers(reviewers, ImmutableList.of(foo), ImmutableList.of());
+
+    reviewers = suggestReviewers(createChange().getChangeId(), "secondary", 4);
+    assertReviewers(reviewers, ImmutableList.of(foo), ImmutableList.of());
+  }
+
+  @Test
+  public void cannotSuggestBySecondaryEmailWithoutModifyAccount() throws Exception {
+    String secondaryEmail = "foo.secondary@example.com";
+    createAccountWithSecondaryEmail("foo", secondaryEmail);
+
+    setApiUser(user);
+    List<SuggestedReviewerInfo> reviewers =
+        suggestReviewers(createChange().getChangeId(), secondaryEmail, 4);
+    assertThat(reviewers).isEmpty();
+
+    reviewers = suggestReviewers(createChange().getChangeId(), "secondary2", 4);
+    assertThat(reviewers).isEmpty();
+  }
+
+  @Test
+  public void secondaryEmailsInSuggestions() throws Exception {
+    String secondaryEmail = "foo.secondary@example.com";
+    TestAccount foo = createAccountWithSecondaryEmail("foo", secondaryEmail);
+
+    List<SuggestedReviewerInfo> reviewers =
+        suggestReviewers(createChange().getChangeId(), "foo", 4);
+    assertReviewers(reviewers, ImmutableList.of(foo), ImmutableList.of());
+    assertThat(Iterables.getOnlyElement(reviewers).account.secondaryEmails)
+        .containsExactly(secondaryEmail);
+
+    setApiUser(user);
+    reviewers = suggestReviewers(createChange().getChangeId(), "foo", 4);
+    assertReviewers(reviewers, ImmutableList.of(foo), ImmutableList.of());
+    assertThat(Iterables.getOnlyElement(reviewers).account.secondaryEmails).isNull();
+  }
+
+  private TestAccount createAccountWithSecondaryEmail(String name, String secondaryEmail)
+      throws Exception {
+    TestAccount foo = accountCreator.create(name(name), "foo.primary@example.com", "Foo");
+    EmailInput input = new EmailInput();
+    input.email = secondaryEmail;
+    input.noConfirmation = true;
+    gApi.accounts().id(foo.id.get()).addEmail(input);
+    return foo;
+  }
+
   private List<SuggestedReviewerInfo> suggestReviewers(String changeId, String query)
       throws Exception {
     return gApi.changes().id(changeId).suggestReviewers(query).get();

javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java

@@ -26,10 +26,14 @@ import com.google.gerrit.extensions.client.ListAccountsOption;
 import com.google.gerrit.extensions.client.ProjectWatchInfo;
 import com.google.gerrit.extensions.common.AccountExternalIdInfo;
 import com.google.gerrit.extensions.common.AccountInfo;
+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.RestApiException;
+import com.google.gerrit.index.FieldDef;
 import com.google.gerrit.index.IndexConfig;
 import com.google.gerrit.index.QueryOptions;
+import com.google.gerrit.index.Schema;
 import com.google.gerrit.index.query.FieldBundle;
 import com.google.gerrit.lifecycle.LifecycleManager;
 import com.google.gerrit.reviewdb.client.Account;
@@ -121,7 +125,7 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
   protected Injector injector;
   protected ReviewDb db;
   protected AccountInfo currentUserInfo;
-  protected CurrentUser user;
+  protected CurrentUser admin;
 
   protected abstract Injector createInjector();
 
@@ -145,10 +149,10 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
     db = schemaFactory.open();
     schemaCreator.create(db);
 
-    Account.Id userId = createAccount("user", "User", "user@example.com", true);
+    Account.Id adminId = createAccount("admin", "Administrator", "admin@example.com", true);
-    user = userFactory.create(userId);
+    admin = userFactory.create(adminId);
-    requestContext.setContext(newRequestContext(userId));
+    requestContext.setContext(newRequestContext(adminId));
-    currentUserInfo = gApi.accounts().id(userId.get()).get();
+    currentUserInfo = gApi.accounts().id(adminId.get()).get();
   }
 
   protected RequestContext newRequestContext(Account.Id requestUserId) {
@@ -237,6 +241,52 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
     assertQuery("email:" + user5.email.toUpperCase(), user5);
   }
 
+  @Test
+  public void bySecondaryEmail() throws Exception {
+    String prefix = name("secondary");
+    String domain = name("test.com");
+    String secondaryEmail = prefix + "@" + domain;
+    AccountInfo user1 = newAccountWithEmail("user1", name("user1@example.com"));
+    addEmails(user1, secondaryEmail);
+
+    AccountInfo user2 = newAccountWithEmail("user2", name("user2@example.com"));
+    addEmails(user2, name("other@" + domain));
+
+    assertQuery(secondaryEmail, user1);
+    assertQuery("email:" + secondaryEmail, user1);
+    assertQuery("email:" + prefix, user1);
+    assertQuery(domain, user1, user2);
+  }
+
+  @Test
+  public void byEmailWithoutModifyAccountCapability() throws Exception {
+    String preferredEmail = name("primary@test.com");
+    String secondaryEmail = name("secondary@test.com");
+    AccountInfo user1 = newAccountWithEmail("user1", preferredEmail);
+    addEmails(user1, secondaryEmail);
+
+    AccountInfo user2 = newAccount("user");
+    requestContext.setContext(newRequestContext(new Account.Id(user2._accountId)));
+
+    if (getSchemaVersion() < 5) {
+      assertMissingField(AccountField.PREFERRED_EMAIL);
+      assertFailingQuery("email:foo", "'email' operator is not supported by account index version");
+      return;
+    }
+
+    // This at least needs the PREFERRED_EMAIL field which is available from schema version 5.
+    if (getSchemaVersion() >= 5) {
+      assertQuery(preferredEmail, user1);
+    } else {
+      assertQuery(preferredEmail);
+    }
+
+    assertQuery(secondaryEmail);
+
+    assertQuery("email:" + preferredEmail, user1);
+    assertQuery("email:" + secondaryEmail);
+  }
+
   @Test
   public void byUsername() throws Exception {
     AccountInfo user1 = newAccount("myuser");
@@ -297,6 +347,49 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
     assertQuery("name:" + quote(user2.name), user2);
   }
 
+  @Test
+  public void byNameWithoutModifyAccountCapability() throws Exception {
+    AccountInfo user1 = newAccountWithFullName("jdoe", "John Doe");
+    AccountInfo user2 = newAccountWithFullName("jroe", "Jane Roe");
+
+    AccountInfo user3 = newAccount("user");
+    requestContext.setContext(newRequestContext(new Account.Id(user3._accountId)));
+
+    assertQuery("notexisting");
+    assertQuery("Not Existing");
+
+    // by full name works with any index version
+    assertQuery(quote(user1.name), user1);
+    assertQuery("name:" + quote(user1.name), user1);
+    assertQuery(quote(user2.name), user2);
+    assertQuery("name:" + quote(user2.name), user2);
+
+    // by self/me works with any index version
+    assertQuery("self", user3);
+    assertQuery("me", user3);
+
+    if (getSchemaVersion() < 8) {
+      assertMissingField(AccountField.NAME_PART_NO_SECONDARY_EMAIL);
+
+      // prefix queries only work if the NAME_PART_NO_SECONDARY_EMAIL field is available
+      assertQuery("john");
+      return;
+    }
+
+    assertQuery("John", user1);
+    assertQuery("john", user1);
+    assertQuery("Doe", user1);
+    assertQuery("doe", user1);
+    assertQuery("DOE", user1);
+    assertQuery("Jo Do", user1);
+    assertQuery("jo do", user1);
+    assertQuery("name:John", user1);
+    assertQuery("name:john", user1);
+    assertQuery("name:Doe", user1);
+    assertQuery("name:doe", user1);
+    assertQuery("name:DOE", user1);
+  }
+
   @Test
   public void byWatchedProject() throws Exception {
     Project.NameKey p = createProject(name("p"));
@@ -375,6 +468,11 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
     List<AccountInfo> result = assertQuery(user1.username, user1);
     assertThat(result.get(0).secondaryEmails).isNull();
 
+    result = assertQuery(newQuery(user1.username).withSuggest(true), user1);
+    assertThat(result.get(0).secondaryEmails)
+        .containsExactlyElementsIn(Arrays.asList(secondaryEmails))
+        .inOrder();
+
     result = assertQuery(newQuery(user1.username).withOption(ListAccountsOption.DETAILS), user1);
     assertThat(result.get(0).secondaryEmails).isNull();
 
@@ -393,6 +491,21 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
         .inOrder();
   }
 
+  @Test
+  public void withSecondaryEmailsWithoutModifyAccountCapability() throws Exception {
+    AccountInfo user = newAccount("myuser", "My User", "abc@example.com", true);
+    String[] secondaryEmails = new String[] {"dfg@example.com", "hij@example.com"};
+    addEmails(user, secondaryEmails);
+
+    requestContext.setContext(newRequestContext(new Account.Id(user._accountId)));
+
+    List<AccountInfo> result = newQuery(user.username).withSuggest(true).get();
+    assertThat(result.get(0).secondaryEmails).isNull();
+
+    exception.expect(AuthException.class);
+    newQuery(user.username).withOption(ListAccountsOption.ALL_EMAILS).get();
+  }
+
   @Test
   public void asAnonymous() throws Exception {
     AccountInfo user1 = newAccount("user1");
@@ -432,7 +545,7 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
 
   @Test
   public void rawDocument() throws Exception {
-    AccountInfo userInfo = gApi.accounts().id(user.getAccountId().get()).get();
+    AccountInfo userInfo = gApi.accounts().id(admin.getAccountId().get()).get();
 
     Optional<FieldBundle> rawFields =
         indexes
@@ -633,6 +746,29 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
     return accounts.stream().map(a -> a._accountId).collect(toList());
   }
 
+  protected void assertMissingField(FieldDef<AccountState, ?> field) {
+    assertThat(getSchema().hasField(field))
+        .named("schema %s has field %s", getSchemaVersion(), field.getName())
+        .isFalse();
+  }
+
+  protected void assertFailingQuery(String query, String expectedMessage) throws Exception {
+    try {
+      assertQuery(query);
+      fail("expected BadRequestException for query '" + query + "'");
+    } catch (BadRequestException e) {
+      assertThat(e.getMessage()).isEqualTo(expectedMessage);
+    }
+  }
+
+  protected int getSchemaVersion() {
+    return getSchema().getVersion();
+  }
+
+  protected Schema<AccountState> getSchema() {
+    return indexes.getSearchIndex().getSchema();
+  }
+
   /** Boiler plate code to check two byte arrays for equality */
   private static class ByteArrayWrapper {
     private byte[] arr;
@@ -654,8 +790,4 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
       return Arrays.hashCode(arr);
     }
   }
-
-  protected int getSchemaVersion() {
-    return indexes.getSearchIndex().getSchema().getVersion();
-  }
 }