This is an automated email from the ASF dual-hosted git repository.
angela pushed a commit to branch OAK-10286_OAK-10285
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 1ac567e59dadad7fe0a6eeacb24b9d26e75b8f13
Author: angela <anch...@adobe.com>
AuthorDate: Thu Jun 8 16:34:13 2023 +0200
OAK-10286 : AutoMembershipPrincipals.isInheritedMember add check for cyclic
membership, OAK-10285 : MembershipProvider change log level to ERROR for cyclic
membership
---
.../impl/principal/AutoMembershipPrincipals.java | 12 ++-
.../impl/principal/AutoMembershipCycleTest.java | 109 +++++++++++++++++++++
.../oak/security/user/MembershipProvider.java | 2 +-
3 files changed, 120 insertions(+), 3 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 f7f29c823d..fc9c664940 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
@@ -127,15 +127,23 @@ 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)) {
return true;
}
-
+
Iterator<Authorizable> declaredGroupMembers =
Iterators.filter(group.getDeclaredMembers(), Authorizable::isGroup);
while (declaredGroupMembers.hasNext()) {
Group grMember = (Group) declaredGroupMembers.next();
- if (isInheritedMember(idpName, grMember, authorizable)) {
+ if (isInheritedMember(idpName, grMember, authorizable,
processedIds)) {
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
new file mode 100644
index 0000000000..60a088f821
--- /dev/null
+++
b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipCycleTest.java
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package
org.apache.jackrabbit.oak.spi.security.authentication.external.impl.principal;
+
+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.jetbrains.annotations.NotNull;
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.jcr.RepositoryException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+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.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+
+public class AutoMembershipCycleTest extends AbstractAutoMembershipTest {
+
+ private AutoMembershipPrincipals amp;
+ 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());
+ }
+
+ @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"));
+
+ // declared automembership
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, amGr1, authorizable));
+ verify(amGr1, never()).getDeclaredMembers();
+
+ // declared automembership in amGr1
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, authorizable));
+ verify(gr).getDeclaredMembers();
+ verify(amGr1, never()).getDeclaredMembers();
+ clearInvocations(amGr1, gr);
+
+ // declared automembership
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, amGr1, gr));
+ verify(amGr1, never()).getDeclaredMembers();
+ verify(gr, never()).getDeclaredMembers();
+
+ assertTrue(amp.isInheritedMember(IDP_VALID_AM, gr, amGr1));
+ verify(gr).getDeclaredMembers();
+ verify(amGr1, never()).getDeclaredMembers();
+ }
+
+ @Test
+ public void testIsInheritedMemberCircularWithoutAutoMembership() throws
Exception {
+ // mock a membership cycle
+ Group gr = mockGroup("testGroup");
+ Group gr2 = mockGroup("testGroup2");
+
+
when(gr.getDeclaredMembers()).thenReturn(Iterators.singletonIterator(gr2)).thenThrow(new
RuntimeException("circle"));
+
when(gr2.getDeclaredMembers()).thenReturn(Iterators.singletonIterator(gr)).thenThrow(new
RuntimeException("circle"));
+
+ 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();
+ 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;
+ }
+}
\ No newline at end of file
diff --git
a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java
b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java
index af0615ac22..4457049589 100644
---
a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java
+++
b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java
@@ -202,7 +202,7 @@ class MembershipProvider extends AuthorizableBaseProvider {
@Override
protected boolean hasProcessedReference(@NotNull String value) {
if (groupContentId.equals(value)) {
- log.warn("Cyclic group membership detected for contentId
{}", groupContentId);
+ log.error("Cyclic group membership detected for contentId
{}", groupContentId);
return false;
}
return processedRefs.add(value);
