token enricher should use new access token api

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<String, Object> claims);
+  void enrich(AccessTokenBuilder builder);
 }

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<Object> getCustom(String key) {
     return Optional.ofNullable(claims.get(key));
   }

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<TokenClaimsEnricher> enrichers;
   
   private String subject;
   private String issuer;
@@ -71,12 +70,9 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder {
   
   private final Map<String,Object> custom = Maps.newHashMap();
   
-  JwtAccessTokenBuilder(
-    KeyGenerator keyGenerator, SecureKeyResolver keyResolver, Set<TokenClaimsEnricher> 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);
     

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<TokenClaimsEnricher> enrichers;
+  private final Set<AccessTokenEnricher> enrichers;
 
   @Inject
   public JwtAccessTokenBuilderFactory(
-    KeyGenerator keyGenerator, SecureKeyResolver keyResolver, Set<TokenClaimsEnricher> enrichers
+    KeyGenerator keyGenerator, SecureKeyResolver keyResolver, Set<AccessTokenEnricher> 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;
   }
 
 }

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() {
   }

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 <a href="https://goo.gl/s67xO3">Issue 793</a>
  * @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<HttpServletRequest> requestProvider;
@@ -69,17 +69,17 @@ public class XsrfTokenClaimsEnricher implements TokenClaimsEnricher {
    * @param requestProvider http request provider
    */
   @Inject
-  public XsrfTokenClaimsEnricher(ScmConfiguration configuration, Provider<HttpServletRequest> requestProvider) {
+  public XsrfAccessTokenEnricher(ScmConfiguration configuration, Provider<HttpServletRequest> requestProvider) {
     this.configuration = configuration;
     this.requestProvider = requestProvider;
   }
   
   @Override
-  public void enrich(Map<String, Object> 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();
   }

scm-webapp/src/main/java/sonia/scm/security/XsrfTokenClaimsValidator.java

@@ -70,7 +70,7 @@ public class XsrfTokenClaimsValidator implements TokenClaimsValidator {
   
   @Override
   public boolean validate(Map<String, Object> 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);

scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java

@@ -63,9 +63,9 @@ public class JwtAccessTokenBuilderTest {
   @Mock
   private SecureKeyResolver secureKeyResolver;
   
-  private Set<TokenClaimsEnricher> enrichers;
+  private Set<AccessTokenEnricher> 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")

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<String,Object> 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<String,Object> 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<String,Object> claims = Maps.newHashMap();
-    enricher.enrich(claims);
+    enricher.enrich(builder);
     
     // assert
-    assertNull(claims.get(Xsrf.CLAIMS_KEY)); 
+    verify(builder, never()).custom(Xsrf.TOKEN_KEY, "42");
   }
 
 }

scm-webapp/src/test/java/sonia/scm/security/XsrfTokenClaimsValidatorTest.java

@@ -70,7 +70,7 @@ public class XsrfTokenClaimsValidatorTest {
   public void testValidate() {
     // prepare
     Map<String, Object> 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<String, Object> 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<String, Object> claims = Maps.newHashMap();
-    claims.put(Xsrf.CLAIMS_KEY, "abc");
+    claims.put(Xsrf.TOKEN_KEY, "abc");
     
     // execute and assert
     assertFalse(validator.validate(claims));