This is an automated email from the ASF dual-hosted git repository.
angela pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push:
new da19433ebe OAK-10486 : Resolution of inherited groups may terminate
pre-maturely for external users (#1147)
da19433ebe is described below
commit da19433ebe9eed5dc78e938d05fb01ce7364d3a6
Author: anchela <[email protected]>
AuthorDate: Tue Oct 17 08:50:22 2023 +0200
OAK-10486 : Resolution of inherited groups may terminate pre-maturely for
external users (#1147)
---
.../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);
