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 2008/04/17 18:27:20 UTC

svn commit: r649172 [1/2] - in /jackrabbit/trunk/jackrabbit-core/src: main/java/org/apache/jackrabbit/core/ main/java/org/apache/jackrabbit/core/observation/ main/java/org/apache/jackrabbit/core/query/lucene/ main/java/org/apache/jackrabbit/core/securi...

Author: angela
Date: Thu Apr 17 09:27:09 2008
New Revision: 649172

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

> introduce AccessManager.canRead(Path)
> retrieve Policy/ACE from ACProvider by Path rather than by ID
> add CompiledPermission.canReadAll for optimization
> fix ACLTemplate (various bugs) and add additional tests
> get rid of deprecated AccessManager.isGranted(ItemId, int) wherever possible
  and mark/simplify other usages
  - ItemManager
  - BatchedItemOperations
  - add TODO with QueryResultImpl that still uses the isGranted method.
  - EventConsumer: still uses isGranted. but various calls were replaced by private
    canRead -> simplify later optimization.
> remove unused ACLCache.java
> ItemManager:
  - make getItem consistent with itemExists (check perm even if obtained from cache)
  - replace getItemState by getItemData
  - get rid of duplicate code
  - add canRead(ItemData, Path) which is used (almost) everywhere and avoid permission
    check for NEW,non-protected items
  - consequently getItem(ItemId, boolean) is not required any more.
  - upon building LazyItemIterator for children: omit the duplicate permission
    check while building the set of childIds.
    the perm-check is executed upon the subsequent ItemManager.getItem call anyway 
> NodeImpl: createChildNode called ItemManager.getItem for an new item that has
  not yet been added as CNE to its parent -> replace by ItemManager.createItemInstance
  which omits the permission check.
> ItemImpl: 
  - fix javadoc
  - replace removed method ItemMgr.getItem(ItemId, boolean) by getItem(ItemId)

Added:
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/AbstractEvaluationTest.java   (with props)
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/combined/EvaluationTest.java   (with props)
Removed:
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLCache.java
Modified:
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/BatchedItemOperations.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/DefaultSecurityManager.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemImpl.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemManager.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/LazyItemIterator.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/NodeImpl.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SystemSession.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/observation/EventConsumer.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/query/lucene/QueryResultImpl.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/AccessManager.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/DefaultAccessManager.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/SimpleJBossAccessManager.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractAccessControlProvider.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractCompiledPermissions.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlEditor.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlProvider.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/CompiledPermissions.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLProvider.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/acl/ACLTemplate.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/combined/CombinedProvider.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/combined/PolicyTemplateImpl.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/simple/SimpleAccessManager.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserAccessControlProvider.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/AbstractPolicyTemplateTest.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/acl/ACLTemplateTest.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/acl/EvaluationTest.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/combined/TestAll.java

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/BatchedItemOperations.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/BatchedItemOperations.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/BatchedItemOperations.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/BatchedItemOperations.java Thu Apr 17 09:27:09 2008
@@ -1846,7 +1846,8 @@
             iter = srcState.getPropertyNames().iterator();
             while (iter.hasNext()) {
                 Name propName = (Name) iter.next();
-                if (!srcAccessMgr.isGranted(srcPath, propName, Permission.READ)) {
+                Path propPath = PathFactoryImpl.getInstance().create(srcPath, propName, true);
+                if (!srcAccessMgr.canRead(propPath)) {
                     continue;
                 }
                 PropertyId propId = new PropertyId(srcState.getNodeId(), propName);

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/DefaultSecurityManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/DefaultSecurityManager.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/DefaultSecurityManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/DefaultSecurityManager.java Thu Apr 17 09:27:09 2008
@@ -16,6 +16,10 @@
  */
 package org.apache.jackrabbit.core;
 
+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.UserManager;
 import org.apache.jackrabbit.core.config.AccessManagerConfig;
 import org.apache.jackrabbit.core.config.BeanConfig;
 import org.apache.jackrabbit.core.config.LoginModuleConfig;
@@ -26,17 +30,11 @@
 import org.apache.jackrabbit.core.security.AccessManager;
 import org.apache.jackrabbit.core.security.JackrabbitSecurityManager;
 import org.apache.jackrabbit.core.security.SecurityConstants;
-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.UserManager;
 import org.apache.jackrabbit.core.security.authentication.AuthContext;
 import org.apache.jackrabbit.core.security.authentication.AuthContextProvider;
 import org.apache.jackrabbit.core.security.authorization.AccessControlProvider;
 import org.apache.jackrabbit.core.security.authorization.AccessControlProviderFactory;
 import org.apache.jackrabbit.core.security.authorization.AccessControlProviderFactoryImpl;
-import org.apache.jackrabbit.core.security.authorization.CompiledPermissions;
-import org.apache.jackrabbit.core.security.authorization.Permission;
 import org.apache.jackrabbit.core.security.authorization.WorkspaceAccessManager;
 import org.apache.jackrabbit.core.security.jsr283.security.AccessControlException;
 import org.apache.jackrabbit.core.security.principal.DefaultPrincipalProvider;
@@ -46,8 +44,6 @@
 import org.apache.jackrabbit.core.security.principal.PrincipalProviderRegistry;
 import org.apache.jackrabbit.core.security.principal.ProviderRegistryImpl;
 import org.apache.jackrabbit.core.security.user.UserManagerImpl;
-import org.apache.jackrabbit.spi.Path;
-import org.apache.jackrabbit.spi.commons.name.PathFactoryImpl;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -444,8 +440,6 @@
      */
     private class WorkspaceAccessManagerImpl implements SecurityConstants, WorkspaceAccessManager {
 
-        private final Path rootPath = PathFactoryImpl.getInstance().getRootPath();
-
         //-----------------------------------------< WorkspaceAccessManager >---
         /**
          * {@inheritDoc}
@@ -466,10 +460,8 @@
          */
         public boolean grants(Set principals, String workspaceName) throws RepositoryException {
             try {
-                // TODO: improve
                 AccessControlProvider prov = getAccessControlProvider(workspaceName);
-                CompiledPermissions cp = prov.compilePermissions(principals);
-                return cp.grants(rootPath, Permission.READ);
+                return prov.canAccessRoot(principals);
             } catch (NoSuchWorkspaceException e) {
                 // no such workspace -> return false.
                 return false;

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemImpl.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemImpl.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemImpl.java Thu Apr 17 09:27:09 2008
@@ -112,9 +112,7 @@
      *
      * @param itemMgr   the <code>ItemManager</code> that created this <code>Item</code>
      * @param session   the <code>Session</code> through which this <code>Item</code> is acquired
-     * @param id        id of this <code>Item</code>
-     * @param state     state associated with this <code>Item</code>
-     * @param listeners listeners on life cycle changes of this <code>ItemImpl</code>
+     * @param data      ItemData of this <code>Item</code>
      */
     ItemImpl(ItemManager itemMgr, SessionImpl session, ItemData data) {
         this.session = session;
@@ -613,8 +611,7 @@
         // walk through list of transient items and persist each one
         while (iter.hasNext()) {
             ItemState state = (ItemState) iter.next();
-            ItemImpl item = itemMgr.getItem(state.getId(),
-                    state.getStatus() == ItemState.STATUS_NEW);
+            ItemImpl item = itemMgr.getItem(state.getId());
             // persist state of transient item
             item.makePersistent();
         }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemManager.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemManager.java Thu Apr 17 09:27:09 2008
@@ -179,41 +179,138 @@
     }
 
     /**
-     * Retrieves state of item with given <code>id</code>. If the specified item
-     * doesn't exist an <code>ItemNotFoundException</code> will be thrown.
+     * Common implementation for all variants of item/node/propertyExists
+     * with both itemId or path param.
+     *
+     * @param itemId The id of the item to test.
+     * @param path Path of the item to check if known or <code>null</code>. In
+     * the latter case the test for access permission is executed using the
+     * itemId.
+     * @return true if the item with the given <code>itemId</code> exists AND
+     * can be read by this session.
+     */
+    private boolean itemExists(ItemId itemId, Path path) {
+        try {
+            // check sanity of session
+            session.sanityCheck();
+
+            // check if state exists for the given item
+            if (!itemStateProvider.hasItemState(itemId)) {
+                return false;
+            }
+
+            ItemData data = getItemData(itemId, path);
+            return data != null;
+        } catch (ItemNotFoundException infe) {
+            return false;
+        } catch (RepositoryException re) {
+            return false;
+        }
+    }
+
+    /**
+     * Common implementation for all variants of getItem/getNode/getProperty
+     * with both itemId or path parameter.
+     *
+     * @param itemId
+     * @param path Path of the item to retrieve or <code>null</code>. In
+     * the latter case the test for access permission is executed using the
+     * itemId.
+     * @return The item identified by the given <code>itemId</code>.
+     * @throws ItemNotFoundException
+     * @throws AccessDeniedException
+     * @throws RepositoryException
+     */
+    private ItemImpl getItem(ItemId itemId, Path path) throws ItemNotFoundException, AccessDeniedException, RepositoryException {
+        // check sanity of session
+        session.sanityCheck();
+
+        ItemData data = getItemData(itemId, path);
+        if (data == null) {
+            throw new AccessDeniedException("cannot read item " + itemId);
+        }
+        return createItemInstance(data);
+    }
+
+    /**
+     * Retrieves the data of the item with given <code>id</code>. If the
+     * specified item doesn't exist an <code>ItemNotFoundException</code> will
+     * be thrown.
      * If the item exists but the current session is not granted read access an
      * <code>AccessDeniedException</code> will be thrown.
      *
-     * @param id id of item to be retrieved
+     * @param itemId id of item to be retrieved
+     * @param path The path of the item to retrieve the data for or
+     * <code>null</code>. In the latter case the id (instead of the path) is
+     * used to test if READ permission is granted.
      * @return state state of said item
      * @throws ItemNotFoundException if no item with given <code>id</code> exists
      * @throws AccessDeniedException if the current session is not allowed to
      *                               read the said item
      * @throws RepositoryException   if another error occurs
      */
-    private ItemState getItemState(ItemId id)
+    private ItemData getItemData(ItemId itemId, Path path)
             throws ItemNotFoundException, AccessDeniedException,
             RepositoryException {
-        // check privileges
-        if (!canRead(id)) {
-            // clear cache
-            evictItems(id);
-            throw new AccessDeniedException("cannot read item " + id);
+        ItemData data = retrieveItem(itemId);
+        if (data == null) {
+            // not yet in cache, need to create instance:
+            // - retrieve item state
+            // - create instance of item data
+            // NOTE: permission check & caching within createItemData
+            ItemState state;
+            try {
+                state = itemStateProvider.getItemState(itemId);
+            } catch (NoSuchItemStateException nsise) {
+                throw new ItemNotFoundException(itemId.toString());
+            } catch (ItemStateException ise) {
+                String msg = "failed to retrieve item state of item " + itemId;
+                log.error(msg, ise);
+                throw new RepositoryException(msg, ise);
+            }
+            // create item data including: perm check and caching.
+            data = createItemData(state, path);
+        } else {
+            // already cached: check if read-permission is granted in order
+            // to have a consistent behaviour to 'itemExists' above.
+            if (!canRead(data, path)) {
+                // item exists but read-perm has been revoked in the mean time.
+                // -> remove from cache
+                evictItems(itemId);
+                return null;
+            }
         }
+        return data;
+    }
 
-        try {
-            return itemStateProvider.getItemState(id);
-        } catch (NoSuchItemStateException nsise) {
-            String msg = "no such item: " + id;
-            log.debug(msg);
-            throw new ItemNotFoundException(msg);
-        } catch (ItemStateException ise) {
-            String msg = "failed to retrieve item state of " + id;
-            log.error(msg);
-            throw new RepositoryException(msg, ise);
+    /**
+     * @param data
+     * @param path Path to be used for the permission check or <code>null</code>
+     * in which case the itemId present with the specified <code>data</code> is used.
+     * @return true if the item with the given <code>data</code> can be read;
+     * <code>false</code> otherwise.
+     * @throws AccessDeniedException
+     * @throws RepositoryException
+     */
+    private boolean canRead(ItemData data, Path path) throws AccessDeniedException, RepositoryException {
+        if (data.getState().getStatus() == ItemState.STATUS_NEW &&
+                !data.getDefinition().isProtected()) {
+            // NEW items can always be read as long they have been added
+            // through the API and NOT by the system (i.e. protected props).
+            return true;
+        } else {
+            return (path == null) ?
+                    canRead(data.getId()) :
+                    session.getAccessManager().canRead(path);
         }
     }
 
+    /**
+     * @param id
+     * @return true if the item with the given <code>id</code> can be read;
+     * <code>false</code> otherwise.
+     * @throws RepositoryException
+     */
     private boolean canRead(ItemId id) throws RepositoryException {
         return session.getAccessManager().isGranted(id, AccessManager.READ);
     }
@@ -235,7 +332,7 @@
             session.sanityCheck();
 
             ItemId id = hierMgr.resolvePath(path);
-            return (id != null) && itemExists(id);
+            return (id != null) && itemExists(id, path);
         } catch (RepositoryException re) {
             return false;
         }
@@ -253,7 +350,7 @@
             session.sanityCheck();
 
             NodeId id = hierMgr.resolveNodePath(path);
-            return (id != null) && itemExists(id);
+            return (id != null) && itemExists(id, path);
         } catch (RepositoryException re) {
             return false;
         }
@@ -271,7 +368,7 @@
             session.sanityCheck();
 
             PropertyId id = hierMgr.resolvePropertyPath(path);
-            return (id != null) && itemExists(id);
+            return (id != null) && itemExists(id, path);
         } catch (RepositoryException re) {
             return false;
         }
@@ -284,28 +381,7 @@
      * @return true if the specified item exists
      */
     public boolean itemExists(ItemId id) {
-        try {
-            // check sanity of session
-            session.sanityCheck();
-
-            // check if state exists for the given item
-            if (!itemStateProvider.hasItemState(id)) {
-                return false;
-            }
-
-            // check privileges
-            if (!canRead(id)) {
-                // clear cache
-                evictItems(id);
-                // item exists but the session has not been granted read access
-                return false;
-            }
-            return true;
-        } catch (ItemNotFoundException infe) {
-            return false;
-        } catch (RepositoryException re) {
-            return false;
-        }
+        return itemExists(id, null);
     }
 
     /**
@@ -337,7 +413,7 @@
             throw new PathNotFoundException(safeGetJCRPath(path));
         }
         try {
-            return getItem(id);
+            return getItem(id, path);
         } catch (ItemNotFoundException infe) {
             throw new PathNotFoundException(safeGetJCRPath(path));
         }
@@ -357,7 +433,7 @@
             throw new PathNotFoundException(safeGetJCRPath(path));
         }
         try {
-            return (NodeImpl) getItem(id);
+            return (NodeImpl) getItem(id, path);
         } catch (ItemNotFoundException infe) {
             throw new PathNotFoundException(safeGetJCRPath(path));
         }
@@ -377,7 +453,7 @@
             throw new PathNotFoundException(safeGetJCRPath(path));
         }
         try {
-            return (PropertyImpl) getItem(id);
+            return (PropertyImpl) getItem(id, path);
         } catch (ItemNotFoundException infe) {
             throw new PathNotFoundException(safeGetJCRPath(path));
         }
@@ -390,21 +466,7 @@
      */
     public synchronized ItemImpl getItem(ItemId id)
             throws ItemNotFoundException, AccessDeniedException, RepositoryException {
-        // check sanity of session
-        session.sanityCheck();
-
-        ItemData data = retrieveItem(id);
-        if (data == null) {
-            // not yet in cache, need to create instance:
-            // check privileges
-            if (!canRead(id)) {
-                throw new AccessDeniedException("cannot read item " + id);
-            }
-            // create instance of item data
-            data = createItemData(id);
-            cacheItem(data);
-        }
-        return createItemInstance(data);
+        return getItem(id, null);
     }
 
     /**
@@ -419,14 +481,12 @@
      */
     public synchronized NodeImpl getNode(NodeId id, NodeId parentId)
             throws ItemNotFoundException, AccessDeniedException, RepositoryException {
-
         if (parentId == null) {
             return (NodeImpl) getItem(id);
         }
         AbstractNodeData data = (AbstractNodeData) retrieveItem(id, parentId);
         if (data == null) {
-            data = (AbstractNodeData) createItemData(id);
-            cacheItem(data);
+            data = (AbstractNodeData) getItemData(id, null);
         }
         if (!data.getParentId().equals(parentId)) {
             // verify that parent actually appears in the shared set
@@ -435,6 +495,7 @@
                         + "' does not have shared parent with id: " + parentId;
                 throw new ItemNotFoundException(msg);
             }
+            // TODO: ev. need to check if read perm. is granted.
             data = new NodeDataRef(data, parentId);
             cacheItem(data);
         }
@@ -442,37 +503,9 @@
     }
 
     /**
-     * Returns the item instance for the given item id.
-     *
-     * @param state the item state
-     * @param checkAccess whether to check access
-     * @return the item instance for the given item <code>state</code>.
-     * @throws RepositoryException
-     */
-    synchronized ItemImpl getItem(ItemId id, boolean isNew)
-            throws ItemNotFoundException, AccessDeniedException, RepositoryException {
-        // check sanity of session
-        session.sanityCheck();
-
-        // check cache
-        ItemData data = retrieveItem(id);
-        if (data == null) {
-            // not yet in cache, need to create instance:
-            // only check privileges if state is not new
-            if (!isNew && !canRead(id)) {
-                throw new AccessDeniedException("cannot read item " + id);
-            }
-            // create instance of item
-            data = createItemData(id);
-            cacheItem(data);
-        }
-        return createItemInstance(data);
-    }
-
-    /**
      * Create an item instance from an item state. This method creates a
-     * new <code>ItemData</code> instance without looking at the cache and
-     * returns a new item instance.
+     * new <code>ItemData</code> instance without looking at the cache nor
+     * testing if the item can be read and returns a new item instance.
      *
      * @param state item state
      * @return item instance
@@ -480,9 +513,7 @@
      */
     synchronized ItemImpl createItemInstance(ItemState state)
             throws RepositoryException {
-
-        ItemData data = createItemData(state);
-        cacheItem(data);
+        ItemData data = createItemData(state, null, false);
         return createItemInstance(data);
     }
 
@@ -498,18 +529,17 @@
         // check sanity of session
         session.sanityCheck();
 
-        ItemState state = getItemState(parentId);
-        if (!state.isNode()) {
+        ItemData data = getItemData(parentId, null);
+        if (!data.isNode()) {
             String msg = "can't list child nodes of property " + parentId;
             log.debug(msg);
             throw new RepositoryException(msg);
         }
-        NodeState nodeState = (NodeState) state;
-        Iterator iter = nodeState.getChildNodeEntries().iterator();
+        Iterator iter = ((NodeState) data.getState()).getChildNodeEntries().iterator();
 
         while (iter.hasNext()) {
             NodeState.ChildNodeEntry entry = (NodeState.ChildNodeEntry) iter.next();
-            // check read access
+            // make sure any of the properties can be read.
             if (canRead(entry.getId())) {
                 return true;
             }
@@ -529,22 +559,20 @@
         // check sanity of session
         session.sanityCheck();
 
-        ItemState state = getItemState(parentId);
-        if (!state.isNode()) {
+        ItemData data = getItemData(parentId, null);
+        if (!data.isNode()) {
             String msg = "can't list child nodes of property " + parentId;
             log.debug(msg);
             throw new RepositoryException(msg);
         }
-        NodeState nodeState = (NodeState) state;
         ArrayList childIds = new ArrayList();
-        Iterator iter = nodeState.getChildNodeEntries().iterator();
+        Iterator iter = ((NodeState) data.getState()).getChildNodeEntries().iterator();
 
         while (iter.hasNext()) {
             NodeState.ChildNodeEntry entry = (NodeState.ChildNodeEntry) iter.next();
-            // check read access
-            if (canRead(entry.getId())) {
-                childIds.add(entry.getId());
-            }
+            // delay check for read-access until item is being built
+            // thus avoid duplicate check
+            childIds.add(entry.getId());
         }
 
         return new LazyItemIterator(this, childIds, parentId);
@@ -562,18 +590,17 @@
         // check sanity of session
         session.sanityCheck();
 
-        ItemState state = getItemState(parentId);
-        if (!state.isNode()) {
+        ItemData data = getItemData(parentId, null);
+        if (!data.isNode()) {
             String msg = "can't list child properties of property " + parentId;
             log.debug(msg);
             throw new RepositoryException(msg);
         }
-        NodeState nodeState = (NodeState) state;
-        Iterator iter = nodeState.getPropertyNames().iterator();
+        Iterator iter = ((NodeState) data.getState()).getPropertyNames().iterator();
 
         while (iter.hasNext()) {
             Name propName = (Name) iter.next();
-            // check read access
+            // make sure any of the properties can be read.
             if (canRead(new PropertyId(parentId, propName))) {
                 return true;
             }
@@ -594,58 +621,74 @@
         // check sanity of session
         session.sanityCheck();
 
-        ItemState state = getItemState(parentId);
-        if (!state.isNode()) {
+        ItemData data = getItemData(parentId, null);
+        if (!data.isNode()) {
             String msg = "can't list child properties of property " + parentId;
             log.debug(msg);
             throw new RepositoryException(msg);
         }
-        NodeState nodeState = (NodeState) state;
         ArrayList childIds = new ArrayList();
-        Iterator iter = nodeState.getPropertyNames().iterator();
+        Iterator iter = ((NodeState) data.getState()).getPropertyNames().iterator();
 
         while (iter.hasNext()) {
             Name propName = (Name) iter.next();
             PropertyId id = new PropertyId(parentId, propName);
-            // check read access
-            if (canRead(id)) {
-                childIds.add(id);
-            }
+            // delay check for read-access until item is being built
+            // thus avoid duplicate check
+            childIds.add(id);
         }
 
         return new LazyItemIterator(this, childIds);
     }
 
     //-------------------------------------------------< item factory methods >
-
-    private ItemData createItemData(ItemId id)
-            throws ItemNotFoundException, RepositoryException {
-
-        ItemState state;
-        try {
-            state = itemStateProvider.getItemState(id);
-        } catch (NoSuchItemStateException nsise) {
-            throw new ItemNotFoundException(id.toString());
-        } catch (ItemStateException ise) {
-            String msg = "failed to retrieve item state of item " + id;
-            log.error(msg, ise);
-            throw new RepositoryException(msg, ise);
-        }
-        return createItemData(state);
+    /**
+     * Same as {@link #createItemData(ItemState, Path, boolean)} where the
+     * permissionCheck flag is 'true'. This method will throw
+     * <code>AccessDeniedException</code> if reading the item data is not
+     * allowed.
+     *
+     * @param state
+     * @return
+     * @throws RepositoryException
+     */
+    private ItemData createItemData(ItemState state, Path path) throws RepositoryException {
+        return createItemData(state, path, true);
     }
 
-    private ItemData createItemData(ItemState state) throws RepositoryException {
+    /**
+     * Builds the <code>ItemData</code> for the specified <code>state</code>.
+     * If <code>permissionCheck</code> is <code>true</code>, the access manager
+     * is used to determine if reading that item would be granted. If this is
+     * not the case an <code>AccessDeniedException</code> is thrown.
+     * Before returning the created <code>ItemData</code> it is put into the
+     * cache. In order to benefit from the cache {@link #getItemData(ItemId, Path)}
+     * should be called
+     *
+     * @param state
+     * @return
+     * @throws RepositoryException
+     */
+    private ItemData createItemData(ItemState state, Path path, boolean permissionCheck) throws RepositoryException {
+        ItemData data;
         ItemId id = state.getId();
         if (id.equals(rootNodeId)) {
             // special handling required for root node
-            return new NodeData((NodeState) state, rootNodeDef);
+            data = new NodeData((NodeState) state, rootNodeDef);
         } else if (state.isNode()) {
             NodeState nodeState = (NodeState) state;
-            return new NodeData(nodeState, getDefinition(nodeState));
+            data = new NodeData(nodeState, getDefinition(nodeState));
         } else {
             PropertyState propertyState = (PropertyState) state;
-            return new PropertyData(propertyState, getDefinition(propertyState));
+            data = new PropertyData(propertyState, getDefinition(propertyState));
+        }
+        // make sure read-perm. is granted before returning the data.
+        if (permissionCheck && !canRead(data, path)) {
+            throw new AccessDeniedException("cannot read item " + state.getId());
         }
+        // before returning the data: put them into the cache.
+        cacheItem(data);
+        return data;
     }
 
     private ItemImpl createItemInstance(ItemData data) {
@@ -715,7 +758,7 @@
      * Puts the reference of an item in the cache with
      * the item's path as the key.
      *
-     * @param item the item to cache
+     * @param data the item data to cache
      */
     private void cacheItem(ItemData data) {
         synchronized (itemCache) {
@@ -760,7 +803,7 @@
     /**
      * Removes a cache entry for a specific item.
      *
-     * @param id id of the item to remove from the cache
+     * @param data The item data to remove from the cache
      */
     private void evictItem(ItemData data) {
         if (log.isDebugEnabled()) {

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/LazyItemIterator.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/LazyItemIterator.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/LazyItemIterator.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/LazyItemIterator.java Thu Apr 17 09:27:09 2008
@@ -26,6 +26,7 @@
 import javax.jcr.PropertyIterator;
 import javax.jcr.RepositoryException;
 import javax.jcr.ItemNotFoundException;
+import javax.jcr.AccessDeniedException;
 import java.util.List;
 import java.util.NoSuchElementException;
 import java.util.ArrayList;
@@ -110,6 +111,11 @@
                     next = itemMgr.getItem(id);
                 }
             } catch (ItemNotFoundException e) {
+                log.debug("ignoring nonexistent item " + id);
+                // remove invalid id
+                idList.remove(pos);
+                // try next
+            } catch (AccessDeniedException e) {
                 log.debug("ignoring nonexistent item " + id);
                 // remove invalid id
                 idList.remove(pos);

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/NodeImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/NodeImpl.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/NodeImpl.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/NodeImpl.java Thu Apr 17 09:27:09 2008
@@ -122,10 +122,7 @@
      *
      * @param itemMgr    the <code>ItemManager</code> that created this <code>Node</code> instance
      * @param session    the <code>Session</code> through which this <code>Node</code> is acquired
-     * @param id         id of this <code>Node</code>
-     * @param state      state associated with this <code>Node</code>
-     * @param definition definition of <i>this</i> <code>Node</code>
-     * @param listeners  listeners on life cylce changes of this <code>NodeImpl</code>
+     * @param data       the node data
      */
     protected NodeImpl(ItemManager itemMgr, SessionImpl session, AbstractNodeData data) {
         super(itemMgr, session, data);
@@ -522,7 +519,12 @@
         // create Node instance wrapping new node state
         NodeImpl node;
         try {
-            node = (NodeImpl) itemMgr.getItem(id);
+            // NOTE: since the node is not yet connected to its parent, avoid
+            // calling ItemManager#getItem(ItemId) which may include a permission
+            // check (with subsequent usage of the hierarachy-mgr -> error).
+            // just let the mgr create the new node that is known to exist and
+            // which has not been accessed before.
+            node = (NodeImpl) itemMgr.createItemInstance(nodeState);
         } catch (RepositoryException re) {
             // something went wrong
             stateMgr.disposeTransientItemState(nodeState);

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SystemSession.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SystemSession.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SystemSession.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SystemSession.java Thu Apr 17 09:27:09 2008
@@ -166,6 +166,16 @@
         }
 
         /**
+         * Always returns true.
+         *
+         * @see AccessManager#canRead(Path)
+         * @param itemPath
+         */
+        public boolean canRead(Path itemPath) throws ItemNotFoundException, RepositoryException {
+            return true;
+        }
+
+        /**
          * {@inheritDoc}
          *
          * @return always <code>true</code>

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/observation/EventConsumer.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/observation/EventConsumer.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/observation/EventConsumer.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/observation/EventConsumer.java Thu Apr 17 09:27:09 2008
@@ -150,7 +150,7 @@
                 ItemId targetId = state.getTargetId();
                 boolean granted = false;
                 try {
-                    granted = session.getAccessManager().isGranted(targetId, AccessManager.READ);
+                    granted = canRead(targetId);
                 } catch (RepositoryException e) {
                     log.warn("Unable to check access rights for item: " + targetId);
                 }
@@ -181,7 +181,7 @@
             // check read permission
             boolean granted = false;
             try {
-                granted = session.getAccessManager().isGranted(item.getId(), AccessManager.READ);
+                granted = canRead(item.getId());
             } catch (RepositoryException e) {
                 log.warn("Unable to check access rights for item: " + item.getId());
             }
@@ -213,7 +213,7 @@
                     || state.getType() == Event.PROPERTY_ADDED
                     || state.getType() == Event.PROPERTY_CHANGED) {
                 ItemId targetId = state.getTargetId();
-                if (!session.getAccessManager().isGranted(targetId, AccessManager.READ)) {
+                if (!canRead(targetId)) {
                     if (denied == null) {
                         denied = new HashSet();
                     }
@@ -269,5 +269,9 @@
             hashCode = session.hashCode() ^ listener.hashCode();
         }
         return hashCode;
+    }
+
+    private boolean canRead(ItemId itemId) throws RepositoryException {
+        return session.getAccessManager().isGranted(itemId, AccessManager.READ);
     }
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/query/lucene/QueryResultImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/query/lucene/QueryResultImpl.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/query/lucene/QueryResultImpl.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/query/lucene/QueryResultImpl.java Thu Apr 17 09:27:09 2008
@@ -25,11 +25,11 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import javax.jcr.NodeIterator;
-import javax.jcr.RepositoryException;
 import javax.jcr.ItemNotFoundException;
-import javax.jcr.Node;
 import javax.jcr.NamespaceException;
+import javax.jcr.Node;
+import javax.jcr.NodeIterator;
+import javax.jcr.RepositoryException;
 import javax.jcr.query.QueryResult;
 import javax.jcr.query.RowIterator;
 import java.io.IOException;
@@ -338,8 +338,8 @@
             throws RepositoryException {
         for (int i = 0; i < nodes.length; i++) {
             try {
-                if (nodes[i] != null && !accessMgr.isGranted(
-                        nodes[i].getNodeId(), AccessManager.READ)) {
+                // TODO: rather use AccessManager.canRead(Path)
+                if (nodes[i] != null && !accessMgr.isGranted(nodes[i].getNodeId(), AccessManager.READ)) {
                     return false;
                 }
             } catch (ItemNotFoundException e) {

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/AccessManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/AccessManager.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/AccessManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/AccessManager.java Thu Apr 17 09:27:09 2008
@@ -170,6 +170,16 @@
     boolean isGranted(Path parentPath, Name childName, int permissions) throws ItemNotFoundException, RepositoryException;
 
     /**
+     * Determines whether the item with the specified id can be read.
+     *
+     * @param itemPath
+     * @return <code>true</code> if the item can be read; otherwise <code>false</code>.
+     * @throws ItemNotFoundException
+     * @throws RepositoryException
+     */
+    boolean canRead(Path itemPath) throws ItemNotFoundException, RepositoryException;
+
+    /**
      * Determines whether the subject of the current context is granted access
      * to the given workspace. Note that an implementation is free to test for
      * the existance of a workspace with the specified name. In this case

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/DefaultAccessManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/DefaultAccessManager.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/DefaultAccessManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/DefaultAccessManager.java Thu Apr 17 09:27:09 2008
@@ -151,6 +151,10 @@
                 public int getPrivileges(Path absPath) throws RepositoryException {
                     return 0;
                 }
+
+                public boolean canReadAll() throws RepositoryException {
+                    return false;
+                }
             };
         }
 
@@ -192,24 +196,28 @@
     public boolean isGranted(ItemId id, int actions)
             throws ItemNotFoundException, RepositoryException {
         checkInitialized();
-        int perm = 0;
-        if ((actions & READ) == READ) {
-            perm |= Permission.READ;
-        }
-        if ((actions & WRITE) == WRITE) {
-            if (id.denotesNode()) {
-                // TODO: check again if correct
-                perm |= Permission.SET_PROPERTY;
-                perm |= Permission.ADD_NODE;
-            } else {
-                perm |= Permission.SET_PROPERTY;
+        if (actions == READ && compiledPermissions.canReadAll()) {
+            return true;
+        } else {
+            int perm = 0;
+            if ((actions & READ) == READ) {
+                perm |= Permission.READ;
             }
+            if ((actions & WRITE) == WRITE) {
+                if (id.denotesNode()) {
+                    // TODO: check again if correct
+                    perm |= Permission.SET_PROPERTY;
+                    perm |= Permission.ADD_NODE;
+                } else {
+                    perm |= Permission.SET_PROPERTY;
+                }
+            }
+            if ((actions & REMOVE) == REMOVE) {
+                perm |= (id.denotesNode()) ? Permission.REMOVE_NODE : Permission.REMOVE_PROPERTY;
+            }
+            Path path = hierMgr.getPath(id);
+            return isGranted(path, perm);
         }
-        if ((actions & REMOVE) == REMOVE) {
-            perm |= (id.denotesNode()) ? Permission.REMOVE_NODE : Permission.REMOVE_PROPERTY;
-        }
-        Path path = hierMgr.getPath(id);
-        return isGranted(path, perm);
     }
 
     public boolean isGranted(Path absPath, int permissions) throws RepositoryException {
@@ -226,6 +234,18 @@
     }
 
     /**
+     * @see AccessManager#canRead(Path)
+     */
+    public boolean canRead(Path itemPath) throws ItemNotFoundException, RepositoryException {
+        Path path;
+        if (compiledPermissions.canReadAll()) {
+            return true;
+        } else {
+            return isGranted(itemPath, Permission.READ);
+        }
+    }
+
+    /**
      * @see AccessManager#canAccess(String)
      */
     public boolean canAccess(String workspaceName) throws NoSuchWorkspaceException, RepositoryException {
@@ -282,7 +302,7 @@
         checkPrivileges(absPath, PrivilegeRegistry.READ_AC);
 
         // TODO: acProvider may not retrieve the correct policy in case of transient modifications
-        return acProvider.getPolicy(getNodeId(absPath));
+        return acProvider.getPolicy(getPath(absPath));
     }
 
     /**
@@ -351,7 +371,7 @@
         checkInitialized();
         checkPrivileges(absPath, PrivilegeRegistry.READ_AC);
 
-        return acProvider.getAccessControlEntries(getNodeId(absPath));
+        return acProvider.getAccessControlEntries(getPath(absPath));
     }
 
     /**
@@ -450,12 +470,13 @@
         return (compiledPermissions.getPrivileges(p) | ~privileges) == -1;
     }
 
-    private NodeId getNodeId(String absPath) throws RepositoryException {
-        NodeId id = hierMgr.resolveNodePath(resolver.getQPath(absPath));
+    private Path getPath(String absPath) throws RepositoryException {
+        Path path = resolver.getQPath(absPath);
+        NodeId id = hierMgr.resolveNodePath(path);
         if (id == null) {
             throw new PathNotFoundException(absPath);
         }
-        return id;
+        return path;
     }
 
     /**

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/SimpleJBossAccessManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/SimpleJBossAccessManager.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/SimpleJBossAccessManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/SimpleJBossAccessManager.java Thu Apr 17 09:27:09 2008
@@ -119,6 +119,10 @@
         return internalIsGranted(permissions);
     }
 
+    public boolean canRead(Path itemPath) throws ItemNotFoundException, RepositoryException {
+        return true;
+    }
+
     public boolean canAccess(String workspaceName) {
         return system || anonymous;
     }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractAccessControlProvider.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractAccessControlProvider.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractAccessControlProvider.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractAccessControlProvider.java Thu Apr 17 09:27:09 2008
@@ -18,7 +18,6 @@
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.apache.jackrabbit.core.NodeId;
 import org.apache.jackrabbit.core.SessionImpl;
 import org.apache.jackrabbit.core.security.jsr283.security.AccessControlPolicy;
 import org.apache.jackrabbit.core.security.jsr283.security.AccessControlEntry;
@@ -54,6 +53,7 @@
     protected NamePathResolver resolver;
 
     private boolean initialized;
+    private Principal everyone;
 
     protected AbstractAccessControlProvider() {
         this(AbstractAccessControlProvider.class.getName() + ": default Policy", null);
@@ -73,6 +73,20 @@
         }
     }
 
+    /**
+     * Simple test if the specified path points to an item that defines AC
+     * information.
+     * 
+     * @param absPath
+     * @return
+     */
+    protected abstract boolean isAcItem(Path absPath) throws RepositoryException;
+
+    /**
+     *
+     * @param principals
+     * @return
+     */
     protected static boolean isAdminOrSystem(Set principals) {
         for (Iterator it = principals.iterator(); it.hasNext();) {
             Principal p = (Principal) it.next();
@@ -83,6 +97,10 @@
         return false;
     }
 
+    /**
+     *
+     * @return
+     */
     protected static CompiledPermissions getAdminPermissions() {
         return new CompiledPermissions() {
             public void close() {
@@ -94,19 +112,50 @@
             public int getPrivileges(Path absPath) throws RepositoryException {
                 return PrivilegeRegistry.ALL;
             }
+            public boolean canReadAll() throws RepositoryException {
+                return true;
+            }
         };
     }
 
-    protected static CompiledPermissions getReadOnlyPermissions() {
+    /**
+     * Simple implementation to determine if the given set of principals
+     * only will result in read-only access.
+     *
+     * @param principals
+     * @return true if the given set only contains the everyone group.
+     */
+    protected boolean isReadOnly(Set principals) {
+        // TODO: improve. need to detect if 'anonymous' is included.
+        return principals.size() == 1 && principals.contains(everyone);
+    }
+
+    /**
+     *
+     * @return
+     */
+    protected CompiledPermissions getReadOnlyPermissions() {
         return new CompiledPermissions() {
             public void close() {
                 //nop
             }
             public boolean grants(Path absPath, int permissions) throws RepositoryException {
-                return permissions == Permission.READ;
+                if (isAcItem(absPath)) {
+                    // read-only never has read-AC permission
+                    return false;
+                } else {
+                    return permissions == Permission.READ;
+                }
             }
             public int getPrivileges(Path absPath) throws RepositoryException {
-                return PrivilegeRegistry.READ;
+                if (isAcItem(absPath)) {
+                    return PrivilegeRegistry.NO_PRIVILEGE;
+                } else {
+                    return PrivilegeRegistry.READ;
+                }
+            }
+            public boolean canReadAll() throws RepositoryException {
+                return false;
             }
         };
     }
@@ -132,6 +181,8 @@
         observationMgr = systemSession.getWorkspace().getObservationManager();
         resolver = (SessionImpl) systemSession;
 
+        everyone = session.getPrincipalManager().getEveryone();
+
         initialized = true;
     }
 
@@ -144,9 +195,10 @@
     }
 
     /**
-     * @see AccessControlProvider#getPolicy(NodeId)
+     * @see AccessControlProvider#getPolicy(Path)
+     * @param absPath
      */
-    public AccessControlPolicy getPolicy(NodeId nodeId) throws ItemNotFoundException, RepositoryException {
+    public AccessControlPolicy getPolicy(Path absPath) throws ItemNotFoundException, RepositoryException {
         checkInitialized();
         return new AccessControlPolicy() {
             public String getName() throws RepositoryException {
@@ -159,9 +211,10 @@
     }
 
     /**
-     * @see AccessControlProvider#getAccessControlEntries(NodeId)
+     * @see AccessControlProvider#getAccessControlEntries(Path)
+     * @param absPath
      */
-    public AccessControlEntry[] getAccessControlEntries(NodeId nodeId) throws RepositoryException {
+    public AccessControlEntry[] getAccessControlEntries(Path absPath) throws RepositoryException {
         checkInitialized();
         // always empty array, since aces will never be changed using the api.
         return new AccessControlEntry[0];

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractCompiledPermissions.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractCompiledPermissions.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractCompiledPermissions.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AbstractCompiledPermissions.java Thu Apr 17 09:27:09 2008
@@ -93,6 +93,13 @@
         return getResult(absPath).getPrivileges();
     }
 
+    /**
+     * @see CompiledPermissions#canReadAll()
+     */
+    public boolean canReadAll() throws RepositoryException {
+        return false;
+    }
+
     //--------------------------------------------------------< inner class >---
 
     protected class Result {

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlEditor.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlEditor.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlEditor.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlEditor.java Thu Apr 17 09:27:09 2008
@@ -16,7 +16,6 @@
  */
 package org.apache.jackrabbit.core.security.authorization;
 
-import org.apache.jackrabbit.core.NodeId;
 import org.apache.jackrabbit.core.security.jsr283.security.AccessControlException;
 import org.apache.jackrabbit.core.security.jsr283.security.Privilege;
 import org.apache.jackrabbit.core.security.jsr283.security.AccessControlEntry;
@@ -42,7 +41,7 @@
      * Modification will therefore not take effect, until it is written back to
      * the editor and persisted.
      * <p/>
-     * Compared to the policy returned by {@link AccessControlProvider#getPolicy(NodeId)},
+     * Compared to the policy returned by {@link AccessControlProvider#getPolicy(org.apache.jackrabbit.spi.Path)},
      * the scope of the PolicyTemplate it limited to the Node itself and does
      * not take inherited elements into account.
      *
@@ -67,7 +66,7 @@
      * representation. Modification will therefore not take effect, until it is
      * written back to the editor and persisted.
      * <p/>
-     * Compared to the policy returned by {@link AccessControlProvider#getPolicy(NodeId)},
+     * Compared to the policy returned by {@link AccessControlProvider#getPolicy(org.apache.jackrabbit.spi.Path)},
      * the scope of the PolicyTemplate it limited to the Node itself and does
      * never not take inherited elements into account.
      *

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlProvider.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlProvider.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlProvider.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/AccessControlProvider.java Thu Apr 17 09:27:09 2008
@@ -16,9 +16,9 @@
  */
 package org.apache.jackrabbit.core.security.authorization;
 
-import org.apache.jackrabbit.core.NodeId;
 import org.apache.jackrabbit.core.security.jsr283.security.AccessControlEntry;
 import org.apache.jackrabbit.core.security.jsr283.security.AccessControlPolicy;
+import org.apache.jackrabbit.spi.Path;
 
 import javax.jcr.ItemNotFoundException;
 import javax.jcr.RepositoryException;
@@ -72,31 +72,31 @@
     void close();
 
     /**
-     * Returns the effective policy for the specified node.
+     * Returns the effective policy for the node at the given absPath.
      *
-     * @param nodeId
-     * @return The effective policy that applies at <code>nodeId</code>.
+     * @param absPath an absolute path.
+     * @return The effective policy that applies at <code>absPath</code>.
      * @throws ItemNotFoundException If no Node with the specified
-     * <code>nodeId</code> exists.
+     * <code>absPath</code> exists.
      * @throws RepositoryException If another error occurs.
      * @see org.apache.jackrabbit.core.security.jsr283.security.AccessControlManager#getEffectivePolicy(String)
      */
-    AccessControlPolicy getPolicy(NodeId nodeId) throws ItemNotFoundException, RepositoryException;
+    AccessControlPolicy getPolicy(Path absPath) throws ItemNotFoundException, RepositoryException;
 
     /**
-     * Returns the effective 'grant' access control entries for the specified
-     * item. An implementation may retrieve the entries from the effective
+     * Returns the effective 'grant' access control entries for the node at absPath.
+     * An implementation may retrieve the entries from the effective
      * policy or by other implementation specific means.
      *
-     * @param nodeId
+     * @param absPath an absolute path.
      * @return The effective access control entries or an empty array if
-     * no entries apply at <code>nodeId</code>.
+     * no entries apply at <code>absPath</code>.
      * @throws ItemNotFoundException If no Node with the specified
-     * <code>nodeId</code> exists.
+     * <code>absPath</code> exists.
      * @throws RepositoryException If an error occurs.
      * @see org.apache.jackrabbit.core.security.jsr283.security.AccessControlManager#getEffectiveAccessControlEntries(String)
      */
-    AccessControlEntry[] getAccessControlEntries(NodeId nodeId) throws RepositoryException;
+    AccessControlEntry[] getAccessControlEntries(Path absPath) throws RepositoryException;
 
     /**
      * Returns an <code>AccessControlEditor</code> for the given Session object
@@ -117,9 +117,20 @@
      * caller is adviced to pass a Set that respects the order of insertion.
      * @return The effective, compiled CompiledPolicy that applies for the
      * specified set of principals.
-     * @throws ItemNotFoundException If no Node with the specified
-     * <code>nodeId</code> exists.
-     * @throws RepositoryException If another error occurs.
+     * @throws RepositoryException If an error occurs.
+     */
+    CompiledPermissions compilePermissions(Set principals) throws RepositoryException;
+
+    /**
+     * Returns <code>true</code> if the given set of principals can access the
+     * root node of the workspace this provider has been built for;
+     * <code>false</code> otherwise.
+     *
+     * @param principals
+     * @return <code>true</code> if the given set of principals can access the
+     * root node of the workspace this provider has been built for;
+     * <code>false</code> otherwise.
+     * @throws RepositoryException
      */
-    CompiledPermissions compilePermissions(Set principals) throws ItemNotFoundException, RepositoryException;
+    boolean canAccessRoot(Set principals) throws RepositoryException;
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/CompiledPermissions.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/CompiledPermissions.java?rev=649172&r1=649171&r2=649172&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/CompiledPermissions.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authorization/CompiledPermissions.java Thu Apr 17 09:27:09 2008
@@ -19,7 +19,6 @@
 import org.apache.jackrabbit.spi.Path;
 
 import javax.jcr.RepositoryException;
-import javax.jcr.ItemNotFoundException;
 
 /**
  * <code>CompiledPermissions</code> represents the evaluation of an
@@ -46,8 +45,7 @@
      * defined by {@link Permission} encoded as a bitmask value
      * @return <code>true</code> if the specified permissions are granted,
      * <code>false</code> otherwise.
-     * @throws ItemNotFoundException if neither the path nor its direct
-     * ancestor point to an existing item.
+     * @throws RepositoryException if an error occurs.
      */
     boolean grants(Path absPath, int permissions) throws RepositoryException;
 
@@ -58,6 +56,19 @@
      *
      * @return the granted privileges at <code>absPath</code> or zero if
      * the path does not denote an existing <code>Node</code>.
+     * @throws RepositoryException if an error occurs
      */
     int getPrivileges(Path absPath) throws RepositoryException;
+
+    /**
+     * Returns <code>true</code> if READ permission is granted everywhere.
+     * This method acts as shortcut for {@link #grants(Path, int)} where
+     * permissions is {@link Permission#READ} and allows to shorten the
+     * evaluation time given the fact that a check for READ permission is
+     * considered to be the most frequent test.
+     *
+     * @return <code>true</code> if the READ permission is granted everywhere.
+     * @throws RepositoryException if an error occurs
+     */
+    boolean canReadAll() throws RepositoryException;
 }

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=649172&r1=649171&r2=649172&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 Apr 17 09:27:09 2008
@@ -16,12 +16,12 @@
  */
 package org.apache.jackrabbit.core.security.authorization.acl;
 
+import org.apache.jackrabbit.api.JackrabbitSession;
+import org.apache.jackrabbit.api.security.principal.PrincipalManager;
 import org.apache.jackrabbit.core.NodeId;
 import org.apache.jackrabbit.core.NodeImpl;
 import org.apache.jackrabbit.core.PropertyImpl;
 import org.apache.jackrabbit.core.security.SecurityConstants;
-import org.apache.jackrabbit.api.security.principal.PrincipalManager;
-import org.apache.jackrabbit.api.JackrabbitSession;
 import org.apache.jackrabbit.core.security.authorization.AbstractAccessControlProvider;
 import org.apache.jackrabbit.core.security.authorization.AbstractCompiledPermissions;
 import org.apache.jackrabbit.core.security.authorization.AccessControlConstants;
@@ -37,30 +37,31 @@
 import org.apache.jackrabbit.core.security.jsr283.security.Privilege;
 import org.apache.jackrabbit.core.security.principal.PrincipalImpl;
 import org.apache.jackrabbit.spi.Path;
-import org.apache.jackrabbit.util.ISO9075;
+import org.apache.jackrabbit.spi.commons.name.PathFactoryImpl;
 import org.apache.jackrabbit.util.Text;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.jcr.ItemNotFoundException;
+import javax.jcr.NodeIterator;
 import javax.jcr.RepositoryException;
 import javax.jcr.Session;
 import javax.jcr.Value;
-import javax.jcr.NodeIterator;
+import javax.jcr.Node;
 import javax.jcr.observation.Event;
 import javax.jcr.observation.EventIterator;
 import javax.jcr.observation.EventListener;
 import javax.jcr.query.Query;
 import javax.jcr.query.QueryManager;
 import java.security.Principal;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.List;
-import java.util.Arrays;
 
 /**
  * The ACLProvider generates access control policies out of the items stored
@@ -97,7 +98,19 @@
      */
     private NodeId rootNodeId;
 
-    private String policyNodeName;
+    //--------------------------------------< AbstractAccessControlProvider >---
+    /**
+     * @see AbstractAccessControlProvider#isAcItem(Path)
+     */
+    protected boolean isAcItem(Path absPath) throws RepositoryException {
+        Path.Element[] elems = absPath.getElements();
+        for (int i = 0; i < elems.length; i++) {
+            if (N_POLICY.equals(elems[i].getName())) {
+                return true;
+            }
+        }
+        return false;
+    }
 
     //----------------------------------------------< AccessControlProvider >---
     /**
@@ -110,7 +123,6 @@
         // minimal protection on the root node.
         NodeImpl root = (NodeImpl) session.getRootNode();
         rootNodeId = root.getNodeId();
-        policyNodeName = resolver.getJCRName(N_POLICY);
         systemEditor = new ACLEditor(systemSession);
 
         if (!isAccessControlled(root)) {
@@ -119,19 +131,21 @@
     }
 
     /**
-     * @see AccessControlProvider#getPolicy(NodeId)
+     * @see AccessControlProvider#getPolicy(Path)
+     * @param absPath
      */
-    public AccessControlPolicy getPolicy(NodeId nodeId) throws ItemNotFoundException, RepositoryException {
+    public AccessControlPolicy getPolicy(Path absPath) throws ItemNotFoundException, RepositoryException {
         checkInitialized();
-        return getACL(nodeId);
+        return getACL(absPath);
     }
 
     /**
-     * @see AccessControlProvider#getAccessControlEntries(NodeId)
+     * @see AccessControlProvider#getAccessControlEntries(Path)
+     * @param absPath
      */
-    public AccessControlEntry[] getAccessControlEntries(NodeId nodeId) throws RepositoryException {
+    public AccessControlEntry[] getAccessControlEntries(Path absPath) throws RepositoryException {
         checkInitialized();
-        ACLImpl acl = getACL(nodeId);
+        ACLImpl acl = getACL(absPath);
 
         // TODO: check again what the expected return value would be.
         // TODO: check again if correct. call probably expensive.
@@ -190,45 +204,59 @@
     /**
      * @see AccessControlProvider#compilePermissions(Set)
      */
-    public CompiledPermissions compilePermissions(Set principals) throws ItemNotFoundException, RepositoryException {
+    public CompiledPermissions compilePermissions(Set principals) throws RepositoryException {
         checkInitialized();
         if (isAdminOrSystem(principals)) {
             return getAdminPermissions();
+        } else if (isReadOnly(principals)) {
+            return getReadOnlyPermissions();
         } else {
             return new AclPermissions(principals);
         }
     }
 
+    /**
+     * @see AccessControlProvider#canAccessRoot(Set)
+     */
+    public boolean canAccessRoot(Set principals) throws RepositoryException {
+        checkInitialized();
+        if (isAdminOrSystem(principals)) {
+            return true;
+        } else {
+            return new AclPermissions(principals, false).grants(PathFactoryImpl.getInstance().getRootPath(), Permission.READ);
+        }
+    }
+
     //------------------------------------------------------------< private >---
     /**
-     * Build the ACL that is effective on the Node identified by
-     * <code>nodeId</code>. In contrast to {@link #getACL(NodeId, Set)}
+     * Build the ACL that is effective on the Node at
+     * <code>absPath</code>. In contrast to {@link #getACL(NodeImpl, Set)}
      * the returned ACL contains all entries that apply to that node.
      *
-     * @param nodeId
+     * @param absPath
      * @return
      * @throws ItemNotFoundException
      * @throws RepositoryException
      */
-    private ACLImpl getACL(NodeId nodeId) throws ItemNotFoundException, RepositoryException {
-        return getACL(nodeId, Collections.EMPTY_SET);
+    private ACLImpl getACL(Path absPath) throws ItemNotFoundException, RepositoryException {
+        return getACL((NodeImpl) session.getNode(session.getJCRPath(absPath)),
+                Collections.EMPTY_SET);
     }
 
     /**
-     * Build the ACL that is effective on the Node identified by
-     * <code>nodeId</code>, but only retrieve those entries that apply to
+     * Build the ACL that is effective on the Node at
+     * <code>absPath</code>, but only retrieve those entries that apply to
      * any of the principals whose name is present in the given
      * <code>principalNameFilter</code>.
      *
-     * @param nodeId
+     * @param node
      * @param principalNameFilter
      * @return
      * @throws ItemNotFoundException
      * @throws RepositoryException
      */
-    private ACLImpl getACL(NodeId nodeId, Set principalNameFilter) throws ItemNotFoundException, RepositoryException {
-        // -> build the acl for the Node identified by 'id'
-        NodeImpl node = session.getNodeById(nodeId);
+    private ACLImpl getACL(NodeImpl node, Set principalNameFilter) throws ItemNotFoundException, RepositoryException {
+        // -> build the acl for the Node
         ACLImpl acl;
         // check for special ACL building item
         if (protectsNode(node)) {
@@ -239,7 +267,7 @@
                 parentNode = (NodeImpl) node.getParent().getParent();
             }
             ACLImpl baseACL = buildAcl(parentNode, principalNameFilter);
-            acl = new ACLImpl(nodeId, baseACL, true);
+            acl = new ACLImpl(node.getNodeId(), baseACL, true);
         } else {
             // build Acl for non-protection node.
             acl = buildAcl(node, principalNameFilter);
@@ -380,36 +408,56 @@
     private class AclPermissions extends AbstractCompiledPermissions implements EventListener {
 
         private final Set principalNames;
-        private boolean readAllowed;
+
+        /**
+         * flag indicating that there is not 'deny READ'.
+         * -> simplify {@link #grants(Path, int)} in case of permissions == READ
+         */
+        private boolean readAllowed = false;
+        /**
+         * flag indicating if only READ is granted
+         * -> simplify {@link #grants(Path, int)} in case of permissions != READ
+         */
+        private boolean readOnly = false;
 
         private AclPermissions(Set principals) throws RepositoryException {
+            this(principals, true);
+        }
+        private AclPermissions(Set principals, boolean listenToEvents) throws RepositoryException {
             principalNames = new HashSet(principals.size());
             for (Iterator it = principals.iterator(); it.hasNext();) {
                 principalNames.add(((Principal) it.next()).getName());
             }
 
-            /*
-             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 = readAllowedEveryWhere(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)
-             - changing ACE-entry for any of the principals (PROPERTY_CHANGED)
-               > new permissions granted/denied
-               >
-             - removed ACE-entry for any of the principals (NODE_REMOVED)
-            */
-            int events = Event.PROPERTY_CHANGED | Event.NODE_ADDED | Event.NODE_REMOVED;
-            String[] ntNames = new String[] {
-                    resolver.getJCRName(NT_REP_ACE),
-                    resolver.getJCRName(NT_REP_ACL)
-            };
-            observationMgr.addEventListener(this, events, session.getRootNode().getPath(), true, null, ntNames, true);
+            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.
+                */
+                searchReadDeny(principalNames);
+                /*
+                Determine if there is any ACE node that grants another permission
+                than READ.
+                */
+                searchNonReadAllow(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)
+                 - changing ACE-entry for any of the principals (PROPERTY_CHANGED)
+                   > new permissions granted/denied
+                   >
+                 - removed ACE-entry for any of the principals (NODE_REMOVED)
+                */
+                int events = Event.PROPERTY_CHANGED | Event.NODE_ADDED | Event.NODE_REMOVED;
+                String[] ntNames = new String[] {
+                        resolver.getJCRName(NT_REP_ACE),
+                        resolver.getJCRName(NT_REP_ACL)
+                };
+                observationMgr.addEventListener(this, events, session.getRootNode().getPath(), true, null, ntNames, true);
+            }
         }
 
         /**
@@ -417,53 +465,74 @@
          * principals AND denies-READ.
          *
          * @param principalnames
-         * @return true if read is allowed on all items.
          */
-        private boolean readAllowedEveryWhere(Set principalnames) {
+        private void searchReadDeny(Set principalnames) {
             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(")[");
+                stmt.append(")[(");
 
                 // where the rep:principalName property exactly matches any of
                 // the given principalsNames
                 int i = 0;
                 Iterator itr = principalnames.iterator();
                 while (itr.hasNext()) {
-                    stmt.append("@");
-                    String pName = resolver.getJCRName(P_PRINCIPAL_NAME);
-                    stmt.append(ISO9075.encode(pName));
-                    stmt.append("='").append(itr.next().toString()).append("'");
+                    stmt.append("@").append(resolver.getJCRName(P_PRINCIPAL_NAME)).append(" eq ");
+                    stmt.append("'").append(itr.next().toString()).append("'");
                     if (++i < principalnames.size()) {
                         stmt.append(" or ");
                     }
                 }
                 // AND rep:privileges contains the READ privilege
-                stmt.append(" and jcr:like(@");
-                String pName = resolver.getJCRName(P_PRIVILEGES);
-                stmt.append(ISO9075.encode(pName));
-                stmt.append(",'%").append(Privilege.READ).append("%')");
-                stmt.append("]");
+                stmt.append(") and @ ");
+                stmt.append(resolver.getJCRName(P_PRIVILEGES));
+                stmt.append(" = '").append(Privilege.READ).append("']");
+
                 Query q = qm.createQuery(stmt.toString(), Query.XPATH);
 
                 NodeIterator it = q.execute().getNodes();
-                while (it.hasNext()) {
-                    String path = it.nextNode().getPath();
-                    // if there is a node that AND it is not below /accesscontrol
-                    // we cannot use the short-cut within 'grants'
-                    if (!Text.isDescendantOrEqual("/"+ N_ACCESSCONTROL, path)) {
-                        return false;
+                readAllowed =  !it.hasNext();
+            } catch (RepositoryException e) {
+                log.error(e.toString());
+                // unable to determine... -> no shortcut upon grants
+                readAllowed = false;
+            }
+        }
+
+        private void searchNonReadAllow(Set principalnames) {
+            try {
+                QueryManager qm = session.getWorkspace().getQueryManager();
+                StringBuffer stmt = new StringBuffer("/jcr:root");
+                stmt.append("//element(*,");
+                stmt.append(resolver.getJCRName(NT_REP_GRANT_ACE));
+                stmt.append(")[(");
+                // where the rep:principalName property exactly matches any of
+                // the given principalsNames
+                int i = 0;
+                Iterator itr = principalnames.iterator();
+                while (itr.hasNext()) {
+                    stmt.append("@").append(resolver.getJCRName(P_PRINCIPAL_NAME)).append(" eq ");
+                    stmt.append("'").append(itr.next().toString()).append("'");
+                    if (++i < principalnames.size()) {
+                        stmt.append(" or ");
                     }
                 }
-                // didn't find any matching ACE that denies READ for any
-                // of the principals.
-                return true;
+
+                // AND rep:privileges contains the READ privilege
+                stmt.append(") and @");
+                stmt.append(resolver.getJCRName(P_PRIVILEGES));
+                stmt.append(" ne \"").append(Privilege.READ).append("\"]");
+
+                Query q = qm.createQuery(stmt.toString(), Query.XPATH);
+
+                NodeIterator it = q.execute().getNodes();
+                readOnly =  !it.hasNext();
             } catch (RepositoryException e) {
                 log.error(e.toString());
                 // unable to determine... -> no shortcut upon grants
-                return false;
+                readOnly = false;
             }
         }
 
@@ -473,35 +542,33 @@
          */
         protected Result buildResult(Path absPath) throws RepositoryException {
             boolean existingNode = false;
-            NodeId nid = null;
+            Node node = null;
             String jcrPath = resolver.getJCRPath(absPath);
 
             if (session.nodeExists(jcrPath)) {
-                nid = session.getHierarchyManager().resolveNodePath(absPath);
+                node = session.getNode(jcrPath);
                 existingNode = true;
             } else {
                 // path points to existing prop or non-existing item (node or prop).
                 // -> find the nearest persisted node
-                Path parentPath = absPath.getAncestor(1);
-                while (nid == null) {
-                    nid = session.getHierarchyManager().resolveNodePath(parentPath);
-                    if (parentPath.getDepth() == Path.ROOT_DEPTH) {
-                        // root-node reached
+                String parentPath = Text.getRelativeParent(jcrPath, 1);
+                while (parentPath.length() > 0) {
+                    if (session.nodeExists(parentPath)) {
+                        node = session.getNode(parentPath);
                         break;
-                    } else {
-                        parentPath = parentPath.getAncestor(1);
                     }
+                    parentPath = Text.getRelativeParent(parentPath, 1);
                 }
             }
 
-            if (nid == null) {
+            if (node == null) {
                 // should never get here
                 throw new ItemNotFoundException("Item out of hierarchy.");
             }
 
             // build the ACL for the specified principals at path or at the
             // direct ancestor of path (that must be definition exist).
-            ACLImpl acl = getACL(nid, principalNames);
+            ACLImpl acl = getACL((NodeImpl) node, principalNames);
 
             // privileges to expose
             int privileges = acl.getPrivileges();
@@ -525,7 +592,7 @@
             try {
                 observationMgr.removeEventListener(this);
             } catch (RepositoryException e) {
-                log.error("Internal error: ", e.getMessage());
+                log.debug("Unable to unregister listener: ", e.getMessage());
             }
             super.close();
         }
@@ -536,15 +603,16 @@
          * @param permissions
          * @return
          * @throws RepositoryException
+         * @see CompiledPermissions#grants(Path, int)
          */
         public boolean grants(Path absPath, int permissions) throws RepositoryException {
-            // common check
-            if (permissions == Permission.READ && readAllowed &&
-                    /* easy check if path doesn't point to AC-content */
-                    resolver.getJCRPath(absPath).indexOf(policyNodeName) == -1) {
+            if (permissions == Permission.READ && readAllowed && !isAcItem(absPath)) {
                 return true;
+            } else if (permissions != Permission.READ && readOnly) {
+                return false;
+            } else {
+                return super.grants(absPath, permissions);
             }
-            return super.grants(absPath, permissions);
         }
 
         //--------------------------------------------------< EventListener >---
@@ -559,11 +627,7 @@
                 try {
                     Event ev = events.nextEvent();
                     String path = ev.getPath();
-                    if (Text.isDescendantOrEqual("/"+ N_ACCESSCONTROL, path)) {
-                        // access control change applies to the 'combined' acls
-                        // -> ignore
-                        continue;
-                    }
+                    // TODO: check if valid. check required.
 
                     switch (ev.getType()) {
                         case Event.NODE_ADDED:
@@ -580,6 +644,15 @@
                                     for (int i = 0; i < vs.length; i++) {
                                         if (Privilege.READ.equals(vs[i].getString())) {
                                             readAllowed = false;
+                                        }
+                                    }
+                                }
+                                // if ace is a new ALLOW -> check if obsoletes read-only
+                                if (readOnly && n.isNodeType(NT_REP_GRANT_ACE)) {
+                                    Value[] vs = n.getProperty(P_PRIVILEGES).getValues();
+                                    for (int i = 0; i < vs.length; i++) {
+                                        if (!Privilege.READ.equals(vs[i].getString())) {
+                                            readOnly = false;
                                         }
                                     }
                                 }

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=649172&r1=649171&r2=649172&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 Apr 17 09:27:09 2008
@@ -89,7 +89,7 @@
             throw new IllegalArgumentException("Node must be of type: " +
                     AccessControlConstants.NT_REP_ACL);
         }
-        path = aclNode.getPath();
+        path = aclNode.getParent().getPath();
         description = null;
         loadEntries(aclNode, principalNames);
     }
@@ -118,6 +118,14 @@
         PrivilegeRegistry.getBits(ace.getPrivileges());
     }
 
+    private List internalGetEntries() {
+        List l = new ArrayList();
+        for (Iterator it = entries.values().iterator(); it.hasNext();) {
+            l.addAll((List) it.next());
+        }
+        return l;
+    }
+
     private List internalGetEntries(Principal principal) {
         String principalName = principal.getName();
         if (entries.containsKey(principalName)) {
@@ -152,7 +160,6 @@
             if (entry.isAllow() == entries[i].isAllow()) {
                 // replace the existing entry with the new one at the end.
                 l.remove(i);
-                l.add(entry);
             } else {
                 complementEntry = t;
             }
@@ -170,8 +177,10 @@
                 l.remove(complementEntry);
                 ACEImpl tmpl = new ACEImpl(entry.getPrincipal(), resultPrivs, !entry.isAllow());
                 l.add(tmpl);
-            } /* else: complement entry is null or does not need to be modified.*/
+            } /* else: does not need to be modified.*/
         }
+        // finally add the new entry at the end.
+        l.add(entry);
         return true;
     }
 
@@ -282,17 +291,14 @@
      * @see PolicyTemplate#size()
      */
     public int size() {
-        return entries.size();
+        return internalGetEntries().size();
     }
 
     /**
      * @see PolicyTemplate#getEntries()
      */
     public PolicyEntry[] getEntries() {
-        List l = new ArrayList();
-        for (Iterator it = entries.values().iterator(); it.hasNext();) {
-            l.addAll((List) it.next());
-        }
+        List l = internalGetEntries();
         return (PolicyEntry[]) l.toArray(new PolicyEntry[l.size()]);
     }