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"
