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);