You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jackrabbit.apache.org by an...@apache.org on 2009/10/22 19:26:39 UTC

svn commit: r828791 [2/8] - in /jackrabbit/trunk: jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/user/ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/ jackrabbi...

Added: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/UserManagerConfig.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/UserManagerConfig.java?rev=828791&view=auto
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/UserManagerConfig.java (added)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/UserManagerConfig.java Thu Oct 22 17:26:37 2009
@@ -0,0 +1,92 @@
+/*
+ * 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.core.config;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.apache.jackrabbit.api.security.user.UserManager;
+
+import java.lang.reflect.Constructor;
+
+/**
+ * User manager configuration. This bean configuration class is used to
+ * create user manager objects.
+ * <p>
+ * This configuration is an optional part of the SecurityManager configuration.
+ *
+ * @see org.apache.jackrabbit.core.config.SecurityManagerConfig#getUserManagerConfig()
+ */
+public class UserManagerConfig extends BeanConfig {
+
+    /**
+     * logger instance
+     */
+    private static final Logger log = LoggerFactory.getLogger(UserManagerConfig.class);
+
+    private Constructor<?> constr;
+
+    public UserManagerConfig(BeanConfig<?> config) {
+        super(config);
+        setValidate(false); // omit validation of the config properties
+    }
+
+    /**
+     * Build a new <code>UserManager</code> instance based on this configuration.
+     * Since the initial requirement for the User Management was to allow for
+     * implementations that don't store and retrieve user information from
+     * repository content, the otherwise used <code>init</code> interface method
+     * has intentionally be omitted. This method attempts to retrieve a
+     * constructor matching the given <code>parameterTypes</code> and creates
+     * an new instance from the <code>initArgs</code> matching the
+     * <code>parameterTypes</code>.
+     * 
+     * @param assignablefrom An UserManager class from which the configured
+     * implementation must be assignable.
+     * @param parameterTypes Array of classes used to lookup the constructor.
+     * @param initArgs The arguments to create the new user manager instance
+     * matching the <code>parameterTypes</code>.
+     * @return A new instance of <code>UserManager</code> that is assignable from
+     * the class passed as <code>assignablefrom</code>.
+     * @throws ConfigurationException If the configured user manager implementation
+     * is not assignable from the given UserManager class, does not provide
+     * a constructor matching <code>parameterTypes</code> or creating the instance
+     * fails.
+     */
+    public UserManager getUserManager(Class<? extends UserManager> assignablefrom, Class<?>[] parameterTypes, Object... initArgs) throws ConfigurationException {
+        if (constr == null) {
+            String msg = "Invalid UserManager implementation '" + getClassName() + "'.";
+            try {
+                Class<?> umgrCl = Class.forName(getClassName(), true, getClassLoader());
+                if (assignablefrom.isAssignableFrom(umgrCl)) {
+                    constr = umgrCl.getConstructor(parameterTypes);
+                } else {
+                    throw new ConfigurationException("Configured UserManager '" + getClassName() + "' is not assignable from " + assignablefrom);
+                }
+            } catch (ClassNotFoundException e) {
+                throw new ConfigurationException(msg, e);
+            } catch (NoSuchMethodException e) {
+                throw new ConfigurationException(msg, e);
+            }
+        }
+
+        try {
+            return (UserManager) constr.newInstance(initArgs);
+        } catch (Exception e) {
+            throw new ConfigurationException("Invalid UserManager implementation '" + getClassName() + "'.", e);
+        }
+    }
+}
\ No newline at end of file

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/WorkspaceConfig.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/WorkspaceConfig.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/WorkspaceConfig.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/config/WorkspaceConfig.java Thu Oct 22 17:26:37 2009
@@ -79,6 +79,15 @@
     private final WorkspaceSecurityConfig workspaceSecurityConfig;
 
     /**
+     * Optional configuration for the xml import behavior. Up to now this consists
+     * of a single configuration point: the treatment
+     * of protected nodes and properties that is defined by a set of classes
+     * implementing {@link org.apache.jackrabbit.core.xml.ProtectedNodeImporter}
+     * or {@link org.apache.jackrabbit.core.xml.ProtectedPropertyImporter}.
+     */
+    private final ImportConfig importConfig;
+
+    /**
      * Creates a workspace configuration object.
      *
      * @param home home directory
@@ -95,6 +104,27 @@
                            QueryHandlerFactory qhf,
                            ISMLockingFactory ismLockingFactory,
                            WorkspaceSecurityConfig workspaceSecurityConfig) {
+        this(home, name, clustered, fsf, pmc, qhf, ismLockingFactory, workspaceSecurityConfig, null);
+    }
+
+    /**
+     * Creates a workspace configuration object.
+     *
+     * @param home home directory
+     * @param name workspace name
+     * @param clustered
+     * @param fsf file system factory
+     * @param pmc persistence manager configuration
+     * @param qhf query handler factory, or <code>null</code> if not configured
+     * @param ismLockingFactory the item state manager locking factory
+     * @param workspaceSecurityConfig the workspace specific security configuration.
+     */
+    public WorkspaceConfig(String home, String name, boolean clustered,
+                           FileSystemFactory fsf, PersistenceManagerConfig pmc,
+                           QueryHandlerFactory qhf,
+                           ISMLockingFactory ismLockingFactory,
+                           WorkspaceSecurityConfig workspaceSecurityConfig,
+                           ImportConfig importConfig) {
         this.home = home;
         this.name = name;
         this.clustered = clustered;
@@ -103,6 +133,7 @@
         this.qhf = qhf;
         this.ismLockingFactory = ismLockingFactory;
         this.workspaceSecurityConfig = workspaceSecurityConfig;
+        this.importConfig = importConfig;
     }
 
     /**
@@ -193,4 +224,11 @@
     public WorkspaceSecurityConfig getSecurityConfig() {
         return workspaceSecurityConfig;
     }
+
+    /**
+     * @return xml import settings
+     */
+    public ImportConfig getImportConfig() {
+        return importConfig;
+    }
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/JackrabbitSecurityManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/JackrabbitSecurityManager.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/JackrabbitSecurityManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/JackrabbitSecurityManager.java Thu Oct 22 17:26:37 2009
@@ -52,11 +52,12 @@
      *
      * @param creds
      * @param subject
+     * @param workspaceName The name of the workspace to login.
      * @return A new <code>AuthContext</code> for the given <code>creds</code>
      * and <code>subject</code>.
      * @throws RepositoryException
      */
-    AuthContext getAuthContext(Credentials creds, Subject subject) throws RepositoryException;
+    AuthContext getAuthContext(Credentials creds, Subject subject, String workspaceName) throws RepositoryException;
 
     /**
      * Retrieve the <code>AccessManager</code> for the given <code>session</code>.
@@ -95,8 +96,9 @@
      * the specified subject.
      *
      * @param subject
+     * @param workspaceName
      * @return userID to be displayed upon {@link Session#getUserID()}.
      * @throws RepositoryException
      */
-    String getUserID(Subject subject) throws RepositoryException;
+    String getUserID(Subject subject, String workspaceName) throws RepositoryException;
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/WorkspaceAccessManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/WorkspaceAccessManager.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/WorkspaceAccessManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/WorkspaceAccessManager.java Thu Oct 22 17:26:37 2009
@@ -31,10 +31,10 @@
     /**
      * Initialize this <code>WorkspaceAccessManager</code>.
      *
-     * @param securitySession The security session.
+     * @param systemSession Session used to initialize this instance.
      * @throws RepositoryException if an error occurs.
      */
-    void init(Session securitySession) throws RepositoryException;
+    void init(Session systemSession) throws RepositoryException;
 
     /**
      * Dispose this <code>WorkspaceAccessManager</code> and its resources.

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLProvider.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLProvider.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLProvider.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLProvider.java Thu Oct 22 17:26:37 2009
@@ -17,14 +17,15 @@
 package org.apache.jackrabbit.core.security.authorization.acl;
 
 import java.security.Principal;
+import java.security.acl.Group;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.Iterator;
 
 import javax.jcr.ItemNotFoundException;
 import javax.jcr.NodeIterator;
@@ -33,8 +34,6 @@
 import javax.jcr.Value;
 import javax.jcr.observation.Event;
 import javax.jcr.observation.EventIterator;
-import javax.jcr.query.Query;
-import javax.jcr.query.QueryManager;
 import javax.jcr.security.AccessControlEntry;
 import javax.jcr.security.AccessControlList;
 import javax.jcr.security.AccessControlManager;
@@ -53,7 +52,6 @@
 import org.apache.jackrabbit.core.security.authorization.AbstractCompiledPermissions;
 import org.apache.jackrabbit.core.security.authorization.AccessControlConstants;
 import org.apache.jackrabbit.core.security.authorization.AccessControlEditor;
-import org.apache.jackrabbit.core.security.authorization.AccessControlEntryIterator;
 import org.apache.jackrabbit.core.security.authorization.CompiledPermissions;
 import org.apache.jackrabbit.core.security.authorization.Permission;
 import org.apache.jackrabbit.core.security.authorization.PrivilegeRegistry;
@@ -62,6 +60,7 @@
 import org.apache.jackrabbit.spi.Path;
 import org.apache.jackrabbit.spi.commons.name.PathFactoryImpl;
 import org.apache.jackrabbit.util.Text;
+import org.apache.commons.collections.iterators.IteratorChain;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -103,15 +102,9 @@
      */
     private NodeId rootNodeId;
 
-    /**
-     * Flag indicating whether or not this provider should be create the default
-     * ACLs upon initialization.
-     */
-    private boolean initializedWithDefaults;
-
     //-------------------------------------------------< AccessControlUtils >---
     /**
-     * @see AbstractAccessControlProvider#isAcItem(Path)
+     * @see org.apache.jackrabbit.core.security.authorization.AccessControlUtils#isAcItem(Path)
      */
     public boolean isAcItem(Path absPath) throws RepositoryException {
         Path.Element[] elems = absPath.getElements();
@@ -125,7 +118,7 @@
 
     /**
      * Test if the given node is itself a rep:ACL or a rep:ACE node.
-     * @see AbstractAccessControlProvider#isAcItem(ItemImpl)
+     * @see org.apache.jackrabbit.core.security.authorization.AccessControlUtils#isAcItem(ItemImpl)
      */
     public boolean isAcItem(ItemImpl item) throws RepositoryException {
         NodeImpl n = ((item.isNode()) ? (NodeImpl) item : (NodeImpl) item.getParent());
@@ -136,6 +129,7 @@
     /**
      * @see org.apache.jackrabbit.core.security.authorization.AccessControlProvider#init(Session, Map)
      */
+    @Override
     public void init(Session systemSession, Map configuration) throws RepositoryException {
         super.init(systemSession, configuration);
 
@@ -144,7 +138,8 @@
         NodeImpl root = (NodeImpl) session.getRootNode();
         rootNodeId = root.getNodeId();
         systemEditor = new ACLEditor(systemSession, this);
-        initializedWithDefaults = !configuration.containsKey(PARAM_OMIT_DEFAULT_PERMISSIONS);
+        // TODO: replace by configurable default policy (see JCR-2331)
+        boolean initializedWithDefaults = !configuration.containsKey(PARAM_OMIT_DEFAULT_PERMISSIONS);
         if (initializedWithDefaults && !isAccessControlled(root)) {
             initRootACL(session, systemEditor);
         }
@@ -208,8 +203,21 @@
         }
     }
 
-    //------------------------------------------------------------< private >---
+    //----------------------------------------------------------< protected >---
+    /**
+     * Retrieve an iterator of <code>AccessControlEntry</code> to be evaluated
+     * upon {@link AbstractCompiledPermissions#buildResult}.
+     *
+     * @param node Target node.
+     * @param principalNames List of principal names.
+     * @return an iterator of <code>AccessControlEntry</code>.
+     * @throws RepositoryException If an error occurs.
+     */
+    protected Iterator<AccessControlEntry> retrieveResultEntries(NodeImpl node, List<String> principalNames) throws RepositoryException {
+        return new Entries(node, principalNames).iterator();
+    }
 
+    //------------------------------------------------------------< private >---
     /**
      * Returns the given <code>targetNode</code> unless the node itself stores
      * access control information in which case it's nearest non-ac-parent is
@@ -331,13 +339,6 @@
     private class AclPermissions extends AbstractCompiledPermissions implements SynchronousEventListener {
 
         private final List<String> principalNames;
-        private final String jcrReadPrivilegeName;
-
-        /**
-         * flag indicating that there is not 'deny READ'.
-         * -> simplify {@link #grants(Path, int)} in case of permissions == READ
-         */
-        private boolean readAllowed;
 
         private AclPermissions(Set<Principal> principals) throws RepositoryException {
             this(principals, true);
@@ -348,17 +349,9 @@
             for (Principal princ : principals) {
                 principalNames.add(princ.getName());
             }
-            jcrReadPrivilegeName = session.getAccessControlManager().privilegeFromName(Privilege.JCR_READ).getName();
 
             if (listenToEvents) {
                 /*
-                 Determine if there is any 'denyRead' entry (since the default
-                 is that everyone can READ everywhere -> makes evaluation for
-                 the most common check (can-read) easy.
-                */
-                readAllowed = isReadAllowed(principalNames);
-
-                /*
                  Make sure this AclPermission recalculates the permissions if
                  any ACL concerning it is modified. interesting events are:
                  - new ACE-entry for any of the principals (NODE_ADDED)
@@ -376,56 +369,11 @@
             }
         }
 
-        /**
-         * If this provider defines read-permission for everyone (defined upon
-         * init with default values), search if there is any ACE that defines
-         * permissions for any of the principals AND denies-READ. Otherwise
-         * this shortcut is not possible.
-         *
-         * @param principalnames names of the principals
-         * @return true if read is allowed everywhere.
-         */
-        private boolean isReadAllowed(Collection<String> principalnames) {
-            boolean isReadAllowed = false;
-            if (initializedWithDefaults) {
-                try {
-                    QueryManager qm = session.getWorkspace().getQueryManager();
-                    StringBuffer stmt = new StringBuffer("/jcr:root");
-                    stmt.append("//element(*,");
-                    stmt.append(resolver.getJCRName(NT_REP_DENY_ACE));
-                    stmt.append(")[(");
-
-                    // where the rep:principalName property exactly matches any of
-                    // the given principalsNames
-                    int i = 0;
-                    for (String principalname : principalnames) {
-                        stmt.append("@").append(resolver.getJCRName(P_PRINCIPAL_NAME)).append(" eq ");
-                        stmt.append("'").append(principalname).append("'");
-                        if (++i < principalnames.size()) {
-                            stmt.append(" or ");
-                        }
-                    }
-                    // AND rep:privileges contains the READ privilege
-                    stmt.append(") and @");
-                    stmt.append(resolver.getJCRName(P_PRIVILEGES));
-                    stmt.append(" = '").append(jcrReadPrivilegeName).append("']");
-
-                    Query q = qm.createQuery(stmt.toString(), Query.XPATH);
-
-                    NodeIterator it = q.execute().getNodes();
-                    isReadAllowed =  !it.hasNext();
-                } catch (RepositoryException e) {
-                    log.error(e.toString());
-                    // unable to determine... -> no shortcut upon grants
-                }
-            }
-            return isReadAllowed;
-        }
-
         //------------------------------------< AbstractCompiledPermissions >---
         /**
          * @see AbstractCompiledPermissions#buildResult(Path)
          */
+        @Override
         protected Result buildResult(Path absPath) throws RepositoryException {
             boolean existingNode = false;
             NodeImpl node = null;
@@ -456,14 +404,14 @@
 
             // retrieve all ACEs at path or at the direct ancestor of path that
             // apply for the principal names.
-            AccessControlEntryIterator entries = new Entries(getNode(node), principalNames).iterator();
+            Iterator<AccessControlEntry> entries = retrieveResultEntries(getNode(node), principalNames);
             // build a list of ACEs that are defined locally at the node
-            List localACEs;
+            List<AccessControlEntry> localACEs;
             if (existingNode && isAccessControlled(node)) {
                 NodeImpl aclNode = node.getNode(N_POLICY);
                 localACEs = Arrays.asList(systemEditor.getACL(aclNode).getAccessControlEntries());
             } else {
-                localACEs = Collections.EMPTY_LIST;
+                localACEs = Collections.emptyList();
             }
             /*
              Calculate privileges and permissions:
@@ -520,22 +468,6 @@
             super.close();
         }
 
-        /**
-         *
-         * @param absPath absolute path
-         * @param permissions permission bits
-         * @return <code>true</code> if the permissions are granted
-         * @throws RepositoryException
-         * @see CompiledPermissions#grants(Path, int)
-         */
-        public boolean grants(Path absPath, int permissions) throws RepositoryException {
-            if (permissions == Permission.READ && readAllowed && !isAcItem(absPath)) {
-                return true;
-            } else {
-                return super.grants(absPath, permissions);
-            }
-        }
-
         //--------------------------------------------------< EventListener >---
         /**
          * @see javax.jcr.observation.EventListener#onEvent(EventIterator)
@@ -556,16 +488,6 @@
                             NodeImpl n = (NodeImpl) session.getNode(path);
                             if (n.isNodeType(NT_REP_ACE) &&
                                     principalNames.contains(n.getProperty(P_PRINCIPAL_NAME).getString())) {
-                                // and reset the readAllowed flag, if the new
-                                // ACE denies READ.
-                                if (readAllowed && n.isNodeType(NT_REP_DENY_ACE)) {
-                                    Value[] vs = n.getProperty(P_PRIVILEGES).getValues();
-                                    for (Value v : vs) {
-                                        if (jcrReadPrivilegeName.equals(v.getString())) {
-                                            readAllowed = false;
-                                        }
-                                    }
-                                }
                                 clearCache = true;
                             }
                             break;
@@ -573,7 +495,6 @@
                         case Event.NODE_REMOVED:
                             // can't find out if the removed ACL/ACE node was
                             // relevant for the principals
-                            readAllowed = isReadAllowed(principalNames);
                             clearCache = true;
                             break;
                         case Event.PROPERTY_ADDED:
@@ -594,7 +515,6 @@
                                 }
                                 if (principalName != null &&
                                         principalNames.contains(principalName)) {
-                                    readAllowed = isReadAllowed(principalNames);
                                     clearCache = true;
                                 }
                             }
@@ -624,13 +544,12 @@
      */
     private class Entries {
 
-        private final Map<String, List<AccessControlEntry>> principalNamesToEntries;
+        private final Collection<String> principalNames;
+        private final List<AccessControlEntry> userAces = new ArrayList();
+        private final List<AccessControlEntry> groupAces = new ArrayList();
 
         private Entries(NodeImpl node, Collection<String> principalNames) throws RepositoryException {
-            principalNamesToEntries = new LinkedHashMap<String, List<AccessControlEntry>>();
-            for (String name : principalNames) {
-                principalNamesToEntries.put(name, new ArrayList<AccessControlEntry>());
-            }
+            this.principalNames = principalNames;
             collectEntries(node);
         }
 
@@ -640,22 +559,63 @@
             if (isAccessControlled(node)) {
                 // build acl for the access controlled node
                 NodeImpl aclNode = node.getNode(N_POLICY);
-                ACLTemplate.collectEntries(aclNode, principalNamesToEntries);
+                //collectEntries(aclNode, principalNamesToEntries);
+                collectEntriesFromAcl(aclNode);
             }
-            // then, recursively look for access controlled parents up the hierarchy.
+            // recursively look for access controlled parents up the hierarchy.
             if (!rootNodeId.equals(node.getId())) {
                 NodeImpl parentNode = (NodeImpl) node.getParent();
                 collectEntries(parentNode);
             }
         }
 
-        private AccessControlEntryIterator iterator() {
-            List<AccessControlEntry> entries = new ArrayList<AccessControlEntry>();
-            for (List<AccessControlEntry> list: principalNamesToEntries.values()) {
-                entries.addAll(list);
+        /**
+         * Separately collect the entries defined for the user and group
+         * principals.
+         *
+         * @param aclNode acl node
+         * @throws RepositoryException if an error occurs
+         */
+        private void collectEntriesFromAcl(NodeImpl aclNode) throws RepositoryException {
+            SessionImpl sImpl = (SessionImpl) aclNode.getSession();
+            PrincipalManager principalMgr = sImpl.getPrincipalManager();
+            AccessControlManager acMgr = sImpl.getAccessControlManager();
+
+            NodeIterator itr = aclNode.getNodes();
+            while (itr.hasNext()) {
+                NodeImpl aceNode = (NodeImpl) itr.nextNode();
+                String principalName = aceNode.getProperty(AccessControlConstants.P_PRINCIPAL_NAME).getString();
+                // only process aceNode if 'principalName' is contained in the given set
+                if (principalNames.contains(principalName)) {
+                    Principal princ = principalMgr.getPrincipal(principalName);
+                    if (princ == null) {
+                        log.warn("Principal with name " + principalName + " unknown to PrincipalManager -> Ignored from AC evaluation.");
+                        continue;
+                    }
+
+                    Value[] privValues = aceNode.getProperty(AccessControlConstants.P_PRIVILEGES).getValues();
+                    Privilege[] privs = new Privilege[privValues.length];
+                    for (int i = 0; i < privValues.length; i++) {
+                        privs[i] = acMgr.privilegeFromName(privValues[i].getString());
+                    }
+                    // create a new ACEImpl (omitting validation check)
+                    AccessControlEntry ace = new ACLTemplate.Entry(
+                            princ,
+                            privs,
+                            aceNode.isNodeType(AccessControlConstants.NT_REP_GRANT_ACE),
+                            sImpl.getValueFactory());
+                    // add it to the proper list (e.g. separated by principals)
+                    if (princ instanceof Group) {
+                        groupAces.add(ace);
+                    } else {
+                        userAces.add(ace);
+                    }
+                }
             }
-            return new AccessControlEntryIterator(entries);
         }
-    }
 
+        private Iterator<AccessControlEntry> iterator() {
+            return new IteratorChain(userAces.iterator(), groupAces.iterator());
+        }
+    }
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLTemplate.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLTemplate.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLTemplate.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLTemplate.java Thu Oct 22 17:26:37 2009
@@ -140,51 +140,6 @@
         }
     }
 
-    /**
-     * Separately collect the entries defined for the principals with the
-     * specified names and return a map consisting of principal name key
-     * and a list of ACEs as value.
-     *
-     * @param aclNode acl node
-     * @param princToEntries Map of key = principalName and value = ArrayList
-     * to be filled with ACEs matching the principal names.
-     * @throws RepositoryException if an error occurs
-     */
-    static void collectEntries(NodeImpl aclNode, Map<String, List<AccessControlEntry>> princToEntries)
-            throws RepositoryException {
-        SessionImpl sImpl = (SessionImpl) aclNode.getSession();
-        PrincipalManager principalMgr = sImpl.getPrincipalManager();
-        AccessControlManager acMgr = sImpl.getAccessControlManager();
-
-        NodeIterator itr = aclNode.getNodes();
-        while (itr.hasNext()) {
-            NodeImpl aceNode = (NodeImpl) itr.nextNode();
-            String principalName = aceNode.getProperty(AccessControlConstants.P_PRINCIPAL_NAME).getString();
-            // only process aceNode if 'principalName' is contained in the given set
-            if (princToEntries.containsKey(principalName)) {
-                Principal princ = principalMgr.getPrincipal(principalName);
-                if (princ == null) {
-                    log.warn("Principal with name " + principalName + " unknown to PrincipalManager.");
-                    princ = new PrincipalImpl(principalName);
-                }
-
-                Value[] privValues = aceNode.getProperty(AccessControlConstants.P_PRIVILEGES).getValues();
-                Privilege[] privs = new Privilege[privValues.length];
-                for (int i = 0; i < privValues.length; i++) {
-                    privs[i] = acMgr.privilegeFromName(privValues[i].getString());
-                }
-                // create a new ACEImpl (omitting validation check)
-                Entry ace = new Entry(
-                        princ,
-                        privs,
-                        aceNode.isNodeType(AccessControlConstants.NT_REP_GRANT_ACE),
-                        sImpl.getValueFactory());
-                // add it to the proper list (e.g. separated by principals)
-                princToEntries.get(principalName).add(ace);
-            }
-        }
-    }
-
     private List<? extends AccessControlEntry> internalGetEntries() {
         List<Entry> l = new ArrayList<Entry>();
         for (List<Entry> o : entries.values()) {
@@ -298,7 +253,7 @@
         if (!(ace instanceof Entry)) {
             throw new AccessControlException("Invalid AccessControlEntry implementation " + ace.getClass().getName() + ".");
         }
-        List l = internalGetEntries(ace.getPrincipal());
+        List<Entry> l = internalGetEntries(ace.getPrincipal());
         if (l.remove(ace)) {
             if (l.isEmpty()) {
                 entries.remove(ace.getPrincipal().getName());

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/combined/CombinedProvider.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/combined/CombinedProvider.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/combined/CombinedProvider.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/combined/CombinedProvider.java Thu Oct 22 17:26:37 2009
@@ -81,6 +81,7 @@
     /**
      * @see AccessControlProvider#close()
      */
+    @Override
     public void close() {
         for (AccessControlProvider provider : providers) {
             provider.close();
@@ -91,6 +92,7 @@
     /**
      * @see AccessControlProvider#init(javax.jcr.Session, java.util.Map)
      */
+    @Override
     public void init(Session systemSession, Map configuration) throws RepositoryException {
         super.init(systemSession, configuration);
 
@@ -207,6 +209,7 @@
         /**
          * @see AbstractCompiledPermissions#buildResult(Path)
          */
+        @Override
         protected Result buildResult(Path absPath) throws RepositoryException {
             Result res = null;
             for (AbstractCompiledPermissions acp : cPermissions) {
@@ -219,6 +222,7 @@
         /**
          * @see AbstractCompiledPermissions#getResult(Path)
          */
+        @Override
         public Result getResult(Path absPath) throws RepositoryException {
             // TODO: missing caching
             return buildResult(absPath);
@@ -228,10 +232,11 @@
         /**
          * @see CompiledPermissions#close()
          */
+        @Override
         public synchronized void close() {
             // close all c-permissions retained in the list and clear the list.
-            for (Iterator it = cPermissions.iterator(); it.hasNext();) {
-                CompiledPermissions cp = (CompiledPermissions) it.next();
+            for (Iterator<AbstractCompiledPermissions> it = cPermissions.iterator(); it.hasNext();) {
+                CompiledPermissions cp = it.next();
                 cp.close();
                 it.remove();
             }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/principalbased/ACLProvider.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/principalbased/ACLProvider.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/principalbased/ACLProvider.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/principalbased/ACLProvider.java Thu Oct 22 17:26:37 2009
@@ -99,6 +99,7 @@
     /**
      * @see org.apache.jackrabbit.core.security.authorization.AccessControlProvider#init(javax.jcr.Session, java.util.Map)
      */
+    @Override
     public void init(Session systemSession, Map configuration) throws RepositoryException {
         super.init(systemSession, configuration);
 
@@ -113,6 +114,7 @@
         }
 
         editor = new ACLEditor(session, resolver.getQPath(acRoot.getPath()));
+        // TODO: replace by configurable default policy (see JCR-2331)
         if (!configuration.containsKey(PARAM_OMIT_DEFAULT_PERMISSIONS)) {
             try {
                 log.debug("Install initial permissions: ...");
@@ -276,6 +278,7 @@
         /**
          * @see AbstractCompiledPermissions#buildResult(Path)
          */
+        @Override
         protected synchronized Result buildResult(Path absPath) throws RepositoryException {
             if (!absPath.isAbsolute()) {
                 throw new RepositoryException("Absolute path expected.");
@@ -355,6 +358,7 @@
         /**
          * @see CompiledPermissions#close()
          */
+        @Override
         public void close() {
             try {
                 observationMgr.removeEventListener(this);

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=828791&r1=828790&r2=828791&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 Thu Oct 22 17:26:37 2009
@@ -16,18 +16,6 @@
  */
 package org.apache.jackrabbit.core.security.principal;
 
-import java.security.Principal;
-import java.util.Iterator;
-import java.util.LinkedHashSet;
-import java.util.Map;
-import java.util.Set;
-
-import javax.jcr.RepositoryException;
-import javax.jcr.Session;
-import javax.jcr.observation.Event;
-import javax.jcr.observation.EventIterator;
-import javax.jcr.observation.EventListener;
-
 import org.apache.commons.collections.iterators.IteratorChain;
 import org.apache.commons.collections.map.LRUMap;
 import org.apache.jackrabbit.api.security.principal.PrincipalIterator;
@@ -36,12 +24,25 @@
 import org.apache.jackrabbit.api.security.user.Group;
 import org.apache.jackrabbit.api.security.user.UserManager;
 import org.apache.jackrabbit.core.SessionImpl;
+import org.apache.jackrabbit.core.security.SystemPrincipal;
 import org.apache.jackrabbit.core.security.user.UserManagerImpl;
 import org.apache.jackrabbit.spi.commons.conversion.NameResolver;
 import org.apache.jackrabbit.util.Text;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+import javax.jcr.observation.Event;
+import javax.jcr.observation.EventIterator;
+import javax.jcr.observation.EventListener;
+import javax.security.auth.Subject;
+import java.security.Principal;
+import java.util.Iterator;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Set;
+
 /**
  * Provides principals for the users contained within the Repository.<p/>
  * Each {@link Authorizable} accessible via {@link UserManager}
@@ -71,7 +72,7 @@
 
     private final EveryonePrincipal everyonePrincipal;
 
-    private final String pGroupName;
+    private final String pMembers;
     private final String pPrincipalName;
 
     /**
@@ -93,25 +94,19 @@
         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);
+            ntNames[0] = resolver.getJCRName(UserManagerImpl.NT_REP_GROUP);
+            pMembers = resolver.getJCRName(UserManagerImpl.P_MEMBERS);
             pPrincipalName = resolver.getJCRName(UserManagerImpl.P_PRINCIPAL_NAME);
         } else {
-            ntNames[0] = "rep:User";
-            pGroupName = "rep:groups";
+            ntNames[0] = "rep:Group";
+            pMembers = "rep:members";
             pPrincipalName = "rep:principalName";
         }
 
-        // find common ancestor of all user and group nodes.
-        String userPath = userManager.getUsersPath();
         String groupPath = userManager.getGroupsPath();
-        String obsPath = userPath;
-        while (!Text.isDescendant(obsPath, groupPath)) {
-            obsPath = Text.getRelativeParent(obsPath, 1);
-        }       
         securitySession.getWorkspace().getObservationManager().addEventListener(this,
                 Event.NODE_REMOVED | Event.PROPERTY_ADDED | Event.PROPERTY_CHANGED | Event.PROPERTY_REMOVED,
-                obsPath,
+                groupPath,
                 true,
                 null,
                 ntNames,
@@ -217,18 +212,12 @@
     }
 
     /**
-     * Always returns true.
-     *
      * @see PrincipalProvider#canReadPrincipal(javax.jcr.Session,java.security.Principal)
      */
     public boolean canReadPrincipal(Session session, Principal principal) {
         checkInitialized();
-        // by default (UserAccessControlProvider) READ-privilege is granted to
-        // everybody -> omit any (expensive) checks.
-        return true;
-        /*
-        // TODO: uncomment code if it turns out that the previous assumption is problematic.
-        // check if the session is granted read to the node.
+        // check if the session can read the user/group associated with the
+        // given principal
         if (session instanceof SessionImpl) {
             SessionImpl sImpl = (SessionImpl) session;
             Subject subject = sImpl.getSubject();
@@ -237,14 +226,13 @@
                 return true;
             }
             try {
-                UserManager umgr = ((SessionImpl)session).getUserManager();
+                UserManager umgr = sImpl.getUserManager();
                 return umgr.getAuthorizable(principal) != null;
             } catch (RepositoryException e) {
                 // ignore and return false
             }
         }
         return false;
-        */
     }
 
     //------------------------------------------------------< EventListener >---
@@ -262,7 +250,7 @@
             if (type == Event.PROPERTY_ADDED || type == Event.PROPERTY_CHANGED
                     || type == Event.PROPERTY_REMOVED) {
                 try {
-                    if (pGroupName.equals(Text.getName(ev.getPath()))) {
+                    if (pMembers.equals(Text.getName(ev.getPath()))) {
                         synchronized (membershipCache) {
                             membershipCache.clear();
                         }
@@ -286,9 +274,9 @@
      * including inherited membership.
      */
     private Set<Principal> collectGroupMembership(Principal princ) {
-        Set<Principal> membership = new LinkedHashSet<Principal>();
+        final Set<Principal> membership = new LinkedHashSet<Principal>();
             try {
-                Authorizable auth = userManager.getAuthorizable(princ);
+                final Authorizable auth = userManager.getAuthorizable(princ);
                 if (auth != null) {
                     addToCache(princ);
                     Iterator<Group> itr = auth.memberOf();
@@ -315,7 +303,7 @@
     private PrincipalIterator findUserPrincipals(String simpleFilter) {
         synchronized (userManager) {
             try {
-                Iterator itr = userManager.findAuthorizables(pPrincipalName, simpleFilter, UserManager.SEARCH_TYPE_USER);
+                Iterator<Authorizable> itr = userManager.findAuthorizables(pPrincipalName, simpleFilter, UserManager.SEARCH_TYPE_USER);
                 return new PrincipalIteratorImpl(itr, false);
             } catch (RepositoryException e) {
                 log.error("Error while searching user principals.", e);
@@ -332,7 +320,7 @@
     private PrincipalIterator findGroupPrincipals(final String simpleFilter) {
         synchronized (userManager) {
             try {
-                Iterator itr = userManager.findAuthorizables(pPrincipalName, simpleFilter, UserManager.SEARCH_TYPE_GROUP);
+                Iterator<Authorizable> itr = userManager.findAuthorizables(pPrincipalName, simpleFilter, UserManager.SEARCH_TYPE_GROUP);
 
                 // everyone will not be found by the user manager -> extra test
                 boolean addEveryone = everyonePrincipal.getName().matches(".*"+simpleFilter+".*");
@@ -353,10 +341,10 @@
      */
     private class PrincipalIteratorImpl extends AbstractPrincipalIterator {
 
-        private final Iterator authorizableItr;
+        private final Iterator<Authorizable> authorizableItr;
         private boolean addEveryone;
 
-        private PrincipalIteratorImpl(Iterator authorizableItr, boolean addEveryone) {
+        private PrincipalIteratorImpl(Iterator<Authorizable> authorizableItr, boolean addEveryone) {
             this.authorizableItr = authorizableItr;
             this.addEveryone = addEveryone;
 
@@ -369,7 +357,7 @@
         protected Principal seekNext() {
             while (authorizableItr.hasNext()) {
                 try {
-                    Principal p = ((Authorizable) authorizableItr.next()).getPrincipal();
+                    Principal p = authorizableItr.next().getPrincipal();
                     addToCache(p);
                     return p;
                 } catch (RepositoryException e) {

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/ProviderRegistryImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/ProviderRegistryImpl.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/ProviderRegistryImpl.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/principal/ProviderRegistryImpl.java Thu Oct 22 17:26:37 2009
@@ -49,10 +49,11 @@
      */
     public ProviderRegistryImpl(PrincipalProvider defaultPrincipalProvider) {
         this.defaultPrincipalProvider = defaultPrincipalProvider;
-        providers.put(defaultPrincipalProvider.getClass().getName(), defaultPrincipalProvider);
+        if (defaultPrincipalProvider != null) {
+            providers.put(defaultPrincipalProvider.getClass().getName(), defaultPrincipalProvider);
+        }
     }
 
-
     //------------------------------------------< PrincipalProviderRegistry >---
     /**
      * @see PrincipalProviderRegistry#registerProvider(Properties)

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/simple/SimpleSecurityManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/simple/SimpleSecurityManager.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/simple/SimpleSecurityManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/simple/SimpleSecurityManager.java Thu Oct 22 17:26:37 2009
@@ -254,10 +254,11 @@
     }
 
     /**
-     * @see JackrabbitSecurityManager#getUserID(Subject)
+     * @see JackrabbitSecurityManager#getUserID(javax.security.auth.Subject, String)
      */
-    public String getUserID(Subject subject) throws RepositoryException {
+    public String getUserID(Subject subject, String workspaceName) throws RepositoryException {
         String uid = null;
+
         // if SimpleCredentials are present, the UserID can easily be retrieved.
         Iterator<SimpleCredentials> creds = subject.getPublicCredentials(SimpleCredentials.class).iterator();
         if (creds.hasNext()) {
@@ -288,7 +289,7 @@
      * @return an {@link AuthContext} for the given Credentials, Subject
      * @throws RepositoryException in other exceptional repository states
      */
-    public AuthContext getAuthContext(Credentials creds, Subject subject)
+    public AuthContext getAuthContext(Credentials creds, Subject subject, String workspaceName)
             throws RepositoryException {
         checkInitialized();
         return authCtxProvider.getAuthContext(creds, subject, systemSession, principalProviderRegistry, adminID, anonymID);

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/simple/SimpleWorkspaceAccessManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/simple/SimpleWorkspaceAccessManager.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/simple/SimpleWorkspaceAccessManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/simple/SimpleWorkspaceAccessManager.java Thu Oct 22 17:26:37 2009
@@ -32,7 +32,7 @@
     /**
      * @see WorkspaceAccessManager#init(Session)
      */
-    public void init(Session securitySession) {
+    public void init(Session systemSession) {
         // nothing to do
     }
 

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=828791&r1=828790&r2=828791&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 Thu Oct 22 17:26:37 2009
@@ -21,8 +21,8 @@
 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.core.PropertyImpl;
 import org.apache.jackrabbit.core.SessionImpl;
+import org.apache.jackrabbit.core.PropertyImpl;
 import org.apache.jackrabbit.core.nodetype.NodeTypeImpl;
 import org.apache.jackrabbit.core.security.principal.PrincipalImpl;
 import org.apache.jackrabbit.spi.Name;
@@ -30,18 +30,23 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import javax.jcr.ItemNotFoundException;
 import javax.jcr.Property;
 import javax.jcr.PropertyIterator;
-import javax.jcr.PropertyType;
 import javax.jcr.RepositoryException;
 import javax.jcr.Value;
+import javax.jcr.Session;
+import javax.jcr.ItemNotFoundException;
+import javax.jcr.AccessDeniedException;
+import javax.jcr.ItemVisitor;
+import javax.jcr.Node;
+import javax.jcr.util.TraversingItemVisitor;
 import javax.jcr.nodetype.ConstraintViolationException;
 import javax.jcr.nodetype.PropertyDefinition;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Set;
+import java.util.HashSet;
 
 /**
  * AuthorizableImpl
@@ -52,6 +57,7 @@
 
     final UserManagerImpl userManager;
     private final NodeImpl node;
+    private int hashCode;
 
     /**
      * @param node The node this Authorizable is persisted to.
@@ -62,9 +68,6 @@
      */
     protected AuthorizableImpl(NodeImpl node, UserManagerImpl userManager)
             throws RepositoryException {
-        if (!node.isNodeType(NT_REP_AUTHORIZABLE)) {
-            throw new IllegalArgumentException("Node argument of NodeType " + NT_REP_AUTHORIZABLE + " required");
-        }
         this.node = node;
         this.userManager = userManager;
     }
@@ -84,7 +87,7 @@
      * @see Authorizable#declaredMemberOf()
      */
     public Iterator<Group> declaredMemberOf() throws RepositoryException {
-        List<Group> memberShip = new ArrayList<Group>();
+        Set<Group> memberShip = new HashSet<Group>();
         collectMembership(memberShip, false);
         return memberShip.iterator();
     }
@@ -93,7 +96,7 @@
      * @see Authorizable#memberOf()
      */
     public Iterator<Group> memberOf() throws RepositoryException {
-        List<Group> memberShip = new ArrayList<Group>();
+        Set<Group> memberShip = new HashSet<Group>();
         collectMembership(memberShip, true);
         return memberShip.iterator();
     }
@@ -151,12 +154,20 @@
     public synchronized void setProperty(String name, Value value) throws RepositoryException {
         checkProtectedProperty(name);
         try {
+            // check if the property has already been created as multi valued
+            // property before -> in this case remove in order to avoid valueformatex.
+            if (node.hasProperty(name)) {
+                Property p = node.getProperty(name);
+                if (p.isMultiple()) {
+                    p.remove();
+                }
+            }
             node.setProperty(name, value);
-            if (!userManager.batchModus) {
+            if (userManager.isAutoSave()) {
                 node.save();
             }
         } catch (RepositoryException e) {
-            log.warn("Failed to set Property " + name + " for Authorizable " + getID());
+            log.warn("Failed to set Property " + name + " for " + this, e);
             node.refresh(false);
             throw e;
         }
@@ -176,12 +187,20 @@
     public synchronized void setProperty(String name, Value[] values) throws RepositoryException {
         checkProtectedProperty(name);
         try {
+            // check if the property has already been created as single valued
+            // property before -> in this case remove in order to avoid valueformatex.
+            if (node.hasProperty(name)) {
+                Property p = node.getProperty(name);
+                if (!p.isMultiple()) {
+                    p.remove();
+                }
+            }
             node.setProperty(name, values);
-            if (!userManager.batchModus) {
+            if (userManager.isAutoSave()) {
                 node.save();
             }
         } catch (RepositoryException e) {
-            log.warn("Failed to set Property " + name + " for Authorizable " + getID());
+            log.warn("Failed to set Property " + name + " for " + this, e);
             node.refresh(false);
             throw e;
         }
@@ -200,7 +219,7 @@
                 } else {
                     p.setValue((Value) null);
                 }
-                if (!userManager.batchModus) {
+                if (userManager.isAutoSave()) {
                     node.save();
                 }
                 return true;
@@ -208,7 +227,7 @@
                 return false;
             }
         } catch (RepositoryException e) {
-            log.warn("Failed to remove Property " + name + " from Authorizable " + getID());
+            log.warn("Failed to remove Property " + name + " from " + this, e);
             node.refresh(false);
             throw e;
         }
@@ -223,7 +242,51 @@
         if (!isGroup() && ((User) this).isAdmin()) {
             throw new RepositoryException("The administrator cannot be removed.");
         }
-        userManager.removeProtectedItem(node, node.getParent());
+        Session s = getSession();
+        node.remove();
+        if (userManager.isAutoSave()) {
+            s.save();
+        }
+    }
+
+    //-------------------------------------------------------------< Object >---
+    @Override
+    public int hashCode() {
+        if (hashCode == 0) {
+            try {
+                StringBuilder sb = new StringBuilder();
+                sb.append(isGroup() ? "group:" : "user:");
+                sb.append(getSession().getWorkspace().getName());
+                sb.append(":");
+                sb.append(node.getIdentifier());
+                hashCode = sb.toString().hashCode();
+            } catch (RepositoryException e) {
+            }
+        }
+        return hashCode;
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+        if (obj instanceof AuthorizableImpl) {
+            AuthorizableImpl otherAuth = (AuthorizableImpl) obj;
+            try {
+                return isGroup() == otherAuth.isGroup() && node.isSame(otherAuth.node);
+            } catch (RepositoryException e) {
+                // should not occur -> return false in this case.
+            }
+        }
+        return false;
+    }
+
+    @Override
+    public String toString() {
+        try {
+            String typeStr = (isGroup()) ? "Group '" : "User '";
+            return new StringBuilder().append(typeStr).append(getID()).append("'").toString();
+        } catch (RepositoryException e) {
+            return super.toString();
+        }
     }
 
     //--------------------------------------------------------------------------
@@ -243,79 +306,69 @@
         return node.getProperty(P_PRINCIPAL_NAME).getString();
     }
 
-    boolean addToGroup(GroupImpl group) throws RepositoryException {
-        try {
-            Value[] values;
-            Value added = getSession().getValueFactory().createValue(group.getNode(), true);
-            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, PropertyType.WEAKREFERENCE);
-            return true;
-        } catch (RepositoryException e) {
-            // revert all pending changes and rethrow.
-            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(), true);
-        PropertyImpl property = node.getProperty(P_GROUPS);
-        List<Value> valList = new ArrayList<Value>(Arrays.asList(property.getValues()));
-        if (valList.remove(toRemove)) {
-            try {
-                if (valList.isEmpty()) {
-                    userManager.removeProtectedItem(property, node);
-                } else {
-                    Value[] values = valList.toArray(new Value[valList.size()]);
-                    userManager.setProtectedProperty(node, P_GROUPS, values, PropertyType.WEAKREFERENCE);
+    private void collectMembership(final Set<Group> groups, boolean includeIndirect) throws RepositoryException {
+        PropertyIterator refs = getMembershipReferences();
+        if (refs != null) {
+            while (refs.hasNext()) {
+                try {
+                    NodeImpl n = (NodeImpl) refs.nextProperty().getParent();
+                    if (n.isNodeType(NT_REP_GROUP)) {
+                        Group group = userManager.createGroup(n);
+                        // only retrieve indirect membership if the group is not
+                        // yet present (detected eventual circular membership).
+                        if (groups.add(group) && includeIndirect) {
+                            ((AuthorizableImpl) group).collectMembership(groups, true);
+                        }
+                    } else {
+                        // weak-ref property 'rep:members' that doesn't reside under an
+                        // group node -> doesn't represent a valid group member.
+                        log.debug("Invalid member reference to '" + this + "' -> Not included in membership set.");
+                    }
+                } catch (ItemNotFoundException e) {
+                    // group node doesn't exist  -> -> ignore exception
+                    // and skip this reference from membership list.
+                } catch (AccessDeniedException e) {
+                    // not allowed to see the group node -> ignore exception
+                    // and skip this reference from membership list.
                 }
-                return true;
-            } catch (RepositoryException e) {
-                // modification failed -> revert all pending changes.
-                node.refresh(false);
-                throw e;
             }
         } else {
-            // nothing changed
-            log.debug(message);
-            return false;
+            // workaround for failure of Node#getWeakReferences
+            // traverse the tree below groups-path and collect membership manually.
+            log.info("Traversing groups tree to collect membership.");
+            ItemVisitor visitor = new TraversingItemVisitor.Default() {
+                @Override
+                protected void entering(Property property, int level) throws RepositoryException {
+                    PropertyImpl pImpl = (PropertyImpl) property;
+                    NodeImpl n = (NodeImpl) pImpl.getParent();
+                    if (P_MEMBERS.equals(pImpl.getQName()) && n.isNodeType(NT_REP_GROUP)) {
+                        for (Value value : property.getValues()) {
+                            if (value.getString().equals(node.getIdentifier())) {
+                                Group gr = (Group) userManager.getAuthorizable(n);
+                                groups.add(gr);
+                            }
+                        }
+                    }
+                }
+            };
+            Node groupsNode = getSession().getNode(userManager.getGroupsPath());
+            visitor.visit(groupsNode);
         }
     }
 
-    private void collectMembership(List<Group> groups, boolean includedIndirect) throws RepositoryException {
-        NodeImpl node = getNode();
-        if (!node.hasProperty(P_GROUPS)) {
-            return;
-        }
-        Value[] refs = node.getProperty(P_GROUPS).getValues();
-        for (Value ref : refs) {
-            try {
-                NodeImpl groupNode = (NodeImpl) getSession().getNodeByUUID(ref.getString());
-                Group group = GroupImpl.create(groupNode, userManager);
-                if (groups.add(group) && includedIndirect) {
-                    ((AuthorizableImpl) group).collectMembership(groups, true);
-                }
-            } catch (ItemNotFoundException e) {
-                // groupNode doesn't exist any more
-                log.warn("Group node referenced by " + getID() + " doesn't exist anymore -> Ignored from membership list.");
-                // TODO: possibly clean up list of group memberships
-            }
+    /**
+     * @return the iterator returned by {@link Node#getWeakReferences(String)}
+     * or <code>null</code> if the method call fails with <code>RepositoryException</code>.
+     * See fallback scenario above.
+     */
+    private PropertyIterator getMembershipReferences() {
+        PropertyIterator refs = null;
+        try {
+            refs = node.getWeakReferences(getSession().getJCRName(P_MEMBERS));
+        } catch (RepositoryException e) {
+            log.error("Failed to retrieve membership references of " + this + ".", e);
         }
+        return refs;
     }
 
     /**
@@ -343,11 +396,8 @@
      * following that has a special meaning and must be altered using this
      * user API:
      * <ul>
-     * <ul>
      * <li>rep:principalName</li>
-     * <li>rep:userId</li>
-     * <li>rep:referees</li>
-     * <li>rep:groups</li>
+     * <li>rep:members</li>
      * <li>rep:impersonators</li>
      * <li>rep:password</li>
      * </ul>
@@ -363,7 +413,7 @@
     private boolean isProtectedProperty(String propertyName) throws RepositoryException {
         Name pName = getSession().getQName(propertyName);
         return P_PRINCIPAL_NAME.equals(pName)
-                || P_GROUPS.equals(pName)
+                || P_MEMBERS.equals(pName)
                 || P_IMPERSONATORS.equals(pName) || P_PASSWORD.equals(pName);
     }
 
@@ -378,7 +428,7 @@
      */
     private void checkProtectedProperty(String propertyName) throws ConstraintViolationException, RepositoryException {
         if (isProtectedProperty(propertyName)) {
-            throw new ConstraintViolationException("Attempt to modify protected property " + propertyName + " of an Authorizable.");
+            throw new ConstraintViolationException("Attempt to modify protected property " + propertyName + " of " + this);
         }
     }
 

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=828791&r1=828790&r2=828791&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 Thu Oct 22 17:26:37 2009
@@ -19,14 +19,17 @@
 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.api.security.user.UserManager;
 import org.apache.jackrabbit.core.NodeImpl;
-import org.apache.jackrabbit.util.Text;
+import org.apache.jackrabbit.core.PropertyImpl;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.jcr.Node;
-import javax.jcr.PropertyIterator;
 import javax.jcr.RepositoryException;
+import javax.jcr.Value;
+import javax.jcr.ItemNotFoundException;
+import javax.jcr.PropertyType;
 import java.io.IOException;
 import java.io.ObjectOutputStream;
 import java.security.Principal;
@@ -36,6 +39,9 @@
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Set;
+import java.util.List;
+import java.util.ArrayList;
+import java.util.Arrays;
 
 /**
  * GroupImpl...
@@ -46,21 +52,10 @@
 
     private Principal principal;
 
-    private GroupImpl(NodeImpl node, UserManagerImpl userManager) throws RepositoryException {
+    protected GroupImpl(NodeImpl node, UserManagerImpl userManager) throws RepositoryException {
         super(node, userManager);
     }
 
-    static Group create(NodeImpl node, UserManagerImpl userManager) throws RepositoryException {
-        if (node == null || !node.isNodeType(NT_REP_GROUP)) {
-            throw new IllegalArgumentException();
-        }
-        if (!Text.isDescendant(userManager.getGroupsPath(), node.getPath())) {
-            throw new IllegalArgumentException("Group has to be within the Group Path");
-        }
-        return new GroupImpl(node, userManager);
-    }
-
-
     //-------------------------------------------------------< Authorizable >---
     /**
      * @see Authorizable#isGroup()
@@ -84,27 +79,28 @@
      * @see Group#getDeclaredMembers()
      */
     public Iterator<Authorizable> getDeclaredMembers() throws RepositoryException {
-        return getMembers(false).iterator();
+        return getMembers(false, UserManager.SEARCH_TYPE_AUTHORIZABLE).iterator();
     }
 
     /**
      * @see Group#getMembers()
      */
     public Iterator<Authorizable> getMembers() throws RepositoryException {
-        return getMembers(true).iterator();
+        return getMembers(true, UserManager.SEARCH_TYPE_AUTHORIZABLE).iterator();
     }
 
     /**
      * @see Group#isMember(Authorizable)
      */
     public boolean isMember(Authorizable authorizable) throws RepositoryException {
-        if (authorizable == null || !(authorizable instanceof AuthorizableImpl)) {
+        if (authorizable == null || !(authorizable instanceof AuthorizableImpl)
+                || getNode().isSame(((AuthorizableImpl) authorizable).getNode())) {
             return false;
         } else {
             String thisID = getID();
             AuthorizableImpl impl = (AuthorizableImpl) authorizable;
-            for (Iterator it = impl.memberOf(); it.hasNext();) {
-                if (thisID.equals(((GroupImpl) it.next()).getID())) {
+            for (Iterator<Group> it = impl.memberOf(); it.hasNext();) {
+                if (thisID.equals(it.next().getID())) {
                     return true;
                 }
             }
@@ -116,12 +112,7 @@
      * @see Group#addMember(Authorizable)
      */
     public boolean addMember(Authorizable authorizable) throws RepositoryException {
-        if (authorizable == null || !(authorizable instanceof AuthorizableImpl)
-                || isMember(authorizable)) {
-            return false;
-        }
-        if (isCyclicMembership(authorizable)) {
-            log.warn("Attempt to create circular group membership.");
+        if (authorizable == null || !(authorizable instanceof AuthorizableImpl)) {
             return false;
         }
 
@@ -133,18 +124,71 @@
             return false;
         }
 
-        // preconditions are met -> delegate to authorizableImpl
-        return authImpl.addToGroup(this);
+        if (isCyclicMembership(authImpl)) {
+            log.warn("Attempt to create circular group membership.");
+            return false;
+        }
+
+        Value[] values;
+        Value toAdd = getSession().getValueFactory().createValue(memberNode, true);
+        NodeImpl node = getNode();
+        if (node.hasProperty(P_MEMBERS)) {
+            Value[] old = node.getProperty(P_MEMBERS).getValues();
+            for (Value v : old) {
+                if (v.equals(toAdd)) {
+                    log.debug("Authorizable " + authImpl + " is already member of " + this);
+                    return false;
+                }
+            }
+
+            values = new Value[old.length + 1];
+            System.arraycopy(old, 0, values, 0, old.length);
+        } else {
+            values = new Value[1];
+        }
+        values[values.length - 1] = toAdd;
+
+        userManager.setProtectedProperty(node, P_MEMBERS, values, PropertyType.WEAKREFERENCE);
+        return true;
     }
 
     /**
      * @see Group#removeMember(Authorizable)
      */
     public boolean removeMember(Authorizable authorizable) throws RepositoryException {
-        if (!isMember(authorizable) || !(authorizable instanceof AuthorizableImpl)) {
+        if (!(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(), true);
+
+        PropertyImpl property = node.getProperty(P_MEMBERS);
+        List<Value> valList = new ArrayList<Value>(Arrays.asList(property.getValues()));
+
+        if (valList.remove(toRemove)) {
+            try {
+                if (valList.isEmpty()) {
+                    userManager.removeProtectedItem(property, node);
+                } else {
+                    Value[] values = 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);
     }
 
     //--------------------------------------------------------------------------
@@ -152,50 +196,68 @@
      *
      * @param includeIndirect If <code>true</code> all members of this group
      * will be return; otherwise only the declared members.
+     * @param type Any of {@link UserManager#SEARCH_TYPE_AUTHORIZABLE},
+     * {@link UserManager#SEARCH_TYPE_GROUP}, {@link UserManager#SEARCH_TYPE_USER}.
      * @return A collection of members of this group.
      * @throws RepositoryException If an error occurs while collecting the members.
      */
-    private Collection<Authorizable> getMembers(boolean includeIndirect) throws RepositoryException {
-        PropertyIterator itr = getNode().getWeakReferences(getSession().getJCRName(P_GROUPS));
-        Collection<Authorizable> members = new HashSet<Authorizable>((int) itr.getSize());
-        while (itr.hasNext()) {
-            NodeImpl n = (NodeImpl) itr.nextProperty().getParent();
-            if (n.isNodeType(NT_REP_GROUP)) {
-                Group group = userManager.createGroup(n);
-                // 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));
+    private Collection<Authorizable> getMembers(boolean includeIndirect, int type) throws RepositoryException {
+        Collection<Authorizable> members = new HashSet<Authorizable>();
+        if (getNode().hasProperty(P_MEMBERS)) {
+            Value[] vs = getNode().getProperty(P_MEMBERS).getValues();
+            for (Value v : vs) {
+                try {
+                    NodeImpl n = (NodeImpl) getSession().getNodeByIdentifier(v.getString());
+                    if (n.isNodeType(NT_REP_GROUP)) {
+                        if (type != UserManager.SEARCH_TYPE_USER) {
+                            Group group = userManager.createGroup(n);
+                            // 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, type));
+                            }
+                        } // else: groups are ignored
+                    } else if (n.isNodeType(NT_REP_USER)) {
+                        if (type != UserManager.SEARCH_TYPE_GROUP) {
+                            User user = userManager.createUser(n);
+                            members.add(user);
+                        }
+                    } else {
+                        // reference does point to an authorizable node -> not a
+                        // member of this group -> ignore
+                        log.debug("Group member entry with invalid node type " + n.getPrimaryNodeType().getName() + " -> Not included in member set.");
+                    }
+                } catch (ItemNotFoundException e) {
+                    // dangling weak reference -> clean upon next write.
+                    log.debug("Authorizable node referenced by " + getID() + " doesn't exist any more -> Ignored from member list.");
                 }
-            } else if (n.isNodeType(NT_REP_USER)) {
-                User user = userManager.createUser(n);
-                members.add(user);
-            } else {
-                // weak-ref property 'rep:groups' that doesn't reside under an
-                // authorizable node -> doesn't represent a member of this group.
-                log.debug("Undefined reference to group '" + getID() + "' -> Not included in member set.");
             }
-        }
+        } // no rep:member property
         return members;
     }
 
     /**
-     * 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.
+     * Returns <code>true</code> if the given <code>newMember</code> is a Group
+     * and contains <code>this</code> Group as declared or inherited member.
      *
      * @param newMember The new member to be tested for cyclic membership.
      * @return true if the 'newMember' is a group and 'this' is an declared or
      * inherited member of it.
      * @throws javax.jcr.RepositoryException If an error occurs.
      */
-    private boolean isCyclicMembership(Authorizable newMember) throws RepositoryException {
-        boolean cyclic = false;
+    private boolean isCyclicMembership(AuthorizableImpl newMember) throws RepositoryException {
         if (newMember.isGroup()) {
-            Group gr = (Group) newMember;
-            cyclic = gr.isMember(this);
+            GroupImpl gr = (GroupImpl) newMember;
+            for (Authorizable member : gr.getMembers(true, UserManager.SEARCH_TYPE_GROUP)) {
+                GroupImpl grMemberImpl = (GroupImpl) member;
+                if (getNode().getUUID().equals(grMemberImpl.getNode().getUUID())) {
+                    // found cyclic group membership
+                    return true;
+                }
+
+            }
         }
-        return cyclic;
+        return false;
     }
 
     //------------------------------------------------------< inner classes >---
@@ -281,9 +343,8 @@
             if (members == null) {
                 members = new HashSet<Principal>();
                 try {
-                    for (Iterator it = GroupImpl.this.getMembers(); it.hasNext();) {
-                        Authorizable authrz = (Authorizable) it.next();
-                        members.add(authrz.getPrincipal());
+                    for (Iterator<Authorizable> it = GroupImpl.this.getMembers(); it.hasNext();) {
+                        members.add(it.next().getPrincipal());
                     }
                 } catch (RepositoryException e) {
                     // should not occur.

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/ImpersonationImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/ImpersonationImpl.java?rev=828791&r1=828790&r2=828791&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/ImpersonationImpl.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/ImpersonationImpl.java Thu Oct 22 17:26:37 2009
@@ -83,14 +83,14 @@
      */
     public synchronized boolean grantImpersonation(Principal principal) throws RepositoryException {
         if (principal instanceof AdminPrincipal || principal instanceof SystemPrincipal) {
-            log.debug("Admin and System principal are already granted impersonation.");
+            log.warn("Admin and System principal are already granted impersonation.");
             return false;
         }
 
         // make sure the given principals belong to an existing authorizable
         Authorizable auth = user.userManager.getAuthorizable(principal);
         if (auth == null || auth.isGroup()) {
-            log.debug("Cannot grant impersonation to a principal that is a Group " +
+            log.warn("Cannot grant impersonation to a principal that is a Group " +
                       "or an unknown Authorizable.");
             return false;
         }
@@ -98,7 +98,7 @@
         String pName = principal.getName();
         // make sure user does not impersonate himself
         if (user.getPrincipal().getName().equals(pName)) {
-            log.debug("Cannot grant impersonation to oneself.");
+            log.warn("Cannot grant impersonation to oneself.");
             return false;
         }
 
@@ -116,7 +116,7 @@
      */
     public synchronized boolean revokeImpersonation(Principal principal) throws RepositoryException {
         if (principal instanceof AdminPrincipal || principal instanceof SystemPrincipal) {
-            log.debug("Admin and System principal are always granted impersonation.");
+            log.warn("Admin and System principal are always granted impersonation.");
             return false;
         }
 
@@ -151,7 +151,7 @@
 
         boolean allows = false;
         try {
-            Set impersonators = getImpersonatorNames();
+            Set<String> impersonators = getImpersonatorNames();
             allows = impersonators.removeAll(principalNames);
         } catch (RepositoryException e) {
             // should never get here