From 9e8bd299f040042b35b12a5d1dcf656fffa7d7dc Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Thu, 23 Aug 2018 08:24:19 +0200 Subject: [PATCH] fix cookie path, if scm-manager runs with context path / --- .../scm/security/AccessTokenCookieIssuer.java | 30 +++-- .../security/AccessTokenCookieIssuerTest.java | 113 ++++++++++++++++++ 2 files changed, 132 insertions(+), 11 deletions(-) create mode 100644 scm-webapp/src/test/java/sonia/scm/security/AccessTokenCookieIssuerTest.java diff --git a/scm-webapp/src/main/java/sonia/scm/security/AccessTokenCookieIssuer.java b/scm-webapp/src/main/java/sonia/scm/security/AccessTokenCookieIssuer.java index 52760e1c9a..bb1473dca6 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/AccessTokenCookieIssuer.java +++ b/scm-webapp/src/main/java/sonia/scm/security/AccessTokenCookieIssuer.java @@ -31,20 +31,19 @@ package sonia.scm.security; import com.google.common.annotations.VisibleForTesting; -import java.util.Date; +import com.google.common.base.Strings; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import sonia.scm.config.ScmConfiguration; +import sonia.scm.util.HttpUtil; +import sonia.scm.util.Util; -import java.util.concurrent.TimeUnit; import javax.inject.Inject; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import sonia.scm.config.ScmConfiguration; -import sonia.scm.util.HttpUtil; -import sonia.scm.util.Util; +import java.util.Date; +import java.util.concurrent.TimeUnit; /** * Generates cookies and invalidates access token cookies. @@ -81,7 +80,7 @@ public final class AccessTokenCookieIssuer { public void authenticate(HttpServletRequest request, HttpServletResponse response, AccessToken accessToken) { LOG.trace("create and attach cookie for access token {}", accessToken.getId()); Cookie c = new Cookie(HttpUtil.COOKIE_BEARER_AUTHENTICATION, accessToken.compact()); - c.setPath(request.getContextPath()); + c.setPath(contextPath(request)); c.setMaxAge(getMaxAge(accessToken)); c.setHttpOnly(isHttpOnly()); c.setSecure(isSecure(request)); @@ -100,7 +99,7 @@ public final class AccessTokenCookieIssuer { LOG.trace("invalidates access token cookie"); Cookie c = new Cookie(HttpUtil.COOKIE_BEARER_AUTHENTICATION, Util.EMPTY_STRING); - c.setPath(request.getContextPath()); + c.setPath(contextPath(request)); c.setMaxAge(0); c.setHttpOnly(isHttpOnly()); c.setSecure(isSecure(request)); @@ -108,6 +107,15 @@ public final class AccessTokenCookieIssuer { // attach empty cookie, that the browser can remove it response.addCookie(c); } + + @VisibleForTesting + String contextPath(HttpServletRequest request) { + String contextPath = request.getContextPath(); + if (Strings.isNullOrEmpty(contextPath)) { + return "/"; + } + return contextPath; + } private int getMaxAge(AccessToken accessToken){ long maxAgeMs = accessToken.getExpiration().getTime() - new Date().getTime(); diff --git a/scm-webapp/src/test/java/sonia/scm/security/AccessTokenCookieIssuerTest.java b/scm-webapp/src/test/java/sonia/scm/security/AccessTokenCookieIssuerTest.java new file mode 100644 index 0000000000..03cf174226 --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/security/AccessTokenCookieIssuerTest.java @@ -0,0 +1,113 @@ +package sonia.scm.security; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +import sonia.scm.config.ScmConfiguration; + +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import java.util.Date; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@RunWith(MockitoJUnitRunner.class) +public class AccessTokenCookieIssuerTest { + + private ScmConfiguration configuration; + + private AccessTokenCookieIssuer issuer; + + @Mock + private HttpServletRequest request; + + @Mock + private HttpServletResponse response; + + @Mock + private AccessToken accessToken; + + @Captor + private ArgumentCaptor cookieArgumentCaptor; + + @Before + public void setUp() { + configuration = new ScmConfiguration(); + issuer = new AccessTokenCookieIssuer(configuration); + } + + @Test + public void testContextPath() { + assertContextPath("/scm", "/scm"); + assertContextPath("/", "/"); + assertContextPath("", "/"); + assertContextPath(null, "/"); + } + + @Test + public void httpOnlyShouldBeEnabledIfXsrfProtectionIsDisabled() { + configuration.setEnabledXsrfProtection(false); + + Cookie cookie = authenticate(); + + assertTrue(cookie.isHttpOnly()); + } + + @Test + public void httpOnlyShouldBeDisabled() { + Cookie cookie = authenticate(); + + assertFalse(cookie.isHttpOnly()); + } + + @Test + public void secureShouldBeSetIfTheRequestIsSecure() { + when(request.isSecure()).thenReturn(true); + + Cookie cookie = authenticate(); + + assertTrue(cookie.getSecure()); + } + + @Test + public void secureShouldBeDisabledIfTheRequestIsNotSecure() { + when(request.isSecure()).thenReturn(false); + + Cookie cookie = authenticate(); + + assertFalse(cookie.getSecure()); + } + + @Test + public void testInvalidate() { + issuer.invalidate(request, response); + + verify(response).addCookie(cookieArgumentCaptor.capture()); + Cookie cookie = cookieArgumentCaptor.getValue(); + + assertEquals(0, cookie.getMaxAge()); + } + + private Cookie authenticate() { + when(accessToken.getExpiration()).thenReturn(new Date()); + + issuer.authenticate(request, response, accessToken); + + verify(response).addCookie(cookieArgumentCaptor.capture()); + return cookieArgumentCaptor.getValue(); + } + + + private void assertContextPath(String contextPath, String expected) { + when(request.getContextPath()).thenReturn(contextPath); + assertEquals(expected, issuer.contextPath(request)); + } +}