From 70d5942250f39a723c80f3ce78f3d411e13f3d82 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 17 Jan 2017 15:33:19 +0100 Subject: [PATCH] token enricher should use new access token api --- ...Enricher.java => AccessTokenEnricher.java} | 15 ++++--- .../sonia/scm/security/JwtAccessToken.java | 1 + .../scm/security/JwtAccessTokenBuilder.java | 11 +---- .../JwtAccessTokenBuilderFactory.java | 13 ++++-- .../main/java/sonia/scm/security/Xsrf.java | 2 +- ...cher.java => XsrfAccessTokenEnricher.java} | 27 ++++++------ .../security/XsrfTokenClaimsValidator.java | 2 +- .../security/JwtAccessTokenBuilderTest.java | 19 ++++----- ....java => XsrfAccessTokenEnricherTest.java} | 42 +++++++++---------- .../XsrfTokenClaimsValidatorTest.java | 6 +-- 10 files changed, 68 insertions(+), 70 deletions(-) rename scm-core/src/main/java/sonia/scm/security/{TokenClaimsEnricher.java => AccessTokenEnricher.java} (77%) rename scm-webapp/src/main/java/sonia/scm/security/{XsrfTokenClaimsEnricher.java => XsrfAccessTokenEnricher.java} (76%) rename scm-webapp/src/test/java/sonia/scm/security/{XsrfTokenClaimsEnricherTest.java => XsrfAccessTokenEnricherTest.java} (76%) diff --git a/scm-core/src/main/java/sonia/scm/security/TokenClaimsEnricher.java b/scm-core/src/main/java/sonia/scm/security/AccessTokenEnricher.java similarity index 77% rename from scm-core/src/main/java/sonia/scm/security/TokenClaimsEnricher.java rename to scm-core/src/main/java/sonia/scm/security/AccessTokenEnricher.java index b8d69886b6..83fa0b5e91 100644 --- a/scm-core/src/main/java/sonia/scm/security/TokenClaimsEnricher.java +++ b/scm-core/src/main/java/sonia/scm/security/AccessTokenEnricher.java @@ -30,24 +30,23 @@ */ package sonia.scm.security; -import java.util.Map; - import sonia.scm.plugin.ExtensionPoint; /** - * TokenClaimsEnricher is able to modify the claims of a JWT token, before it is delivered to the client. - * TokenClaimsEnricher can be used to add custom values to the token claim. + * AccessTokenEnricher is able to enhance the {@link AccessToken}, before it is delivered to the client. + * AccessTokenEnricher can be used to add custom fields to the {@link AccessToken}. The enricher is always called before + * an {@link AccessToken} is build by the {@link AccessTokenBuilder}. * * @author Sebastian Sdorra * @since 2.0.0 */ @ExtensionPoint -public interface TokenClaimsEnricher { +public interface AccessTokenEnricher { /** - * Modify the token claims. + * Enriches the access token by adding fields to the {@link AccessTokenBuilder}. * - * @param claims token claims + * @param builder access token builder */ - void enrich(Map claims); + void enrich(AccessTokenBuilder builder); } diff --git a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessToken.java b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessToken.java index 9089444ebd..46f4c68e74 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessToken.java +++ b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessToken.java @@ -81,6 +81,7 @@ public final class JwtAccessToken implements AccessToken { } @Override + @SuppressWarnings("unchecked") public Optional getCustom(String key) { return Optional.ofNullable(claims.get(key)); } diff --git a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java index 0c27f1cb88..1b387730bf 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java +++ b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java @@ -61,7 +61,6 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { private final KeyGenerator keyGenerator; private final SecureKeyResolver keyResolver; - private final Set enrichers; private String subject; private String issuer; @@ -71,12 +70,9 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { private final Map custom = Maps.newHashMap(); - JwtAccessTokenBuilder( - KeyGenerator keyGenerator, SecureKeyResolver keyResolver, Set enrichers - ) { + JwtAccessTokenBuilder(KeyGenerator keyGenerator, SecureKeyResolver keyResolver) { this.keyGenerator = keyGenerator; this.keyResolver = keyResolver; - this.enrichers = enrichers; } @Override @@ -143,11 +139,6 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { // add scope to custom claims Scopes.toClaims(customClaims, scope); - // enrich claims with registered enrichers - enrichers.forEach((enricher) -> { - enricher.enrich(customClaims); - }); - Date now = new Date(); long expiration = expiresInUnit.toMillis(expiresIn); diff --git a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilderFactory.java b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilderFactory.java index 7eda91be01..63c4ea981c 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilderFactory.java +++ b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilderFactory.java @@ -45,11 +45,11 @@ public final class JwtAccessTokenBuilderFactory implements AccessTokenBuilderFac private final KeyGenerator keyGenerator; private final SecureKeyResolver keyResolver; - private final Set enrichers; + private final Set enrichers; @Inject public JwtAccessTokenBuilderFactory( - KeyGenerator keyGenerator, SecureKeyResolver keyResolver, Set enrichers + KeyGenerator keyGenerator, SecureKeyResolver keyResolver, Set enrichers ) { this.keyGenerator = keyGenerator; this.keyResolver = keyResolver; @@ -58,7 +58,14 @@ public final class JwtAccessTokenBuilderFactory implements AccessTokenBuilderFac @Override public JwtAccessTokenBuilder create() { - return new JwtAccessTokenBuilder(keyGenerator, keyResolver, enrichers); + JwtAccessTokenBuilder builder = new JwtAccessTokenBuilder(keyGenerator, keyResolver); + + // enrich access token builder + enrichers.forEach((enricher) -> { + enricher.enrich(builder); + }); + + return builder; } } diff --git a/scm-webapp/src/main/java/sonia/scm/security/Xsrf.java b/scm-webapp/src/main/java/sonia/scm/security/Xsrf.java index 0241d94b20..f9ee8a0872 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/Xsrf.java +++ b/scm-webapp/src/main/java/sonia/scm/security/Xsrf.java @@ -40,7 +40,7 @@ public final class Xsrf { static final String HEADER_KEY = "X-XSRF-Token"; - static final String CLAIMS_KEY = "xsrf"; + static final String TOKEN_KEY = "xsrf"; private Xsrf() { } diff --git a/scm-webapp/src/main/java/sonia/scm/security/XsrfTokenClaimsEnricher.java b/scm-webapp/src/main/java/sonia/scm/security/XsrfAccessTokenEnricher.java similarity index 76% rename from scm-webapp/src/main/java/sonia/scm/security/XsrfTokenClaimsEnricher.java rename to scm-webapp/src/main/java/sonia/scm/security/XsrfAccessTokenEnricher.java index 1c9f477328..7690c48b30 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/XsrfTokenClaimsEnricher.java +++ b/scm-webapp/src/main/java/sonia/scm/security/XsrfAccessTokenEnricher.java @@ -30,7 +30,7 @@ */ package sonia.scm.security; -import java.util.Map; +import com.google.common.annotations.VisibleForTesting; import java.util.UUID; import javax.inject.Inject; import javax.inject.Provider; @@ -42,22 +42,22 @@ import sonia.scm.plugin.Extension; import sonia.scm.util.HttpUtil; /** - * Xsrf token claims enricher will add an xsrf protection key to the claims of the jwt token. The enricher will only - * add the xsrf protection key, if the authentication request is issued from the web interface and xsrf protection is - * enabled. The xsrf key will be validated on every request by the {@link XsrfTokenClaimsValidator}. Xsrf protection is - * disabled by default and can be enabled with {@link ScmConfiguration#setEnabledXsrfProtection(boolean)}. + * Xsrf access token enricher will add an xsrf custom field to the access token. The enricher will only + * add the xsrf field, if the authentication request is issued from the web interface and xsrf protection is + * enabled. The xsrf field will be validated on every request by the {@link XsrfTokenClaimsValidator}. Xsrf protection + * can be disabled with {@link ScmConfiguration#setEnabledXsrfProtection(boolean)}. * - * @see https://bitbucket.org/sdorra/scm-manager/issues/793/json-hijacking-vulnerability-cwe-116-cwe + * @see Issue 793 * @author Sebastian Sdorra * @since 2.0.0 */ @Extension -public class XsrfTokenClaimsEnricher implements TokenClaimsEnricher { +public class XsrfAccessTokenEnricher implements AccessTokenEnricher { /** - * the logger for XsrfTokenClaimsEnricher + * the logger for XsrfAccessTokenEnricher */ - private static final Logger LOG = LoggerFactory.getLogger(XsrfTokenClaimsEnricher.class); + private static final Logger LOG = LoggerFactory.getLogger(XsrfAccessTokenEnricher.class); private final ScmConfiguration configuration; private final Provider requestProvider; @@ -69,17 +69,17 @@ public class XsrfTokenClaimsEnricher implements TokenClaimsEnricher { * @param requestProvider http request provider */ @Inject - public XsrfTokenClaimsEnricher(ScmConfiguration configuration, Provider requestProvider) { + public XsrfAccessTokenEnricher(ScmConfiguration configuration, Provider requestProvider) { this.configuration = configuration; this.requestProvider = requestProvider; } @Override - public void enrich(Map claims) { + public void enrich(AccessTokenBuilder builder) { if (configuration.isEnabledXsrfProtection()) { if (HttpUtil.isWUIRequest(requestProvider.get())) { LOG.debug("received wui token claim, enrich jwt with xsrf key"); - claims.put(Xsrf.CLAIMS_KEY, createToken()); + builder.custom(Xsrf.TOKEN_KEY, createToken()); } else { LOG.trace("skip xsrf enrichment, because jwt session is started from a non wui client"); } @@ -88,7 +88,8 @@ public class XsrfTokenClaimsEnricher implements TokenClaimsEnricher { } } - private String createToken() { + @VisibleForTesting + String createToken() { // TODO create interface and use a better method return UUID.randomUUID().toString(); } diff --git a/scm-webapp/src/main/java/sonia/scm/security/XsrfTokenClaimsValidator.java b/scm-webapp/src/main/java/sonia/scm/security/XsrfTokenClaimsValidator.java index 669d5c6dbc..c71be5706e 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/XsrfTokenClaimsValidator.java +++ b/scm-webapp/src/main/java/sonia/scm/security/XsrfTokenClaimsValidator.java @@ -70,7 +70,7 @@ public class XsrfTokenClaimsValidator implements TokenClaimsValidator { @Override public boolean validate(Map claims) { - String xsrfClaimValue = (String) claims.get(Xsrf.CLAIMS_KEY); + String xsrfClaimValue = (String) claims.get(Xsrf.TOKEN_KEY); if (!Strings.isNullOrEmpty(xsrfClaimValue)) { String xsrfHeaderValue = requestProvider.get().getHeader(Xsrf.HEADER_KEY); return xsrfClaimValue.equals(xsrfHeaderValue); diff --git a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java b/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java index 61216d8db0..f8a07eae83 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java @@ -63,9 +63,9 @@ public class JwtAccessTokenBuilderTest { @Mock private SecureKeyResolver secureKeyResolver; - private Set enrichers; + private Set enrichers; - private JwtAccessTokenBuilder builder; + private JwtAccessTokenBuilderFactory factory; @Rule public ShiroRule shiro = new ShiroRule(); @@ -78,9 +78,8 @@ public class JwtAccessTokenBuilderTest { when(keyGenerator.createKey()).thenReturn("42"); when(secureKeyResolver.getSecureKey(anyString())).thenReturn(createSecureKey()); enrichers = Sets.newHashSet(); - JwtAccessTokenBuilderFactory factory = new JwtAccessTokenBuilderFactory(keyGenerator, secureKeyResolver, enrichers); - builder = factory.create(); - } + factory = new JwtAccessTokenBuilderFactory(keyGenerator, secureKeyResolver, enrichers); + } /** * Tests {@link JwtAccessTokenBuilder#build()} with subject from shiro context. @@ -92,7 +91,7 @@ public class JwtAccessTokenBuilderTest { password = "secret" ) public void testBuildWithoutSubject() { - JwtAccessToken token = builder.build(); + JwtAccessToken token = factory.create().build(); assertEquals("trillian", token.getSubject()); } @@ -101,7 +100,7 @@ public class JwtAccessTokenBuilderTest { */ @Test public void testBuildWithSubject() { - JwtAccessToken token = builder.subject("dent").build(); + JwtAccessToken token = factory.create().subject("dent").build(); assertEquals("dent", token.getSubject()); } @@ -110,8 +109,8 @@ public class JwtAccessTokenBuilderTest { */ @Test public void testBuildWithEnricher() { - enrichers.add((claims) -> claims.put("c", "d")); - JwtAccessToken token = builder.subject("dent").build(); + enrichers.add((b) -> b.custom("c", "d")); + JwtAccessToken token = factory.create().subject("dent").build(); assertEquals("d", token.getCustom("c").get()); } @@ -120,7 +119,7 @@ public class JwtAccessTokenBuilderTest { */ @Test public void testBuild(){ - JwtAccessToken token = builder.subject("dent") + JwtAccessToken token = factory.create().subject("dent") .issuer("https://www.scm-manager.org") .expiresIn(5, TimeUnit.SECONDS) .custom("a", "b") diff --git a/scm-webapp/src/test/java/sonia/scm/security/XsrfTokenClaimsEnricherTest.java b/scm-webapp/src/test/java/sonia/scm/security/XsrfAccessTokenEnricherTest.java similarity index 76% rename from scm-webapp/src/test/java/sonia/scm/security/XsrfTokenClaimsEnricherTest.java rename to scm-webapp/src/test/java/sonia/scm/security/XsrfAccessTokenEnricherTest.java index e089946f15..c7f87e7414 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/XsrfTokenClaimsEnricherTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/XsrfAccessTokenEnricherTest.java @@ -31,13 +31,8 @@ package sonia.scm.security; -import com.google.common.collect.Maps; -import java.util.Map; -import javax.inject.Provider; import javax.servlet.http.HttpServletRequest; import org.junit.Test; -import static org.junit.Assert.*; -import static org.hamcrest.Matchers.*; import org.junit.Before; import org.junit.runner.RunWith; import org.mockito.Mock; @@ -47,19 +42,22 @@ import sonia.scm.config.ScmConfiguration; import sonia.scm.util.HttpUtil; /** - * Unit tests for {@link XsrfTokenClaimsEnricher}. + * Unit tests for {@link XsrfAccessTokenEnricher}. * * @author Sebastian Sdorra */ @RunWith(MockitoJUnitRunner.class) -public class XsrfTokenClaimsEnricherTest { +public class XsrfAccessTokenEnricherTest { @Mock private HttpServletRequest request; + @Mock + private AccessTokenBuilder builder; + private ScmConfiguration configuration; - private XsrfTokenClaimsEnricher enricher; + private XsrfAccessTokenEnricher enricher; /** * Prepare object under test. @@ -67,11 +65,16 @@ public class XsrfTokenClaimsEnricherTest { @Before public void prepareObjectUnderTest() { configuration = new ScmConfiguration(); - enricher = new XsrfTokenClaimsEnricher(configuration, () -> request); + enricher = new XsrfAccessTokenEnricher(configuration, () -> request) { + @Override + String createToken() { + return "42"; + } + }; } /** - * Tests {@link XsrfTokenClaimsEnricher#enrich(java.util.Map)}. + * Tests {@link XsrfAccessTokenEnricher#enrich(java.util.Map)}. */ @Test public void testEnrich() { @@ -80,15 +83,14 @@ public class XsrfTokenClaimsEnricherTest { when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); // execute - Map claims = Maps.newHashMap(); - enricher.enrich(claims); + enricher.enrich(builder); // assert - assertNotNull(claims.get(Xsrf.CLAIMS_KEY)); + verify(builder).custom(Xsrf.TOKEN_KEY, "42"); } /** - * Tests {@link XsrfTokenClaimsEnricher#enrich(java.util.Map)} with disabled xsrf protection. + * Tests {@link XsrfAccessTokenEnricher#enrich(java.util.Map)} with disabled xsrf protection. */ @Test public void testEnrichWithDisabledXsrf() { @@ -97,15 +99,14 @@ public class XsrfTokenClaimsEnricherTest { when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); // execute - Map claims = Maps.newHashMap(); - enricher.enrich(claims); + enricher.enrich(builder); // assert - assertNull(claims.get(Xsrf.CLAIMS_KEY)); + verify(builder, never()).custom(Xsrf.TOKEN_KEY, "42"); } /** - * Tests {@link XsrfTokenClaimsEnricher#enrich(java.util.Map)} with disabled xsrf protection. + * Tests {@link XsrfAccessTokenEnricher#enrich(java.util.Map)} with disabled xsrf protection. */ @Test public void testEnrichWithNonWuiClient() { @@ -113,11 +114,10 @@ public class XsrfTokenClaimsEnricherTest { configuration.setEnabledXsrfProtection(true); // execute - Map claims = Maps.newHashMap(); - enricher.enrich(claims); + enricher.enrich(builder); // assert - assertNull(claims.get(Xsrf.CLAIMS_KEY)); + verify(builder, never()).custom(Xsrf.TOKEN_KEY, "42"); } } \ No newline at end of file diff --git a/scm-webapp/src/test/java/sonia/scm/security/XsrfTokenClaimsValidatorTest.java b/scm-webapp/src/test/java/sonia/scm/security/XsrfTokenClaimsValidatorTest.java index c99286f9ad..2e47a3cfeb 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/XsrfTokenClaimsValidatorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/XsrfTokenClaimsValidatorTest.java @@ -70,7 +70,7 @@ public class XsrfTokenClaimsValidatorTest { public void testValidate() { // prepare Map claims = Maps.newHashMap(); - claims.put(Xsrf.CLAIMS_KEY, "abc"); + claims.put(Xsrf.TOKEN_KEY, "abc"); when(request.getHeader(Xsrf.HEADER_KEY)).thenReturn("abc"); // execute and assert @@ -84,7 +84,7 @@ public class XsrfTokenClaimsValidatorTest { public void testValidateWithWrongHeader() { // prepare Map claims = Maps.newHashMap(); - claims.put(Xsrf.CLAIMS_KEY, "abc"); + claims.put(Xsrf.TOKEN_KEY, "abc"); when(request.getHeader(Xsrf.HEADER_KEY)).thenReturn("123"); // execute and assert @@ -98,7 +98,7 @@ public class XsrfTokenClaimsValidatorTest { public void testValidateWithoutHeader() { // prepare Map claims = Maps.newHashMap(); - claims.put(Xsrf.CLAIMS_KEY, "abc"); + claims.put(Xsrf.TOKEN_KEY, "abc"); // execute and assert assertFalse(validator.validate(claims));