You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by an...@apache.org on 2023/01/24 15:22:04 UTC
[jackrabbit-oak] 01/01: OAK-10082 : Group.getMembers() needs to resolve inherited members of dynamic groups
This is an automated email from the ASF dual-hosted git repository.
angela pushed a commit to branch OAK-10082
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit b6f8307bc557eaa4c3062e3d22bf4221dd818f8c
Author: angela <an...@adobe.com>
AuthorDate: Tue Jan 24 16:21:43 2023 +0100
OAK-10082 : Group.getMembers() needs to resolve inherited members of dynamic groups
---
.../external/impl/DynamicSyncTest.java | 38 +++++-
.../jackrabbit/oak/security/user/GroupImpl.java | 6 +-
.../security/user/InheritedMembersIterator.java | 92 +++++++++++++
.../user/InheritedMembersIteratorTest.java | 142 +++++++++++++++++++++
4 files changed, 276 insertions(+), 2 deletions(-)
diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncTest.java
index 348c188ea5..1f83bfad5d 100644
--- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncTest.java
+++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncTest.java
@@ -56,6 +56,7 @@ public class DynamicSyncTest extends AbstractDynamicTest {
private Group autoForGroups;
private Group autoForUsers;
private Group base;
+ private Group base2;
@Override
public void before() throws Exception {
@@ -72,7 +73,7 @@ public class DynamicSyncTest extends AbstractDynamicTest {
userManager.createGroup(EveryonePrincipal.getInstance());
- Group base2 = userManager.createGroup(BASE2_ID);
+ base2 = userManager.createGroup(BASE2_ID);
base2.addMember(autoForUsers);
r.commit();
@@ -177,6 +178,41 @@ public class DynamicSyncTest extends AbstractDynamicTest {
assertFalse(autoForUsers.isMember(base));
}
+ @Test
+ public void testInheritedBaseGroup() throws Exception {
+ ExternalUser externalUser = idp.getUser(USER_ID);
+ sync(externalUser, SyncResult.Status.ADD);
+
+ Authorizable user = userManager.getAuthorizable(USER_ID);
+
+ // verify group 'base'
+ Set<String> expDeclaredMemberIds = ImmutableSet.of(AUTO_GROUPS, AUTO_USERS, "a", "b");
+ assertExpectedIds(expDeclaredMemberIds, base.getDeclaredMembers());
+ assertFalse(base.isDeclaredMember(user));
+
+ Set<String> expMemberIds = ImmutableSet.of(USER_ID, AUTO_GROUPS, AUTO_USERS, "a", "b", "c", "aa", "aaa");
+ assertExpectedIds(expMemberIds, base.getMembers());
+ assertTrue(base.isMember(user));
+ }
+
+ @Test
+ public void testInheritedBase2Group() throws Exception {
+ ExternalUser externalUser = idp.getUser(USER_ID);
+ sync(externalUser, SyncResult.Status.ADD);
+
+ Authorizable user = userManager.getAuthorizable(USER_ID);
+
+ // verify group 'base2'
+ Set<String> expDeclaredMemberIds = ImmutableSet.of(AUTO_USERS);
+ assertExpectedIds(expDeclaredMemberIds, base2.getDeclaredMembers());
+
+ assertFalse(base2.isDeclaredMember(user));
+
+ Set<String> expMemberIds = ImmutableSet.of(USER_ID, AUTO_USERS);
+ assertExpectedIds(expMemberIds, base2.getMembers());
+ assertTrue(base2.isMember(user));
+ }
+
private static void assertIsMember(@NotNull Group group, boolean declared, @NotNull Authorizable... members) {
try {
for (Authorizable member : members) {
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java
index 59d0fa786f..48076dcdb8 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java
@@ -222,7 +222,11 @@ class GroupImpl extends AuthorizableImpl implements Group {
return AuthorizableIterator.create(true, dynamicMembers, AuthorizableIterator.empty());
}
- AuthorizableIterator members = AuthorizableIterator.create(trees, userMgr, AuthorizableType.AUTHORIZABLE);
+ Iterator<Authorizable> members = AuthorizableIterator.create(trees, userMgr, AuthorizableType.AUTHORIZABLE);
+ if (includeInherited) {
+ // need to resolve dynamic members of declared and inherited group-members
+ members = new InheritedMembersIterator(members, dmp);
+ }
AuthorizableIterator allMembers = AuthorizableIterator.create(true, dynamicMembers, members);
return new RangeIteratorAdapter(allMembers, allMembers.getSize());
}
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/InheritedMembersIterator.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/InheritedMembersIterator.java
new file mode 100644
index 0000000000..b84c99e57d
--- /dev/null
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/InheritedMembersIterator.java
@@ -0,0 +1,92 @@
+/*
+ * 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.security.user;
+
+import com.google.common.collect.Iterators;
+import org.apache.jackrabbit.api.security.user.Authorizable;
+import org.apache.jackrabbit.api.security.user.Group;
+import org.apache.jackrabbit.commons.iterator.AbstractLazyIterator;
+import org.apache.jackrabbit.oak.spi.security.user.DynamicMembershipProvider;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.jcr.RepositoryException;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * Wrapper around a members iterator that will resolve dynamic members of the contained groups.
+ */
+class InheritedMembersIterator extends AbstractLazyIterator<Authorizable> {
+
+ private static final Logger log = LoggerFactory.getLogger(InheritedMembersIterator.class);
+
+ private final Iterator<Authorizable> members;
+ private final DynamicMembershipProvider dynamicMembershipProvider;
+ private final List<Group> groups = new ArrayList<>();
+
+ private Iterator<Authorizable> dynamicMembers;
+
+ InheritedMembersIterator(@NotNull Iterator<Authorizable> members, @NotNull DynamicMembershipProvider dynamicMembershipProvider) {
+ this.members = members;
+ this.dynamicMembershipProvider = dynamicMembershipProvider;
+ }
+
+ @Override
+ protected Authorizable getNext() {
+ if (members.hasNext()) {
+ Authorizable member = members.next();
+ if (member.isGroup()) {
+ rememberGroup((Group) member);
+ }
+ return member;
+ }
+
+ if (dynamicMembers == null || !dynamicMembers.hasNext()) {
+ dynamicMembers = getNextDynamicMembersIterator();
+ }
+
+ if (dynamicMembers.hasNext()) {
+ return dynamicMembers.next();
+ }
+
+ // all dynamic members have been processed
+ return null;
+ }
+
+ private void rememberGroup(@NotNull Group group) {
+ groups.add(group);
+ }
+
+ @NotNull
+ private Iterator<Authorizable> getNextDynamicMembersIterator() {
+ while (!groups.isEmpty()) {
+ Group group = groups.remove(0);
+ try {
+ Iterator<Authorizable> it = dynamicMembershipProvider.getMembers(group, false);
+ if (it.hasNext()) {
+ return it;
+ }
+ } catch (RepositoryException e) {
+ log.error("Failed to retrieve dynamic members of group '{}'", Utils.getIdOrNull(group), e);
+ }
+ }
+ return Iterators.emptyIterator();
+ }
+}
\ No newline at end of file
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/InheritedMembersIteratorTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/InheritedMembersIteratorTest.java
new file mode 100644
index 0000000000..6e4cd25280
--- /dev/null
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/InheritedMembersIteratorTest.java
@@ -0,0 +1,142 @@
+/*
+ * 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.security.user;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterators;
+import org.apache.jackrabbit.api.security.user.Authorizable;
+import org.apache.jackrabbit.api.security.user.Group;
+import org.apache.jackrabbit.api.security.user.User;
+import org.apache.jackrabbit.api.security.user.UserManager;
+import org.apache.jackrabbit.oak.AbstractSecurityTest;
+import org.apache.jackrabbit.oak.spi.security.user.DynamicMembershipProvider;
+import org.jetbrains.annotations.NotNull;
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.jcr.RepositoryException;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+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 InheritedMembersIteratorTest extends AbstractSecurityTest {
+
+ private Group base;
+ private Group nonDynamicGroup;
+ private Group dynamicGroup;
+ private User dynamicUser;
+
+ private final Set<String> ids = new HashSet<>();
+
+ @Before
+ public void before() throws Exception {
+ super.before();
+
+ UserManager um = getUserManager(root);
+ base = um.createGroup("base");
+ nonDynamicGroup = um.createGroup("testGroup");
+
+ // add 2 members to the base group
+ base.addMember(nonDynamicGroup);
+ base.addMember(getTestUser());
+
+ // add 1 nested group member to 'nonDynamicGroup'
+ dynamicGroup = um.createGroup("dynamicTestGroup");
+ nonDynamicGroup.addMember(dynamicGroup);
+
+ // create another user
+ dynamicUser = um.createUser("dynamicTestUser", null);
+ root.commit();
+
+ // remember all IDs for cleanup
+ ids.addAll(ImmutableSet.of("base", "testGroup", "dynamicTestGroup", "dynamicTestUser"));
+ }
+
+ @Override
+ public void after() throws Exception {
+ try {
+ UserManager um = getUserManager(root);
+ for (String id : ids) {
+ Authorizable a = um.getAuthorizable(id);
+ if (a != null) {
+ a.remove();
+ }
+ }
+ root.commit();
+ } finally {
+ super.after();
+ }
+ }
+
+ @Test
+ public void testNoDynamicMembers() throws Exception {
+ DynamicMembershipProvider dmp = mock(DynamicMembershipProvider.class);
+ when(dmp.getMembers(any(Group.class), anyBoolean())).thenReturn(Iterators.emptyIterator());
+
+ // no dynamic members in result
+ Set<String> expectedMemberIds = ImmutableSet.of(getTestUser().getID(), "testGroup", "dynamicTestGroup");
+ assertEquals(expectedMemberIds, getMembersIds(new InheritedMembersIterator(base.getMembers(), dmp)));
+
+ verify(dmp, times(2)).getMembers(any(Group.class), eq(false));
+ verifyNoMoreInteractions(dmp);
+ }
+
+ @Test
+ public void testDynamicMembers() throws Exception {
+ DynamicMembershipProvider dmp = mock(DynamicMembershipProvider.class);
+ when(dmp.getMembers(dynamicGroup, false)).thenReturn(Iterators.forArray(dynamicUser, getTestUser()));
+ when(dmp.getMembers(nonDynamicGroup, false)).thenReturn(Iterators.emptyIterator());
+
+ // dynamic members get resolved this time
+ Set<String> expectedMemberIds = ImmutableSet.of(getTestUser().getID(), "testGroup", "dynamicTestGroup", "dynamicTestUser");
+ assertEquals(expectedMemberIds, getMembersIds(new InheritedMembersIterator(base.getMembers(), dmp)));
+
+ verify(dmp, times(2)).getMembers(any(Group.class), eq(false));
+ verifyNoMoreInteractions(dmp);
+ }
+
+ @Test
+ public void testDynamicMembersFails() throws Exception {
+ DynamicMembershipProvider dmp = mock(DynamicMembershipProvider.class);
+ when(dmp.getMembers(any(Group.class), anyBoolean())).thenThrow(new RepositoryException());
+
+ Set<String> expectedMemberIds = ImmutableSet.of(getTestUser().getID(), "testGroup", "dynamicTestGroup");
+ assertEquals(expectedMemberIds, getMembersIds(new InheritedMembersIterator(base.getMembers(), dmp)));
+
+ verify(dmp, times(2)).getMembers(any(Group.class), eq(false));
+ verifyNoMoreInteractions(dmp);
+ }
+
+ private static @NotNull Set<String> getMembersIds(@NotNull InheritedMembersIterator it) {
+ return ImmutableSet.copyOf(Iterators.transform(it, authorizable -> {
+ try {
+ return authorizable.getID();
+ } catch (RepositoryException repositoryException) {
+ return null;
+ }
+ }));
+ }
+}
\ No newline at end of file