diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD index b590e64242..5e59007bee 100644 --- a/java/com/google/gerrit/acceptance/BUILD +++ b/java/com/google/gerrit/acceptance/BUILD @@ -110,6 +110,7 @@ java_library2( "//java/com/google/gerrit/pgm/init", "//java/com/google/gerrit/reviewdb:server", "//java/com/google/gerrit/server", + "//java/com/google/gerrit/server/audit", "//java/com/google/gerrit/server/git/receive", "//java/com/google/gerrit/server/restapi", "//java/com/google/gerrit/server/schema", diff --git a/java/com/google/gerrit/acceptance/FakeGroupAuditService.java b/java/com/google/gerrit/acceptance/FakeGroupAuditService.java new file mode 100644 index 0000000000..48dc4083dd --- /dev/null +++ b/java/com/google/gerrit/acceptance/FakeGroupAuditService.java @@ -0,0 +1,95 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance; + +import static java.util.Comparator.comparing; +import static java.util.concurrent.TimeUnit.SECONDS; + +import com.google.common.collect.ImmutableList; +import com.google.common.primitives.Ints; +import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.httpd.GitOverHttpServlet; +import com.google.gerrit.server.AuditEvent; +import com.google.gerrit.server.audit.AuditListener; +import com.google.gerrit.server.audit.AuditService; +import com.google.gerrit.server.audit.HttpAuditEvent; +import com.google.gerrit.server.audit.group.GroupAuditListener; +import com.google.gerrit.server.group.GroupAuditService; +import com.google.gerrit.server.plugincontext.PluginSetContext; +import com.google.inject.AbstractModule; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.atomic.AtomicLong; + +@Singleton +public class FakeGroupAuditService extends AuditService { + public static class Module extends AbstractModule { + @Override + public void configure() { + DynamicSet.setOf(binder(), GroupAuditListener.class); + DynamicSet.setOf(binder(), AuditListener.class); + + // Use this fake service at the Guice level rather than depending on tests binding their own + // audit listeners. If we used per-test listeners, then there would be a race between + // dispatching the audit events from HTTP requests performed during test setup in + // AbstractDaemonTest, and the later test setup binding the audit listener. Using a separate + // audit service implementation ensures all events get recorded. + bind(GroupAuditService.class).to(FakeGroupAuditService.class); + } + } + + private final GitOverHttpServlet.Metrics httpMetrics; + private final BlockingQueue httpEvents; + private final AtomicLong drainedSoFar; + + @Inject + FakeGroupAuditService( + PluginSetContext auditListeners, + PluginSetContext groupAuditListeners, + GitOverHttpServlet.Metrics httpMetrics) { + super(auditListeners, groupAuditListeners); + this.httpMetrics = httpMetrics; + this.httpEvents = new LinkedBlockingQueue<>(); + this.drainedSoFar = new AtomicLong(); + } + + @Override + public void dispatch(AuditEvent action) { + super.dispatch(action); + if (action instanceof HttpAuditEvent) { + httpEvents.add((HttpAuditEvent) action); + } + } + + public ImmutableList drainHttpAuditEvents() throws Exception { + // Assumes that all HttpAuditEvents are produced by GitOverHttpServlet. + int expectedSize = Ints.checkedCast(httpMetrics.getRequestsStarted() - drainedSoFar.get()); + List result = new ArrayList<>(); + for (int i = 0; i < expectedSize; i++) { + HttpAuditEvent e = httpEvents.poll(30, SECONDS); + if (e == null) { + throw new AssertionError( + String.format("Timeout after receiving %d/%d audit events", i, expectedSize)); + } + drainedSoFar.incrementAndGet(); + result.add(e); + } + return ImmutableList.sortedCopyOf(comparing(e -> e.when), result); + } +} diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index 1d1ed93273..570b59cff4 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java @@ -47,7 +47,6 @@ import com.google.gerrit.server.ssh.NoSshModule; import com.google.gerrit.server.util.SocketUtil; import com.google.gerrit.server.util.SystemLog; import com.google.gerrit.testing.FakeEmailSender; -import com.google.gerrit.testing.FakeGroupAuditService; import com.google.gerrit.testing.InMemoryRepositoryManager; import com.google.gerrit.testing.SshMode; import com.google.inject.AbstractModule; diff --git a/java/com/google/gerrit/httpd/GitOverHttpServlet.java b/java/com/google/gerrit/httpd/GitOverHttpServlet.java index 7fbb8d7393..c97ee1048f 100644 --- a/java/com/google/gerrit/httpd/GitOverHttpServlet.java +++ b/java/com/google/gerrit/httpd/GitOverHttpServlet.java @@ -14,6 +14,7 @@ package com.google.gerrit.httpd; +import com.google.common.annotations.VisibleForTesting; import com.google.common.cache.Cache; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ListMultimap; @@ -54,6 +55,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -129,6 +131,25 @@ public class GitOverHttpServlet extends GitServlet { .expireAfterWrite(Duration.ofMinutes(10)); } }); + + // Don't bind Metrics, which is bound in a parent injector in tests. + } + } + + @VisibleForTesting + @Singleton + public static class Metrics { + // Recording requests separately in this class is only necessary because of a bug in the + // implementation of the generic RequestMetricsFilter; see + // https://gerrit-review.googlesource.com/c/gerrit/+/211692 + private final AtomicLong requestsStarted = new AtomicLong(); + + void requestStarted() { + requestsStarted.incrementAndGet(); + } + + public long getRequestsStarted() { + return requestsStarted.get(); } } @@ -317,22 +338,26 @@ public class GitOverHttpServlet extends GitServlet { private final PermissionBackend permissionBackend; private final Provider userProvider; private final GroupAuditService groupAuditService; + private final Metrics metrics; @Inject UploadFilter( UploadValidators.Factory uploadValidatorsFactory, PermissionBackend permissionBackend, Provider userProvider, - GroupAuditService groupAuditService) { + GroupAuditService groupAuditService, + Metrics metrics) { this.uploadValidatorsFactory = uploadValidatorsFactory; this.permissionBackend = permissionBackend; this.userProvider = userProvider; this.groupAuditService = groupAuditService; + this.metrics = metrics; } @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException { + metrics.requestStarted(); // The Resolver above already checked READ access for us. Repository repo = ServletUtils.getRepository(request); ProjectState state = (ProjectState) request.getAttribute(ATT_STATE); @@ -431,22 +456,26 @@ public class GitOverHttpServlet extends GitServlet { private final PermissionBackend permissionBackend; private final Provider userProvider; private final GroupAuditService groupAuditService; + private final Metrics metrics; @Inject ReceiveFilter( @Named(ID_CACHE) Cache> cache, PermissionBackend permissionBackend, Provider userProvider, - GroupAuditService groupAuditService) { + GroupAuditService groupAuditService, + Metrics metrics) { this.cache = cache; this.permissionBackend = permissionBackend; this.userProvider = userProvider; this.groupAuditService = groupAuditService; + this.metrics = metrics; } @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { + metrics.requestStarted(); boolean isGet = "GET".equalsIgnoreCase(((HttpServletRequest) request).getMethod()); AsyncReceiveCommits arc = (AsyncReceiveCommits) request.getAttribute(ATT_ARC); diff --git a/java/com/google/gerrit/testing/FakeGroupAuditService.java b/java/com/google/gerrit/testing/FakeGroupAuditService.java deleted file mode 100644 index 7765e02ad7..0000000000 --- a/java/com/google/gerrit/testing/FakeGroupAuditService.java +++ /dev/null @@ -1,115 +0,0 @@ -// Copyright (C) 2018 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.testing; - -import com.google.common.collect.ImmutableSet; -import com.google.gerrit.extensions.registration.DynamicSet; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.server.AuditEvent; -import com.google.gerrit.server.audit.AuditListener; -import com.google.gerrit.server.audit.group.GroupAuditListener; -import com.google.gerrit.server.audit.group.GroupMemberAuditEvent; -import com.google.gerrit.server.audit.group.GroupSubgroupAuditEvent; -import com.google.gerrit.server.group.GroupAuditService; -import com.google.gerrit.server.plugincontext.PluginSetContext; -import com.google.inject.AbstractModule; -import com.google.inject.Inject; -import com.google.inject.Singleton; -import java.sql.Timestamp; -import java.util.ArrayList; -import java.util.List; - -@Singleton -public class FakeGroupAuditService implements GroupAuditService { - - protected final PluginSetContext groupAuditListeners; - protected final PluginSetContext auditListeners; - - public final List auditEvents = new ArrayList<>(); - - public static class Module extends AbstractModule { - @Override - public void configure() { - DynamicSet.setOf(binder(), GroupAuditListener.class); - DynamicSet.setOf(binder(), AuditListener.class); - bind(GroupAuditService.class).to(FakeGroupAuditService.class); - } - } - - @Inject - public FakeGroupAuditService( - PluginSetContext groupAuditListeners, - PluginSetContext auditListeners) { - this.groupAuditListeners = groupAuditListeners; - this.auditListeners = auditListeners; - } - - public void clearEvents() { - auditEvents.clear(); - } - - @Override - public void dispatch(AuditEvent action) { - synchronized (auditEvents) { - auditEvents.add(action); - auditEvents.notifyAll(); - } - } - - @Override - public void dispatchAddMembers( - Account.Id actor, - AccountGroup.UUID updatedGroup, - ImmutableSet addedMembers, - Timestamp addedOn) { - GroupMemberAuditEvent event = - GroupMemberAuditEvent.create(actor, updatedGroup, addedMembers, addedOn); - groupAuditListeners.runEach(l -> l.onAddMembers(event)); - } - - @Override - public void dispatchDeleteMembers( - Account.Id actor, - AccountGroup.UUID updatedGroup, - ImmutableSet deletedMembers, - Timestamp deletedOn) { - GroupMemberAuditEvent event = - GroupMemberAuditEvent.create(actor, updatedGroup, deletedMembers, deletedOn); - groupAuditListeners.runEach(l -> l.onDeleteMembers(event)); - } - - @Override - public void dispatchAddSubgroups( - Account.Id actor, - AccountGroup.UUID updatedGroup, - ImmutableSet addedSubgroups, - Timestamp addedOn) { - GroupSubgroupAuditEvent event = - GroupSubgroupAuditEvent.create(actor, updatedGroup, addedSubgroups, addedOn); - groupAuditListeners.runEach(l -> l.onAddSubgroups(event)); - } - - @Override - public void dispatchDeleteSubgroups( - Account.Id actor, - AccountGroup.UUID updatedGroup, - ImmutableSet deletedSubgroups, - Timestamp deletedOn) { - GroupSubgroupAuditEvent event = - GroupSubgroupAuditEvent.create(actor, updatedGroup, deletedSubgroups, deletedOn); - groupAuditListeners.runEach(l -> l.onDeleteSubgroups(event)); - } -} diff --git a/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletIT.java b/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletIT.java deleted file mode 100644 index 26ace25578..0000000000 --- a/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletIT.java +++ /dev/null @@ -1,93 +0,0 @@ -// Copyright (C) 2018 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.acceptance.git; - -import static com.google.common.truth.Truth.assertThat; - -import com.google.gerrit.acceptance.Sandboxed; -import com.google.gerrit.server.AuditEvent; -import com.google.gerrit.server.audit.HttpAuditEvent; -import com.google.gerrit.testing.FakeGroupAuditService; -import java.util.Collections; -import javax.servlet.http.HttpServletResponse; -import org.eclipse.jgit.transport.CredentialsProvider; -import org.eclipse.jgit.transport.RefSpec; -import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; -import org.junit.Before; -import org.junit.Test; - -public class GitOverHttpServletIT extends AbstractPushForReview { - private static final long AUDIT_EVENT_TIMEOUT = 500L; - - @Before - public void beforeEach() throws Exception { - CredentialsProvider.setDefault( - new UsernamePasswordCredentialsProvider(admin.username, admin.httpPassword)); - selectProtocol(AbstractPushForReview.Protocol.HTTP); - } - - @Test - @Sandboxed - public void receivePackAuditEventLog() throws Exception { - FakeGroupAuditService auditService = clearAuditService(); - testRepo - .git() - .push() - .setRemote("origin") - .setRefSpecs(new RefSpec("HEAD:refs/for/master")) - .call(); - waitForAudit(auditService); - - // Git smart protocol makes two requests: - // https://github.com/git/git/blob/master/Documentation/technical/http-protocol.txt - assertThat(auditService.auditEvents.size()).isEqualTo(2); - - AuditEvent e = auditService.auditEvents.get(1); - assertThat(e.who.getAccountId()).isEqualTo(admin.id); - assertThat(e.what).endsWith("/git-receive-pack"); - assertThat(e.params).isEmpty(); - assertThat(((HttpAuditEvent) e).httpStatus).isEqualTo(HttpServletResponse.SC_OK); - } - - @Test - @Sandboxed - public void uploadPackAuditEventLog() throws Exception { - FakeGroupAuditService auditService = clearAuditService(); - testRepo.git().fetch().call(); - waitForAudit(auditService); - - assertThat(auditService.auditEvents.size()).isEqualTo(1); - - AuditEvent e = auditService.auditEvents.get(0); - assertThat(e.who.toString()).isEqualTo("ANONYMOUS"); - assertThat(e.params.get("service")) - .containsExactlyElementsIn(Collections.singletonList("git-upload-pack")); - assertThat(e.what).endsWith("service=git-upload-pack"); - assertThat(((HttpAuditEvent) e).httpStatus).isEqualTo(HttpServletResponse.SC_OK); - } - - private FakeGroupAuditService clearAuditService() { - FakeGroupAuditService auditService = - server.getTestInjector().getInstance(FakeGroupAuditService.class); - auditService.clearEvents(); - return auditService; - } - - private void waitForAudit(FakeGroupAuditService auditService) throws InterruptedException { - synchronized (auditService.auditEvents) { - auditService.auditEvents.wait(AUDIT_EVENT_TIMEOUT); - } - } -} diff --git a/javatests/com/google/gerrit/acceptance/git/HttpPushForReviewIT.java b/javatests/com/google/gerrit/acceptance/git/HttpPushForReviewIT.java index ed17c38156..6309e79190 100644 --- a/javatests/com/google/gerrit/acceptance/git/HttpPushForReviewIT.java +++ b/javatests/com/google/gerrit/acceptance/git/HttpPushForReviewIT.java @@ -14,15 +14,82 @@ package com.google.gerrit.acceptance.git; +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.acceptance.FakeGroupAuditService; +import com.google.gerrit.server.AnonymousUser; +import com.google.gerrit.server.audit.HttpAuditEvent; +import com.google.inject.Inject; +import javax.servlet.http.HttpServletResponse; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.CredentialsProvider; +import org.eclipse.jgit.transport.RefSpec; import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; import org.junit.Before; +import org.junit.Test; public class HttpPushForReviewIT extends AbstractPushForReview { + @Inject private FakeGroupAuditService auditService; + @Before public void selectHttpUrl() throws Exception { CredentialsProvider.setDefault( new UsernamePasswordCredentialsProvider(admin.username, admin.httpPassword)); selectProtocol(Protocol.HTTP); + // Don't clear audit events here, since we can't guarantee all test setup has run yet. + } + + @Test + public void receivePackAuditEventLog() throws Exception { + auditService.drainHttpAuditEvents(); + testRepo + .git() + .push() + .setRemote("origin") + .setRefSpecs(new RefSpec("HEAD:refs/for/master")) + .call(); + + ImmutableList auditEvents = auditService.drainHttpAuditEvents(); + assertThat(auditEvents).hasSize(2); + + HttpAuditEvent lsRemote = auditEvents.get(0); + assertThat(lsRemote.who.getAccountId()).isEqualTo(admin.id); + assertThat(lsRemote.what).endsWith("/info/refs?service=git-receive-pack"); + assertThat(lsRemote.params).containsExactly("service", "git-receive-pack"); + assertThat(lsRemote.httpStatus).isEqualTo(HttpServletResponse.SC_OK); + + HttpAuditEvent receivePack = auditEvents.get(1); + assertThat(receivePack.who.getAccountId()).isEqualTo(admin.id); + assertThat(receivePack.what).endsWith("/git-receive-pack"); + assertThat(receivePack.params).isEmpty(); + assertThat(receivePack.httpStatus).isEqualTo(HttpServletResponse.SC_OK); + } + + @Test + public void uploadPackAuditEventLog() throws Exception { + auditService.drainHttpAuditEvents(); + // testRepo is already a clone. Make a server-side change so we have something to fetch. + try (Repository repo = repoManager.openRepository(project)) { + new TestRepository<>(repo).branch("master").commit().create(); + } + testRepo.git().fetch().call(); + + ImmutableList auditEvents = auditService.drainHttpAuditEvents(); + assertThat(auditEvents).hasSize(2); + + HttpAuditEvent lsRemote = auditEvents.get(0); + // Repo URL doesn't include /a, so fetching doesn't cause authentication. + assertThat(lsRemote.who).isInstanceOf(AnonymousUser.class); + assertThat(lsRemote.what).endsWith("/info/refs?service=git-upload-pack"); + assertThat(lsRemote.params).containsExactly("service", "git-upload-pack"); + assertThat(lsRemote.httpStatus).isEqualTo(HttpServletResponse.SC_OK); + + HttpAuditEvent uploadPack = auditEvents.get(1); + assertThat(lsRemote.who).isInstanceOf(AnonymousUser.class); + assertThat(uploadPack.what).endsWith("/git-upload-pack"); + assertThat(uploadPack.params).isEmpty(); + assertThat(uploadPack.httpStatus).isEqualTo(HttpServletResponse.SC_OK); } }