Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java Thu Apr 7 14:57:18 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/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java Thu Apr 7 14:57:18 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/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java Thu Apr 7 14:57:18 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/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java Thu Apr 7 14:57:18 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/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java Thu Apr 7 14:57:18 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/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java Thu Apr 7 14:57:18 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/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.java Thu Apr 7 14:57:18 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/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java Thu Apr 7 14:57:18 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/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsBestEffortTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsBestEffortTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsBestEffortTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsBestEffortTest.java Thu Apr 7 14:57:18 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/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.java Thu Apr 7 14:57:18 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/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java Thu Apr 7 14:57:18 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/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java Thu Apr 7 14:57:18 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;
