Remove authorisation from PullReplicationFilter Assuming that the caller of the pull-replication APIs is an identified user is not something that the filter should do: Gerrit has already authentication defined in its filters and we should rely on them. Without the check of identified user, the pul-replication must trust internal users to call its own APIs, therefore add the condition of being an internal user in the ACL evaluation. Bug: Issue 16298 Change-Id: I5660b08fe1f8abeceefbc454b3fd7da6600e67ee
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchPreconditions.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchPreconditions.java index ca1557a..161bcf4 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchPreconditions.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchPreconditions.java
@@ -40,8 +40,10 @@ } public Boolean canCallFetchApi() { - PermissionBackend.WithUser userPermission = permissionBackend.user(userProvider.get()); - return userPermission.testOrFalse(GlobalPermission.ADMINISTRATE_SERVER) + CurrentUser currentUser = userProvider.get(); + PermissionBackend.WithUser userPermission = permissionBackend.user(currentUser); + return currentUser.isInternalUser() + || userPermission.testOrFalse(GlobalPermission.ADMINISTRATE_SERVER) || userPermission.testOrFalse(new PluginPermission(pluginName, CALL_FETCH_ACTION)); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilter.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilter.java index d2bca2b..8c47801 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilter.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilter.java
@@ -23,7 +23,6 @@ import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import static javax.servlet.http.HttpServletResponse.SC_OK; -import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import com.google.common.flogger.FluentLogger; import com.google.gerrit.extensions.annotations.PluginName; @@ -40,7 +39,6 @@ import com.google.gerrit.httpd.AllRequestFilter; import com.google.gerrit.httpd.restapi.RestApiServlet; import com.google.gerrit.json.OutputFormat; -import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ProjectResource; import com.google.gerrit.server.restapi.project.ProjectsCollection; @@ -49,7 +47,6 @@ import com.google.gson.stream.JsonReader; import com.google.gson.stream.MalformedJsonException; import com.google.inject.Inject; -import com.google.inject.Provider; import com.google.inject.TypeLiteral; import com.googlesource.gerrit.plugins.replication.pull.api.FetchAction.Input; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionInput; @@ -86,7 +83,6 @@ private ProjectDeletionAction projectDeletionAction; private ProjectsCollection projectsCollection; private Gson gson; - private Provider<CurrentUser> userProvider; private String pluginName; @Inject @@ -98,7 +94,6 @@ UpdateHeadAction updateHEADAction, ProjectDeletionAction projectDeletionAction, ProjectsCollection projectsCollection, - Provider<CurrentUser> userProvider, @PluginName String pluginName) { this.fetchAction = fetchAction; this.applyObjectAction = applyObjectAction; @@ -107,7 +102,6 @@ this.updateHEADAction = updateHEADAction; this.projectDeletionAction = projectDeletionAction; this.projectsCollection = projectsCollection; - this.userProvider = userProvider; this.pluginName = pluginName; this.gson = OutputFormat.JSON.newGsonBuilder().create(); } @@ -124,44 +118,20 @@ HttpServletRequest httpRequest = (HttpServletRequest) request; try { if (isFetchAction(httpRequest)) { - if (userProvider.get().isIdentifiedUser()) { - writeResponse(httpResponse, doFetch(httpRequest)); - } else { - httpResponse.sendError(SC_UNAUTHORIZED); - } + writeResponse(httpResponse, doFetch(httpRequest)); } else if (isApplyObjectAction(httpRequest)) { - if (userProvider.get().isIdentifiedUser()) { - writeResponse(httpResponse, doApplyObject(httpRequest)); - } else { - httpResponse.sendError(SC_UNAUTHORIZED); - } + writeResponse(httpResponse, doApplyObject(httpRequest)); } else if (isApplyObjectsAction(httpRequest)) { - if (userProvider.get().isIdentifiedUser()) { - writeResponse(httpResponse, doApplyObjects(httpRequest)); - } else { - httpResponse.sendError(SC_UNAUTHORIZED); - } + writeResponse(httpResponse, doApplyObjects(httpRequest)); } else if (isInitProjectAction(httpRequest)) { - if (userProvider.get().isIdentifiedUser()) { - if (!checkAcceptHeader(httpRequest, httpResponse)) { - return; - } - doInitProject(httpRequest, httpResponse); - } else { - httpResponse.sendError(SC_UNAUTHORIZED); + if (!checkAcceptHeader(httpRequest, httpResponse)) { + return; } + doInitProject(httpRequest, httpResponse); } else if (isUpdateHEADAction(httpRequest)) { - if (userProvider.get().isIdentifiedUser()) { - writeResponse(httpResponse, doUpdateHEAD(httpRequest)); - } else { - httpResponse.sendError(SC_UNAUTHORIZED); - } + writeResponse(httpResponse, doUpdateHEAD(httpRequest)); } else if (isDeleteProjectAction(httpRequest)) { - if (userProvider.get().isIdentifiedUser()) { - writeResponse(httpResponse, doDeleteProject(httpRequest)); - } else { - httpResponse.sendError(SC_UNAUTHORIZED); - } + writeResponse(httpResponse, doDeleteProject(httpRequest)); } else { chain.doFilter(request, response); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionIT.java index 0de3d03..a5fd63c 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionIT.java
@@ -120,7 +120,7 @@ @Test @GerritConfig(name = "container.replica", value = "true") - public void shouldReturnUnauthorizedWhenNodeIsAReplicaAndUSerIsAnonymous() throws Exception { + public void shouldReturnForbiddenWhenNodeIsAReplicaAndUSerIsAnonymous() throws Exception { String payloadWithoutAsyncFieldTemplate = "{\"label\":\"" + TEST_REPLICATION_REMOTE @@ -136,7 +136,7 @@ httpClientFactory .create(source) - .execute(createRequest(sendObjectPayload), assertHttpResponseCode(401)); + .execute(createRequest(sendObjectPayload), assertHttpResponseCode(403)); } @Test
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchActionIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchActionIT.java index 36f4361..5634b2a 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchActionIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchActionIT.java
@@ -64,7 +64,7 @@ @Test @GerritConfig(name = "container.replica", value = "true") - public void shouldReturnUnauthorizedWhenNodeIsAReplicaAndUSerIsAnonymous() throws Exception { + public void shouldReturnForbiddenWhenNodeIsAReplicaAndUSerIsAnonymous() throws Exception { String refName = createRef(); String sendObjectPayload = "{\"label\":\"" @@ -75,7 +75,7 @@ httpClientFactory .create(source) - .execute(createRequest(sendObjectPayload), assertHttpResponseCode(401)); + .execute(createRequest(sendObjectPayload), assertHttpResponseCode(403)); } @Override
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ProjectDeletionActionIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ProjectDeletionActionIT.java index 1a37517..2f61cff 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ProjectDeletionActionIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ProjectDeletionActionIT.java
@@ -87,11 +87,10 @@ @Test @GerritConfig(name = "container.replica", value = "true") - public void shouldReturnUnauthorizedForUserWithoutPermissionsOnReplica() throws Exception { + public void shouldReturnForbiddenForUserWithoutPermissionsOnReplica() throws Exception { httpClientFactory .create(source) - .execute( - createDeleteRequest(), assertHttpResponseCode(HttpServletResponse.SC_UNAUTHORIZED)); + .execute(createDeleteRequest(), assertHttpResponseCode(HttpServletResponse.SC_FORBIDDEN)); } @Test
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ProjectInitializationActionIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ProjectInitializationActionIT.java index 8bcb9ea..d1242a0 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ProjectInitializationActionIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ProjectInitializationActionIT.java
@@ -173,13 +173,12 @@ @Test @GerritConfig(name = "container.replica", value = "true") - public void shouldReturnUnauthorizedForUserWithoutPermissionsWhenNodeIsAReplica() - throws Exception { + public void shouldReturnForbiddenForUserWithoutPermissionsWhenNodeIsAReplica() throws Exception { httpClientFactory .create(source) .execute( createPutRequestWithHeaders(), - assertHttpResponseCode(HttpServletResponse.SC_UNAUTHORIZED)); + assertHttpResponseCode(HttpServletResponse.SC_FORBIDDEN)); } @Override
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilterTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilterTest.java index da2d4a5..2ed1466 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilterTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilterTest.java
@@ -12,10 +12,8 @@ import com.google.common.net.MediaType; import com.google.gerrit.extensions.restapi.*; -import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.project.ProjectResource; import com.google.gerrit.server.restapi.project.ProjectsCollection; -import com.google.inject.Provider; import java.io.*; import java.nio.charset.StandardCharsets; import javax.servlet.FilterChain; @@ -40,8 +38,6 @@ @Mock private UpdateHeadAction updateHEADAction; @Mock private ProjectDeletionAction projectDeletionAction; @Mock private ProjectsCollection projectsCollection; - @Mock private CurrentUser currentUser; - @Mock private Provider<CurrentUser> userProvider; @Mock private ProjectResource projectResource; @Mock private ServletOutputStream outputStream; @Mock private PrintWriter printWriter; @@ -71,14 +67,11 @@ updateHEADAction, projectDeletionAction, projectsCollection, - userProvider, PLUGIN_NAME); } private void defineBehaviours(byte[] payload, String uri) throws Exception { when(request.getRequestURI()).thenReturn(uri); - when(userProvider.get()).thenReturn(currentUser); - when(currentUser.isIdentifiedUser()).thenReturn(true); InputStream is = new ByteArrayInputStream(payload); BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(is)); when(request.getReader()).thenReturn(bufferedReader); @@ -89,8 +82,6 @@ private void verifyBehaviours() throws Exception { verify(request, atLeastOnce()).getRequestURI(); - verify(userProvider).get(); - verify(currentUser).isIdentifiedUser(); verify(request).getReader(); verify(projectsCollection).parse(TopLevelResource.INSTANCE, IdString.fromDecoded(PROJECT_NAME)); verify(response).getWriter(); @@ -168,8 +159,6 @@ when(request.getRequestURI()).thenReturn(INIT_PROJECT_URI); when(request.getHeader(ACCEPT)).thenReturn(MediaType.PLAIN_TEXT_UTF_8.toString()); - when(userProvider.get()).thenReturn(currentUser); - when(currentUser.isIdentifiedUser()).thenReturn(true); when(projectInitializationAction.initProject(PROJECT_NAME_GIT)).thenReturn(true); when(response.getWriter()).thenReturn(printWriter); @@ -177,8 +166,6 @@ pullReplicationFilter.doFilter(request, response, filterChain); verify(request, times(5)).getRequestURI(); - verify(userProvider).get(); - verify(currentUser).isIdentifiedUser(); verify(projectInitializationAction).initProject(eq(PROJECT_NAME_GIT)); verify(response).getWriter(); } @@ -202,8 +189,6 @@ public void shouldFilterProjectDeletionAction() throws Exception { when(request.getRequestURI()).thenReturn(DELETE_PROJECT_URI); when(request.getMethod()).thenReturn("DELETE"); - when(userProvider.get()).thenReturn(currentUser); - when(currentUser.isIdentifiedUser()).thenReturn(true); when(projectsCollection.parse(TopLevelResource.INSTANCE, IdString.fromDecoded(PROJECT_NAME))) .thenReturn(projectResource); when(projectDeletionAction.apply(any(), any())).thenReturn(OK_RESPONSE); @@ -213,8 +198,6 @@ pullReplicationFilter.doFilter(request, response, filterChain); verify(request, times(7)).getRequestURI(); - verify(userProvider).get(); - verify(currentUser).isIdentifiedUser(); verify(projectsCollection).parse(TopLevelResource.INSTANCE, IdString.fromDecoded(PROJECT_NAME)); verify(projectDeletionAction).apply(eq(projectResource), any()); verify(response).getWriter(); @@ -237,8 +220,6 @@ BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(is)); when(request.getRequestURI()).thenReturn(FETCH_URI); when(request.getReader()).thenReturn(bufferedReader); - when(userProvider.get()).thenReturn(currentUser); - when(currentUser.isIdentifiedUser()).thenReturn(true); when(response.getOutputStream()).thenReturn(outputStream); PullReplicationFilter pullReplicationFilter = createPullReplicationFilter(); @@ -251,8 +232,6 @@ public void shouldBe500WhenProjectCannotBeInitiated() throws Exception { when(request.getRequestURI()).thenReturn(INIT_PROJECT_URI); when(request.getHeader(ACCEPT)).thenReturn(MediaType.PLAIN_TEXT_UTF_8.toString()); - when(userProvider.get()).thenReturn(currentUser); - when(currentUser.isIdentifiedUser()).thenReturn(true); when(projectInitializationAction.initProject(PROJECT_NAME_GIT)).thenReturn(false); when(response.getOutputStream()).thenReturn(outputStream); @@ -266,8 +245,6 @@ public void shouldBe500WhenResourceNotFound() throws Exception { when(request.getRequestURI()).thenReturn(DELETE_PROJECT_URI); when(request.getMethod()).thenReturn("DELETE"); - when(userProvider.get()).thenReturn(currentUser); - when(currentUser.isIdentifiedUser()).thenReturn(true); when(projectsCollection.parse(TopLevelResource.INSTANCE, IdString.fromDecoded(PROJECT_NAME))) .thenReturn(projectResource); when(projectDeletionAction.apply(any(), any())) @@ -326,8 +303,6 @@ public void shouldBe409WhenThereIsResourceConflict() throws Exception { when(request.getRequestURI()).thenReturn(DELETE_PROJECT_URI); when(request.getMethod()).thenReturn("DELETE"); - when(userProvider.get()).thenReturn(currentUser); - when(currentUser.isIdentifiedUser()).thenReturn(true); when(projectsCollection.parse(TopLevelResource.INSTANCE, IdString.fromDecoded(PROJECT_NAME))) .thenReturn(projectResource); @@ -346,8 +321,6 @@ when(request.getRequestURI()) .thenReturn(String.format("any-prefix/projects/%s~delete-project", PLUGIN_NAME)); when(request.getMethod()).thenReturn("DELETE"); - when(userProvider.get()).thenReturn(currentUser); - when(currentUser.isIdentifiedUser()).thenReturn(true); when(response.getOutputStream()).thenReturn(outputStream); PullReplicationFilter pullReplicationFilter = createPullReplicationFilter();