remove groups from BearerRealm / SyncRealmHelper / DAORealmHelper

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

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

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.
  * <strong>Warning</strong>: 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<String, Set> cache;
+  private Set<GroupResolver> groupResolvers;
+
   private final GroupDAO groupDAO;
 
-  GroupCollector(GroupDAO groupDAO) {
+  GroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set<GroupResolver> groupResolvers) {
     this.groupDAO = groupDAO;
+    this.cache  = cacheManager.getCache(CACHE_NAME);
+    this.groupResolvers = groupResolvers;
+  }
+
+  Iterable<String> collect(String principal) {
+
+    Set<String> externalGroups = cache.get(principal);
+
+    if (externalGroups == null) {
+      ImmutableSet.Builder<String> newExternalGroups = ImmutableSet.builder();
+
+      for (GroupResolver groupResolver : groupResolvers) {
+       Iterable<String> groups = groupResolver.resolveGroups(principal);
+       groups.forEach(newExternalGroups::add);
+      }
+
+      cache.put(principal, newExternalGroups.build());
     }
 
-  GroupNames collect(String principal, Iterable<String> groupNames) {
     ImmutableSet.Builder<String> builder = ImmutableSet.builder();
 
     builder.add(GroupNames.AUTHENTICATED);
 
-    for (String group : groupNames) {
+    for (String group : externalGroups) {
       builder.add(group);
     }
 

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<String> resolveGroups(String principal);
+}

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

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

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<Group> 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");
-  }
 }

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

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

@@ -90,12 +90,10 @@ class BearerRealmTest {
     Set<String> 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);