Index: oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java (revision 1788086) +++ oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java (revision ) @@ -644,8 +644,10 @@ private static void assertCyclicCommitFailed(RepositoryException e) { Throwable th = e.getCause(); + if (th != null) { - assertTrue(th instanceof CommitFailedException); - assertEquals(31, ((CommitFailedException) th).getCode()); + assertTrue(th instanceof CommitFailedException); + assertEquals(31, ((CommitFailedException) th).getCode()); + } } @Test \ No newline at end of file Index: oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java (revision 1788086) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java (revision ) @@ -301,76 +301,6 @@ } } - - /** - * @since oak 1.0 cyclic group membership added in a single set of transient - * modifications must be detected upon save. - */ - @Test - public void testDetectCyclicMembership() 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); - - // manually create the cyclic membership - Tree group3Tree = root.getTree(group3.getPath()); - Set 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 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 testCreateNestedUser() throws Exception { Tree userTree = root.getTree(getTestUser().getPath()); \ No newline at end of file Index: oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java (revision 1788086) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java (revision ) @@ -16,14 +16,22 @@ */ package org.apache.jackrabbit.oak.security.user; +import java.util.Set; import javax.jcr.nodetype.ConstraintViolationException; +import org.apache.jackrabbit.api.security.user.Group; +import org.apache.jackrabbit.oak.api.CommitFailedException; import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters; +import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal; import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; import org.apache.jackrabbit.oak.spi.xml.ImportBehavior; import org.apache.jackrabbit.oak.spi.xml.ProtectedItemImporter; import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + public class AddMembersByIdAbortTest extends AbstractAddMembersByIdTest { @Override @@ -42,5 +50,38 @@ @Test(expected = ConstraintViolationException.class) public void testExistingMemberWithoutAccess() throws Exception { addExistingMemberWithoutAccess(); + } + + @Test + public void testCyclicMembership() throws Exception { + memberGroup.addMember(testGroup); + root.commit(); + + try { + Set failed = testGroup.addMembers(memberGroup.getID()); + assertTrue(failed.isEmpty()); + root.commit(); + fail("cyclic group membership must be detected latest upon commit"); + } catch (CommitFailedException e) { + // success + assertTrue(e.isConstraintViolation()); + assertEquals(31, e.getCode()); + } catch (ConstraintViolationException e) { + // success + } + } + + @Test + public void testEveryoneAsMember() throws Exception { + UserManagerImpl userManager = (UserManagerImpl) getUserManager(root); + Group everyone = userManager.createGroup(EveryonePrincipal.getInstance()); + try { + testGroup.addMembers(everyone.getID()); + fail("adding everyone as member must be detected"); + } catch (ConstraintViolationException e) { + // success + } finally { + everyone.remove(); + } } } \ No newline at end of file Index: oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java (revision 1788086) +++ oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java (revision ) @@ -159,10 +159,12 @@ private static void assertCyclicMembershipError(Exception e) { Throwable th = e.getCause(); + if (th != null) { - assertTrue(th instanceof CommitFailedException); - CommitFailedException ce = (CommitFailedException) th; - assertEquals(CommitFailedException.CONSTRAINT, ce.getType()); - assertEquals(31, ce.getCode()); + assertTrue(th instanceof CommitFailedException); + CommitFailedException ce = (CommitFailedException) th; + assertEquals(CommitFailedException.CONSTRAINT, ce.getType()); + assertEquals(31, ce.getCode()); + } } @Test \ No newline at end of file Index: oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java (revision 1788086) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java (revision ) @@ -31,7 +31,6 @@ import org.apache.jackrabbit.api.security.user.UserManager; import org.apache.jackrabbit.commons.jackrabbit.authorization.AccessControlUtils; import org.apache.jackrabbit.oak.AbstractSecurityTest; -import org.apache.jackrabbit.oak.api.CommitFailedException; import org.apache.jackrabbit.oak.api.ContentSession; import org.apache.jackrabbit.oak.api.PropertyState; import org.apache.jackrabbit.oak.api.Root; @@ -48,7 +47,6 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; public abstract class AbstractAddMembersByIdTest extends AbstractSecurityTest { @@ -237,16 +235,8 @@ root.commit(); Set failed = testGroup.addMembers(memberGroup.getID()); - assertTrue(failed.isEmpty()); - - try { - root.commit(); - fail("cyclic group membership must be detected upon commit"); - } catch (CommitFailedException e) { - // success - assertTrue(e.isConstraintViolation()); - assertEquals(31, e.getCode()); - } + assertFalse(failed.isEmpty()); + assertTrue(failed.contains(memberGroup.getID())); } @Test \ No newline at end of file Index: oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportIgnoreTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportIgnoreTest.java (revision 1788086) +++ oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportIgnoreTest.java (revision ) @@ -20,11 +20,16 @@ import java.util.List; import java.util.UUID; +import com.google.common.collect.Iterators; import org.apache.jackrabbit.api.security.user.Authorizable; import org.apache.jackrabbit.api.security.user.Group; import org.apache.jackrabbit.oak.spi.xml.ImportBehavior; +import org.apache.jackrabbit.test.NotExecutableException; import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; /** @@ -94,5 +99,48 @@ getImportSession().refresh(false); } } + } + + @Test + public void testImportCircularMembership() throws Exception { + String g1Id = "0120a4f9-196a-3f9e-b9f5-23f31f914da7"; + String gId = "b2f5ff47-4366-31b6-a533-d8dc3614845d"; // groupId of 'g' group. + if (getUserManager().getAuthorizable("g") != null) { + throw new NotExecutableException(); + } + + String xml = "" + + "" + + " rep:AuthorizableFolder" + + "rep:Group" + + " " + g1Id + "" + + " g1" + + " " +gId+ "" + + "" + + "rep:Group" + + " " + gId + "" + + " g1" + + " " +g1Id+ "" + + "" + + ""; + + /* + import groups with cyclic membership with with IGNORE. + expected: + - group is imported + - circular membership is ignored + - g is member of g1 + - g1 isn't member of g (circular membership detected) + */ + doImport(getTargetPath(), xml); + + Group g1 = userMgr.getAuthorizable("g1", Group.class); + Group g = userMgr.getAuthorizable("g", Group.class); + + boolean b = g1.isDeclaredMember(g); + assertEquals("Circular membership must be detected", !b, g.isDeclaredMember(g1)); + + assertEquals("Circular membership must be detected", b, Iterators.contains(g1.getMembers(), g)); + assertEquals("Circular membership must be detected", !b, Iterators.contains(g.getMembers(), g1)); } } Index: oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java (revision 1788086) +++ oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java (revision ) @@ -23,12 +23,11 @@ import javax.jcr.PropertyType; import javax.jcr.Session; import javax.jcr.Value; -import javax.jcr.nodetype.ConstraintViolationException; +import com.google.common.collect.Iterators; import org.apache.jackrabbit.api.security.user.Authorizable; import org.apache.jackrabbit.api.security.user.Group; import org.apache.jackrabbit.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; @@ -168,13 +167,11 @@ doImport(getTargetPath(), xml); /* - now try to import the 'g' group that has a circular group - membership references. + now try to import the 'g' group that has a circular group membership references. expected: - group is imported - - circular membership is ignored (OR fails upon save) - - g is member of g1 - - g1 isn't member of g (circular membership detected) OR saving changes fails + - circular membership is not spotted due to best-effort optimization + - circular membership is spotted upon resolution of members */ doImport(getTargetPath() + "/gFolder", xml2); @@ -185,19 +182,11 @@ 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 { - assertNotDeclaredMember(g, g1Id, getImportSession()); - } + assertTrue(g.isDeclaredMember(g1)); + + // circular membership created during import -> must be spotted upon member-access + assertEquals(1, Iterators.size(g1.getMembers())); + assertEquals(1, Iterators.size(g.getMembers())); } @Test Index: oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportAbortTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportAbortTest.java (revision 1788086) +++ oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportAbortTest.java (revision ) @@ -20,10 +20,15 @@ import java.util.List; import java.util.UUID; import javax.jcr.RepositoryException; +import javax.jcr.nodetype.ConstraintViolationException; +import org.apache.jackrabbit.oak.api.CommitFailedException; import org.apache.jackrabbit.oak.spi.xml.ImportBehavior; +import org.apache.jackrabbit.test.NotExecutableException; import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; /** @@ -86,6 +91,45 @@ fail("Importing self as group with ImportBehavior.ABORT must fail."); } catch (RepositoryException e) { // success. + } + } + + @Test + public void testImportCircularMembership() throws Exception { + String g1Id = "0120a4f9-196a-3f9e-b9f5-23f31f914da7"; + String gId = "b2f5ff47-4366-31b6-a533-d8dc3614845d"; // groupId of 'g' group. + if (getUserManager().getAuthorizable("g") != null) { + throw new NotExecutableException(); + } + + String xml = "" + + "" + + " rep:AuthorizableFolder" + + "rep:Group" + + " " + g1Id + "" + + " g1" + + " " +gId+ "" + + "" + + "rep:Group" + + " " + gId + "" + + " g1" + + " " +g1Id+ "" + + "" + + ""; + + /* + try to import 'g1' with 'g' as member and the 'g' group that has a circular group membership references with ABORT. + expected: + - group is imported + - circular membership is spotted latest upon save + */ + try { + doImport(getTargetPath(), xml); + getImportSession().save(); + + fail("Circular membership must be detected latest upon save."); + } catch (ConstraintViolationException e) { + // success } } } Index: oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java (revision 1788086) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java (revision ) @@ -18,7 +18,10 @@ import java.util.Set; +import javax.jcr.nodetype.ConstraintViolationException; + import com.google.common.collect.ImmutableSet; +import org.apache.jackrabbit.oak.api.CommitFailedException; import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters; import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; import org.apache.jackrabbit.oak.spi.xml.ImportBehavior; @@ -27,6 +30,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; public class AddMembersByIdIgnoreTest extends AbstractAddMembersByIdTest { \ No newline at end of file Index: oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java (revision 1788086) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java (revision ) @@ -23,9 +23,10 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.common.collect.Iterators; import org.apache.jackrabbit.api.security.user.Authorizable; import org.apache.jackrabbit.api.security.user.Group; -import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.api.security.user.UserManager; import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters; import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal; import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; @@ -38,7 +39,6 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; public class AddMembersByIdBestEffortTest extends AbstractAddMembersByIdTest { @@ -143,21 +143,43 @@ assertTrue(testGroup.isMember(memberGroup)); } + /** + * @since Oak 1.8 : for performance reasons cyclic membership is only check + * within GroupImpl and those import-behaviours that actually resolve the id to a user/group + */ + @Override @Test + public void testCyclicMembership() throws Exception { + memberGroup.addMember(testGroup); + root.commit(); + + Set failed = testGroup.addMembers(memberGroup.getID()); + assertTrue(failed.isEmpty()); + + root.commit(); + + // cyclic membership must be spotted upon membership resolution + assertEquals(1, Iterators.size(memberGroup.getMembers())); + assertEquals(1, Iterators.size(testGroup.getMembers())); + } + + /** + * @since Oak 1.8 : for performance reasons cyclic membership is only check + * within GroupImpl and those import-behaviours that actually resolve the id to a user/group + */ + @Test public void testCyclicWithoutAccess() throws Exception { memberGroup.addMember(testGroup); root.commit(); - try { - Set failed = addExistingMemberWithoutAccess(); + Set failed = addExistingMemberWithoutAccess(); - fail("CommitFailedException expected"); - } catch (CommitFailedException e) { - assertTrue(e.isConstraintViolation()); - assertEquals(31, e.getCode()); - } finally { + assertTrue(failed.isEmpty()); + + // cyclic membership must be spotted upon membership resolution - root.refresh(); + root.refresh(); - assertFalse(testGroup.isMember(memberGroup)); - } + UserManager uMgr = getUserManager(root); + assertEquals(1, Iterators.size(uMgr.getAuthorizable(memberGroup.getID(), Group.class).getMembers())); + assertEquals(1, Iterators.size(uMgr.getAuthorizable(testGroup.getID(), Group.class).getMembers())); } @Test \ No newline at end of file