Allow all users to call the Check Code Owner REST endpoint So far calling the Check Code Owner REST endpoint required the caller to be an admin (have the 'Administrate Server' capability or the 'Check Code Owner Capability'). Due to this normal users couldn't debug issues with OWNERS files on their own, but had to file tickets to find someone that calls the REST endpoint and explains them the result. To reduce the ticket load we are offering the Check Code Owner REST endpoint as a self-service now so that every user can invoke it. Most of the information that is provided by the REST endpoint is not sensitive and can be shown to normal users as they already have access to this information via other REST endpoints (via the code owner suggestion they can find out if a user is a code owner, the inspected code owner config files are already returned via the code owner suggestion REST endpoint, whether an email is resolvable can be checked via the account API, whether a user can see a change can be checked by trying to add the user as a reviewer to the change, whether a user can vote on a change is contained in ChangeInfo when the user is a reviewer). The returned debug logs however may contain information which should only be shown to admins (e.g. messages that explain why a code owner email is not resolvable reveal information about whether an email exists). This is why with change Ib28802d38 we distinguish between messages that can be shown to all users vs. admins only. Now we are making use of this and return user messages for normal users and admin messages for admins. The 'user' option of the Check Code Owner REST endpoint checks the code ownership of a user on behal of another user. This is something that only admins should be able to do, hence we keep this disabled for normal users (e.g. normal users should not be able to check code ownership on behalf of an admin user as this would reveal accounts that the admin user can see, but which are not visible to the calling user). So far the Check Code Owner REST endpoint only checked the visibility of code owners when a user was specified to check whether that user can see the code owners (the 'user' option). If a user was not specified the code owner visibility was not checked, since the REST endpoint could only be invoked by admins this was not necessary and it was intended that they could see all accounts. Now that also normal users can call the REST endpoint we do check the code owner visibility when the calling user is not an admin. Bug: Google b/345161989 Change-Id: I3a2d5d9cc6fde0bb1b4dd690008111ce7c311cf5 Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java index 303f906..2eb6f92 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -46,6 +46,7 @@ import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration; import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval; import com.google.gerrit.plugins.codeowners.util.JgitPath; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.ChangeFinder; import com.google.gerrit.server.notedb.ChangeNotes; @@ -63,6 +64,7 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; @@ -88,6 +90,7 @@ private final UnresolvedImportFormatter unresolvedImportFormatter; private final ChangeFinder changeFinder; private final CodeOwnerConfigFileJson codeOwnerConfigFileJson; + private final Provider<CurrentUser> self; private String email; private String path; @@ -95,6 +98,7 @@ private ChangeNotes changeNotes; private String user; private IdentifiedUser identifiedUser; + private boolean isAdmin; @Inject public CheckCodeOwner( @@ -108,7 +112,8 @@ AccountsCollection accountsCollection, UnresolvedImportFormatter unresolvedImportFormatter, ChangeFinder changeFinder, - CodeOwnerConfigFileJson codeOwnerConfigFileJson) { + CodeOwnerConfigFileJson codeOwnerConfigFileJson, + Provider<CurrentUser> self) { this.checkCodeOwnerCapability = checkCodeOwnerCapability; this.permissionBackend = permissionBackend; this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; @@ -120,6 +125,7 @@ this.unresolvedImportFormatter = unresolvedImportFormatter; this.changeFinder = changeFinder; this.codeOwnerConfigFileJson = codeOwnerConfigFileJson; + this.self = self; } @Option(name = "--email", usage = "email for which the code ownership should be checked") @@ -154,7 +160,11 @@ public Response<CodeOwnerCheckInfo> apply(BranchResource branchResource) throws BadRequestException, AuthException, IOException, ConfigInvalidException, PermissionBackendException, ResourceNotFoundException { - permissionBackend.currentUser().check(checkCodeOwnerCapability.getPermission()); + if (!self.get().isIdentifiedUser()) { + throw new AuthException("Authentication required"); + } + + isAdmin = permissionBackend.currentUser().test(checkCodeOwnerCapability.getPermission()); validateInput(branchResource); @@ -376,8 +386,15 @@ codeOwnerCheckInfo.isGlobalCodeOwner = isGlobalCodeOwner; codeOwnerCheckInfo.isOwnedByAllUsers = isCodeOwnershipAssignedToAllUsers.get(); codeOwnerCheckInfo.annotations = sort(annotations); + codeOwnerCheckInfo.debugLogs = - messages.stream().map(DebugMessage::adminMessage).collect(toImmutableList()); + messages.stream() + .map( + debugMessage -> + isAdmin ? debugMessage.adminMessage() : debugMessage.userMessage().orElse(null)) + .filter(Objects::nonNull) + .collect(toImmutableList()); + return Response.ok(codeOwnerCheckInfo); } @@ -396,6 +413,16 @@ } if (user != null) { try { + permissionBackend.currentUser().check(checkCodeOwnerCapability.getPermission()); + } catch (AuthException e) { + throw new AuthException( + String.format( + "%s: cannot specify a user to check a code owner on behalf of this user", + e.getMessage()), + e); + } + + try { identifiedUser = accountsCollection .parse(TopLevelResource.INSTANCE, IdString.fromDecoded(user)) @@ -449,8 +476,9 @@ if (identifiedUser != null) { codeOwnerResolver.forUser(identifiedUser); } else { - codeOwnerResolver.enforceVisibility(false); + codeOwnerResolver.enforceVisibility(isAdmin ? false : true); } + OptionalResultWithMessages<CodeOwner> resolveResult = codeOwnerResolver.resolveWithMessages(CodeOwnerReference.create(email));
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java index 05a29bf..0208bc9 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -134,14 +134,60 @@ } @Test - public void requiresCallerToBeAdminOrHaveTheCheckCodeOwnerCapability() throws Exception { + public void requiresLogin() throws Exception { + requestScopeOperations.setApiUserAnonymous(); + AuthException exception = + assertThrows(AuthException.class, () -> checkCodeOwner(ROOT_PATH, user.email())); + assertThat(exception).hasMessageThat().contains("Authentication required"); + } + + @Test + public void checkCodeOwnerForOtherUserRequiresCallerToBeAdminOrHaveTheCheckCodeOwnerCapability() + throws Exception { requestScopeOperations.setApiUser(user.id()); AuthException authException = - assertThrows(AuthException.class, () -> checkCodeOwner(ROOT_PATH, user.email())); + assertThrows( + AuthException.class, () -> checkCodeOwner(ROOT_PATH, user.email(), admin.email())); assertThat(authException) .hasMessageThat() .isEqualTo( - String.format("%s for plugin code-owners not permitted", CheckCodeOwnerCapability.ID)); + String.format( + "%s for plugin code-owners not permitted: " + + "cannot specify a user to check a code owner on behalf of this user", + CheckCodeOwnerCapability.ID)); + } + + @Test + public void adminMessagesAreNotReturnedForNormalUser() throws Exception { + TestAccount codeOwner = + accountCreator.create( + "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null); + String secondaryEmail = "codeOwnerSecondary@example.com"; + accountOperations + .account(codeOwner.id()) + .forUpdate() + .addSecondaryEmail(secondaryEmail) + .update(); + + setAsRootCodeOwners(secondaryEmail); + + CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, secondaryEmail, user.email()); + assertThat(checkCodeOwnerInfo) + .hasDebugLogsThatContainAllOf( + String.format( + "cannot resolve code owner email %s: account %s is referenced by secondary email but user %s cannot see secondary emails", + secondaryEmail, codeOwner.id(), user.username())); + + requestScopeOperations.setApiUser(user.id()); + checkCodeOwnerInfo = checkCodeOwner(ROOT_PATH, secondaryEmail); + + // For a non-admin the message doesn't reveal that the email exists as a secondary email that is + // not visible to the user. + assertThat(checkCodeOwnerInfo) + .hasDebugLogsThatContainAllOf( + String.format( + "cannot resolve code owner email %s: email doesn't exist or is not visible", + secondaryEmail)); } @Test
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md index 015cb9e..d1e559f 100644 --- a/resources/Documentation/rest-api.md +++ b/resources/Documentation/rest-api.md
@@ -269,18 +269,11 @@ | `email` | mandatory | Email for which the code ownership should be checked. | `path` | mandatory | Path for which the code ownership should be checked. | `change` | optional | Change for which permissions should be checked. If not specified change permissions are not checked. -| `user` | optional | User for which the code owner visibility should be checked. If not specified the code owner visibility is not checked. Can be used to investigate why a code owner is not shown/suggested to this user. - -Requires that the caller has the [Check Code Owner](#checkCodeOwner) or the -[Administrate Server](../../../Documentation/access-control.html#capability_administrateServer) -global capability. +| `user` | optional | User for which the code owner visibility should be checked. Can be used to investigate why a code owner is not shown/suggested to this user. Requires that the caller has the [Check Code Owner](#checkCodeOwner) or the [Administrate Server](../../../Documentation/access-control.html#capability_administrateServer) global capability. If not specified the code owner visibility is checked for the calling user. This REST endpoint is intended to investigate code owner configurations that do not work as intended. The response contains debug logs that may point out issues -with the code owner configuration. For example, with this REST endpoint it is -possible to find out why a certain email that is listed as code owner in a code -owner config file is ignored (e.g. because it is ambiguous or because it belongs -to an inactive account). +with the code owner configuration. #### Request @@ -990,7 +983,7 @@ | `is_global_code_owner` | Whether the given email is configured as a global code owner. Note that if the email is configured as global code owner, but the email is not resolvable (see `is_resolvable` field), the user is not a code owner. | `is_owned_by_all_users` | Whether the the specified path in the branch is owned by all users (aka `*`). | `annotation` | Annotations that were set for the user. Contains only supported annotations (unsupported annotations are reported in the `debugs_logs`). Sorted alphabetically. -| `debug_logs` | List of debug logs that may help to understand why the user is or isn't a code owner. This information is purely for debugging and the output may be changed at any time. This means bot callers must not parse the debug logs. +| `debug_logs` | List of debug logs that may help to understand why the user is or isn't a code owner. Full debug logs are only returned for callers that have the [Check Code Owner](#checkCodeOwner) or the [Administrate Server](../../../Documentation/access-control.html#capability_administrateServer) global capability, for other callers the debug logs are limited. This information is purely for debugging and the output may be changed at any time. This means bot callers must not parse the debug logs. ---