Merge branch 'stable-3.2-issue-13858' into stable-3.2 * stable-3.2-issue-13858: Set version to 3.2.8-SNAPSHOT Set version to 3.2.7 Avoid creating HTTP Sessions for Git-over-HTTP Change-Id: Ie5231a13b9e65185dfd6dea058fb384e6b3633fe
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index 3d7d9a7..986f549 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -589,6 +589,10 @@ 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 03b20e0..6e22704 100644 --- a/java/com/google/gerrit/httpd/GitOverHttpServlet.java +++ b/java/com/google/gerrit/httpd/GitOverHttpServlet.java
@@ -360,6 +360,7 @@ private final Metrics metrics; private final PluginSetContext<RequestListener> requestListeners; private final UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook; + private final Provider<WebSession> sessionProvider; @Inject UploadFilter( @@ -369,7 +370,8 @@ GroupAuditService groupAuditService, Metrics metrics, PluginSetContext<RequestListener> requestListeners, - UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook) { + UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook, + Provider<WebSession> sessionProvider) { this.uploadValidatorsFactory = uploadValidatorsFactory; this.permissionBackend = permissionBackend; this.userProvider = userProvider; @@ -377,6 +379,7 @@ this.metrics = metrics; this.requestListeners = requestListeners; this.usersSelfAdvertiseRefsHook = usersSelfAdvertiseRefsHook; + this.sessionProvider = sessionProvider; } @Override @@ -392,7 +395,7 @@ 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 @@ private final Provider<CurrentUser> userProvider; private final GroupAuditService groupAuditService; private final Metrics metrics; + private final Provider<WebSession> sessionProvider; @Inject ReceiveFilter( @@ -502,12 +506,14 @@ PermissionBackend permissionBackend, Provider<CurrentUser> userProvider, GroupAuditService groupAuditService, - Metrics metrics) { + Metrics metrics, + Provider<WebSession> sessionProvider) { this.cache = cache; this.permissionBackend = permissionBackend; this.userProvider = userProvider; this.groupAuditService = groupAuditService; this.metrics = metrics; + this.sessionProvider = sessionProvider; } @Override @@ -547,7 +553,7 @@ } finally { groupAuditService.dispatch( new HttpAuditEvent( - httpRequest.getSession().getId(), + getSessionIdOrNull(sessionProvider), userProvider.get(), extractWhat(httpRequest), TimeUtil.nowMs(), @@ -603,4 +609,12 @@ @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 10f5ba3..0eaf75d 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -236,6 +236,11 @@ 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 b59dfc9..89b4228 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 @@ 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.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 @@ private final Metrics metrics; private boolean reverseProxy; private ConnectionStatistics connStats; + private final SessionHandler sessionHandler; + private final AtomicLong sessionsCounter; @Inject JettyServer( @@ -218,8 +224,27 @@ 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 @@ httpd.setStopAtShutdown(false); } + @VisibleForTesting + public long numActiveSessions() { + return sessionsCounter.longValue(); + } + Metrics getMetrics() { return metrics; } @@ -445,7 +475,7 @@ 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(); @@ -460,7 +490,7 @@ 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) { @@ -478,13 +508,14 @@ 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 fe1c264..35f8270 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java
@@ -19,11 +19,15 @@ 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 @@ 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 @@ 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<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(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); } }
diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index 7d51500..ee3527c 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml
@@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-acceptance-framework</artifactId> - <version>3.2.7-SNAPSHOT</version> + <version>3.2.8-SNAPSHOT</version> <packaging>jar</packaging> <name>Gerrit Code Review - Acceptance Test Framework</name> <description>Framework for Gerrit's acceptance tests</description>
diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index 935dbb0..74018ea 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml
@@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-extension-api</artifactId> - <version>3.2.7-SNAPSHOT</version> + <version>3.2.8-SNAPSHOT</version> <packaging>jar</packaging> <name>Gerrit Code Review - Extension API</name> <description>API for Gerrit Extensions</description>
diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index a08624e..d65090f 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml
@@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-plugin-api</artifactId> - <version>3.2.7-SNAPSHOT</version> + <version>3.2.8-SNAPSHOT</version> <packaging>jar</packaging> <name>Gerrit Code Review - Plugin API</name> <description>API for Gerrit Plugins</description>
diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index d9d2f8c..f88f954 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml
@@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-war</artifactId> - <version>3.2.7-SNAPSHOT</version> + <version>3.2.8-SNAPSHOT</version> <packaging>war</packaging> <name>Gerrit Code Review - WAR</name> <description>Gerrit WAR</description>
diff --git a/version.bzl b/version.bzl index 2550819..4f80099 100644 --- a/version.bzl +++ b/version.bzl
@@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "3.2.7-SNAPSHOT" +GERRIT_VERSION = "3.2.8-SNAPSHOT"