refactor GroupResolver + GroupCollector

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

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

scm-core/src/main/java/sonia/scm/security/GroupCollector.java

@@ -1,5 +0,0 @@
-package sonia.scm.security;
-
-public interface GroupCollector {
-  Iterable<String> collect(String principal);
-}

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.
  * <strong>Warning</strong>: 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 */
+  @VisibleForTesting
-  public static final String CACHE_NAME = "sonia.cache.externalGroups";
+  static final String CACHE_NAME = "sonia.cache.externalGroups";
-
-  /** Field description */
-  private final Cache<String, Set<String>> cache;
-  private Set<GroupResolver> groupResolvers;
 
   private final GroupDAO groupDAO;
+  private final Cache<String, Set<String>> cache;
+  private final Set<GroupResolver> groupResolvers;
 
-  DefaultGroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set<GroupResolver> groupResolvers) {
+  @Inject
+  public DefaultGroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set<GroupResolver> groupResolvers) {
     this.groupDAO = groupDAO;
-    this.cache  = cacheManager.getCache(CACHE_NAME);
+    this.cache = cacheManager.getCache(CACHE_NAME);
     this.groupResolvers = groupResolvers;
   }
 
   @Override
-  public Iterable<String> collect(String principal) {
+  public Set<String> collect(String principal) {
+    ImmutableSet.Builder<String> builder = ImmutableSet.builder();
 
+    builder.add(AUTHENTICATED);
+    builder.addAll(resolveExternalGroups(principal));
+    appendInternalGroups(principal, builder);
+
+    Set<String> groups = builder.build();
+    LOG.debug("collected following groups for principal {}: {}", principal, groups);
+    return groups;
+  }
+
+  private void appendInternalGroups(String principal, ImmutableSet.Builder<String> builder) {
+    for (Group group : groupDAO.getAll()) {
+      if (group.isMember(principal)) {
+        builder.add(group.getName());
+      }
+    }
+  }
+
+  private Set<String> resolveExternalGroups(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);
+        newExternalGroups.addAll(groupResolver.resolve(principal));
-       groups.forEach(newExternalGroups::add);
       }
-
+      externalGroups = newExternalGroups.build();
-      cache.put(principal, newExternalGroups.build());
+      cache.put(principal, externalGroups);
     }
-
+    return externalGroups;
-    ImmutableSet.Builder<String> 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;
   }
 }

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<GroupResolver> groupResolvers;
+
+  private DefaultGroupCollector collector;
+
+  @BeforeEach
+  void initCollector() {
+    groupResolvers = new HashSet<>();
+    mapCacheManager = new MapCacheManager();
+    collector = new DefaultGroupCollector(groupDAO, mapCacheManager, groupResolvers);
+  }
+
+  @Test
+  void shouldAlwaysReturnAuthenticatedGroup() {
+    Iterable<String> groupNames = collector.collect("trillian");
+    assertThat(groupNames).containsOnly("_authenticated");
+  }
+
+  @Test
+  void shouldReturnGroupsFromCache() {
+    MapCache<String, Set<String>> cache = mapCacheManager.getCache(DefaultGroupCollector.CACHE_NAME);
+    cache.put("trillian", ImmutableSet.of("awesome", "incredible"));
+
+    Set<String> groups = collector.collect("trillian");
+    assertThat(groups).containsOnly("_authenticated", "awesome", "incredible");
+  }
+
+  @Test
+  void shouldNotCallResolverIfExternalGroupsAreCached() {
+    groupResolvers.add(groupResolver);
+
+    MapCache<String, Set<String>> cache = mapCacheManager.getCache(DefaultGroupCollector.CACHE_NAME);
+    cache.put("trillian", ImmutableSet.of("awesome", "incredible"));
+
+    Set<String> groups = collector.collect("trillian");
+    assertThat(groups).containsOnly("_authenticated", "awesome", "incredible");
+
+    verify(groupResolver, never()).resolve("trillian");
+  }
+
+  @Nested
+  class WithGroupsFromDao {
+
+    @BeforeEach
+    void setUpGroupsDao() {
+      List<Group> 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<String> 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<String> groupNames = collector.collect("trillian");
+      assertThat(groupNames).containsOnly("_authenticated", "heartOfGold", "fjordsOfAfrican", "awesome", "incredible");
+    }
+  }
+}

scm-webapp/src/test/java/sonia/scm/group/GroupCollectorTest.java

@@ -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<String> groupNames = collector.collect("trillian");
-    assertThat(groupNames).containsOnly("_authenticated");
-  }
-
-  @Nested
-  class WithGroupsFromDao {
-
-    @BeforeEach
-    void setUpGroupsDao() {
-      List<Group> 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<String> groupNames = collector.collect("trillian");
-      assertThat(groupNames).contains("_authenticated", "heartOfGold", "fjordsOfAfrican");
-    }
-
-    @Test
-    void shouldCombineGivenWithDao() {
-      Iterable<String> groupNames = collector.collect("trillian");
-      assertThat(groupNames).contains("_authenticated", "heartOfGold", "fjordsOfAfrican", "awesome", "incredible");
-    }
-
-  }
-
-}