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);
+ }
}
