From 86af7b23eb5e54780183014d1d2aca4eda4e2f63 Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Thu, 1 Aug 2019 15:43:12 +0200 Subject: [PATCH 01/12] remove groups from BearerRealm / SyncRealmHelper / DAORealmHelper --- .../sonia/scm/security/DAORealmHelper.java | 36 ++++++------- .../scm/security/DAORealmHelperFactory.java | 14 +++-- .../sonia/scm/security/GroupCollector.java | 33 ++++++++++-- .../sonia/scm/security/GroupResolver.java | 9 ++++ .../scm/security/SyncingRealmHelper.java | 8 +-- .../scm/security/DAORealmHelperTest.java | 27 +--------- .../scm/security/SyncingRealmHelperTest.java | 54 +++++-------------- .../java/sonia/scm/security/BearerRealm.java | 1 - .../sonia/scm/security/BearerRealmTest.java | 2 - 9 files changed, 80 insertions(+), 104 deletions(-) create mode 100644 scm-core/src/main/java/sonia/scm/security/GroupResolver.java diff --git a/scm-core/src/main/java/sonia/scm/security/DAORealmHelper.java b/scm-core/src/main/java/sonia/scm/security/DAORealmHelper.java index ea3e7ce9f5..6ec64a67de 100644 --- a/scm-core/src/main/java/sonia/scm/security/DAORealmHelper.java +++ b/scm-core/src/main/java/sonia/scm/security/DAORealmHelper.java @@ -45,7 +45,6 @@ import org.apache.shiro.authc.credential.CredentialsMatcher; import org.apache.shiro.subject.SimplePrincipalCollection; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import sonia.scm.group.GroupDAO; import sonia.scm.user.User; import sonia.scm.user.UserDAO; @@ -71,8 +70,6 @@ public final class DAORealmHelper { private final UserDAO userDAO; - private final GroupCollector groupCollector; - private final String realm; //~--- constructors --------------------------------------------------------- @@ -83,14 +80,12 @@ public final class DAORealmHelper { * * @param loginAttemptHandler login attempt handler for wrapping credentials matcher * @param userDAO user dao - * @param groupCollector collect groups for a principal * @param realm name of realm */ - public DAORealmHelper(LoginAttemptHandler loginAttemptHandler, UserDAO userDAO, GroupCollector groupCollector, String realm) { + public DAORealmHelper(LoginAttemptHandler loginAttemptHandler, UserDAO userDAO, String realm) { this.loginAttemptHandler = loginAttemptHandler; this.realm = realm; this.userDAO = userDAO; - this.groupCollector = groupCollector; } //~--- get methods ---------------------------------------------------------- @@ -120,7 +115,7 @@ public final class DAORealmHelper { UsernamePasswordToken upt = (UsernamePasswordToken) token; String principal = upt.getUsername(); - return getAuthenticationInfo(principal, null, null, Collections.emptySet()); + return getAuthenticationInfo(principal, null, null); } /** @@ -135,7 +130,7 @@ public final class DAORealmHelper { } - private AuthenticationInfo getAuthenticationInfo(String principal, String credentials, Scope scope, Iterable groups) { + private AuthenticationInfo getAuthenticationInfo(String principal, String credentials, Scope scope) { checkArgument(!Strings.isNullOrEmpty(principal), "username is required"); LOG.debug("try to authenticate {}", principal); @@ -153,7 +148,6 @@ public final class DAORealmHelper { collection.add(principal, realm); collection.add(user, realm); - collection.add(groupCollector.collect(principal, groups), realm); collection.add(MoreObjects.firstNonNull(scope, Scope.empty()), realm); String creds = credentials; @@ -207,17 +201,17 @@ public final class DAORealmHelper { return this; } - /** - * With groups adds extra groups, besides those which come from the {@link GroupDAO}, to the authentication info. - * - * @param groups extra groups - * - * @return {@code this} - */ - public AuthenticationInfoBuilder withGroups(Iterable groups) { - this.groups = groups; - return this; - } +// /** +// * With groups adds extra groups, besides those which come from the {@link GroupDAO}, to the authentication info. +// * +// * @param groups extra groups +// * +// * @return {@code this} +// */ +// public AuthenticationInfoBuilder withGroups(Iterable groups) { +// this.groups = groups; +// return this; +// } /** * Build creates the authentication info from the given information. @@ -225,7 +219,7 @@ public final class DAORealmHelper { * @return authentication info */ public AuthenticationInfo build() { - return getAuthenticationInfo(principal, credentials, scope, groups); + return getAuthenticationInfo(principal, credentials, scope); } } diff --git a/scm-core/src/main/java/sonia/scm/security/DAORealmHelperFactory.java b/scm-core/src/main/java/sonia/scm/security/DAORealmHelperFactory.java index ee2bf11e21..b503ff8375 100644 --- a/scm-core/src/main/java/sonia/scm/security/DAORealmHelperFactory.java +++ b/scm-core/src/main/java/sonia/scm/security/DAORealmHelperFactory.java @@ -30,6 +30,7 @@ */ package sonia.scm.security; +import sonia.scm.cache.CacheManager; import sonia.scm.group.GroupDAO; import sonia.scm.user.UserDAO; @@ -45,20 +46,23 @@ public final class DAORealmHelperFactory { private final LoginAttemptHandler loginAttemptHandler; private final UserDAO userDAO; - private final GroupCollector groupCollector; + private final CacheManager cacheManager; + private final GroupResolver groupResolver; /** * Constructs a new instance. - * * @param loginAttemptHandler login attempt handler * @param userDAO user dao * @param groupDAO group dao + * @param cacheManager + * @param groupResolver */ @Inject - public DAORealmHelperFactory(LoginAttemptHandler loginAttemptHandler, UserDAO userDAO, GroupDAO groupDAO) { + public DAORealmHelperFactory(LoginAttemptHandler loginAttemptHandler, UserDAO userDAO, GroupDAO groupDAO, CacheManager cacheManager, GroupResolver groupResolver) { this.loginAttemptHandler = loginAttemptHandler; this.userDAO = userDAO; - this.groupCollector = new GroupCollector(groupDAO); + this.groupResolver = groupResolver; + this.cacheManager = cacheManager; } /** @@ -69,7 +73,7 @@ public final class DAORealmHelperFactory { * @return new {@link DAORealmHelper} instance. */ public DAORealmHelper create(String realm) { - return new DAORealmHelper(loginAttemptHandler, userDAO, groupCollector, realm); + return new DAORealmHelper(loginAttemptHandler, userDAO, realm); } } diff --git a/scm-core/src/main/java/sonia/scm/security/GroupCollector.java b/scm-core/src/main/java/sonia/scm/security/GroupCollector.java index 56687af7ef..06ac590a9a 100644 --- a/scm-core/src/main/java/sonia/scm/security/GroupCollector.java +++ b/scm-core/src/main/java/sonia/scm/security/GroupCollector.java @@ -3,10 +3,14 @@ package sonia.scm.security; import com.google.common.collect.ImmutableSet; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import sonia.scm.cache.Cache; +import sonia.scm.cache.CacheManager; import sonia.scm.group.Group; import sonia.scm.group.GroupDAO; import sonia.scm.group.GroupNames; +import java.util.Set; + /** * Collect groups for a certain principal. * Warning: The class is only for internal use and should never used directly. @@ -15,18 +19,41 @@ class GroupCollector { private static final Logger LOG = LoggerFactory.getLogger(GroupCollector.class); + /** Field description */ + public static final String CACHE_NAME = "sonia.cache.externalGroups"; + + /** Field description */ + private final Cache cache; + private Set groupResolvers; + private final GroupDAO groupDAO; - GroupCollector(GroupDAO groupDAO) { + GroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set groupResolvers) { this.groupDAO = groupDAO; + this.cache = cacheManager.getCache(CACHE_NAME); + this.groupResolvers = groupResolvers; } - GroupNames collect(String principal, Iterable groupNames) { + Iterable collect(String principal) { + + Set externalGroups = cache.get(principal); + + if (externalGroups == null) { + ImmutableSet.Builder newExternalGroups = ImmutableSet.builder(); + + for (GroupResolver groupResolver : groupResolvers) { + Iterable groups = groupResolver.resolveGroups(principal); + groups.forEach(newExternalGroups::add); + } + + cache.put(principal, newExternalGroups.build()); + } + ImmutableSet.Builder builder = ImmutableSet.builder(); builder.add(GroupNames.AUTHENTICATED); - for (String group : groupNames) { + for (String group : externalGroups) { builder.add(group); } diff --git a/scm-core/src/main/java/sonia/scm/security/GroupResolver.java b/scm-core/src/main/java/sonia/scm/security/GroupResolver.java new file mode 100644 index 0000000000..3845628913 --- /dev/null +++ b/scm-core/src/main/java/sonia/scm/security/GroupResolver.java @@ -0,0 +1,9 @@ +package sonia.scm.security; + +import sonia.scm.plugin.ExtensionPoint; + +@ExtensionPoint +public interface GroupResolver { + + Iterable resolveGroups(String principal); +} 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 d421d33f45..b209184902 100644 --- a/scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java +++ b/scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java @@ -36,6 +36,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sonia.scm.AlreadyExistsException; import sonia.scm.NotFoundException; +import sonia.scm.cache.CacheManager; import sonia.scm.group.ExternalGroupNames; import sonia.scm.group.Group; import sonia.scm.group.GroupDAO; @@ -65,7 +66,7 @@ public final class SyncingRealmHelper { private final AdministrationContext ctx; private final UserManager userManager; private final GroupManager groupManager; - private final GroupCollector groupCollector; + private final CacheManager cacheManager; /** * Constructs a new SyncingRealmHelper. @@ -76,11 +77,11 @@ public final class SyncingRealmHelper { * @param groupDAO group dao */ @Inject - public SyncingRealmHelper(AdministrationContext ctx, UserManager userManager, GroupManager groupManager, GroupDAO groupDAO) { + public SyncingRealmHelper(AdministrationContext ctx, UserManager userManager, GroupManager groupManager, GroupDAO groupDAO, CacheManager cacheManager) { this.ctx = ctx; this.userManager = userManager; this.groupManager = groupManager; - this.groupCollector = new GroupCollector(groupDAO); + this.cacheManager = cacheManager; } /** @@ -199,7 +200,6 @@ public final class SyncingRealmHelper { collection.add(user.getId(), realm); collection.add(user, realm); - collection.add(groupCollector.collect(user.getId(), groups), realm); collection.add(new ExternalGroupNames(externalGroups), realm); return new SimpleAuthenticationInfo(collection, user.getPassword()); diff --git a/scm-core/src/test/java/sonia/scm/security/DAORealmHelperTest.java b/scm-core/src/test/java/sonia/scm/security/DAORealmHelperTest.java index 78dbd4fdd2..0fbcc20ac0 100644 --- a/scm-core/src/test/java/sonia/scm/security/DAORealmHelperTest.java +++ b/scm-core/src/test/java/sonia/scm/security/DAORealmHelperTest.java @@ -1,20 +1,16 @@ package sonia.scm.security; -import com.google.common.collect.ImmutableList; import org.apache.shiro.authc.AuthenticationInfo; import org.apache.shiro.authc.DisabledAccountException; import org.apache.shiro.authc.UnknownAccountException; import org.apache.shiro.authc.UsernamePasswordToken; import org.apache.shiro.subject.PrincipalCollection; -import org.junit.Ignore; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import sonia.scm.group.Group; import sonia.scm.group.GroupDAO; -import sonia.scm.group.GroupNames; import sonia.scm.user.User; import sonia.scm.user.UserDAO; @@ -38,7 +34,7 @@ class DAORealmHelperTest { @BeforeEach void setUpObjectUnderTest() { - helper = new DAORealmHelper(loginAttemptHandler, userDAO, new GroupCollector(groupDAO), "hitchhiker"); + helper = new DAORealmHelper(loginAttemptHandler, userDAO, "hitchhiker"); } @Test @@ -73,29 +69,9 @@ class DAORealmHelperTest { AuthenticationInfo authenticationInfo = helper.authenticationInfoBuilder("trillian").build(); PrincipalCollection principals = authenticationInfo.getPrincipals(); assertThat(principals.oneByType(User.class)).isSameAs(user); - assertThat(principals.oneByType(GroupNames.class)).containsOnly("_authenticated"); assertThat(principals.oneByType(Scope.class)).isEmpty(); } - @Test - @Ignore - void shouldReturnAuthenticationInfoWithGroups() { - User user = new User("trillian"); - when(userDAO.get("trillian")).thenReturn(user); - - Group one = new Group("xml", "one", "trillian"); - Group two = new Group("xml", "two", "trillian"); - Group six = new Group("xml", "six", "dent"); - when(groupDAO.getAll()).thenReturn(ImmutableList.of(one, two, six)); - - AuthenticationInfo authenticationInfo = helper.authenticationInfoBuilder("trillian") - .withGroups(ImmutableList.of("three")) - .build(); - - PrincipalCollection principals = authenticationInfo.getPrincipals(); - assertThat(principals.oneByType(GroupNames.class)).containsOnly("_authenticated", "one", "two", "three"); - } - @Test void shouldReturnAuthenticationInfoWithScope() { User user = new User("trillian"); @@ -148,7 +124,6 @@ class DAORealmHelperTest { PrincipalCollection principals = authenticationInfo.getPrincipals(); assertThat(principals.oneByType(User.class)).isSameAs(user); - assertThat(principals.oneByType(GroupNames.class)).containsOnly("_authenticated"); assertThat(principals.oneByType(Scope.class)).isEmpty(); assertThat(authenticationInfo.getCredentials()).isNull(); 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 20d1010b57..b7fbd97aac 100644 --- a/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java +++ b/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java @@ -36,7 +36,6 @@ package sonia.scm.security; //~--- non-JDK imports -------------------------------------------------------- import com.google.common.base.Throwables; -import com.google.common.collect.Lists; import org.apache.shiro.authc.AuthenticationInfo; import org.assertj.core.api.Assertions; import org.junit.Before; @@ -45,22 +44,26 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import sonia.scm.AlreadyExistsException; +import sonia.scm.cache.CacheManager; import sonia.scm.group.ExternalGroupNames; import sonia.scm.group.Group; import sonia.scm.group.GroupDAO; import sonia.scm.group.GroupManager; -import sonia.scm.group.GroupNames; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.web.security.AdministrationContext; import sonia.scm.web.security.PrivilegedAction; import java.io.IOException; -import java.util.List; import static org.hamcrest.Matchers.hasItem; -import static org.junit.Assert.*; -import static org.mockito.Mockito.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; //~--- JDK imports ------------------------------------------------------------ @@ -81,6 +84,9 @@ public class SyncingRealmHelperTest { @Mock private GroupDAO groupDAO; + @Mock + CacheManager cacheManager; + private SyncingRealmHelper helper; /** @@ -106,7 +112,7 @@ public class SyncingRealmHelperTest { } }; - helper = new SyncingRealmHelper(ctx, userManager, groupManager, groupDAO); + helper = new SyncingRealmHelper(ctx, userManager, groupManager, groupDAO, cacheManager); } /** @@ -183,19 +189,6 @@ public class SyncingRealmHelperTest { verify(userManager, times(1)).modify(user); } - @Test - public void builderShouldSetInternalGroups() { - AuthenticationInfo authenticationInfo = helper - .authenticationInfo() - .forRealm("unit-test") - .andUser(new User("ziltoid")) - .withGroups("internal") - .build(); - - GroupNames groupNames = authenticationInfo.getPrincipals().oneByType(GroupNames.class); - Assertions.assertThat(groupNames.getCollection()).contains("_authenticated", "internal"); - } - @Test public void builderShouldSetExternalGroups() { AuthenticationInfo authenticationInfo = helper @@ -223,27 +216,4 @@ public class SyncingRealmHelperTest { assertThat(authInfo.getPrincipals().getRealmNames(), hasItem("unit-test")); assertEquals(user, authInfo.getPrincipals().oneByType(User.class)); } - - @Test - public void shouldReturnCombinedGroupNames() { - User user = new User("tricia"); - - List groups = Lists.newArrayList(new Group("xml", "heartOfGold", "tricia")); - when(groupDAO.getAll()).thenReturn(groups); - - AuthenticationInfo authInfo = helper - .authenticationInfo() - .forRealm("unit-test") - .andUser(user) - .withGroups("fjordsOfAfrican") - .withExternalGroups("g42") - .build(); - - - GroupNames groupNames = authInfo.getPrincipals().oneByType(GroupNames.class); - Assertions.assertThat(groupNames).contains("_authenticated", "heartOfGold", "fjordsOfAfrican"); - - ExternalGroupNames externalGroupNames = authInfo.getPrincipals().oneByType(ExternalGroupNames.class); - Assertions.assertThat(externalGroupNames).contains("g42"); - } } diff --git a/scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java b/scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java index b237a0a5ff..324dfe9082 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java @@ -104,7 +104,6 @@ public class BearerRealm extends AuthenticatingRealm return helper.authenticationInfoBuilder(accessToken.getSubject()) .withCredentials(bt.getCredentials()) .withScope(Scopes.fromClaims(accessToken.getClaims())) - .withGroups(accessToken.getGroups()) .build(); } 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 c2d75358fd..d458f9c72c 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java @@ -90,12 +90,10 @@ class BearerRealmTest { Set groups = ImmutableSet.of("HeartOfGold", "Puzzle42"); when(accessToken.getSubject()).thenReturn("trillian"); - when(accessToken.getGroups()).thenReturn(groups); when(accessToken.getClaims()).thenReturn(new HashMap<>()); when(accessTokenResolver.resolve(bearerToken)).thenReturn(accessToken); when(realmHelper.authenticationInfoBuilder("trillian")).thenReturn(builder); - when(builder.withGroups(groups)).thenReturn(builder); when(builder.withCredentials("__bearer__")).thenReturn(builder); when(builder.withScope(any(Scope.class))).thenReturn(builder); when(builder.build()).thenReturn(authenticationInfo); From 2cff893d738da2cfde605af1d3eeb8934d2b758c Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Thu, 1 Aug 2019 16:05:22 +0200 Subject: [PATCH 02/12] add interface in core + move groupCollector to webapp --- .../sonia/scm/security/GroupCollector.java | 69 +----------------- .../scm/group/DefaultGroupCollector.java | 70 +++++++++++++++++++ .../sonia/scm/group}/GroupCollectorTest.java | 14 ++-- 3 files changed, 77 insertions(+), 76 deletions(-) create mode 100644 scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java rename {scm-core/src/test/java/sonia/scm/security => scm-webapp/src/test/java/sonia/scm/group}/GroupCollectorTest.java (74%) diff --git a/scm-core/src/main/java/sonia/scm/security/GroupCollector.java b/scm-core/src/main/java/sonia/scm/security/GroupCollector.java index 06ac590a9a..6c9bf2e659 100644 --- a/scm-core/src/main/java/sonia/scm/security/GroupCollector.java +++ b/scm-core/src/main/java/sonia/scm/security/GroupCollector.java @@ -1,70 +1,5 @@ package sonia.scm.security; -import com.google.common.collect.ImmutableSet; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import sonia.scm.cache.Cache; -import sonia.scm.cache.CacheManager; -import sonia.scm.group.Group; -import sonia.scm.group.GroupDAO; -import sonia.scm.group.GroupNames; - -import java.util.Set; - -/** - * Collect groups for a certain principal. - * Warning: The class is only for internal use and should never used directly. - */ -class GroupCollector { - - private static final Logger LOG = LoggerFactory.getLogger(GroupCollector.class); - - /** Field description */ - public static final String CACHE_NAME = "sonia.cache.externalGroups"; - - /** Field description */ - private final Cache cache; - private Set groupResolvers; - - private final GroupDAO groupDAO; - - GroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set groupResolvers) { - this.groupDAO = groupDAO; - this.cache = cacheManager.getCache(CACHE_NAME); - this.groupResolvers = groupResolvers; - } - - Iterable collect(String principal) { - - Set externalGroups = cache.get(principal); - - if (externalGroups == null) { - ImmutableSet.Builder newExternalGroups = ImmutableSet.builder(); - - for (GroupResolver groupResolver : groupResolvers) { - Iterable groups = groupResolver.resolveGroups(principal); - groups.forEach(newExternalGroups::add); - } - - cache.put(principal, newExternalGroups.build()); - } - - ImmutableSet.Builder builder = ImmutableSet.builder(); - - builder.add(GroupNames.AUTHENTICATED); - - for (String group : externalGroups) { - builder.add(group); - } - - for (Group group : groupDAO.getAll()) { - if (group.isMember(principal)) { - builder.add(group.getName()); - } - } - - GroupNames groups = new GroupNames(builder.build()); - LOG.debug("collected following groups for principal {}: {}", principal, groups); - return groups; - } +public interface GroupCollector { + Iterable collect(String principal); } diff --git a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java new file mode 100644 index 0000000000..cdaf0eb7b2 --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java @@ -0,0 +1,70 @@ +package sonia.scm.group; + +import com.google.common.collect.ImmutableSet; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import sonia.scm.cache.Cache; +import sonia.scm.cache.CacheManager; +import sonia.scm.security.GroupCollector; +import sonia.scm.security.GroupResolver; + +import java.util.Set; + +/** + * Collect groups for a certain principal. + * Warning: The class is only for internal use and should never used directly. + */ +class DefaultGroupCollector implements GroupCollector { + + private static final Logger LOG = LoggerFactory.getLogger(DefaultGroupCollector.class); + + /** Field description */ + public static final String CACHE_NAME = "sonia.cache.externalGroups"; + + /** Field description */ + private final Cache> cache; + private Set groupResolvers; + + private final GroupDAO groupDAO; + + DefaultGroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set groupResolvers) { + this.groupDAO = groupDAO; + this.cache = cacheManager.getCache(CACHE_NAME); + this.groupResolvers = groupResolvers; + } + + @Override + public Iterable collect(String principal) { + + Set externalGroups = cache.get(principal); + + if (externalGroups == null) { + ImmutableSet.Builder newExternalGroups = ImmutableSet.builder(); + + for (GroupResolver groupResolver : groupResolvers) { + Iterable groups = groupResolver.resolveGroups(principal); + groups.forEach(newExternalGroups::add); + } + + cache.put(principal, newExternalGroups.build()); + } + + ImmutableSet.Builder builder = ImmutableSet.builder(); + + builder.add(GroupNames.AUTHENTICATED); + + for (String group : externalGroups) { + builder.add(group); + } + + for (Group group : groupDAO.getAll()) { + if (group.isMember(principal)) { + builder.add(group.getName()); + } + } + + GroupNames groups = new GroupNames(builder.build()); + LOG.debug("collected following groups for principal {}: {}", principal, groups); + return groups; + } +} diff --git a/scm-core/src/test/java/sonia/scm/security/GroupCollectorTest.java b/scm-webapp/src/test/java/sonia/scm/group/GroupCollectorTest.java similarity index 74% rename from scm-core/src/test/java/sonia/scm/security/GroupCollectorTest.java rename to scm-webapp/src/test/java/sonia/scm/group/GroupCollectorTest.java index 3fb59d1614..4bdcd2694a 100644 --- a/scm-core/src/test/java/sonia/scm/security/GroupCollectorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/group/GroupCollectorTest.java @@ -1,6 +1,5 @@ -package sonia.scm.security; +package sonia.scm.group; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; @@ -9,11 +8,8 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import sonia.scm.group.Group; -import sonia.scm.group.GroupDAO; -import sonia.scm.group.GroupNames; +import sonia.scm.security.GroupCollector; -import java.util.Collections; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -30,7 +26,7 @@ class GroupCollectorTest { @Test void shouldAlwaysReturnAuthenticatedGroup() { - GroupNames groupNames = collector.collect("trillian", Collections.emptySet()); + Iterable groupNames = collector.collect("trillian"); assertThat(groupNames).containsOnly("_authenticated"); } @@ -49,13 +45,13 @@ class GroupCollectorTest { @Test void shouldReturnGroupsFromDao() { - GroupNames groupNames = collector.collect("trillian", Collections.emptySet()); + Iterable groupNames = collector.collect("trillian"); assertThat(groupNames).contains("_authenticated", "heartOfGold", "fjordsOfAfrican"); } @Test void shouldCombineGivenWithDao() { - GroupNames groupNames = collector.collect("trillian", ImmutableList.of("awesome", "incredible")); + Iterable groupNames = collector.collect("trillian"); assertThat(groupNames).contains("_authenticated", "heartOfGold", "fjordsOfAfrican", "awesome", "incredible"); } From 8550baaea902b041481e3af251c1e86cac5230cc Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 08:17:17 +0200 Subject: [PATCH 03/12] refactor GroupResolver + GroupCollector --- .../java/sonia/scm/group/GroupCollector.java | 10 ++ .../{security => group}/GroupResolver.java | 7 +- .../sonia/scm/security/GroupCollector.java | 5 - .../scm/group/DefaultGroupCollector.java | 70 ++++++------ .../scm/group/DefaultGroupCollectorTest.java | 100 ++++++++++++++++++ .../sonia/scm/group/GroupCollectorTest.java | 60 ----------- 6 files changed, 150 insertions(+), 102 deletions(-) create mode 100644 scm-core/src/main/java/sonia/scm/group/GroupCollector.java rename scm-core/src/main/java/sonia/scm/{security => group}/GroupResolver.java (51%) delete mode 100644 scm-core/src/main/java/sonia/scm/security/GroupCollector.java create mode 100644 scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java delete mode 100644 scm-webapp/src/test/java/sonia/scm/group/GroupCollectorTest.java diff --git a/scm-core/src/main/java/sonia/scm/group/GroupCollector.java b/scm-core/src/main/java/sonia/scm/group/GroupCollector.java new file mode 100644 index 0000000000..4546db1bc4 --- /dev/null +++ b/scm-core/src/main/java/sonia/scm/group/GroupCollector.java @@ -0,0 +1,10 @@ +package sonia.scm.group; + +import java.util.Set; + +public interface GroupCollector { + + String AUTHENTICATED = "_authenticated"; + + Set collect(String principal); +} diff --git a/scm-core/src/main/java/sonia/scm/security/GroupResolver.java b/scm-core/src/main/java/sonia/scm/group/GroupResolver.java similarity index 51% rename from scm-core/src/main/java/sonia/scm/security/GroupResolver.java rename to scm-core/src/main/java/sonia/scm/group/GroupResolver.java index 3845628913..5aba63c93b 100644 --- a/scm-core/src/main/java/sonia/scm/security/GroupResolver.java +++ b/scm-core/src/main/java/sonia/scm/group/GroupResolver.java @@ -1,9 +1,10 @@ -package sonia.scm.security; +package sonia.scm.group; import sonia.scm.plugin.ExtensionPoint; +import java.util.Set; + @ExtensionPoint public interface GroupResolver { - - Iterable resolveGroups(String principal); + Set resolve(String principal); } diff --git a/scm-core/src/main/java/sonia/scm/security/GroupCollector.java b/scm-core/src/main/java/sonia/scm/security/GroupCollector.java deleted file mode 100644 index 6c9bf2e659..0000000000 --- a/scm-core/src/main/java/sonia/scm/security/GroupCollector.java +++ /dev/null @@ -1,5 +0,0 @@ -package sonia.scm.security; - -public interface GroupCollector { - Iterable collect(String principal); -} diff --git a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java index cdaf0eb7b2..8c6bac8103 100644 --- a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java @@ -1,70 +1,72 @@ package sonia.scm.group; +import com.cronutils.utils.VisibleForTesting; import com.google.common.collect.ImmutableSet; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sonia.scm.cache.Cache; import sonia.scm.cache.CacheManager; -import sonia.scm.security.GroupCollector; -import sonia.scm.security.GroupResolver; +import javax.inject.Inject; +import javax.inject.Singleton; import java.util.Set; /** * Collect groups for a certain principal. * Warning: The class is only for internal use and should never used directly. */ -class DefaultGroupCollector implements GroupCollector { +@Singleton +public class DefaultGroupCollector implements GroupCollector { private static final Logger LOG = LoggerFactory.getLogger(DefaultGroupCollector.class); - /** Field description */ - public static final String CACHE_NAME = "sonia.cache.externalGroups"; - - /** Field description */ - private final Cache> cache; - private Set groupResolvers; + @VisibleForTesting + static final String CACHE_NAME = "sonia.cache.externalGroups"; private final GroupDAO groupDAO; + private final Cache> cache; + private final Set groupResolvers; - DefaultGroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set groupResolvers) { + @Inject + public DefaultGroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set groupResolvers) { this.groupDAO = groupDAO; - this.cache = cacheManager.getCache(CACHE_NAME); + this.cache = cacheManager.getCache(CACHE_NAME); this.groupResolvers = groupResolvers; } @Override - public Iterable collect(String principal) { + public Set collect(String principal) { + ImmutableSet.Builder builder = ImmutableSet.builder(); + builder.add(AUTHENTICATED); + builder.addAll(resolveExternalGroups(principal)); + appendInternalGroups(principal, builder); + + Set groups = builder.build(); + LOG.debug("collected following groups for principal {}: {}", principal, groups); + return groups; + } + + private void appendInternalGroups(String principal, ImmutableSet.Builder builder) { + for (Group group : groupDAO.getAll()) { + if (group.isMember(principal)) { + builder.add(group.getName()); + } + } + } + + private Set resolveExternalGroups(String principal) { Set externalGroups = cache.get(principal); if (externalGroups == null) { ImmutableSet.Builder newExternalGroups = ImmutableSet.builder(); for (GroupResolver groupResolver : groupResolvers) { - Iterable groups = groupResolver.resolveGroups(principal); - groups.forEach(newExternalGroups::add); + newExternalGroups.addAll(groupResolver.resolve(principal)); } - - cache.put(principal, newExternalGroups.build()); + externalGroups = newExternalGroups.build(); + cache.put(principal, externalGroups); } - - ImmutableSet.Builder builder = ImmutableSet.builder(); - - builder.add(GroupNames.AUTHENTICATED); - - for (String group : externalGroups) { - builder.add(group); - } - - for (Group group : groupDAO.getAll()) { - if (group.isMember(principal)) { - builder.add(group.getName()); - } - } - - GroupNames groups = new GroupNames(builder.build()); - LOG.debug("collected following groups for principal {}: {}", principal, groups); - return groups; + return externalGroups; } } diff --git a/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java b/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java new file mode 100644 index 0000000000..edd23151b9 --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java @@ -0,0 +1,100 @@ +package sonia.scm.group; + +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Lists; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import sonia.scm.cache.MapCache; +import sonia.scm.cache.MapCacheManager; + +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class DefaultGroupCollectorTest { + + @Mock + private GroupDAO groupDAO; + + @Mock + private GroupResolver groupResolver; + + private MapCacheManager mapCacheManager; + + private Set groupResolvers; + + private DefaultGroupCollector collector; + + @BeforeEach + void initCollector() { + groupResolvers = new HashSet<>(); + mapCacheManager = new MapCacheManager(); + collector = new DefaultGroupCollector(groupDAO, mapCacheManager, groupResolvers); + } + + @Test + void shouldAlwaysReturnAuthenticatedGroup() { + Iterable groupNames = collector.collect("trillian"); + assertThat(groupNames).containsOnly("_authenticated"); + } + + @Test + void shouldReturnGroupsFromCache() { + MapCache> cache = mapCacheManager.getCache(DefaultGroupCollector.CACHE_NAME); + cache.put("trillian", ImmutableSet.of("awesome", "incredible")); + + Set groups = collector.collect("trillian"); + assertThat(groups).containsOnly("_authenticated", "awesome", "incredible"); + } + + @Test + void shouldNotCallResolverIfExternalGroupsAreCached() { + groupResolvers.add(groupResolver); + + MapCache> cache = mapCacheManager.getCache(DefaultGroupCollector.CACHE_NAME); + cache.put("trillian", ImmutableSet.of("awesome", "incredible")); + + Set groups = collector.collect("trillian"); + assertThat(groups).containsOnly("_authenticated", "awesome", "incredible"); + + verify(groupResolver, never()).resolve("trillian"); + } + + @Nested + class WithGroupsFromDao { + + @BeforeEach + void setUpGroupsDao() { + List groups = Lists.newArrayList( + new Group("xml", "heartOfGold", "trillian"), + new Group("xml", "g42", "dent", "prefect"), + new Group("xml", "fjordsOfAfrican", "dent", "trillian") + ); + when(groupDAO.getAll()).thenReturn(groups); + } + + @Test + void shouldReturnGroupsFromDao() { + Iterable groupNames = collector.collect("trillian"); + assertThat(groupNames).containsOnly("_authenticated", "heartOfGold", "fjordsOfAfrican"); + } + + @Test + void shouldCombineWithResolvers() { + when(groupResolver.resolve("trillian")).thenReturn(ImmutableSet.of("awesome", "incredible")); + groupResolvers.add(groupResolver); + Iterable groupNames = collector.collect("trillian"); + assertThat(groupNames).containsOnly("_authenticated", "heartOfGold", "fjordsOfAfrican", "awesome", "incredible"); + } + } +} diff --git a/scm-webapp/src/test/java/sonia/scm/group/GroupCollectorTest.java b/scm-webapp/src/test/java/sonia/scm/group/GroupCollectorTest.java deleted file mode 100644 index 4bdcd2694a..0000000000 --- a/scm-webapp/src/test/java/sonia/scm/group/GroupCollectorTest.java +++ /dev/null @@ -1,60 +0,0 @@ -package sonia.scm.group; - -import com.google.common.collect.Lists; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Nested; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; -import sonia.scm.security.GroupCollector; - -import java.util.List; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.when; - -@ExtendWith(MockitoExtension.class) -class GroupCollectorTest { - - @Mock - private GroupDAO groupDAO; - - @InjectMocks - private GroupCollector collector; - - @Test - void shouldAlwaysReturnAuthenticatedGroup() { - Iterable groupNames = collector.collect("trillian"); - assertThat(groupNames).containsOnly("_authenticated"); - } - - @Nested - class WithGroupsFromDao { - - @BeforeEach - void setUpGroupsDao() { - List groups = Lists.newArrayList( - new Group("xml", "heartOfGold", "trillian"), - new Group("xml", "g42", "dent", "prefect"), - new Group("xml", "fjordsOfAfrican", "dent", "trillian") - ); - when(groupDAO.getAll()).thenReturn(groups); - } - - @Test - void shouldReturnGroupsFromDao() { - Iterable groupNames = collector.collect("trillian"); - assertThat(groupNames).contains("_authenticated", "heartOfGold", "fjordsOfAfrican"); - } - - @Test - void shouldCombineGivenWithDao() { - Iterable groupNames = collector.collect("trillian"); - assertThat(groupNames).contains("_authenticated", "heartOfGold", "fjordsOfAfrican", "awesome", "incredible"); - } - - } - -} From 442aacbcdb073baa7fbcba60b5b34256bec11ffb Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 09:32:44 +0200 Subject: [PATCH 04/12] remove GroupNames and ExternalGroupNames in favor of GroupCollector --- .../sonia/scm/group/ExternalGroupNames.java | 22 --- .../main/java/sonia/scm/group/GroupNames.java | 187 ------------------ .../scm/security/AccessTokenBuilder.java | 9 - .../scm/security/DAORealmHelperFactory.java | 7 +- .../scm/security/SyncingRealmHelper.java | 122 +----------- .../scm/security/SyncingRealmHelperTest.java | 30 +-- .../sonia/scm/api/v2/resources/MeDto.java | 4 +- .../scm/api/v2/resources/MeDtoFactory.java | 18 +- .../DefaultAuthorizationCollector.java | 32 +-- .../java/sonia/scm/security/DefaultRealm.java | 18 +- .../scm/security/JwtAccessTokenBuilder.java | 18 -- .../DefaultAdministrationContext.java | 10 +- .../api/v2/resources/MeDtoFactoryTest.java | 26 +-- .../DefaultAuthorizationCollectorTest.java | 16 +- .../sonia/scm/security/DefaultRealmTest.java | 97 +++------ .../security/JwtAccessTokenBuilderTest.java | 50 +---- 16 files changed, 100 insertions(+), 566 deletions(-) delete mode 100644 scm-core/src/main/java/sonia/scm/group/ExternalGroupNames.java delete mode 100644 scm-core/src/main/java/sonia/scm/group/GroupNames.java diff --git a/scm-core/src/main/java/sonia/scm/group/ExternalGroupNames.java b/scm-core/src/main/java/sonia/scm/group/ExternalGroupNames.java deleted file mode 100644 index 179e488236..0000000000 --- a/scm-core/src/main/java/sonia/scm/group/ExternalGroupNames.java +++ /dev/null @@ -1,22 +0,0 @@ -package sonia.scm.group; - -import java.util.Collection; - -/** - * This class represents all associated groups which are provided by external systems for a certain user. - * - * @author Sebastian Sdorra - * @since 2.0.0 - */ -public class ExternalGroupNames extends GroupNames { - public ExternalGroupNames() { - } - - public ExternalGroupNames(String groupName, String... groupNames) { - super(groupName, groupNames); - } - - public ExternalGroupNames(Collection collection) { - super(collection); - } -} diff --git a/scm-core/src/main/java/sonia/scm/group/GroupNames.java b/scm-core/src/main/java/sonia/scm/group/GroupNames.java deleted file mode 100644 index c28f9f5ef1..0000000000 --- a/scm-core/src/main/java/sonia/scm/group/GroupNames.java +++ /dev/null @@ -1,187 +0,0 @@ -/** - * Copyright (c) 2010, Sebastian Sdorra - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * 1. Redistributions of source code must retain the above copyright notice, - * this list of conditions and the following disclaimer. - * 2. Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following disclaimer in the documentation - * and/or other materials provided with the distribution. - * 3. Neither the name of SCM-Manager; nor the names of its - * contributors may be used to endorse or promote products derived from this - * software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" - * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY - * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON - * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS - * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - * - * http://bitbucket.org/sdorra/scm-manager - * - */ - - - -package sonia.scm.group; - -//~--- non-JDK imports -------------------------------------------------------- - -import com.google.common.base.Joiner; -import com.google.common.base.Objects; -import com.google.common.collect.Lists; - -import java.io.Serializable; -import java.util.Collection; -import java.util.Collections; -import java.util.Iterator; - -//~--- JDK imports ------------------------------------------------------------ - -/** - * This class represents all associated groups for a user. - * - * @author Sebastian Sdorra - * @since 1.21 - */ -public class GroupNames implements Serializable, Iterable -{ - - /** - * Group for all authenticated users - * @since 1.31 - */ - public static final String AUTHENTICATED = "_authenticated"; - - /** Field description */ - private static final long serialVersionUID = 8615685985213897947L; - - //~--- constructors --------------------------------------------------------- - - /** - * Constructs ... - * - */ - public GroupNames() - { - this(Collections.emptyList()); - } - - /** - * Constructs ... - * - * - * @param groupName - * @param groupNames - */ - public GroupNames(String groupName, String... groupNames) - { - this(Lists.asList(groupName, groupNames)); - } - - /** - * Constructs ... - * - * - * @param collection - */ - public GroupNames(Collection collection) - { - this.collection = Collections.unmodifiableCollection(collection); - } - - //~--- methods -------------------------------------------------------------- - - /** - * Method description - * - * - * @param groupName - * - * @return - */ - public boolean contains(String groupName) - { - return collection.contains(groupName); - } - - /** - * {@inheritDoc} - */ - @Override - public boolean equals(Object obj) - { - if (obj == null) - { - return false; - } - - if (getClass() != obj.getClass()) - { - return false; - } - - final GroupNames other = (GroupNames) obj; - - return Objects.equal(collection, other.collection); - } - - /** - * {@inheritDoc} - */ - @Override - public int hashCode() - { - return Objects.hashCode(collection); - } - - /** - * Method description - * - * - * @return - */ - @Override - public Iterator iterator() - { - return collection.iterator(); - } - - /** - * Method description - * - * - * @return - */ - @Override - public String toString() - { - return Joiner.on(", ").join(collection); - } - - //~--- get methods ---------------------------------------------------------- - - /** - * Method description - * - * - * @return - */ - public Collection getCollection() - { - return collection; - } - - - //~--- fields --------------------------------------------------------------- - /** Field description */ - private final Collection collection; -} diff --git a/scm-core/src/main/java/sonia/scm/security/AccessTokenBuilder.java b/scm-core/src/main/java/sonia/scm/security/AccessTokenBuilder.java index 0924716bd8..afe81ac27f 100644 --- a/scm-core/src/main/java/sonia/scm/security/AccessTokenBuilder.java +++ b/scm-core/src/main/java/sonia/scm/security/AccessTokenBuilder.java @@ -99,15 +99,6 @@ public interface AccessTokenBuilder { */ AccessTokenBuilder scope(Scope scope); - /** - * Define the logged in user as member of the given groups. - * - * @param groups group names - * - * @return {@code this} - */ - AccessTokenBuilder groups(String... groups); - /** * Creates a new {@link AccessToken} with the provided settings. * diff --git a/scm-core/src/main/java/sonia/scm/security/DAORealmHelperFactory.java b/scm-core/src/main/java/sonia/scm/security/DAORealmHelperFactory.java index b503ff8375..dd59de2ac8 100644 --- a/scm-core/src/main/java/sonia/scm/security/DAORealmHelperFactory.java +++ b/scm-core/src/main/java/sonia/scm/security/DAORealmHelperFactory.java @@ -31,7 +31,6 @@ package sonia.scm.security; import sonia.scm.cache.CacheManager; -import sonia.scm.group.GroupDAO; import sonia.scm.user.UserDAO; import javax.inject.Inject; @@ -47,21 +46,17 @@ public final class DAORealmHelperFactory { private final LoginAttemptHandler loginAttemptHandler; private final UserDAO userDAO; private final CacheManager cacheManager; - private final GroupResolver groupResolver; /** * Constructs a new instance. * @param loginAttemptHandler login attempt handler * @param userDAO user dao - * @param groupDAO group dao * @param cacheManager - * @param groupResolver */ @Inject - public DAORealmHelperFactory(LoginAttemptHandler loginAttemptHandler, UserDAO userDAO, GroupDAO groupDAO, CacheManager cacheManager, GroupResolver groupResolver) { + public DAORealmHelperFactory(LoginAttemptHandler loginAttemptHandler, UserDAO userDAO, CacheManager cacheManager) { this.loginAttemptHandler = loginAttemptHandler; this.userDAO = userDAO; - this.groupResolver = groupResolver; this.cacheManager = cacheManager; } 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 b209184902..b2175f304a 100644 --- a/scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java +++ b/scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java @@ -32,25 +32,15 @@ import com.google.inject.Inject; import org.apache.shiro.authc.AuthenticationInfo; import org.apache.shiro.authc.SimpleAuthenticationInfo; import org.apache.shiro.subject.SimplePrincipalCollection; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import sonia.scm.AlreadyExistsException; import sonia.scm.NotFoundException; -import sonia.scm.cache.CacheManager; -import sonia.scm.group.ExternalGroupNames; import sonia.scm.group.Group; -import sonia.scm.group.GroupDAO; import sonia.scm.group.GroupManager; import sonia.scm.plugin.Extension; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.web.security.AdministrationContext; -import java.util.Collection; -import java.util.Collections; - -import static java.util.Arrays.asList; - /** * Helper class for syncing realms. The class should simplify the creation of realms, which are syncing authenticated * users with the local database. @@ -61,12 +51,9 @@ import static java.util.Arrays.asList; @Extension public final class SyncingRealmHelper { - private static final Logger LOG = LoggerFactory.getLogger(SyncingRealmHelper.class); - private final AdministrationContext ctx; private final UserManager userManager; private final GroupManager groupManager; - private final CacheManager cacheManager; /** * Constructs a new SyncingRealmHelper. @@ -74,133 +61,28 @@ public final class SyncingRealmHelper { * @param ctx administration context * @param userManager user manager * @param groupManager group manager - * @param groupDAO group dao */ @Inject - public SyncingRealmHelper(AdministrationContext ctx, UserManager userManager, GroupManager groupManager, GroupDAO groupDAO, CacheManager cacheManager) { + public SyncingRealmHelper(AdministrationContext ctx, UserManager userManager, GroupManager groupManager) { this.ctx = ctx; this.userManager = userManager; this.groupManager = groupManager; - this.cacheManager = cacheManager; } - /** - * Create {@link AuthenticationInfo} from user and groups. - */ - public AuthenticationInfoBuilder.ForRealm authenticationInfo() { - return new AuthenticationInfoBuilder().new ForRealm(); - } - - public class AuthenticationInfoBuilder { - private String realm; - private User user; - private Collection groups = Collections.emptySet(); - private Collection externalGroups = Collections.emptySet(); - - private AuthenticationInfo build() { - return SyncingRealmHelper.this.createAuthenticationInfo(realm, user, groups, externalGroups); - } - - 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(); - } - } - - 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(); - } - } - - public class WithGroups { - private WithGroups() { - } - - /** - * Set the internal groups for the user. - * @param groups groups of the authenticated user - * @return builder step for groups - */ - public WithGroups withGroups(String... groups) { - return withGroups(asList(groups)); - } - - /** - * Set the internal groups for the user. - * @param groups groups of the authenticated user - * @return builder step for groups - */ - public WithGroups withGroups(Collection groups) { - AuthenticationInfoBuilder.this.groups = groups; - return this; - } - - /** - * Set the external groups for the user. - * @param externalGroups external groups of the authenticated user - * @return builder step for groups - */ - public WithGroups withExternalGroups(String... externalGroups) { - return withExternalGroups(asList(externalGroups)); - } - - /** - * Set the external groups for the user. - * @param externalGroups external groups of the authenticated user - * @return builder step for groups - */ - public WithGroups withExternalGroups(Collection externalGroups) { - AuthenticationInfoBuilder.this.externalGroups = externalGroups; - return this; - } - - /** - * Builds the {@link AuthenticationInfo} from the given options. - * - * @return complete autentication info - */ - public AuthenticationInfo build() { - return AuthenticationInfoBuilder.this.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 */ - private AuthenticationInfo createAuthenticationInfo(String realm, User user, - Collection groups, Collection externalGroups) { + public AuthenticationInfo createAuthenticationInfo(String realm, User user) { SimplePrincipalCollection collection = new SimplePrincipalCollection(); collection.add(user.getId(), realm); collection.add(user, realm); - collection.add(new ExternalGroupNames(externalGroups), realm); return new SimpleAuthenticationInfo(collection, user.getPassword()); } 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 b7fbd97aac..ca7c2efdc6 100644 --- a/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java +++ b/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java @@ -37,17 +37,13 @@ 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; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import sonia.scm.AlreadyExistsException; -import sonia.scm.cache.CacheManager; -import sonia.scm.group.ExternalGroupNames; import sonia.scm.group.Group; -import sonia.scm.group.GroupDAO; import sonia.scm.group.GroupManager; import sonia.scm.user.User; import sonia.scm.user.UserManager; @@ -81,12 +77,6 @@ public class SyncingRealmHelperTest { @Mock private UserManager userManager; - @Mock - private GroupDAO groupDAO; - - @Mock - CacheManager cacheManager; - private SyncingRealmHelper helper; /** @@ -112,7 +102,7 @@ public class SyncingRealmHelperTest { } }; - helper = new SyncingRealmHelper(ctx, userManager, groupManager, groupDAO, cacheManager); + helper = new SyncingRealmHelper(ctx, userManager, groupManager); } /** @@ -189,27 +179,11 @@ public class SyncingRealmHelperTest { verify(userManager, times(1)).modify(user); } - @Test - public void builderShouldSetExternalGroups() { - AuthenticationInfo authenticationInfo = helper - .authenticationInfo() - .forRealm("unit-test") - .andUser(new User("ziltoid")) - .withExternalGroups("external") - .build(); - - ExternalGroupNames groupNames = authenticationInfo.getPrincipals().oneByType(ExternalGroupNames.class); - Assertions.assertThat(groupNames.getCollection()).containsOnly("external"); - } @Test public void builderShouldSetValues() { User user = new User("ziltoid"); - AuthenticationInfo authInfo = helper - .authenticationInfo() - .forRealm("unit-test") - .andUser(user) - .build(); + AuthenticationInfo authInfo = helper.createAuthenticationInfo("unit-test", user); assertNotNull(authInfo); assertEquals("ziltoid", authInfo.getPrincipals().getPrimaryPrincipal()); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeDto.java index 84fbbfe290..a87ad7df17 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeDto.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeDto.java @@ -7,7 +7,7 @@ import lombok.Getter; import lombok.NoArgsConstructor; import lombok.Setter; -import java.util.List; +import java.util.Set; @Getter @Setter @@ -17,7 +17,7 @@ public class MeDto extends HalRepresentation { private String name; private String displayName; private String mail; - private List groups; + private Set groups; MeDto(Links links, Embedded embedded) { super(links, embedded); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeDtoFactory.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeDtoFactory.java index b5e1998066..c2bebd389a 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeDtoFactory.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeDtoFactory.java @@ -1,18 +1,16 @@ package sonia.scm.api.v2.resources; -import com.google.common.collect.ImmutableList; import de.otto.edison.hal.Embedded; import de.otto.edison.hal.Links; import org.apache.shiro.SecurityUtils; import org.apache.shiro.subject.PrincipalCollection; import org.apache.shiro.subject.Subject; -import sonia.scm.group.GroupNames; +import sonia.scm.group.GroupCollector; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.user.UserPermissions; import javax.inject.Inject; -import java.util.Collections; import static de.otto.edison.hal.Embedded.embeddedBuilder; import static de.otto.edison.hal.Link.link; @@ -22,11 +20,13 @@ public class MeDtoFactory extends HalAppenderMapper { private final ResourceLinks resourceLinks; private final UserManager userManager; + private final GroupCollector groupCollector; @Inject - public MeDtoFactory(ResourceLinks resourceLinks, UserManager userManager) { + public MeDtoFactory(ResourceLinks resourceLinks, UserManager userManager, GroupCollector groupCollector) { this.resourceLinks = resourceLinks; this.userManager = userManager; + this.groupCollector = groupCollector; } public MeDto create() { @@ -35,16 +35,12 @@ public class MeDtoFactory extends HalAppenderMapper { MeDto dto = createDto(user); mapUserProperties(user, dto); - mapGroups(principals, dto); + mapGroups(user, dto); return dto; } - private void mapGroups(PrincipalCollection principals, MeDto dto) { - Iterable groups = principals.oneByType(GroupNames.class); - if (groups == null) { - groups = Collections.emptySet(); - } - dto.setGroups(ImmutableList.copyOf(groups)); + private void mapGroups(User user, MeDto dto) { + dto.setGroups(groupCollector.collect(user.getName())); } private void mapUserProperties(User user, MeDto dto) { diff --git a/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java b/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java index baff3b951c..28f61df34f 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java @@ -52,17 +52,18 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sonia.scm.cache.Cache; import sonia.scm.cache.CacheManager; -import sonia.scm.group.GroupNames; +import sonia.scm.group.GroupCollector; import sonia.scm.group.GroupPermissions; import sonia.scm.plugin.Extension; -import sonia.scm.repository.RepositoryPermission; import sonia.scm.repository.Repository; import sonia.scm.repository.RepositoryDAO; +import sonia.scm.repository.RepositoryPermission; import sonia.scm.user.User; import sonia.scm.user.UserPermissions; import sonia.scm.util.Util; import java.util.Collection; +import java.util.Set; //~--- JDK imports ------------------------------------------------------------ @@ -88,19 +89,21 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector /** * Constructs ... - * @param cacheManager + * @param cacheManager * @param repositoryDAO * @param securitySystem * @param repositoryPermissionProvider + * @param groupCollector */ @Inject public DefaultAuthorizationCollector(CacheManager cacheManager, - RepositoryDAO repositoryDAO, SecuritySystem securitySystem, RepositoryPermissionProvider repositoryPermissionProvider) + RepositoryDAO repositoryDAO, SecuritySystem securitySystem, RepositoryPermissionProvider repositoryPermissionProvider, GroupCollector groupCollector) { this.cache = cacheManager.getCache(CACHE_NAME); this.repositoryDAO = repositoryDAO; this.securitySystem = securitySystem; this.repositoryPermissionProvider = repositoryPermissionProvider; + this.groupCollector = groupCollector; } //~--- methods -------------------------------------------------------------- @@ -145,16 +148,16 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector Preconditions.checkNotNull(user, "no user found in principal collection"); - GroupNames groupNames = principals.oneByType(GroupNames.class); + Set groups = groupCollector.collect(user.getName()); - CacheKey cacheKey = new CacheKey(user.getId(), groupNames); + CacheKey cacheKey = new CacheKey(user.getId(), groups); AuthorizationInfo info = cache.get(cacheKey); if (info == null) { logger.trace("collect AuthorizationInfo for user {}", user.getName()); - info = createAuthorizationInfo(user, groupNames); + info = createAuthorizationInfo(user, groups); cache.put(cacheKey, info); } else if (logger.isTraceEnabled()) @@ -166,7 +169,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector } private void collectGlobalPermissions(Builder builder, - final User user, final GroupNames groups) + final User user, final Set groups) { Collection globalPermissions = securitySystem.getPermissions((AssignedPermission input) -> isUserPermitted(user, groups, input)); @@ -181,7 +184,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector } private void collectRepositoryPermissions(Builder builder, User user, - GroupNames groups) + Set groups) { for (Repository repository : repositoryDAO.getAll()) { @@ -190,7 +193,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector } private void collectRepositoryPermissions(Builder builder, - Repository repository, User user, GroupNames groups) + Repository repository, User user, Set groups) { Collection repositoryPermissions = repository.getPermissions(); @@ -245,7 +248,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector .getVerbs(); } - private AuthorizationInfo createAuthorizationInfo(User user, GroupNames groups) { + private AuthorizationInfo createAuthorizationInfo(User user, Set groups) { Builder builder = ImmutableSet.builder(); collectGlobalPermissions(builder, user, groups); @@ -279,7 +282,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector //~--- get methods ---------------------------------------------------------- - private boolean isUserPermitted(User user, GroupNames groups, + private boolean isUserPermitted(User user, Set groups, PermissionObject perm) { //J- @@ -314,7 +317,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector */ private static class CacheKey { - private CacheKey(String username, GroupNames groupnames) + private CacheKey(String username, Set groupnames) { this.username = username; this.groupnames = groupnames; @@ -356,7 +359,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector //~--- fields ------------------------------------------------------------- /** group names */ - private final GroupNames groupnames; + private final Set groupnames; /** username */ private final String username; @@ -374,4 +377,5 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector private final SecuritySystem securitySystem; private final RepositoryPermissionProvider repositoryPermissionProvider; + private final GroupCollector groupCollector; } diff --git a/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java b/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java index bacbd9b314..245dcadb78 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java @@ -34,7 +34,6 @@ package sonia.scm.security; //~--- non-JDK imports -------------------------------------------------------- import com.google.common.annotations.VisibleForTesting; - import org.apache.shiro.authc.AuthenticationException; import org.apache.shiro.authc.AuthenticationInfo; import org.apache.shiro.authc.AuthenticationToken; @@ -45,21 +44,16 @@ import org.apache.shiro.authz.AuthorizationInfo; import org.apache.shiro.authz.SimpleAuthorizationInfo; import org.apache.shiro.realm.AuthorizingRealm; import org.apache.shiro.subject.PrincipalCollection; - -import org.apache.shiro.subject.SimplePrincipalCollection; -import sonia.scm.group.GroupNames; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import sonia.scm.plugin.Extension; -//~--- JDK imports ------------------------------------------------------------ - import javax.inject.Inject; import javax.inject.Singleton; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.util.Set; +//~--- JDK imports ------------------------------------------------------------ + /** * Default authorizing realm. * @@ -149,7 +143,7 @@ public class DefaultRealm extends AuthorizingRealm LOG.trace("principal does not contain scope information, returning all permissions"); log(principals, info, null); } - + return info; } @@ -180,8 +174,6 @@ public class DefaultRealm extends AuthorizingRealm StringBuilder buffer = new StringBuilder("authorization summary: "); buffer.append(SEPARATOR).append("username : ").append(collection.getPrimaryPrincipal()); - buffer.append(SEPARATOR).append("groups : "); - append(buffer, collection.oneByType(GroupNames.class)); buffer.append(SEPARATOR).append("roles : "); append(buffer, original.getRoles()); buffer.append(SEPARATOR).append("scope : "); 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 cc1d4be7a7..5a74f77502 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java +++ b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java @@ -40,11 +40,9 @@ import org.apache.shiro.SecurityUtils; import org.apache.shiro.subject.Subject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import sonia.scm.group.ExternalGroupNames; import java.time.Clock; import java.time.Instant; -import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.HashSet; @@ -139,12 +137,6 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { return this; } - @Override - public JwtAccessTokenBuilder groups(String... groups) { - Collections.addAll(this.groups, groups); - return this; - } - JwtAccessTokenBuilder refreshExpiration(Instant refreshExpiration) { this.refreshExpiration = refreshExpiration; this.refreshableFor = 0; @@ -206,16 +198,6 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { claims.setIssuer(issuer); } - if (!groups.isEmpty()) { - claims.put(JwtAccessToken.GROUPS_CLAIM_KEY, groups); - } else { - Subject currentSubject = SecurityUtils.getSubject(); - ExternalGroupNames externalGroupNames = currentSubject.getPrincipals().oneByType(ExternalGroupNames.class); - if (externalGroupNames != null) { - claims.put(JwtAccessToken.GROUPS_CLAIM_KEY, externalGroupNames.getCollection().toArray(new String[]{})); - } - } - // sign token and create compact version String compact = Jwts.builder() .setClaims(claims) diff --git a/scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java b/scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java index 0b380c8088..b62b6c63f3 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java +++ b/scm-webapp/src/main/java/sonia/scm/web/security/DefaultAdministrationContext.java @@ -38,7 +38,6 @@ package sonia.scm.web.security; import com.google.inject.Inject; import com.google.inject.Injector; import com.google.inject.Singleton; - import org.apache.shiro.SecurityUtils; import org.apache.shiro.subject.PrincipalCollection; import org.apache.shiro.subject.SimplePrincipalCollection; @@ -46,21 +45,17 @@ import org.apache.shiro.subject.Subject; import org.apache.shiro.subject.support.SubjectThreadState; import org.apache.shiro.util.ThreadContext; import org.apache.shiro.util.ThreadState; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; - import sonia.scm.SCMContext; -import sonia.scm.group.GroupNames; import sonia.scm.security.Role; import sonia.scm.user.User; import sonia.scm.util.AssertUtil; -//~--- JDK imports ------------------------------------------------------------ - +import javax.xml.bind.JAXB; import java.net.URL; -import javax.xml.bind.JAXB; +//~--- JDK imports ------------------------------------------------------------ /** * @@ -161,7 +156,6 @@ public class DefaultAdministrationContext implements AdministrationContext collection.add(adminUser.getId(), REALM); collection.add(adminUser, REALM); - collection.add(new GroupNames(), REALM); collection.add(AdministrationContextMarker.MARKER, REALM); return collection; diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeDtoFactoryTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeDtoFactoryTest.java index 8a00c69229..d9572dc04c 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeDtoFactoryTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeDtoFactoryTest.java @@ -1,9 +1,9 @@ package sonia.scm.api.v2.resources; +import com.google.common.collect.ImmutableSet; import org.apache.shiro.subject.PrincipalCollection; import org.apache.shiro.subject.Subject; import org.apache.shiro.util.ThreadContext; -import org.assertj.core.util.Lists; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -12,7 +12,7 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; -import sonia.scm.group.GroupNames; +import sonia.scm.group.GroupCollector; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.user.UserTestData; @@ -20,7 +20,6 @@ import sonia.scm.user.UserTestData; import java.net.URI; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -33,6 +32,9 @@ class MeDtoFactoryTest { @Mock private UserManager userManager; + @Mock + private GroupCollector groupCollector; + @Mock private Subject subject; @@ -42,7 +44,7 @@ class MeDtoFactoryTest { void setUpContext() { ThreadContext.bind(subject); ResourceLinks resourceLinks = ResourceLinksMock.createMock(baseUri); - meDtoFactory = new MeDtoFactory(resourceLinks, userManager); + meDtoFactory = new MeDtoFactory(resourceLinks, userManager, groupCollector); } @AfterEach @@ -69,24 +71,16 @@ class MeDtoFactoryTest { @Test void shouldCreateMeDtoWithGroups() { - prepareSubject(UserTestData.createTrillian(), "HeartOfGold", "Puzzle42"); + when(groupCollector.collect("trillian")).thenReturn(ImmutableSet.of("HeartOfGold", "Puzzle42")); + prepareSubject(UserTestData.createTrillian()); MeDto dto = meDtoFactory.create(); assertThat(dto.getGroups()).containsOnly("HeartOfGold", "Puzzle42"); } - private void prepareSubject(User user, String... groups) { + private void prepareSubject(User user) { PrincipalCollection collection = mock(PrincipalCollection.class); when(subject.getPrincipals()).thenReturn(collection); - when(collection.oneByType(any(Class.class))).then(ic -> { - Class type = ic.getArgument(0); - if (type.isAssignableFrom(User.class)) { - return user; - } else if (type.isAssignableFrom(GroupNames.class)) { - return new GroupNames(Lists.newArrayList(groups)); - } else { - return null; - } - }); + when(collection.oneByType(User.class)).thenReturn(user); } @Test diff --git a/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java b/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java index 8e7cb8a70e..930a06d249 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java @@ -33,6 +33,7 @@ package sonia.scm.security; import com.github.sdorra.shiro.ShiroRule; import com.github.sdorra.shiro.SubjectAware; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import org.apache.shiro.authz.AuthorizationInfo; import org.apache.shiro.authz.SimpleAuthorizationInfo; @@ -49,7 +50,7 @@ import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; import sonia.scm.cache.Cache; import sonia.scm.cache.CacheManager; -import sonia.scm.group.GroupNames; +import sonia.scm.group.GroupCollector; import sonia.scm.repository.Repository; import sonia.scm.repository.RepositoryDAO; import sonia.scm.repository.RepositoryPermission; @@ -58,8 +59,6 @@ import sonia.scm.repository.RepositoryTestData; import sonia.scm.user.User; import sonia.scm.user.UserTestData; -import java.util.Collections; - import static java.util.Arrays.asList; import static java.util.Collections.singletonList; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -96,6 +95,9 @@ public class DefaultAuthorizationCollectorTest { @Mock private RepositoryPermissionProvider repositoryPermissionProvider; + @Mock + private GroupCollector groupCollector; + private DefaultAuthorizationCollector collector; @Rule @@ -107,7 +109,7 @@ public class DefaultAuthorizationCollectorTest { @Before public void setUp(){ when(cacheManager.getCache(Mockito.any(String.class))).thenReturn(cache); - collector = new DefaultAuthorizationCollector(cacheManager, repositoryDAO, securitySystem, repositoryPermissionProvider); + collector = new DefaultAuthorizationCollector(cacheManager, repositoryDAO, securitySystem, repositoryPermissionProvider, groupCollector); } /** @@ -290,9 +292,13 @@ public class DefaultAuthorizationCollectorTest { SimplePrincipalCollection spc = new SimplePrincipalCollection(); spc.add(user.getName(), "unit"); spc.add(user, "unit"); - spc.add(new GroupNames(group, groups), "unit"); Subject subject = new Subject.Builder().authenticated(true).principals(spc).buildSubject(); shiro.setSubject(subject); + + ImmutableSet.Builder builder = ImmutableSet.builder(); + builder.add(group); + builder.add(groups); + when(groupCollector.collect(user.getName())).thenReturn(builder.build()); } /** diff --git a/scm-webapp/src/test/java/sonia/scm/security/DefaultRealmTest.java b/scm-webapp/src/test/java/sonia/scm/security/DefaultRealmTest.java index b6fea9e897..ba23411b36 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/DefaultRealmTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/DefaultRealmTest.java @@ -36,8 +36,6 @@ package sonia.scm.security; //~--- non-JDK imports -------------------------------------------------------- import com.google.common.collect.Collections2; -import com.google.common.collect.Lists; - import org.apache.shiro.authc.AuthenticationInfo; import org.apache.shiro.authc.AuthenticationToken; import org.apache.shiro.authc.DisabledAccountException; @@ -45,43 +43,44 @@ import org.apache.shiro.authc.IncorrectCredentialsException; import org.apache.shiro.authc.UnknownAccountException; import org.apache.shiro.authc.UsernamePasswordToken; import org.apache.shiro.authc.credential.DefaultPasswordService; -import org.apache.shiro.crypto.hash.DefaultHashService; -import org.apache.shiro.subject.PrincipalCollection; -import org.apache.shiro.subject.SimplePrincipalCollection; - -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; - -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; - -import sonia.scm.group.Group; -import sonia.scm.group.GroupDAO; -import sonia.scm.group.GroupNames; -import sonia.scm.user.User; -import sonia.scm.user.UserDAO; -import sonia.scm.user.UserTestData; - -import static org.hamcrest.Matchers.*; - -import static org.junit.Assert.*; - -import static org.mockito.Mockito.*; - -//~--- JDK imports ------------------------------------------------------------ - -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Set; - import org.apache.shiro.authz.AuthorizationInfo; import org.apache.shiro.authz.Permission; import org.apache.shiro.authz.SimpleAuthorizationInfo; import org.apache.shiro.authz.permission.WildcardPermissionResolver; +import org.apache.shiro.crypto.hash.DefaultHashService; +import org.apache.shiro.subject.PrincipalCollection; +import org.apache.shiro.subject.SimplePrincipalCollection; import org.hamcrest.Matchers; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +import sonia.scm.group.GroupDAO; +import sonia.scm.user.User; +import sonia.scm.user.UserDAO; +import sonia.scm.user.UserTestData; + +import java.util.HashSet; +import java.util.Set; + +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +//~--- JDK imports ------------------------------------------------------------ /** * @@ -206,32 +205,6 @@ public class DefaultRealmTest ); } - /** - * Method description - * - */ - @Test - public void testGroupCollection() - { - User user = UserTestData.createTrillian(); - //J- - List groups = Lists.newArrayList( - new Group(DefaultRealm.REALM, "scm", user.getName()), - new Group(DefaultRealm.REALM, "developers", "perfect") - ); - //J+ - - when(groupDAO.getAll()).thenReturn(groups); - - UsernamePasswordToken token = daoUser(user, "secret"); - AuthenticationInfo info = realm.getAuthenticationInfo(token); - GroupNames groupNames = info.getPrincipals().oneByType(GroupNames.class); - - assertNotNull(groupNames); - assertThat(groupNames.getCollection(), hasSize(2)); - assertThat(groupNames, hasItems("scm", GroupNames.AUTHENTICATED)); - } - /** * Method description * @@ -251,12 +224,6 @@ public class DefaultRealmTest assertThat(collection.getRealmNames(), hasSize(1)); assertThat(collection.getRealmNames(), hasItem(DefaultRealm.REALM)); assertEquals(user, collection.oneByType(User.class)); - - GroupNames groups = collection.oneByType(GroupNames.class); - - assertNotNull(groups); - assertThat(groups.getCollection(), hasSize(1)); - assertThat(groups.getCollection(), hasItem(GroupNames.AUTHENTICATED)); } /** 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 e2117235fc..7a8c0ef169 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java @@ -36,27 +36,25 @@ 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.ExternalGroupNames; -import java.util.Arrays; -import java.util.List; import java.util.Set; import java.util.concurrent.TimeUnit; -import static org.hamcrest.Matchers.*; -import static org.junit.Assert.*; -import static org.mockito.Mockito.*; +import static org.hamcrest.Matchers.isEmptyOrNullString; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; +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.when; import static sonia.scm.security.SecureKeyTestUtil.createSecureKey; /** @@ -137,7 +135,6 @@ public class JwtAccessTokenBuilderTest { .issuer("https://www.scm-manager.org") .expiresIn(5, TimeUnit.SECONDS) .custom("a", "b") - .groups("one", "two", "three") .scope(Scope.valueOf("repo:*")) .build(); @@ -154,36 +151,6 @@ 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()); - - List groupCollection = Arrays.asList(groups); - if (external) { - when(principals.oneByType(ExternalGroupNames.class)).thenReturn(new ExternalGroupNames(groupCollection)); - } - return principals; - } - private void assertClaims(JwtAccessToken token){ assertThat(token.getId(), not(isEmptyOrNullString())); assertNotNull( token.getIssuedAt() ); @@ -194,6 +161,5 @@ public class JwtAccessTokenBuilderTest { assertEquals(token.getIssuer().get(), "https://www.scm-manager.org"); assertEquals("b", token.getCustom("a").get()); assertEquals("[\"repo:*\"]", token.getScope().toString()); - assertThat(token.getGroups(), containsInAnyOrder("one", "two", "three")); } } From b029b80f63e53257d640f0ea17390b983ccd9477 Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 09:54:40 +0200 Subject: [PATCH 05/12] fix failed unit test after refactoring MeDtoFactory --- .../sonia/scm/api/v2/resources/MeResourceTest.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeResourceTest.java index cd2a172c1b..7a3d1b4304 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeResourceTest.java @@ -2,6 +2,7 @@ package sonia.scm.api.v2.resources; import com.github.sdorra.shiro.ShiroRule; import com.github.sdorra.shiro.SubjectAware; +import com.google.common.collect.ImmutableSet; import org.apache.shiro.SecurityUtils; import org.apache.shiro.authc.credential.PasswordService; import org.apache.shiro.subject.PrincipalCollection; @@ -16,6 +17,7 @@ import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import sonia.scm.ContextEntry; +import sonia.scm.group.GroupCollector; import sonia.scm.user.InvalidPasswordException; import sonia.scm.user.User; import sonia.scm.user.UserManager; @@ -28,7 +30,12 @@ import java.net.URISyntaxException; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; import static org.mockito.MockitoAnnotations.initMocks; import static sonia.scm.api.v2.resources.DispatcherMock.createDispatcher; @@ -50,6 +57,9 @@ public class MeResourceTest { @Mock private ScmPathInfoStore scmPathInfoStore; + @Mock + private GroupCollector groupCollector; + @Mock private UserManager userManager; @@ -69,6 +79,7 @@ public class MeResourceTest { when(userManager.create(userCaptor.capture())).thenAnswer(invocation -> invocation.getArguments()[0]); doNothing().when(userManager).modify(userCaptor.capture()); doNothing().when(userManager).delete(userCaptor.capture()); + when(groupCollector.collect("trillian")).thenReturn(ImmutableSet.of("group1", "group2")); when(userManager.isTypeDefault(userCaptor.capture())).thenCallRealMethod(); when(userManager.getDefaultType()).thenReturn("xml"); MeResource meResource = new MeResource(meDtoFactory, userManager, passwordService); From ef0c57b83f1575b7f4a306483a96f6be06d6f40c Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 09:55:30 +0200 Subject: [PATCH 06/12] bind GroupCollector to DefaultGroupCollector --- .../java/sonia/scm/lifecycle/modules/ScmServletModule.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ScmServletModule.java b/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ScmServletModule.java index 7ece64f719..cd34d03b50 100644 --- a/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ScmServletModule.java +++ b/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ScmServletModule.java @@ -50,8 +50,10 @@ import sonia.scm.cache.CacheManager; import sonia.scm.cache.GuavaCacheManager; import sonia.scm.config.ScmConfiguration; import sonia.scm.event.ScmEventBus; +import sonia.scm.group.DefaultGroupCollector; import sonia.scm.group.DefaultGroupDisplayManager; import sonia.scm.group.DefaultGroupManager; +import sonia.scm.group.GroupCollector; import sonia.scm.group.GroupDAO; import sonia.scm.group.GroupDisplayManager; import sonia.scm.group.GroupManager; @@ -195,7 +197,7 @@ class ScmServletModule extends ServletModule { bindDecorated(GroupManager.class, DefaultGroupManager.class, GroupManagerProvider.class); bind(GroupDisplayManager.class, DefaultGroupDisplayManager.class); - + bind(GroupCollector.class, DefaultGroupCollector.class); bind(CGIExecutorFactory.class, DefaultCGIExecutorFactory.class); // bind sslcontext provider From b4bee0758ddbafbe2c8d3ecc23383776aae88602 Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 09:55:51 +0200 Subject: [PATCH 07/12] adjust caching for external groups --- scm-webapp/src/main/resources/config/gcache.xml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scm-webapp/src/main/resources/config/gcache.xml b/scm-webapp/src/main/resources/config/gcache.xml index eb59f36446..28876558df 100644 --- a/scm-webapp/src/main/resources/config/gcache.xml +++ b/scm-webapp/src/main/resources/config/gcache.xml @@ -40,15 +40,15 @@ /> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - From 47e323e468daf1783d7c6b144b8cc03c8ad706ae Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 12:01:53 +0200 Subject: [PATCH 09/12] disable shiro cache for the DefaultRealm --- scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java b/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java index 245dcadb78..e65c88e679 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java @@ -97,6 +97,9 @@ public class DefaultRealm extends AuthorizingRealm matcher.setPasswordService(service); setCredentialsMatcher(helper.wrapCredentialsMatcher(matcher)); setAuthenticationTokenClass(UsernamePasswordToken.class); + + // we cache in the AuthorizationCollector + setCachingEnabled(false); } //~--- methods -------------------------------------------------------------- From 0890707b3fe52e0b3ad344726176e9beb00d4b77 Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 14:46:33 +0200 Subject: [PATCH 10/12] fix permission reference on clone repositories --- .../java/sonia/scm/repository/Repository.java | 4 +++- .../sonia/scm/repository/RepositoryTest.java | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 scm-core/src/test/java/sonia/scm/repository/RepositoryTest.java diff --git a/scm-core/src/main/java/sonia/scm/repository/Repository.java b/scm-core/src/main/java/sonia/scm/repository/Repository.java index 5bb50db06f..8c7000c25a 100644 --- a/scm-core/src/main/java/sonia/scm/repository/Repository.java +++ b/scm-core/src/main/java/sonia/scm/repository/Repository.java @@ -82,7 +82,7 @@ public class Repository extends BasicPropertiesAware implements ModelObject, Per private String namespace; private String name; @XmlElement(name = "permission") - private final Set permissions = new HashSet<>(); + private Set permissions = new HashSet<>(); @XmlElement(name = "public") private boolean publicReadable = false; private boolean archived = false; @@ -331,6 +331,8 @@ public class Repository extends BasicPropertiesAware implements ModelObject, Per try { repository = (Repository) super.clone(); + // fix permission reference on clone + repository.permissions = new HashSet<>(permissions); } catch (CloneNotSupportedException ex) { throw new RuntimeException(ex); } diff --git a/scm-core/src/test/java/sonia/scm/repository/RepositoryTest.java b/scm-core/src/test/java/sonia/scm/repository/RepositoryTest.java new file mode 100644 index 0000000000..6053e10ad5 --- /dev/null +++ b/scm-core/src/test/java/sonia/scm/repository/RepositoryTest.java @@ -0,0 +1,22 @@ +package sonia.scm.repository; + +import org.junit.jupiter.api.Test; + +import java.util.Arrays; + +import static org.assertj.core.api.Assertions.assertThat; + +class RepositoryTest { + + @Test + void shouldCreateNewPermissionOnClone() { + Repository repository = new Repository(); + repository.setPermissions(Arrays.asList(new RepositoryPermission("one", "role", false))); + + Repository cloned = repository.clone(); + cloned.setPermissions(Arrays.asList(new RepositoryPermission("two", "role", false))); + + assertThat(repository.getPermissions()).extracting(r -> r.getName()).containsOnly("one"); + } + +} From e33b4dc9fc16a1eb8eb30db48437726119b62782 Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 14:46:58 +0200 Subject: [PATCH 11/12] removed unused method --- .../test/java/sonia/scm/it/utils/TestData.java | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/scm-it/src/test/java/sonia/scm/it/utils/TestData.java b/scm-it/src/test/java/sonia/scm/it/utils/TestData.java index 20de47ffa4..d632f13f60 100644 --- a/scm-it/src/test/java/sonia/scm/it/utils/TestData.java +++ b/scm-it/src/test/java/sonia/scm/it/utils/TestData.java @@ -123,23 +123,6 @@ public class TestData { ; } - public static void createUserPermission(String username, String roleName, String repositoryType) { - String defaultPermissionUrl = TestData.getDefaultPermissionUrl(USER_SCM_ADMIN, USER_SCM_ADMIN, repositoryType); - LOG.info("create permission with name {} and role {} using the endpoint: {}", username, roleName, defaultPermissionUrl); - given(VndMediaType.REPOSITORY_PERMISSION) - .when() - .content("{\n" + - "\t\"role\": " + roleName + ",\n" + - "\t\"name\": \"" + username + "\",\n" + - "\t\"groupPermission\": false\n" + - "\t\n" + - "}") - .post(defaultPermissionUrl) - .then() - .statusCode(HttpStatus.SC_CREATED) - ; - } - public static List getUserPermissions(String username, String password, String repositoryType) { return callUserPermissions(username, password, repositoryType, HttpStatus.SC_OK) .extract() From 63c874917f5fc355572f813d07ca787bf85650fd Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 15:02:13 +0200 Subject: [PATCH 12/12] added singleton annotation to AuthorizationChangedEventProducer to be sure that it is not destroyed by the gc --- .../sonia/scm/security/AuthorizationChangedEventProducer.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java b/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java index 66dbb51073..3f81377992 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java +++ b/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java @@ -48,6 +48,8 @@ import sonia.scm.user.User; import sonia.scm.user.UserEvent; import sonia.scm.user.UserModificationEvent; +import javax.inject.Singleton; + /** * Receives all kinds of events, which affects authorization relevant data and fires an * {@link AuthorizationChangedEvent} if authorization data has changed. @@ -55,6 +57,7 @@ import sonia.scm.user.UserModificationEvent; * @author Sebastian Sdorra * @since 1.52 */ +@Singleton @EagerSingleton public class AuthorizationChangedEventProducer {