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 <anch...@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