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/19 14:27:28 UTC
[jackrabbit-oak] branch trunk updated: OAK-10071 : Consistently filter duplicate authorizables in iterators
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 7f4296a2d4 OAK-10071 : Consistently filter duplicate authorizables in iterators
7f4296a2d4 is described below
commit 7f4296a2d42e7d80e83e51069ed531cde4488be2
Author: angela <an...@adobe.com>
AuthorDate: Thu Jan 19 15:27:14 2023 +0100
OAK-10071 : Consistently filter duplicate authorizables in iterators
---
.../oak/security/user/AuthorizableImpl.java | 6 +-
.../oak/security/user/AuthorizableIterator.java | 7 ++
.../jackrabbit/oak/security/user/GroupImpl.java | 4 +-
.../oak/security/user/DuplicateMembershipTest.java | 130 +++++++++++++++++++++
4 files changed, 140 insertions(+), 7 deletions(-)
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizableImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizableImpl.java
index 7e4418682d..f9647c8e4e 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizableImpl.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizableImpl.java
@@ -289,11 +289,7 @@ abstract class AuthorizableImpl implements Authorizable, UserConstants, TreeAwar
MembershipProvider mMgr = getMembershipProvider();
Iterator<Tree> trees = mMgr.getMembership(getTree(), includeInherited);
- if (!trees.hasNext()) {
- return dynamicGroups;
- }
-
- AuthorizableIterator groups = AuthorizableIterator.create(trees, userManager, AuthorizableType.GROUP);
+ AuthorizableIterator groups = (!trees.hasNext()) ? AuthorizableIterator.empty() : AuthorizableIterator.create(trees, userManager, AuthorizableType.GROUP);
AuthorizableIterator allGroups = AuthorizableIterator.create(true, dynamicGroups, groups);
return new RangeIteratorAdapter(allGroups);
}
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizableIterator.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizableIterator.java
index 81f25592af..eabac4abdc 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizableIterator.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizableIterator.java
@@ -44,6 +44,8 @@ final class AuthorizableIterator implements Iterator<Authorizable> {
private final Iterator<Authorizable> authorizables;
private final long size;
private final Set<String> servedIds;
+
+ private static AuthorizableIterator EMPTY = new AuthorizableIterator(Iterators.emptyIterator(), 0, false);
@NotNull
static AuthorizableIterator create(@NotNull Iterator<Tree> authorizableTrees,
@@ -68,6 +70,11 @@ final class AuthorizableIterator implements Iterator<Authorizable> {
}
return new AuthorizableIterator(Iterators.concat(it1, it2), size, filterDuplicates);
}
+
+ @NotNull
+ static AuthorizableIterator empty() {
+ return EMPTY;
+ }
private AuthorizableIterator(Iterator<Authorizable> authorizables, long size, boolean filterDuplicates) {
if (filterDuplicates) {
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 7df7efc72c..59d0fa786f 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
@@ -213,13 +213,13 @@ class GroupImpl extends AuthorizableImpl implements Group {
DynamicMembershipProvider dmp = getUserManager().getDynamicMembershipProvider();
Iterator<Authorizable> dynamicMembers = dmp.getMembers(this, includeInherited);
if (dmp.coversAllMembers(this)) {
- return dynamicMembers;
+ return AuthorizableIterator.create(true, dynamicMembers, AuthorizableIterator.empty());
}
// dynamic membership didn't cover all members -> extract from group-tree
Iterator<Tree> trees = getMembershipProvider().getMembers(getTree(), includeInherited);
if (!trees.hasNext()) {
- return dynamicMembers;
+ return AuthorizableIterator.create(true, dynamicMembers, AuthorizableIterator.empty());
}
AuthorizableIterator members = AuthorizableIterator.create(trees, userMgr, AuthorizableType.AUTHORIZABLE);
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/DuplicateMembershipTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/DuplicateMembershipTest.java
new file mode 100644
index 0000000000..9df87bda75
--- /dev/null
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/DuplicateMembershipTest.java
@@ -0,0 +1,130 @@
+/*
+ * 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.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 static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+public class DuplicateMembershipTest extends AbstractSecurityTest {
+
+ private DynamicMembershipProvider dmp;
+
+ private Group group;
+ private Authorizable member;
+
+ @Before
+ public void before() throws Exception {
+ super.before();
+ UserManagerImpl userManager = (UserManagerImpl) spy(getUserManager(root));
+ member = userManager.getAuthorizable(getTestUser().getID());
+
+ group = userManager.createGroup("dynamicTestGroup");
+ group.addMember(member);
+ root.commit();
+
+ dmp = mockDynamicMembershipProvider();
+ when(userManager.getDynamicMembershipProvider()).thenReturn(dmp);
+ }
+
+ @Override
+ public void after() throws Exception {
+ try {
+ group.remove();
+ root.commit();
+ } finally {
+ super.after();
+ }
+ }
+
+ private @NotNull DynamicMembershipProvider mockDynamicMembershipProvider() throws RepositoryException {
+ DynamicMembershipProvider dmp = mock(DynamicMembershipProvider.class);
+ when(dmp.coversAllMembers(any(Group.class))).thenReturn(true);
+ // mock iterators with duplicate entries
+ when(dmp.getMembers(any(Group.class), anyBoolean())).thenReturn(Iterators.forArray(member, member));
+ when(dmp.getMembership(any(Authorizable.class), anyBoolean())).thenReturn(Iterators.forArray(group, group));
+ return dmp;
+ }
+
+ @Test
+ public void testGetMembers() throws Exception {
+ when(dmp.coversAllMembers(any(Group.class))).thenReturn(false);
+ assertEquals(1, Iterators.size(group.getMembers()));
+ }
+
+ @Test
+ public void testGetDeclaredMembers() throws Exception {
+ when(dmp.coversAllMembers(any(Group.class))).thenReturn(false);
+ assertEquals(1, Iterators.size(group.getDeclaredMembers()));
+ }
+
+ @Test
+ public void testGetMembersCoversAll() throws Exception {
+ when(dmp.coversAllMembers(any(Group.class))).thenReturn(true);
+ assertEquals(1, Iterators.size(group.getMembers()));
+ }
+
+ @Test
+ public void testGetDeclaredMembersCoversAll() throws Exception {
+ when(dmp.coversAllMembers(any(Group.class))).thenReturn(true);
+ assertEquals(1, Iterators.size(group.getDeclaredMembers()));
+ }
+
+ @Test
+ public void testGetMembership() throws Exception {
+ assertEquals(1, Iterators.size(member.memberOf()));
+ }
+
+ @Test
+ public void testGetDeclaredMembership() throws Exception {
+ assertEquals(1, Iterators.size(member.declaredMemberOf()));
+ }
+
+ @Test
+ public void testGetMembershipLocalMembershipRemoved() throws Exception {
+ when(dmp.coversAllMembers(any(Group.class))).thenReturn(false);
+
+ group.removeMember(member);
+ root.commit();
+
+ assertEquals(1, Iterators.size(member.memberOf()));
+ }
+
+ @Test
+ public void testGetDeclaredMembershipLocalMembershipRemoved() throws Exception {
+ when(dmp.coversAllMembers(any(Group.class))).thenReturn(false);
+
+ group.removeMember(member);
+ root.commit();
+
+ assertEquals(1, Iterators.size(member.declaredMemberOf()));
+ }
+}
\ No newline at end of file