Author: angela
Date: Wed Apr 26 08:02:40 2017
New Revision: 1792709
URL: http://svn.apache.org/viewvc?rev=1792709&view=rev
Log:
OAK-6072 : Move check for cyclic membership to GroupImpl
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserValidator.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java
jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/default.md
jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/membership.md
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportAbortTest.java
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportBestEffortTest.java
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportIgnoreTest.java
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.java
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/NestedGroupTest.java
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupImpl.java
Wed Apr 26 08:02:40 2017
@@ -111,7 +111,10 @@ class GroupImpl extends AuthorizableImpl
log.debug(msg);
return false;
}
- // NOTE: detection of circular membership is postponed to the
commit (=> UserValidator)
+ if (isCyclicMembership((Group) authorizable)) {
+ String msg = "Cyclic group membership detected for group " +
getID() + " and member " + authorizable.getID();
+ throw new ConstraintViolationException(msg);
+ }
}
boolean success = getMembershipProvider().addMember(getTree(),
authorizableImpl.getTree());
@@ -266,18 +269,25 @@ class GroupImpl extends AuthorizableImpl
if (ImportBehavior.BESTEFFORT != importBehavior) {
Authorizable member =
getUserManager().getAuthorizable(memberId);
+ String msg = null;
if (member == null) {
+ msg = "Attempt to add or remove a non-existing member '" +
memberId + "' with ImportBehavior = " +
ImportBehavior.nameFromValue(importBehavior);
+ } else if (member.isGroup()) {
+ if (((AuthorizableImpl) member).isEveryone()) {
+ log.debug("Attempt to add everyone group as member.");
+ continue;
+ } else if (isCyclicMembership((Group) member)) {
+ msg = "Cyclic group membership detected for group " +
getID() + " and member " + member.getID();
+ }
+ }
+ if (msg != null) {
if (ImportBehavior.ABORT == importBehavior) {
- throw new ConstraintViolationException("Attempt to add
or remove a non-existing member " + memberId);
+ throw new ConstraintViolationException(msg);
} else {
// ImportBehavior.IGNORE is default in
UserUtil.getImportBehavior
- String msg = "Attempt to add or remove non-existing
member '" + getID() + "' with ImportBehavior = IGNORE.";
log.debug(msg);
continue;
}
- } else if (member.isGroup() && ((AuthorizableImpl)
member).isEveryone()) {
- log.debug("Attempt to add everyone group as member.");
- continue;
}
}
@@ -302,6 +312,10 @@ class GroupImpl extends AuthorizableImpl
return failedIds;
}
+ private boolean isCyclicMembership(@Nonnull Group member) throws
RepositoryException {
+ return member.isMember(this);
+ }
+
/**
* Principal representation of this group instance.
*/
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipProvider.java
Wed Apr 26 08:02:40 2017
@@ -220,7 +220,7 @@ class MembershipProvider extends Authori
*/
@Nonnull
Iterator<String> getMembers(@Nonnull Tree groupTree, boolean
includeInherited) {
- return getMembers(groupTree, includeInherited, new HashSet<String>());
+ return getMembers(groupTree, getContentID(groupTree),
includeInherited, new HashSet<String>());
}
/**
@@ -233,11 +233,16 @@ class MembershipProvider extends Authori
*/
@Nonnull
private Iterator<String> getMembers(@Nonnull final Tree groupTree,
+ @Nonnull final String groupContentId,
final boolean includeInherited,
@Nonnull final Set<String>
processedRefs) {
MemberReferenceIterator mrit = new MemberReferenceIterator(groupTree) {
@Override
protected boolean hasProcessedReference(@Nonnull String value) {
+ if (groupContentId.equals(value)) {
+ log.warn("Cyclic group membership detected for contentId "
+ groupContentId);
+ return false;
+ }
return processedRefs.add(value);
}
};
@@ -261,7 +266,7 @@ class MembershipProvider extends Authori
@Nonnull
@Override
protected Iterator<String> getNextIterator(@Nonnull Tree
groupTree) {
- return getMembers(groupTree, true, processedRefs);
+ return getMembers(groupTree, groupContentId, true,
processedRefs);
}
};
}
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserValidator.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserValidator.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserValidator.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserValidator.java
Wed Apr 26 08:02:40 2017
@@ -16,12 +16,9 @@
*/
package org.apache.jackrabbit.oak.security.user;
-import java.util.Set;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Sets;
import org.apache.jackrabbit.JcrConstants;
import org.apache.jackrabbit.oak.api.CommitFailedException;
import org.apache.jackrabbit.oak.api.PropertyState;
@@ -80,10 +77,6 @@ class UserValidator extends DefaultValid
String msg = "Invalid jcr:uuid for authorizable " +
parentAfter.getName();
throw constraintViolation(21, msg);
}
-
- if (REP_MEMBERS.equals(name)) {
- checkForCyclicMembership(after.getValue(Type.STRINGS));
- }
}
@Override
@@ -112,12 +105,6 @@ class UserValidator extends DefaultValid
String msg = "Password may not be plain text.";
throw constraintViolation(24, msg);
}
-
- if (REP_MEMBERS.equals(name)) {
- Set<String> addedValues =
Sets.newHashSet(after.getValue(Type.STRINGS));
-
addedValues.removeAll(ImmutableSet.copyOf(before.getValue(Type.STRINGS)));
- checkForCyclicMembership(addedValues);
- }
}
@@ -183,20 +170,6 @@ class UserValidator extends DefaultValid
}
}
- private void checkForCyclicMembership(@Nonnull Iterable<String>
memberRefs) throws CommitFailedException {
- String groupContentId = TreeUtil.getString(parentAfter,
JcrConstants.JCR_UUID);
- if (groupContentId == null) {
- throw constraintViolation(30, "Missing content id for group " +
UserUtil.getAuthorizableId(parentAfter) + "; cannot check for cyclic group
membership.");
- }
- MembershipProvider mp = provider.getMembershipProvider();
- for (String memberContentId : memberRefs) {
- Tree memberTree = mp.getByContentID(memberContentId,
AuthorizableType.GROUP);
- if (memberTree != null && mp.isMember(memberTree, parentAfter)) {
- throw constraintViolation(31, "Cyclic group membership
detected in group" + UserUtil.getAuthorizableId(parentAfter));
- }
- }
- }
-
private void validateAuthorizable(@Nonnull Tree tree, @Nullable
AuthorizableType type) throws CommitFailedException {
boolean isSystemUser = (type == AuthorizableType.USER) &&
UserUtil.isSystemUser(tree);
String authRoot =
UserUtil.getAuthorizableRootPath(provider.getConfig(), type);
Modified:
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java
Wed Apr 26 08:02:40 2017
@@ -31,7 +31,6 @@ import org.apache.jackrabbit.api.securit
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.assertNot
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 @@ public abstract class AbstractAddMembers
root.commit();
Set<String> 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
Modified:
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java
Wed Apr 26 08:02:40 2017
@@ -16,14 +16,20 @@
*/
package org.apache.jackrabbit.oak.security.user;
+import java.util.Set;
import javax.jcr.nodetype.ConstraintViolationException;
+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;
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
@@ -43,4 +49,23 @@ public class AddMembersByIdAbortTest ext
public void testExistingMemberWithoutAccess() throws Exception {
addExistingMemberWithoutAccess();
}
+
+ @Test
+ public void testCyclicMembership() throws Exception {
+ memberGroup.addMember(testGroup);
+ root.commit();
+
+ try {
+ Set<String> 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
+ }
+ }
}
\ No newline at end of file
Modified:
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java
Wed Apr 26 08:02:40 2017
@@ -23,9 +23,10 @@ import java.util.Set;
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.assertEqu
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 @@ public class AddMembersByIdBestEffortTes
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<String> 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<String> failed = addExistingMemberWithoutAccess();
- fail("CommitFailedException expected");
- } catch (CommitFailedException e) {
- assertTrue(e.isConstraintViolation());
- assertEquals(31, e.getCode());
- } finally {
- root.refresh();
- assertFalse(testGroup.isMember(memberGroup));
- }
+ Set<String> failed = addExistingMemberWithoutAccess();
+ assertTrue(failed.isEmpty());
+
+ // cyclic membership must be spotted upon membership resolution
+ root.refresh();
+ 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
Modified:
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java
Wed Apr 26 08:02:40 2017
@@ -18,7 +18,10 @@ package org.apache.jackrabbit.oak.securi
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 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;
public class AddMembersByIdIgnoreTest extends AbstractAddMembersByIdTest {
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=1792709&r1=1792708&r2=1792709&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
Wed Apr 26 08:02:40 2017
@@ -301,76 +301,6 @@ public class UserValidatorTest extends A
}
}
-
- /**
- * @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<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);
- 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());
Modified:
jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/default.md
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/default.md?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/default.md
(original)
+++ jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/default.md Wed
Apr 26 08:02:40 2017
@@ -220,7 +220,7 @@ The corresponding errors are all of type
| 0028 | Attempt to create outside of configured scope
|
| 0029 | Intermediate folders not rep:AuthorizableFolder
|
| 0030 | Missing uuid for group (check for cyclic membership)
|
-| 0031 | Cyclic group membership
|
+| <s>0031</s> | <s>Cyclic group membership</s> (see [OAK-6072])
|
| 0032 | Attempt to set password with system user
|
| 0033 | Attempt to add rep:pwd node to a system user
|
@@ -327,3 +327,4 @@ implementation.
[UserConfiguration]:
/oak/docs/apidocs/org/apache/jackrabbit/oak/spi/security/user/UserConfiguration.html
[UserAuthenticationFactory]:
/oak/docs/apidocs/org/apache/jackrabbit/oak/spi/security/user/UserAuthenticationFactory.html
[Authentication]:
/oak/docs/apidocs/org/apache/jackrabbit/oak/spi/security/authentication/Authentication.html
+[OAK-6072]: https://issues.apache.org/jira/browse/OAK-6072
Modified:
jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/membership.md
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/membership.md?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/membership.md
(original)
+++ jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/membership.md
Wed Apr 26 08:02:40 2017
@@ -148,11 +148,11 @@ group membership by specifying the membe
The following details are worth mentioning:
- a `null` or empty String id will immediately fail the operation with
`ConstraintViolationException`; changes already made will not be reverted,
-- an attemt to make the same group member of itself will list that id in the
return value but will not fail the operation,
+- an attempt to make the same group member of itself will list that id in the
return value but will not fail the operation,
- duplicate ids in the parameter list will be silently ignored,
-- cyclic membership validation is postponed to the validator called upon
`Root.commit`
+- <s>cyclic membership validation is postponed to the validator called upon
`Root.commit`
and will only fail at that point; the cyclic membership then needs to be
manually
- resolved by the application,
+ resolved by the application</s> (see [OAK-3170] and below)
- whether or not a non-existing (or not accessible) authorizable can be added
or
removed depends on the configured `ImportBehavior`:
- ABORT: each id is resolved to the corresponding authorizable; if it
doesn't
@@ -164,6 +164,41 @@ The following details are worth mentioni
- IGNORE: each id is resolved to the corresponding authorizable; if it
doesn't
exist it will be returned as _failed_ in the return value.
+#### Invalid Membership
+
+##### Invalid Authorizable
+
+Adding a different implementation of `Authorizable` is not allowed. This is
always
+verified when calling `Group.addMember(Authorizable)`.
+
+##### Same Group as Member
+
+Adding the target group as member of itself will not succeed. When adding
+members by ID (`Group.addMembers(String...)`) the violation is spotted by
+simple ID comparison.
+
+##### Everyone Group and Everyone as Member
+
+The group representing the `EveryonePrincipal` is specially handled. Due to
+it's dynamic nature adding members to this group is not allowed and adding it
+as a member to any other group would cause a cyclic membership.
+
+Note however, that this validation is omitted in case of
`Group.addMembers(String...)`
+with `ImportBehavior.BESTEFFORT` (see above).
+
+##### Cyclic Membership
+
+Since Oak 1.7.0 the explicit check for cyclic group membership has been
+moved from the `Validator` to the `Group` implementation. As before cyclic
+membership might not be spotted and the membership resolution will log the
+cycle upon collection of all members/groups.
+
+The following scenarios may leave the cycle unnoticed upon adding members:
+- `Group.addMember(Authorizable)` when the editing `Session` cannot read all
groups included in the cycle.
+- `Group.addMembers(String...)` with `ImportBehavior.BESTEFFORT` where the
member ID is not resolved.
+
+See [OAK-3170] for additional information.
+
### Configuration
Note that as of Oak 1.0 the implementation is responsible for defining the
@@ -171,7 +206,6 @@ content structure and will expand the mu
Consequently, the following configuration option `groupMembershipSplitSize`
present
with Jackrabbit 2.x is not supported anymore.
-
<!-- hidden references -->
[org.apache.jackrabbit.api.security.user.Group]:
http://svn.apache.org/repos/asf/jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Group.java
[org.apache.jackrabbit.api.security.user.Authorizable]:
http://svn.apache.org/repos/asf/jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Authorizable.java
Modified:
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportAbortTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportAbortTest.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportAbortTest.java
(original)
+++
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportAbortTest.java
Wed Apr 26 08:02:40 2017
@@ -20,10 +20,15 @@ import java.util.ArrayList;
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;
/**
@@ -88,4 +93,43 @@ public class GroupImportAbortTest extend
// 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 = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" +
+ "<sv:node sv:name=\"gFolder\"
xmlns:mix=\"http://www.jcp.org/jcr/mix/1.0\";
xmlns:nt=\"http://www.jcp.org/jcr/nt/1.0\";
xmlns:fn_old=\"http://www.w3.org/2004/10/xpath-functions\";
xmlns:fn=\"http://www.w3.org/2005/xpath-functions\";
xmlns:xs=\"http://www.w3.org/2001/XMLSchema\";
xmlns:sv=\"http://www.jcp.org/jcr/sv/1.0\"; xmlns:rep=\"internal\"
xmlns:jcr=\"http://www.jcp.org/jcr/1.0\";>" +
+ " <sv:property sv:name=\"jcr:primaryType\"
sv:type=\"Name\"><sv:value>rep:AuthorizableFolder</sv:value></sv:property>" +
+ "<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>" + g1Id + "</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:type=\"WeakReference\"><sv:value>" +gId+ "</sv:value></sv:property>" +
+ "</sv:node>" +
+ "<sv:node sv:name=\"g\"><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>" + gId + "</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:type=\"WeakReference\"><sv:value>" +g1Id+ "</sv:value></sv:property>" +
+ "</sv:node>" +
+ "</sv:node>";
+
+ /*
+ 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
+ }
+ }
}
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=1792709&r1=1792708&r2=1792709&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
Wed Apr 26 08:02:40 2017
@@ -23,12 +23,11 @@ import javax.jcr.Node;
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 @@ public class GroupImportBestEffortTest e
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 @@ public class GroupImportBestEffortTest e
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
Modified:
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportIgnoreTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportIgnoreTest.java?rev=1792709&r1=1792708&r2=1792709&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportIgnoreTest.java
(original)
+++
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportIgnoreTest.java
Wed Apr 26 08:02:40 2017
@@ -20,11 +20,16 @@ import java.util.ArrayList;
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;
/**
@@ -95,4 +100,47 @@ public class GroupImportIgnoreTest exten
}
}
}
+
+ @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 = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" +
+ "<sv:node sv:name=\"gFolder\"
xmlns:mix=\"http://www.jcp.org/jcr/mix/1.0\";
xmlns:nt=\"http://www.jcp.org/jcr/nt/1.0\";
xmlns:fn_old=\"http://www.w3.org/2004/10/xpath-functions\";
xmlns:fn=\"http://www.w3.org/2005/xpath-functions\";
xmlns:xs=\"http://www.w3.org/2001/XMLSchema\";
xmlns:sv=\"http://www.jcp.org/jcr/sv/1.0\"; xmlns:rep=\"internal\"
xmlns:jcr=\"http://www.jcp.org/jcr/1.0\";>" +
+ " <sv:property sv:name=\"jcr:primaryType\"
sv:type=\"Name\"><sv:value>rep:AuthorizableFolder</sv:value></sv:property>" +
+ "<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>" + g1Id + "</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:type=\"WeakReference\"><sv:value>" +gId+ "</sv:value></sv:property>" +
+ "</sv:node>" +
+ "<sv:node sv:name=\"g\"><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>" + gId + "</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:type=\"WeakReference\"><sv:value>" +g1Id+ "</sv:value></sv:property>" +
+ "</sv:node>" +
+ "</sv:node>";
+
+ /*
+ 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));
+ }
}
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=1792709&r1=1792708&r2=1792709&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
Wed Apr 26 08:02:40 2017
@@ -644,8 +644,10 @@ public class GroupTest extends AbstractU
private static void assertCyclicCommitFailed(RepositoryException e) {
Throwable th = e.getCause();
- assertTrue(th instanceof CommitFailedException);
- assertEquals(31, ((CommitFailedException) th).getCode());
+ if (th != null) {
+ assertTrue(th instanceof CommitFailedException);
+ assertEquals(31, ((CommitFailedException) th).getCode());
+ }
}
@Test
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=1792709&r1=1792708&r2=1792709&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
Wed Apr 26 08:02:40 2017
@@ -159,10 +159,12 @@ 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());
+ if (th != null) {
+ assertTrue(th instanceof CommitFailedException);
+ CommitFailedException ce = (CommitFailedException) th;
+ assertEquals(CommitFailedException.CONSTRAINT, ce.getType());
+ assertEquals(31, ce.getCode());
+ }
}
@Test
