Test anonymous calls to REST endpoints and make the behavior consistent
In Gerrit core REST endpoints that require authentication reject
anonymous requests with "Authentication required". Do this for the check
REST endpoints too to be consistent. This message is nice for users
which have the needed permissions but just forget to authenticate (e.g.
that forget the '/a' prefix in the URL).
Checks can be accessed anonymously if the change is visible to anonymous
users.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ic88f05530091e25b63a4a925488857da46109a87
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckersCollection.java b/java/com/google/gerrit/plugins/checks/api/CheckersCollection.java
index aa22d90..9d596ba 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckersCollection.java
+++ b/java/com/google/gerrit/plugins/checks/api/CheckersCollection.java
@@ -25,7 +25,6 @@
import com.google.gerrit.plugins.checks.AdministrateCheckersPermission;
import com.google.gerrit.plugins.checks.Checker;
import com.google.gerrit.plugins.checks.Checkers;
-import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -69,13 +68,9 @@
public CheckerResource parse(TopLevelResource parent, IdString id)
throws AuthException, ResourceNotFoundException, PermissionBackendException, IOException,
ConfigInvalidException {
- CurrentUser user = self.get();
- if (user instanceof AnonymousUser) {
+ if (!self.get().isIdentifiedUser()) {
throw new AuthException("Authentication required");
- } else if (!(user.isIdentifiedUser())) {
- throw new ResourceNotFoundException(id);
}
-
permissionBackend.currentUser().check(permission);
Checker checker =
diff --git a/java/com/google/gerrit/plugins/checks/api/CreateChecker.java b/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
index 8bea2f3..7bf61af 100644
--- a/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
+++ b/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
@@ -16,6 +16,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSortedSet;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
@@ -34,6 +35,7 @@
import com.google.gerrit.plugins.checks.CheckersUpdate;
import com.google.gerrit.plugins.checks.UrlValidator;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.UserInitiated;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -49,6 +51,7 @@
@Singleton
public class CreateChecker
implements RestCollectionModifyView<TopLevelResource, CheckerResource, CheckerInput> {
+ private final Provider<CurrentUser> self;
private final PermissionBackend permissionBackend;
private final Provider<CheckersUpdate> checkersUpdate;
private final CheckerJson checkerJson;
@@ -57,11 +60,13 @@
@Inject
public CreateChecker(
+ Provider<CurrentUser> self,
PermissionBackend permissionBackend,
@UserInitiated Provider<CheckersUpdate> checkersUpdate,
CheckerJson checkerJson,
AdministrateCheckersPermission permission,
ProjectCache projectCache) {
+ this.self = self;
this.permissionBackend = permissionBackend;
this.checkersUpdate = checkersUpdate;
this.checkerJson = checkerJson;
@@ -72,6 +77,9 @@
@Override
public Response<CheckerInfo> apply(TopLevelResource parentResource, CheckerInput input)
throws RestApiException, PermissionBackendException, IOException, ConfigInvalidException {
+ if (!self.get().isIdentifiedUser()) {
+ throw new AuthException("Authentication required");
+ }
permissionBackend.currentUser().check(permission);
if (input == null) {
diff --git a/java/com/google/gerrit/plugins/checks/api/ListCheckers.java b/java/com/google/gerrit/plugins/checks/api/ListCheckers.java
index a912d6e..0328a99 100644
--- a/java/com/google/gerrit/plugins/checks/api/ListCheckers.java
+++ b/java/com/google/gerrit/plugins/checks/api/ListCheckers.java
@@ -16,21 +16,25 @@
import static java.util.stream.Collectors.toList;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.plugins.checks.AdministrateCheckersPermission;
import com.google.gerrit.plugins.checks.CheckerJson;
import com.google.gerrit.plugins.checks.Checkers;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
@Singleton
public class ListCheckers implements RestReadView<TopLevelResource> {
+ private final Provider<CurrentUser> self;
private final PermissionBackend permissionBackend;
private final Checkers checkers;
private final CheckerJson checkerJson;
@@ -38,10 +42,12 @@
@Inject
public ListCheckers(
+ Provider<CurrentUser> self,
PermissionBackend permissionBackend,
Checkers checkers,
CheckerJson checkerJson,
AdministrateCheckersPermission permission) {
+ this.self = self;
this.permissionBackend = permissionBackend;
this.checkers = checkers;
this.checkerJson = checkerJson;
@@ -51,6 +57,9 @@
@Override
public List<CheckerInfo> apply(TopLevelResource resource)
throws RestApiException, PermissionBackendException, IOException {
+ if (!self.get().isIdentifiedUser()) {
+ throw new AuthException("Authentication required");
+ }
permissionBackend.currentUser().check(permission);
return checkers.listCheckers().stream().map(checkerJson::format).collect(toList());
diff --git a/java/com/google/gerrit/plugins/checks/api/PostCheck.java b/java/com/google/gerrit/plugins/checks/api/PostCheck.java
index 1d8f1e8..1a1d1c6 100644
--- a/java/com/google/gerrit/plugins/checks/api/PostCheck.java
+++ b/java/com/google/gerrit/plugins/checks/api/PostCheck.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.checks.api;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestCollectionModifyView;
@@ -29,6 +30,7 @@
import com.google.gerrit.plugins.checks.Checks.GetCheckOptions;
import com.google.gerrit.plugins.checks.ChecksUpdate;
import com.google.gerrit.plugins.checks.UrlValidator;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.UserInitiated;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -44,6 +46,7 @@
@Singleton
public class PostCheck
implements RestCollectionModifyView<RevisionResource, CheckResource, CheckInput> {
+ private final Provider<CurrentUser> self;
private final PermissionBackend permissionBackend;
private final AdministrateCheckersPermission permission;
private final Checkers checkers;
@@ -53,12 +56,14 @@
@Inject
PostCheck(
+ Provider<CurrentUser> self,
PermissionBackend permissionBackend,
AdministrateCheckersPermission permission,
Checkers checkers,
Checks checks,
@UserInitiated Provider<ChecksUpdate> checksUpdate,
CheckJson.Factory checkJsonFactory) {
+ this.self = self;
this.permissionBackend = permissionBackend;
this.permission = permission;
this.checkers = checkers;
@@ -81,6 +86,9 @@
throw new BadRequestException(String.format("invalid checker UUID: %s", input.checkerUuid));
}
+ if (!self.get().isIdentifiedUser()) {
+ throw new AuthException("Authentication required");
+ }
permissionBackend.currentUser().check(permission);
CheckerUuid checkerUuid = CheckerUuid.parse(input.checkerUuid);
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
index 93bbdd4..9c68e13 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
@@ -315,6 +315,21 @@
checksApiFactory.revision(patchSetId).create(input);
}
+ @Test
+ public void cannotCreateCheckAnonymously() throws Exception {
+ requestScopeOperations.setApiUserAnonymous();
+
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid.get();
+ input.state = CheckState.RUNNING;
+
+ exception.expect(AuthException.class);
+ exception.expectMessage("Authentication required");
+ checksApiFactory.revision(patchSetId).create(input);
+ }
+
// TODO(gerrit-team) More tests, especially for multiple checkers and PS and how commits behave
private Check getCheck(Project.NameKey project, PatchSet.Id patchSetId, CheckerUuid checkerUuid)
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
index 98a04b8..1733f57 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
@@ -430,4 +430,17 @@
exception.expectMessage("administrateCheckers for plugin checks not permitted");
checkersApi.create(input);
}
+
+ @Test
+ public void createCheckerAnonymouslyFails() throws Exception {
+ requestScopeOperations.setApiUserAnonymous();
+
+ CheckerInput input = new CheckerInput();
+ input.uuid = "test:my-checker";
+ input.repository = allProjects.get();
+
+ exception.expect(AuthException.class);
+ exception.expectMessage("Authentication required");
+ checkersApi.create(input);
+ }
}
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 a522d7b..980aece 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -441,6 +441,16 @@
}
@Test
+ public void getCheckAnonymously() throws Exception {
+ requestScopeOperations.setApiUserAnonymous();
+
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations.newCheck(CheckKey.create(project, patchSetId, checkerUuid)).upsert();
+
+ assertThat(getCheckInfo(patchSetId, checkerUuid)).isNotNull();
+ }
+
+ @Test
public void checkForDeletedChangeDoesNotExist() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
index de28631..32f4649 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
@@ -234,6 +234,18 @@
}
@Test
+ public void getCheckerAnonymouslyFails() throws Exception {
+ String name = "my-checker";
+ CheckerUuid checkerUuid = checkerOperations.newChecker().name(name).create();
+
+ requestScopeOperations.setApiUserAnonymous();
+
+ exception.expect(AuthException.class);
+ exception.expectMessage("Authentication required");
+ getCheckerInfo(checkerUuid);
+ }
+
+ @Test
public void administrateCheckersCapabilityIsAdvertised() throws Exception {
Map<String, CapabilityInfo> capabilities = listCapabilities.apply(new ConfigResource());
String capability = "checks-administrateCheckers";
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
index 52c7334..33d5733 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
@@ -72,6 +72,20 @@
}
@Test
+ public void listCheckersAnonymouslyFails() throws Exception {
+ checkerOperations.newChecker().name("my-checker").create();
+
+ requestScopeOperations.setApiUserAnonymous();
+
+ try {
+ checkersApi.all();
+ assert_().fail("expected AuthException");
+ } catch (AuthException e) {
+ assertThat(e.getMessage()).isEqualTo("Authentication required");
+ }
+ }
+
+ @Test
public void listIgnoresInvalidCheckers() throws Exception {
CheckerUuid checkerUuid =
checkerOperations.newChecker().name("checker-with-name-only").create();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
index 3b3012a..8530208 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -21,6 +21,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckerUuid;
@@ -32,6 +33,7 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.inject.Inject;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.List;
@@ -40,6 +42,8 @@
import org.junit.Test;
public class ListChecksIT extends AbstractCheckersTest {
+ @Inject private RequestScopeOperations requestScopeOperations;
+
private String changeId;
private PatchSet.Id patchSetId;
@@ -288,6 +292,26 @@
assertThat(checksApiFactory.revision(currentPatchSet).list()).hasSize(1);
}
+ @Test
+ public void listAllWithoutAdministrateCheckers() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations.newCheck(CheckKey.create(project, patchSetId, checkerUuid)).upsert();
+
+ requestScopeOperations.setApiUser(user.getId());
+
+ assertThat(checksApiFactory.revision(patchSetId).list()).hasSize(1);
+ }
+
+ @Test
+ public void listAllAnonymously() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations.newCheck(CheckKey.create(project, patchSetId, checkerUuid)).upsert();
+
+ requestScopeOperations.setApiUserAnonymous();
+
+ assertThat(checksApiFactory.revision(patchSetId).list()).hasSize(1);
+ }
+
private Timestamp getPatchSetCreated(Change.Id changeId) throws RestApiException {
return getOnlyElement(
gApi.changes().id(changeId.get()).get(CURRENT_REVISION).revisions.values())
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/QueryPendingChecksIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/QueryPendingChecksIT.java
index b5c9978..32d41ee 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/QueryPendingChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/QueryPendingChecksIT.java
@@ -519,6 +519,26 @@
}
@Test
+ public void queryPendingChecksAnonymouslyWorks() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ requestScopeOperations.setApiUserAnonymous();
+ List<PendingChecksInfo> pendingChecksList =
+ queryPendingChecks(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(1);
+ PendingChecksInfo pendingChecks = Iterables.getOnlyElement(pendingChecksList);
+ assertThat(pendingChecks).hasRepository(project);
+ assertThat(pendingChecks).hasPatchSet(patchSetId);
+ assertThat(pendingChecks)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.get(), new PendingCheckInfo(CheckState.NOT_STARTED));
+ }
+
+ @Test
public void pendingChecksDontIncludeChecksForNonVisibleChanges() throws Exception {
// restrict project visibility so that it is only visible to administrators
try (ProjectConfigUpdate u = updateProject(project)) {
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
index debef5f..6e4dd2e 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
@@ -258,6 +258,15 @@
}
@Test
+ public void cannotUpdateCheckAnonymously() throws Exception {
+ requestScopeOperations.setApiUserAnonymous();
+
+ exception.expect(AuthException.class);
+ exception.expectMessage("Authentication required");
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(new CheckInput());
+ }
+
+ @Test
public void otherPropertiesCanBeSetWithoutEverSettingTheState() throws Exception {
// Create a new checker so that we know for sure that no other update ever happened for it.
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
index da9025a..a0f2663 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
@@ -551,4 +551,18 @@
exception.expectMessage("administrateCheckers for plugin checks not permitted");
checkersApi.id(checkerUuid).update(input);
}
+
+ @Test
+ public void updateCheckerAnonymouslyFails() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+
+ requestScopeOperations.setApiUserAnonymous();
+
+ CheckerInput input = new CheckerInput();
+ input.name = "my-renamed-checker";
+
+ exception.expect(AuthException.class);
+ exception.expectMessage("Authentication required");
+ checkersApi.id(checkerUuid).update(input);
+ }
}
