You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by an...@apache.org on 2016/04/07 16:57:19 UTC
svn commit: r1738138 [1/2] - in /jackrabbit/oak/trunk: oak-commons/
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/identifier/
oak-core/src/main/java/org/apache/jackrabbit/oak/secur...
Author: angela
Date: Thu Apr 7 14:57:18 2016
New Revision: 1738138
URL: http://svn.apache.org/viewvc?rev=1738138&view=rev
Log:
OAK-4119 : potential improvements to membership management - Take 1
- improvements as listed in the issue except for changes wrt 'Group.isDeclaredMember' which didn't yet prove to be justified
- merging and resolving conflicts arising from OAK-4003 in GroupImpl and UserImporter. this also required minor adjustments to the corresponding test-cases
Added:
jackrabbit/oak/trunk/oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/QueryUtils.java
Modified:
jackrabbit/oak/trunk/oak-commons/pom.xml
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/identifier/IdentifierManager.java
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/MembershipWriter.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserImporter.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserInitializer.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserValidator.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/QueryUtil.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/AsyncIndexUpdateTest.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/AddMembersByIdBestEffortTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/MembershipProviderTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserInitializerTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserValidatorTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionBestEffortTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/spi/security/user/action/GroupActionTest.java
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AbstractEvaluationTest.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/GroupImportWithActionsBestEffortTest.java
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupImportWithActionsTest.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-commons/pom.xml
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-commons/pom.xml?rev=1738138&r1=1738137&r2=1738138&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-commons/pom.xml (original)
+++ jackrabbit/oak/trunk/oak-commons/pom.xml Thu Apr 7 14:57:18 2016
@@ -76,6 +76,11 @@
<artifactId>commons-io</artifactId>
<version>2.4</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.jackrabbit</groupId>
+ <artifactId>jackrabbit-jcr-commons</artifactId>
+ <version>${jackrabbit.version}</version>
+ </dependency>
<!-- Test dependencies -->
<dependency>
Added: jackrabbit/oak/trunk/oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/QueryUtils.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/QueryUtils.java?rev=1738138&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/QueryUtils.java (added)
+++ jackrabbit/oak/trunk/oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/QueryUtils.java Thu Apr 7 14:57:18 2016
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.commons;
+
+import javax.annotation.Nonnull;
+
+import org.apache.jackrabbit.util.Text;
+
+public final class QueryUtils {
+
+ private QueryUtils() {}
+
+ /**
+ * Escape {@code string} for matching in jcr escaped node names
+ *
+ * @param string string to escape
+ * @return escaped string
+ */
+ @Nonnull
+ public static String escapeNodeName(@Nonnull String string) {
+ StringBuilder result = new StringBuilder();
+
+ int k = 0;
+ int j;
+ do {
+ j = string.indexOf('%', k);
+ if (j < 0) {
+ // jcr escape trail
+ result.append(Text.escapeIllegalJcrChars(string.substring(k)));
+ } else if (j > 0 && string.charAt(j - 1) == '\\') {
+ // literal occurrence of % -> jcr escape
+ result.append(Text.escapeIllegalJcrChars(string.substring(k, j) + '%'));
+ } else {
+ // wildcard occurrence of % -> jcr escape all but %
+ result.append(Text.escapeIllegalJcrChars(string.substring(k, j))).append('%');
+ }
+
+ k = j + 1;
+ } while (j >= 0);
+
+ return result.toString();
+ }
+
+ @Nonnull
+ public static String escapeForQuery(@Nonnull String value) {
+ StringBuilder ret = new StringBuilder();
+ for (int i = 0; i < value.length(); i++) {
+ char c = value.charAt(i);
+ if (c == '\\') {
+ ret.append("\\\\");
+ } else if (c == '\'') {
+ ret.append("''");
+ } else {
+ ret.append(c);
+ }
+ }
+ return ret.toString();
+ }
+}
\ No newline at end of file
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/identifier/IdentifierManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/identifier/IdentifierManager.java?rev=1738138&r1=1738137&r2=1738138&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/identifier/IdentifierManager.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/identifier/IdentifierManager.java Thu Apr 7 14:57:18 2016
@@ -29,6 +29,8 @@ import javax.jcr.query.Query;
import com.google.common.base.Charsets;
import com.google.common.base.Function;
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Iterators;
import org.apache.jackrabbit.JcrConstants;
@@ -41,6 +43,7 @@ import org.apache.jackrabbit.oak.api.Roo
import org.apache.jackrabbit.oak.api.Tree;
import org.apache.jackrabbit.oak.api.Type;
import org.apache.jackrabbit.oak.commons.PathUtils;
+import org.apache.jackrabbit.oak.commons.QueryUtils;
import org.apache.jackrabbit.oak.namepath.NamePathMapper;
import org.apache.jackrabbit.oak.plugins.memory.StringPropertyState;
import org.apache.jackrabbit.oak.plugins.nodetype.ReadOnlyNodeTypeManager;
@@ -270,7 +273,7 @@ public class IdentifierManager {
}
// skip references from the version storage (OAK-1196)
- if (!rowPath.startsWith(VersionConstants.VERSION_STORE_PATH)) {
+ if (!rowPath.startsWith(VersionConstants.VERSION_STORE_PATH)) {
Tree tree = root.getTree(rowPath);
if (nodeTypeNames.length == 0 || containsNodeType(tree, nodeTypeNames)) {
if (propertyName == null) {
@@ -299,6 +302,58 @@ public class IdentifierManager {
};
}
+ /**
+ * Searches all reference properties to the specified {@code tree} that match
+ * the given {@code propertyName} and the specified, mandatory node type
+ * constraint ({@code ntName}). In contrast to {@link #getReferences} this
+ * method requires all parameters to be specified, which eases the handling
+ * of the result set and doesn't require the trees associated with the
+ * result set to be resolved.
+ *
+ * @param tree The tree for which references should be searched.
+ * @param propertyName The name of the reference properties.
+ * @param ntName The node type name to be used for the query.
+ * @param weak if {@code true} only weak references are returned. Otherwise on hard references are returned.
+ * @return A set of oak paths of those reference properties referring to the
+ * specified {@code tree} and matching the constraints.
+ */
+ @Nonnull
+ public Iterable<String> getReferences(@Nonnull Tree tree, final @Nonnull String propertyName,
+ @Nonnull String ntName, boolean weak) {
+ if (!nodeTypeManager.isNodeType(tree, JcrConstants.MIX_REFERENCEABLE)) {
+ return Collections.emptySet(); // shortcut
+ }
+
+ final String uuid = getIdentifier(tree);
+ String reference = weak ? PropertyType.TYPENAME_WEAKREFERENCE : PropertyType.TYPENAME_REFERENCE;
+ Map<String, ? extends PropertyValue> bindings = Collections.singletonMap("uuid", PropertyValues.newString(uuid));
+
+ try {
+ String escapedPropName = QueryUtils.escapeForQuery(propertyName);
+ Result result = root.getQueryEngine().executeQuery(
+ "SELECT * FROM [" + ntName + "] WHERE PROPERTY([" + escapedPropName + "], '" + reference + "') = $uuid" +
+ QueryEngine.INTERNAL_SQL2_QUERY,
+ Query.JCR_SQL2, bindings, NO_MAPPINGS);
+
+ Iterable<String> resultPaths = Iterables.transform(result.getRows(), new Function<ResultRow, String>() {
+ @Override
+ public String apply(ResultRow row) {
+ return PathUtils.concat(row.getPath(), propertyName);
+ }
+ });
+ return Iterables.filter(resultPaths, new Predicate<String>() {
+ @Override
+ public boolean apply(String path) {
+ return !path.startsWith(VersionConstants.VERSION_STORE_PATH);
+ }
+ }
+ );
+ } catch (ParseException e) {
+ log.error("query failed", e);
+ return Collections.emptySet();
+ }
+ }
+
@CheckForNull
public String resolveUUID(String uuid) {
return resolveUUID(StringPropertyState.stringProperty("", uuid));
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=1738138&r1=1738137&r2=1738138&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 Thu Apr 7 14:57:18 2016
@@ -18,6 +18,7 @@ package org.apache.jackrabbit.oak.securi
import java.security.Principal;
import java.util.Iterator;
+import java.util.Map;
import java.util.Set;
import javax.annotation.Nonnull;
@@ -29,6 +30,7 @@ import com.google.common.base.Predicate;
import com.google.common.base.Strings;
import com.google.common.collect.Iterators;
import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import org.apache.jackrabbit.api.security.user.Authorizable;
import org.apache.jackrabbit.api.security.user.Group;
@@ -111,10 +113,7 @@ class GroupImpl extends AuthorizableImpl
log.debug(msg);
return false;
}
- if (isCyclicMembership(authorizableImpl)) {
- log.warn("Attempt to create circular group membership.");
- return false;
- }
+ // NOTE: detection of circular membership is postponed to the commit (=> UserValidator)
}
boolean success = getMembershipProvider().addMember(getTree(), authorizableImpl.getTree());
@@ -131,26 +130,6 @@ class GroupImpl extends AuthorizableImpl
return updateMembers(false, memberIds);
}
- /**
- * Returns {@code true} if the given {@code newMember} is a Group
- * and contains {@code this} Group as declared or inherited member.
- *
- * @param newMember The new member to be tested for cyclic membership.
- * @return true if the 'newMember' is a group and 'this' is an declared or
- * inherited member of it.
- */
- private boolean isCyclicMembership(AuthorizableImpl newMember) {
- if (newMember.isGroup()) {
- MembershipProvider mProvider = getMembershipProvider();
- String contentId = mProvider.getContentID(getTree());
- if (mProvider.isMember(newMember.getTree(), contentId, true)) {
- // found cyclic group membership
- return true;
- }
- }
- return false;
- }
-
@Override
public boolean removeMember(Authorizable authorizable) throws RepositoryException {
if (!isValidAuthorizableImpl(authorizable)) {
@@ -239,10 +218,16 @@ class GroupImpl extends AuthorizableImpl
return false;
} else if (isEveryone()) {
return true;
+ } else if (((AuthorizableImpl) authorizable).isEveryone()) {
+ return false;
} else {
Tree authorizableTree = ((AuthorizableImpl) authorizable).getTree();
MembershipProvider mgr = getUserManager().getMembershipProvider();
- return mgr.isMember(this.getTree(), authorizableTree, includeInherited);
+ if (includeInherited) {
+ return mgr.isMember(this.getTree(), authorizableTree);
+ } else {
+ return mgr.isDeclaredMember(this.getTree(), authorizableTree);
+ }
}
}
@@ -258,11 +243,17 @@ class GroupImpl extends AuthorizableImpl
* authorizable.
* @throws javax.jcr.RepositoryException If another error occurs.
*/
- private final Set<String> updateMembers(boolean isRemove, @Nonnull String... memberIds) throws RepositoryException {
+ private Set<String> updateMembers(boolean isRemove, @Nonnull String... memberIds) throws RepositoryException {
Set<String> idSet = Sets.newLinkedHashSet(Lists.newArrayList(memberIds));
- Set<String> processedIds = Sets.newLinkedHashSet();
int importBehavior = UserUtil.getImportBehavior(getUserManager().getConfig());
+ if (isEveryone()) {
+ String msg = "Attempt to add or remove from everyone group.";
+ log.debug(msg);
+ return idSet;
+ }
+
+ Map<String, String> updateMap = Maps.newHashMapWithExpectedSize(idSet.size());
Iterator<String> idIterator = idSet.iterator();
while (idIterator.hasNext()) {
String memberId = idIterator.next();
@@ -285,24 +276,29 @@ class GroupImpl extends AuthorizableImpl
log.debug(msg);
continue;
}
+ } else if (member.isGroup() && ((AuthorizableImpl) member).isEveryone()) {
+ log.debug("Attempt to add everyone group as member.");
+ continue;
}
}
- boolean success;
- String contentId = AuthorizableBaseProvider.getContentID(memberId);
+ idIterator.remove();
+ updateMap.put(AuthorizableBaseProvider.getContentID(memberId), memberId);
+ }
+
+ Set<String> processedIds = Sets.newHashSet(updateMap.values());
+ if (!updateMap.isEmpty()) {
+ Set<String> failedIds;
if (isRemove) {
- success = getMembershipProvider().removeMember(getTree(), contentId);
+ failedIds = getMembershipProvider().removeMembers(getTree(), updateMap);
} else {
- success = getMembershipProvider().addMember(getTree(), contentId);
- }
- if (success) {
- idIterator.remove();
- processedIds.add(memberId);
+ failedIds = getMembershipProvider().addMembers(getTree(), updateMap);
}
+ idSet.addAll(failedIds);
+ processedIds.removeAll(failedIds);
}
getUserManager().onGroupUpdate(this, isRemove, false, processedIds, idSet);
-
return idSet;
}
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=1738138&r1=1738137&r2=1738138&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 Thu Apr 7 14:57:18 2016
@@ -16,9 +16,13 @@
*/
package org.apache.jackrabbit.oak.security.user;
+import java.util.ArrayList;
import java.util.HashSet;
import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
import java.util.Set;
+import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import javax.jcr.RepositoryException;
@@ -111,7 +115,7 @@ class MembershipProvider extends Authori
* @param root the current root
* @param config the security configuration
*/
- MembershipProvider(Root root, ConfigurationParameters config) {
+ MembershipProvider(@Nonnull Root root, @Nonnull ConfigurationParameters config) {
super(root, config);
}
@@ -139,7 +143,7 @@ class MembershipProvider extends Authori
* @return an iterator over all membership paths.
*/
@Nonnull
- Iterator<String> getMembership(Tree authorizableTree, final boolean includeInherited) {
+ Iterator<String> getMembership(@Nonnull Tree authorizableTree, final boolean includeInherited) {
return getMembership(authorizableTree, includeInherited, new HashSet<String>());
}
@@ -152,18 +156,16 @@ class MembershipProvider extends Authori
* @return an iterator over all membership paths.
*/
@Nonnull
- private Iterator<String> getMembership(Tree authorizableTree, final boolean includeInherited,
- final Set<String> processedPaths) {
+ private Iterator<String> getMembership(@Nonnull Tree authorizableTree, final boolean includeInherited,
+ @Nonnull final Set<String> processedPaths) {
final Iterable<String> refPaths = identifierManager.getReferences(
- true, authorizableTree, REP_MEMBERS, NT_REP_MEMBER_REFERENCES
+ authorizableTree, REP_MEMBERS, NT_REP_MEMBER_REFERENCES, true
);
return new AbstractMemberIterator(refPaths.iterator()) {
@Override
- protected String internalGetNext() {
+ protected String internalGetNext(@Nonnull String propPath) {
String next = null;
- // get the next rep:members property path
- String propPath = references.next();
int index = propPath.indexOf('/' + REP_MEMBERS_LIST);
if (index < 0) {
index = propPath.indexOf('/' + REP_MEMBERS);
@@ -177,7 +179,7 @@ class MembershipProvider extends Authori
// inject a parent iterator of the inherited memberships is needed
Tree group = getByPath(groupPath);
if (UserUtil.isType(group, AuthorizableType.GROUP)) {
- parent = getMembership(group, true, processedPaths);
+ remember(group);
}
}
}
@@ -186,10 +188,28 @@ class MembershipProvider extends Authori
}
return next;
}
+
+ @Nonnull
+ @Override
+ protected Iterator<String> getNextIterator(@Nonnull Tree groupTree) {
+ return getMembership(groupTree, true, processedPaths);
+ }
};
}
/**
+ * Tests if the membership of the specified {@code authorizableTree}
+ * contains the given target group as defined by {@code groupTree}.
+ *
+ * @param authorizableTree The tree of the authorizable for which to resolve the membership.
+ * @param groupPath The path of the group which needs to be tested.
+ * @return {@code true} if the group is contained in the membership of the specified authorizable.
+ */
+ private boolean hasMembership(@Nonnull Tree authorizableTree, @Nonnull String groupPath) {
+ return Iterators.contains(getMembership(authorizableTree, true), groupPath);
+ }
+
+ /**
* Returns an iterator over all member paths of the given group.
*
* @param groupTree the group tree
@@ -214,11 +234,17 @@ class MembershipProvider extends Authori
@Nonnull
private Iterator<String> getMembers(@Nonnull final Tree groupTree, @Nonnull final AuthorizableType authorizableType,
final boolean includeInherited, @Nonnull final Set<String> processedRefs) {
+ MemberReferenceIterator mrit = new MemberReferenceIterator(groupTree) {
+ @Override
+ protected boolean hasProcessedReference(@Nonnull String value) {
+ return processedRefs.add(value);
+ }
+ };
+
+ return new AbstractMemberIterator(mrit) {
- return new AbstractMemberIterator(new MemberReferenceIterator(groupTree, processedRefs)) {
@Override
- protected String internalGetNext() {
- String value = references.next();
+ protected String internalGetNext(@Nonnull String value) {
String next = identifierManager.getPath(PropertyValues.newWeakReference(value));
// filter by authorizable type, and/or get inherited members
@@ -227,7 +253,7 @@ class MembershipProvider extends Authori
AuthorizableType type = (auth == null) ? null : UserUtil.getType(auth);
if (includeInherited && type == AuthorizableType.GROUP) {
- parent = getMembers(auth, authorizableType, true, processedRefs);
+ remember(auth);
}
if (authorizableType != AuthorizableType.AUTHORIZABLE && type != authorizableType) {
next = null;
@@ -235,6 +261,12 @@ class MembershipProvider extends Authori
}
return next;
}
+
+ @Nonnull
+ @Override
+ protected Iterator<String> getNextIterator(@Nonnull Tree groupTree) {
+ return getMembers(groupTree, authorizableType, true, processedRefs);
+ }
};
}
@@ -243,41 +275,61 @@ class MembershipProvider extends Authori
*
* @param groupTree The new member to be tested for cyclic membership.
* @param authorizableTree The authorizable to check
- * @param includeInherited {@code true} to also check inherited members
*
* @return true if the group has given member.
*/
- boolean isMember(Tree groupTree, Tree authorizableTree, boolean includeInherited) {
- return isMember(groupTree, getContentID(authorizableTree), includeInherited);
+ boolean isMember(@Nonnull Tree groupTree, @Nonnull Tree authorizableTree) {
+ if (!hasMembers(groupTree)) {
+ return false;
+ }
+ if (pendingChanges(groupTree)) {
+ return Iterators.contains(getMembers(groupTree, AuthorizableType.AUTHORIZABLE, true), authorizableTree.getPath());
+ } else {
+ return hasMembership(authorizableTree, groupTree.getPath());
+ }
+ }
+
+ boolean isDeclaredMember(@Nonnull Tree groupTree, @Nonnull Tree authorizableTree) {
+ if (!hasMembers(groupTree)) {
+ return false;
+ }
+
+ String contentId = getContentID(authorizableTree);
+ MemberReferenceIterator refs = new MemberReferenceIterator(groupTree) {
+ @Override
+ protected boolean hasProcessedReference(@Nonnull String value) {
+ return true;
+ }
+ };
+ return Iterators.contains(refs, contentId);
}
/**
- * Returns {@code true} if the given {@code groupTree} contains a member with the given {@code contentId}
- *
- * @param groupTree The new member to be tested for cyclic membership.
- * @param contentId The content ID of the group.
- * @param includeInherited {@code true} to also check inherited members
+ * Utility to determine if a given group has any members.
*
- * @return true if the group has given member.
+ * @param groupTree The tree of the target group.
+ * @return {@code true} if the group has any members i.e. if it has a rep:members
+ * property or a rep:membersList child node.
*/
- boolean isMember(Tree groupTree, String contentId, boolean includeInherited) {
- if (includeInherited) {
- Set<String> refs = new HashSet<String>();
- for (Iterator<String> it = getMembers(groupTree, AuthorizableType.AUTHORIZABLE, includeInherited, refs); it.hasNext();) {
- it.next();
- if (refs.contains(contentId)) {
- return true;
- }
- }
- } else {
- MemberReferenceIterator refs = new MemberReferenceIterator(groupTree, new HashSet<String>());
- while (refs.hasNext()) {
- if (contentId.equals(refs.next())) {
- return true;
- }
- }
- }
- return false;
+ private static boolean hasMembers(@Nonnull Tree groupTree) {
+ return groupTree.getPropertyStatus(REP_MEMBERS) != null || groupTree.hasChild(REP_MEMBERS_LIST);
+ }
+
+ /**
+ * Determine if the group has (potentially) been modified in which case the
+ * query can't be used:
+ * - rep:members property has been modified
+ * - any potential modification in the member-ref-list subtree, which is not
+ * easy to detect => relying on pending changes on the root object
+ *
+ * @param groupTree The tree of the target group.
+ * @return {@code true} if the specified group tree has an unmodified rep:members
+ * property or if the root has pending changes.
+ */
+ private boolean pendingChanges(@Nonnull Tree groupTree) {
+ Tree.Status memberPropStatus = groupTree.getPropertyStatus(REP_MEMBERS);
+ // rep:members is new or has been modified or root has pending changes
+ return Tree.Status.UNCHANGED != memberPropStatus || root.hasPendingChanges();
}
/**
@@ -287,19 +339,19 @@ class MembershipProvider extends Authori
* @return {@code true} if the member was added
* @throws RepositoryException if an error occurs
*/
- boolean addMember(Tree groupTree, Tree newMemberTree) throws RepositoryException {
+ boolean addMember(@Nonnull Tree groupTree, @Nonnull Tree newMemberTree) throws RepositoryException {
return writer.addMember(groupTree, getContentID(newMemberTree));
}
/**
- * Adds a new member to the given {@code groupTree}.
- * @param groupTree the group to add the member to
- * @param memberContentId the id of the new member
- * @return {@code true} if the member was added
- * @throws RepositoryException if an error occurs
+ * Add the members from the given group.
+ *
+ * @param groupTree group to add the new members
+ * @param memberIds Map of 'contentId':'memberId' of all members to be added.
+ * @return the set of member IDs that was not successfully processed.
*/
- boolean addMember(Tree groupTree, String memberContentId) throws RepositoryException {
- return writer.addMember(groupTree, memberContentId);
+ Set<String> addMembers(@Nonnull Tree groupTree, @Nonnull Map<String, String> memberIds) throws RepositoryException {
+ return writer.addMembers(groupTree, memberIds);
}
/**
@@ -309,7 +361,7 @@ class MembershipProvider extends Authori
* @param memberTree member to remove
* @return {@code true} if the member was removed.
*/
- boolean removeMember(Tree groupTree, Tree memberTree) {
+ boolean removeMember(@Nonnull Tree groupTree, @Nonnull Tree memberTree) {
if (writer.removeMember(groupTree, getContentID(memberTree))) {
return true;
} else {
@@ -319,29 +371,25 @@ class MembershipProvider extends Authori
}
/**
- * Removes the member with the specified contentID from the given group.
+ * Removes the members from the given group.
*
* @param groupTree group to remove the member from
- * @param memberContentId the contentID of the member to remove
- * @return {@code true} if the member was removed.
+ * @param memberIds Map of 'contentId':'memberId' of all members that need to be removed.
+ * @return the set of member IDs that was not successfully processed.
*/
- boolean removeMember(@Nonnull Tree groupTree, @Nonnull String memberContentId) {
- return writer.removeMember(groupTree, memberContentId);
+ Set<String> removeMembers(@Nonnull Tree groupTree, @Nonnull Map<String, String> memberIds) {
+ return writer.removeMembers(groupTree, memberIds);
}
/**
* Iterator that provides member references based on the rep:members properties of a underlying tree iterator.
*/
- private static final class MemberReferenceIterator extends AbstractLazyIterator<String> {
-
- private final Set<String> processedRefs;
+ private abstract class MemberReferenceIterator extends AbstractLazyIterator<String> {
private final Iterator<Tree> trees;
-
private Iterator<String> propertyValues;
- private MemberReferenceIterator(@Nonnull Tree groupTree, @Nonnull Set<String> processedRefs) {
- this.processedRefs = processedRefs;
+ private MemberReferenceIterator(@Nonnull Tree groupTree) {
this.trees = Iterators.concat(
Iterators.singletonIterator(groupTree),
groupTree.getChild(REP_MEMBERS_LIST).getChildren().iterator()
@@ -352,7 +400,6 @@ class MembershipProvider extends Authori
protected String getNext() {
String next = null;
while (next == null) {
-
if (propertyValues == null) {
// check if there are more trees that can provide a rep:members property
if (!trees.hasNext()) {
@@ -368,19 +415,22 @@ class MembershipProvider extends Authori
propertyValues = null;
} else {
String value = propertyValues.next();
- if (processedRefs.add(value)) {
+ if (hasProcessedReference(value)) {
next = value;
}
}
}
return next;
}
+
+ protected abstract boolean hasProcessedReference(@Nonnull String value);
}
private abstract class AbstractMemberIterator extends AbstractLazyIterator<String> {
- protected Iterator<String> references;
- protected Iterator<String> parent;
+ private Iterator<String> references;
+ private List<Tree> groupTrees;
+ private Iterator<String> parent;
AbstractMemberIterator(@Nonnull Iterator<String> references) {
this.references = references;
@@ -390,23 +440,62 @@ class MembershipProvider extends Authori
protected String getNext() {
String next = null;
while (next == null) {
- // process parent iterators first
- if (parent != null) {
+ if (references.hasNext()) {
+ next = internalGetNext(references.next());
+ } else if (parent != null) {
if (parent.hasNext()) {
next = parent.next();
} else {
+ // force retrieval of next parent iterator
parent = null;
}
- } else if (!references.hasNext()) {
- // if there are no more values left, reset the iterator
- break;
} else {
- next = internalGetNext();
+ // try to retrieve the next 'parent' iterator for the first
+ // group tree remembered in the list.
+ if (groupTrees == null || groupTrees.isEmpty()) {
+ // no more parents to process => reset the iterator.
+ break;
+ } else {
+ parent = getNextIterator(groupTrees.remove(0));
+ }
}
}
return next;
}
- protected abstract String internalGetNext();
+ /**
+ * Remember a group that needs to be search for references ('parent')
+ * once all 'references' have been processed.
+ *
+ * @param groupTree A tree associated with a group
+ * @see #getNextIterator(Tree)
+ */
+ protected void remember(@Nonnull Tree groupTree) {
+ if (groupTrees == null) {
+ groupTrees = new ArrayList<Tree>();
+ }
+ groupTrees.add(groupTree);
+ }
+
+ /**
+ * Abstract method to obtain the next authorizable path from the given
+ * reference value.
+ *
+ * @param nextReference The next reference as obtained from the iterator.
+ * @return The path of the authorizable identified by {@code nextReference}
+ * or {@code null} if it cannot be resolved.
+ */
+ @CheckForNull
+ protected abstract String internalGetNext(@Nonnull String nextReference);
+
+ /**
+ * Abstract method to retrieve the next member iterator for the given
+ * {@code groupTree}.
+ *
+ * @param groupTree Tree referring to a group.
+ * @return The next member reference 'parent' iterator to be processed.
+ */
+ @Nonnull
+ protected abstract Iterator<String> getNextIterator(@Nonnull Tree groupTree);
}
}
\ No newline at end of file
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipWriter.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipWriter.java?rev=1738138&r1=1738137&r2=1738138&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipWriter.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/MembershipWriter.java Thu Apr 7 14:57:18 2016
@@ -16,11 +16,16 @@
*/
package org.apache.jackrabbit.oak.security.user;
+import java.util.HashSet;
import java.util.Iterator;
+import java.util.Map;
import java.util.Set;
+import javax.annotation.Nonnull;
import javax.jcr.RepositoryException;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
import org.apache.jackrabbit.JcrConstants;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.api.Tree;
@@ -60,23 +65,46 @@ public class MembershipWriter {
* @throws RepositoryException if an error occurs
*/
boolean addMember(Tree groupTree, String memberContentId) throws RepositoryException {
+ Map<String, String> m = Maps.newHashMapWithExpectedSize(1);
+ m.put(memberContentId, "-");
+ return addMembers(groupTree, m).isEmpty();
+ }
+
+ /**
+ * Adds a new member to the given {@code groupTree}.
+ *
+ * @param groupTree the group to add the member to
+ * @param memberIds the ids of the new members as map of 'contentId':'memberId'
+ * @return the set of member IDs that was not successfully processed.
+ * @throws RepositoryException if an error occurs
+ */
+ Set<String> addMembers(@Nonnull Tree groupTree, @Nonnull Map<String, String> memberIds) throws RepositoryException {
// check all possible rep:members properties for the new member and also find the one with the least values
Tree membersList = groupTree.getChild(UserConstants.REP_MEMBERS_LIST);
Iterator<Tree> trees = Iterators.concat(
Iterators.singletonIterator(groupTree),
membersList.getChildren().iterator()
);
+
+ Set<String> failed = new HashSet<String>(memberIds.size());
int bestCount = membershipSizeThreshold;
PropertyState bestProperty = null;
Tree bestTree = null;
- while (trees.hasNext()) {
+
+ // remove existing memberIds from the map and find best-matching tree
+ // for the insertion of the new members.
+ while (trees.hasNext() && !memberIds.isEmpty()) {
Tree t = trees.next();
PropertyState refs = t.getProperty(UserConstants.REP_MEMBERS);
if (refs != null) {
int numRefs = 0;
for (String ref : refs.getValue(Type.WEAKREFERENCES)) {
- if (ref.equals(memberContentId)) {
- return false;
+ String id = memberIds.remove(ref);
+ if (id != null) {
+ failed.add(id);
+ if (memberIds.isEmpty()) {
+ break;
+ }
}
numRefs++;
}
@@ -88,35 +116,73 @@ public class MembershipWriter {
}
}
- PropertyBuilder<String> propertyBuilder;
- if (bestProperty == null) {
- // we don't have a good candidate to store the new member.
- // so there are no members at all or all are full
- if (!groupTree.hasProperty(UserConstants.REP_MEMBERS)) {
- bestTree = groupTree;
- } else {
- if (!membersList.exists()) {
- membersList = groupTree.addChild(UserConstants.REP_MEMBERS_LIST);
- membersList.setProperty(JcrConstants.JCR_PRIMARYTYPE, UserConstants.NT_REP_MEMBER_REFERENCES_LIST, NAME);
- bestTree = membersList.addChild("0");
+ // update member content structure by starting inserting new member IDs
+ // with the best-matching property and create new member-ref-nodes as needed.
+ if (!memberIds.isEmpty()) {
+ PropertyBuilder<String> propertyBuilder;
+ int propCnt;
+ if (bestProperty == null) {
+ // we don't have a good candidate to store the new members.
+ // so there are no members at all or all are full
+ if (!groupTree.hasProperty(UserConstants.REP_MEMBERS)) {
+ bestTree = groupTree;
} else {
- // keep node names linear
- int i = 0;
- String name = String.valueOf(i);
- while (membersList.hasChild(name)) {
- name = String.valueOf(++i);
+ bestTree = createMemberRefTree(groupTree, membersList);
+ }
+ propertyBuilder = PropertyBuilder.array(Type.WEAKREFERENCE, UserConstants.REP_MEMBERS);
+ propCnt = 0;
+ } else {
+ propertyBuilder = PropertyBuilder.copy(Type.WEAKREFERENCE, bestProperty);
+ propCnt = bestCount;
+ }
+ // if adding all new members to best-property would exceed the threshold
+ // the new ids need to be distributed to different member-ref-nodes
+ // for simplicity this is achieved by introducing new tree(s)
+ if ((propCnt + memberIds.size()) > membershipSizeThreshold) {
+ while (!memberIds.isEmpty()) {
+ Set s = new HashSet();
+ Iterator it = memberIds.keySet().iterator();
+ while (propCnt < membershipSizeThreshold && it.hasNext()) {
+ s.add(it.next());
+ it.remove();
+ propCnt++;
+ }
+ propertyBuilder.addValues(s);
+ bestTree.setProperty(propertyBuilder.getPropertyState());
+
+ if (it.hasNext()) {
+ // continue filling the next (new) node + propertyBuilder pair
+ propCnt = 0;
+ bestTree = createMemberRefTree(groupTree, membersList);
+ propertyBuilder = PropertyBuilder.array(Type.WEAKREFERENCE, UserConstants.REP_MEMBERS);
}
- bestTree = membersList.addChild(name);
}
- bestTree.setProperty(JcrConstants.JCR_PRIMARYTYPE, UserConstants.NT_REP_MEMBER_REFERENCES, NAME);
+ } else {
+ propertyBuilder.addValues(memberIds.keySet());
+ bestTree.setProperty(propertyBuilder.getPropertyState());
}
- propertyBuilder = PropertyBuilder.array(Type.WEAKREFERENCE, UserConstants.REP_MEMBERS);
- } else {
- propertyBuilder = PropertyBuilder.copy(Type.WEAKREFERENCE, bestProperty);
}
- propertyBuilder.addValue(memberContentId);
- bestTree.setProperty(propertyBuilder.getPropertyState());
- return true;
+ return failed;
+ }
+
+ private static Tree createMemberRefTree(@Nonnull Tree groupTree, @Nonnull Tree membersList) {
+ if (!membersList.exists()) {
+ membersList = groupTree.addChild(UserConstants.REP_MEMBERS_LIST);
+ membersList.setProperty(JcrConstants.JCR_PRIMARYTYPE, UserConstants.NT_REP_MEMBER_REFERENCES_LIST, NAME);
+ }
+ Tree refTree = membersList.addChild(nextRefNodeName(membersList));
+ refTree.setProperty(JcrConstants.JCR_PRIMARYTYPE, UserConstants.NT_REP_MEMBER_REFERENCES, NAME);
+ return refTree;
+ }
+
+ private static String nextRefNodeName(@Nonnull Tree membersList) {
+ // keep node names linear
+ int i = 0;
+ String name = String.valueOf(i);
+ while (membersList.hasChild(name)) {
+ name = String.valueOf(++i);
+ }
+ return name;
}
/**
@@ -126,33 +192,50 @@ public class MembershipWriter {
* @param memberContentId member to remove
* @return {@code true} if the member was removed.
*/
- boolean removeMember(Tree groupTree, String memberContentId) {
+ boolean removeMember(@Nonnull Tree groupTree, @Nonnull String memberContentId) {
+ Map<String, String> m = Maps.newHashMapWithExpectedSize(1);
+ m.put(memberContentId, "-");
+ return removeMembers(groupTree, m).isEmpty();
+ }
+
+ /**
+ * Removes the members from the given group.
+ *
+ * @param groupTree group to remove the member from
+ * @param memberIds Map of 'contentId':'memberId' of all members that need to be removed.
+ * @return the set of member IDs that was not successfully processed.
+ */
+ Set<String> removeMembers(@Nonnull Tree groupTree, @Nonnull Map<String, String> memberIds) {
Tree membersList = groupTree.getChild(UserConstants.REP_MEMBERS_LIST);
Iterator<Tree> trees = Iterators.concat(
Iterators.singletonIterator(groupTree),
membersList.getChildren().iterator()
);
- while (trees.hasNext()) {
+ while (trees.hasNext() && !memberIds.isEmpty()) {
Tree t = trees.next();
PropertyState refs = t.getProperty(UserConstants.REP_MEMBERS);
if (refs != null) {
PropertyBuilder<String> prop = PropertyBuilder.copy(Type.WEAKREFERENCE, refs);
- if (prop.hasValue(memberContentId)) {
- prop.removeValue(memberContentId);
- if (prop.isEmpty()) {
- if (t == groupTree) {
- t.removeProperty(UserConstants.REP_MEMBERS);
+ Iterator<Map.Entry<String,String>> memberEntries = memberIds.entrySet().iterator();
+ while (memberEntries.hasNext()) {
+ String memberContentId = memberEntries.next().getKey();
+ if (prop.hasValue(memberContentId)) {
+ prop.removeValue(memberContentId);
+ if (prop.isEmpty()) {
+ if (t == groupTree) {
+ t.removeProperty(UserConstants.REP_MEMBERS);
+ } else {
+ t.remove();
+ }
} else {
- t.remove();
+ t.setProperty(prop.getPropertyState());
}
- } else {
- t.setProperty(prop.getPropertyState());
+ memberEntries.remove();
}
- return true;
}
}
}
- return false;
+ return Sets.newHashSet(memberIds.values());
}
/**
@@ -161,7 +244,7 @@ public class MembershipWriter {
* @param group node builder of group
* @param members set of content ids to set
*/
- public void setMembers(NodeBuilder group, Set<String> members) {
+ public void setMembers(@Nonnull NodeBuilder group, @Nonnull Set<String> members) {
group.removeProperty(UserConstants.REP_MEMBERS);
if (group.hasChildNode(UserConstants.REP_MEMBERS)) {
group.getChildNode(UserConstants.REP_MEMBERS).remove();
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserImporter.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserImporter.java?rev=1738138&r1=1738137&r2=1738138&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserImporter.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserImporter.java Thu Apr 7 14:57:18 2016
@@ -35,6 +35,8 @@ import javax.jcr.Session;
import javax.jcr.nodetype.ConstraintViolationException;
import javax.jcr.nodetype.PropertyDefinition;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import org.apache.jackrabbit.api.JackrabbitSession;
import org.apache.jackrabbit.api.security.principal.PrincipalIterator;
@@ -594,8 +596,8 @@ class UserImporter implements ProtectedP
toRemove.put(dm.getID(), dm);
}
- List<Authorizable> toAdd = new ArrayList<Authorizable>();
- Set<String> nonExisting = new HashSet<String>();
+ Map<String, Authorizable> toAdd = Maps.newHashMapWithExpectedSize(members.size());
+ Map<String, String> nonExisting = Maps.newHashMap();
for (String contentId : members) {
String remapped = referenceTracker.get(contentId);
@@ -611,27 +613,29 @@ class UserImporter implements ProtectedP
}
if (member != null) {
if (toRemove.remove(member.getID()) == null) {
- toAdd.add(member);
+ toAdd.put(member.getID(), member);
} // else: no need to remove from rep:members
} else {
handleFailure("New member of " + gr + ": No such authorizable (NodeID = " + memberContentId + ')');
if (importBehavior == ImportBehavior.BESTEFFORT) {
log.info("ImportBehavior.BESTEFFORT: Remember non-existing member for processing.");
- nonExisting.add(contentId);
+ /* since we ignore the set of failed ids later on and
+ don't know the real memberId => use fake memberId as
+ value in the map */
+ nonExisting.put(contentId, "-");
}
}
}
// 2. adjust members of the group
- for (Authorizable m : toRemove.values()) {
- if (!gr.removeMember(m)) {
- handleFailure("Failed remove existing member (" + m + ") from " + gr);
- }
+ if (!toRemove.isEmpty()) {
+ Set<String> failed = gr.removeMembers(toRemove.keySet().toArray(new String[toRemove.size()]));
+ handleFailure("Failed removing members " + Iterables.toString(failed) + " to " + gr);
}
- for (Authorizable m : toAdd) {
- if (!gr.addMember(m)) {
- handleFailure("Failed add member (" + m + ") to " + gr);
- }
+
+ if (!toAdd.isEmpty()) {
+ Set<String> failed = gr.addMembers(toAdd.keySet().toArray(new String[toAdd.size()]));
+ handleFailure("Failed add members " + Iterables.toString(failed) + " to " + gr);
}
// handling non-existing members in case of best-effort
@@ -640,16 +644,11 @@ class UserImporter implements ProtectedP
Tree groupTree = root.getTree(gr.getPath());
MembershipProvider membershipProvider = userManager.getMembershipProvider();
- Set<String> memberContentIds = Sets.newLinkedHashSet();
- Set<String> failedContentIds = Sets.newLinkedHashSet();
- for (String member : nonExisting) {
- boolean success = membershipProvider.addMember(groupTree, member);
- if (success) {
- memberContentIds.add(member);
- } else {
- failedContentIds.add(member);
- }
- }
+
+ Set<String> memberContentIds = Sets.newHashSet(nonExisting.keySet());
+ Set<String> failedContentIds = membershipProvider.addMembers(groupTree, nonExisting);
+ memberContentIds.removeAll(failedContentIds);
+
userManager.onGroupUpdate(gr, false, true, memberContentIds, failedContentIds);
}
}
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserInitializer.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserInitializer.java?rev=1738138&r1=1738137&r2=1738138&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserInitializer.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserInitializer.java Thu Apr 7 14:57:18 2016
@@ -124,6 +124,13 @@ class UserInitializer implements Workspa
"to enforce uniqueness of rep:principalName property values, " +
"and to quickly search a principal by name if it was constructed manually.");
}
+ if (!index.hasChild("repMembers")) {
+ NodeUtil members = IndexUtils.createIndexDefinition(index, "repMembers", false,
+ new String[]{REP_MEMBERS},
+ new String[]{NT_REP_MEMBER_REFERENCES});
+ members.setString("info",
+ "Oak index used by the user management to lookup group membership.");
+ }
ConfigurationParameters params = userConfiguration.getParameters();
String adminId = params.getConfigValue(PARAM_ADMIN_ID, DEFAULT_ADMIN_ID);
@@ -149,5 +156,4 @@ class UserInitializer implements Workspa
NodeState target = store.getRoot();
target.compareAgainstBaseState(base, new ApplyDiff(builder));
}
-
}
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=1738138&r1=1738137&r2=1738138&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 Thu Apr 7 14:57:18 2016
@@ -114,9 +114,9 @@ class UserValidator extends DefaultValid
}
if (REP_MEMBERS.equals(name)) {
- Set<String> afterValues = Sets.newHashSet(after.getValue(Type.STRINGS));
- afterValues.removeAll(ImmutableSet.copyOf(before.getValue(Type.STRINGS)));
- checkForCyclicMembership(afterValues);
+ Set<String> addedValues = Sets.newHashSet(after.getValue(Type.STRINGS));
+ addedValues.removeAll(ImmutableSet.copyOf(before.getValue(Type.STRINGS)));
+ checkForCyclicMembership(addedValues);
}
}
@@ -191,7 +191,7 @@ class UserValidator extends DefaultValid
MembershipProvider mp = provider.getMembershipProvider();
for (String memberContentId : memberRefs) {
Tree memberTree = mp.getByContentID(memberContentId, AuthorizableType.GROUP);
- if (memberTree != null && mp.isMember(memberTree, groupContentId, true)) {
+ if (memberTree != null && mp.isMember(memberTree, parentAfter)) {
throw constraintViolation(31, "Cyclic group membership detected in group" + UserUtil.getAuthorizableId(parentAfter));
}
}
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/QueryUtil.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/QueryUtil.java?rev=1738138&r1=1738137&r2=1738138&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/QueryUtil.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/QueryUtil.java Thu Apr 7 14:57:18 2016
@@ -22,12 +22,12 @@ import javax.jcr.RepositoryException;
import javax.jcr.Value;
import org.apache.jackrabbit.api.security.user.QueryBuilder;
+import org.apache.jackrabbit.oak.commons.QueryUtils;
import org.apache.jackrabbit.oak.namepath.NamePathMapper;
import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters;
import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
import org.apache.jackrabbit.oak.spi.security.user.UserConstants;
import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
-import org.apache.jackrabbit.util.Text;
/**
* Common utilities used for user/group queries.
@@ -84,27 +84,7 @@ public final class QueryUtil {
*/
@Nonnull
public static String escapeNodeName(@Nonnull String string) {
- StringBuilder result = new StringBuilder();
-
- int k = 0;
- int j;
- do {
- j = string.indexOf('%', k);
- if (j < 0) {
- // jcr escape trail
- result.append(Text.escapeIllegalJcrChars(string.substring(k)));
- } else if (j > 0 && string.charAt(j - 1) == '\\') {
- // literal occurrence of % -> jcr escape
- result.append(Text.escapeIllegalJcrChars(string.substring(k, j) + '%'));
- } else {
- // wildcard occurrence of % -> jcr escape all but %
- result.append(Text.escapeIllegalJcrChars(string.substring(k, j))).append('%');
- }
-
- k = j + 1;
- } while (j >= 0);
-
- return result.toString();
+ return QueryUtils.escapeNodeName(string);
}
@Nonnull
@@ -133,18 +113,7 @@ public final class QueryUtil {
@Nonnull
public static String escapeForQuery(@Nonnull String value) {
- StringBuilder ret = new StringBuilder();
- for (int i = 0; i < value.length(); i++) {
- char c = value.charAt(i);
- if (c == '\\') {
- ret.append("\\\\");
- } else if (c == '\'') {
- ret.append("''");
- } else {
- ret.append(c);
- }
- }
- return ret.toString();
+ return QueryUtils.escapeForQuery(value);
}
@Nonnull
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/AsyncIndexUpdateTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/AsyncIndexUpdateTest.java?rev=1738138&r1=1738137&r2=1738138&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/AsyncIndexUpdateTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/AsyncIndexUpdateTest.java Thu Apr 7 14:57:18 2016
@@ -27,7 +27,6 @@ import static org.hamcrest.CoreMatchers.
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
-import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
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=1738138&r1=1738137&r2=1738138&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 Thu Apr 7 14:57:18 2016
@@ -16,6 +16,7 @@
*/
package org.apache.jackrabbit.oak.security.user;
+import java.util.Iterator;
import java.util.Set;
import java.util.UUID;
import javax.annotation.Nonnull;
@@ -36,12 +37,14 @@ import org.apache.jackrabbit.oak.api.Pro
import org.apache.jackrabbit.oak.api.Root;
import org.apache.jackrabbit.oak.api.Tree;
import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants;
import org.apache.jackrabbit.oak.spi.security.user.UserConstants;
import org.junit.Test;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
@@ -185,7 +188,8 @@ public abstract class AbstractAddMembers
} catch (ConstraintViolationException e) {
// expected
}
- assertTrue(root.hasPendingChanges());
+ // no modifications expected as testing for empty id is done before changes are made
+ assertFalse(root.hasPendingChanges());
}
@Test(expected = NullPointerException.class)
@@ -205,7 +209,8 @@ public abstract class AbstractAddMembers
} catch (ConstraintViolationException e) {
// expected
}
- assertTrue(root.hasPendingChanges());
+ // no modifications expected as testing for null id is done before changes are made
+ assertFalse(root.hasPendingChanges());
}
@Test
@@ -252,4 +257,38 @@ public abstract class AbstractAddMembers
Set<String> failed = testGroup.addMembers(getTestUser().getID(), memberGroup.getID());
assertEquals(2, failed.size());
}
+
+ @Test
+ public void testEveryoneAsMember() throws Exception {
+ UserManagerImpl userManager = (UserManagerImpl) getUserManager(root);
+ Group everyone = userManager.createGroup(EveryonePrincipal.getInstance());
+ try {
+ Set<String> failed = testGroup.addMembers(everyone.getID());
+ assertFalse(failed.isEmpty());
+ assertTrue(failed.contains(everyone.getID()));
+ root.commit();
+
+ assertFalse(testGroup.isDeclaredMember(everyone));
+ assertFalse(testGroup.isMember(everyone));
+ for (Iterator<Group> it = everyone.memberOf(); it.hasNext(); ) {
+ assertNotEquals(testGroup.getID(), it.next().getID());
+ }
+ for (Iterator<Group> it = everyone.declaredMemberOf(); it.hasNext(); ) {
+ assertNotEquals(testGroup.getID(), it.next().getID());
+ }
+
+ boolean found = false;
+ MembershipProvider mp = userManager.getMembershipProvider();
+ for (Iterator<String> it = mp.getMembership(root.getTree(everyone.getPath()), true); it.hasNext(); ) {
+ String p = it.next();
+ if (testGroup.getPath().equals(p)) {
+ found = true;
+ }
+ }
+ assertFalse(found);
+ } finally {
+ everyone.remove();
+ root.commit();
+ }
+ }
}
\ 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=1738138&r1=1738137&r2=1738138&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 Thu Apr 7 14:57:18 2016
@@ -24,8 +24,10 @@ import java.util.Set;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
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.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;
@@ -33,6 +35,7 @@ import org.junit.Test;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@@ -61,6 +64,44 @@ public class AddMembersByIdBestEffortTes
);
}
+ /**
+ * "Oddity" when adding per id + besteffort: everyone group will not be
+ * dealt with separately and will end up being listed in a rep:members property.
+ */
+ @Test
+ public void testEveryoneAsMember() throws Exception {
+ UserManagerImpl userManager = (UserManagerImpl) getUserManager(root);
+ Group everyone = userManager.createGroup(EveryonePrincipal.getInstance());
+ try {
+ Set<String> failed = testGroup.addMembers(everyone.getID());
+ assertTrue(failed.isEmpty());
+ root.commit();
+
+ assertFalse(testGroup.isDeclaredMember(everyone));
+ assertFalse(testGroup.isMember(everyone));
+ for (Iterator<Group> it = everyone.memberOf(); it.hasNext(); ) {
+ assertNotEquals(testGroup.getID(), it.next().getID());
+ }
+ for (Iterator<Group> it = everyone.declaredMemberOf(); it.hasNext(); ) {
+ assertNotEquals(testGroup.getID(), it.next().getID());
+ }
+
+ // oddity of the current impl that add members without testing for
+ boolean found = false;
+ MembershipProvider mp = userManager.getMembershipProvider();
+ for (Iterator<String> it = mp.getMembership(root.getTree(everyone.getPath()), true); it.hasNext(); ) {
+ String p = it.next();
+ if (testGroup.getPath().equals(p)) {
+ found = true;
+ }
+ }
+ assertTrue(found);
+ } finally {
+ everyone.remove();
+ root.commit();
+ }
+ }
+
@Test
public void testNonExistingMember() throws Exception {
Set<String> failed = addNonExistingMember();
@@ -115,6 +156,22 @@ public class AddMembersByIdBestEffortTes
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.addMembers("member2");
+ assertFalse(failed.isEmpty());
+
+ } finally {
+ mp.setMembershipSizeThreshold(100); // back to default
+ }
}
}
\ No newline at end of file