CodeOwnerResolver: Fix visibility checks if user is given
CodeOwnerResolver checks the visibility of the code owners, either for
the user provided by calling the forUser(IdentifiedUser) method or the
calling user.
So far if a user was provided by calling the forUser(IdentifiedUser)
method this user was only used for the account visibility check (see
canSee(AccountState) method) but not for the visibility checks of
secondary emails. The latter always used the current user. Fix this and
extend the tests to cover this case.
While we are here also update the javadocs that only spoke about
checking visibility for the calling user, while actually it could be a
different user, if one was provided by calling the
forUser(IdentifiedUser) method.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Icbb3627051cb546fc1823dfa85e9161d1f9f3e5e
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 1b7accc..da32379 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -240,11 +240,12 @@
* Gerrit core that also treats ambiguous identifiers as non-resolveable.
* </ul>
*
- * <p>This methods checks whether the calling user can see the accounts of the code owners and
- * returns code owners whose accounts are visible.
+ * <p>This methods checks whether the {@link #user} or the calling user (if {@link #user} is
+ * unset) can see the accounts of the code owners and returns code owners whose accounts are
+ * visible.
*
* <p>In addition code owners that are referenced by a secondary email are only returned if the
- * calling user can see the secondary email:
+ * {@link #user} or the calling user (if {@link #user} is unset) can see the secondary email:
*
* <ul>
* <li>every user can see the own secondary emails
@@ -348,43 +349,63 @@
}
/**
- * Checks whether the given account and email are visible to the calling user.
+ * Checks whether the given account and email are visible to the {@link #user} or the calling user
+ * (if {@link #user} is unset).
*
- * <p>If the email is a secondary email it is only visible if it is owned by the calling user or
- * if the calling user has the {@code Modify Account} global capability.
+ * <p>If the email is a secondary email it is only visible if
*
- * @param accountState the account for which it should be checked whether it's visible to the
- * calling user
+ * <ul>
+ * <li>it is owned by the {@link #user} or the calling user (if {@link #user} is unset)
+ * <li>if the {@link #user} or the calling user (if {@link #user} is unset) has the {@code
+ * Modify Account} global capability
+ * </ul>
+ *
+ * @param accountState the account for which it should be checked whether it's visible to the user
* @param email email that was used to reference the account
- * @return {@code true} if the given account and email are visible to the calling user, otherwise
- * {@code false}
+ * @return {@code true} if the given account and email are visible to the user, otherwise {@code
+ * false}
*/
private boolean isVisible(AccountState accountState, String email) {
if (!canSee(accountState)) {
logger.atFine().log(
- "cannot resolve code owner email %s: account %s is not visible to calling user",
- email, accountState.account().id());
+ "cannot resolve code owner email %s: account %s is not visible to user %s",
+ email,
+ accountState.account().id(),
+ user != null ? user.getLoggableName() : currentUser.get().getLoggableName());
return false;
}
if (!email.equals(accountState.account().preferredEmail())) {
// the email is a secondary email of the account
- if (currentUser.get().isIdentifiedUser()
+ if (user != null) {
+ if (user.hasEmailAddress(email)) {
+ // it's a secondary email of the user, users can always see their own secondary emails
+ return true;
+ }
+ } else if (currentUser.get().isIdentifiedUser()
&& currentUser.get().asIdentifiedUser().hasEmailAddress(email)) {
// it's a secondary email of the calling user, users can always see their own secondary
// emails
return true;
}
- // the email is a secondary email of another account, check if the calling user can see
- // secondary emails
+ // the email is a secondary email of another account, check if the user can see secondary
+ // emails
try {
- if (!permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
+ if (user != null) {
+ if (!permissionBackend.user(user).test(GlobalPermission.MODIFY_ACCOUNT)) {
+ logger.atFine().log(
+ "cannot resolve code owner email %s: account %s is referenced by secondary email,"
+ + " but user %s cannot see secondary emails",
+ email, accountState.account().id(), user.getLoggableName());
+ return false;
+ }
+ } else if (!permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
logger.atFine().log(
"cannot resolve code owner email %s: account %s is referenced by secondary email,"
- + " but the calling user cannot see secondary emails",
- email, accountState.account().id());
+ + " but the calling user %s cannot see secondary emails",
+ email, accountState.account().id(), currentUser.get().getLoggableName());
return false;
}
} catch (PermissionBackendException e) {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 86652b2..1210ae5 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -154,6 +154,8 @@
@Test
public void resolveCodeOwnerReferenceForSecondaryEmail() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
// add secondary email to user account
String secondaryEmail = "user@foo.bar";
accountOperations.account(user.id()).forUpdate().addSecondaryEmail(secondaryEmail).update();
@@ -164,10 +166,29 @@
codeOwnerResolver.get().resolve(CodeOwnerReference.create(secondaryEmail));
assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id());
+ // admin has the "Modify Account" global capability and hence can see the secondary email of the
+ // user account if another user is the calling user
+ requestScopeOperations.setApiUser(user2.id());
+ codeOwner =
+ codeOwnerResolver
+ .get()
+ .forUser(identifiedUserFactory.create(admin.id()))
+ .resolve(CodeOwnerReference.create(secondaryEmail));
+ assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id());
+
// user can see its own secondary email.
requestScopeOperations.setApiUser(user.id());
codeOwner = codeOwnerResolver.get().resolve(CodeOwnerReference.create(secondaryEmail));
assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id());
+
+ // user can see its own secondary email if another user is the calling user.
+ requestScopeOperations.setApiUser(user2.id());
+ codeOwner =
+ codeOwnerResolver
+ .get()
+ .forUser(identifiedUserFactory.create(user.id()))
+ .resolve(CodeOwnerReference.create(secondaryEmail));
+ assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id());
}
@Test
@@ -181,6 +202,16 @@
requestScopeOperations.setApiUser(user.id());
assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create(secondaryEmail)))
.isEmpty();
+
+ // user doesn't have the "Modify Account" global capability and hence cannot see the secondary
+ // email of the admin account if another user is the calling user
+ requestScopeOperations.setApiUser(admin.id());
+ assertThat(
+ codeOwnerResolver
+ .get()
+ .forUser(identifiedUserFactory.create(user.id()))
+ .resolve(CodeOwnerReference.create(secondaryEmail)))
+ .isEmpty();
}
@Test
