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 dj...@apache.org on 2016/08/17 14:21:26 UTC

svn commit: r1756641 [2/2] - in /jackrabbit/oak/branches/1.4: ./ oak-commons/ oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/identifier/ oak-core/src/main/java/org/apache/jackrabbit...

Modified: jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java Wed Aug 17 14:21:25 2016
@@ -16,17 +16,25 @@
  */
 package org.apache.jackrabbit.oak.security.user;
 
+import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
 import java.util.Set;
-
+import javax.annotation.Nonnull;
 import javax.jcr.RepositoryException;
 
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Maps;
 import org.apache.jackrabbit.JcrConstants;
 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.oak.AbstractSecurityTest;
+import org.apache.jackrabbit.oak.api.PropertyState;
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.api.Type;
 import org.apache.jackrabbit.oak.spi.security.user.UserConstants;
@@ -39,56 +47,53 @@ import org.junit.Test;
 import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertTrue;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 
 /**
  * Tests large group and user graphs.
  * <ul>
- *     <li>{@link #NUM_USERS} users</li>
- *     <li>{@link #NUM_GROUPS} groups</li>
- *     <li>1 group with all users</li>
- *     <li>1 user with all groups</li>
+ * <li>{@link #NUM_USERS} users</li>
+ * <li>{@link #NUM_GROUPS} groups</li>
+ * <li>1 group with all users</li>
+ * <li>1 user with all groups</li>
  * </ul>
  *
  * @since OAK 1.0
  */
-public class MembershipProviderTest extends AbstractSecurityTest {
+public class MembershipProviderTest extends AbstractSecurityTest implements UserConstants {
 
     private static final int NUM_USERS = 1000;
     private static final int NUM_GROUPS = 1000;
     private static final int SIZE_TH = 10;
 
     private UserManagerImpl userMgr;
+    private MembershipProvider mp;
     private Set<String> testUsers = new HashSet<String>();
     private Set<String> testGroups = new HashSet<String>();
 
     @Before
     public void before() throws Exception {
         super.before();
-        testUsers.clear();
-        testGroups.clear();
         userMgr = new UserManagerImpl(root, namePathMapper, getSecurityProvider());
-        // set the membership size threshold low for testing
-        userMgr.getMembershipProvider().setMembershipSizeThreshold(SIZE_TH);
+        mp = userMgr.getMembershipProvider();
+        // set the threshold low for testing
+        mp.setMembershipSizeThreshold(SIZE_TH);
     }
 
     @After
     public void after() throws Exception {
         try {
-            for (String authId: testGroups) {
-                Authorizable auth = userMgr.getAuthorizable(authId);
-                if (auth != null) {
-                    auth.remove();
-                }
-            }
-            for (String authId: testUsers) {
-                Authorizable auth = userMgr.getAuthorizable(authId);
+            for (String path : Iterables.concat(testUsers, testGroups)) {
+                Authorizable auth = userMgr.getAuthorizableByPath(path);
                 if (auth != null) {
                     auth.remove();
                 }
             }
             root.commit();
         } finally {
+            testUsers.clear();
+            testGroups.clear();
             super.after();
         }
     }
@@ -96,24 +101,24 @@ public class MembershipProviderTest exte
     @Test
     public void testManyMembers() throws Exception {
         Set<String> members = new HashSet<String>();
-        Group grp  = createGroup();
-        for (int i=0; i<NUM_USERS; i++) {
+        Group grp = createGroup();
+        for (int i = 0; i < NUM_USERS; i++) {
             User usr = createUser();
             grp.addMember(usr);
             members.add(usr.getID());
         }
         root.commit();
-        checkMembers(grp, members);
+        assertMembers(grp, members);
 
         // also check storage structure
         Tree tree = root.getTree(grp.getPath());
         assertEquals(
                 "rep:members property must have correct number of references",
                 SIZE_TH,
-                tree.getProperty(UserConstants.REP_MEMBERS).count()
+                tree.getProperty(REP_MEMBERS).count()
         );
 
-        Tree membersList = tree.getChild(UserConstants.REP_MEMBERS_LIST);
+        Tree membersList = tree.getChild(REP_MEMBERS_LIST);
         assertTrue(
                 "rep:memberList must exist",
                 membersList.exists()
@@ -121,7 +126,7 @@ public class MembershipProviderTest exte
 
         assertEquals(
                 "rep:memberList must have correct primary type.",
-                UserConstants.NT_REP_MEMBER_REFERENCES_LIST,
+                NT_REP_MEMBER_REFERENCES_LIST,
                 membersList.getProperty(JcrConstants.JCR_PRIMARYTYPE).getValue(Type.STRING)
         );
 
@@ -136,7 +141,7 @@ public class MembershipProviderTest exte
     public void testManyMemberships() throws Exception {
         Set<String> memberships = new HashSet<String>();
         User usr = createUser();
-        for (int i=0; i<NUM_GROUPS; i++) {
+        for (int i = 0; i < NUM_GROUPS; i++) {
             Group grp = createGroup();
             grp.addMember(usr);
             memberships.add(grp.getID());
@@ -155,17 +160,17 @@ public class MembershipProviderTest exte
     public void testNestedMembers() throws Exception {
         Set<String> members = new HashSet<String>();
         Set<String> declaredMembers = new HashSet<String>();
-        Group grp  = createGroup();
-        for (int i=0; i<10; i++) {
+        Group grp = createGroup();
+        for (int i = 0; i < 10; i++) {
             Group g1 = createGroup();
             grp.addMember(g1);
             members.add(g1.getID());
             declaredMembers.add(g1.getID());
-            for (int j=0; j<10; j++) {
+            for (int j = 0; j < 10; j++) {
                 Group g2 = createGroup();
                 g1.addMember(g2);
                 members.add(g2.getID());
-                for (int k=0; k<10; k++) {
+                for (int k = 0; k < 10; k++) {
                     User usr = createUser();
                     g2.addMember(usr);
                     members.add(usr.getID());
@@ -174,7 +179,7 @@ public class MembershipProviderTest exte
         }
         root.commit();
 
-        checkMembers(grp, members);
+        assertMembers(grp, members);
 
         Iterator<Authorizable> iter = grp.getDeclaredMembers();
         while (iter.hasNext()) {
@@ -187,14 +192,14 @@ public class MembershipProviderTest exte
     @Test
     public void testNestedMemberships() throws Exception {
         Set<String> memberships = new HashSet<String>();
-        User user  = createUser();
+        User user = createUser();
         Group grp = createGroup();
         memberships.add(grp.getID());
-        for (int i=0; i<10; i++) {
+        for (int i = 0; i < 10; i++) {
             Group g1 = createGroup();
             grp.addMember(g1);
             memberships.add(g1.getID());
-            for (int j=0; j<10; j++) {
+            for (int j = 0; j < 10; j++) {
                 Group g2 = createGroup();
                 g1.addMember(g2);
                 memberships.add(g2.getID());
@@ -215,8 +220,8 @@ public class MembershipProviderTest exte
     public void testRemoveMembers() throws Exception {
         Set<String> members = new HashSet<String>();
         String[] users = new String[NUM_USERS];
-        Group grp  = createGroup();
-        for (int i=0; i<NUM_USERS; i++) {
+        Group grp = createGroup();
+        for (int i = 0; i < NUM_USERS; i++) {
             User usr = createUser();
             grp.addMember(usr);
             members.add(usr.getID());
@@ -226,24 +231,24 @@ public class MembershipProviderTest exte
 
         // remove the first TH users, this should remove all references from rep:members in the group node and remove
         // the rep:members property
-        for (int i=0; i<SIZE_TH; i++) {
+        for (int i = 0; i < SIZE_TH; i++) {
             Authorizable auth = userMgr.getAuthorizable(users[i]);
             members.remove(users[i]);
             grp.removeMember(auth);
         }
         root.commit();
 
-        checkMembers(grp, members);
+        assertMembers(grp, members);
 
         // also check storage structure
         Tree tree = root.getTree(grp.getPath());
         assertNull(
                 "rep:members property not exist",
-                tree.getProperty(UserConstants.REP_MEMBERS)
+                tree.getProperty(REP_MEMBERS)
         );
 
         // now add TH/2 again.
-        for (int i=0; i<SIZE_TH/2; i++) {
+        for (int i = 0; i < SIZE_TH / 2; i++) {
             Authorizable auth = userMgr.getAuthorizable(users[i]);
             members.add(users[i]);
             grp.addMember(auth);
@@ -253,47 +258,45 @@ public class MembershipProviderTest exte
         assertEquals(
                 "rep:members property must have correct number of references",
                 SIZE_TH / 2,
-                tree.getProperty(UserConstants.REP_MEMBERS).count()
+                tree.getProperty(REP_MEMBERS).count()
         );
-        checkMembers(grp, members);
+        assertMembers(grp, members);
 
         // now remove the users 20-30, this should remove the 2nd overflow node
-        for (int i=2*SIZE_TH; i< (3*SIZE_TH); i++) {
+        for (int i = 2 * SIZE_TH; i < (3 * SIZE_TH); i++) {
             Authorizable auth = userMgr.getAuthorizable(users[i]);
             members.remove(users[i]);
             grp.removeMember(auth);
         }
         root.commit();
 
-        checkMembers(grp, members);
+        assertMembers(grp, members);
 
-        Tree membersList = tree.getChild(UserConstants.REP_MEMBERS_LIST);
+        Tree membersList = tree.getChild(REP_MEMBERS_LIST);
         assertFalse(
                 "the first overflow node must not exist",
                 membersList.getChild("1").exists()
         );
 
         // now add 10 users and check if the "1" node exists again
-        for (int i=2*SIZE_TH; i< (3*SIZE_TH); i++) {
+        for (int i = 2 * SIZE_TH; i < (3 * SIZE_TH); i++) {
             Authorizable auth = userMgr.getAuthorizable(users[i]);
             members.add(users[i]);
             grp.addMember(auth);
         }
         root.commit();
-        checkMembers(grp, members);
+        assertMembers(grp, members);
 
-        membersList = tree.getChild(UserConstants.REP_MEMBERS_LIST);
-        assertTrue(
-                "the first overflow node must not exist",
-                membersList.getChild("1").exists()
+        membersList = tree.getChild(REP_MEMBERS_LIST);
+        assertTrue("the first overflow node must not exist", membersList.getChild("1").exists()
         );
     }
 
     @Test
     public void testAddMembersAgain() throws Exception {
         Set<String> members = new HashSet<String>();
-        Group grp  = createGroup();
-        for (int i=0; i<NUM_USERS; i++) {
+        Group grp = createGroup();
+        for (int i = 0; i < NUM_USERS; i++) {
             User usr = createUser();
             grp.addMember(usr);
             members.add(usr.getID());
@@ -308,45 +311,271 @@ public class MembershipProviderTest exte
     @Test
     public void testAddMembersAgainOnMembershipProvider() throws Exception {
         Set<String> memberPaths = new HashSet<String>();
-        Group grp  = createGroup();
-        for (int i=0; i<NUM_USERS; i++) {
+        Group grp = createGroup();
+        for (int i = 0; i < NUM_USERS; i++) {
             User usr = createUser();
             grp.addMember(usr);
             memberPaths.add(usr.getPath());
         }
         root.commit();
 
-
-        MembershipProvider mp = userMgr.getMembershipProvider();
         Tree groupTree = root.getTree(grp.getPath());
         for (String path : memberPaths) {
             Tree memberTree = root.getTree(path);
             assertFalse(mp.addMember(groupTree, memberTree));
-            assertFalse(mp.addMember(groupTree, TreeUtil.getString(memberTree, JcrConstants.JCR_UUID)));
+            assertFalse(mp.addMember(groupTree, memberTree));
+
+            String memberId = TreeUtil.getString(memberTree, REP_AUTHORIZABLE_ID);
+            Map<String, String> m = new HashMap<String, String>(1);
+            m.put(TreeUtil.getString(memberTree, JcrConstants.JCR_UUID), memberId);
+
+            Set<String> failed = mp.addMembers(groupTree, m);
+            assertFalse(failed.isEmpty());
+            assertTrue(failed.contains(memberId));
+        }
+    }
+
+    @Test
+    public void testIsDeclaredMemberTransient() throws Exception {
+        Group g = createGroup();
+        List<Authorizable> members = createMembers(g, NUM_USERS/2);
+
+        Tree groupTree = getTree(g);
+
+        // test declared members with transient modifications
+        for (Authorizable a : members) {
+            assertTrue(mp.isDeclaredMember(groupTree, getTree(a)));
+        }
+    }
+
+    @Test
+    public void testIsDeclaredMember() throws Exception {
+        Group g = createGroup();
+        List<Authorizable> members = createMembers(g, NUM_USERS/2);
+        root.commit();
+
+        Tree groupTree = getTree(g);
+
+        // test declared members after commit
+        for (Authorizable a : members) {
+            assertTrue(mp.isDeclaredMember(groupTree, getTree(a)));
+        }
+    }
+
+    @Test
+    public void testIsDeclaredMemberFew() throws Exception {
+        Group g = createGroup();
+        Group m1 = createGroup();
+        User m2 = createUser();
+
+        g.addMembers(m1.getID(), m2.getID());
+
+        Tree groupTree = getTree(g);
+        assertFalse(groupTree.hasChild(REP_MEMBERS_LIST));
+
+        // test declared members with transient modifications
+        assertTrue(mp.isDeclaredMember(groupTree, getTree(m1)));
+        assertTrue(mp.isDeclaredMember(groupTree, getTree(m2)));
+
+        // ... and after commit
+        root.commit();
+        assertTrue(mp.isDeclaredMember(groupTree, getTree(m1)));
+        assertTrue(mp.isDeclaredMember(groupTree, getTree(m2)));
+    }
+
+    @Test
+    public void testIsMemberTransient() throws Exception {
+        Group g = createGroup();
+        Group g2 = createGroup();
+        g.addMember(g2);
+
+        List<Authorizable> members = createMembers(g2, 50);
+
+        Tree groupTree = getTree(g);
+
+        // test members with transient modifications
+        for (Authorizable a : members) {
+            Tree tree = getTree(a);
+            assertFalse(mp.isDeclaredMember(groupTree, tree));
+            assertTrue(mp.isMember(groupTree, tree));
+        }
+    }
+
+    @Test
+    public void testIsMember() throws Exception {
+        Group g = createGroup();
+        Group g2 = createGroup();
+        g.addMember(g2);
+
+        List<Authorizable> members = createMembers(g2, NUM_USERS/2);
+        root.commit();
+
+        // test members after commit
+        Tree groupTree = getTree(g);
+        for (Authorizable a : members) {
+            Tree tree = getTree(a);
+            assertFalse(mp.isDeclaredMember(groupTree, tree));
+            assertTrue(mp.isMember(groupTree, tree));
         }
     }
 
+    @Test
+    public void testIsMemberFew() throws Exception {
+        Group g = createGroup();
+        Group g2 = createGroup();
+        g.addMember(g2);
+
+        User m1 = createUser();
+        Group m2 = createGroup();
+        g2.addMember(m1);
+        g2.addMember(m2);
+
+        Tree groupTree = getTree(g);
+
+        // test members with transient modifications
+        assertFalse(mp.isDeclaredMember(groupTree, getTree(m1)));
+        assertFalse(mp.isDeclaredMember(groupTree, getTree(m2)));
+        assertTrue(mp.isMember(groupTree, getTree(m1)));
+        assertTrue(mp.isMember(groupTree, getTree(m2)));
+
+        // ... and after commit
+        root.commit();
+        assertFalse(mp.isDeclaredMember(groupTree, getTree(m1)));
+        assertFalse(mp.isDeclaredMember(groupTree, getTree(m2)));
+        assertTrue(mp.isMember(groupTree, getTree(m1)));
+        assertTrue(mp.isMember(groupTree, getTree(m2)));
+    }
+
+    @Test
+    public void testTransientInMembersList() throws Exception {
+        Group g = createGroup();
+        createMembers(g, NUM_USERS/2);
+        root.commit();
+
+        // add another transient member that will end up in the members-ref-list
+        User u = createUser();
+        g.addMember(u);
+
+        Tree groupTree = getTree(g);
+        Tree memberTree = getTree(u);
+
+        assertTrue(mp.isDeclaredMember(groupTree, memberTree));
+        assertTrue(mp.isMember(groupTree, memberTree));
+
+        assertFalse(Iterators.contains(mp.getMembership(memberTree, false), groupTree.getPath()));
+        assertFalse(Iterators.contains(mp.getMembership(memberTree, true), groupTree.getPath()));
+        root.commit();
+        assertTrue(Iterators.contains(mp.getMembership(memberTree, false), groupTree.getPath()));
+        assertTrue(Iterators.contains(mp.getMembership(memberTree, true), groupTree.getPath()));
+    }
+
+    @Test
+    public void testNoMember() throws Exception {
+        Group g = createGroup();
+        Group notMember = createGroup();
+        User notMember2 = createUser();
+
+        assertFalse(g.isDeclaredMember(notMember));
+        assertFalse(g.isDeclaredMember(notMember2));
+
+        assertFalse(g.isMember(notMember));
+        assertFalse(g.isMember(notMember2));
+
+        root.commit();
+
+        assertFalse(g.isDeclaredMember(notMember));
+        assertFalse(g.isDeclaredMember(notMember2));
+
+        assertFalse(g.isMember(notMember));
+        assertFalse(g.isMember(notMember2));
+    }
+
+    @Test
+    public void testAddMembersExceedThreshold() throws Exception {
+        Tree groupTree = root.getTree(createGroup().getPath());
+
+        // 1. add array of 21 memberIDs exceeding the threshold
+        Map<String, String> memberIds = createIdMap(0, 21);
+        mp.addMembers(groupTree, memberIds);
+
+        PropertyState repMembers = groupTree.getProperty(REP_MEMBERS);
+        assertNotNull(repMembers);
+        assertEquals(SIZE_TH, repMembers.count());
+
+        // the members-list must how have two ref-members nodes one with 10 and
+        // one with a single ref-value
+        assertMemberList(groupTree, 2, 1);
+
+        // 2. add more members without reaching threshold => still 2 ref-nodes
+        memberIds = createIdMap(21, 29);
+        mp.addMembers(groupTree, memberIds);
+        assertMemberList(groupTree, 2, 9);
+
+        // 3. fill up second ref-members node => a new one must be created
+        memberIds = createIdMap(29, 35);
+        mp.addMembers(groupTree, memberIds);
+        assertMemberList(groupTree, 3, 5);
+
+        // 4. remove members from the initial set => ref nodes as before, rep:members prop on group modified
+        memberIds.clear();
+        memberIds.put(MembershipProvider.getContentID("member1"), "member1");
+        memberIds.put(MembershipProvider.getContentID("member2"), "member2");
+        mp.removeMembers(groupTree, Maps.newHashMap(memberIds));
+
+        assertMemberList(groupTree, 3, 5);
+        assertEquals(8, groupTree.getProperty(REP_MEMBERS).count());
+
+        // 5. add members again => best-tree is the ref-member-node
+        memberIds = createIdMap(35, 39);
+        mp.addMembers(groupTree, memberIds);
+
+        assertEquals(8, groupTree.getProperty(REP_MEMBERS).count());
+        assertMemberList(groupTree, 3, 9);
+
+        // 6. adding more members will fill up rep:members again and create new ref-node
+        memberIds = createIdMap(39, 45);
+        mp.addMembers(groupTree, memberIds);
+
+        assertEquals(SIZE_TH, groupTree.getProperty(REP_MEMBERS).count());
+        assertEquals(4, groupTree.getChild(REP_MEMBERS_LIST).getChildrenCount(10));
+    }
+
     private User createUser() throws RepositoryException {
         String userId = "testUser" + testUsers.size();
         User usr = userMgr.createUser(userId, "pw");
-        testUsers.add(userId);
-        if ((testUsers.size() + testGroups.size()) % 100 == 0) {
-            System.out.println("created " + testGroups.size() + " groups, " + testUsers.size() + " users.");
-        }
+        testUsers.add(usr.getPath());
         return usr;
     }
 
     private Group createGroup() throws RepositoryException {
         String groupName = "testGroup" + testGroups.size();
         Group grp = userMgr.createGroup(groupName);
-        testGroups.add(groupName);
-        if ((testUsers.size() + testGroups.size()) % 100 == 0) {
-            System.out.println("created " + testGroups.size() + " groups, " + testUsers.size() + " users.");
-        }
+        testGroups.add(grp.getPath());
         return grp;
     }
 
-    private void checkMembers(Group grp, Set<String> ms) throws RepositoryException {
+    private List<Authorizable> createMembers(@Nonnull Group g, int cnt) throws Exception {
+        List<Authorizable> members = new ArrayList();
+        for (int i = 0; i <= cnt; i++) {
+            User u = createUser();
+            Group gr = createGroup();
+            g.addMembers(u.getID(), gr.getID());
+            members.add(u);
+            members.add(gr);
+        }
+        return members;
+    }
+
+    private static Map<String, String> createIdMap(int start, int end) {
+        Map<String, String> memberIds = Maps.newLinkedHashMap();
+        for (int i = start; i < end; i++) {
+            String memberId = "member" + i;
+            memberIds.put(MembershipProvider.getContentID(memberId), memberId);
+        }
+        return memberIds;
+    }
+
+    private static void assertMembers(Group grp, Set<String> ms) throws RepositoryException {
         Set<String> members = new HashSet<String>(ms);
         Iterator<Authorizable> iter = grp.getMembers();
         while (iter.hasNext()) {
@@ -355,4 +584,19 @@ public class MembershipProviderTest exte
         }
         assertEquals("Group must have all members", 0, members.size());
     }
+
+    private static void assertMemberList(@Nonnull Tree groupTree, int cntRefTrees, int cnt) {
+        Tree list = groupTree.getChild(REP_MEMBERS_LIST);
+        assertTrue(list.exists());
+        assertEquals(cntRefTrees, list.getChildrenCount(5));
+        for (Tree c : list.getChildren()) {
+            PropertyState repMembers = c.getProperty(REP_MEMBERS);
+            assertNotNull(repMembers);
+            assertTrue(SIZE_TH == repMembers.count() || cnt == repMembers.count());
+        }
+    }
+
+    private Tree getTree(@Nonnull Authorizable a) throws Exception {
+        return root.getTree(a.getPath());
+    }
 }
\ No newline at end of file

Modified: jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java Wed Aug 17 14:21:25 2016
@@ -62,4 +62,20 @@ public class RemoveMembersByIdBestEffort
         root.refresh();
         assertFalse(testGroup.isMember(memberGroup));
     }
+
+    @Test
+    public void testMemberListExistingMembers() throws Exception {
+        MembershipProvider mp = ((UserManagerImpl) getUserManager(root)).getMembershipProvider();
+        try {
+            mp.setMembershipSizeThreshold(5);
+            for (int i = 0; i < 10; i++) {
+                testGroup.addMembers("member" + i);
+            }
+
+            Set<String> failed = testGroup.removeMembers("member8");
+            assertTrue(failed.isEmpty());
+        } finally {
+            mp.setMembershipSizeThreshold(100); // back to default
+        }
+    }
 }
\ No newline at end of file

Modified: jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java Wed Aug 17 14:21:25 2016
@@ -130,6 +130,13 @@ public class UserInitializerTest extends
         assertArrayEquals(
                 new String[]{UserConstants.NT_REP_AUTHORIZABLE},
                 Iterables.toArray(declaringNtNames, String.class));
+
+        Tree repMembers = oakIndex.getChild("repMembers");
+        assertIndexDefinition(repMembers, UserConstants.REP_MEMBERS, false);
+        declaringNtNames = TreeUtil.getStrings(repMembers, IndexConstants.DECLARING_NODE_TYPES);
+        assertArrayEquals(
+                new String[]{UserConstants.NT_REP_MEMBER_REFERENCES},
+                Iterables.toArray(declaringNtNames, String.class));
     }
 
     /**

Modified: jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java Wed Aug 17 14:21:25 2016
@@ -48,7 +48,6 @@ import org.junit.Before;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.fail;
@@ -320,9 +319,41 @@ public class UserValidatorTest extends A
 
             group1.addMember(group2);
             group2.addMember(group3);
-            
-            assertFalse(group3.addMember(group1));
-            
+
+            // manually create the cyclic membership
+            Tree group3Tree = root.getTree(group3.getPath());
+            Set<String> values = Collections.singleton(root.getTree(group1.getPath()).getProperty(JcrConstants.JCR_UUID).getValue(Type.STRING));
+            PropertyState prop = PropertyStates.createProperty(REP_MEMBERS, values, Type.WEAKREFERENCES);
+            group3Tree.setProperty(prop);
+            root.commit();
+            fail("Cyclic group membership must be detected");
+        } catch (CommitFailedException e) {
+            // success
+        } finally {
+            if (group1 != null) group1.remove();
+            if (group2 != null) group2.remove();
+            if (group3 != null) group3.remove();
+            root.commit();
+        }
+    }
+
+    @Test
+    public void testDetectCyclicMembershipWithIntermediateCommit() throws Exception {
+        Group group1 = null;
+        Group group2 = null;
+        Group group3 = null;
+
+        UserManager userMgr = getUserManager(root);
+        try {
+            group1 = userMgr.createGroup("group1");
+            group2 = userMgr.createGroup("group2");
+            group3 = userMgr.createGroup("group3");
+
+            group1.addMember(group2);
+            group2.addMember(group3);
+            root.commit();
+
+            // manually create the cyclic membership
             Tree group3Tree = root.getTree(group3.getPath());
             Set<String> values = Collections.singleton(root.getTree(group1.getPath()).getProperty(JcrConstants.JCR_UUID).getValue(Type.STRING));
             PropertyState prop = PropertyStates.createProperty(REP_MEMBERS, values, Type.WEAKREFERENCES);

Modified: jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java Wed Aug 17 14:21:25 2016
@@ -16,13 +16,14 @@
  */
 package org.apache.jackrabbit.oak.spi.security.user.action;
 
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import org.apache.jackrabbit.oak.spi.xml.ImportBehavior;
 import org.junit.Test;
 
-import java.util.List;
+import java.util.Set;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -30,7 +31,7 @@ public class GroupActionBestEffortTest e
 
     @Test
     public void testMembersAddedNonExisting() throws Exception {
-        List<String> nonExisting = ImmutableList.of("blinder", "passagier");
+        Set<String> nonExisting = ImmutableSet.of("blinder", "passagier");
 
         testGroup.addMembers(nonExisting.toArray(new String[nonExisting.size()]));
         assertTrue(Iterables.elementsEqual(nonExisting, groupAction.memberIds));
@@ -39,11 +40,11 @@ public class GroupActionBestEffortTest e
 
     @Test
     public void testMembersRemovedNonExisting() throws Exception {
-        List<String> nonExisting = ImmutableList.of("blinder", "passagier");
+        Set<String> nonExisting = ImmutableSet.of("blinder", "passagier");
 
         testGroup.removeMembers(nonExisting.toArray(new String[nonExisting.size()]));
         assertFalse(groupAction.memberIds.iterator().hasNext());
-        assertTrue(Iterables.elementsEqual(nonExisting, groupAction.failedIds));
+        assertEquals(nonExisting, groupAction.failedIds);
     }
 
     @Override

Modified: jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java Wed Aug 17 14:21:25 2016
@@ -17,6 +17,7 @@
 package org.apache.jackrabbit.oak.spi.security.user.action;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
@@ -41,6 +42,7 @@ import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.jcr.RepositoryException;
 import java.util.List;
+import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -113,20 +115,20 @@ public class GroupActionTest extends Abs
         testUser02 = getUserManager(root).createUser(TEST_USER_PREFIX + "02", "");
         testGroup.addMember(testUser02);
 
-        List<String> memberIds = ImmutableList.of(testUser01.getID());
-        List<String> failedIds = ImmutableList.of(testUser02.getID(), testGroup.getID());
+        Set<String> memberIds = ImmutableSet.of(testUser01.getID());
+        Set<String> failedIds = ImmutableSet.of(testUser02.getID(), testGroup.getID());
         Iterable<String> ids = Iterables.concat(memberIds, failedIds);
 
         testGroup.addMembers(Iterables.toArray(ids, String.class));
         assertTrue(groupAction.onMembersAddedCalled);
         assertEquals(testGroup, groupAction.group);
-        assertTrue(Iterables.elementsEqual(memberIds, groupAction.memberIds));
-        assertTrue(Iterables.elementsEqual(failedIds, groupAction.failedIds));
+        assertEquals(memberIds, groupAction.memberIds);
+        assertEquals(failedIds, groupAction.failedIds);
     }
 
     @Test
     public void testMembersAddedNonExisting() throws Exception {
-        List<String> nonExisting = ImmutableList.of("blinder", "passagier");
+        Set<String> nonExisting = ImmutableSet.of("blinder", "passagier");
 
         testGroup.addMembers(nonExisting.toArray(new String[nonExisting.size()]));
         assertFalse(groupAction.memberIds.iterator().hasNext());
@@ -139,20 +141,20 @@ public class GroupActionTest extends Abs
         testUser02 = getUserManager(root).createUser(TEST_USER_PREFIX + "02", "");
         testGroup.addMember(testUser01);
 
-        List<String> memberIds = ImmutableList.of(testUser01.getID());
-        List<String> failedIds = ImmutableList.of(testUser02.getID(), testGroup.getID());
+        Set<String> memberIds = ImmutableSet.of(testUser01.getID());
+        Set<String> failedIds = ImmutableSet.of(testUser02.getID(), testGroup.getID());
         Iterable<String> ids = Iterables.concat(memberIds, failedIds);
 
         testGroup.removeMembers(Iterables.toArray(ids, String.class));
         assertTrue(groupAction.onMembersRemovedCalled);
         assertEquals(testGroup, groupAction.group);
-        assertTrue(Iterables.elementsEqual(memberIds, groupAction.memberIds));
-        assertTrue(Iterables.elementsEqual(failedIds, groupAction.failedIds));
+        assertEquals(memberIds, groupAction.memberIds);
+        assertEquals(failedIds, groupAction.failedIds);
     }
 
     @Test
     public void testMembersRemovedNonExisting() throws Exception {
-        List<String> nonExisting = ImmutableList.of("blinder", "passagier");
+        Set<String> nonExisting = ImmutableSet.of("blinder", "passagier");
 
         testGroup.removeMembers(nonExisting.toArray(new String[nonExisting.size()]));
         assertFalse(groupAction.memberIds.iterator().hasNext());
@@ -175,8 +177,8 @@ public class GroupActionTest extends Abs
         boolean onMembersRemovedCalled = false;
 
         Group group;
-        Iterable<String> memberIds;
-        Iterable<String> failedIds;
+        Set<String> memberIds;
+        Set<String> failedIds;
         Authorizable member;
 
         @Override
@@ -189,8 +191,8 @@ public class GroupActionTest extends Abs
         @Override
         public void onMembersAdded(@Nonnull Group group, @Nonnull Iterable<String> memberIds, @Nonnull Iterable<String> failedIds, @Nonnull Root root, @Nonnull NamePathMapper namePathMapper) throws RepositoryException {
             this.group = group;
-            this.memberIds = memberIds;
-            this.failedIds = failedIds;
+            this.memberIds = ImmutableSet.copyOf(memberIds);
+            this.failedIds = ImmutableSet.copyOf(failedIds);
             onMembersAddedCalled = true;
         }
 
@@ -204,8 +206,8 @@ public class GroupActionTest extends Abs
         @Override
         public void onMembersRemoved(@Nonnull Group group, @Nonnull Iterable<String> memberIds, @Nonnull Iterable<String> failedIds, @Nonnull Root root, @Nonnull NamePathMapper namePathMapper) throws RepositoryException {
             this.group = group;
-            this.memberIds = memberIds;
-            this.failedIds = failedIds;
+            this.memberIds = ImmutableSet.copyOf(memberIds);
+            this.failedIds = ImmutableSet.copyOf(failedIds);
             onMembersRemovedCalled = true;
         }
     }

Modified: jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.java Wed Aug 17 14:21:25 2016
@@ -45,6 +45,7 @@ import org.apache.jackrabbit.api.securit
 import org.apache.jackrabbit.api.security.user.User;
 import org.apache.jackrabbit.api.security.user.UserManager;
 import org.apache.jackrabbit.commons.jackrabbit.authorization.AccessControlUtils;
+import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
 import org.apache.jackrabbit.test.NotExecutableException;
 import org.apache.jackrabbit.test.api.security.AbstractAccessControlTest;
 import org.junit.After;

Modified: jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java Wed Aug 17 14:21:25 2016
@@ -23,10 +23,12 @@ import javax.jcr.Node;
 import javax.jcr.PropertyType;
 import javax.jcr.Session;
 import javax.jcr.Value;
+import javax.jcr.nodetype.ConstraintViolationException;
 
 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.oak.api.CommitFailedException;
 import org.apache.jackrabbit.oak.spi.security.user.UserConstants;
 import org.apache.jackrabbit.oak.spi.xml.ImportBehavior;
 import org.apache.jackrabbit.test.NotExecutableException;
@@ -170,18 +172,31 @@ public class GroupImportBestEffortTest e
         membership references.
         expected:
         - group is imported
-        - circular membership is ignored
+        - circular membership is ignored (OR fails upon save)
         - g is member of g1
-        - g1 isn't member of g
+        - g1 isn't member of g  (circular membership detected) OR saving changes fails
         */
         doImport(getTargetPath() + "/gFolder", xml2);
 
-        Authorizable g = getUserManager().getAuthorizable("g");
-        assertNotNull(g);
-        if (g.isGroup()) {
-            assertNotDeclaredMember((Group) g, g1Id, getImportSession());
+        Group g = getUserManager().getAuthorizable("g", Group.class);
+        Group g1 = getUserManager().getAuthorizable("g1", Group.class);
+
+        assertNotNull("'g' was not imported as Group.", g);
+        assertNotNull("'g1' was not imported as Group.", g1);
+
+        assertTrue(g1.isDeclaredMember(g));
+        if (g.isDeclaredMember(g1)) {
+            // circular membership created during import
+            try {
+                getImportSession().save();
+                fail("Circular membership must be detected upon save.");
+            } catch (ConstraintViolationException e) {
+                Throwable th = e.getCause();
+                assertTrue(th instanceof CommitFailedException);
+                assertEquals(31, ((CommitFailedException) th).getCode());
+            }
         } else {
-            fail("'g' was not imported as Group.");
+            assertNotDeclaredMember(g, g1Id, getImportSession());
         }
     }
 

Modified: jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsBestEffortTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsBestEffortTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsBestEffortTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsBestEffortTest.java Wed Aug 17 14:21:25 2016
@@ -16,9 +16,9 @@
  */
 package org.apache.jackrabbit.oak.jcr.security.user;
 
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterables;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
 import org.apache.jackrabbit.api.security.user.User;
@@ -40,6 +40,7 @@ import javax.jcr.RepositoryException;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.UUID;
 
 import static org.junit.Assert.assertEquals;
@@ -62,8 +63,6 @@ public class GroupImportWithActionsBestE
 
     @Test
     public void testImportMembersBestEffort() throws Exception {
-
-
         User user1 = getUserManager().createUser("user1", "");
         String uuid1 = getImportSession().getNode(user1.getPath()).getUUID();
         User user2 = getUserManager().createUser("user2", "");
@@ -80,7 +79,8 @@ public class GroupImportWithActionsBestE
                         "   <sv:node sv:name=\"g1\">" +
                         "      <sv:property sv:name=\"jcr:primaryType\" sv:type=\"Name\"><sv:value>rep:Group</sv:value></sv:property>" +
                         "      <sv:property sv:name=\"jcr:uuid\" sv:type=\"String\"><sv:value>0120a4f9-196a-3f9e-b9f5-23f31f914da7</sv:value></sv:property>" +
-                        "      <sv:property sv:name=\"rep:principalName\" sv:type=\"String\"><sv:value>g1</sv:value></sv:property>" + "   <sv:property sv:name=\"rep:members\" sv:multiple=\"true\" sv:type=\"WeakReference\">" +
+                        "      <sv:property sv:name=\"rep:principalName\" sv:type=\"String\"><sv:value>g1</sv:value></sv:property>" +
+                        "      <sv:property sv:name=\"rep:members\" sv:multiple=\"true\" sv:type=\"WeakReference\">" +
                         "         <sv:value>" + uuid1 + "</sv:value>" +
                         "         <sv:value>" + uuid2 + "</sv:value>" +
                         "         <sv:value>" + nonExistingUUID + "</sv:value>" +
@@ -92,11 +92,11 @@ public class GroupImportWithActionsBestE
         doImport(getTargetPath(), xml);
 
         Group g1 = (Group) getUserManager().getAuthorizable("g1");
-        assertTrue(groupAction.onMemberAddedCalled);
+        assertTrue(groupAction.onMembersAddedCalled);
         assertTrue(groupAction.onMembersAddedContentIdCalled);
         assertEquals(g1.getID(), groupAction.group.getID());
-        assertTrue(Iterables.elementsEqual(ImmutableList.of(user1.getID(), user2.getID()), groupAction.memberIds));
-        assertTrue(Iterables.elementsEqual(ImmutableList.of(nonExistingUUID), groupAction.memberContentIds));
+        assertEquals(ImmutableSet.of(user1.getID(), user2.getID()), groupAction.memberIds);
+        assertEquals(ImmutableSet.of(nonExistingUUID), groupAction.memberContentIds);
         assertFalse(groupAction.failedIds.iterator().hasNext()); // duplicate uuids are swallowed by the set in userImporter: nonExisting#add
     }
 
@@ -121,25 +121,34 @@ public class GroupImportWithActionsBestE
     private class TestGroupAction extends AbstractGroupAction {
 
         boolean onMemberAddedCalled = false;
+        boolean onMembersAddedCalled = false;
         boolean onMembersAddedContentIdCalled = false;
 
         Group group;
-        List<String> memberIds = Lists.newArrayList();
-        Iterable<String> memberContentIds = Lists.newArrayList();
-        Iterable<String> failedIds;
+        Set<String> memberIds = Sets.newHashSet();
+        Set<String> memberContentIds = Sets.newHashSet();
+        Set<String> failedIds = Sets.newHashSet();
 
         @Override
         public void onMemberAdded(@Nonnull Group group, @Nonnull Authorizable member, @Nonnull Root root, @Nonnull NamePathMapper namePathMapper) throws RepositoryException {
             this.group = group;
-            memberIds.add(member.getID());
+            this.memberIds.add(member.getID());
             onMemberAddedCalled = true;
         }
 
         @Override
+        public void onMembersAdded(@Nonnull Group group, @Nonnull Iterable<String> memberIds, @Nonnull Iterable<String> failedIds, @Nonnull Root root, @Nonnull NamePathMapper namePathMapper) throws RepositoryException {
+            this.group = group;
+            this.memberIds.addAll(ImmutableSet.copyOf(memberIds));
+            this.failedIds.addAll(ImmutableSet.copyOf(failedIds));
+            onMembersAddedCalled = true;
+        }
+
+        @Override
         public void onMembersAddedContentId(@Nonnull Group group, @Nonnull Iterable<String> memberContentIds, @Nonnull Iterable<String> failedIds, @Nonnull Root root, @Nonnull NamePathMapper namePathMapper) throws RepositoryException {
             this.group = group;
-            this.memberContentIds = memberContentIds;
-            this.failedIds = failedIds;
+            this.memberContentIds.addAll(ImmutableSet.copyOf(memberContentIds));
+            this.failedIds.addAll(ImmutableSet.copyOf(failedIds));
             onMembersAddedContentIdCalled = true;
         }
     }

Modified: jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.java Wed Aug 17 14:21:25 2016
@@ -16,9 +16,9 @@
  */
 package org.apache.jackrabbit.oak.jcr.security.user;
 
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterables;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
 import org.apache.jackrabbit.api.security.user.User;
@@ -40,6 +40,7 @@ import javax.jcr.RepositoryException;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.UUID;
 
 import static org.junit.Assert.assertEquals;
@@ -91,10 +92,13 @@ public class GroupImportWithActionsTest
         doImport(getTargetPath(), xml);
 
         Group g1 = (Group) getUserManager().getAuthorizable("g1");
-        assertTrue(groupAction.onMemberAddedCalled);
         assertEquals(g1.getID(), groupAction.group.getID());
-        assertTrue(Iterables.elementsEqual(ImmutableList.of(user1.getID(), user2.getID()), groupAction.memberIds));
+
+        assertFalse(groupAction.onMemberAddedCalled);
         assertFalse(groupAction.onMembersAddedContentIdCalled);
+
+        assertTrue(groupAction.onMembersAddedCalled);
+        assertEquals(ImmutableSet.of(user1.getID(), user2.getID()), groupAction.memberIds);
     }
 
     @Override
@@ -118,14 +122,21 @@ public class GroupImportWithActionsTest
     private class TestGroupAction extends AbstractGroupAction {
 
         private boolean onMemberAddedCalled = false;
+        private boolean onMembersAddedCalled = false;
         private boolean onMembersAddedContentIdCalled = false;
 
         Group group;
-        List<String> memberIds = Lists.newArrayList();
+        Set<String> memberIds = Sets.newHashSet();
 
         @Override
-        public void onMemberAdded(@Nonnull Group group, @Nonnull Authorizable member, @Nonnull Root root, @Nonnull NamePathMapper namePathMapper) throws RepositoryException {
+        public void onMembersAdded(@Nonnull Group group, @Nonnull Iterable<String> memberIds, @Nonnull Iterable<String> failedIds, @Nonnull Root root, @Nonnull NamePathMapper namePathMapper) throws RepositoryException {
             this.group = group;
+            this.memberIds.addAll(ImmutableSet.copyOf(memberIds));
+            onMembersAddedCalled = true;
+        }
+
+        @Override
+        public void onMemberAdded(@Nonnull Group group, @Nonnull Authorizable member, @Nonnull Root root, @Nonnull NamePathMapper namePathMapper) throws RepositoryException {
             memberIds.add(member.getID());
             onMemberAddedCalled = true;
         }

Modified: jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java Wed Aug 17 14:21:25 2016
@@ -31,6 +31,8 @@ import org.apache.jackrabbit.api.securit
 import org.apache.jackrabbit.api.security.user.AuthorizableExistsException;
 import org.apache.jackrabbit.api.security.user.Group;
 import org.apache.jackrabbit.api.security.user.User;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
 import org.apache.jackrabbit.test.NotExecutableException;
 import org.apache.jackrabbit.util.Text;
 import org.junit.Before;
@@ -600,8 +602,13 @@ public class GroupTest extends AbstractU
             superuser.save();
             assertTrue(group2.addMember(group3));
             superuser.save();
-            assertFalse(group3.addMember(group1));
-            superuser.save();
+
+            if (group3.addMember(group1)) {
+                superuser.save();
+                fail("Cyclic group membership must be detected.");
+            }
+        } catch (RepositoryException e) {
+            assertCyclicCommitFailed(e);
         } finally {
             if (group1 != null) group1.remove();
             if (group2 != null) group2.remove();
@@ -621,9 +628,13 @@ public class GroupTest extends AbstractU
 
             assertTrue(group1.addMember(group2));
             assertTrue(group2.addMember(group3));
-            assertFalse("Cyclic group membership must be detected.", group3.addMember(group1));
+            if (group3.addMember(group1)) {
+                // circular membership not detected => try save
+                superuser.save();
+                fail("Cyclic group membership must be detected.");
+            } // else: success, circular membership detected upon addMember
         } catch (RepositoryException e) {
-            // success
+            assertCyclicCommitFailed(e);
         } finally {
             if (group1 != null) group1.remove();
             if (group2 != null) group2.remove();
@@ -631,6 +642,12 @@ public class GroupTest extends AbstractU
         }
     }
 
+    private static void assertCyclicCommitFailed(RepositoryException e) {
+        Throwable th = e.getCause();
+        assertTrue(th instanceof CommitFailedException);
+        assertEquals(31, ((CommitFailedException) th).getCode());
+    }
+
     @Test
     public void testRemoveMemberTwice() throws NotExecutableException, RepositoryException {
         User auth = getTestUser(superuser);
@@ -864,6 +881,28 @@ public class GroupTest extends AbstractU
         }
     }
 
+    public void testAddSelfById() throws Exception {
+        Set<String> failed = group.addMembers(group.getID());
+        assertFalse(failed.isEmpty());
+        assertTrue(failed.contains(group.getID()));
+    }
+
+    public void testAddToEveryoneById() throws Exception {
+        Group everyone = null;
+        try {
+            everyone = userMgr.createGroup(EveryonePrincipal.getInstance());
+
+            Set<String> failed = everyone.addMembers(group.getID());
+            assertFalse(failed.isEmpty());
+            assertTrue(failed.contains(group.getID()));
+        } finally {
+            if (everyone != null) {
+                everyone.remove();
+                superuser.save();
+            }
+        }
+    }
+
     public void testRemoveMembersById() throws Exception {
         Group newGroup = null;
         try {
@@ -878,7 +917,28 @@ public class GroupTest extends AbstractU
                 superuser.save();
             }
         }
+    }
+
+    public void testRemoveSelfById() throws Exception {
+        Set<String> failed = group.removeMembers(group.getID());
+        assertFalse(failed.isEmpty());
+        assertTrue(failed.contains(group.getID()));
+    }
 
+    public void testRemoveFromEveryoneById() throws Exception {
+        Group everyone = null;
+        try {
+            everyone = userMgr.createGroup(EveryonePrincipal.getInstance());
+
+            Set<String> failed = everyone.removeMembers(group.getID());
+            assertFalse(failed.isEmpty());
+            assertTrue(failed.contains(group.getID()));
+        } finally {
+            if (everyone != null) {
+                everyone.remove();
+                superuser.save();
+            }
+        }
     }
 
     private void checkDeclaredMembers(Group grp, String ... ids) throws RepositoryException {

Modified: jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java Wed Aug 17 14:21:25 2016
@@ -18,12 +18,14 @@ package org.apache.jackrabbit.oak.jcr.se
 
 import java.security.Principal;
 import javax.jcr.RepositoryException;
+import javax.jcr.nodetype.ConstraintViolationException;
 
 import org.apache.jackrabbit.api.JackrabbitSession;
 import org.apache.jackrabbit.api.security.principal.PrincipalIterator;
 import org.apache.jackrabbit.api.security.principal.PrincipalManager;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
 import org.apache.jackrabbit.test.NotExecutableException;
 import org.junit.Test;
 
@@ -100,7 +102,12 @@ public class NestedGroupTest extends Abs
             gr2 = createGroup(getTestPrincipal());
 
             assertTrue(addMember(gr1, gr2));
-            assertFalse(addMember(gr2, gr1));
+            try {
+                assertFalse(addMember(gr2, gr1));
+            } catch (ConstraintViolationException e) {
+                // cycle detected upon save => success
+                assertCyclicMembershipError(e);
+            }
 
         } finally {
             if (gr1 != null && gr1.isMember(gr2)) {
@@ -126,7 +133,12 @@ public class NestedGroupTest extends Abs
 
             assertTrue(addMember(gr1, gr2));
             assertTrue(addMember(gr2, gr3));
-            assertFalse(addMember(gr3, gr1));
+            try {
+                assertFalse(addMember(gr3, gr1));
+            } catch (ConstraintViolationException e) {
+                // cycle detected upon save => success
+                assertCyclicMembershipError(e);
+            }
 
         } finally {
             if (gr1 != null) {
@@ -145,6 +157,14 @@ public class NestedGroupTest extends Abs
         }
     }
 
+    private static void assertCyclicMembershipError(Exception e) {
+        Throwable th = e.getCause();
+        assertTrue(th instanceof CommitFailedException);
+        CommitFailedException ce = (CommitFailedException) th;
+        assertEquals(CommitFailedException.CONSTRAINT, ce.getType());
+        assertEquals(31, ce.getCode());
+    }
+
     @Test
     public void testInheritedMembership() throws NotExecutableException, RepositoryException {
         Group gr1 = null;

Modified: jackrabbit/oak/branches/1.4/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/cli/SegmentToJdbcTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.4/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/cli/SegmentToJdbcTest.java?rev=1756641&r1=1756640&r2=1756641&view=diff
==============================================================================
--- jackrabbit/oak/branches/1.4/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/cli/SegmentToJdbcTest.java (original)
+++ jackrabbit/oak/branches/1.4/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/cli/SegmentToJdbcTest.java Wed Aug 17 14:21:25 2016
@@ -21,7 +21,9 @@ import java.io.IOException;
 import org.apache.jackrabbit.oak.upgrade.cli.container.JdbcNodeStoreContainer;
 import org.apache.jackrabbit.oak.upgrade.cli.container.NodeStoreContainer;
 import org.apache.jackrabbit.oak.upgrade.cli.container.SegmentNodeStoreContainer;
+import org.junit.Ignore;
 
+@Ignore
 public class SegmentToJdbcTest extends AbstractOak2OakTest {
 
     private final NodeStoreContainer source;