This is an automated email from the ASF dual-hosted git repository. angela pushed a commit to branch issue/OAK-10486 in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 7e3056f337490cb66cf7336136628bb8cbda0ec9 Author: angela <anch...@adobe.com> AuthorDate: Wed Oct 11 19:12:39 2023 +0200 OAK-10486 : Resolution of inherited groups may terminate pre-maturely for external users --- .../principal/InheritedMembershipIterator.java | 29 ++++++---- .../external/impl/DynamicSyncTest.java | 66 ++++++++++++++++++++++ 2 files changed, 85 insertions(+), 10 deletions(-) diff --git a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/InheritedMembershipIterator.java b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/InheritedMembershipIterator.java index 8b19f07532..0fe885276d 100644 --- a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/InheritedMembershipIterator.java +++ b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/InheritedMembershipIterator.java @@ -54,6 +54,7 @@ class InheritedMembershipIterator extends AbstractLazyIterator<Group> { try { // call 'memberof' to cover nested inheritance Iterator<Group> it = gr.memberOf(); + // verify that the group-iterator has any elements before remembering it for further processing if (it.hasNext()) { inherited.add(it); } @@ -63,20 +64,13 @@ class InheritedMembershipIterator extends AbstractLazyIterator<Group> { return gr; } - if (inheritedIterator == null) { - inheritedIterator = getNextInheritedIterator(); - } - - while (inheritedIterator.hasNext()) { + while (inheritedHasNext()) { Group gr = inheritedIterator.next(); if (notProcessedBefore(gr)) { return gr; } - if (!inheritedIterator.hasNext()) { - inheritedIterator = getNextInheritedIterator(); - } - } - + } + // all inherited groups have been processed return null; } @@ -89,6 +83,21 @@ class InheritedMembershipIterator extends AbstractLazyIterator<Group> { } } + private boolean inheritedHasNext() { + if (inheritedIterator == null) { + // initialize the inherited iterator (i.e. get the first one after having processed all dynamic groups) + inheritedIterator = getNextInheritedIterator(); + } + if (inheritedIterator.hasNext()) { + return true; + } else { + // no more elements in the current 'inheritedIterator'. move on to the next inherited iterator + // (or an empty one if there are no more iterators left to process) + inheritedIterator = getNextInheritedIterator(); + return inheritedIterator.hasNext(); + } + } + @NotNull private Iterator<Group> getNextInheritedIterator() { if (inherited.isEmpty()) { 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 c653741007..c9f81effce 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 @@ -50,6 +50,8 @@ public class DynamicSyncTest extends AbstractDynamicTest { private static final String BASE_ID = "base"; private static final String BASE2_ID = "base2"; + private static final String BASE3_ID = "base3"; + private static final String BASE4_ID = "base4"; private static final String AUTO_GROUPS = "autoForGroups"; private static final String AUTO_USERS = "autoForUsers"; @@ -57,6 +59,7 @@ public class DynamicSyncTest extends AbstractDynamicTest { private Group autoForUsers; private Group base; private Group base2; + private Group base3; @Override public void before() throws Exception { @@ -75,6 +78,10 @@ public class DynamicSyncTest extends AbstractDynamicTest { base2 = userManager.createGroup(BASE2_ID); base2.addMember(autoForUsers); + + base3 = userManager.createGroup(BASE3_ID); + Group base4 = userManager.createGroup(BASE4_ID); + base4.addMembers(BASE3_ID); r.commit(); } @@ -130,6 +137,65 @@ public class DynamicSyncTest extends AbstractDynamicTest { assertEquals(10, principalNames.size()); } + @Test + public void testSyncedUserDifferentBaseGroups() throws Exception { + // 'b' is member of local group 'base3' (not of 'base') + // 'a' is member of local group 'base' + assertTrue(base3.addMembers("b").isEmpty()); + assertTrue(base.removeMembers("b").isEmpty()); + root.commit(); + + ExternalUser externalUser = idp.getUser(USER_ID); + sync(externalUser, SyncResult.Status.ADD); + + Authorizable user = userManager.getAuthorizable(USER_ID); + assertNotNull(user); + + // assert membership + Set<String> expDeclaredGroupIds = ImmutableSet.of("a", "b", "c", "aa", "aaa", AUTO_GROUPS, AUTO_USERS, EveryonePrincipal.NAME); + assertExpectedIds(expDeclaredGroupIds, user.declaredMemberOf()); + + Set<String> expGroupIds = ImmutableSet.of(BASE_ID, BASE2_ID, BASE3_ID, BASE4_ID, "a", "b", "c", "aa", "aaa", AUTO_GROUPS, AUTO_USERS, EveryonePrincipal.NAME); + assertExpectedIds(expGroupIds, user.memberOf()); + + // assert groups + user.declaredMemberOf().forEachRemaining(group -> assertIsMember(group, true, user)); + user.memberOf().forEachRemaining(group -> assertIsMember(group, false, user)); + + // assert principals + List<String> principalNames = getPrincipalNames(getPrincipalManager(r).getGroupMembership(user.getPrincipal())); + assertEquals(12, principalNames.size()); + } + + @Test + public void testSyncedUserDifferentBaseGroupsWithDuplication() throws Exception { + // 'b' is member of local group 'base3' and 'base' + // 'a' is member of local group 'base' + assertTrue(base3.addMembers("b").isEmpty()); + root.commit(); + + ExternalUser externalUser = idp.getUser(USER_ID); + sync(externalUser, SyncResult.Status.ADD); + + Authorizable user = userManager.getAuthorizable(USER_ID); + assertNotNull(user); + + // assert membership + Set<String> expDeclaredGroupIds = ImmutableSet.of("a", "b", "c", "aa", "aaa", AUTO_GROUPS, AUTO_USERS, EveryonePrincipal.NAME); + assertExpectedIds(expDeclaredGroupIds, user.declaredMemberOf()); + + Set<String> expGroupIds = ImmutableSet.of(BASE_ID, BASE2_ID, BASE3_ID, BASE4_ID, "a", "b", "c", "aa", "aaa", AUTO_GROUPS, AUTO_USERS, EveryonePrincipal.NAME); + assertExpectedIds(expGroupIds, user.memberOf()); + + // assert groups + user.declaredMemberOf().forEachRemaining(group -> assertIsMember(group, true, user)); + user.memberOf().forEachRemaining(group -> assertIsMember(group, false, user)); + + // assert principals + List<String> principalNames = getPrincipalNames(getPrincipalManager(r).getGroupMembership(user.getPrincipal())); + assertEquals(12, principalNames.size()); + } + @Test public void testSyncedGroup() throws Exception { ExternalUser externalUser = idp.getUser(USER_ID);