Add global capability that allows to view secondary emails
If users have multiple emails only the preferred email is visible to
other users. This means when users change their preferred email, the new
preferred email becomes visible while the old preferred email stops to
be visible.
This can be a problem for bots that operate on emails, as emails can
suddenly become non-visible. E.g. if all users have a corporate email
and optionally other emails, a bot that uses the corporate emails (e.g.
to auto-assign reviewers on changes) stops working when users change
their preferred email to a non-corporate email, as this makes the
corporate email non-visible. Now, with the new View Secondary Emails
global capability it's possible to allow the bot to view all emails, so
that this is no longer a problem.
This became an issue now after change I799bf1c57 fixed an issue that
allowed users to resolve secondary emails although they should not have
been able to see them.
So far the only work-around was to assign the bot the Modify Account
global capability that allows to view secondary emails too, but this
capability is too broad as it also allows to modify all accounts.
So basically the new View Secondary Emails capability is just a subset
of the existing Modify Account capability: both capabilities allow to
see secondary emails of other accounts, but the Modify Account
capability allows in addition to modify accounts.
The Modify Account global capability continues allowing to view
secondary emails. This means having the Modify Account capability
implies having the View Secondary Emails capability.
Having the permission to view secondary emails of other accounts,
doesn't change anything about account visibility, but it only allows
resolving secondary emails to visible accounts. This means if a
secondary email is used as account identifier in the REST API it can
only be resolved if the account is visible and the calling user can see
secondary emails.
If a user asks for secondary emails of an account (e.g. a REST call to
list all emails of an account, or querying accounts with
ListAccountsOption ALL_EMAILS), but the user cannot see secondary emails
we return "view secondary emails not permitted" as the error message now
(before the error message was "modify account not permitted"). The new
error message should be easier to understand.
Bug: Google b/272679324
Release-Notes: Added global capability that allows to view secondary emails
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Iec901ec050974ed62bc74c9df5f8ca88c2956fae
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 348649d..cf89982 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -1417,8 +1417,8 @@
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. In addition this capability is required to view secondary emails
-of other accounts.
+setting. In addition this capability allows to view secondary emails of other
+accounts.
[[capability_priority]]
=== Priority
@@ -1520,7 +1520,8 @@
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.
+link:#capability_viewSecondaryEmails[View Secondary Emails] capability
+or the link:#capability_modifyAccount[Modify Account] capability.
[[capability_viewCaches]]
@@ -1553,6 +1554,15 @@
link:cmd-show-queue.html[look at the Gerrit task queue via ssh].
+[[capability_viewSecondaryEmails]]
+=== View Secondary Emails
+
+Allows viewing secondary emails of other accounts.
+
+Users with the link:#capability_modifyAccount[Modify Account] capability have
+this capbility implicitly.
+
+
[[reference]]
== Permission evaluation reference
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt
index 7e06e4a..7646777 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -2316,6 +2316,9 @@
link:access-control.html#capability_viewPlugins[View Plugins] capability.
|`viewQueue` |not set if `false`|Whether the user has the
link:access-control.html#capability_viewQueue[View Queue] capability.
+|`viewSecondaryEmails`|not set if `false`|Whether the user has the
+link:access-control.html#capability_viewSecondaryEmails[View Secondary
+Emails] capability.
|=================================
[[contributor-agreement-info]]
diff --git a/java/com/google/gerrit/common/data/GlobalCapability.java b/java/com/google/gerrit/common/data/GlobalCapability.java
index c7a9a63..23151c2 100644
--- a/java/com/google/gerrit/common/data/GlobalCapability.java
+++ b/java/com/google/gerrit/common/data/GlobalCapability.java
@@ -129,6 +129,9 @@
/** Can view all pending tasks in the queue (not just the filtered set). */
public static final String VIEW_QUEUE = "viewQueue";
+ /** Can view secondary emails of other accounts. */
+ public static final String VIEW_SECONDARY_EMAILS = "viewSecondaryEmails";
+
private static final List<String> NAMES_ALL;
private static final List<String> NAMES_LC;
private static final String[] RANGE_NAMES = {
@@ -160,6 +163,7 @@
NAMES_ALL.add(VIEW_CONNECTIONS);
NAMES_ALL.add(VIEW_PLUGINS);
NAMES_ALL.add(VIEW_QUEUE);
+ NAMES_ALL.add(VIEW_SECONDARY_EMAILS);
NAMES_LC = new ArrayList<>(NAMES_ALL.size());
for (String name : NAMES_ALL) {
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 1818b1b..2020d2f 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -434,16 +434,16 @@
@Override
public Stream<AccountState> search(String input, CurrentUser asUser) throws IOException {
- boolean canSeeSecondaryEmails = false;
+ boolean canViewSecondaryEmails = false;
try {
- if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
- canSeeSecondaryEmails = true;
+ if (permissionBackend.user(asUser).test(GlobalPermission.VIEW_SECONDARY_EMAILS)) {
+ canViewSecondaryEmails = true;
}
} catch (PermissionBackendException e) {
// remains false
}
- if (canSeeSecondaryEmails) {
+ if (canViewSecondaryEmails) {
return toAccountStates(emails.getAccountFor(input));
}
@@ -549,15 +549,15 @@
// up with a reasonable result list.
// TODO(dborowitz): This doesn't match the documentation; consider whether it's possible to be
// more strict here.
- boolean canSeeSecondaryEmails = false;
+ boolean canViewSecondaryEmails = false;
try {
- if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
- canSeeSecondaryEmails = true;
+ if (permissionBackend.user(asUser).test(GlobalPermission.VIEW_SECONDARY_EMAILS)) {
+ canViewSecondaryEmails = true;
}
} catch (PermissionBackendException e) {
// remains false
}
- return accountQueryProvider.get().byDefault(input, canSeeSecondaryEmails).stream();
+ return accountQueryProvider.get().byDefault(input, canViewSecondaryEmails).stream();
}
@Override
diff --git a/java/com/google/gerrit/server/account/InternalAccountDirectory.java b/java/com/google/gerrit/server/account/InternalAccountDirectory.java
index b895834..64b8ec0 100644
--- a/java/com/google/gerrit/server/account/InternalAccountDirectory.java
+++ b/java/com/google/gerrit/server/account/InternalAccountDirectory.java
@@ -96,12 +96,12 @@
return;
}
- boolean canModifyAccount = false;
+ boolean canViewSecondaryEmails = false;
Account.Id currentUserId = null;
if (self.get().isIdentifiedUser()) {
currentUserId = self.get().getAccountId();
- if (permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
- canModifyAccount = true;
+ if (permissionBackend.currentUser().test(GlobalPermission.VIEW_SECONDARY_EMAILS)) {
+ canViewSecondaryEmails = true;
}
}
@@ -115,7 +115,7 @@
if (state != null) {
if (!options.contains(FillOptions.SECONDARY_EMAILS)
|| Objects.equals(currentUserId, state.account().id())
- || canModifyAccount) {
+ || canViewSecondaryEmails) {
fill(info, accountStates.get(id), options);
} else {
// user is not allowed to see secondary emails
diff --git a/java/com/google/gerrit/server/config/CapabilityConstants.java b/java/com/google/gerrit/server/config/CapabilityConstants.java
index 59819bb..1f799c6 100644
--- a/java/com/google/gerrit/server/config/CapabilityConstants.java
+++ b/java/com/google/gerrit/server/config/CapabilityConstants.java
@@ -45,4 +45,5 @@
public String viewConnections;
public String viewPlugins;
public String viewQueue;
+ public String viewSecondaryEmails;
}
diff --git a/java/com/google/gerrit/server/index/account/AccountField.java b/java/com/google/gerrit/server/index/account/AccountField.java
index cc86ffc..e675003 100644
--- a/java/com/google/gerrit/server/index/account/AccountField.java
+++ b/java/com/google/gerrit/server/index/account/AccountField.java
@@ -66,7 +66,8 @@
* External IDs.
*
* <p>This field includes secondary emails. Use this field only if the current user is allowed to
- * see secondary emails (requires the {@link GlobalCapability#MODIFY_ACCOUNT} capability).
+ * see secondary emails (requires the {@link GlobalCapability#VIEW_SECONDARY_EMAILS} capability or
+ * the {@link GlobalCapability#MODIFY_ACCOUNT} capability).
*/
public static final IndexedField<AccountState, Iterable<String>> EXTERNAL_ID_FIELD =
IndexedField.<AccountState>iterableStringBuilder("ExternalId")
@@ -80,8 +81,9 @@
* Fuzzy prefix match on name and email parts.
*
* <p>This field includes parts from the secondary emails. Use this field only if the current user
- * is allowed to see secondary emails (requires the {@link GlobalCapability#MODIFY_ACCOUNT}
- * capability).
+ * is allowed to see secondary emails (requires requires the {@link
+ * GlobalCapability#VIEW_SECONDARY_EMAILS} capability or the {@link
+ * GlobalCapability#MODIFY_ACCOUNT} capability).
*
* <p>Use the {@link AccountField#NAME_PART_NO_SECONDARY_EMAIL_SPEC} if the current user can't see
* secondary emails.
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
index bf4d05a..a4ee052 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
@@ -195,12 +195,17 @@
case MODIFY_ACCOUNT:
case READ_AS:
case STREAM_EVENTS:
+ case VIEW_ACCESS:
case VIEW_ALL_ACCOUNTS:
case VIEW_CONNECTIONS:
case VIEW_PLUGINS:
- case VIEW_ACCESS:
return has(globalPermissionName(perm)) || isAdmin();
+ case VIEW_SECONDARY_EMAILS:
+ return has(globalPermissionName(perm))
+ || has(globalPermissionName(GlobalPermission.MODIFY_ACCOUNT))
+ || isAdmin();
+
case ACCESS_DATABASE:
case RUN_AS:
return has(globalPermissionName(perm));
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
index da24dcd..958de1b 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
@@ -61,6 +61,7 @@
.put(GlobalPermission.VIEW_CONNECTIONS, GlobalCapability.VIEW_CONNECTIONS)
.put(GlobalPermission.VIEW_PLUGINS, GlobalCapability.VIEW_PLUGINS)
.put(GlobalPermission.VIEW_QUEUE, GlobalCapability.VIEW_QUEUE)
+ .put(GlobalPermission.VIEW_SECONDARY_EMAILS, GlobalCapability.VIEW_SECONDARY_EMAILS)
.build();
static {
diff --git a/java/com/google/gerrit/server/permissions/GlobalPermission.java b/java/com/google/gerrit/server/permissions/GlobalPermission.java
index c0b44e5..3429978 100644
--- a/java/com/google/gerrit/server/permissions/GlobalPermission.java
+++ b/java/com/google/gerrit/server/permissions/GlobalPermission.java
@@ -58,7 +58,8 @@
VIEW_CACHES,
VIEW_CONNECTIONS,
VIEW_PLUGINS,
- VIEW_QUEUE;
+ VIEW_QUEUE,
+ VIEW_SECONDARY_EMAILS;
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
diff --git a/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java b/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
index d5c4a97..2f4a923 100644
--- a/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
@@ -151,7 +151,7 @@
@Operator
public Predicate<AccountState> email(String email)
throws PermissionBackendException, QueryParseException {
- if (canSeeSecondaryEmails()) {
+ if (canViewSecondaryEmails()) {
return AccountPredicates.emailIncludingSecondaryEmails(email);
}
@@ -185,7 +185,7 @@
@Operator
public Predicate<AccountState> name(String name)
throws PermissionBackendException, QueryParseException {
- if (canSeeSecondaryEmails()) {
+ if (canViewSecondaryEmails()) {
return AccountPredicates.equalsNameIncludingSecondaryEmails(name);
}
@@ -210,7 +210,7 @@
@Override
protected Predicate<AccountState> defaultField(String query) {
Predicate<AccountState> defaultPredicate =
- AccountPredicates.defaultPredicate(args.schema(), checkedCanSeeSecondaryEmails(), query);
+ AccountPredicates.defaultPredicate(args.schema(), checkedCanViewSecondaryEmails(), query);
if (query.startsWith("cansee:")) {
try {
return cansee(query.substring(7));
@@ -233,13 +233,13 @@
return args.getIdentifiedUser().getAccountId();
}
- private boolean canSeeSecondaryEmails() throws PermissionBackendException, QueryParseException {
- return args.permissionBackend.user(args.getUser()).test(GlobalPermission.MODIFY_ACCOUNT);
+ private boolean canViewSecondaryEmails() throws PermissionBackendException, QueryParseException {
+ return args.permissionBackend.user(args.getUser()).test(GlobalPermission.VIEW_SECONDARY_EMAILS);
}
- private boolean checkedCanSeeSecondaryEmails() {
+ private boolean checkedCanViewSecondaryEmails() {
try {
- return canSeeSecondaryEmails();
+ return canViewSecondaryEmails();
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("Permission check failed");
return false;
diff --git a/java/com/google/gerrit/server/restapi/account/GetEmails.java b/java/com/google/gerrit/server/restapi/account/GetEmails.java
index 4d70eb9..a518532 100644
--- a/java/com/google/gerrit/server/restapi/account/GetEmails.java
+++ b/java/com/google/gerrit/server/restapi/account/GetEmails.java
@@ -52,7 +52,7 @@
public Response<List<EmailInfo>> apply(AccountResource rsrc)
throws AuthException, PermissionBackendException {
if (!self.get().hasSameAccountId(rsrc.getUser())) {
- permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
+ permissionBackend.currentUser().check(GlobalPermission.VIEW_SECONDARY_EMAILS);
}
return Response.ok(
rsrc.getUser().getEmailAddresses().stream()
diff --git a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
index 79737f3..9fc0c42 100644
--- a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
+++ b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
@@ -173,7 +173,7 @@
}
boolean modifyAccountCapabilityChecked = false;
if (options.contains(ListAccountsOption.ALL_EMAILS)) {
- permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
+ permissionBackend.currentUser().check(GlobalPermission.VIEW_SECONDARY_EMAILS);
modifyAccountCapabilityChecked = true;
fillOptions.add(FillOptions.EMAIL);
fillOptions.add(FillOptions.SECONDARY_EMAILS);
@@ -185,7 +185,7 @@
if (modifyAccountCapabilityChecked) {
fillOptions.add(FillOptions.SECONDARY_EMAILS);
} else {
- if (permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
+ if (permissionBackend.currentUser().test(GlobalPermission.VIEW_SECONDARY_EMAILS)) {
fillOptions.add(FillOptions.SECONDARY_EMAILS);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index a9a0b70..dd04200 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -991,14 +991,15 @@
}
@Test
- public void cannotGetEmailsOfOtherAccountWithoutModifyAccount() throws Exception {
+ public void cannotGetEmailsOfOtherAccountWithoutViewSecondaryEmailsAndWithoutModifyAccount()
+ throws Exception {
String email = "preferred2@example.com";
TestAccount foo = accountCreator.create(name("foo"), email, "Foo", null);
requestScopeOperations.setApiUser(user.id());
AuthException thrown =
assertThrows(AuthException.class, () -> gApi.accounts().id(foo.id().get()).getEmails());
- assertThat(thrown).hasMessageThat().contains("modify account not permitted");
+ assertThat(thrown).hasMessageThat().contains("view secondary emails not permitted");
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
index 43c8684..3f3ad37 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
@@ -549,6 +549,19 @@
@Test
public void changesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability()
throws Exception {
+ testCangesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability(
+ GlobalCapability.MODIFY_ACCOUNT);
+ }
+
+ @Test
+ public void changesFoundWhenQueryingBySecondaryEmailWithViewSecondaryEmailsCapability()
+ throws Exception {
+ testCangesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability(
+ GlobalCapability.VIEW_SECONDARY_EMAILS);
+ }
+
+ private void testCangesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability(
+ String globalCapability) throws Exception {
String secondaryOwnerEmail = "owner-secondary@example.com";
Account.Id owner =
accountOperations
@@ -574,7 +587,7 @@
projectOperations
.allProjectsForUpdate()
- .add(allowCapability(GlobalCapability.MODIFY_ACCOUNT).group(REGISTERED_USERS))
+ .add(allowCapability(globalCapability).group(REGISTERED_USERS))
.update();
requestScopeOperations.setApiUser(user.id());
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/CapabilityInfo.java b/javatests/com/google/gerrit/acceptance/rest/account/CapabilityInfo.java
index 7598062..3ce6d8d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/CapabilityInfo.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/CapabilityInfo.java
@@ -38,6 +38,7 @@
public boolean viewConnections;
public boolean viewPlugins;
public boolean viewQueue;
+ public boolean viewSecondaryEmails;
static class QueryLimit {
short min;
diff --git a/resources/com/google/gerrit/server/config/CapabilityConstants.properties b/resources/com/google/gerrit/server/config/CapabilityConstants.properties
index 1a355eb..b6aca6f 100644
--- a/resources/com/google/gerrit/server/config/CapabilityConstants.properties
+++ b/resources/com/google/gerrit/server/config/CapabilityConstants.properties
@@ -21,3 +21,4 @@
viewConnections = View Connections
viewPlugins = View Plugins
viewQueue = View Queue
+viewSecondaryEmails = View Secondary Emails
