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 2015/08/04 14:48:51 UTC

svn commit: r1694049 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/autosave/ oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user...

Author: angela
Date: Tue Aug  4 12:48:50 2015
New Revision: 1694049

URL: http://svn.apache.org/r1694049
Log:
OAK-3170 : Implement Group extensions as proposed in JCR-3880

Added:
    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/AbstractRemoveMembersByIdTest.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/RemoveMembersByIdAbortTest.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/RemoveMembersByIdIgnoreTest.java
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/UserImporter.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/autosave/GroupImpl.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/package-info.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/autosave/AutoSaveEnabledManagerTest.java
    jackrabbit/oak/trunk/oak-doc/src/site/markdown/security/user/membership.md
    jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/GroupDelegator.java
    jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/GroupTest.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=1694049&r1=1694048&r2=1694049&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 Tue Aug  4 12:48:50 2015
@@ -18,12 +18,17 @@ package org.apache.jackrabbit.oak.securi
 
 import java.security.Principal;
 import java.util.Iterator;
+import java.util.Set;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.jcr.RepositoryException;
+import javax.jcr.nodetype.ConstraintViolationException;
 
 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.Sets;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
 import org.apache.jackrabbit.api.security.user.UserManager;
@@ -31,6 +36,7 @@ import org.apache.jackrabbit.commons.ite
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
 import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.oak.spi.xml.ImportBehavior;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -113,6 +119,11 @@ class GroupImpl extends AuthorizableImpl
         return getMembershipProvider().addMember(getTree(), authorizableImpl.getTree());
     }
 
+    @Override
+    public Set<String> addMembers(@Nonnull String... memberIds) throws RepositoryException {
+        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.
@@ -147,6 +158,11 @@ class GroupImpl extends AuthorizableImpl
         }
     }
 
+    @Override
+    public Set<String> removeMembers(@Nonnull String... memberIds) throws RepositoryException {
+        return updateMembers(true, memberIds);
+    }
+
     //--------------------------------------------------------------------------
     /**
      * Internal implementation of {@link #getDeclaredMembers()} and {@link #getMembers()}.
@@ -217,6 +233,61 @@ class GroupImpl extends AuthorizableImpl
     }
 
     /**
+     * Internal method to add or remove members by ID.
+     *
+     * @param isRemove Boolean flag indicating if members should be added or removed.
+     * @param memberIds The {@code memberIds} to be added or removed.
+     * @return The sub-set of {@code memberIds} that could not be added/removed.
+     * @throws javax.jcr.nodetype.ConstraintViolationException If any of the specified
+     * IDs is empty string or null or if {@link org.apache.jackrabbit.oak.spi.xml.ImportBehavior#ABORT}
+     * is configured and an ID cannot be resolved to an existing (or accessible)
+     * authorizable.
+     * @throws javax.jcr.RepositoryException If another error occurs.
+     */
+    private final Set<String> updateMembers(boolean isRemove, @Nonnull String... memberIds) throws RepositoryException {
+        Set<String> idSet = Sets.newLinkedHashSet(Lists.newArrayList(memberIds));
+        int importBehavior = UserUtil.getImportBehavior(getUserManager().getConfig());
+
+        Iterator<String> idIterator = idSet.iterator();
+        while (idIterator.hasNext()) {
+            String memberId = idIterator.next();
+            if (Strings.isNullOrEmpty(memberId)) {
+                throw new ConstraintViolationException("MemberId must not be null or empty.");
+            }
+            if (getID().equals(memberId)) {
+                String msg = "Attempt to add or remove a group as member of itself (" + getID() + ").";
+                log.debug(msg);
+                continue;
+            }
+
+            if (ImportBehavior.BESTEFFORT != importBehavior) {
+                Authorizable member = getUserManager().getAuthorizable(memberId);
+                if (member == null) {
+                    if (ImportBehavior.ABORT == importBehavior) {
+                        throw new ConstraintViolationException("Attempt to add or remove a non-existing member " + memberId);
+                    } else if (ImportBehavior.IGNORE == importBehavior) {
+                        String msg = "Attempt to add or remove non-existing member '" + getID() + "' with ImportBehavior = IGNORE.";
+                        log.debug(msg);
+                        continue;
+                    }
+                }
+            }
+
+            boolean success;
+            String contentId = AuthorizableBaseProvider.getContentID(memberId);
+            if (isRemove) {
+                success = getMembershipProvider().removeMember(getTree(), contentId);
+            } else {
+                success = getMembershipProvider().addMember(getTree(), contentId);
+            }
+            if (success) {
+                idIterator.remove();
+            }
+        }
+        return idSet;
+    }
+
+    /**
      * Principal representation of this group instance.
      */
     private final class GroupPrincipal extends AbstractGroupPrincipal {

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=1694049&r1=1694048&r2=1694049&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 Tue Aug  4 12:48:50 2015
@@ -358,6 +358,17 @@ class MembershipProvider extends Authori
     }
 
     /**
+     * Removes the member with the specified contentID 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.
+     */
+    boolean removeMember(@Nonnull Tree groupTree, @Nonnull String memberContentId) {
+        return writer.removeMember(groupTree, memberContentId);
+    }
+
+    /**
      * Iterator that provides member references based on the rep:members properties of a underlying tree iterator.
      */
     private static final class MemberReferenceIterator extends AbstractLazyIterator<String> {

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=1694049&r1=1694048&r2=1694049&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 Tue Aug  4 12:48:50 2015
@@ -55,6 +55,7 @@ import org.apache.jackrabbit.oak.spi.sec
 import org.apache.jackrabbit.oak.spi.security.SecurityProvider;
 import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
 import org.apache.jackrabbit.oak.spi.security.user.UserConstants;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
 import org.apache.jackrabbit.oak.spi.xml.ImportBehavior;
 import org.apache.jackrabbit.oak.spi.xml.NodeInfo;
 import org.apache.jackrabbit.oak.spi.xml.PropInfo;
@@ -161,8 +162,7 @@ class UserImporter implements ProtectedP
     private Map<String, Principal> principals = new HashMap<String, Principal>();
 
     UserImporter(ConfigurationParameters config) {
-        String importBehaviorStr = config.getConfigValue(PARAM_IMPORT_BEHAVIOR, ImportBehavior.NAME_IGNORE);
-        importBehavior = ImportBehavior.valueFromString(importBehaviorStr);
+        importBehavior = UserUtil.getImportBehavior(config);
     }
 
     //----------------------------------------------< ProtectedItemImporter >---

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/autosave/GroupImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/autosave/GroupImpl.java?rev=1694049&r1=1694048&r2=1694049&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/autosave/GroupImpl.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/autosave/GroupImpl.java Tue Aug  4 12:48:50 2015
@@ -17,6 +17,8 @@
 package org.apache.jackrabbit.oak.security.user.autosave;
 
 import java.util.Iterator;
+import java.util.Set;
+import javax.annotation.Nonnull;
 import javax.jcr.RepositoryException;
 
 import org.apache.jackrabbit.api.security.user.Authorizable;
@@ -74,6 +76,15 @@ class GroupImpl extends AuthorizableImpl
     }
 
     @Override
+    public Set<String> addMembers(@Nonnull String... memberIds) throws RepositoryException {
+        try {
+            return getDelegate().addMembers(memberIds);
+        } finally {
+            getMgr().autosave();
+        }
+    }
+
+    @Override
     public boolean removeMember(Authorizable authorizable) throws RepositoryException {
         try {
             if (isValid(authorizable)) {
@@ -84,6 +95,15 @@ class GroupImpl extends AuthorizableImpl
         } finally {
             getMgr().autosave();
         }
+    }
+
+    @Override
+    public Set<String> removeMembers(@Nonnull String... memberIds) throws RepositoryException {
+        try {
+            return getDelegate().removeMembers(memberIds);
+        } finally {
+            getMgr().autosave();
+        }
     }
 
     private boolean isValid(Authorizable a) {

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java?rev=1694049&r1=1694048&r2=1694049&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java Tue Aug  4 12:48:50 2015
@@ -27,6 +27,8 @@ import org.apache.jackrabbit.oak.api.Tre
 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.xml.ImportBehavior;
+import org.apache.jackrabbit.oak.spi.xml.ProtectedItemImporter;
 import org.apache.jackrabbit.oak.util.TreeUtil;
 import org.apache.jackrabbit.util.Text;
 
@@ -163,4 +165,21 @@ public final class UserUtil implements U
             throw new AuthorizableTypeException("Invalid authorizable type '" + ((authorizableClass == null) ? "null" : authorizableClass) + '\'');
         }
     }
+
+    /**
+     * Return the configured {@link org.apache.jackrabbit.oak.spi.xml.ImportBehavior}
+     * for the given {@code config}. The default behavior in case
+     * {@link org.apache.jackrabbit.oak.spi.xml.ProtectedItemImporter#PARAM_IMPORT_BEHAVIOR}
+     * is not contained in the {@code config} object is
+     * {@link org.apache.jackrabbit.oak.spi.xml.ImportBehavior#IGNORE}
+     *
+     * @param config The configuration parameters.
+     * @return The import behavior as defined by {@link org.apache.jackrabbit.oak.spi.xml.ProtectedItemImporter#PARAM_IMPORT_BEHAVIOR}
+     * or {@link org.apache.jackrabbit.oak.spi.xml.ImportBehavior#IGNORE} if this
+     * config parameter is missing.
+     */
+    public static int getImportBehavior(@Nonnull ConfigurationParameters config) {
+        String importBehaviorStr = config.getConfigValue(ProtectedItemImporter.PARAM_IMPORT_BEHAVIOR, ImportBehavior.NAME_IGNORE);
+        return ImportBehavior.valueFromString(importBehaviorStr);
+    }
 }
\ No newline at end of file

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/package-info.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/package-info.java?rev=1694049&r1=1694048&r2=1694049&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/package-info.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/package-info.java Tue Aug  4 12:48:50 2015
@@ -14,7 +14,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-@Version("1.1.0")
+@Version("1.2.0")
 @Export(optional = "provide:=true")
 package org.apache.jackrabbit.oak.spi.security.user.util;
 

Added: 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=1694049&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractAddMembersByIdTest.java Tue Aug  4 12:48:50 2015
@@ -0,0 +1,255 @@
+/*
+ * 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.security.user;
+
+import java.util.Set;
+import java.util.UUID;
+import javax.annotation.Nonnull;
+import javax.jcr.RepositoryException;
+import javax.jcr.SimpleCredentials;
+import javax.jcr.nodetype.ConstraintViolationException;
+import javax.jcr.security.AccessControlManager;
+
+import com.google.common.collect.Iterables;
+import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
+import org.apache.jackrabbit.api.security.user.Group;
+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;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.Type;
+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.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 {
+
+    static final String[] NON_EXISTING_IDS = new String[] {"nonExisting1", "nonExisting2"};
+
+    Group testGroup;
+    Group memberGroup;
+
+    @Override
+    public void before() throws Exception {
+        super.before();
+
+        UserManager uMgr = getUserManager(root);
+        for (String id : NON_EXISTING_IDS) {
+            assertNull(uMgr.getAuthorizable(id));
+        }
+
+        testGroup = uMgr.createGroup("testGroup" + UUID.randomUUID().toString());
+        memberGroup = uMgr.createGroup("memberGroup" + UUID.randomUUID().toString());
+        root.commit();
+    }
+
+    @Override
+    public void after() throws Exception {
+        try {
+            root.refresh();
+
+            if (testGroup != null) {
+                testGroup.remove();
+            }
+            if (memberGroup != null) {
+                memberGroup.remove();
+            }
+            if (root.hasPendingChanges()) {
+                root.commit();
+            }
+        } finally {
+            super.after();
+        }
+    }
+
+    @Nonnull
+    Iterable<String> getMemberIds(@Nonnull Group group) throws RepositoryException {
+        // test group tree
+        Tree groupTree = root.getTree(group.getPath());
+        PropertyState membersProp = groupTree.getProperty(UserConstants.REP_MEMBERS);
+        assertNotNull(membersProp);
+        return membersProp.getValue(Type.WEAKREFERENCES);
+    }
+
+    @Nonnull
+    Set<String> addNonExistingMember() throws Exception {
+        return testGroup.addMembers(NON_EXISTING_IDS);
+    }
+
+    Set<String> addExistingMemberWithoutAccess() throws Exception {
+        AccessControlManager acMgr = getAccessControlManager(root);
+        JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, testGroup.getPath());
+        if (acl != null) {
+            if (acl.addEntry(getTestUser().getPrincipal(), privilegesFromNames(PrivilegeConstants.JCR_READ, PrivilegeConstants.REP_USER_MANAGEMENT), true)) {
+                acMgr.setPolicy(testGroup.getPath(), acl);
+                root.commit();
+            }
+        }
+
+        String userId = getTestUser().getID();
+        ContentSession testSession = null;
+        try {
+            testSession = login(new SimpleCredentials(userId, userId.toCharArray()));
+            Root testRoot = testSession.getLatestRoot();
+
+            assertFalse(testRoot.getTree(memberGroup.getPath()).exists());
+
+            Group gr = getUserManager(testRoot).getAuthorizable(testGroup.getID(), Group.class);
+            Set<String> failed = gr.addMembers(memberGroup.getID());
+            testRoot.commit();
+            return failed;
+        } finally {
+            if (testSession != null) {
+                testSession.close();
+            }
+        }
+    }
+
+    @Test
+    public void testGroupMembers() throws Exception {
+        Set<String> failed = testGroup.addMembers(memberGroup.getID());
+        assertTrue(failed.isEmpty());
+
+        root.commit();
+
+        assertTrue(testGroup.isDeclaredMember(memberGroup));
+    }
+
+    @Test
+    public void testUserMembers() throws Exception {
+        Set<String> failed = testGroup.addMembers(getTestUser().getID());
+        assertTrue(failed.isEmpty());
+
+        root.commit();
+
+        assertTrue(testGroup.isDeclaredMember(getTestUser()));
+    }
+
+    @Test
+    public void testMixedMembers() throws Exception {
+        Set<String> failed = testGroup.addMembers(getTestUser().getID(), memberGroup.getID());
+        assertTrue(failed.isEmpty());
+
+        root.commit();
+
+        assertTrue(testGroup.isDeclaredMember(getTestUser()));
+        assertTrue(testGroup.isDeclaredMember(memberGroup));
+    }
+
+    @Test
+    public void testNoMembers() throws Exception {
+        Set<String> failed = testGroup.addMembers();
+        assertTrue(failed.isEmpty());
+    }
+
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testEmptyId() throws Exception {
+        testGroup.addMembers("");
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testValidAndEmptyId() throws Exception {
+        testGroup.addMembers(getTestUser().getID(), "");
+    }
+
+    @Test
+    public void testValidAndEmptyIdModifiesRoot() throws Exception {
+        try {
+            testGroup.addMembers(getTestUser().getID(), "");
+        } catch (ConstraintViolationException e) {
+            // expected
+        }
+        assertTrue(root.hasPendingChanges());
+    }
+
+    @Test(expected = NullPointerException.class)
+    public void testNullId() throws Exception {
+        testGroup.addMembers(null);
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testValidAndNullId() throws Exception {
+        testGroup.addMembers(getTestUser().getID(), null);
+    }
+
+    @Test
+    public void testValidAndNullIdModifiesRoot() throws Exception {
+        try {
+            testGroup.addMembers(getTestUser().getID(), null);
+        } catch (ConstraintViolationException e) {
+            // expected
+        }
+        assertTrue(root.hasPendingChanges());
+    }
+
+    @Test
+    public void testSameId() throws Exception {
+        Set<String> failed = testGroup.addMembers(testGroup.getID());
+        assertEquals(1, failed.size());
+        assertTrue(failed.contains(testGroup.getID()));
+    }
+
+    @Test
+    public void testTwice() throws Exception {
+        Set<String> failed = testGroup.addMembers(memberGroup.getID(), memberGroup.getID());
+        assertTrue(failed.isEmpty());
+
+        assertTrue(testGroup.isDeclaredMember(memberGroup));
+
+        Iterable<String> memberIds = getMemberIds(testGroup);
+        assertEquals(1, Iterables.size(memberIds));
+    }
+
+    @Test
+    public void testCyclicMembership() throws Exception {
+        memberGroup.addMember(testGroup);
+        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());
+        }
+    }
+
+    @Test
+    public void testAlreadyMember() throws Exception {
+        testGroup.addMember(getTestUser());
+        testGroup.addMember(memberGroup);
+
+        Set<String> failed = testGroup.addMembers(getTestUser().getID(), memberGroup.getID());
+        assertEquals(2, failed.size());
+    }
+}
\ No newline at end of file

Added: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractRemoveMembersByIdTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractRemoveMembersByIdTest.java?rev=1694049&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractRemoveMembersByIdTest.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AbstractRemoveMembersByIdTest.java Tue Aug  4 12:48:50 2015
@@ -0,0 +1,178 @@
+/*
+ * 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.security.user;
+
+import java.util.Set;
+import java.util.UUID;
+
+import javax.jcr.SimpleCredentials;
+import javax.jcr.nodetype.ConstraintViolationException;
+import javax.jcr.security.AccessControlManager;
+
+import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
+import org.apache.jackrabbit.api.security.user.Group;
+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.ContentSession;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+public abstract class AbstractRemoveMembersByIdTest extends AbstractSecurityTest {
+
+    static final String[] NON_EXISTING_IDS = new String[] {"nonExisting1", "nonExisting2"};
+
+    Group testGroup;
+    Group memberGroup;
+
+    @Override
+    public void before() throws Exception {
+        super.before();
+
+        UserManager uMgr = getUserManager(root);
+        for (String id : NON_EXISTING_IDS) {
+            assertNull(uMgr.getAuthorizable(id));
+        }
+
+        testGroup = uMgr.createGroup("testGroup" + UUID.randomUUID().toString());
+        memberGroup = uMgr.createGroup("memberGroup" + UUID.randomUUID().toString());
+        testGroup.addMember(memberGroup);
+        testGroup.addMember(getTestUser());
+        root.commit();
+    }
+
+    @Override
+    public void after() throws Exception {
+        try {
+            root.refresh();
+
+            if (testGroup != null) {
+                testGroup.remove();
+            }
+            if (memberGroup != null) {
+                memberGroup.remove();
+            }
+            if (root.hasPendingChanges()) {
+                root.commit();
+            }
+        } finally {
+            super.after();
+        }
+    }
+
+    Set<String> removeNonExistingMember() throws Exception {
+        return testGroup.removeMembers(NON_EXISTING_IDS);
+    }
+
+    Set<String> removeExistingMemberWithoutAccess() throws Exception {
+        AccessControlManager acMgr = getAccessControlManager(root);
+        JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, testGroup.getPath());
+        if (acl != null) {
+            if (acl.addEntry(getTestUser().getPrincipal(), privilegesFromNames(PrivilegeConstants.JCR_READ, PrivilegeConstants.REP_USER_MANAGEMENT), true)) {
+                acMgr.setPolicy(testGroup.getPath(), acl);
+                root.commit();
+            }
+        }
+
+        String userId = getTestUser().getID();
+        ContentSession testSession = null;
+        try {
+            testSession = login(new SimpleCredentials(userId, userId.toCharArray()));
+            Root testRoot = testSession.getLatestRoot();
+
+            assertFalse(testRoot.getTree(memberGroup.getPath()).exists());
+
+            Group gr = getUserManager(testRoot).getAuthorizable(testGroup.getID(), Group.class);
+            Set<String> failed = gr.removeMembers(memberGroup.getID());
+            testRoot.commit();
+            return failed;
+        } finally {
+            if (testSession != null) {
+                testSession.close();
+            }
+        }
+    }
+
+    @Test
+    public void testUserMember() throws Exception {
+        Set<String> failed = testGroup.removeMembers(getTestUser().getID());
+        assertTrue(failed.isEmpty());
+    }
+
+    @Test
+    public void testGroupMember() throws Exception {
+        Set<String> failed = testGroup.removeMembers(memberGroup.getID());
+        assertTrue(failed.isEmpty());
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testEmptyId() throws Exception {
+        testGroup.removeMembers("");
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testValidAndEmptyId() throws Exception {
+        testGroup.removeMembers(getTestUser().getID(), "");
+    }
+
+    @Test(expected = NullPointerException.class)
+    public void testNullId() throws Exception {
+        testGroup.removeMembers(null);
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testValidAndNullId() throws Exception {
+        testGroup.removeMembers(getTestUser().getID(), null);
+    }
+
+
+    @Test
+    public void testSameId() throws Exception {
+        Set<String> failed = testGroup.removeMembers(testGroup.getID());
+        assertEquals(1, failed.size());
+        assertTrue(failed.contains(testGroup.getID()));
+    }
+
+    @Test
+    public void testTwice() throws Exception {
+        Set<String> failed = testGroup.removeMembers(memberGroup.getID(), memberGroup.getID());
+        assertTrue(failed.isEmpty());
+
+        assertFalse(testGroup.isDeclaredMember(memberGroup));
+    }
+
+    @Test
+    public void testNotMember() throws Exception {
+        Group gr = null;
+        try {
+            gr = getUserManager(root).createGroup("testGroup" + UUID.randomUUID().toString());
+
+            Set<String> failed = testGroup.removeMembers(gr.getID());
+            assertFalse(failed.isEmpty());
+        } finally {
+            if (gr != null) {
+                gr.remove();
+            }
+        }
+    }
+}
\ No newline at end of file

Added: 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=1694049&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdAbortTest.java Tue Aug  4 12:48:50 2015
@@ -0,0 +1,46 @@
+/*
+ * 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.security.user;
+
+import javax.jcr.nodetype.ConstraintViolationException;
+
+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;
+
+public class AddMembersByIdAbortTest extends AbstractAddMembersByIdTest {
+
+    @Override
+    protected ConfigurationParameters getSecurityConfigParameters() {
+        return ConfigurationParameters.of(UserConfiguration.NAME,
+                ConfigurationParameters.of(
+                        ProtectedItemImporter.PARAM_IMPORT_BEHAVIOR, ImportBehavior.NAME_ABORT)
+        );
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testNonExistingMember() throws Exception {
+        addNonExistingMember();
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testExistingMemberWithoutAccess() throws Exception {
+        addExistingMemberWithoutAccess();
+    }
+}
\ No newline at end of file

Added: 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=1694049&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdBestEffortTest.java Tue Aug  4 12:48:50 2015
@@ -0,0 +1,120 @@
+/*
+ * 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.security.user;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+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.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.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class AddMembersByIdBestEffortTest extends AbstractAddMembersByIdTest {
+
+    private List<Authorizable> toRemove;
+
+    @Override
+    public void after() throws Exception {
+        try {
+            if (toRemove != null) {
+                for (Authorizable a : toRemove) {
+                    a.remove();
+                }
+            }
+        } finally {
+            super.after();
+        }
+    }
+
+    @Override
+    protected ConfigurationParameters getSecurityConfigParameters() {
+        return ConfigurationParameters.of(UserConfiguration.NAME,
+                ConfigurationParameters.of(
+                        ProtectedItemImporter.PARAM_IMPORT_BEHAVIOR, ImportBehavior.NAME_BESTEFFORT)
+        );
+    }
+
+    @Test
+    public void testNonExistingMember() throws Exception {
+        Set<String> failed = addNonExistingMember();
+        assertTrue(failed.isEmpty());
+
+        Iterable<String> memberIds = getMemberIds(testGroup);
+        Iterables.elementsEqual(ImmutableList.copyOf(NON_EXISTING_IDS), memberIds);
+
+        Iterator<Authorizable> members = testGroup.getDeclaredMembers();
+        assertFalse(members.hasNext());
+
+        toRemove = new ArrayList<Authorizable>(NON_EXISTING_IDS.length);
+        for (String id : NON_EXISTING_IDS) {
+            toRemove.add(getUserManager(root).createGroup(id));
+        }
+        members = testGroup.getDeclaredMembers();
+        assertTrue(members.hasNext());
+        for (Authorizable a : toRemove) {
+            assertTrue(testGroup.isDeclaredMember(a));
+        }
+    }
+
+    @Test
+    public void testAddByContentID() throws Exception {
+        Set<String> failed = testGroup.addMembers(AuthorizableBaseProvider.getContentID(getTestUser().getID()));
+        assertTrue(failed.isEmpty());
+
+        assertFalse(testGroup.isMember(getTestUser()));
+    }
+
+    @Test
+    public void testExistingMemberWithoutAccess() throws Exception {
+        Set<String> failed = addExistingMemberWithoutAccess();
+        assertTrue(failed.isEmpty());
+
+        root.refresh();
+        assertTrue(testGroup.isMember(memberGroup));
+    }
+
+    @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));
+        }
+
+    }
+}
\ No newline at end of file

Added: 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=1694049&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/AddMembersByIdIgnoreTest.java Tue Aug  4 12:48:50 2015
@@ -0,0 +1,55 @@
+/*
+ * 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.security.user;
+
+import java.util.Set;
+
+import com.google.common.collect.ImmutableSet;
+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.assertFalse;
+
+public class AddMembersByIdIgnoreTest extends AbstractAddMembersByIdTest {
+
+    @Override
+    protected ConfigurationParameters getSecurityConfigParameters() {
+        return ConfigurationParameters.of(UserConfiguration.NAME,
+                ConfigurationParameters.of(
+                        ProtectedItemImporter.PARAM_IMPORT_BEHAVIOR, ImportBehavior.NAME_IGNORE)
+        );
+    }
+
+    @Test
+    public void testNonExistingMember() throws Exception {
+        Set<String> failed = addNonExistingMember();
+        assertEquals(ImmutableSet.copyOf(NON_EXISTING_IDS), failed);
+    }
+
+    @Test
+    public void testExistingMemberWithoutAccess() throws Exception {
+        Set<String> failed = addExistingMemberWithoutAccess();
+        assertEquals(ImmutableSet.of(memberGroup.getID()), failed);
+
+        root.refresh();
+        assertFalse(testGroup.isMember(memberGroup));
+    }
+}
\ No newline at end of file

Added: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdAbortTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdAbortTest.java?rev=1694049&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdAbortTest.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdAbortTest.java Tue Aug  4 12:48:50 2015
@@ -0,0 +1,46 @@
+/*
+ * 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.security.user;
+
+import javax.jcr.nodetype.ConstraintViolationException;
+
+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;
+
+public class RemoveMembersByIdAbortTest extends AbstractRemoveMembersByIdTest {
+
+    @Override
+    protected ConfigurationParameters getSecurityConfigParameters() {
+        return ConfigurationParameters.of(UserConfiguration.NAME,
+                ConfigurationParameters.of(
+                        ProtectedItemImporter.PARAM_IMPORT_BEHAVIOR, ImportBehavior.NAME_ABORT)
+        );
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testNonExistingMember() throws Exception {
+        removeNonExistingMember();
+    }
+
+    @Test(expected = ConstraintViolationException.class)
+    public void testMissingAccessMember() throws Exception {
+        removeExistingMemberWithoutAccess();
+    }
+}
\ No newline at end of file

Added: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java?rev=1694049&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdBestEffortTest.java Tue Aug  4 12:48:50 2015
@@ -0,0 +1,65 @@
+/*
+ * 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.security.user;
+
+import java.util.Set;
+
+import com.google.common.collect.ImmutableSet;
+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.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class RemoveMembersByIdBestEffortTest extends AbstractRemoveMembersByIdTest {
+
+    @Override
+    protected ConfigurationParameters getSecurityConfigParameters() {
+        return ConfigurationParameters.of(UserConfiguration.NAME,
+                ConfigurationParameters.of(
+                        ProtectedItemImporter.PARAM_IMPORT_BEHAVIOR, ImportBehavior.NAME_BESTEFFORT)
+        );
+    }
+
+    @Test
+    public void testNonExistingMember() throws Exception {
+        Set<String> failed = removeNonExistingMember();
+        assertEquals(ImmutableSet.copyOf(NON_EXISTING_IDS), failed);
+    }
+
+    @Test
+    public void testNonExistingMemberAfterAdd() throws Exception {
+        Set<String> failed = testGroup.addMembers(NON_EXISTING_IDS);
+        assertTrue(failed.isEmpty());
+
+        failed = removeNonExistingMember();
+        assertTrue(failed.isEmpty());
+    }
+
+    @Test
+    public void testMissingAccessMember() throws Exception {
+        Set<String> failed = removeExistingMemberWithoutAccess();
+        assertTrue(failed.isEmpty());
+
+        root.refresh();
+        assertFalse(testGroup.isMember(memberGroup));
+    }
+}
\ No newline at end of file

Added: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdIgnoreTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdIgnoreTest.java?rev=1694049&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdIgnoreTest.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RemoveMembersByIdIgnoreTest.java Tue Aug  4 12:48:50 2015
@@ -0,0 +1,55 @@
+/*
+ * 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.security.user;
+
+import java.util.Set;
+
+import com.google.common.collect.ImmutableSet;
+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;
+
+public class RemoveMembersByIdIgnoreTest extends AbstractRemoveMembersByIdTest {
+
+    @Override
+    protected ConfigurationParameters getSecurityConfigParameters() {
+        return ConfigurationParameters.of(UserConfiguration.NAME,
+                ConfigurationParameters.of(
+                        ProtectedItemImporter.PARAM_IMPORT_BEHAVIOR, ImportBehavior.NAME_IGNORE)
+        );
+    }
+
+    @Test
+    public void testNonExistingMember() throws Exception {
+        Set<String> failed = removeNonExistingMember();
+        assertEquals(ImmutableSet.copyOf(NON_EXISTING_IDS), failed);
+    }
+
+    @Test
+    public void testMissingAccessMember() throws Exception {
+        Set<String> failed = removeExistingMemberWithoutAccess();
+        assertEquals(ImmutableSet.of(memberGroup.getID()), failed);
+
+        root.refresh();
+        assertTrue(testGroup.isMember(memberGroup));
+    }
+}
\ No newline at end of file

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/autosave/AutoSaveEnabledManagerTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/autosave/AutoSaveEnabledManagerTest.java?rev=1694049&r1=1694048&r2=1694049&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/autosave/AutoSaveEnabledManagerTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/autosave/AutoSaveEnabledManagerTest.java Tue Aug  4 12:48:50 2015
@@ -248,4 +248,23 @@ public class AutoSaveEnabledManagerTest
         assertFalse(root.hasPendingChanges());
         assertFalse(u.declaredMemberOf().hasNext());
     }
+
+    @Test
+    public void testAddMembers() throws Exception {
+        User u = mgr.createUser("u", "u");
+        Group g = mgr.createGroup("g");
+
+        assertTrue(g.addMembers(u.getID()).isEmpty());
+        assertFalse(root.hasPendingChanges());
+    }
+
+    @Test
+    public void testRemoveMembers() throws Exception {
+        User u = mgr.createUser("u", "u");
+        Group g = mgr.createGroup("g");
+        g.addMember(u);
+
+        assertTrue(g.removeMembers(u.getID()).isEmpty());
+        assertFalse(root.hasPendingChanges());
+    }
 }
\ No newline at end of file

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=1694049&r1=1694048&r2=1694049&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 Tue Aug  4 12:48:50 2015
@@ -30,6 +30,8 @@ member relationship of users and groups:
     - `isMember(Authorizable boolean`
     - `addMember(Authorizable) boolean`
     - `removeMember(Authorizable) boolen`
+    - `addMembers(String...) Set<String>`
+    - `removeMembers(String...) Set<String>`
 
 - [org.apache.jackrabbit.api.security.user.Authorizable]
     - `declaredMemberOf() Iterator<Group>`
@@ -139,6 +141,29 @@ internally processed using the normal us
 node structure after the import might not be the same as the one represented in
 the input.
 
+#### Add and Remove Group Members by Id
+
+Since Oak 1.3.4 the default user management implementation also allows to modify
+group membership by specifying the member id(s) (see [JCR-3880] and [OAK-3170]).
+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,
+- duplicate ids in the parameter list will be silently ignored,
+- 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,
+- 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
+      exist `ConstraintViolationException` is thrown immediately; changes already
+      made will not be reverted.
+    - BESTEFFORT: the specified ids are not resolved to the corresponding
+      authorizables and are silently added|removed to|from the set of members;
+      ids that were not successfully added|removed are listed in the return value.
+    - IGNORE: each id is resolved to the corresponding authorizable; if it doesn't
+      exist it will be returned as _failed_ in the return value.
+
 ### Configuration
 
 Note that as of Oak 1.0 the implementation is responsible for defining the
@@ -149,4 +174,6 @@ with Jackrabbit 2.x is not supported any
 
 <!-- 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
\ No newline at end of file
+[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
+[JCR-3880]: https://issues.apache.org/jira/browse/JCR-3880
+[OAK-3170]: https://issues.apache.org/jira/browse/OAK-3170

Modified: jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/GroupDelegator.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/GroupDelegator.java?rev=1694049&r1=1694048&r2=1694049&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/GroupDelegator.java (original)
+++ jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/GroupDelegator.java Tue Aug  4 12:48:50 2015
@@ -20,6 +20,7 @@
 package org.apache.jackrabbit.oak.jcr.delegate;
 
 import java.util.Iterator;
+import java.util.Set;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.jcr.RepositoryException;
@@ -134,6 +135,17 @@ final class GroupDelegator extends Autho
     }
 
     @Override
+    public Set<String> addMembers(@Nonnull final String... memberIds) throws RepositoryException {
+        return sessionDelegate.perform(new SessionOperation<Set<String>>("addMembers") {
+            @Nonnull
+            @Override
+            public Set<String> perform() throws RepositoryException {
+                return getDelegate().addMembers(memberIds);
+            }
+        });
+    }
+
+    @Override
     public boolean removeMember(final Authorizable authorizable) throws RepositoryException {
         return sessionDelegate.perform(new SessionOperation<Boolean>("removeMember") {
             @Nonnull
@@ -143,4 +155,15 @@ final class GroupDelegator extends Autho
             }
         });
     }
+
+    @Override
+    public Set<String> removeMembers(@Nonnull final String... memberIds) throws RepositoryException {
+        return sessionDelegate.perform(new SessionOperation<Set<String>>("removeMembers") {
+            @Nonnull
+            @Override
+            public Set<String> perform() throws RepositoryException {
+                return getDelegate().removeMembers(memberIds);
+            }
+        });
+    }
 }

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=1694049&r1=1694048&r2=1694049&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 Tue Aug  4 12:48:50 2015
@@ -848,6 +848,39 @@ public class GroupTest extends AbstractU
         }
     }
 
+    public void testAddMembersById() throws Exception {
+        Group newGroup = null;
+        try {
+            newGroup = userMgr.createGroup(createGroupId());
+
+            Set<String> failed = group.addMembers("nonExistingMember", newGroup.getID());
+            assertFalse(failed.isEmpty());
+            assertTrue(group.isMember(newGroup));
+        } finally {
+            if (newGroup != null) {
+                newGroup.remove();
+                superuser.save();
+            }
+        }
+    }
+
+    public void testRemoveMembersById() throws Exception {
+        Group newGroup = null;
+        try {
+            newGroup = userMgr.createGroup(createGroupId());
+
+            Set<String> failed = group.removeMembers("nonExistingMember", newGroup.getID(), user.getID());
+            assertFalse(failed.isEmpty());
+            assertFalse(group.isMember(user));
+        } finally {
+            if (newGroup != null) {
+                newGroup.remove();
+                superuser.save();
+            }
+        }
+
+    }
+
     private void checkDeclaredMembers(Group grp, String ... ids) throws RepositoryException {
         TreeSet<String> members = new TreeSet<String>();
         Iterator<Authorizable> iter = grp.getMembers();