This is an automated email from the ASF dual-hosted git repository.
angela pushed a commit to branch OAK-10318
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/OAK-10318 by this push:
new 0ea2af524a OAK-10318 : Improve
AutoMembershipPrincipals#isInheritedMember (add trace to cycle-warning as
suggested by joergH, fix typo, change order of checks)
0ea2af524a is described below
commit 0ea2af524a168efd502a8ad220377ac6ba670bbe
Author: angela <[email protected]>
AuthorDate: Mon Jul 24 12:17:27 2023 +0200
OAK-10318 : Improve AutoMembershipPrincipals#isInheritedMember (add trace
to cycle-warning as suggested by joergH, fix typo, change order of checks)
---
.../impl/principal/AutoMembershipPrincipals.java | 25 ++++----
.../impl/principal/AutoMembershipCycleTest.java | 70 +++++++++++++++++++++-
2 files changed, 80 insertions(+), 15 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 f5ceb84d16..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
@@ -147,7 +147,7 @@ final class AutoMembershipPrincipals {
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)) {
+ } else if (hasInheritedMembership(gr.declaredMemberOf(), groupId,
automembershipId, processed, "> ")) {
return true;
}
}
@@ -155,28 +155,29 @@ final class AutoMembershipPrincipals {
}
private static boolean hasInheritedMembership(@NotNull Iterator<Group>
declared, @NotNull String groupId,
- @NotNull String memberId,
@NotNull Set<String> processed) throws RepositoryException {
+ @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 (memberId.equals(grId)) {
- log.error("Cyclic group membership detected for group id {}",
memberId);
- }
- if (!processed.add(grId)) {
- // group has already been processed before (shared membership
e.g. for the 'everyone' group)
- return false;
- }
if (groupId.equals(grId)) {
// the specified groupId defines inherited membership of a
configured auto-membership group
return true;
}
- // remember group for traversing up the membership hierarchy
- groups.add(gr);
+ 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)) {
+ if (hasInheritedMembership(group.declaredMemberOf(), groupId,
memberId, processed, String.format("%s %s > ", trace, group.getID()))) {
return true;
}
}
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 c8664a3add..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
@@ -40,7 +40,6 @@ import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -142,6 +141,48 @@ public class AutoMembershipCycleTest extends
AbstractAutoMembershipTest {
verifyNoMoreInteractions(gr, umMock);
}
+ @Test
+ 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);
+
+ // 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
@@ -152,7 +193,7 @@ public class AutoMembershipCycleTest extends
AbstractAutoMembershipTest {
assertFalse(amp.isInheritedMember(IDP_VALID_AM, gr, gr2));
- verify(gr, times(1)).getID();
+ verify(gr).getID();
verifyNoMoreInteractions(gr, gr2);
}
@@ -171,9 +212,32 @@ public class AutoMembershipCycleTest extends
AbstractAutoMembershipTest {
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();
- clearInvocations(gr, amGr1);
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
