From aec66c023adb1247e28f124503fff29370ed04a3 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 18 Feb 2019 18:01:11 +0100 Subject: [PATCH] define AuthorizationCollector as extension point with multiple implmentations --- .../src/main/java/sonia/scm/ScmState.java | 20 +-------- .../main/java/sonia/scm/ScmStateFactory.java | 22 +++------ .../scm/security/AuthorizationCollector.java | 6 ++- .../DefaultAuthorizationCollector.java | 6 ++- .../java/sonia/scm/security/DefaultRealm.java | 45 ++++++++++++++----- .../DefaultRepositoryManagerPerfTest.java | 2 +- .../sonia/scm/security/DefaultRealmTest.java | 42 ++++++++++++++++- 7 files changed, 91 insertions(+), 52 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/ScmState.java b/scm-core/src/main/java/sonia/scm/ScmState.java index 84fb07a1f7..09def95d54 100644 --- a/scm-core/src/main/java/sonia/scm/ScmState.java +++ b/scm-core/src/main/java/sonia/scm/ScmState.java @@ -77,15 +77,13 @@ public final class ScmState * @param repositoryTypes available repository types * @param defaultUserType default user type * @param clientConfig client configuration - * @param assignedPermission assigned permissions * @param availablePermissions list of available permissions * * @since 2.0.0 */ public ScmState(String version, User user, Collection groups, String token, Collection repositoryTypes, String defaultUserType, - ScmClientConfig clientConfig, List assignedPermission, - Collection availablePermissions) + ScmClientConfig clientConfig, Collection availablePermissions) { this.version = version; this.user = user; @@ -94,24 +92,11 @@ public final class ScmState this.repositoryTypes = repositoryTypes; this.clientConfig = clientConfig; this.defaultUserType = defaultUserType; - this.assignedPermissions = assignedPermission; this.availablePermissions = availablePermissions; } //~--- get methods ---------------------------------------------------------- - /** - * Return a list of assigned permissions. - * - * - * @return list of assigned permissions - * @since 1.31 - */ - public List getAssignedPermissions() - { - return assignedPermissions; - } - /** * Returns a list of available global permissions. * @@ -225,9 +210,6 @@ public final class ScmState /** authentication token */ private String token; - /** Field description */ - private List assignedPermissions; - /** * Avaliable global permission * @since 1.31 diff --git a/scm-core/src/main/java/sonia/scm/ScmStateFactory.java b/scm-core/src/main/java/sonia/scm/ScmStateFactory.java index e839a0ddcc..ed8bfba5dc 100644 --- a/scm-core/src/main/java/sonia/scm/ScmStateFactory.java +++ b/scm-core/src/main/java/sonia/scm/ScmStateFactory.java @@ -74,20 +74,17 @@ public final class ScmStateFactory * @param repositoryManger repository manager * @param userManager user manager * @param securitySystem security system - * @param authorizationCollector authorization collector */ @Inject public ScmStateFactory(SCMContextProvider contextProvider, ScmConfiguration configuration, RepositoryManager repositoryManger, - UserManager userManager, SecuritySystem securitySystem, - AuthorizationCollector authorizationCollector) + UserManager userManager, SecuritySystem securitySystem) { this.contextProvider = contextProvider; this.configuration = configuration; this.repositoryManger = repositoryManger; this.userManager = userManager; this.securitySystem = securitySystem; - this.authorizationCollector = authorizationCollector; } //~--- methods -------------------------------------------------------------- @@ -101,8 +98,7 @@ public final class ScmStateFactory @SuppressWarnings("unchecked") public ScmState createAnonymousState() { - return createState(SCMContext.ANONYMOUS, Collections.EMPTY_LIST, null, - Collections.EMPTY_LIST, Collections.EMPTY_LIST); + return createState(SCMContext.ANONYMOUS, Collections.EMPTY_LIST, null, Collections.EMPTY_LIST); } /** @@ -141,15 +137,11 @@ public final class ScmStateFactory ap = securitySystem.getAvailablePermissions(); } - List permissions = - ImmutableList.copyOf( - authorizationCollector.collect().getStringPermissions()); - - return createState(user, groups.getCollection(), token, permissions, ap); + return createState(user, groups.getCollection(), token, ap); } private ScmState createState(User user, Collection groups, - String token, List assignedPermissions, + String token, Collection availablePermissions) { User u = user.clone(); @@ -159,15 +151,11 @@ public final class ScmStateFactory return new ScmState(contextProvider.getVersion(), u, groups, token, repositoryManger.getConfiguredTypes(), userManager.getDefaultType(), - new ScmClientConfig(configuration), assignedPermissions, - availablePermissions); + new ScmClientConfig(configuration), availablePermissions); } //~--- fields --------------------------------------------------------------- - /** authorization collector */ - private final AuthorizationCollector authorizationCollector; - /** configuration */ private final ScmConfiguration configuration; diff --git a/scm-core/src/main/java/sonia/scm/security/AuthorizationCollector.java b/scm-core/src/main/java/sonia/scm/security/AuthorizationCollector.java index d1151c3b35..b12d8b6978 100644 --- a/scm-core/src/main/java/sonia/scm/security/AuthorizationCollector.java +++ b/scm-core/src/main/java/sonia/scm/security/AuthorizationCollector.java @@ -34,6 +34,7 @@ package sonia.scm.security; //~--- non-JDK imports -------------------------------------------------------- import org.apache.shiro.authz.AuthorizationInfo; +import org.apache.shiro.subject.PrincipalCollection; import sonia.scm.plugin.ExtensionPoint; /** @@ -42,15 +43,16 @@ import sonia.scm.plugin.ExtensionPoint; * @author Sebastian Sdorra * @since 2.0.0 */ -@ExtensionPoint(multi = false) +@ExtensionPoint public interface AuthorizationCollector { /** * Returns {@link AuthorizationInfo} for the authenticated user. * + * @param principalCollection collected principals * * @return {@link AuthorizationInfo} for authenticated user */ - public AuthorizationInfo collect(); + AuthorizationInfo collect(PrincipalCollection principalCollection); } 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 3fdbcdf351..9dca7a774e 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java @@ -36,6 +36,7 @@ package sonia.scm.security; //~--- non-JDK imports -------------------------------------------------------- import com.github.legman.Subscribe; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; @@ -118,8 +119,8 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector * * @return */ - @Override - public AuthorizationInfo collect() + @VisibleForTesting + AuthorizationInfo collect() { AuthorizationInfo authorizationInfo; Subject subject = SecurityUtils.getSubject(); @@ -143,6 +144,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector * * @return */ + @Override public AuthorizationInfo collect(PrincipalCollection principals) { Preconditions.checkNotNull(principals, "principals parameter is required"); 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 0cc5db846c..bacbd9b314 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/security/DefaultRealm.java @@ -42,9 +42,11 @@ import org.apache.shiro.authc.UsernamePasswordToken; import org.apache.shiro.authc.credential.PasswordMatcher; import org.apache.shiro.authc.credential.PasswordService; 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 sonia.scm.plugin.Extension; @@ -56,6 +58,8 @@ import javax.inject.Singleton; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.Set; + /** * Default authorizing realm. * @@ -85,14 +89,13 @@ public class DefaultRealm extends AuthorizingRealm * * * @param service - * @param collector + * @param authorizationCollectors * @param helperFactory */ @Inject - public DefaultRealm(PasswordService service, - DefaultAuthorizationCollector collector, DAORealmHelperFactory helperFactory) + public DefaultRealm(PasswordService service, Set authorizationCollectors, DAORealmHelperFactory helperFactory) { - this.collector = collector; + this.authorizationCollectors = authorizationCollectors; this.helper = helperFactory.create(REALM); PasswordMatcher matcher = new PasswordMatcher(); @@ -133,8 +136,7 @@ public class DefaultRealm extends AuthorizingRealm @Override protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principals) { - AuthorizationInfo info = collector.collect(principals); - + AuthorizationInfo info = collectors(principals); Scope scope = principals.oneByType(Scope.class); if (scope != null && ! scope.isEmpty()) { LOG.trace("filter permissions by scope {}", scope); @@ -144,13 +146,36 @@ public class DefaultRealm extends AuthorizingRealm } return filtered; } else if (LOG.isTraceEnabled()) { - LOG.trace("principal does not contain scope informations, returning all permissions"); + LOG.trace("principal does not contain scope information, returning all permissions"); log(principals, info, null); } return info; } - + + private AuthorizationInfo collectors(PrincipalCollection principals) { + SimpleAuthorizationInfo merged = new SimpleAuthorizationInfo(); + for (AuthorizationCollector collector : authorizationCollectors) { + AuthorizationInfo authorizationInfo = collector.collect(principals); + merge(merged, authorizationInfo); + } + return merged; + } + + private void merge(SimpleAuthorizationInfo merged, AuthorizationInfo authorizationInfo) { + if (authorizationInfo != null) { + if (authorizationInfo.getRoles() != null) { + merged.addRoles(authorizationInfo.getRoles()); + } + if (authorizationInfo.getObjectPermissions() != null) { + merged.addObjectPermissions(authorizationInfo.getObjectPermissions()); + } + if (authorizationInfo.getStringPermissions() != null) { + merged.addStringPermissions(authorizationInfo.getStringPermissions()); + } + } + } + private void log( PrincipalCollection collection, AuthorizationInfo original, AuthorizationInfo filtered ) { StringBuilder buffer = new StringBuilder("authorization summary: "); @@ -190,8 +215,8 @@ public class DefaultRealm extends AuthorizingRealm //~--- fields --------------------------------------------------------------- - /** default authorization collector */ - private final DefaultAuthorizationCollector collector; + /** set of authorization collector */ + private final Set authorizationCollectors; /** realm helper */ private final DAORealmHelper helper; diff --git a/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerPerfTest.java b/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerPerfTest.java index 3d00fcaa7e..f3ff9fd0f9 100644 --- a/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerPerfTest.java +++ b/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerPerfTest.java @@ -205,7 +205,7 @@ private long calculateAverage(List times) { @Override protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principals) { - return authzCollector.collect(); + return authzCollector.collect(principals); } } 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 02b1a6ed1b..b6fea9e897 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/DefaultRealmTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/DefaultRealmTest.java @@ -71,7 +71,11 @@ 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; @@ -132,6 +136,36 @@ public class DefaultRealmTest assertThat(realmsAutz.getStringPermissions(), Matchers.contains("repository:*")); } + @Test + public void testGetAuthorizationInfoWithMultipleAuthorizationCollectors(){ + SimplePrincipalCollection col = new SimplePrincipalCollection(); + col.add(Scope.empty(), DefaultRealm.REALM); + + SimpleAuthorizationInfo collectedFromDefault = new SimpleAuthorizationInfo(); + collectedFromDefault.addStringPermission("repository:*"); + when(collector.collect(col)).thenReturn(collectedFromDefault); + + SimpleAuthorizationInfo collectedFromSecond = new SimpleAuthorizationInfo(); + collectedFromSecond.addStringPermission("user:*"); + collectedFromSecond.addRole("awesome"); + + AuthorizationCollector secondCollector = principalCollection -> collectedFromSecond; + authorizationCollectors.add(secondCollector); + + SimpleAuthorizationInfo collectedFromThird = new SimpleAuthorizationInfo(); + Permission permission = p -> false; + collectedFromThird.addObjectPermission(permission); + collectedFromThird.addRole("awesome"); + + AuthorizationCollector thirdCollector = principalCollection -> collectedFromThird; + authorizationCollectors.add(thirdCollector); + + AuthorizationInfo realmsAuthz = realm.doGetAuthorizationInfo(col); + assertThat(realmsAuthz.getObjectPermissions(), contains(permission)); + assertThat(realmsAuthz.getStringPermissions(), containsInAnyOrder("repository:*", "user:*")); + assertThat(realmsAuthz.getRoles(), Matchers.contains("awesome")); + } + /** * Tests {@link DefaultRealm#doGetAuthorizationInfo(PrincipalCollection)} with empty scope. */ @@ -284,7 +318,11 @@ public class DefaultRealmTest // use a small number of iterations for faster test execution hashService.setHashIterations(512); service.setHashService(hashService); - realm = new DefaultRealm(service, collector, helperFactory); + + authorizationCollectors = new HashSet<>(); + authorizationCollectors.add(collector); + + realm = new DefaultRealm(service, authorizationCollectors, helperFactory); // set permission resolver realm.setPermissionResolver(new WildcardPermissionResolver()); @@ -358,6 +396,8 @@ public class DefaultRealmTest @Mock private DefaultAuthorizationCollector collector; + private Set authorizationCollectors; + @Mock private LoginAttemptHandler loginAttemptHandler;