diff options
5 files changed, 95 insertions, 12 deletions
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index bb828afece..5e8698f15a 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java @@ -541,6 +541,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 c97ee1048f..bd8ba62dbb 100644 --- a/java/com/google/gerrit/httpd/GitOverHttpServlet.java +++ b/java/com/google/gerrit/httpd/GitOverHttpServlet.java @@ -339,17 +339,20 @@ public class GitOverHttpServlet extends GitServlet { private final Provider<CurrentUser> userProvider; private final GroupAuditService groupAuditService; private final Metrics metrics; + private final Provider<WebSession> sessionProvider; @Inject UploadFilter( UploadValidators.Factory uploadValidatorsFactory, PermissionBackend permissionBackend, Provider<CurrentUser> userProvider, + Provider<WebSession> sessionProvider, GroupAuditService groupAuditService, Metrics metrics) { this.uploadValidatorsFactory = uploadValidatorsFactory; this.permissionBackend = permissionBackend; this.userProvider = userProvider; + this.sessionProvider = sessionProvider; this.groupAuditService = groupAuditService; this.metrics = metrics; } @@ -367,7 +370,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 { try { @@ -457,17 +460,20 @@ public class GitOverHttpServlet extends GitServlet { private final Provider<CurrentUser> userProvider; private final GroupAuditService groupAuditService; private final Metrics metrics; + private final Provider<WebSession> sessionProvider; @Inject ReceiveFilter( @Named(ID_CACHE) Cache<AdvertisedObjectsCacheKey, Set<ObjectId>> cache, PermissionBackend permissionBackend, Provider<CurrentUser> userProvider, + Provider<WebSession> sessionProvider, GroupAuditService groupAuditService, Metrics metrics) { this.cache = cache; this.permissionBackend = permissionBackend; this.userProvider = userProvider; + this.sessionProvider = sessionProvider; this.groupAuditService = groupAuditService; this.metrics = metrics; } @@ -509,7 +515,7 @@ public class GitOverHttpServlet extends GitServlet { } finally { groupAuditService.dispatch( new HttpAuditEvent( - httpRequest.getSession().getId(), + getSessionIdOrNull(sessionProvider), userProvider.get(), extractWhat(httpRequest), TimeUtil.nowMs(), @@ -565,4 +571,12 @@ public class GitOverHttpServlet extends GitServlet { @Override public void destroy() {} } + + private static String getSessionIdOrNull(Provider<WebSession> 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 a5c6dacbc4..8887385bf6 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -217,6 +217,11 @@ public class Daemon extends SiteProgram { httpd = enable; } + @VisibleForTesting + public Injector getHttpdInjector() { + return httpdInjector; + } + public void setSlave(boolean slave) { this.slave = slave; } diff --git a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java index 0bbb51d198..3980bd8ade 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; @@ -40,8 +41,11 @@ import java.util.HashSet; import java.util.List; 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; @@ -198,6 +202,8 @@ public class JettyServer { private final Metrics metrics; private boolean reverseProxy; private ConnectionStatistics connStats; + private final SessionHandler sessionHandler; + private final AtomicLong sessionsCounter; @Inject JettyServer( @@ -216,8 +222,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()); @@ -244,6 +269,11 @@ public class JettyServer { httpd.setStopAtShutdown(false); } + @VisibleForTesting + public long numActiveSessions() { + return sessionsCounter.longValue(); + } + Metrics getMetrics() { return metrics; } @@ -443,7 +473,7 @@ public class JettyServer { return pool; } - private Handler makeContext(JettyEnv env, Config cfg) { + private Handler makeContext(JettyEnv env, Config cfg, SessionHandler sessionHandler) { final Set<String> paths = new HashSet<>(); for (URI u : listenURLs(cfg)) { String p = u.getPath(); @@ -458,7 +488,7 @@ public class JettyServer { final List<ContextHandler> all = new ArrayList<>(); for (String path : paths) { - all.add(makeContext(path, env, cfg)); + all.add(makeContext(path, env, cfg, sessionHandler)); } if (all.size() == 1) { @@ -476,13 +506,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..cdd047092a 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.pgm.http.jetty.JettyServer; +import com.google.gerrit.reviewdb.client.Account; 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); @@ -71,29 +77,52 @@ public class AbstractGitOverHttpServlet extends AbstractPushForReview { @Test @Sandboxed - public void uploadPackAuditEventLog() throws Exception { + public void anonymousUploadPackAuditEventLog() throws Exception { + uploadPackAuditEventLog(Constants.DEFAULT_REMOTE_NAME, Optional.empty()); + } + + @Test + @Sandboxed + 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<Account.Id> 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<HttpAuditEvent> 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(lsRemote.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); } } |