From 0b2bf619e8b8e48fe4237cb6cdc29f0cd3d820c0 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Thu, 24 Dec 2020 00:15:32 +0100 Subject: 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 --- Documentation/rest-api-changes.txt | 6 ++ .../gerrit/server/restapi/change/QueryChanges.java | 24 +++++- .../acceptance/api/change/QueryChangesIT.java | 88 ++++++++++++++++++++++ 3 files changed, 117 insertions(+), 1 deletion(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 3cb40f6a0d..f67670b893 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -138,6 +138,12 @@ limit or a supplied `n` query parameter, the last change object has a 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 6e5f554304..bf4d197147 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.extensions.restapi.TopLevelResource; 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 @@ public class QueryChanges implements RestReadView, DynamicOpti private final ChangeJson.Factory json; private final ChangeQueryBuilder qb; private final ChangeQueryProcessor imp; + private final Provider userProvider; + private final PermissionBackend permissionBackend; private EnumSet options; @Option( @@ -88,16 +94,32 @@ public class QueryChanges implements RestReadView, DynamicOpti 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 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 78354d653b..6839b967fc 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 queryChangesProvider; + @Inject private RequestScopeOperations requestScopeOperations; @Test @SuppressWarnings("unchecked") @@ -97,9 +107,87 @@ public class QueryChangesIT extends AbstractDaemonTest { 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> result = + (List>) 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> result2 = + (List>) queryChanges.apply(TopLevelResource.INSTANCE).value(); + assertThat(result2).hasSize(0); + + queryChanges = queryChangesProvider.get(); + queryChanges.addQuery("is:open repo:" + project.get()); + queryChanges.skipVisibility(true); + List> result3 = + (List>) queryChanges.apply(TopLevelResource.INSTANCE).value(); + assertThat(result3).hasSize(1); + } + + @Test + @SuppressWarnings("unchecked") + public void skipVisibility_privateChange() throws Exception { + TestRepository 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> result2 = + (List>) queryChanges.apply(TopLevelResource.INSTANCE).value(); + assertThat(result2).hasSize(0); + + queryChanges = queryChangesProvider.get(); + queryChanges.addQuery("is:open repo:" + project.get()); + queryChanges.skipVisibility(true); + List> result3 = + (List>) queryChanges.apply(TopLevelResource.INSTANCE).value(); + assertThat(result3).hasSize(1); + } + private static void assertNoChangeHasMoreChangesSet(List 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)); + } } -- cgit v1.2.3