Add query option allowing administrators to skip visibility filtering
If an administrator wants to reindex changes which were created or
updated in a given period based on a query for that period the
query results are subject to visibility filtering. This can have the
effect that e.g. private changes are missed.
Add a query option "skip-visibility" to allow administrators
to skip visibility filtering.
Change-Id: I66c13659587b9459eb7cc585697c1655926ceac3
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 3cb40f6..f67670b 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -138,6 +138,12 @@
The `S` or `start` query parameter can be supplied to skip a number
of changes from the list.
+Administrators can use the `skip-visibility` query parameter to skip visibility filtering.
+This can be used to ensure that no changes are missed e.g. when querying for changes which
+need to be reindexed. Without this parameter query results the user has no permission to read
+are filtered out. REST requests with the skip-visibility option are rejected when the current
+user doesn't have the ADMINISTRATE_SERVER capability.
+
Clients are allowed to specify more than one query by setting the `q`
parameter multiple times. In this case the result is an array of
arrays, one per query in the same order the queries were given in.
diff --git a/java/com/google/gerrit/server/restapi/change/QueryChanges.java b/java/com/google/gerrit/server/restapi/change/QueryChanges.java
index 6e5f554..bf4d197 100644
--- a/java/com/google/gerrit/server/restapi/change/QueryChanges.java
+++ b/java/com/google/gerrit/server/restapi/change/QueryChanges.java
@@ -27,13 +27,17 @@
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.index.query.QueryRequiresAuthException;
import com.google.gerrit.index.query.QueryResult;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.change.ChangeJson;
+import com.google.gerrit.server.permissions.GlobalPermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ChangeQueryProcessor;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import java.util.ArrayList;
import java.util.Collections;
import java.util.EnumSet;
@@ -46,6 +50,8 @@
private final ChangeJson.Factory json;
private final ChangeQueryBuilder qb;
private final ChangeQueryProcessor imp;
+ private final Provider<CurrentUser> userProvider;
+ private final PermissionBackend permissionBackend;
private EnumSet<ListChangesOption> options;
@Option(
@@ -88,16 +94,32 @@
imp.setNoLimit(on);
}
+ @Option(name = "--skip-visibility", usage = "Skip visibility check, only for administrators")
+ public void skipVisibility(boolean on) throws AuthException, PermissionBackendException {
+ if (on) {
+ CurrentUser user = userProvider.get();
+ permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
+ imp.enforceVisibility(false);
+ }
+ }
+
@Override
public void setDynamicBean(String plugin, DynamicOptions.DynamicBean dynamicBean) {
imp.setDynamicBean(plugin, dynamicBean);
}
@Inject
- QueryChanges(ChangeJson.Factory json, ChangeQueryBuilder qb, ChangeQueryProcessor qp) {
+ QueryChanges(
+ ChangeJson.Factory json,
+ ChangeQueryBuilder qb,
+ ChangeQueryProcessor qp,
+ Provider<CurrentUser> userProvider,
+ PermissionBackend permissionBackend) {
this.json = json;
this.qb = qb;
this.imp = qp;
+ this.userProvider = userProvider;
+ this.permissionBackend = permissionBackend;
options = EnumSet.noneOf(ListChangesOption.class);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
index 78354d6..6839b96 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
@@ -15,22 +15,32 @@
package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.restapi.change.QueryChanges;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import java.util.Arrays;
import java.util.List;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
import org.junit.Test;
@NoHttpd
public class QueryChangesIT extends AbstractDaemonTest {
@Inject private Provider<QueryChanges> queryChangesProvider;
+ @Inject private RequestScopeOperations requestScopeOperations;
@Test
@SuppressWarnings("unchecked")
@@ -97,9 +107,87 @@
assertThat(result2.get(1).get(0)._moreChanges).isTrue();
}
+ @Test
+ public void skipVisibility_rejectedForNonAdmin() throws Exception {
+ requestScopeOperations.setApiUser(user.id());
+ final QueryChanges queryChanges = queryChangesProvider.get();
+ String query = "is:open repo:" + project.get();
+ queryChanges.addQuery(query);
+ AuthException thrown =
+ assertThrows(AuthException.class, () -> queryChanges.skipVisibility(true));
+ assertThat(thrown).hasMessageThat().isEqualTo("administrate server not permitted");
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void skipVisibility_noReadPermission() throws Exception {
+ createChange().getChangeId();
+ requestScopeOperations.setApiUser(admin.id());
+ QueryChanges queryChanges = queryChangesProvider.get();
+
+ queryChanges.addQuery("is:open repo:" + project.get());
+ List<List<ChangeInfo>> result =
+ (List<List<ChangeInfo>>) queryChanges.apply(TopLevelResource.INSTANCE).value();
+ assertThat(result).hasSize(1);
+
+ try (ProjectConfigUpdate u = updateProject(allProjects)) {
+ ProjectConfig cfg = u.getConfig();
+ removeAllBranchPermissions(cfg, Permission.READ);
+ u.save();
+ }
+
+ queryChanges = queryChangesProvider.get();
+ queryChanges.addQuery("is:open repo:" + project.get());
+ List<List<ChangeInfo>> result2 =
+ (List<List<ChangeInfo>>) queryChanges.apply(TopLevelResource.INSTANCE).value();
+ assertThat(result2).hasSize(0);
+
+ queryChanges = queryChangesProvider.get();
+ queryChanges.addQuery("is:open repo:" + project.get());
+ queryChanges.skipVisibility(true);
+ List<List<ChangeInfo>> result3 =
+ (List<List<ChangeInfo>>) queryChanges.apply(TopLevelResource.INSTANCE).value();
+ assertThat(result3).hasSize(1);
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void skipVisibility_privateChange() throws Exception {
+ TestRepository<InMemoryRepository> userRepo = cloneProject(project, user);
+ PushOneCommit.Result result =
+ pushFactory.create(user.newIdent(), userRepo).to("refs/for/master");
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(result.getChangeId()).setPrivate(true);
+
+ requestScopeOperations.setApiUser(admin.id());
+ QueryChanges queryChanges = queryChangesProvider.get();
+
+ queryChanges.addQuery("is:open repo:" + project.get());
+ List<List<ChangeInfo>> result2 =
+ (List<List<ChangeInfo>>) queryChanges.apply(TopLevelResource.INSTANCE).value();
+ assertThat(result2).hasSize(0);
+
+ queryChanges = queryChangesProvider.get();
+ queryChanges.addQuery("is:open repo:" + project.get());
+ queryChanges.skipVisibility(true);
+ List<List<ChangeInfo>> result3 =
+ (List<List<ChangeInfo>>) queryChanges.apply(TopLevelResource.INSTANCE).value();
+ assertThat(result3).hasSize(1);
+ }
+
private static void assertNoChangeHasMoreChangesSet(List<ChangeInfo> results) {
for (ChangeInfo info : results) {
assertThat(info._moreChanges).isNull();
}
}
+
+ private static void removeAllBranchPermissions(ProjectConfig cfg, String... permissions) {
+ cfg.getAccessSections().stream()
+ .filter(
+ s ->
+ s.getName().startsWith("refs/heads/")
+ || s.getName().startsWith("refs/for/")
+ || s.getName().equals("refs/*"))
+ .forEach(s -> Arrays.stream(permissions).forEach(s::removePermission));
+ }
}
