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

Only the preferred email should be readily available to other users.
Secondary emails should only be accessible to the user that owns the
account or to users that have the 'Modify Account' capability. We choose
to check on the 'Modify Account' capability because the intention of
this change is to hide secondary emails from normal users and 'Modify
Account' is a capability that is almost certainly only assigned to
administrator users (as opposed to e.g. the 'View All Accounts'
capability which may also be granted to normal users to see all
accounts but not all account data). Also when you edit an account you
must be able to see its full data. Admittedly the 'Modify Account'
capability is not a perfect match for this and it would be nicer to have
a new 'View Secondary Emails' capability for this. For now we refrain
from adding yet another capability while we have no specific use case
for it but follow-up changes can add it if needed.

So far secondary emails of other users could be accessed in various
ways:

* GetEmails REST endpoint
* QueryAccounts REST endpoint with the ALL_EMAILS options
* QueryAccounts REST endpoint with the suggest option

The GetEmails REST endpoint is now only supported if the calling user
owns the account or if the calling user has the 'Modify Account'
capability.

Using the ALL_EMAILS option on the QueryAccounts REST endpoint also
requires the 'Modify Account' capability now.

The QueryAccounts REST endpoint with the suggest option is changed to
only include secondary emails if the calling user has the 'Modify
Account' capability.

Also the GetExternalIds REST endpoint includes secondary emails in its
response. However this REST endpoint already requires that the calling
user owns the account or has the 'Access Database' capability. Hence
already now normal users can't use this REST endpoint to get access of
the secondary emails of other accounts.

To hide secondary emails we must also disable querying accounts by
secondary email. Otherwise one could search with 'email:foo.com' to find
all accounts that have a '*@foo.com' email address.

The following index fields contain the secondary emails (or parts of
them):

* EMAIL
* NAME_PART
* EXTERNAL_ID

The EMAIL field is used for queries with the 'email' operator. With this
change the EMAIL field is now only used if the calling user has the
'Modify Account' capability. If the calling user doesn't have this
capability we now use the PREFERRED_EMAIL field instead.

The NAME_PART field is used for default queries and queries with the
'name' operator. With this change the NAME_PART field is now only used
if the calling user has the 'Modify Account' capability. For users that
don't have this capability a new NAME_PART_NO_SECONDARY_EMAIL field was
added that does not contain name parts of secondary emails and that then
can be used instead of the NAME_PART field (see predecessor change).
However this means that prefix searches by name parts are not working
while a search index version is used that doesn't include the new
NAME_PART_NO_SECONDARY_EMAIL field yet (e.g. while online reindexing
hasn't finsished yet). This does affects reviewer suggestion, but
shouldn't be a big issue since online reindexing for accounts is
expected to be fast.

The EXTERNAL_ID field is only used internally and there is no query
operator that maps to it.

Disabling querying by secondary email for users without the 'Modify
Account' capability means that these users can't get reviewers suggested
by secondary email anymore.

Change-Id: Icf3108d45fb1a7c5f6965c28c4ddc2bfbfedb38a
Signed-off-by: Edwin Kempin <ekempin@google.com>

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() {
-    return getEmails.apply(account);
+  public List<EmailInfo> getEmails() throws RestApiException {
+    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) {
-    return AccountPredicates.email(email);
+  public Predicate<AccountState> email(String 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) {
-    return AccountPredicates.equalsName(name);
+  public Predicate<AccountState> name(String 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) {
-    Set<FillOptions> fillOptions = EnumSet.of(FillOptions.SECONDARY_EMAILS);
-    fillOptions.addAll(AccountLoader.DETAILED_OPTIONS);
-    this.accountLoader = accountLoaderFactory.create(fillOptions);
+      AccountControl.Factory accountControlFactory,
+      Provider<CurrentUser> self,
+      PermissionBackend permissionBackend) {
+    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);
-    user = userFactory.create(userId);
-    requestContext.setContext(newRequestContext(userId));
-    currentUserInfo = gApi.accounts().id(userId.get()).get();
+    Account.Id adminId = createAccount("admin", "Administrator", "admin@example.com", true);
+    admin = userFactory.create(adminId);
+    requestContext.setContext(newRequestContext(adminId));
+    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();
-  }
 }