This is an automated email from the ASF dual-hosted git repository.
angela pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push:
new 615b2e9002 OAK-10318 : Improve
AutoMembershipPrincipals#isInheritedMember
615b2e9002 is described below
commit 615b2e90021b2136549ca33a28e4a453c8d2132f
Author: anchela <ang...@apache.org>
AuthorDate: Tue Jul 25 14:28:09 2023 +0200
OAK-10318 : Improve AutoMembershipPrincipals#isInheritedMember
* OAK-10318 : Improve AutoMembershipPrincipals#isInheritedMember
* OAK-10318 : Improve AutoMembershipPrincipals#isInheritedMember (add trace
to cycle-warning as suggested by joergH, fix typo, change order of checks)
---
.../impl/principal/AutoMembershipPrincipals.java | 68 +++++--
.../impl/principal/AutoMembershipCycleTest.java | 218 +++++++++++++++++----
2 files changed, 227 insertions(+), 59 deletions(-)
diff --git
a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipPrincipals.java
b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipPrincipals.java
index fc9c664940..eb26a19b1e 100644
---
a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipPrincipals.java
+++
b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipPrincipals.java
@@ -16,12 +16,12 @@
*/
package
org.apache.jackrabbit.oak.spi.security.authentication.external.impl.principal;
-import org.apache.jackrabbit.guava.common.collect.ImmutableSet;
-import org.apache.jackrabbit.guava.common.collect.Iterators;
-import org.apache.jackrabbit.guava.common.collect.Maps;
import org.apache.jackrabbit.api.security.user.Authorizable;
import org.apache.jackrabbit.api.security.user.Group;
import org.apache.jackrabbit.api.security.user.UserManager;
+import org.apache.jackrabbit.guava.common.collect.ImmutableSet;
+import org.apache.jackrabbit.guava.common.collect.Iterators;
+import org.apache.jackrabbit.guava.common.collect.Maps;
import
org.apache.jackrabbit.oak.spi.security.authentication.external.basic.AutoMembershipConfig;
import org.apache.jackrabbit.oak.spi.security.principal.GroupPrincipals;
import org.jetbrains.annotations.NotNull;
@@ -32,6 +32,7 @@ import org.slf4j.LoggerFactory;
import javax.jcr.RepositoryException;
import java.security.Principal;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
@@ -103,7 +104,7 @@ final class AutoMembershipPrincipals {
*
* @param idpName The name of an IDP
* @param groupId The target group id
- * @param authorizable The authorizable for which to evaluation if it is a
automatic member of the group identified by {@code groupId}.
+ * @param authorizable The authorizable for which to evaluation if it is
an automatic member of the group identified by {@code groupId}.
* @return {@code true} if the given authorizable is an automatic member
of the group identified by {@code groupId}; {@code false} otherwise.
* @see AutoMembershipProvider#isMember(Group, Authorizable, boolean)
*/
@@ -127,23 +128,56 @@ final class AutoMembershipPrincipals {
}
boolean isInheritedMember(@NotNull String idpName, @NotNull Group group,
@NotNull Authorizable authorizable) throws RepositoryException {
- return isInheritedMember(idpName, group, authorizable, new
HashSet<>());
- }
-
- boolean isInheritedMember(@NotNull String idpName, @NotNull Group group,
@NotNull Authorizable authorizable, @NotNull Set<String> processedIds) throws
RepositoryException {
String groupId = group.getID();
- if (!processedIds.add(groupId)) {
- log.error("Cyclic group membership detected for group id {}",
groupId);
- return false;
- }
if (isMember(idpName, groupId, authorizable)) {
+ // groupId is listed in auto-membership configuration
return true;
}
- Iterator<Authorizable> declaredGroupMembers =
Iterators.filter(group.getDeclaredMembers(), Authorizable::isGroup);
- while (declaredGroupMembers.hasNext()) {
- Group grMember = (Group) declaredGroupMembers.next();
- if (isInheritedMember(idpName, grMember, authorizable,
processedIds)) {
+ // to test for inherited membership collect automembership-ids and
loop auto-membership groups
+ Set<String> automembershipIds = new
HashSet<>(Arrays.asList(autoMembershipMapping.get(idpName)));
+ AutoMembershipConfig config = autoMembershipConfigMap.get(idpName);
+ if (config != null) {
+ automembershipIds.addAll(config.getAutoMembership(authorizable));
+ }
+
+ // keep track of processed ids over all 'automembership' ids to avoid
repeated evaluation
+ Set<String> processed = new HashSet<>();
+ for (String automembershipId : automembershipIds) {
+ Authorizable gr = userManager.getAuthorizable(automembershipId);
+ if (gr == null || !gr.isGroup()) {
+ log.warn("Configured automembership id '{}' is not a valid
group", automembershipId);
+ } else if (hasInheritedMembership(gr.declaredMemberOf(), groupId,
automembershipId, processed, "> ")) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private static boolean hasInheritedMembership(@NotNull Iterator<Group>
declared, @NotNull String groupId,
+ @NotNull String memberId,
@NotNull Set<String> processed,
+ @NotNull String trace)
throws RepositoryException {
+ List<Group> groups = new ArrayList<>();
+ while (declared.hasNext()) {
+ Group gr = declared.next();
+ String grId = gr.getID();
+ if (groupId.equals(grId)) {
+ // the specified groupId defines inherited membership of a
configured auto-membership group
+ return true;
+ }
+ if (memberId.equals(grId)) {
+ log.error("Cyclic group membership detected for group id {}
via {}{}", memberId, trace, grId);
+ continue;
+ }
+ if (processed.add(grId)) {
+ // remember group for traversing up the membership hierarchy
if it has not already been
+ // processed before (shared membership e.g. for the 'everyone'
group)
+ groups.add(gr);
+ }
+ }
+ // recursively call to search for inherited membership
+ for (Group group : groups) {
+ if (hasInheritedMembership(group.declaredMemberOf(), groupId,
memberId, processed, String.format("%s %s > ", trace, group.getID()))) {
return true;
}
}
@@ -152,7 +186,7 @@ final class AutoMembershipPrincipals {
/**
* Returns the group principal that given authorizable is an automatic
member of. This method evaluates both the
- * global auto-membership settings as well as {@link AutoMembershipConfig}
if they exist for the given IDP name.
+ * global auto-membership settings and {@link AutoMembershipConfig} if
they exist for the given IDP name.
*
* @param idpName The name of an IDP
* @param authorizable The target user/group
diff --git
a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipCycleTest.java
b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipCycleTest.java
index 60a088f821..049ef74970 100644
---
a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipCycleTest.java
+++
b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipCycleTest.java
@@ -18,92 +18,226 @@ package
org.apache.jackrabbit.oak.spi.security.authentication.external.impl.prin
import org.apache.jackrabbit.api.security.user.Authorizable;
import org.apache.jackrabbit.api.security.user.Group;
-import org.apache.jackrabbit.guava.common.collect.Iterators;
+import org.apache.jackrabbit.api.security.user.UserManager;
+import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters;
+import
org.apache.jackrabbit.oak.spi.security.authentication.external.basic.DefaultSyncConfig;
+import
org.apache.jackrabbit.oak.spi.security.authentication.external.basic.DefaultSyncContext;
+import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.xml.ImportBehavior;
+import org.apache.jackrabbit.oak.spi.xml.ProtectedItemImporter;
import org.jetbrains.annotations.NotNull;
import org.junit.Before;
import org.junit.Test;
-import javax.jcr.RepositoryException;
-import java.util.Arrays;
+import javax.jcr.ValueFactory;
import java.util.Collections;
-import java.util.List;
import java.util.Map;
+import static
org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.PARAM_PROTECT_EXTERNAL_IDS;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
public class AutoMembershipCycleTest extends AbstractAutoMembershipTest {
private AutoMembershipPrincipals amp;
+ private UserManager umMock;
+ private Group gr;
+ private Group gr2;
+ private Group amGr1;
private final Authorizable authorizable = mock(Authorizable.class);
@Before
public void before() throws Exception {
super.before();
Map<String, String[]> mapping = Collections.singletonMap(IDP_VALID_AM,
new String[] {AUTOMEMBERSHIP_GROUP_ID_1});
- amp = new AutoMembershipPrincipals(userManager, mapping,
getAutoMembershipConfigMapping());
+
+ umMock = spy(userManager);
+ amGr1 = spy(automembershipGroup1);
+ gr = spy(umMock.createGroup("testGroup"));
+ gr2 = spy(umMock.createGroup("testGroup2"));
+
when(umMock.getAuthorizable(AUTOMEMBERSHIP_GROUP_ID_1)).thenReturn(amGr1);
+
+ umMock.createGroup(EveryonePrincipal.getInstance());
+ root.commit();
+
+
+ amp = new AutoMembershipPrincipals(umMock, mapping,
getAutoMembershipConfigMapping());
+ clearInvocations(umMock, amGr1, gr, gr2);
}
-
+
+ @Override
+ public void after() throws Exception {
+ try {
+ amGr1.removeMembers("testGroup");
+ gr.remove();
+ gr2.remove();
+ root.commit();
+ } finally {
+ super.after();
+ }
+ }
+
+ @Override
+ protected ConfigurationParameters getSecurityConfigParameters() {
+ return ConfigurationParameters.of(UserConfiguration.NAME,
+ ConfigurationParameters.of(
+ ProtectedItemImporter.PARAM_IMPORT_BEHAVIOR,
ImportBehavior.NAME_BESTEFFORT)
+ );
+ }
+
+ @Override
+ protected @NotNull DefaultSyncConfig createSyncConfig() {
+ DefaultSyncConfig dsc = super.createSyncConfig();
+
dsc.user().setDynamicMembership(true).setAutoMembership(AUTOMEMBERSHIP_GROUP_ID_1);
+
dsc.group().setDynamicGroups(true).setAutoMembership(AUTOMEMBERSHIP_GROUP_ID_1);
+ return dsc;
+ }
+
@Test
public void testIsInheritedMemberCircularIncludingAutoMembership() throws
Exception {
- Group amGr1 = mockGroup(AUTOMEMBERSHIP_GROUP_ID_1);
- Group gr = mockGroup("testGroup");
-
- // mock a membership cycle
- List<Authorizable> members = Arrays.asList(authorizable, amGr1);
- when(gr.getDeclaredMembers()).thenReturn(members.iterator(),
members.iterator(), members.iterator()).thenThrow(new
RuntimeException("cicle"));
-
when(amGr1.getDeclaredMembers()).thenReturn(Iterators.singletonIterator(gr),
Iterators.singletonIterator(gr), Iterators.singletonIterator(gr)).thenThrow(new
RuntimeException("cicle"));
+ // create cycle
+ assertTrue(amGr1.addMembers(gr.getID()).isEmpty());
+ assertTrue(gr.addMembers(amGr1.getID()).isEmpty());
+ root.commit();
+ clearInvocations(gr, amGr1);
// declared automembership
assertTrue(amp.isInheritedMember(IDP_VALID_AM, amGr1, authorizable));
- verify(amGr1, never()).getDeclaredMembers();
+ verify(amGr1).getID();
+ verifyNoMoreInteractions(amGr1);
+ verifyNoInteractions(authorizable, gr);
+ clearInvocations(authorizable, gr, amGr1);
- // declared automembership in amGr1
+ // declared automembership in amGr1 (cycle)
assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, authorizable));
- verify(gr).getDeclaredMembers();
- verify(amGr1, never()).getDeclaredMembers();
- clearInvocations(amGr1, gr);
+ verifyNoInteractions(authorizable);
+ verify(gr).getID();
+ verify(amGr1).declaredMemberOf();
+ verify(umMock).getAuthorizable(AUTOMEMBERSHIP_GROUP_ID_1);
+ verifyNoMoreInteractions(gr);
+ clearInvocations(authorizable, amGr1, gr, umMock);
- // declared automembership
+ // declared automembership for group
assertTrue(amp.isInheritedMember(IDP_VALID_AM, amGr1, gr));
- verify(amGr1, never()).getDeclaredMembers();
- verify(gr, never()).getDeclaredMembers();
-
+ verify(amGr1).getID();
+ verifyNoMoreInteractions(amGr1);
+ verifyNoInteractions(gr, umMock);
+ clearInvocations(amGr1, gr, umMock);
+
+ // declared automembership in amGr1 (cycle)
assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, amGr1));
- verify(gr).getDeclaredMembers();
- verify(amGr1, never()).getDeclaredMembers();
+ verify(gr).getID();
+ verify(amGr1).declaredMemberOf();
+ verify(umMock).getAuthorizable(AUTOMEMBERSHIP_GROUP_ID_1);
+
+ verifyNoMoreInteractions(gr, umMock);
}
@Test
- public void testIsInheritedMemberCircularWithoutAutoMembership() throws
Exception {
- // mock a membership cycle
- Group gr = mockGroup("testGroup");
- Group gr2 = mockGroup("testGroup2");
+ public void testIsInheritedMemberNestedCircularIncludingAutoMembership()
throws Exception {
+ // create cycle
+ assertTrue(amGr1.addMembers(gr.getID()).isEmpty());
+ assertTrue(gr.addMembers(gr2.getID()).isEmpty());
+ assertTrue(gr2.addMembers(amGr1.getID()).isEmpty());
+ root.commit();
+ clearInvocations(gr, gr2, amGr1);
+
+ // declared automembership in amGr1 (cycle)
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, authorizable));
+ verifyNoInteractions(authorizable);
+ verify(gr).getID();
+ verify(amGr1).declaredMemberOf();
+ verify(umMock).getAuthorizable(AUTOMEMBERSHIP_GROUP_ID_1);
+ verifyNoMoreInteractions(gr);
+ clearInvocations(authorizable, amGr1, gr, gr2, umMock);
+
+ // declared automembership in amGr1 (cycle)
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, amGr1));
+ verify(gr).getID();
+ verify(amGr1).declaredMemberOf();
+ verify(umMock).getAuthorizable(AUTOMEMBERSHIP_GROUP_ID_1);
+ verifyNoMoreInteractions(gr);
+ clearInvocations(authorizable, amGr1, gr, gr2, umMock);
+
+ // declared automembership in amGr1 (cycle)
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr2, authorizable));
+ verify(gr2).getID();
+ verify(amGr1).declaredMemberOf();
+ verify(umMock).getAuthorizable(AUTOMEMBERSHIP_GROUP_ID_1);
+ verifyNoMoreInteractions(gr, gr2, umMock);
+ clearInvocations(authorizable, amGr1, gr, gr2, umMock);
-
when(gr.getDeclaredMembers()).thenReturn(Iterators.singletonIterator(gr2)).thenThrow(new
RuntimeException("circle"));
-
when(gr2.getDeclaredMembers()).thenReturn(Iterators.singletonIterator(gr)).thenThrow(new
RuntimeException("circle"));
+ // declared automembership in amGr1 (cycle)
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr2, amGr1));
+ verify(gr2).getID();
+ verify(amGr1).declaredMemberOf();
+ verify(umMock).getAuthorizable(AUTOMEMBERSHIP_GROUP_ID_1);
+ verifyNoMoreInteractions(gr, gr2, umMock);
+ }
+ @Test
+ public void testIsInheritedMemberCircularWithoutAutoMembership() throws
Exception {
+ // create cycle
+ assertTrue(gr.addMembers(gr2.getID()).isEmpty());
+ assertTrue(gr2.addMembers(gr.getID()).isEmpty());
+ root.commit();
+ clearInvocations(gr, gr2);
+
assertFalse(amp.isInheritedMember(IDP_VALID_AM, gr, gr2));
- verify(gr, times(1)).getDeclaredMembers();
- verify(gr, times(2)).getID();
- verify(gr, times(1)).isGroup();
- verify(gr2, times(1)).getDeclaredMembers();
- verify(gr2, times(1)).getID();
- verify(gr2, times(1)).isGroup();
+ verify(gr).getID();
verifyNoMoreInteractions(gr, gr2);
}
- private static @NotNull Group mockGroup(@NotNull String id) throws
RepositoryException {
- Group gr =
when(mock(Group.class).isGroup()).thenReturn(true).getMock();
- when(gr.getID()).thenReturn(id);
- return gr;
+ @Test
+ public void testIsInheritedMemberCircularIncludingExternalGroup() throws
Exception {
+
externalPrincipalConfiguration.setParameters(ConfigurationParameters.of(
+ PARAM_PROTECT_EXTERNAL_IDS, false));
+
+ // create cycle
+ assertTrue(amGr1.addMembers(gr.getID()).isEmpty());
+ assertTrue(gr.addMembers(amGr1.getID()).isEmpty());
+ root.commit();
+
+ // mark groups as external identities
+ ValueFactory vf = getValueFactory(root);
+ gr.setProperty(DefaultSyncContext.REP_EXTERNAL_ID,
vf.createValue(gr.getID() + ';' + idp.getName()));
+ amGr1.setProperty(DefaultSyncContext.REP_EXTERNAL_ID,
vf.createValue(amGr1.getID() + ';' + idp.getName()));
+ root.commit();
+
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, authorizable));
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, amGr1));
+ }
+
+ @Test
+ public void testIsInheritedMemberNestedCircularIncludingExternalGroup()
throws Exception {
+
externalPrincipalConfiguration.setParameters(ConfigurationParameters.of(
+ PARAM_PROTECT_EXTERNAL_IDS, false));
+
+ // create cycle
+ assertTrue(amGr1.addMembers(gr2.getID()).isEmpty());
+ assertTrue(gr2.addMembers(gr.getID()).isEmpty());
+ assertTrue(gr.addMembers(amGr1.getID()).isEmpty());
+ root.commit();
+
+ // mark groups as external identities
+ ValueFactory vf = getValueFactory(root);
+ gr.setProperty(DefaultSyncContext.REP_EXTERNAL_ID,
vf.createValue(gr.getID() + ';' + idp.getName()));
+ amGr1.setProperty(DefaultSyncContext.REP_EXTERNAL_ID,
vf.createValue(amGr1.getID() + ';' + idp.getName()));
+ root.commit();
+
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, authorizable));
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, amGr1));
+
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr2, authorizable));
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr2, amGr1));
}
}
\ No newline at end of file
