Merge branch 'stable-3.8' * stable-3.8: Avoid using 'objectUnderTest' term in GitHubOAuthConfigTest Honour the cookieDomain also for the OAuth scope selection Revert "Build OAuth redirect URL when X-Forwarded-Host is present" Build OAuth redirect URL when X-Forwarded-Host is present Fix HTTP session leaks during OAuth reauthentication Stop filtering Gerrti's static resources with OAuthFilter Change-Id: I263195c4ced7e83bc2d62ae25543a098995706ae

diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java
index 1aed6cf..f65ad02 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java

@@ -158,6 +158,7 @@
       Cookie scopeCookie = new Cookie("scope", scopeRequested);
       scopeCookie.setPath("/");
       scopeCookie.setMaxAge((int) SCOPE_COOKIE_NEVER_EXPIRES);
+      config.getCookieDomain().ifPresent(scopeCookie::setDomain);
       response.addCookie(scopeCookie);
     }
 

diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
index 7ef81d1..d86feda 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java

@@ -34,6 +34,7 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
 import java.util.stream.Collectors;
@@ -78,6 +79,7 @@
   public final long httpReadTimeout;
   private final Map<String, KeyConfig> keyConfigMap;
   private final KeyConfig currentKeyConfig;
+  private final Optional<String> cookieDomain;
 
   @Inject
   protected GitHubOAuthConfig(@GerritServerConfig Config config, CanonicalWebUrl canonicalWebUrl) {
@@ -110,6 +112,7 @@
     logoutRedirectUrl = config.getString(CONF_SECTION, null, "logoutRedirectUrl");
 
     enabled = config.getString("auth", null, "type").equalsIgnoreCase(AuthType.HTTP.toString());
+    cookieDomain = Optional.ofNullable(config.getString("auth", null, "cookieDomain"));
     scopes = getScopes(config);
     sortedScopesKeys =
         scopes.keySet().stream()
@@ -207,6 +210,10 @@
     return keyConfigMap.get(subsection);
   }
 
+  public Optional<String> getCookieDomain() {
+    return cookieDomain;
+  }
+
   public class KeyConfig {
 
     public static final int PASSWORD_LENGTH_DEFAULT = 16;

diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java
index fdde648..da840bf 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java

@@ -13,12 +13,16 @@
 // limitations under the License.
 package com.googlesource.gerrit.plugins.github.oauth;
 
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import java.util.Optional;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
 
 public abstract class HttpSessionProvider<T> implements ScopedProvider<T> {
+  private final Supplier<String> singletonKey = Suppliers.memoize(() -> getClass().getName());
 
   @Inject private Provider<T> provider;
 
@@ -32,19 +36,23 @@
   @Override
   public T get(final HttpServletRequest req) {
     HttpSession session = req.getSession();
-    String singletonKey = getClass().getName();
 
     synchronized (this) {
       @SuppressWarnings("unchecked")
-      T instance = (T) session.getAttribute(singletonKey);
+      T instance = (T) session.getAttribute(singletonKey.get());
       if (instance == null) {
         instance = provider.get();
-        session.setAttribute(singletonKey, instance);
+        session.setAttribute(singletonKey.get(), instance);
       }
       return instance;
     }
   }
 
+  public void clear(final HttpServletRequest req) {
+    Optional.ofNullable(req.getSession(false))
+        .ifPresent((s) -> s.removeAttribute(singletonKey.get()));
+  }
+
   @Override
   public HttpServletRequest getScopedRequest() {
     return httpRequestProvider.get();

diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java
index 425755f..b15c8e5 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java

@@ -37,7 +37,9 @@
   private static final org.slf4j.Logger log = LoggerFactory.getLogger(OAuthFilter.class);
   private static Pattern GIT_HTTP_REQUEST_PATTERN = Pattern.compile(GitOverHttpServlet.URL_REGEX);
   private static final Set<String> GERRIT_STATIC_RESOURCES_EXTS =
-      Sets.newHashSet("css", "png", "jpg", "gif", "woff", "otf", "ttf", "map", "js", "swf", "txt");
+      Sets.newHashSet(
+          "css", "gif", "ico", "jpeg", "jpg", "js", "map", "otf", "pdf", "png", "rtf", "svg", "swf",
+          "text", "tif", "tiff", "ttf", "txt", "woff", "woff2");
   private static final Set<String> GERRIT_ALLOWED_PATHS = Sets.newHashSet("Documentation");
   private static final Set<String> GERRIT_ALLOWED_PAGES = Sets.newHashSet("scope.html");
 

diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
index 3c72141..0bca570 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java

@@ -104,7 +104,7 @@
         chain.doFilter(httpRequest, httpResponse);
       }
     } finally {
-      HttpSession httpSession = httpRequest.getSession();
+      HttpSession httpSession = httpRequest.getSession(false);
       if (gerritCookie != null && httpSession != null) {
         String gerritCookieValue = gerritCookie.getValue();
         String gerritSessionValue = (String) httpSession.getAttribute("GerritAccount");
@@ -112,7 +112,8 @@
         if (gerritSessionValue == null) {
           httpSession.setAttribute("GerritAccount", gerritCookieValue);
         } else if (!gerritSessionValue.equals(gerritCookieValue)) {
-          httpSession.invalidate();
+          httpSession.setAttribute("GerritAccount", null);
+          loginProvider.clear(httpRequest);
         }
       }
     }

diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java
index 9c68dde..307a93b 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java

@@ -19,5 +19,7 @@
 public interface ScopedProvider<T> extends Provider<T> {
   T get(HttpServletRequest request);
 
+  void clear(HttpServletRequest request);
+
   HttpServletRequest getScopedRequest();
 }

diff --git a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java
index a5bf767..9f90c56 100644
--- a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java
+++ b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java

@@ -28,6 +28,7 @@
 import com.google.inject.AbstractModule;
 import com.google.inject.Guice;
 import com.google.inject.util.Providers;
+import java.util.Optional;
 import org.eclipse.jgit.lib.Config;
 import org.junit.Before;
 import org.junit.Test;
@@ -70,12 +71,10 @@
     config.setString(CONF_KEY_SECTION, keySubsection, CIPHER_ALGO_CONFIG_LABEL, cipherAlgorithm);
     config.setString(CONF_KEY_SECTION, keySubsection, SECRET_KEY_CONFIG_LABEL, secretKeyAlgorithm);
 
-    GitHubOAuthConfig objectUnderTest = objectUnderTest();
-
-    assertEquals(objectUnderTest.getCurrentKeyConfig().isCurrent(), true);
-    assertEquals(objectUnderTest.getCurrentKeyConfig().getCipherAlgorithm(), cipherAlgorithm);
-    assertEquals(objectUnderTest.getCurrentKeyConfig().getSecretKeyAlgorithm(), secretKeyAlgorithm);
-    assertEquals(objectUnderTest.getCurrentKeyConfig().getKeyId(), keySubsection);
+    assertEquals(githubOAuthConfig().getCurrentKeyConfig().isCurrent(), true);
+    assertEquals(githubOAuthConfig().getCurrentKeyConfig().getCipherAlgorithm(), cipherAlgorithm);
+    assertEquals(githubOAuthConfig().getCurrentKeyConfig().getSecretKeyAlgorithm(), secretKeyAlgorithm);
+    assertEquals(githubOAuthConfig().getCurrentKeyConfig().getKeyId(), keySubsection);
   }
 
   @Test
@@ -89,7 +88,7 @@
     config.setString(
         CONF_KEY_SECTION, someOtherKeyConfig, PASSWORD_DEVICE_CONFIG_LABEL, testPasswordDevice);
 
-    assertEquals(objectUnderTest().getCurrentKeyConfig().getKeyId(), currentKeyConfig);
+    assertEquals(githubOAuthConfig().getCurrentKeyConfig().getKeyId(), currentKeyConfig);
   }
 
   @Test
@@ -103,16 +102,14 @@
     config.setString(
         CONF_KEY_SECTION, someOtherKeyConfig, PASSWORD_DEVICE_CONFIG_LABEL, testPasswordDevice);
 
-    GitHubOAuthConfig objectUnderTest = objectUnderTest();
-
-    assertEquals(objectUnderTest.getKeyConfig(currentKeyConfig).getKeyId(), currentKeyConfig);
-    assertEquals(objectUnderTest.getKeyConfig(someOtherKeyConfig).getKeyId(), someOtherKeyConfig);
+    assertEquals(githubOAuthConfig().getKeyConfig(currentKeyConfig).getKeyId(), currentKeyConfig);
+    assertEquals(githubOAuthConfig().getKeyConfig(someOtherKeyConfig).getKeyId(), someOtherKeyConfig);
   }
 
   @Test
   public void shouldThrowWhenNoKeyIdIsConfigured() {
     IllegalStateException illegalStateException =
-        assertThrows(IllegalStateException.class, this::objectUnderTest);
+        assertThrows(IllegalStateException.class, this::githubOAuthConfig);
 
     assertEquals(
         illegalStateException.getMessage(),
@@ -125,7 +122,7 @@
   public void shouldThrowWhenNoKeyConfigIsSetAsCurrent() {
     config.setBoolean(CONF_KEY_SECTION, "someKeyConfig", CURRENT_CONFIG_LABEL, false);
 
-    assertThrows(IllegalStateException.class, this::objectUnderTest);
+    assertThrows(IllegalStateException.class, this::githubOAuthConfig);
   }
 
   @Test
@@ -134,7 +131,7 @@
     config.setBoolean(CONF_KEY_SECTION, invalidSubsection, CURRENT_CONFIG_LABEL, false);
 
     IllegalStateException illegalStateException =
-        assertThrows(IllegalStateException.class, this::objectUnderTest);
+        assertThrows(IllegalStateException.class, this::githubOAuthConfig);
 
     assertEquals(
         illegalStateException.getMessage(),
@@ -148,7 +145,7 @@
     config.setBoolean(CONF_KEY_SECTION, "someKeyConfig", CURRENT_CONFIG_LABEL, true);
     config.setBoolean(CONF_KEY_SECTION, "someOtherKeyConfig", CURRENT_CONFIG_LABEL, true);
 
-    assertThrows(IllegalStateException.class, this::objectUnderTest);
+    assertThrows(IllegalStateException.class, this::githubOAuthConfig);
   }
 
   @Test
@@ -157,7 +154,7 @@
     config.setBoolean(CONF_KEY_SECTION, someKeyConfig, CURRENT_CONFIG_LABEL, true);
 
     IllegalStateException illegalStateException =
-        assertThrows(IllegalStateException.class, this::objectUnderTest);
+        assertThrows(IllegalStateException.class, this::githubOAuthConfig);
 
     assertEquals(
         String.format(
@@ -166,7 +163,33 @@
         illegalStateException.getMessage());
   }
 
-  private GitHubOAuthConfig objectUnderTest() {
+  @Test
+  public void shouldReturnEmptyCookieDomainByDefault() {
+    setupEncryptionConfig();
+    assertEquals(Optional.empty(), githubOAuthConfig().getCookieDomain());
+  }
+
+  @Test
+  public void shouldReturnTheCookieDomainFromAuth() {
+    setupEncryptionConfig();
+    String myDomain = ".mydomain.com";
+    config.setString("auth", null, "cookieDomain", myDomain);
+
+    assertEquals(Optional.of(myDomain), githubOAuthConfig().getCookieDomain());
+  }
+
+  private GitHubOAuthConfig githubOAuthConfig() {
     return new GitHubOAuthConfig(config, canonicalWebUrl);
   }
+
+  private void setupEncryptionConfig() {
+    String keySubsection = "someKeyConfig";
+    String cipherAlgorithm = "AES/CFB8/NoPadding";
+    String secretKeyAlgorithm = "DES";
+    config.setBoolean(CONF_KEY_SECTION, keySubsection, CURRENT_CONFIG_LABEL, true);
+    config.setString(
+        CONF_KEY_SECTION, keySubsection, PASSWORD_DEVICE_CONFIG_LABEL, testPasswordDevice);
+    config.setString(CONF_KEY_SECTION, keySubsection, CIPHER_ALGO_CONFIG_LABEL, cipherAlgorithm);
+    config.setString(CONF_KEY_SECTION, keySubsection, SECRET_KEY_CONFIG_LABEL, secretKeyAlgorithm);
+  }
 }