Merge changes Ia973e3df,I331ebacf,I638ddce2,I3a2d5d9c * changes: Check Code Owner screen: Show note to non-admins that debug logs are limited Document Check Code Owner Self Service Adapt screen to check code owners to this being a self service now Allow all users to call the Check Code Owner REST endpoint
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/config-faqs.md b/resources/Documentation/config-faqs.md index be0bfb0..fb6226c 100644 --- a/resources/Documentation/config-faqs.md +++ b/resources/Documentation/config-faqs.md
@@ -88,16 +88,21 @@ * a bug in the @PLUGIN@ plugin Since code owner config files are part of the source code, any issues with them -should be investigated and fixed by the project owners and host administrators. +should be investigated and fixed by the project team, the project owners and +the host administrators. To do this they can: +* Check the code ownership of a user for a certain path by using the [Check Code + Owner Self Service](@URL@/x/code-owners/check-code-owner). This is calling the + [Check Code Owner REST endpoint](rest-api.html#check-code-owner). Any user can + use this self sevice, but for users that have the + [Administrate Server](../../../Documentation/access-control.html#capability_administrateServer) + global capability or the [Check Code Owner](rest-api.html#checkCodeOwner) + global capability the returned debug information (field `debug_logs`) is more + detailed. * Check the code owner config files for issues by calling the [Check Code Owner Config File REST endpoint](rest-api.html#check-code-owner-config-files) -* Check the code ownership of a user for a certain path by calling the [Check - Code Owner REST endpoint](rest-api.html#check-code-owner) (requires the caller - to be host administrator or have the [Check Code Owner - capability](rest-api.html#checkCodeOwner)). Bugs with the @PLUGIN@ plugin should be filed as issues for the Gerrit team, but only after issues with the code owner config files have been excluded. @@ -117,22 +122,28 @@ * a bug in the @PLUGIN@ plugin Issues with code owner config files, user permissions, account visibility and -account states should be investigated and fixed by the project owners and host -administrators. +account states should be investigated and fixed by the project team, the project +owners and the host administrators. To do this they can: +* Check the code ownership of a user for a certain path by using the [Check Code + Owner Self Service](@URL@/x/code-owners/check-code-owner). This is calling the + [Check Code Owner REST endpoint](rest-api.html#check-code-owner). Any user can + use this self sevice, but for users that have the + [Administrate Server](../../../Documentation/access-control.html#capability_administrateServer) + global capability or the [Check Code Owner](rest-api.html#checkCodeOwner) + global capability the returned debug information (field `debug_logs`) is more + detailed. * Use the `--debug` option of the [List Code Owners](rest-api.html#list-code-owners-for-path-in-branch) REST endpoints to - get debug logs included into the response (requires the caller - to be host administrator or have the [Check Code Owner - capability](rest-api.html#checkCodeOwner)). + get debug information (field `debug_logs`) included into the response + (requires the caller to have the [Administrate + Server](../../../Documentation/access-control.html#capability_administrateServer) + global capability or the [Check Code Owner](rest-api.html#checkCodeOwner) + global capability). * Check the code owner config files for issues by calling the [Check Code Owner Config File REST endpoint](rest-api.html#check-code-owner-config-files). -* Check the code ownership of a user for a certain path by calling the [Check - Code Owner REST endpoint](rest-api.html#check-code-owner) (requires the caller - to be host administrator or have the [Check Code Owner - capability](rest-api.html#checkCodeOwner)). Bugs with the @PLUGIN@ plugin should be filed as issues for the Gerrit team, but only after other causes have been excluded.
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md index 015cb9e..ad944aa 100644 --- a/resources/Documentation/rest-api.md +++ b/resources/Documentation/rest-api.md
@@ -269,18 +269,15 @@ | `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. + +This REST endpoint is available as a +[Self Service](@URL@/x/code-owners/check-code-owner) so that users can easily +inspect issues with code ownerships. #### Request @@ -990,7 +987,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. ---
diff --git a/web/gr-check-code-owner.ts b/web/gr-check-code-owner.ts index cba304f..a39a532 100644 --- a/web/gr-check-code-owner.ts +++ b/web/gr-check-code-owner.ts
@@ -17,6 +17,7 @@ import {customElement, query, property, state} from 'lit/decorators'; import {css, CSSResult, html, LitElement} from 'lit'; +import {classMap} from 'lit/directives/class-map.js'; import {PluginApi} from '@gerritcodereview/typescript-api/plugin'; declare global { @@ -25,6 +26,12 @@ } } +// https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#capability-info +export interface AccountCapabilityInfo { + administrateServer: boolean; + 'code-owners-checkCodeOwner': boolean; +} + @customElement('gr-check-code-owner') export class GrCheckCodeOwner extends LitElement { @query('#projectInput') @@ -45,6 +52,9 @@ @query('#resultOutput') resultOutput!: HTMLInputElement; + @query('#noteAboutLimitedDebugInformation') + noteAboutLimitedDebugInformation!: HTMLInputElement; + @property() plugin!: PluginApi; @@ -54,6 +64,9 @@ @state() isChecking = false; + @state() + hasAdminPermissions = false; + static override get styles() { return [ window.Gerrit.styles.font as CSSResult, @@ -70,6 +83,9 @@ .output { min-width: 50em; } + .hidden { + display: none; + } `, ]; } @@ -84,14 +100,7 @@ Checks the code ownership of a user for a path in a branch, see <a href="${window.CANONICAL_PATH || ''}/plugins/code-owners/Documentation/rest-api.html#check-code-owner" target="_blank">documentation<a/>. </p> - <p> - Requires that the caller has the - <a href="${window.CANONICAL_PATH || ''}/plugins/code-owners/Documentation/rest-api.html#checkCodeOwner" target="_blank">Check Code Owner</a> - or the - <a href="${window.CANONICAL_PATH || ''}/Documentation/access-control.html#capability_administrateServer" target="_blank">Administrate Server</a> - global capability. - </p> - <p>All fields, except the 'Calling User' field, are required.</p> + <p>Required fields:</p> <fieldset> <section> <span class="title"> @@ -162,6 +171,13 @@ /> </span> </section> + </fieldset> + <p>Admin options (usage requires having the + <a href="${window.CANONICAL_PATH || ''}/plugins/code-owners/Documentation/rest-api.html#checkCodeOwner" target="_blank">Check Code Owner</a> + or the + <a href="${window.CANONICAL_PATH || ''}/Documentation/access-control.html#capability_administrateServer" target="_blank">Administrate Server</a> + global capability): + <fieldset> <section> <span class="title"> <gr-tooltip-content @@ -175,6 +191,7 @@ <input id="userInput" type="text" + ?disabled=${!this.hasAdminPermissions} @input=${this.validateData} /> </span> @@ -201,10 +218,37 @@ </iron-autogrow-textarea> </span> </section> + <p + class=${classMap({hidden: this.hasAdminPermissions})} + > + Note: The calling user doesn't have the + <a href="${window.CANONICAL_PATH || ''}/plugins/code-owners/Documentation/rest-api.html#checkCodeOwner" target="_blank">Check Code Owner</a> + or the + <a href="${window.CANONICAL_PATH || ''}/Documentation/access-control.html#capability_administrateServer" target="_blank">Administrate Server</a> + global capability, hence the returned debug information (field + 'debug_logs') is limited. If more information is needed, please reach + out to a host administrator to check the code ownership. + </p> </main> `; } + override connectedCallback() { + super.connectedCallback(); + this.checkAdminPermissions(); + } + + private async checkAdminPermissions() { + await this.plugin + .restApi() + .get<AccountCapabilityInfo>('/accounts/self/capabilities/') + .then(capabilities => { + this.hasAdminPermissions = capabilities && + (capabilities['administrateServer'] || + capabilities['code-owners-checkCodeOwner']); + }); + } + private validateData() { this.dataValid = this.validateHasValue(this.projectInput.value) &&