From f8f5aa2ebd426804d85b24d7f9e6b9ffde8f2445 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Fri, 20 Mar 2020 11:10:05 +0100 Subject: [PATCH] X-SCM-Session-ID and X-SCM-Client could now be send via query parameter The use of query parameters is required for SSE, because the standard does not support header. This works currently only for GET request to avoid parsing of request body. --- .../java/sonia/scm/security/BearerToken.java | 8 +--- .../java/sonia/scm/security/SessionId.java | 30 ++++++------- .../main/java/sonia/scm/util/HttpUtil.java | 34 +++++++++----- .../sonia/scm/security/SessionIdTest.java | 42 +++++++++++++++++ .../java/sonia/scm/util/HttpUtilTest.java | 45 +++++++++++++++++++ .../scm/web/BearerWebTokenGenerator.java | 7 ++- .../web/CookieBearerWebTokenGenerator.java | 17 +++---- .../sonia/scm/security/BearerRealmTest.java | 4 +- .../scm/web/BearerWebTokenGeneratorTest.java | 15 +++---- .../CookieBearerWebTokenGeneratorTest.java | 2 +- 10 files changed, 145 insertions(+), 59 deletions(-) create mode 100644 scm-core/src/test/java/sonia/scm/security/SessionIdTest.java diff --git a/scm-core/src/main/java/sonia/scm/security/BearerToken.java b/scm-core/src/main/java/sonia/scm/security/BearerToken.java index 43a225b5b6..f42147fbe2 100644 --- a/scm-core/src/main/java/sonia/scm/security/BearerToken.java +++ b/scm-core/src/main/java/sonia/scm/security/BearerToken.java @@ -96,17 +96,13 @@ public final class BearerToken implements AuthenticationToken { /** * Creates a new {@link BearerToken} from raw string representation for the given ui session id. * - * @param sessionId session id of the client + * @param session session id of the client * @param rawToken bearer token string representation * * @return new bearer token */ - public static BearerToken create(@Nullable String sessionId, String rawToken) { + public static BearerToken create(@Nullable SessionId session, String rawToken) { Preconditions.checkArgument(!Strings.isNullOrEmpty(rawToken), "raw token is required"); - SessionId session = null; - if (!Strings.isNullOrEmpty(sessionId)) { - session = SessionId.valueOf(sessionId); - } return new BearerToken(session, rawToken); } } diff --git a/scm-core/src/main/java/sonia/scm/security/SessionId.java b/scm-core/src/main/java/sonia/scm/security/SessionId.java index 3c4fb126ff..7d1b657bfb 100644 --- a/scm-core/src/main/java/sonia/scm/security/SessionId.java +++ b/scm-core/src/main/java/sonia/scm/security/SessionId.java @@ -1,14 +1,23 @@ package sonia.scm.security; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Strings; +import lombok.EqualsAndHashCode; +import sonia.scm.util.HttpUtil; -import java.util.Objects; +import javax.servlet.http.HttpServletRequest; +import java.io.Serializable; +import java.util.Optional; /** * Client side session id. */ -public final class SessionId { +@EqualsAndHashCode +public final class SessionId implements Serializable { + + @VisibleForTesting + public static final String PARAMETER = "X-SCM-Session-ID"; private final String value; @@ -16,24 +25,15 @@ public final class SessionId { this.value = value; } - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - SessionId sessionID = (SessionId) o; - return Objects.equals(value, sessionID.value); - } - - @Override - public int hashCode() { - return Objects.hash(value); - } - @Override public String toString() { return value; } + public static Optional from(HttpServletRequest request) { + return HttpUtil.getHeaderOrGetParameter(request, PARAMETER).map(SessionId::valueOf); + } + public static SessionId valueOf(String value) { Preconditions.checkArgument(!Strings.isNullOrEmpty(value), "session id could not be empty or null"); return new SessionId(value); diff --git a/scm-core/src/main/java/sonia/scm/util/HttpUtil.java b/scm-core/src/main/java/sonia/scm/util/HttpUtil.java index 8edc0c8e14..820f492b8f 100644 --- a/scm-core/src/main/java/sonia/scm/util/HttpUtil.java +++ b/scm-core/src/main/java/sonia/scm/util/HttpUtil.java @@ -37,7 +37,6 @@ package sonia.scm.util; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.CharMatcher; -import com.google.common.base.MoreObjects; import com.google.common.base.Strings; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -52,6 +51,7 @@ import java.net.URLDecoder; import java.net.URLEncoder; import java.util.Arrays; import java.util.Locale; +import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -116,12 +116,6 @@ public final class HttpUtil */ public static final String HEADER_SCM_CLIENT = "X-SCM-Client"; - /** - * header for identifying the scm-manager client session - * @since 2.0.0 - */ - public static final String HEADER_SCM_SESSION = "X-SCM-Session-ID"; - /** Field description */ public static final String HEADER_USERAGENT = "User-Agent"; @@ -830,6 +824,24 @@ public final class HttpUtil return uri; } + /** + * Returns header value or query parameter if the request is a get request. + * + * @param request http request + * @param parameter name of header/parameter + * + * @return header value or query parameter + * + * @since 2.0.0 + */ + public static Optional getHeaderOrGetParameter(HttpServletRequest request, String parameter) { + String value = request.getHeader(parameter); + if (Strings.isNullOrEmpty(value) && "GET".equalsIgnoreCase(request.getMethod())) { + value = request.getParameter(parameter); + } + return Optional.ofNullable(value); + } + /** * Returns the given uri without leading separator. * @@ -882,16 +894,14 @@ public final class HttpUtil /** * Returns true if the http request is send by the scm-manager web interface. * - * * @param request http request * * @return true if the request comes from the web interface. * @since 1.19 */ - public static boolean isWUIRequest(HttpServletRequest request) - { - return SCM_CLIENT_WUI.equalsIgnoreCase( - request.getHeader(HEADER_SCM_CLIENT)); + public static boolean isWUIRequest(HttpServletRequest request) { + Optional client = getHeaderOrGetParameter(request, HEADER_SCM_CLIENT); + return client.isPresent() && SCM_CLIENT_WUI.equalsIgnoreCase(client.get()); } //~--- methods -------------------------------------------------------------- diff --git a/scm-core/src/test/java/sonia/scm/security/SessionIdTest.java b/scm-core/src/test/java/sonia/scm/security/SessionIdTest.java new file mode 100644 index 0000000000..89bcf5f594 --- /dev/null +++ b/scm-core/src/test/java/sonia/scm/security/SessionIdTest.java @@ -0,0 +1,42 @@ +package sonia.scm.security; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import javax.servlet.http.HttpServletRequest; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class SessionIdTest { + + @Mock + private HttpServletRequest request; + + @Test + void shouldReturnSessionIdFromHeader() { + when(request.getHeader(SessionId.PARAMETER)).thenReturn("abc42"); + + assertThat(SessionId.from(request)).contains(SessionId.valueOf("abc42")); + } + + @Test + void shouldReturnSessionIdFromQueryParameter() { + when(request.getMethod()).thenReturn("GET"); + when(request.getParameter(SessionId.PARAMETER)).thenReturn("abc42"); + + assertThat(SessionId.from(request)).contains(SessionId.valueOf("abc42")); + } + + @Test + void shouldReturnSessionIdFromQueryParameterOnlyForGetRequest() { + when(request.getMethod()).thenReturn("POST"); + lenient().when(request.getParameter(SessionId.PARAMETER)).thenReturn("abc42"); + + assertThat(SessionId.from(request)).isEmpty(); + } +} diff --git a/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java b/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java index c827764a92..8e2804a749 100644 --- a/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java +++ b/scm-core/src/test/java/sonia/scm/util/HttpUtilTest.java @@ -38,7 +38,9 @@ package sonia.scm.util; import org.junit.Test; import sonia.scm.config.ScmConfiguration; +import sonia.scm.security.SessionId; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.*; import static org.mockito.Mockito.*; @@ -466,4 +468,47 @@ public class HttpUtilTest assertEquals("test/two/three", HttpUtil.getUriWithoutStartSeperator("test/two/three")); } + + @Test + public void testGetHeaderOrGetParameterWithHeader() { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getHeader("Domain")).thenReturn("hitchhiker"); + + assertThat(HttpUtil.getHeaderOrGetParameter(request, "Domain")).contains("hitchhiker"); + } + + @Test + public void testGetHeaderOrGetParameterWithParameter() { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getMethod()).thenReturn("GET"); + when(request.getParameter("Domain")).thenReturn("hitchhiker"); + + assertThat(HttpUtil.getHeaderOrGetParameter(request, "Domain")).contains("hitchhiker"); + } + + @Test + public void testGetHeaderOrGetParameterOnPost() { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getMethod()).thenReturn("POST"); + lenient().when(request.getParameter("Domain")).thenReturn("hitchhiker"); + + assertThat(HttpUtil.getHeaderOrGetParameter(request, "Domain")).isEmpty(); + } + + @Test + public void testIsWUIRequestWithHeader() { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); + + assertThat(HttpUtil.isWUIRequest(request)).isTrue(); + } + + @Test + public void testIsWUIRequestWithParameter() { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getMethod()).thenReturn("GET"); + when(request.getParameter(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); + + assertThat(HttpUtil.isWUIRequest(request)).isTrue(); + } } diff --git a/scm-webapp/src/main/java/sonia/scm/web/BearerWebTokenGenerator.java b/scm-webapp/src/main/java/sonia/scm/web/BearerWebTokenGenerator.java index 4bb79e7b9a..21330c6e17 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/BearerWebTokenGenerator.java +++ b/scm-webapp/src/main/java/sonia/scm/web/BearerWebTokenGenerator.java @@ -35,6 +35,7 @@ package sonia.scm.web; import sonia.scm.plugin.Extension; import sonia.scm.security.BearerToken; +import sonia.scm.security.SessionId; import sonia.scm.util.HttpUtil; //~--- JDK imports ------------------------------------------------------------ @@ -68,10 +69,8 @@ public class BearerWebTokenGenerator extends SchemeBasedWebTokenGenerator { BearerToken token = null; - if (HttpUtil.AUTHORIZATION_SCHEME_BEARER.equalsIgnoreCase(scheme)) - { - String sessionId = request.getHeader(HttpUtil.HEADER_SCM_SESSION); - token = BearerToken.create(sessionId, authorization); + if (HttpUtil.AUTHORIZATION_SCHEME_BEARER.equalsIgnoreCase(scheme)) { + token = BearerToken.create(SessionId.from(request).orElse(null), authorization); } return token; diff --git a/scm-webapp/src/main/java/sonia/scm/web/CookieBearerWebTokenGenerator.java b/scm-webapp/src/main/java/sonia/scm/web/CookieBearerWebTokenGenerator.java index 2a0a97c07e..a6b2a16296 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/CookieBearerWebTokenGenerator.java +++ b/scm-webapp/src/main/java/sonia/scm/web/CookieBearerWebTokenGenerator.java @@ -40,6 +40,8 @@ import sonia.scm.security.BearerToken; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; + +import sonia.scm.security.SessionId; import sonia.scm.util.HttpUtil; /** @@ -62,19 +64,14 @@ public class CookieBearerWebTokenGenerator implements WebTokenGenerator * @return {@link BearerToken} or {@code null} */ @Override - public BearerToken createToken(HttpServletRequest request) - { + public BearerToken createToken(HttpServletRequest request) { BearerToken token = null; Cookie[] cookies = request.getCookies(); - if (cookies != null) - { - for (Cookie cookie : cookies) - { - if (HttpUtil.COOKIE_BEARER_AUTHENTICATION.equals(cookie.getName())) - { - String sessionId = HttpUtil.getHeader(request, HttpUtil.HEADER_SCM_SESSION, null); - token = BearerToken.create(sessionId, cookie.getValue()); + if (cookies != null) { + for (Cookie cookie : cookies) { + if (HttpUtil.COOKIE_BEARER_AUTHENTICATION.equals(cookie.getName())) { + token = BearerToken.create(SessionId.from(request).orElse(null), cookie.getValue()); break; } diff --git a/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java b/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java index 897b251cfa..6213c234bb 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java @@ -31,7 +31,6 @@ package sonia.scm.security; -import com.google.common.collect.ImmutableSet; import org.apache.shiro.authc.AuthenticationInfo; import org.apache.shiro.authc.UsernamePasswordToken; import org.junit.jupiter.api.BeforeEach; @@ -42,7 +41,6 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import java.util.HashMap; -import java.util.Set; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -84,7 +82,7 @@ class BearerRealmTest { @Test void shouldDoGetAuthentication() { - BearerToken bearerToken = BearerToken.create("__session__", "__bearer__"); + BearerToken bearerToken = BearerToken.create(SessionId.valueOf("__session__"), "__bearer__"); AccessToken accessToken = mock(AccessToken.class); when(accessToken.getSubject()).thenReturn("trillian"); diff --git a/scm-webapp/src/test/java/sonia/scm/web/BearerWebTokenGeneratorTest.java b/scm-webapp/src/test/java/sonia/scm/web/BearerWebTokenGeneratorTest.java index 1c054111b7..d77d2068b9 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/BearerWebTokenGeneratorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/BearerWebTokenGeneratorTest.java @@ -31,20 +31,19 @@ package sonia.scm.web; -import javax.servlet.http.HttpServletRequest; import org.apache.shiro.authc.AuthenticationToken; - - import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.*; import org.mockito.junit.jupiter.MockitoExtension; import sonia.scm.security.BearerToken; import sonia.scm.security.SessionId; -import sonia.scm.util.HttpUtil; + +import javax.servlet.http.HttpServletRequest; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.when; /** * @@ -90,7 +89,7 @@ class BearerWebTokenGeneratorTest { @Test void shouldCreateTokenWithSessionId(){ doReturn("Bearer asd").when(request).getHeader("Authorization"); - doReturn("bcd123").when(request).getHeader(HttpUtil.HEADER_SCM_SESSION); + doReturn("bcd123").when(request).getHeader(SessionId.PARAMETER); AuthenticationToken token = tokenGenerator.createToken(request); assertThat(token) diff --git a/scm-webapp/src/test/java/sonia/scm/web/CookieBearerWebTokenGeneratorTest.java b/scm-webapp/src/test/java/sonia/scm/web/CookieBearerWebTokenGeneratorTest.java index 45d2c46b83..8cfe8c1b31 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/CookieBearerWebTokenGeneratorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/CookieBearerWebTokenGeneratorTest.java @@ -76,7 +76,7 @@ class CookieBearerWebTokenGeneratorTest { @Test void shouldCreateTokenWithSessionId() { - when(request.getHeader(HttpUtil.HEADER_SCM_SESSION)).thenReturn("abc123"); + when(request.getHeader(SessionId.PARAMETER)).thenReturn("abc123"); assignBearerCookie("authc");