Return ACL info when request is rejected due to a permission issue
For users that have the 'View Access' global capability we return an ACL
info for requests that fail due to permission errors (response code
403). The ACL info informs about the permissions rules that have been
checked. This allows the user to understand due to which permission rule
the request has been rejected.
The 'View Access' global capability already allows to use the Check
Access REST endpoint, hence they already have access to the ACL info and
it's OK to return it in other places too.
Bug: Google b/330836100
Change-Id: I4b7683f6b24056ad633812ae0555e928ffbaf45f
Signed-off-by: Edwin Kempin <ekempin@google.com>
Release-Notes: For users that have the 'View Access' global capability return ACL info when request is rejected due to a permission issue
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index a179905..52b2b18 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -1564,7 +1564,12 @@
Allow checking access rights for arbitrary (user, project) pairs,
using the link:rest-api-projects.html#check-access[check.access]
-endpoint
+endpoint.
+
+In addition, when a request fails due to permission errors and the caller has
+this capability, ACL info is returned that contains information about the
+permissions rules that have been checked. This allows the user to understand
+which permissions rule caused request to be rejected.
[[capability_viewAllAccounts]]
=== View All Accounts
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 41c158f..14334bb 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -89,6 +89,7 @@
import com.google.gerrit.httpd.restapi.ParameterParser.QueryParams;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.server.AccessPath;
+import com.google.gerrit.server.AclInfoController;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CancellationMetrics;
import com.google.gerrit.server.CurrentUser;
@@ -243,6 +244,7 @@
final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
final DeadlineChecker.Factory deadlineCheckerFactory;
final CancellationMetrics cancellationMetrics;
+ final AclInfoController aclInfoController;
@Inject
Globals(
@@ -262,7 +264,8 @@
Injector injector,
DynamicMap<DynamicOptions.DynamicBean> dynamicBeans,
DeadlineChecker.Factory deadlineCheckerFactory,
- CancellationMetrics cancellationMetrics) {
+ CancellationMetrics cancellationMetrics,
+ AclInfoController aclInfoController) {
this.currentUser = currentUser;
this.webSession = webSession;
this.paramParser = paramParser;
@@ -281,6 +284,7 @@
this.dynamicBeans = dynamicBeans;
this.deadlineCheckerFactory = deadlineCheckerFactory;
this.cancellationMetrics = cancellationMetrics;
+ this.aclInfoController = aclInfoController;
}
}
@@ -328,6 +332,8 @@
RequestInfo requestInfo = createRequestInfo(traceContext, req, requestUri, path);
globals.requestListeners.runEach(l -> l.onRequest(requestInfo));
+ globals.aclInfoController.enableAclLoggingIfUserCanViewAccess(traceContext);
+
// It's important that the PerformanceLogContext is closed before the response is sent to
// the client. Only this way it is ensured that the invocation of the PerformanceLogger
// plugins happens before the client sees the response. This is needed for being able to
@@ -629,9 +635,16 @@
req, res, statusCode = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e.caching(), e);
} catch (AuthException e) {
cause = Optional.of(e);
+
+ StringBuilder messageBuilder = new StringBuilder(messageOr(e, "Forbidden"));
+ globals
+ .aclInfoController
+ .getAclInfoMessage()
+ .ifPresent(aclInfo -> messageBuilder.append("\n\n").append(aclInfo));
+
responseBytes =
replyError(
- req, res, statusCode = SC_FORBIDDEN, messageOr(e, "Forbidden"), e.caching(), e);
+ req, res, statusCode = SC_FORBIDDEN, messageBuilder.toString(), e.caching(), e);
} catch (AmbiguousViewException e) {
cause = Optional.of(e);
responseBytes =
diff --git a/java/com/google/gerrit/server/AclInfoController.java b/java/com/google/gerrit/server/AclInfoController.java
new file mode 100644
index 0000000..1563ba3
--- /dev/null
+++ b/java/com/google/gerrit/server/AclInfoController.java
@@ -0,0 +1,70 @@
+// Copyright (C) 2024 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.server.logging.LoggingContext;
+import com.google.gerrit.server.logging.TraceContext;
+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.Singleton;
+import java.util.Optional;
+
+/** Class to control when ACL infos should be collected and be returned to the user. */
+@Singleton
+public class AclInfoController {
+ private final PermissionBackend permissionBackend;
+
+ @Inject
+ AclInfoController(PermissionBackend permissionBackend) {
+ this.permissionBackend = permissionBackend;
+ }
+
+ public void enableAclLoggingIfUserCanViewAccess(TraceContext traceContext)
+ throws PermissionBackendException {
+ if (canViewAclInfos()) {
+ traceContext.enableAclLogging();
+ }
+ }
+
+ /**
+ * Returns message containing the ACL logs that have been collected for the request, {@link
+ * Optional#empty()} if ACL logging hasn't been turned on
+ */
+ public Optional<String> getAclInfoMessage() {
+ // ACL logging is only enabled if the user can view ACL infos. This is checked when ACL logging
+ // is turned on in enableAclLoggingIfUserCanViewAccess. Hence we can return ACL infos if ACL
+ // logging is on and do not need to check the permission again. We want to avoid re-checking the
+ // permission so that we do not need to handle PermissionBackendException.
+ if (!LoggingContext.getInstance().isAclLogging()) {
+ return Optional.empty();
+ }
+
+ ImmutableList<String> aclLogRecords = TraceContext.getAclLogRecords();
+ if (aclLogRecords.isEmpty()) {
+ aclLogRecords = ImmutableList.of("Found no rules that apply, so defaulting to no permission");
+ }
+
+ StringBuilder msgBuilder = new StringBuilder("ACL info:");
+ aclLogRecords.forEach(aclLogRecord -> msgBuilder.append("\n* ").append(aclLogRecord));
+ return Optional.of(msgBuilder.toString());
+ }
+
+ private boolean canViewAclInfos() throws PermissionBackendException {
+ return permissionBackend.currentUser().test(GlobalPermission.VIEW_ACCESS);
+ }
+}
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index a111c5e..7544f95 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -107,6 +107,7 @@
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Field;
import com.google.gerrit.metrics.MetricMaker;
+import com.google.gerrit.server.AclInfoController;
import com.google.gerrit.server.CancellationMetrics;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CreateGroupPermissionSyncer;
@@ -385,6 +386,7 @@
// Injected fields.
private final AccountResolver accountResolver;
+ private final AclInfoController aclInfoController;
private final AllProjectsName allProjectsName;
private final BatchUpdate.Factory batchUpdateFactory;
private final BatchUpdates batchUpdates;
@@ -453,6 +455,8 @@
/** Multimap of error text to refnames that produced that error. */
private final ListMultimap<String, String> errors;
+ private final LinkedHashMap<ReceiveCommand, RejectionReason> rejectionReasons;
+
private final ListMultimap<String, String> pushOptions;
private final ReceivePackRefCache receivePackRefCache;
private final Map<Change.Id, ReplaceRequest> replaceByChange;
@@ -475,6 +479,7 @@
@Inject
ReceiveCommits(
AccountResolver accountResolver,
+ AclInfoController aclInfoController,
AllProjectsName allProjectsName,
BatchUpdate.Factory batchUpdateFactory,
BatchUpdates batchUpdates,
@@ -534,6 +539,7 @@
throws IOException {
// Injected fields.
this.accountResolver = accountResolver;
+ this.aclInfoController = aclInfoController;
this.allProjectsName = allProjectsName;
this.batchUpdateFactory = batchUpdateFactory;
this.batchUpdates = batchUpdates;
@@ -600,6 +606,7 @@
// Collections populated during processing.
errors = MultimapBuilder.linkedHashKeys().arrayListValues().build();
+ rejectionReasons = new LinkedHashMap<>();
messages = new ConcurrentLinkedQueue<>();
pushOptions = LinkedListMultimap.create();
replaceByChange = new LinkedHashMap<>();
@@ -704,6 +711,8 @@
requestListeners.runEach(l -> l.onRequest(requestInfo));
traceContext.addTag(RequestId.Type.RECEIVE_ID, new RequestId(project.getNameKey().get()));
+ aclInfoController.enableAclLoggingIfUserCanViewAccess(traceContext);
+
// Log the push options here, rather than in parsePushOptions(), so that they are included
// into the trace if tracing is enabled.
logger.atFine().log("push options: %s", receivePack.getPushOptions());
@@ -761,10 +770,18 @@
// successfully.
sendErrorMessages();
+ // If there was a permission issue send ACL infos to the client (if ACL logging was turned
+ // on).
+ if (rejectionReasons.values().stream()
+ .map(RejectionReason::statusCode)
+ .anyMatch(statusCode -> statusCode == 403)) {
+ aclInfoController.getAclInfoMessage().ifPresent(this::addMessage);
+ }
+
commandProgress.end();
loggingTags = traceContext.getTags();
logger.atFine().log("Processing commands done.");
- } catch (RuntimeException e) {
+ } catch (PermissionBackendException | RuntimeException e) {
String formattedCause = getFormattedCause(e).orElse(e.getClass().getSimpleName());
int statusCode =
getStatus(e).map(ExceptionHook.Status::statusCode).orElse(SC_INTERNAL_SERVER_ERROR);
@@ -775,7 +792,13 @@
pushKind += " by service user";
}
metrics.rejectCount.increment(pushKind, formattedCause, statusCode);
- throw e;
+
+ // Re-throw any RuntimeException as they are.
+ Throwables.throwIfUnchecked(e);
+
+ // Wrap any checked exception (e.g. PermissionBackendException) into a StorageException, which
+ // is an unchecked exception, as we cannot throw checked exceptions from this method.
+ throw new StorageException(e);
}
progress.end();
return result.build();
@@ -3958,6 +3981,8 @@
metrics.rejectCount.increment(pushKind, reason.metricBucket(), reason.statusCode());
cmd.setResult(REJECTED_OTHER_REASON, reason.why());
+
+ rejectionReasons.put(cmd, reason);
}
private void rejectRemaining(Collection<ReceiveCommand> commands, RejectionReason reason) {
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index aa96d1b..34b858e 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -2673,6 +2673,58 @@
.isEqualTo(Iterables.getLast(commits).name());
}
+ @Test
+ public void aclInfoIsReturnedIfPushFailsDueToAPermissionError() throws Exception {
+ String master = "refs/heads/master";
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.PUSH).ref(master).group(REGISTERED_USERS))
+ .update();
+
+ // without VIEW_ACCESS capability no ACL info is returned
+ TestRepository<?> userRepo = cloneProject(project, user);
+ userRepo
+ .branch("HEAD")
+ .commit()
+ .message("New Commit 1")
+ .author(user.newIdent())
+ .committer(user.newIdent())
+ .insertChangeId()
+ .create();
+ PushResult pushResult = pushHead(userRepo, master);
+ assertPushRejected(pushResult, master, "prohibited by Gerrit: not permitted: update");
+ assertThat(pushResult.getMessages()).doesNotContain("ACL info");
+
+ // with VIEW_ACCESS capability an ACL info is returned when the request fails due to a
+ // permission error
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability(GlobalCapability.VIEW_ACCESS).group(REGISTERED_USERS))
+ .update();
+ pushResult = pushHead(userRepo, master);
+ assertPushRejected(pushResult, master, "prohibited by Gerrit: not permitted: update");
+ assertThat(pushResult.getMessages())
+ .contains(
+ String.format(
+ "ACL info:\n"
+ + "* '%s' cannot perform 'push' with force=false on project '%s'"
+ + " for ref '%s' because this permission is blocked",
+ user.username(), project, master));
+
+ // with VIEW_ACCESS capability no ACL info is returned when the request doesn't fail due to a
+ // permission error
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.PUSH).ref(master).group(REGISTERED_USERS))
+ .update();
+ pushResult = pushHead(userRepo, master);
+ assertPushOk(pushResult, master);
+ assertThat(pushResult.getMessages()).doesNotContain("ACL info");
+ }
+
private static class TestValidator implements CommitValidationListener {
private final AtomicInteger count = new AtomicInteger();
private final boolean validateAll;
diff --git a/javatests/com/google/gerrit/acceptance/rest/AclInfoRestIT.java b/javatests/com/google/gerrit/acceptance/rest/AclInfoRestIT.java
new file mode 100644
index 0000000..24ba1c3
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/AclInfoRestIT.java
@@ -0,0 +1,120 @@
+// Copyright (C) 2024 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.rest;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
+import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.common.data.GlobalCapability;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Permission;
+import com.google.gerrit.extensions.client.Comment;
+import com.google.gerrit.extensions.common.ApplyProvidedFixInput;
+import com.google.gerrit.extensions.common.FixReplacementInfo;
+import com.google.inject.Inject;
+import java.util.Arrays;
+import java.util.List;
+import org.junit.Test;
+
+public class AclInfoRestIT extends AbstractDaemonTest {
+ @Inject private ProjectOperations projectOperations;
+ @Inject private ChangeOperations changeOperations;
+
+ @Test
+ public void cannotApplyProvidedFixlWithoutAddPatchSetPermission() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.ADD_PATCH_SET).ref("refs/*").group(ANONYMOUS_USERS))
+ .update();
+
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file("foo.txt")
+ .content("some content")
+ .create();
+
+ Comment.Range range = new Comment.Range();
+ range.startLine = 1;
+ range.startCharacter = 0;
+ range.endLine = 1;
+ range.endCharacter = 3;
+
+ FixReplacementInfo fixReplacementInfo = new FixReplacementInfo();
+ fixReplacementInfo.path = "foo.txt";
+ fixReplacementInfo.replacement = "other";
+ fixReplacementInfo.range = range;
+
+ List<FixReplacementInfo> fixReplacementInfoList = Arrays.asList(fixReplacementInfo);
+ ApplyProvidedFixInput applyProvidedFixInput = new ApplyProvidedFixInput();
+ applyProvidedFixInput.fixReplacementInfos = fixReplacementInfoList;
+
+ // without VIEW_ACCESS capability no ACL info is returned
+ RestResponse resp =
+ userRestSession.post(
+ "/changes/" + changeId + "/revisions/current/fix:apply", applyProvidedFixInput);
+ resp.assertStatus(403);
+ assertThat(resp.getEntityContent()).isEqualTo("edit not permitted");
+
+ // with VIEW_ACCESS capability an ACL info is returned when the request fails due to a
+ // permission error
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability(GlobalCapability.VIEW_ACCESS).group(REGISTERED_USERS))
+ .update();
+ resp =
+ userRestSession.post(
+ "/changes/" + changeId + "/revisions/current/fix:apply", applyProvidedFixInput);
+ resp.assertStatus(403);
+ assertThat(resp.getEntityContent())
+ .isEqualTo(
+ String.format(
+ "edit not permitted\n\n"
+ + "ACL info:\n"
+ + "* '%s' can perform 'read' with force=false on project '%s'"
+ + " for ref 'refs/heads/master'"
+ + " (allowed for group 'global:Anonymous-Users' by rule 'group Anonymous Users')\n"
+ + "* '%s' can perform 'push' with force=false on project '%s'"
+ + " for ref 'refs/for/refs/heads/master'"
+ + " (allowed for group 'global:Registered-Users' by rule 'group Registered Users')\n"
+ + "* '%s' cannot perform 'addPatchSet' with force=false on project '%s'"
+ + " for ref 'refs/for/refs/heads/master' because this permission is blocked",
+ user.username(), project, user.username(), project, user.username(), project));
+
+ // with VIEW_ACCESS capability no ACL info is returned when the request doesn't fail due to a
+ // permission error
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.ADD_PATCH_SET).ref("refs/*").group(ANONYMOUS_USERS))
+ .update();
+ resp =
+ userRestSession.post(
+ "/changes/" + changeId + "/revisions/current/fix:apply", applyProvidedFixInput);
+ resp.assertOK();
+ assertThat(resp.getEntityContent()).doesNotContain("ACL info");
+ }
+}
