jackrabbit-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ang...@apache.org
Subject svn commit: r651624 - in /jackrabbit/trunk: jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/secu...
Date Fri, 25 Apr 2008 15:11:57 GMT
Author: angela
Date: Fri Apr 25 08:11:49 2008
New Revision: 651624

URL: http://svn.apache.org/viewvc?rev=651624&view=rev
Log:
JCR-1104 : JSR 283 support (security work in progress)

- user API: add distinction between declared and indirect group membership
- user Impl: move group membership to authorizable node type
- javadoc
- minor fixes/improvements

Modified:
    jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Authorizable.java
    jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Group.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/DefaultPrincipalProvider.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/AuthorizableImpl.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserAccessControlProvider.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserConstants.java
    jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.cnd
    jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.xml
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/AuthorizableTest.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/AuthorizableImplTest.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/GroupAdministratorTest.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/UserAdministratorTest.java

Modified: jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Authorizable.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Authorizable.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Authorizable.java (original)
+++ jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Authorizable.java Fri Apr 25 08:11:49 2008
@@ -112,7 +112,14 @@
     PrincipalIterator getPrincipals() throws RepositoryException;
 
     /**
-     * @return all {@link Group}s, this Authorizable is member of
+     * @return all {@link Group}s, this Authorizable is declared member of.
+     * @throws RepositoryException
+     */
+    Iterator declaredMemberOf() throws RepositoryException;
+
+    /**
+     * @return all {@link Group}s, this Authorizable is member of included
+     * indirect group membership.
      * @throws RepositoryException
      */
     Iterator memberOf() throws RepositoryException;

Modified: jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Group.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Group.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Group.java (original)
+++ jackrabbit/trunk/jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/Group.java Fri Apr 25 08:11:49 2008
@@ -16,8 +16,6 @@
  */
 package org.apache.jackrabbit.api.security.user;
 
-import org.apache.jackrabbit.api.security.user.Authorizable;
-
 import javax.jcr.RepositoryException;
 import java.util.Iterator;
 
@@ -27,14 +25,23 @@
 public interface Group extends Authorizable {
 
     /**
-     * @return Iterator of <code>Authorizable</code>s which are getMembers of
-     * this Group.
+     * @return Iterator of <code>Authorizable</code>s which are declared
+     * members of this Group.
+     * @throws RepositoryException
+     */
+    Iterator getDeclaredMembers() throws RepositoryException;
+
+    /**
+     * @return Iterator of <code>Authorizable</code>s which are members of
+     * this Group. This includes both declared members and all authorizables
+     * that are indirect group members.
      * @throws RepositoryException
      */
     Iterator getMembers() throws RepositoryException;
 
     /**
-     * @return true if the Authorizable to test is a member of this Group.
+     * @return true if the Authorizable to test is a direct or indirect member
+     * of this Group.
      * @throws RepositoryException
      */
     boolean isMember(Authorizable authorizable) throws RepositoryException;
@@ -45,14 +52,14 @@
      *
      * @return true if the <code>Authorizable</code> has successfully been added
      * to this Group, false otherwise (e.g. unknown implemention
-     * or if it already is a member or if the passed authorizable is the
+     * or if it already is a member or if the passed authorizable is this
      * group itself or for some implementation specific constraint).
      * @throws RepositoryException If an error occurs.
      */
     boolean addMember(Authorizable authorizable) throws RepositoryException;
 
     /**
-     * Remove a member to this Group.<br>
+     * Remove a member from this Group.<br>
      * Changes will be persisted immediately.
      *
      * @return true if the Authorizable was successfully removed. False otherwise.

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/DefaultPrincipalProvider.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/DefaultPrincipalProvider.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/DefaultPrincipalProvider.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/DefaultPrincipalProvider.java Fri Apr 25 08:11:49 2008
@@ -25,6 +25,9 @@
 import org.apache.jackrabbit.api.security.user.Group;
 import org.apache.jackrabbit.api.security.user.UserManager;
 import org.apache.jackrabbit.core.security.user.UserManagerImpl;
+import org.apache.jackrabbit.core.SessionImpl;
+import org.apache.jackrabbit.spi.commons.conversion.NameResolver;
+import org.apache.jackrabbit.util.Text;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -67,6 +70,8 @@
 
     private final EveryonePrincipal everyonePrincipal;
 
+    private final String pGroupName;
+
     /**
      * Creates a new DefaultPrincipalProvider reading the principals from the
      * storage below the given security root node.
@@ -81,13 +86,22 @@
         everyonePrincipal = EveryonePrincipal.getInstance();
         membershipCache = new LRUMap();
 
-        // listen to any modifications for users and groups
+        // listen to modifications of group-membership
+        String[] ntNames = new String[1];
+        if (securitySession instanceof SessionImpl) {
+            NameResolver resolver = (SessionImpl) securitySession;
+            ntNames[0] = resolver.getJCRName(UserManagerImpl.NT_REP_USER);
+            pGroupName = resolver.getJCRName(UserManagerImpl.P_GROUPS);
+        } else {
+            ntNames[0] = "rep:User";
+            pGroupName = "rep:groups";
+        }
         securitySession.getWorkspace().getObservationManager().addEventListener(this,
-                Event.NODE_ADDED | Event.NODE_REMOVED | Event.PROPERTY_ADDED | Event.PROPERTY_CHANGED | Event.PROPERTY_REMOVED,
+                Event.NODE_REMOVED | Event.PROPERTY_ADDED | Event.PROPERTY_CHANGED | Event.PROPERTY_REMOVED,
                 UserManagerImpl.SECURITY_ROOT_PATH,
                 true,
                 null,
-                null,
+                ntNames,
                 false);
     }
 
@@ -220,10 +234,27 @@
      * @see EventListener#onEvent(EventIterator)
      */
     public void onEvent(EventIterator eventIterator) {
-        // simple rule: flush all cached
+        // superclass: flush all cached
         clearCache();
-        synchronized (membershipCache) {
-            membershipCache.clear();
+
+        // membership cache:
+        while (eventIterator.hasNext()) {
+            Event ev = eventIterator.nextEvent();
+            int type = ev.getType();
+            if (type == Event.PROPERTY_ADDED || type == Event.PROPERTY_CHANGED
+                    || type == Event.PROPERTY_REMOVED) {
+                try {
+                    if (pGroupName.equals(Text.getName(ev.getPath()))) {
+                        synchronized (membershipCache) {
+                            membershipCache.clear();
+                        }
+                        break;
+                    }
+                } catch (RepositoryException e) {
+                    // should never get here
+                    log.warn(e.getMessage());
+                }
+            }
         }
     }
 
@@ -247,13 +278,7 @@
                 Iterator itr = auth.memberOf();
                 while (itr.hasNext()) {
                     Group group = (Group) itr.next();
-                    Principal groupPrinc = group.getPrincipal();
-                    if (membership.add(groupPrinc)) {
-                        membership.addAll(collectGroupMembership(groupPrinc, membership));
-                    } else {
-                        String msg = "Cyclic group membership detected with Group " + groupPrinc.getName();
-                        log.error(msg);
-                    }
+                    membership.add(group.getPrincipal());
                 }
             } else {
                 log.debug("Cannot find authorizable for principal " + princ.getName());

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/AuthorizableImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/AuthorizableImpl.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/AuthorizableImpl.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/AuthorizableImpl.java Fri Apr 25 08:11:49 2008
@@ -16,14 +16,15 @@
  */
 package org.apache.jackrabbit.core.security.user;
 
-import org.apache.jackrabbit.core.NodeImpl;
-import org.apache.jackrabbit.core.SessionImpl;
-import org.apache.jackrabbit.core.PropertyImpl;
+import org.apache.jackrabbit.api.security.principal.PrincipalIterator;
+import org.apache.jackrabbit.api.security.principal.PrincipalManager;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.AuthorizableExistsException;
 import org.apache.jackrabbit.api.security.user.Group;
-import org.apache.jackrabbit.api.security.principal.PrincipalIterator;
-import org.apache.jackrabbit.api.security.principal.PrincipalManager;
+import org.apache.jackrabbit.api.security.user.User;
+import org.apache.jackrabbit.core.NodeImpl;
+import org.apache.jackrabbit.core.PropertyImpl;
+import org.apache.jackrabbit.core.SessionImpl;
 import org.apache.jackrabbit.core.security.principal.ItemBasedPrincipal;
 import org.apache.jackrabbit.core.security.principal.PrincipalImpl;
 import org.apache.jackrabbit.core.security.principal.PrincipalIteratorAdapter;
@@ -39,8 +40,8 @@
 import javax.jcr.nodetype.ConstraintViolationException;
 import java.security.Principal;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 
@@ -132,20 +133,21 @@
     }
 
     /**
+     * @see Authorizable#declaredMemberOf()
+     */
+    public Iterator declaredMemberOf() throws RepositoryException {
+        List memberShip = new ArrayList();
+        collectMembership(memberShip, false);
+        return memberShip.iterator();
+    }
+
+    /**
      * @see Authorizable#memberOf()
      */
     public Iterator memberOf() throws RepositoryException {
-        // TODO: replace by weak-refs
-        PropertyIterator itr = node.getReferences();
-        Collection tmp = new HashSet((int) itr.getSize());
-        while (itr.hasNext()) {
-            NodeImpl groupNode = (NodeImpl) itr.nextProperty().getParent();
-            if (groupNode.isNodeType(NT_REP_GROUP)) {
-                Group group = GroupImpl.create(groupNode, userManager);
-                tmp.add(group);
-            }
-        }
-        return tmp.iterator();
+        List memberShip = new ArrayList();
+        collectMembership(memberShip, true);
+        return memberShip.iterator();
     }
 
     /**
@@ -252,7 +254,11 @@
      * @see Authorizable#remove()
      */
     public synchronized void remove() throws RepositoryException {
-        // TODO: ev. remove group-memberships first?
+        // don't allow for removal of the administrator even if the executing
+        // session has all permissions.
+        if (!isGroup() && ((User) this).isAdmin()) {
+            throw new RepositoryException("The administrator cannot be removed.");
+        }
         userManager.removeProtectedItem(node, node.getParent());
     }
 
@@ -273,6 +279,77 @@
         return node.getProperty(P_PRINCIPAL_NAME).getString();
     }
 
+    boolean addToGroup(GroupImpl group) throws RepositoryException {
+        try {
+            Value[] values;
+            // TODO: replace by weak-refs
+            Value added = getSession().getValueFactory().createValue(group.getNode());
+            NodeImpl node = getNode();
+            if (node.hasProperty(P_GROUPS)) {
+                Value[] old = node.getProperty(P_GROUPS).getValues();
+                values = new Value[old.length + 1];
+                System.arraycopy(old, 0, values, 0, old.length);
+            } else {
+                values = new Value[1];
+            }
+            values[values.length - 1] = added;
+            userManager.setProtectedProperty(node, P_GROUPS, values);
+            return true;
+        } catch (RepositoryException e) {
+            // revert all pending changes and rethrow.
+            log.error("Error while editing group membership:", e.getMessage());
+            getSession().refresh(false);
+            throw e;
+        }
+    }
+
+    boolean removeFromGroup(GroupImpl group) throws RepositoryException {
+        NodeImpl node = getNode();
+        String message = "Authorizable " + getID() + " is not member of " + group.getID();
+        if (!node.hasProperty(P_GROUPS)) {
+            log.debug(message);
+            return false;
+        }
+
+        Value toRemove = getSession().getValueFactory().createValue(group.getNode());
+        PropertyImpl property = node.getProperty(P_GROUPS);
+        List valList = new ArrayList(Arrays.asList(property.getValues()));
+        if (valList.remove(toRemove)) {
+            try {
+                if (valList.isEmpty()) {
+                    userManager.removeProtectedItem(property, node);
+                } else {
+                    Value[] values = (Value[]) valList.toArray(new Value[valList.size()]);
+                    userManager.setProtectedProperty(node, P_GROUPS, values);
+                }
+                return true;
+            } catch (RepositoryException e) {
+                // modification failed -> revert all pending changes.
+                node.refresh(false);
+                throw e;
+            }
+        } else {
+            // nothing changed
+            log.debug(message);
+            return false;
+        }
+    }
+
+    private void collectMembership(List groups, boolean includedIndirect) throws RepositoryException {
+        NodeImpl node = getNode();
+        if (!node.hasProperty(P_GROUPS)) {
+            return;
+        }
+        Value[] refs = node.getProperty(P_GROUPS).getValues();
+        for (int i = 0; i < refs.length; i++) {
+            NodeImpl groupNode = (NodeImpl) getSession().getNodeByUUID(refs[i].getString());
+            Group group = GroupImpl.create(groupNode, userManager);
+            if (groups.add(group) && includedIndirect) {
+                ((AuthorizableImpl) group).collectMembership(groups, true);
+            }
+        }
+    }
+
     /**
      * Test if the JCR property to be modified/removed is one of the
      * following that has a special meaning and must be altered using this
@@ -282,7 +359,7 @@
      * <li>rep:principalName</li>
      * <li>rep:userId</li>
      * <li>rep:referees</li>
-     * <li>rep:members</li>
+     * <li>rep:groups</li>
      * <li>rep:impersonators</li>
      * </ul>
      * Those properties are 'protected' in their property definition. This
@@ -296,7 +373,7 @@
     private boolean isProtectedProperty(String propertyName) throws RepositoryException {
         Name pName = getSession().getQName(propertyName);
          if (P_PRINCIPAL_NAME.equals(pName) || P_USERID.equals(pName)
-                 || P_REFEREES.equals(pName) || P_MEMBERS.equals(pName)
+                 || P_REFEREES.equals(pName) || P_GROUPS.equals(pName)
                  || P_IMPERSONATORS.equals(pName)) {
              return true;
          } else {

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java Fri Apr 25 08:11:49 2008
@@ -16,30 +16,25 @@
  */
 package org.apache.jackrabbit.core.security.user;
 
-import org.apache.jackrabbit.core.NodeImpl;
-import org.apache.jackrabbit.core.PropertyImpl;
-import org.apache.jackrabbit.api.security.principal.PrincipalManager;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
+import org.apache.jackrabbit.api.security.user.User;
+import org.apache.jackrabbit.core.NodeImpl;
 import org.apache.jackrabbit.util.Text;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.jcr.Node;
-import javax.jcr.Property;
+import javax.jcr.PropertyIterator;
 import javax.jcr.RepositoryException;
-import javax.jcr.Value;
 import java.io.IOException;
 import java.io.ObjectOutputStream;
 import java.security.Principal;
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Enumeration;
 import java.util.HashSet;
 import java.util.Iterator;
-import java.util.List;
 import java.util.NoSuchElementException;
 import java.util.Set;
 
@@ -98,10 +93,17 @@
 
     //--------------------------------------------------------------< Group >---
     /**
+     * @see Group#getDeclaredMembers()
+     */
+    public Iterator getDeclaredMembers() throws RepositoryException {
+        return getMembers(false).iterator();
+    }
+
+    /**
      * @see Group#getMembers()
      */
     public Iterator getMembers() throws RepositoryException {
-        return new MemberIterator(memberUUIDs().iterator());
+        return getMembers(true).iterator();
     }
 
     /**
@@ -111,9 +113,14 @@
         if (authorizable == null || !(authorizable instanceof AuthorizableImpl)) {
             return false;
         } else {
+            String thisID = getID();
             AuthorizableImpl impl = (AuthorizableImpl) authorizable;
-            String uuid = impl.getNode().getUUID();
-            return memberUUIDs().contains(uuid);
+            for (Iterator it = impl.memberOf(); it.hasNext();) {
+                if (thisID.equals(((GroupImpl) it.next()).getID())) {
+                    return true;
+                }
+            }
+            return false;
         }
     }
 
@@ -130,27 +137,16 @@
             return false;
         }
 
-        Node memberNode = ((AuthorizableImpl) authorizable).getNode();
+        AuthorizableImpl authImpl = ((AuthorizableImpl) authorizable);
+        Node memberNode = authImpl.getNode();
         if (memberNode.isSame(getNode())) {
             String msg = "Attempt to add a Group as member of itself (" + getID() + ").";
             log.warn(msg);
             return false;
         }
 
-        Value[] values;
-        // TODO: replace by weak-refs
-        Value added = getSession().getValueFactory().createValue(memberNode);
-        NodeImpl node = getNode();
-        if (node.hasProperty(P_MEMBERS)) {
-            Value[] old = node.getProperty(P_MEMBERS).getValues();
-            values = new Value[old.length + 1];
-            System.arraycopy(old, 0, values, 0, old.length);
-        } else {
-            values = new Value[1];
-        }
-        values[values.length - 1] = added;
-        userManager.setProtectedProperty(node, P_MEMBERS, values);
-        return true;
+        // preconditions are met -> delegate to authorizableImpl
+        return authImpl.addToGroup(this);
     }
 
     /**
@@ -160,61 +156,40 @@
         if (!isMember(authorizable) || !(authorizable instanceof AuthorizableImpl)) {
             return false;
         }
-        NodeImpl node = getNode();
-        if (!node.hasProperty(P_MEMBERS)) {
-            log.debug("Group has no members -> cannot remove member " + authorizable.getID());
-            return false;
-        }
-
-        Value toRemove = getSession().getValueFactory().createValue(((AuthorizableImpl)authorizable).getNode());
-
-        PropertyImpl property = node.getProperty(P_MEMBERS);
-        List valList = new ArrayList(Arrays.asList(property.getValues()));
-
-        if (valList.remove(toRemove)) {
-            try {
-                if (valList.isEmpty()) {
-                    userManager.removeProtectedItem(property, node);
-                } else {
-                    Value[] values = (Value[]) valList.toArray(new Value[valList.size()]);
-                    userManager.setProtectedProperty(node, P_MEMBERS, values);
-                }
-                return true;
-            } catch (RepositoryException e) {
-                // modification failed -> revert all pending changes.
-                node.refresh(false);
-                throw e;
-            }
-        } else {
-            // nothing changed
-            log.debug("Authorizable " + authorizable.getID() + " was not member of " + getID());
-            return false;
-        }
+        return ((AuthorizableImpl) authorizable).removeFromGroup(this);
     }
 
     //--------------------------------------------------------------------------
-
     /**
      *
      * @return
      * @throws RepositoryException
      */
-    private Collection memberUUIDs() throws RepositoryException {
-        Collection tmp = new HashSet();
-        if (getNode().hasProperty(P_MEMBERS)) {
-            Property prop = getNode().getProperty(P_MEMBERS);
-            Value[] val = prop.getValues();
-            for (int i = 0; i < val.length; i++) {
-                tmp.add(val[i].getString());
+    private Collection getMembers(boolean includeIndirect) throws RepositoryException {
+        // TODO: replace by weak-refs
+        PropertyIterator itr = getNode().getReferences();
+        Collection members = new HashSet((int) itr.getSize());
+        while (itr.hasNext()) {
+            NodeImpl n = (NodeImpl) itr.nextProperty().getParent();
+            if (n.isNodeType(NT_REP_GROUP)) {
+                Group group = create(n, userManager);
+                // only retrieve indirect group-members if the group is not
+                // yet present (detected eventual circular membership).
+                if (members.add(group) && includeIndirect) {
+                    members.addAll(((GroupImpl) group).getMembers(true));
+                }
+            } else if (n.isNodeType(NT_REP_USER)) {
+                User user = UserImpl.create(n, userManager);
+                members.add(user);
             }
         }
-        return tmp;
+        return members;
     }
 
     /**
-     * Since {@link #isMember(Authorizable)} only detects the declared
-     * members of this group, this methods is used to avoid cyclic membership
-     * declarations.
+     * Since {@link #isMember(Authorizable)} detects declared and inherited
+     * membership this method simply checks if the potential new member is
+     * a group that would in turn have <code>this</code> as a member.
      *
      * @param newMember
      * @return true if the 'newMember' is a group and 'this' is an declared or
@@ -225,13 +200,6 @@
         if (newMember.isGroup()) {
             Group gr = (Group) newMember;
             cyclic = gr.isMember(this);
-            if (!cyclic) {
-                PrincipalManager pmgr = getSession().getPrincipalManager();
-                for (Iterator it = pmgr.getGroupMembership(getPrincipal());
-                     it.hasNext() && !cyclic;) {
-                    cyclic = newMember.getPrincipal().equals(it.next());
-                }
-            }
         }
         return cyclic;
     }
@@ -370,9 +338,8 @@
                 try {
                     for (Iterator it = GroupImpl.this.getMembers(); it.hasNext();) {
                         Authorizable authrz = (Authorizable) it.next();
-                        // TODO: check again. only add main principal, since
-                        // 'referees' belong to a different provider and should
-                        // not be exposed here
+                        // NOTE: only add main principal, since 'referees' belong
+                        // to a different provider and should not be exposed here
                         members.add(authrz.getPrincipal());
                     }
                 } catch (RepositoryException e) {

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserAccessControlProvider.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserAccessControlProvider.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserAccessControlProvider.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserAccessControlProvider.java Fri Apr 25 08:11:49 2008
@@ -39,6 +39,7 @@
 import javax.jcr.Property;
 import javax.jcr.RepositoryException;
 import javax.jcr.Session;
+import javax.jcr.Value;
 import javax.jcr.observation.Event;
 import javax.jcr.observation.EventListener;
 import javax.jcr.observation.EventIterator;
@@ -55,9 +56,35 @@
  * is used to protected the 'security workspace' containing the user and
  * group data. It applies special care to make sure that modifying user data
  * (e.g. password), group membership and impersonation is properly controlled.
- * The access control policy defined by this provider has the following
+ * <p/>
+ * This provider creates upon initialization the following 2 groups:
+ * <ul>
+ * <li>User administrator</li>
+ * <li>Group administrator</li>
+ * </ul>
+ *
+ * The default access control policy defined by this provider has the following
  * characteristics:
- * TODO describe policy.
+ * <ul>
+ * <li>everybody has READ permission to all items,</li>
+ *
+ * <li>every known user is allowed to modify it's own properties except for
+ * her/his group membership,</li>
+ *
+ * <li>members of the 'User administrator' group are allowed to create, modify
+ * and remove those users whose node representation is within the subtree
+ * defined by the node representation of the editing user,</li>
+ *
+ * <li>members of the 'Group administrator' group are allowed to create, modify
+ * and remove groups,</li>
+ *
+ * <li>group membership can only be edited by members of the 'Group administrator'
+ * and the 'User administrator' group. The range of users that can be added
+ * as member to any Group is limited to those that are editable according to
+ * the restrictions described above for the 'User administrator'.</li>
+ * </ul>
+ *
+ * TODO: allow for editing of additional ac that extends the default permission evaluted by this provided.
  */
 public class UserAccessControlProvider extends AbstractAccessControlProvider
         implements UserConstants {
@@ -261,17 +288,17 @@
             implements EventListener {
 
         private final NodeImpl userNode;
-        private final boolean isUserAdmin;
-        private final boolean isGroupAdmin;
+
+        private boolean isUserAdmin;
+        private boolean isGroupAdmin;
 
         protected CompiledPermissionsImpl(Set principals, NodeImpl userNode) throws RepositoryException {
             this.userNode = userNode;
-
             isUserAdmin = containsGroup(principals, userAdminGroup);
             isGroupAdmin = containsGroup(principals, groupAdminGroup);
 
             int events = Event.PROPERTY_CHANGED | Event.PROPERTY_ADDED | Event.PROPERTY_REMOVED;
-            observationMgr.addEventListener(this, events, GROUPS_PATH, true, null, null, false);
+            observationMgr.addEventListener(this, events, USERS_PATH, true, null, null, false);
         }
 
         //------------------------------------< AbstractCompiledPermissions >---
@@ -290,7 +317,14 @@
             }
 
             Path abs2Path = path.subPath(0, 4);
+            //
             if (usersPath.equals(abs2Path)) {
+                /*
+                 below the user-tree
+                 - determine position of target relative to the node of the editing user
+                 - determine if the editing user is user/group-admin
+                 - special treatment for rep:groups property
+                 */
                 NodeImpl node = (NodeImpl) getExistingNode(path);
                 NodeImpl authN = null;
                 // seek next rep:authorizable parent
@@ -311,25 +345,40 @@
 
                 if (authN != null && authN.isNodeType(NT_REP_USER)) {
                     int relDepth = session.getHierarchyManager().getRelativeDepth(userNode.getNodeId(), authN.getNodeId());
+                    boolean isGroupProp = P_GROUPS.equals(path.getNameElement().getName());
+                    // only user-admin is allowed to modify users.
+                    // for group membership (rep:groups) group-admin is required
+                    // in addition.
+                    boolean requiredGroups = isUserAdmin;
+                    if (requiredGroups && isGroupProp) {
+                        requiredGroups = isGroupAdmin;
+                    }
                     switch (relDepth) {
                         case -1:
                             // authN is not below the userNode -> can't write anyway.
                             break;
                         case 0:
                             /*
-                            authN is same node as userNode. 2 cases to distinguish
+                            authN is same node as userNode. 3 cases to distinguish
                             1) user is User-Admin -> R, W
                             2) user is NOT U-admin but nodeID is its own node.
+                            3) special treatment for rep:group property which can
+                               only be modified by group-administrators
                             */
-                            if (isUserAdmin) {
-                                // principals contain 'user-admin' -> user can modify
-                                // any item below the user-node.
+                            if (requiredGroups) {
+                                // principals contain 'user-admin'
+                                // -> user can modify items below the user-node except rep:group.
+                                // principals contains 'user-admin' + 'group-admin'
+                                // -> user can modify rep:group property as well.
                                 perms = Permission.ALL;
                                 if (calcPrivs) {
+                                    // grant WRITE privilege
+                                    // note: ac-read/modification is not included
                                     privs |= PrivilegeRegistry.WRITE;
                                 }
-                            } else if (userNode.isSame(node)) {
+                            } else if (userNode.isSame(node) && (!isGroupProp || isGroupAdmin)) {
                                 // user can only read && write his own props
+                                // except for the rep:group property.
                                 perms |= (Permission.SET_PROPERTY | Permission.REMOVE_PROPERTY);
                                 if (calcPrivs) {
                                     privs |= PrivilegeRegistry.MODIFY_PROPERTIES;
@@ -342,12 +391,16 @@
                             1) nodeId points to an authorizable below userNode
                             2) nodeId points to an auth-folder below some authorizable below userNode.
 
-                            In either case user-admin group is required to have write
-                            permission.
+                            In either case user-admin group-membership is
+                            required in order to get write permission.
+                            group-admin group-membership is required in addition
+                            if rep:groups is the target item.
                             */
-                            if (isUserAdmin) {
+                            if (requiredGroups) {
                                 perms = Permission.ALL;
                                 if (calcPrivs) {
+                                    // grant WRITE privilege
+                                    // note: ac-read/modification is not included
                                     privs |= PrivilegeRegistry.WRITE;
                                 }
                             }
@@ -357,26 +410,13 @@
                 /*
                 below group-tree:
                 - test if the user is group-administrator.
-                - in addition the following special condition must be checked:
-
-                if the target id is 'rep:members' the user MUST be member of that
-                the containing group in order to have WRITE privilege.
-                this required in order to make sure the group-admin cannot
-                modify the members of some other groups e.g. administrators.
                 */
                 if (isGroupAdmin) {
-                    if (P_MEMBERS.equals(path.getNameElement().getName())) {
-                        if (isMember(userNode, path)) {
-                            perms |= (Permission.SET_PROPERTY | Permission.REMOVE_PROPERTY);
-                        }
-                    } else {
-                        perms = Permission.ALL;
-                        if (calcPrivs) {
-                            privs |= PrivilegeRegistry.WRITE;
-                        }
+                    perms = Permission.ALL;
+                    if (calcPrivs) {
+                        privs |= PrivilegeRegistry.WRITE;
                     }
                 }
-
             } // else outside of user/group tree -> read only.
 
             return new Result(perms, privs);
@@ -426,8 +466,34 @@
                 Event ev = events.nextEvent();
                 try {
                     String evPath = ev.getPath();
-                    if ("rep:members".equals(Text.getName(evPath))) {
-                        // TODO: add better evaluation.
+                    String repGroups = session.getJCRName(UserConstants.P_GROUPS);
+                    // TODO: add better evaluation.
+                    if (repGroups.equals(Text.getName(evPath)) &&
+                            userNode.getPath().equals(Text.getRelativeParent(evPath, 1))) {
+                        // recalculate the is...Admin flags
+                        switch (ev.getType()) {
+                            case Event.PROPERTY_REMOVED:
+                                isUserAdmin = false;
+                                isGroupAdmin = false;
+                                break;
+                            case Event.PROPERTY_ADDED:
+                            case Event.PROPERTY_CHANGED:
+                                // TODO: improve
+                                Value[] vs = session.getProperty(evPath).getValues();
+                                String princName = session.getJCRName(P_PRINCIPAL_NAME);
+                                for (int i = 0; i < vs.length; i++) {
+                                    Node groupNode = session.getNodeByUUID(vs[i].getString());
+                                    String pName = groupNode.getProperty(princName).getString();
+                                    if (userAdminGroup.equals(pName)) {
+                                        isUserAdmin = true;
+                                    } else if (groupAdminGroup.equals(pName)) {
+                                        isGroupAdmin = true;
+                                    }
+                                }
+                                break;
+                            // default: other events are not relevant.
+                        }
+                        // invalidate the cached results
                         clearCache();
                         // only need to clear the cache once. stop processing
                         break;

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserConstants.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserConstants.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserConstants.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserConstants.java Fri Apr 25 08:11:49 2008
@@ -52,7 +52,7 @@
     Name P_USERID = NF.create(Name.NS_REP_URI, "userId");
     Name P_PASSWORD = NF.create(Name.NS_REP_URI, "password");
 
-    Name P_MEMBERS = NF.create(Name.NS_REP_URI, "members");
+    Name P_GROUPS = NF.create(Name.NS_REP_URI, "groups");
 
     /**
      * Name of the user property containing the principal names of those allowed

Modified: jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.cnd
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.cnd?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.cnd (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.cnd Fri Apr 25 08:11:49 2008
@@ -205,6 +205,7 @@
   + * (rep:AuthorizableFolder) = rep:AuthorizableFolder protected version
   - rep:principalName (string) protected mandatory
   - rep:referees (string) protected multiple
+  - rep:groups (reference) protected multiple < 'rep:Group'
   - * (undefined)
   - * (undefined) multiple
 
@@ -217,8 +218,6 @@
   - rep:password (string) mandatory
 
 [rep:Group] > rep:Authorizable
-  - rep:members (reference) protected multiple
-    < 'rep:Authorizable'
 
 [rep:AuthorizableFolder] > nt:base, mix:referenceable
   + * (rep:Authorizable) = rep:User protected version

Modified: jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.xml
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.xml?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.xml (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/resources/org/apache/jackrabbit/core/nodetype/builtin_nodetypes.xml Fri Apr 25 08:11:49 2008
@@ -476,6 +476,11 @@
         </childNodeDefinition>
         <propertyDefinition name="rep:principalName" requiredType="String" autoCreated="false" mandatory="true" onParentVersion="COPY" protected="true" multiple="false" />
         <propertyDefinition name="rep:referees" requiredType="String" autoCreated="false" mandatory="false" onParentVersion="COPY" protected="true" multiple="true" />
+        <propertyDefinition name="rep:groups" requiredType="Reference" autoCreated="false" onParentVersion="COPY" protected="true" multiple="true">
+            <valueConstraints>
+                <valueConstraint>rep:Group</valueConstraint>
+            </valueConstraints>
+        </propertyDefinition>
         <propertyDefinition name="*" requiredType="undefined" autoCreated="false" mandatory="false" onParentVersion="COPY" protected="false" multiple="false" />
         <propertyDefinition name="*" requiredType="undefined" autoCreated="false" mandatory="false" onParentVersion="COPY" protected="false" multiple="true" />
     </nodeType>
@@ -497,11 +502,6 @@
         <supertypes>
             <supertype>rep:Authorizable</supertype>
         </supertypes>
-        <propertyDefinition name="rep:members" requiredType="Reference" autoCreated="false" onParentVersion="COPY" protected="true" multiple="true">
-            <valueConstraints>
-                <valueConstraint>rep:Authorizable</valueConstraint>
-            </valueConstraints>
-        </propertyDefinition>
     </nodeType>
 
     <nodeType name="rep:AuthorizableFolder" isMixin="false" hasOrderableChildNodes="false" primaryItemName="">

Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/AuthorizableTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/AuthorizableTest.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/AuthorizableTest.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/AuthorizableTest.java Fri Apr 25 08:11:49 2008
@@ -194,6 +194,16 @@
         }
     }
 
+    public void testDeclaredMemberOf() throws NotExecutableException, RepositoryException {
+        Authorizable auth = getTestUser(superuser);
+
+        Iterator it = auth.declaredMemberOf();
+        while (it.hasNext()) {
+            Object group = it.next();
+            assertTrue(group instanceof Group);
+        }
+    }
+
     public void testAddReferee() throws NotExecutableException, RepositoryException {
         Authorizable auth = getTestUser(superuser);
 

Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java Fri Apr 25 08:11:49 2008
@@ -22,6 +22,8 @@
 
 import javax.jcr.RepositoryException;
 import java.util.Iterator;
+import java.util.List;
+import java.util.ArrayList;
 
 /**
  * <code>GroupTest</code>...
@@ -30,9 +32,29 @@
 
     private static Logger log = LoggerFactory.getLogger(GroupTest.class);
 
-    private static void assertTrueMemberOfContainsGroup(Authorizable auth, Group gr) throws RepositoryException {
+    private static void assertTrueIsMember(Iterator members, Authorizable auth) throws RepositoryException {
         boolean contained = false;
-        for (Iterator groups = auth.memberOf(); groups.hasNext() && !contained;) {
+        while (members.hasNext() && !contained) {
+            Object next = members.next();
+            assertTrue(next instanceof Authorizable);
+            contained = ((Authorizable)next).getID().equals(auth.getID());
+        }
+        assertTrue("The given set of members must contain " + auth.getID(), contained);
+    }
+
+    private static void assertFalseIsMember(Iterator members, Authorizable auth) throws RepositoryException {
+        boolean contained = false;
+        while (members.hasNext() && !contained) {
+            Object next = members.next();
+            assertTrue(next instanceof Authorizable);
+            contained = ((Authorizable)next).getID().equals(auth.getID());
+        }
+        assertFalse("The given set of members must not contain " + auth.getID(), contained);
+    }
+
+    private static void assertTrueMemberOfContainsGroup(Iterator groups, Group gr) throws RepositoryException {
+        boolean contained = false;
+        while (groups.hasNext() && !contained) {
             Object next = groups.next();
             assertTrue(next instanceof Group);
             contained = ((Group)next).getID().equals(gr.getID());
@@ -40,9 +62,9 @@
         assertTrue("All members of a group must contain that group upon 'memberOf'.", contained);
     }
 
-    private static void assertFalseMemberOfContainsGroup(Authorizable auth, Group gr) throws RepositoryException {
+    private static void assertFalseMemberOfContainsGroup(Iterator groups, Group gr) throws RepositoryException {
         boolean contained = false;
-        for (Iterator groups = auth.memberOf(); groups.hasNext() && !contained;) {
+        while (groups.hasNext() && !contained) {
             Object next = groups.next();
             assertTrue(next instanceof Group);
             contained = ((Group)next).getID().equals(gr.getID());
@@ -55,14 +77,20 @@
         assertTrue(gr.isGroup());
     }
 
-    public void testGetMembersNotNull() throws NotExecutableException, RepositoryException {
+    public void testGetDeclaredMembers() throws NotExecutableException, RepositoryException {
         Group gr = getTestGroup(superuser);
-        assertNotNull(gr.getMembers());
+        Iterator it = gr.getDeclaredMembers();
+        assertNotNull(it);
+        while (it.hasNext()) {
+            assertTrue(it.next() instanceof Authorizable);
+        }
     }
 
-    public void testGetMembersAreAuthorizable() throws NotExecutableException, RepositoryException {
+    public void testGetMembers() throws NotExecutableException, RepositoryException {
         Group gr = getTestGroup(superuser);
-        for (Iterator it = gr.getMembers(); it.hasNext();) {
+        Iterator it = gr.getMembers();
+        assertNotNull(it);
+        while (it.hasNext()) {
             assertTrue(it.next() instanceof Authorizable);
         }
     }
@@ -83,7 +111,29 @@
         Iterator it = gr.getMembers();
         while (it.hasNext()) {
             Authorizable auth = (Authorizable) it.next();
-            assertTrueMemberOfContainsGroup(auth, gr);
+            assertTrueMemberOfContainsGroup(auth.memberOf(), gr);
+        }
+    }
+
+    public void testGetDeclaredMembersAgainstDeclaredMemberOf() throws NotExecutableException, RepositoryException {
+        Group gr = getTestGroup(superuser);
+
+        Iterator it = gr.getDeclaredMembers();
+        while (it.hasNext()) {
+            Authorizable auth = (Authorizable) it.next();
+            assertTrueMemberOfContainsGroup(auth.declaredMemberOf(), gr);
+        }
+    }
+
+    public void testGetMembersContainsDeclaredMembers() throws NotExecutableException, RepositoryException {
+        Group gr = getTestGroup(superuser);
+        List l = new ArrayList();
+        for (Iterator it = gr.getMembers(); it.hasNext();) {
+            l.add(((Authorizable) it.next()).getID());
+        }
+        for (Iterator it = gr.getDeclaredMembers(); it.hasNext();) {
+            assertTrue("All declared members must also be part of the Iterator " +
+                    "returned upon getMembers()",l.contains(((Authorizable) it.next()).getID()));
         }
     }
 
@@ -132,9 +182,10 @@
         try {
             newGroup = userMgr.createGroup(getTestPrincipal());
 
-            assertFalseMemberOfContainsGroup(auth, newGroup);
+            assertFalseMemberOfContainsGroup(auth.memberOf(), newGroup);
             assertTrue(newGroup.addMember(auth));
-            assertTrueMemberOfContainsGroup(auth, newGroup);
+            assertTrueMemberOfContainsGroup(auth.declaredMemberOf(), newGroup);
+            assertTrueMemberOfContainsGroup(auth.memberOf(), newGroup);
         } finally {
             if (newGroup != null) {
                 newGroup.removeMember(auth);
@@ -143,6 +194,60 @@
         }
     }
 
+    public void testAddMemberModifiesGetMembers() throws NotExecutableException, RepositoryException {
+        User auth = getTestUser(superuser);
+        Group newGroup = null;
+        try {
+            newGroup = userMgr.createGroup(getTestPrincipal());
+
+            assertFalseIsMember(newGroup.getMembers(), auth);
+            assertFalseIsMember(newGroup.getDeclaredMembers(), auth);
+            assertTrue(newGroup.addMember(auth));
+            assertTrueIsMember(newGroup.getMembers(), auth);
+            assertTrueIsMember(newGroup.getDeclaredMembers(), auth);
+        } finally {
+            if (newGroup != null) {
+                newGroup.removeMember(auth);
+                newGroup.remove();
+            }
+        }
+    }
+
+    public void testIndirectMembers() throws NotExecutableException, RepositoryException {
+        User auth = getTestUser(superuser);
+        Group newGroup = null;
+        Group newGroup2 = null;
+        try {
+            newGroup = userMgr.createGroup(getTestPrincipal());
+            newGroup2 = userMgr.createGroup(getTestPrincipal());
+
+            newGroup.addMember(newGroup2);
+            assertTrue(newGroup.isMember(newGroup2));
+
+            newGroup2.addMember(auth);
+
+            // testuser must not be declared member of 'newGroup'
+            assertFalseIsMember(newGroup.getDeclaredMembers(), auth);
+            assertFalseMemberOfContainsGroup(auth.declaredMemberOf(), newGroup);
+
+            // testuser must however be member of 'newGroup' (indirect).
+            assertTrueIsMember(newGroup.getMembers(), auth);
+            assertTrueMemberOfContainsGroup(auth.memberOf(), newGroup);
+
+            // testuser cannot be removed from 'newGroup'
+            assertFalse(newGroup.removeMember(auth));
+        } finally {
+            if (newGroup != null) {
+                newGroup.removeMember(newGroup2);
+                newGroup.remove();
+            }
+            if (newGroup2 != null) {
+                newGroup2.removeMember(auth);
+                newGroup2.remove();
+            }
+        }
+    }
+
     public void testRemoveMemberTwice() throws NotExecutableException, RepositoryException {
         User auth = getTestUser(superuser);
         Group newGroup = null;
@@ -174,12 +279,14 @@
     }
 
     /**
-     * Removing a GroupImpl must be possible even if there are still exiting
+     * TODO: uncomment once membership-relation is stored as weak ref.
+     * Removing a GroupImpl must be possible even if there are still existing
      * members present.
      *
      * @throws RepositoryException
      * @throws NotExecutableException
      */
+    /*
     public void testRemoveGroupIfMemberExist() throws RepositoryException, NotExecutableException {
         User auth = getTestUser(superuser);
         String newGroupId = null;
@@ -199,4 +306,5 @@
             }
         }
     }
+    */
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/AuthorizableImplTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/AuthorizableImplTest.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/AuthorizableImplTest.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/AuthorizableImplTest.java Fri Apr 25 08:11:49 2008
@@ -19,9 +19,12 @@
 import org.apache.jackrabbit.api.security.user.AbstractUserTest;
 import org.apache.jackrabbit.api.security.user.Group;
 import org.apache.jackrabbit.api.security.user.User;
+import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.core.NodeImpl;
+import org.apache.jackrabbit.core.SessionImpl;
 import org.apache.jackrabbit.test.NotExecutableException;
 import org.apache.jackrabbit.value.StringValue;
+import org.apache.jackrabbit.spi.commons.conversion.NameResolver;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -29,6 +32,9 @@
 import javax.jcr.RepositoryException;
 import javax.jcr.Value;
 import javax.jcr.nodetype.ConstraintViolationException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Iterator;
 
 /**
  * <code>AuthorizableImplTest</code>...
@@ -37,79 +43,87 @@
 
     private static Logger log = LoggerFactory.getLogger(AuthorizableImplTest.class);
 
+    private List protectedUserProps = new ArrayList();
+    private List protectedGroupProps = new ArrayList();
+
+    protected void setUp() throws Exception {
+        super.setUp();
+
+        if (superuser instanceof SessionImpl) {
+            NameResolver resolver = ((SessionImpl) superuser).getNamePathResolver();
+            protectedUserProps.add(resolver.getJCRName(UserConstants.P_USERID));
+            protectedUserProps.add(resolver.getJCRName(UserConstants.P_GROUPS));
+            protectedUserProps.add(resolver.getJCRName(UserConstants.P_IMPERSONATORS));
+            protectedUserProps.add(resolver.getJCRName(UserConstants.P_PRINCIPAL_NAME));
+            protectedUserProps.add(resolver.getJCRName(UserConstants.P_REFEREES));
+
+            protectedGroupProps.add(resolver.getJCRName(UserConstants.P_PRINCIPAL_NAME));
+            protectedGroupProps.add(resolver.getJCRName(UserConstants.P_REFEREES));
+        } else {
+            throw new NotExecutableException();
+        }
+    }
+
     private static void checkProtected(Property prop) throws RepositoryException {
         assertTrue(prop.getDefinition().isProtected());
     }
 
-    public void testSetSpecialProperties() throws NotExecutableException, RepositoryException {
-        Value v = superuser.getValueFactory().createValue("any_value");
-        User u = getTestUser(superuser);
+    public void testRemoveAdmin() {
+        String adminID = superuser.getUserID();
         try {
-            u.setProperty("rep:userId", v);
-            fail("changing the user id should fail.");
+            Authorizable admin = userMgr.getAuthorizable(adminID);
+            admin.remove();
+            fail("The admin user cannot be removed.");
         } catch (RepositoryException e) {
-            // success
+            // OK superuser cannot be removed. not even by the superuser itself.
         }
+    }
 
-        try {
-            u.setProperty("rep:principalName", v);
-            fail("changing the principalName should fail.");
-        } catch (RepositoryException e) {
-            // success
-        }
+    public void testSetSpecialProperties() throws NotExecutableException, RepositoryException {
+        Value v = superuser.getValueFactory().createValue("any_value");
+        User u = getTestUser(superuser);
 
-        try {
-            Value[] vs = new Value[] {v};
-            u.setProperty("rep:referees", vs);
-            fail("changing the referees property should fail.");
-        } catch (RepositoryException e) {
-            // success
+        for (Iterator it = protectedUserProps.iterator(); it.hasNext();) {
+            String pName = it.next().toString();
+            try {
+                u.setProperty(pName, v);
+                fail("changing the '" +pName+ "' property on a User should fail.");
+            } catch (RepositoryException e) {
+                // success
+            }
         }
-
         Group g = getTestGroup(superuser);
-        try {
-            g.setProperty("rep:principalName", v);
-            fail("changing the principalName should fail.");
-        } catch (RepositoryException e) {
-            // success
-        }
-        try {
-            Value[] vs = new Value[] {v};
-            g.setProperty("rep:members", vs);
-            fail("changing the members property should fail.");
-        } catch (RepositoryException e) {
-            // success
+        for (Iterator it = protectedGroupProps.iterator(); it.hasNext();) {
+            String pName = it.next().toString();
+            try {
+                u.setProperty(pName, v);
+                fail("changing the '" +pName+ "' property on a Group should fail.");
+            } catch (RepositoryException e) {
+                // success
+            }
         }
     }
 
     public void testRemoveSpecialProperties() throws NotExecutableException, RepositoryException {
         User u = getTestUser(superuser);
-        try {
-            u.removeProperty("rep:userId");
-            fail("removing the user id should fail.");
-        } catch (RepositoryException e) {
-            // success
-        }
-
-        try {
-            u.removeProperty("rep:principalName");
-            fail("removing the principalName should fail.");
-        } catch (RepositoryException e) {
-            // success
+        for (Iterator it = protectedUserProps.iterator(); it.hasNext();) {
+            String pName = it.next().toString();
+            try {
+                u.removeProperty(pName);
+                fail("removing the '" +pName+ "' property on a User should fail.");
+            } catch (RepositoryException e) {
+                // success
+            }
         }
-
         Group g = getTestGroup(superuser);
-        try {
-            g.removeProperty("rep:principalName");
-            fail("removing the principalName should fail.");
-        } catch (RepositoryException e) {
-            // success
-        }
-        try {
-            g.removeProperty("rep:members");
-            fail("removing the members property should fail.");
-        } catch (RepositoryException e) {
-            // success
+        for (Iterator it = protectedGroupProps.iterator(); it.hasNext();) {
+            String pName = it.next().toString();
+            try {
+                u.removeProperty(pName);
+                fail("removing the '" +pName+ "' property on a Group should fail.");
+            } catch (RepositoryException e) {
+                // success
+            }
         }
     }
 
@@ -122,6 +136,9 @@
         if (n.hasProperty(UserConstants.P_REFEREES)) {
            checkProtected(n.getProperty(UserConstants.P_REFEREES));
         }
+        if (n.hasProperty(UserConstants.P_GROUPS)) {
+            checkProtected(n.getProperty(UserConstants.P_GROUPS));
+        }
         if (n.hasProperty(UserConstants.P_IMPERSONATORS)) {
            checkProtected(n.getProperty(UserConstants.P_IMPERSONATORS));
         }
@@ -132,8 +149,8 @@
         NodeImpl n = gr.getNode();
 
         checkProtected(n.getProperty(UserConstants.P_PRINCIPAL_NAME));
-        if (n.hasProperty(UserConstants.P_MEMBERS)) {
-           checkProtected(n.getProperty(UserConstants.P_MEMBERS));
+        if (n.hasProperty(UserConstants.P_GROUPS)) {
+            checkProtected(n.getProperty(UserConstants.P_GROUPS));
         }
         if (n.hasProperty(UserConstants.P_REFEREES)) {
            checkProtected(n.getProperty(UserConstants.P_REFEREES));

Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/GroupAdministratorTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/GroupAdministratorTest.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/GroupAdministratorTest.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/GroupAdministratorTest.java Fri Apr 25 08:11:49 2008
@@ -70,16 +70,16 @@
         p = getTestPrincipal();
         childUID = userMgr.createUser(p.getName(), buildCredentials(p), p, uPath).getID();
 
-        // make other user a user-administrator:
-        Authorizable ua = userMgr.getAuthorizable(UserConstants.GROUP_ADMIN_GROUP_NAME);
-        if (ua == null || !ua.isGroup()) {
+        // make other user a group-administrator:
+        Authorizable groupAdmin = userMgr.getAuthorizable(UserConstants.GROUP_ADMIN_GROUP_NAME);
+        if (groupAdmin == null || !groupAdmin.isGroup()) {
             throw new NotExecutableException("Cannot execute test. Group-Admin name has been changed by config.");
         }
-        Group grAdministrators = (Group) ua;
+        Group grAdministrators = (Group) groupAdmin;
         grAdministrators.addMember(u);
         groupID = grAdministrators.getID();
 
-        // create a session for the other user.
+        // create a session for the grou-admin user.
         uSession = helper.getRepository().login(creds);
     }
 
@@ -167,10 +167,71 @@
         Authorizable cU = umgr.getAuthorizable(childUID);
         Group gr = (Group) umgr.getAuthorizable(groupID);
 
-        // adding and removing a child user from a group the group-admin
-        // is member of must succeed.
-        assertTrue(gr.addMember(cU));
-        assertTrue(gr.removeMember(cU));
+        // adding and removing the child-user as member of a group not
+        // succeed as long editing session is not user-admin.
+        try {
+            assertFalse(gr.addMember(cU));
+        } catch (AccessDeniedException e) {
+            // ok
+        } finally {
+            gr.removeMember(cU);
+        }
+    }
+
+    public void testAddChildToGroup2() throws RepositoryException, NotExecutableException {
+        UserManager umgr = getUserManager(uSession);
+        Authorizable cU = umgr.getAuthorizable(childUID);
+
+        Authorizable auth = umgr.getAuthorizable(UserConstants.USER_ADMIN_GROUP_NAME);
+        if (auth == null || !auth.isGroup()) {
+            throw new NotExecutableException("Cannot execute test. User-Admin name has been changed by config.");
+        }
+        Group userAdmin = (Group)auth;
+        User self = (User) umgr.getAuthorizable(uID);
+        try {
+            assertTrue(userAdmin.addMember(self));
+
+            Group gr = (Group) umgr.getAuthorizable(groupID);
+            assertTrue(gr.addMember(cU));
+            assertTrue(gr.removeMember(cU));
+        } finally {
+            userAdmin.removeMember(self);
+        }
+    }
+
+    public void testAddMembersToCreatedGroup() throws RepositoryException, NotExecutableException {
+        UserManager umgr = getUserManager(uSession);
+        Authorizable auth = umgr.getAuthorizable(UserConstants.USER_ADMIN_GROUP_NAME);
+        if (auth == null || !auth.isGroup()) {
+            throw new NotExecutableException("Cannot execute test. User-Admin name has been changed by config.");
+        }
+        Group userAdmin = (Group) auth;
+        Group testGroup = null;
+        User self = (User) umgr.getAuthorizable(uID);
+        try {
+            // let groupadmin create a new group
+            testGroup = umgr.createGroup(getTestPrincipal(), "/a/b/c/d");
+
+            // editing session adds itself to that group -> must succeed.
+            assertTrue(testGroup.addMember(self));
+
+            // editing session adds itself to user-admin group
+            userAdmin.addMember(self);
+            assertTrue(userAdmin.isMember(self));
+
+            // add child-user to test group
+            Authorizable testUser = umgr.getAuthorizable(childUID);
+            assertFalse(testGroup.isMember(testUser));
+            assertTrue(testGroup.addMember(testUser));
+        } finally {
+            if (testGroup != null) {
+                for (Iterator it = testGroup.getDeclaredMembers(); it.hasNext();) {
+                    testGroup.removeMember((Authorizable) it.next());
+                }
+                testGroup.remove();
+            }
+            userAdmin.removeMember(self);            
+        }
     }
 
     public void testAddMemberToForeignGroup() throws RepositoryException, NotExecutableException {
@@ -199,10 +260,39 @@
         Authorizable pU = umgr.getAuthorizable(parentUID);
         Group gr = (Group) umgr.getAuthorizable(groupID);
 
-        // adding and removing the 'parent' user to/from a group the group-admin
-        // is member of must succeed.
-        assertTrue(gr.addMember(pU));
-        assertTrue(gr.removeMember(pU));
+        // adding and removing the parent-user as member of a group must
+        // never succeed.
+        try {
+            assertFalse(gr.addMember(pU));
+        } catch (AccessDeniedException e) {
+            // ok
+        } finally {
+            gr.removeMember(pU);
+        }
+
+        // ... even if the editing user becomes member of the user-admin group
+        Group uAdministrators = null;
+        try {
+            Authorizable userAdmin = userMgr.getAuthorizable(UserConstants.USER_ADMIN_GROUP_NAME);
+            if (userAdmin == null || !userAdmin.isGroup()) {
+                throw new NotExecutableException("Cannot execute test. User-Admin name has been changed by config.");
+            }
+            uAdministrators = (Group) userAdmin;
+            uAdministrators.addMember(userMgr.getAuthorizable(uID));
+
+            try {
+                assertFalse(gr.addMember(pU));
+                gr.removeMember(pU);
+            } catch (AccessDeniedException e) {
+                // fine as well.
+            }
+        } finally {
+            // let superuser do the clean up.
+            // remove testuser from u-admin group again.
+            if (uAdministrators != null) {
+                uAdministrators.removeMember(userMgr.getAuthorizable(uID));
+            }
+        }
     }
 
     public void testAddOwnAuthorizableAsGroupAdmin() throws RepositoryException, NotExecutableException {
@@ -235,15 +325,9 @@
             throw new RepositoryException("Test user is already member -> cannot execute.");
         }
 
-        try {
-            String msg = "GroupAdmin cannot add its own authorizable to a group she/he is not yet member of.";
-            assertFalse(msg, uadminGr.addMember(user));
-
-            // didn't throw -> clean up.
-            uadminGr.removeMember(user);
-        } catch (AccessDeniedException e) {
-            // fine as well
-        }
+        String msg = "GroupAdmin must be able to add its own authorizable to a group she/he is not yet member of.";
+        assertTrue(msg, uadminGr.addMember(user));
+        assertTrue(msg, uadminGr.removeMember(user));
     }
 
     public void testRemoveMembersOfForeignGroup() throws RepositoryException, NotExecutableException {
@@ -263,12 +347,12 @@
 
             Group gr = (Group) getUserManager(uSession).getAuthorizable(nGr.getID());
 
-            // removing any member must fail since uSession is not member of that group.
+            // removing any member must fail unless the testuser is user-admin
             Iterator it = gr.getMembers();
             if (it.hasNext()) {
                 Authorizable auth = (Authorizable) it.next();
 
-                String msg = "GroupAdmin cannot remove members of a group itself is not member of.";
+                String msg = "GroupAdmin cannot remove members of other user unless he/she is user-admin.";
                 assertFalse(msg, gr.removeMember(auth));
             } else {
                 throw new RepositoryException("Must contain members....");
@@ -302,11 +386,11 @@
             Group gr = (Group) getUserManager(uSession).getAuthorizable(nGr.getID());
 
             // since only 1 single member -> removal rather than modification.
-            // since uSession is not member this must fail.
+            // since uSession is not user-admin this must fail.
             for (Iterator it = gr.getMembers(); it.hasNext();) {
                 Authorizable auth = (Authorizable) it.next();
 
-                String msg = "GroupAdmin cannot remove members of a group itself is not member of.";
+                String msg = "GroupAdmin cannot remove members of groups unless he/she is UserAdmin.";
                 assertFalse(gr.removeMember(auth));
             }
         } catch (AccessDeniedException e) {

Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/UserAdministratorTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/UserAdministratorTest.java?rev=651624&r1=651623&r2=651624&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/UserAdministratorTest.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/UserAdministratorTest.java Fri Apr 25 08:11:49 2008
@@ -143,8 +143,6 @@
         }
     }
 
-    // TODO: uncomment as soon as group-members are stored as weak references
-    /*
     public void testRemoveHimSelf() throws RepositoryException, NotExecutableException {
         UserManager umgr = getUserManager(otherSession);
 
@@ -156,7 +154,6 @@
             // success
         }
     }
-    */
 
     public void testRemoveParentUser() throws RepositoryException, NotExecutableException {
         UserManager umgr = getUserManager(otherSession);



Mime
View raw message