From ed7771623b061703c60b1a4526910ce8d666f7d4 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 15 Nov 2017 08:23:40 -0800 Subject: [PATCH] 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 --- Documentation/access-control.txt | 7 +- Documentation/rest-api-accounts.txt | 13 +- .../extensions/api/accounts/Accounts.java | 10 ++ .../server/api/accounts/AccountApiImpl.java | 8 +- .../server/api/accounts/AccountsImpl.java | 1 + .../query/account/AccountPredicates.java | 30 +++- .../query/account/AccountQueryBuilder.java | 70 +++++++- .../query/account/InternalAccountQuery.java | 4 +- .../server/restapi/account/GetEmails.java | 22 ++- .../server/restapi/account/QueryAccounts.java | 23 ++- .../server/restapi/change/ReviewersUtil.java | 30 +++- .../change/SuggestChangeReviewers.java | 4 +- .../acceptance/api/accounts/AccountIT.java | 16 +- .../rest/change/SuggestReviewersIT.java | 55 +++++++ .../account/AbstractQueryAccountsTest.java | 152 ++++++++++++++++-- 15 files changed, 403 insertions(+), 42 deletions(-) diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index bf9cb6de65..a4f03d53b1 100644 --- a/Documentation/access-control.txt +++ b/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 diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt index 8dc3b9d754..5912d1fcbf 100644 --- a/Documentation/rest-api-accounts.txt +++ b/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[ diff --git a/java/com/google/gerrit/extensions/api/accounts/Accounts.java b/java/com/google/gerrit/extensions/api/accounts/Accounts.java index e92d22979b..651e786fe8 100644 --- a/java/com/google/gerrit/extensions/api/accounts/Accounts.java +++ b/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 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 getOptions() { return options; } diff --git a/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java b/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java index 232cc63536..366ebfbe67 100644 --- a/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java +++ b/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java @@ -382,8 +382,12 @@ public class AccountApiImpl implements AccountApi { } @Override - public List getEmails() { - return getEmails.apply(account); + public List getEmails() throws RestApiException { + try { + return getEmails.apply(account); + } catch (Exception e) { + throw asRestApiException("Cannot get emails", e); + } } @Override diff --git a/java/com/google/gerrit/server/api/accounts/AccountsImpl.java b/java/com/google/gerrit/server/api/accounts/AccountsImpl.java index fac16dc495..f5f1a34147 100644 --- a/java/com/google/gerrit/server/api/accounts/AccountsImpl.java +++ b/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); } diff --git a/java/com/google/gerrit/server/query/account/AccountPredicates.java b/java/com/google/gerrit/server/query/account/AccountPredicates.java index 92133537d5..22df2ced45 100644 --- a/java/com/google/gerrit/server/query/account/AccountPredicates.java +++ b/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 defaultPredicate(String query) { + public static Predicate defaultPredicate( + Schema schema, boolean canSeeSecondaryEmails, String query) { // Adapt the capacity of this list when adding more default predicates. List> 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 email(String email) { + public static Predicate 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 equalsName(String name) { + public static Predicate equalsNameIcludingSecondaryEmails(String name) { return new AccountPredicate( AccountField.NAME_PART, AccountQueryBuilder.FIELD_NAME, name.toLowerCase()); } - public static Predicate externalId(String externalId) { + public static Predicate equalsName(String name) { + return new AccountPredicate( + AccountField.NAME_PART_NO_SECONDARY_EMAIL, + AccountQueryBuilder.FIELD_NAME, + name.toLowerCase()); + } + + public static Predicate externalIdIncludingSecondaryEmails(String externalId) { return new AccountPredicate(AccountField.EXTERNAL_ID, externalId); } diff --git a/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java b/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java index 946a729f48..055b423cfa 100644 --- a/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java +++ b/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 { + 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 { public static class Arguments { private final Provider self; + private final AccountIndexCollection indexes; + private final PermissionBackend permissionBackend; @Inject - public Arguments(Provider self) { + public Arguments( + Provider 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 { throw new QueryParseException(NotSignedInException.MESSAGE, e); } } + + Schema schema() { + Index 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 { } @Operator - public Predicate email(String email) { - return AccountPredicates.email(email); + public Predicate 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 { } @Operator - public Predicate name(String name) { - return AccountPredicates.equalsName(name); + public Predicate 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 { @Override protected Predicate defaultField(String query) { - Predicate defaultPredicate = AccountPredicates.defaultPredicate(query); + Predicate 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 { 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; + } + } } diff --git a/java/com/google/gerrit/server/query/account/InternalAccountQuery.java b/java/com/google/gerrit/server/query/account/InternalAccountQuery.java index 2fb837cede..f1be58027d 100644 --- a/java/com/google/gerrit/server/query/account/InternalAccountQuery.java +++ b/java/com/google/gerrit/server/query/account/InternalAccountQuery.java @@ -83,7 +83,7 @@ public class InternalAccountQuery extends InternalQuery { } public List byDefault(String query) throws OrmException { - return query(AccountPredicates.defaultPredicate(query)); + return query(AccountPredicates.defaultPredicate(schema(), true, query)); } public List byExternalId(String scheme, String id) throws OrmException { @@ -91,7 +91,7 @@ public class InternalAccountQuery extends InternalQuery { } public List 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 { diff --git a/java/com/google/gerrit/server/restapi/account/GetEmails.java b/java/com/google/gerrit/server/restapi/account/GetEmails.java index 9c482a3bd3..640cc64fe0 100644 --- a/java/com/google/gerrit/server/restapi/account/GetEmails.java +++ b/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 { + private final Provider self; + private final PermissionBackend permissionBackend; + + @Inject + GetEmails(Provider self, PermissionBackend permissionBackend) { + this.self = self; + this.permissionBackend = permissionBackend; + } @Override - public List apply(AccountResource rsrc) { + public List apply(AccountResource rsrc) + throws AuthException, PermissionBackendException { + if (self.get() != rsrc.getUser()) { + permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT); + } + List emails = new ArrayList<>(); for (String email : rsrc.getUser().getEmailAddresses()) { if (email != null) { diff --git a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java index f508fa204b..fa4550da4d 100644 --- a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java +++ b/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 { private static final int MAX_SUGGEST_RESULTS = 100; + private final Provider 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 { @Inject QueryAccounts( + Provider 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 { @Override public List 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 { 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); diff --git a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java index 70ebbc1dc9..7a2a148612 100644 --- a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java +++ b/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 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 = EnumSet.of(FillOptions.SECONDARY_EMAILS); - fillOptions.addAll(AccountLoader.DETAILED_OPTIONS); - this.accountLoader = accountLoaderFactory.create(fillOptions); + AccountControl.Factory accountControlFactory, + Provider 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 loadAccounts(List accountIds) - throws OrmException { + throws OrmException, PermissionBackendException { + Set 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 reviewer = accountIds diff --git a/java/com/google/gerrit/server/restapi/change/SuggestChangeReviewers.java b/java/com/google/gerrit/server/restapi/change/SuggestChangeReviewers.java index 274d8d5dc3..4dc5b06766 100644 --- a/java/com/google/gerrit/server/restapi/change/SuggestChangeReviewers.java +++ b/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 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"); } diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 4217317878..399d7276a4 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/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()) diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java index 21a1e765a5..c188d63e89 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java +++ b/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 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 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 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 suggestReviewers(String changeId, String query) throws Exception { return gApi.changes().id(changeId).suggestReviewers(query).get(); diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java index 42073930db..657190f295 100644 --- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java +++ b/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 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 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 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 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 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(); - } }