Do not require Administrate Checkers to view checks on a change
ChecksImpl#id(CheckerUuid) used the checkers API to verify that a
checker exists. This is bad because the checkers API requires the
Administrate Checkers capability to view checkers. Consequently viewing
checks of a change required this capability too.
Requiring Administrate Checkers for viewing checks on a visible change
was not intended. To fix this use the internal API to verify the checker
existence instead.
This also improves the error message when asking for checks of a
non-existing checker.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I883bf7d439e294ecd5d4e00d85a79b000d5b717e
diff --git a/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java b/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
index 361efe0..c75a4d7 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
@@ -22,6 +22,7 @@
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.Checks;
import com.google.gerrit.plugins.checks.PostCheck;
import com.google.gerrit.server.change.RevisionResource;
@@ -60,8 +61,21 @@
@Override
public CheckApi id(CheckerUuid checkerUuid) throws RestApiException {
- // Ensure that the checker exists and throw a RestApiException if not.
- checkers.id(checkerUuid).get();
+ // Ensure that the checker exists.
+ // TODO(gerrit-team): We are not asking for any permission for resolving the checker. This means
+ // users can probe which checkers exist. That's okay for now. The only permission that we have
+ // and that we could check is the 'Administrate Checkers' permission, but this permission should
+ // not be required for viewing checks on a change that is visible to the user.
+ try {
+ checkers
+ .getChecker(checkerUuid)
+ .orElseThrow(
+ () ->
+ new ResourceNotFoundException(
+ String.format("Checker %s not found", checkerUuid)));
+ } catch (Exception e) {
+ throw asRestApiException("Cannot retrieve checker", e);
+ }
try {
CheckKey checkKey =
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
index d16e850..c3f8edd 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -26,10 +27,13 @@
import com.google.gerrit.plugins.checks.api.CheckState;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.inject.Inject;
import org.junit.Before;
import org.junit.Test;
public class GetCheckIT extends AbstractCheckersTest {
+ @Inject private RequestScopeOperations requestScopeOperations;
+
private PatchSet.Id patchSetId;
@Before
@@ -92,14 +96,14 @@
checkerOperations.checker(checkerUuid).forUpdate().forceInvalidConfig().update();
exception.expect(RestApiException.class);
- exception.expectMessage("Cannot retrieve checker " + checkerUuid);
+ exception.expectMessage("Cannot retrieve checker");
checksApiFactory.revision(patchSetId).id(checkerUuid.toString());
}
@Test
public void getCheckForNonExistingChecker() throws Exception {
exception.expect(ResourceNotFoundException.class);
- exception.expectMessage("Not found: test:non-existing");
+ exception.expectMessage("Checker test:non-existing not found");
checksApiFactory.revision(patchSetId).id("test:non-existing");
}
@@ -109,4 +113,17 @@
exception.expectMessage("invalid checker UUID: malformed::checker*UUID");
checksApiFactory.revision(patchSetId).id("malformed::checker*UUID");
}
+
+ @Test
+ public void getCheckWithoutAdministrateCheckers() throws Exception {
+ requestScopeOperations.setApiUser(user.getId());
+
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).setState(CheckState.RUNNING).upsert();
+
+ CheckInfo checkInfo = checksApiFactory.revision(patchSetId).id(checkerUuid).get();
+ assertThat(checkInfo).isEqualTo(checkOperations.check(checkKey).asInfo());
+ }
}
