diff --git a/scm-core/src/main/java/sonia/scm/group/GroupNames.java b/scm-core/src/main/java/sonia/scm/group/GroupNames.java index 43a1ffda43..faeec7218b 100644 --- a/scm-core/src/main/java/sonia/scm/group/GroupNames.java +++ b/scm-core/src/main/java/sonia/scm/group/GroupNames.java @@ -176,7 +176,7 @@ public final class GroupNames implements Serializable, Iterable @Override public String toString() { - return Joiner.on(", ").join(collection); + return Joiner.on(", ").join(collection) + "(" + (external? "external": "internal") + ")"; } //~--- get methods ---------------------------------------------------------- diff --git a/scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java b/scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java index d54abcff58..da11400140 100644 --- a/scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java +++ b/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 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 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 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 groups, boolean externalGroups) { SimplePrincipalCollection collection = new SimplePrincipalCollection(); diff --git a/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java b/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java index 7579e7d829..b8d6ae909e 100644 --- a/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java +++ b/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)); + } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java index 7984e6c473..47918080ab 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java +++ b/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()) { 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 a2f4369cd7..61c7631119 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java +++ b/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 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 f7a2c02d01..2928a6c7e6 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java +++ b/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()); @@ -150,7 +159,33 @@ public class JwtAccessTokenBuilderTest { .getBody(); 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() );