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(); - } }