Create external group names claim in token builder directly

scm-core/src/main/java/sonia/scm/group/GroupNames.java

@@ -176,7 +176,7 @@ public final class GroupNames implements Serializable, Iterable<String>
   @Override
   public String toString()
   {
-    return Joiner.on(", ").join(collection);
+    return Joiner.on(", ").join(collection) + "(" + (external? "external": "internal") + ")";
   }
 
   //~--- get methods ----------------------------------------------------------

scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java

@@ -45,7 +45,12 @@ import sonia.scm.user.User;
 import sonia.scm.user.UserManager;
 import sonia.scm.web.security.AdministrationContext;
 
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
 
 /**
  * Helper class for syncing realms. The class should simplify the creation of realms, which are syncing authenticated
@@ -80,6 +85,9 @@ public final class SyncingRealmHelper {
     this.groupManager = groupManager;
   }
 
+  /**
+   * Create {@link AuthenticationInfo} from user and groups.
+   */
   public AuthenticationInfoBuilder.ForRealm authenticationInfo() {
     return new AuthenticationInfoBuilder().new ForRealm();
   }
@@ -95,6 +103,13 @@ public final class SyncingRealmHelper {
     }
 
     public class ForRealm {
+      private ForRealm() {
+      }
+
+      /**
+       * Sets the realm.
+       * @param realm name of the realm
+       */
       public ForUser forRealm(String realm) {
         AuthenticationInfoBuilder.this.realm = realm;
         return AuthenticationInfoBuilder.this.new ForUser();
@@ -102,6 +117,13 @@ public final class SyncingRealmHelper {
     }
 
     public class ForUser {
+      private ForUser() {
+      }
+
+      /**
+       * Sets the user.
+       * @param user authenticated user
+       */
       public AuthenticationInfoBuilder.WithGroups andUser(User user) {
         AuthenticationInfoBuilder.this.user = user;
         return AuthenticationInfoBuilder.this.new WithGroups();
@@ -109,35 +131,60 @@ public final class SyncingRealmHelper {
     }
 
     public class WithGroups {
+      private WithGroups() {
+      }
+
+      /**
+       * Build the authentication info without groups.
+       * @return The complete {@link AuthenticationInfo}
+       */
+      public AuthenticationInfo withoutGroups() {
+        return withGroups(emptyList());
+      }
+
+      /**
+       * Set the internal groups for the user.
+       * @param groups groups of the authenticated user
+       * @return The complete {@link AuthenticationInfo}
+       */
+      public AuthenticationInfo withGroups(String... groups) {
+        return withGroups(asList(groups));
+      }
+
+      /**
+       * Set the internal groups for the user.
+       * @param groups groups of the authenticated user
+       * @return The complete {@link AuthenticationInfo}
+       */
       public AuthenticationInfo withGroups(Collection<String> groups) {
         AuthenticationInfoBuilder.this.groups = groups;
         AuthenticationInfoBuilder.this.external = false;
         return build();
       }
 
+      /**
+       * Set the external groups for the user.
+       * @param groups external groups of the authenticated user
+       * @return The complete {@link AuthenticationInfo}
+       */
+      public AuthenticationInfo withExternalGroups(String... groups) {
+        return withExternalGroups(asList(groups));
+      }
+
+      /**
+       * Set the external groups for the user.
+       * @param groups external groups of the authenticated user
+       * @return The complete {@link AuthenticationInfo}
+       */
       public AuthenticationInfo withExternalGroups(Collection<String> groups) {
         AuthenticationInfoBuilder.this.groups = groups;
-        AuthenticationInfoBuilder.this.external = false;
+        AuthenticationInfoBuilder.this.external = true;
         return build();
       }
     }
   }
 
   //~--- methods --------------------------------------------------------------
-  /**
-   * Create {@link AuthenticationInfo} from user and groups.
-   *
-   *
-   * @param realm name of the realm
-   * @param user authenticated user
-   * @param groups groups of the authenticated user
-   *
-   * @return authentication info
-   */
-  public AuthenticationInfo createAuthenticationInfo(String realm, User user,
-    String... groups) {
-    return createAuthenticationInfo(realm, user, ImmutableList.copyOf(groups));
-  }
 
   /**
    * Create {@link AuthenticationInfo} from user and groups.
@@ -149,22 +196,7 @@ public final class SyncingRealmHelper {
    *
    * @return authentication info
    */
-  public AuthenticationInfo createAuthenticationInfo(String realm, User user,
-    Collection<String> groups) {
-    return this.createAuthenticationInfo(realm, user, groups, false);
-  }
-
-  /**
-   * Create {@link AuthenticationInfo} from user and groups.
-   *
-   *
-   * @param realm name of the realm
-   * @param user authenticated user
-   * @param groups groups of the authenticated user
-   *
-   * @return authentication info
-   */
-  public AuthenticationInfo createAuthenticationInfo(String realm, User user,
+  private AuthenticationInfo createAuthenticationInfo(String realm, User user,
     Collection<String> groups, boolean externalGroups) {
     SimplePrincipalCollection collection = new SimplePrincipalCollection();
 

scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java

@@ -37,6 +37,7 @@ package sonia.scm.security;
 
 import com.google.common.base.Throwables;
 import org.apache.shiro.authc.AuthenticationInfo;
+import org.assertj.core.api.Assertions;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -54,6 +55,7 @@ import sonia.scm.web.security.PrivilegedAction;
 import java.io.IOException;
 
 import static java.util.Collections.singletonList;
+import static org.assertj.core.util.Arrays.asList;
 import static org.hamcrest.Matchers.hasItem;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -109,46 +111,6 @@ public class SyncingRealmHelperTest {
     helper = new SyncingRealmHelper(ctx, userManager, groupManager);
   }
 
-  /**
-   * Tests {@link SyncingRealmHelper#createAuthenticationInfo(String, User, String...)}.
-   */
-  @Test
-  public void testCreateAuthenticationInfo() {
-    User user = new User("tricia");
-    AuthenticationInfo authInfo = helper.createAuthenticationInfo("unit-test",
-      user, singletonList("heartOfGold"));
-
-    assertNotNull(authInfo);
-    assertEquals("tricia", authInfo.getPrincipals().getPrimaryPrincipal());
-    assertThat(authInfo.getPrincipals().getRealmNames(), hasItem("unit-test"));
-    assertEquals(user, authInfo.getPrincipals().oneByType(User.class));
-
-    GroupNames groups = authInfo.getPrincipals().oneByType(GroupNames.class);
-
-    assertThat(groups, hasItem("heartOfGold"));
-    assertFalse(groups.isExternal());
-  }
-
-  /**
-   * Tests {@link SyncingRealmHelper#createAuthenticationInfo(String, User, String...)}.
-   */
-  @Test
-  public void testCreateAuthenticationInfoWithExternalGroups() {
-    User user = new User("tricia");
-    AuthenticationInfo authInfo = helper.createAuthenticationInfo("unit-test",
-      user, singletonList("heartOfGold"), true);
-
-    assertNotNull(authInfo);
-    assertEquals("tricia", authInfo.getPrincipals().getPrimaryPrincipal());
-    assertThat(authInfo.getPrincipals().getRealmNames(), hasItem("unit-test"));
-    assertEquals(user, authInfo.getPrincipals().oneByType(User.class));
-
-    GroupNames groups = authInfo.getPrincipals().oneByType(GroupNames.class);
-
-    assertThat(groups, hasItem("heartOfGold"));
-    assertTrue(groups.isExternal());
-  }
-
   /**
    * Tests {@link SyncingRealmHelper#store(Group)}.
    *
@@ -222,4 +184,45 @@ public class SyncingRealmHelperTest {
     helper.store(user);
     verify(userManager, times(1)).modify(user);
   }
+
+  @Test
+  public void builderShouldSetInternalGroups() {
+    AuthenticationInfo authenticationInfo = helper
+      .authenticationInfo()
+      .forRealm("unit-test")
+      .andUser(new User("ziltoid"))
+      .withGroups("internal");
+
+    GroupNames groupNames = authenticationInfo.getPrincipals().oneByType(GroupNames.class);
+    Assertions.assertThat(groupNames.getCollection()).containsOnly("internal");
+    Assertions.assertThat(groupNames.isExternal()).isFalse();
+  }
+
+  @Test
+  public void builderShouldSetExternalGroups() {
+    AuthenticationInfo authenticationInfo = helper
+      .authenticationInfo()
+      .forRealm("unit-test")
+      .andUser(new User("ziltoid"))
+      .withExternalGroups("external");
+
+    GroupNames groupNames = authenticationInfo.getPrincipals().oneByType(GroupNames.class);
+    Assertions.assertThat(groupNames.getCollection()).containsOnly("external");
+    Assertions.assertThat(groupNames.isExternal()).isTrue();
+  }
+
+  @Test
+  public void builderShouldSetValues() {
+    User user = new User("ziltoid");
+    AuthenticationInfo authInfo = helper
+      .authenticationInfo()
+      .forRealm("unit-test")
+      .andUser(user)
+      .withoutGroups();
+
+    assertNotNull(authInfo);
+    assertEquals("ziltoid", authInfo.getPrincipals().getPrimaryPrincipal());
+    assertThat(authInfo.getPrincipals().getRealmNames(), hasItem("unit-test"));
+    assertEquals(user, authInfo.getPrincipals().oneByType(User.class));
+  }
 }

scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java

@@ -8,7 +8,6 @@ import org.apache.shiro.authc.AuthenticationException;
 import org.apache.shiro.subject.Subject;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import sonia.scm.group.GroupNames;
 import sonia.scm.security.*;
 
 import javax.servlet.http.HttpServletRequest;
@@ -92,11 +91,6 @@ public class AuthenticationResource {
         tokenBuilder.scope(Scope.valueOf(authentication.getScope()));
       }
 
-      GroupNames groupNames = subject.getPrincipals().oneByType(GroupNames.class);
-      if (groupNames != null && groupNames.isExternal()) {
-        tokenBuilder.groups(groupNames.getCollection().toArray(new String[]{}));
-      }
-
       AccessToken token = tokenBuilder.build();
 
       if (authentication.isCookie()) {

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

@@ -50,6 +50,7 @@ import org.apache.shiro.SecurityUtils;
 import org.apache.shiro.subject.Subject;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import sonia.scm.group.GroupNames;
 
 /**
  * Jwt implementation of {@link AccessTokenBuilder}.
@@ -207,6 +208,12 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder {
 
     if (!groups.isEmpty()) {
       claims.put(JwtAccessToken.GROUPS_CLAIM_KEY, groups);
+    } else {
+      Subject currentSubject = SecurityUtils.getSubject();
+      GroupNames groupNames = currentSubject.getPrincipals().oneByType(GroupNames.class);
+      if (groupNames != null && groupNames.isExternal()) {
+        claims.put(JwtAccessToken.GROUPS_CLAIM_KEY, groupNames.getCollection().toArray(new String[]{}));
+      }
     }
 
     // sign token and create compact version

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

@@ -36,23 +36,32 @@ import com.github.sdorra.shiro.SubjectAware;
 import com.google.common.collect.Sets;
 import io.jsonwebtoken.Claims;
 import io.jsonwebtoken.Jwts;
+import org.apache.shiro.SecurityUtils;
+import org.apache.shiro.subject.PrincipalCollection;
+import org.apache.shiro.subject.Subject;
 import org.apache.shiro.util.ThreadContext;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
+import org.mockito.invocation.InvocationOnMock;
 import org.mockito.junit.MockitoJUnitRunner;
+import sonia.scm.group.GroupNames;
 
+import java.util.Arrays;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.anyString;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 import static sonia.scm.security.SecureKeyTestUtil.createSecureKey;
 
@@ -62,6 +71,11 @@ import static sonia.scm.security.SecureKeyTestUtil.createSecureKey;
  * @author Sebastian Sdorra
  */
 @RunWith(MockitoJUnitRunner.class)
+@SubjectAware(
+  configuration = "classpath:sonia/scm/shiro-001.ini",
+  username = "trillian",
+  password = "secret"
+)
 public class JwtAccessTokenBuilderTest {
   
   {
@@ -96,11 +110,6 @@ public class JwtAccessTokenBuilderTest {
    * Tests {@link JwtAccessTokenBuilder#build()} with subject from shiro context.
    */
   @Test
-  @SubjectAware(
-    configuration = "classpath:sonia/scm/shiro-001.ini",
-    username = "trillian",
-    password = "secret"
-  )
   public void testBuildWithoutSubject() {
     JwtAccessToken token = factory.create().build();
     assertEquals("trillian", token.getSubject());
@@ -151,6 +160,32 @@ public class JwtAccessTokenBuilderTest {
     assertClaims(new JwtAccessToken(claims, compact));
   }
 
+  @Test
+  public void testWithExternalGroups() {
+    applyExternalGroupsToSubject(true, "external");
+    JwtAccessToken token = factory.create().subject("dent").build();
+    assertArrayEquals(new String[]{"external"}, token.getCustom(JwtAccessToken.GROUPS_CLAIM_KEY).map(x -> (String[]) x).get());
+  }
+
+  @Test
+  public void testWithInternalGroups() {
+    applyExternalGroupsToSubject(false, "external");
+    JwtAccessToken token = factory.create().subject("dent").build();
+    assertFalse(token.getCustom(JwtAccessToken.GROUPS_CLAIM_KEY).isPresent());
+  }
+
+  private void applyExternalGroupsToSubject(boolean external, String... groups) {
+    Subject subject = spy(SecurityUtils.getSubject());
+    when(subject.getPrincipals()).thenAnswer(invocation -> enrichWithGroups(invocation, groups, external));
+    shiro.setSubject(subject);
+  }
+
+  private Object enrichWithGroups(InvocationOnMock invocation, String[] groups, boolean external) throws Throwable {
+    PrincipalCollection principals = (PrincipalCollection) spy(invocation.callRealMethod());
+    when(principals.oneByType(GroupNames.class)).thenReturn(new GroupNames(Arrays.asList(groups), external));
+    return principals;
+  }
+
   private void assertClaims(JwtAccessToken token){
     assertThat(token.getId(), not(isEmptyOrNullString()));
     assertNotNull( token.getIssuedAt() );