From 8550baaea902b041481e3af251c1e86cac5230cc Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Fri, 2 Aug 2019 08:17:17 +0200 Subject: [PATCH] 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"); - } - - } - -}