diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index 0025396ed7..3f8a5a82cd 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java @@ -602,6 +602,10 @@ public class GerritServer implements AutoCloseable { return testInjector; } + public Injector getHttpdInjector() { + return daemon.getHttpdInjector(); + } + Description getDescription() { return desc; } diff --git a/java/com/google/gerrit/httpd/GitOverHttpServlet.java b/java/com/google/gerrit/httpd/GitOverHttpServlet.java index 03b20e0832..6e22704534 100644 --- a/java/com/google/gerrit/httpd/GitOverHttpServlet.java +++ b/java/com/google/gerrit/httpd/GitOverHttpServlet.java @@ -360,6 +360,7 @@ public class GitOverHttpServlet extends GitServlet { private final Metrics metrics; private final PluginSetContext requestListeners; private final UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook; + private final Provider sessionProvider; @Inject UploadFilter( @@ -369,7 +370,8 @@ public class GitOverHttpServlet extends GitServlet { GroupAuditService groupAuditService, Metrics metrics, PluginSetContext requestListeners, - UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook) { + UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook, + Provider sessionProvider) { this.uploadValidatorsFactory = uploadValidatorsFactory; this.permissionBackend = permissionBackend; this.userProvider = userProvider; @@ -377,6 +379,7 @@ public class GitOverHttpServlet extends GitServlet { this.metrics = metrics; this.requestListeners = requestListeners; this.usersSelfAdvertiseRefsHook = usersSelfAdvertiseRefsHook; + this.sessionProvider = sessionProvider; } @Override @@ -392,7 +395,7 @@ public class GitOverHttpServlet extends GitServlet { HttpServletResponseWithStatusWrapper responseWrapper = new HttpServletResponseWithStatusWrapper((HttpServletResponse) response); HttpServletRequest httpRequest = (HttpServletRequest) request; - String sessionId = httpRequest.getSession().getId(); + String sessionId = getSessionIdOrNull(sessionProvider); try (TraceContext traceContext = TraceContext.open()) { RequestInfo requestInfo = @@ -495,6 +498,7 @@ public class GitOverHttpServlet extends GitServlet { private final Provider userProvider; private final GroupAuditService groupAuditService; private final Metrics metrics; + private final Provider sessionProvider; @Inject ReceiveFilter( @@ -502,12 +506,14 @@ public class GitOverHttpServlet extends GitServlet { PermissionBackend permissionBackend, Provider userProvider, GroupAuditService groupAuditService, - Metrics metrics) { + Metrics metrics, + Provider sessionProvider) { this.cache = cache; this.permissionBackend = permissionBackend; this.userProvider = userProvider; this.groupAuditService = groupAuditService; this.metrics = metrics; + this.sessionProvider = sessionProvider; } @Override @@ -547,7 +553,7 @@ public class GitOverHttpServlet extends GitServlet { } finally { groupAuditService.dispatch( new HttpAuditEvent( - httpRequest.getSession().getId(), + getSessionIdOrNull(sessionProvider), userProvider.get(), extractWhat(httpRequest), TimeUtil.nowMs(), @@ -603,4 +609,12 @@ public class GitOverHttpServlet extends GitServlet { @Override public void destroy() {} } + + private static String getSessionIdOrNull(Provider sessionProvider) { + WebSession session = sessionProvider.get(); + if (session.isSignedIn()) { + return session.getSessionId(); + } + return null; + } } diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index 2eb19aa9c7..f21a350000 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -238,6 +238,11 @@ public class Daemon extends SiteProgram { this.replica = replica; } + @VisibleForTesting + public Injector getHttpdInjector() { + return httpdInjector; + } + @Override public int run() throws Exception { if (stopOnly) { diff --git a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java index b59dfc9c86..89b4228c3f 100644 --- a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java +++ b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java @@ -17,6 +17,7 @@ package com.google.gerrit.pgm.http.jetty; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.gerrit.extensions.client.AuthType; import com.google.gerrit.extensions.events.LifecycleListener; @@ -42,8 +43,11 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; import javax.servlet.DispatcherType; import javax.servlet.Filter; +import javax.servlet.http.HttpSessionEvent; +import javax.servlet.http.HttpSessionListener; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.io.ConnectionStatistics; import org.eclipse.jetty.jmx.MBeanContainer; @@ -200,6 +204,8 @@ public class JettyServer { private final Metrics metrics; private boolean reverseProxy; private ConnectionStatistics connStats; + private final SessionHandler sessionHandler; + private final AtomicLong sessionsCounter; @Inject JettyServer( @@ -218,8 +224,27 @@ public class JettyServer { connector.addBean(connStats); } metrics = new Metrics(pool, connStats); + sessionHandler = new SessionHandler(); + sessionsCounter = new AtomicLong(); - Handler app = makeContext(env, cfg); + /* Code used for testing purposes for making assertions + * on the number of active HTTP sessions. + */ + sessionHandler.addEventListener( + new HttpSessionListener() { + + @Override + public void sessionDestroyed(HttpSessionEvent se) { + sessionsCounter.decrementAndGet(); + } + + @Override + public void sessionCreated(HttpSessionEvent se) { + sessionsCounter.incrementAndGet(); + } + }); + + Handler app = makeContext(env, cfg, sessionHandler); if (cfg.getBoolean("httpd", "requestLog", !reverseProxy)) { RequestLogHandler handler = new RequestLogHandler(); handler.setRequestLog(httpLogFactory.get()); @@ -246,6 +271,11 @@ public class JettyServer { httpd.setStopAtShutdown(false); } + @VisibleForTesting + public long numActiveSessions() { + return sessionsCounter.longValue(); + } + Metrics getMetrics() { return metrics; } @@ -445,7 +475,7 @@ public class JettyServer { return pool; } - private Handler makeContext(JettyEnv env, Config cfg) { + private Handler makeContext(JettyEnv env, Config cfg, SessionHandler sessionHandler) { final Set paths = new HashSet<>(); for (URI u : listenURLs(cfg)) { String p = u.getPath(); @@ -460,7 +490,7 @@ public class JettyServer { final List all = new ArrayList<>(); for (String path : paths) { - all.add(makeContext(path, env, cfg)); + all.add(makeContext(path, env, cfg, sessionHandler)); } if (all.size() == 1) { @@ -478,13 +508,14 @@ public class JettyServer { return r; } - private ContextHandler makeContext(final String contextPath, JettyEnv env, Config cfg) { + private ContextHandler makeContext( + final String contextPath, JettyEnv env, Config cfg, SessionHandler sessionHandler) { final ServletContextHandler app = new ServletContextHandler(); // This enables the use of sessions in Jetty, feature available // for Gerrit plug-ins to enable user-level sessions. // - app.setSessionHandler(new SessionHandler()); + app.setSessionHandler(sessionHandler); app.setErrorHandler(new HiddenErrorHandler()); // This is the path we are accessed by clients within our domain. diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java b/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java index fe1c264647..35f82700c4 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java @@ -19,11 +19,15 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.FakeGroupAuditService; import com.google.gerrit.acceptance.Sandboxed; -import com.google.gerrit.server.AnonymousUser; +import com.google.gerrit.entities.Account; +import com.google.gerrit.pgm.http.jetty.JettyServer; import com.google.gerrit.server.audit.HttpAuditEvent; import com.google.inject.Inject; +import java.util.Optional; import javax.servlet.http.HttpServletResponse; import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.CredentialsProvider; import org.eclipse.jgit.transport.RefSpec; @@ -33,9 +37,11 @@ import org.junit.Test; public class AbstractGitOverHttpServlet extends AbstractPushForReview { @Inject protected FakeGroupAuditService auditService; + private JettyServer jettyServer; @Before public void beforeEach() throws Exception { + jettyServer = server.getHttpdInjector().getInstance(JettyServer.class); CredentialsProvider.setDefault( new UsernamePasswordCredentialsProvider(admin.username(), admin.httpPassword())); selectProtocol(AbstractPushForReview.Protocol.HTTP); @@ -67,33 +73,54 @@ public class AbstractGitOverHttpServlet extends AbstractPushForReview { assertThat(receivePack.what).endsWith("/git-receive-pack"); assertThat(receivePack.params).isEmpty(); assertThat(receivePack.httpStatus).isEqualTo(HttpServletResponse.SC_OK); + assertThat(jettyServer.numActiveSessions()).isEqualTo(0); } @Test - @Sandboxed - public void uploadPackAuditEventLog() throws Exception { + public void anonymousUploadPackAuditEventLog() throws Exception { + uploadPackAuditEventLog(Constants.DEFAULT_REMOTE_NAME, Optional.empty()); + } + + @Test + public void authenticatedUploadPackAuditEventLog() throws Exception { + String remote = "authenticated"; + Config cfg = testRepo.git().getRepository().getConfig(); + + String uri = admin.getHttpUrl(server) + "/a/" + project.get(); + cfg.setString("remote", remote, "url", uri); + cfg.setString("remote", remote, "fetch", "+refs/heads/*:refs/remotes/origin/*"); + + uploadPackAuditEventLog(remote, Optional.of(admin.id())); + } + + private void uploadPackAuditEventLog(String remote, Optional accountId) + 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); TestRepository testRepo = new TestRepository<>(repo)) { testRepo.branch("master").commit().create(); } - testRepo.git().fetch().call(); + testRepo.git().fetch().setRemote(remote).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.who.toString()) + .isEqualTo( + accountId.map(id -> "IdentifiedUser[account " + id.get() + "]").orElse("ANONYMOUS")); 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.who.toString()) + .isEqualTo( + accountId.map(id -> "IdentifiedUser[account " + id.get() + "]").orElse("ANONYMOUS")); assertThat(uploadPack.what).endsWith("/git-upload-pack"); assertThat(uploadPack.params).isEmpty(); assertThat(uploadPack.httpStatus).isEqualTo(HttpServletResponse.SC_OK); + assertThat(jettyServer.numActiveSessions()).isEqualTo(0); } }