Skip to content

Commit eb56de7

Browse files
geminicaprogramsgeminicaprograms
committed
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
2 parents d9e7d35 + 3eb3672 commit eb56de7

File tree

7 files changed

+67
-23
lines changed

7 files changed

+67
-23
lines changed

‎github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ private String getScopesKey(HttpServletRequest request, HttpServletResponse resp
158158
Cookie scopeCookie = new Cookie("scope", scopeRequested);
159159
scopeCookie.setPath("/");
160160
scopeCookie.setMaxAge((int) SCOPE_COOKIE_NEVER_EXPIRES);
161+
config.getCookieDomain().ifPresent(scopeCookie::setDomain);
161162
response.addCookie(scopeCookie);
162163
}
163164

‎github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.Comparator;
3535
import java.util.List;
3636
import java.util.Map;
37+
import java.util.Optional;
3738
import java.util.concurrent.TimeUnit;
3839
import java.util.function.Function;
3940
import java.util.stream.Collectors;
@@ -78,6 +79,7 @@ public class GitHubOAuthConfig {
7879
public final long httpReadTimeout;
7980
private final Map<String, KeyConfig> keyConfigMap;
8081
private final KeyConfig currentKeyConfig;
82+
private final Optional<String> cookieDomain;
8183

8284
@Inject
8385
protected GitHubOAuthConfig(@GerritServerConfig Config config, CanonicalWebUrl canonicalWebUrl) {
@@ -110,6 +112,7 @@ protected GitHubOAuthConfig(@GerritServerConfig Config config, CanonicalWebUrl c
110112
logoutRedirectUrl = config.getString(CONF_SECTION, null, "logoutRedirectUrl");
111113

112114
enabled = config.getString("auth", null, "type").equalsIgnoreCase(AuthType.HTTP.toString());
115+
cookieDomain = Optional.ofNullable(config.getString("auth", null, "cookieDomain"));
113116
scopes = getScopes(config);
114117
sortedScopesKeys =
115118
scopes.keySet().stream()
@@ -207,6 +210,10 @@ public KeyConfig getKeyConfig(String subsection) {
207210
return keyConfigMap.get(subsection);
208211
}
209212

213+
public Optional<String> getCookieDomain() {
214+
return cookieDomain;
215+
}
216+
210217
public class KeyConfig {
211218

212219
public static final int PASSWORD_LENGTH_DEFAULT = 16;

‎github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/HttpSessionProvider.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,16 @@
1313
// limitations under the License.
1414
package com.googlesource.gerrit.plugins.github.oauth;
1515

16+
import com.google.common.base.Supplier;
17+
import com.google.common.base.Suppliers;
1618
import com.google.inject.Inject;
1719
import com.google.inject.Provider;
20+
import java.util.Optional;
1821
import javax.servlet.http.HttpServletRequest;
1922
import javax.servlet.http.HttpSession;
2023

2124
public abstract class HttpSessionProvider<T> implements ScopedProvider<T> {
25+
private final Supplier<String> singletonKey = Suppliers.memoize(() -> getClass().getName());
2226

2327
@Inject private Provider<T> provider;
2428

@@ -32,19 +36,23 @@ public T get() {
3236
@Override
3337
public T get(final HttpServletRequest req) {
3438
HttpSession session = req.getSession();
35-
String singletonKey = getClass().getName();
3639

3740
synchronized (this) {
3841
@SuppressWarnings("unchecked")
39-
T instance = (T) session.getAttribute(singletonKey);
42+
T instance = (T) session.getAttribute(singletonKey.get());
4043
if (instance == null) {
4144
instance = provider.get();
42-
session.setAttribute(singletonKey, instance);
45+
session.setAttribute(singletonKey.get(), instance);
4346
}
4447
return instance;
4548
}
4649
}
4750

51+
public void clear(final HttpServletRequest req) {
52+
Optional.ofNullable(req.getSession(false))
53+
.ifPresent((s) -> s.removeAttribute(singletonKey.get()));
54+
}
55+
4856
@Override
4957
public HttpServletRequest getScopedRequest() {
5058
return httpRequestProvider.get();

‎github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@ public class OAuthFilter implements Filter {
3737
private static final org.slf4j.Logger log = LoggerFactory.getLogger(OAuthFilter.class);
3838
private static Pattern GIT_HTTP_REQUEST_PATTERN = Pattern.compile(GitOverHttpServlet.URL_REGEX);
3939
private static final Set<String> GERRIT_STATIC_RESOURCES_EXTS =
40-
Sets.newHashSet("css", "png", "jpg", "gif", "woff", "otf", "ttf", "map", "js", "swf", "txt");
40+
Sets.newHashSet(
41+
"css", "gif", "ico", "jpeg", "jpg", "js", "map", "otf", "pdf", "png", "rtf", "svg", "swf",
42+
"text", "tif", "tiff", "ttf", "txt", "woff", "woff2");
4143
private static final Set<String> GERRIT_ALLOWED_PATHS = Sets.newHashSet("Documentation");
4244
private static final Set<String> GERRIT_ALLOWED_PAGES = Sets.newHashSet("scope.html");
4345

‎github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,15 +104,16 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
104104
chain.doFilter(httpRequest, httpResponse);
105105
}
106106
} finally {
107-
HttpSession httpSession = httpRequest.getSession();
107+
HttpSession httpSession = httpRequest.getSession(false);
108108
if (gerritCookie != null && httpSession != null) {
109109
String gerritCookieValue = gerritCookie.getValue();
110110
String gerritSessionValue = (String) httpSession.getAttribute("GerritAccount");
111111

112112
if (gerritSessionValue == null) {
113113
httpSession.setAttribute("GerritAccount", gerritCookieValue);
114114
} else if (!gerritSessionValue.equals(gerritCookieValue)) {
115-
httpSession.invalidate();
115+
httpSession.setAttribute("GerritAccount", null);
116+
loginProvider.clear(httpRequest);
116117
}
117118
}
118119
}

‎github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopedProvider.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,5 +19,7 @@
1919
public interface ScopedProvider<T> extends Provider<T> {
2020
T get(HttpServletRequest request);
2121

22+
void clear(HttpServletRequest request);
23+
2224
HttpServletRequest getScopedRequest();
2325
}

‎github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java‎

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import com.google.inject.AbstractModule;
2929
import com.google.inject.Guice;
3030
import com.google.inject.util.Providers;
31+
import java.util.Optional;
3132
import org.eclipse.jgit.lib.Config;
3233
import org.junit.Before;
3334
import org.junit.Test;
@@ -70,12 +71,10 @@ public void shouldReadASpecificKeyConfig() {
7071
config.setString(CONF_KEY_SECTION, keySubsection, CIPHER_ALGO_CONFIG_LABEL, cipherAlgorithm);
7172
config.setString(CONF_KEY_SECTION, keySubsection, SECRET_KEY_CONFIG_LABEL, secretKeyAlgorithm);
7273

73-
GitHubOAuthConfig objectUnderTest = objectUnderTest();
74-
75-
assertEquals(objectUnderTest.getCurrentKeyConfig().isCurrent(), true);
76-
assertEquals(objectUnderTest.getCurrentKeyConfig().getCipherAlgorithm(), cipherAlgorithm);
77-
assertEquals(objectUnderTest.getCurrentKeyConfig().getSecretKeyAlgorithm(), secretKeyAlgorithm);
78-
assertEquals(objectUnderTest.getCurrentKeyConfig().getKeyId(), keySubsection);
74+
assertEquals(githubOAuthConfig().getCurrentKeyConfig().isCurrent(), true);
75+
assertEquals(githubOAuthConfig().getCurrentKeyConfig().getCipherAlgorithm(), cipherAlgorithm);
76+
assertEquals(githubOAuthConfig().getCurrentKeyConfig().getSecretKeyAlgorithm(), secretKeyAlgorithm);
77+
assertEquals(githubOAuthConfig().getCurrentKeyConfig().getKeyId(), keySubsection);
7978
}
8079

8180
@Test
@@ -89,7 +88,7 @@ public void shouldReturnTheExpectedKeyConfigAsCurrent() {
8988
config.setString(
9089
CONF_KEY_SECTION, someOtherKeyConfig, PASSWORD_DEVICE_CONFIG_LABEL, testPasswordDevice);
9190

92-
assertEquals(objectUnderTest().getCurrentKeyConfig().getKeyId(), currentKeyConfig);
91+
assertEquals(githubOAuthConfig().getCurrentKeyConfig().getKeyId(), currentKeyConfig);
9392
}
9493

9594
@Test
@@ -103,16 +102,14 @@ public void shouldReadMultipleKeyConfigs() {
103102
config.setString(
104103
CONF_KEY_SECTION, someOtherKeyConfig, PASSWORD_DEVICE_CONFIG_LABEL, testPasswordDevice);
105104

106-
GitHubOAuthConfig objectUnderTest = objectUnderTest();
107-
108-
assertEquals(objectUnderTest.getKeyConfig(currentKeyConfig).getKeyId(), currentKeyConfig);
109-
assertEquals(objectUnderTest.getKeyConfig(someOtherKeyConfig).getKeyId(), someOtherKeyConfig);
105+
assertEquals(githubOAuthConfig().getKeyConfig(currentKeyConfig).getKeyId(), currentKeyConfig);
106+
assertEquals(githubOAuthConfig().getKeyConfig(someOtherKeyConfig).getKeyId(), someOtherKeyConfig);
110107
}
111108

112109
@Test
113110
public void shouldThrowWhenNoKeyIdIsConfigured() {
114111
IllegalStateException illegalStateException =
115-
assertThrows(IllegalStateException.class, this::objectUnderTest);
112+
assertThrows(IllegalStateException.class, this::githubOAuthConfig);
116113

117114
assertEquals(
118115
illegalStateException.getMessage(),
@@ -125,7 +122,7 @@ public void shouldThrowWhenNoKeyIdIsConfigured() {
125122
public void shouldThrowWhenNoKeyConfigIsSetAsCurrent() {
126123
config.setBoolean(CONF_KEY_SECTION, "someKeyConfig", CURRENT_CONFIG_LABEL, false);
127124

128-
assertThrows(IllegalStateException.class, this::objectUnderTest);
125+
assertThrows(IllegalStateException.class, this::githubOAuthConfig);
129126
}
130127

131128
@Test
@@ -134,7 +131,7 @@ public void shouldThrowWhenKeyConfigContainsDelimiterCharacter() {
134131
config.setBoolean(CONF_KEY_SECTION, invalidSubsection, CURRENT_CONFIG_LABEL, false);
135132

136133
IllegalStateException illegalStateException =
137-
assertThrows(IllegalStateException.class, this::objectUnderTest);
134+
assertThrows(IllegalStateException.class, this::githubOAuthConfig);
138135

139136
assertEquals(
140137
illegalStateException.getMessage(),
@@ -148,7 +145,7 @@ public void shouldThrowWhenMoreThanOneKeyConfigIsSetAsCurrent() {
148145
config.setBoolean(CONF_KEY_SECTION, "someKeyConfig", CURRENT_CONFIG_LABEL, true);
149146
config.setBoolean(CONF_KEY_SECTION, "someOtherKeyConfig", CURRENT_CONFIG_LABEL, true);
150147

151-
assertThrows(IllegalStateException.class, this::objectUnderTest);
148+
assertThrows(IllegalStateException.class, this::githubOAuthConfig);
152149
}
153150

154151
@Test
@@ -157,7 +154,7 @@ public void shouldThrowWhenKeyIdMissesPasswordDevice() {
157154
config.setBoolean(CONF_KEY_SECTION, someKeyConfig, CURRENT_CONFIG_LABEL, true);
158155

159156
IllegalStateException illegalStateException =
160-
assertThrows(IllegalStateException.class, this::objectUnderTest);
157+
assertThrows(IllegalStateException.class, this::githubOAuthConfig);
161158

162159
assertEquals(
163160
String.format(
@@ -166,7 +163,33 @@ public void shouldThrowWhenKeyIdMissesPasswordDevice() {
166163
illegalStateException.getMessage());
167164
}
168165

169-
private GitHubOAuthConfig objectUnderTest() {
166+
@Test
167+
public void shouldReturnEmptyCookieDomainByDefault() {
168+
setupEncryptionConfig();
169+
assertEquals(Optional.empty(), githubOAuthConfig().getCookieDomain());
170+
}
171+
172+
@Test
173+
public void shouldReturnTheCookieDomainFromAuth() {
174+
setupEncryptionConfig();
175+
String myDomain = ".mydomain.com";
176+
config.setString("auth", null, "cookieDomain", myDomain);
177+
178+
assertEquals(Optional.of(myDomain), githubOAuthConfig().getCookieDomain());
179+
}
180+
181+
private GitHubOAuthConfig githubOAuthConfig() {
170182
return new GitHubOAuthConfig(config, canonicalWebUrl);
171183
}
184+
185+
private void setupEncryptionConfig() {
186+
String keySubsection = "someKeyConfig";
187+
String cipherAlgorithm = "AES/CFB8/NoPadding";
188+
String secretKeyAlgorithm = "DES";
189+
config.setBoolean(CONF_KEY_SECTION, keySubsection, CURRENT_CONFIG_LABEL, true);
190+
config.setString(
191+
CONF_KEY_SECTION, keySubsection, PASSWORD_DEVICE_CONFIG_LABEL, testPasswordDevice);
192+
config.setString(CONF_KEY_SECTION, keySubsection, CIPHER_ALGO_CONFIG_LABEL, cipherAlgorithm);
193+
config.setString(CONF_KEY_SECTION, keySubsection, SECRET_KEY_CONFIG_LABEL, secretKeyAlgorithm);
194+
}
172195
}

0 commit comments

Comments
 (0)