Return-Path: X-Original-To: apmail-jackrabbit-commits-archive@www.apache.org Delivered-To: apmail-jackrabbit-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 169E23861 for ; Thu, 28 Apr 2011 08:41:09 +0000 (UTC) Received: (qmail 61128 invoked by uid 500); 28 Apr 2011 08:41:09 -0000 Delivered-To: apmail-jackrabbit-commits-archive@jackrabbit.apache.org Received: (qmail 61097 invoked by uid 500); 28 Apr 2011 08:41:09 -0000 Mailing-List: contact commits-help@jackrabbit.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@jackrabbit.apache.org Delivered-To: mailing list commits@jackrabbit.apache.org Received: (qmail 61090 invoked by uid 99); 28 Apr 2011 08:41:09 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 28 Apr 2011 08:41:09 +0000 X-ASF-Spam-Status: No, hits=-2000.0 required=5.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 28 Apr 2011 08:41:05 +0000 Received: by eris.apache.org (Postfix, from userid 65534) id DD6E12388A19; Thu, 28 Apr 2011 08:40:43 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r1097363 - in /jackrabbit/trunk/jackrabbit-core/src: main/java/org/apache/jackrabbit/core/ItemManager.java main/java/org/apache/jackrabbit/core/NodeImpl.java test/java/org/apache/jackrabbit/core/security/authorization/acl/WriteTest.java Date: Thu, 28 Apr 2011 08:40:43 -0000 To: commits@jackrabbit.apache.org From: angela@apache.org X-Mailer: svnmailer-1.0.8 Message-Id: <20110428084043.DD6E12388A19@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: angela Date: Thu Apr 28 08:40:43 2011 New Revision: 1097363 URL: http://svn.apache.org/viewvc?rev=1097363&view=rev Log: JCR-2951 - Item.remove fails if a child-item is not visible to the editing session Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/ItemManager.java jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/NodeImpl.java jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/acl/WriteTest.java 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=1097363&r1=1097362&r2=1097363&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 28 08:40:43 2011 @@ -304,15 +304,15 @@ public class ItemManager implements Item * @param path Path of the item to retrieve or null. In * the latter case the test for access permission is executed using the * itemId. + * @param permissionCheck * @return The item identified by the given itemId. * @throws ItemNotFoundException * @throws AccessDeniedException * @throws RepositoryException */ - private ItemImpl getItem(ItemId itemId, Path path) throws ItemNotFoundException, AccessDeniedException, RepositoryException { + private ItemImpl getItem(ItemId itemId, Path path, boolean permissionCheck) throws ItemNotFoundException, AccessDeniedException, RepositoryException { sanityCheck(); - boolean permissionCheck = true; ItemData data = getItemData(itemId, path, permissionCheck); return createItemInstance(data); } @@ -540,7 +540,7 @@ public class ItemManager implements Item throw new PathNotFoundException(safeGetJCRPath(path)); } try { - ItemImpl item = getItem(id, path); + ItemImpl item = getItem(id, path, true); // Test, if this item is a shareable node. if (item.isNode() && ((NodeImpl) item).isShareable()) { return getNode(path); @@ -570,7 +570,7 @@ public class ItemManager implements Item } try { if (parentId == null) { - return (NodeImpl) getItem(id, path); + return (NodeImpl) getItem(id, path, true); } // if the node is shareable, it now returns the node with the right // parent @@ -594,12 +594,12 @@ public class ItemManager implements Item throw new PathNotFoundException(safeGetJCRPath(path)); } try { - return (PropertyImpl) getItem(id, path); + return (PropertyImpl) getItem(id, path, true); } catch (ItemNotFoundException infe) { throw new PathNotFoundException(safeGetJCRPath(path)); } } - + /** * @param id * @return @@ -607,7 +607,17 @@ public class ItemManager implements Item */ public synchronized ItemImpl getItem(ItemId id) throws ItemNotFoundException, AccessDeniedException, RepositoryException { - return getItem(id, null); + return getItem(id, null, true); + } + + /** + * @param id + * @return + * @throws RepositoryException + */ + synchronized ItemImpl getItem(ItemId id, boolean permissionCheck) + throws ItemNotFoundException, AccessDeniedException, RepositoryException { + return getItem(id, null, permissionCheck); } /** @@ -622,12 +632,29 @@ public class ItemManager implements Item */ public synchronized NodeImpl getNode(NodeId id, NodeId parentId) throws ItemNotFoundException, AccessDeniedException, RepositoryException { + return getNode(id, parentId, true); + } + + /** + * Returns a node with a given id and parent id. If the indicated node is + * shareable, there might be multiple nodes associated with the same id, + * but there'is only one node with the given parent id. + * + * @param id node id + * @param parentId parent node id + * @param permissionCheck Flag indicating if read permission must be check + * upon retrieving the node. + * @return node + * @throws RepositoryException if an error occurs + */ + synchronized NodeImpl getNode(NodeId id, NodeId parentId, boolean permissionCheck) + throws ItemNotFoundException, AccessDeniedException, RepositoryException { if (parentId == null) { return (NodeImpl) getItem(id); } AbstractNodeData data = retrieveItem(id, parentId); if (data == null) { - data = (AbstractNodeData) getItemData(id); + data = (AbstractNodeData) getItemData(id, null, permissionCheck); } if (!data.getParentId().equals(parentId)) { // verify that parent actually appears in the shared set 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=1097363&r1=1097362&r2=1097363&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 28 08:40:43 2011 @@ -682,7 +682,14 @@ public class NodeImpl extends ItemImpl i NodeId childId = entry.getId(); //NodeImpl childNode = (NodeImpl) itemMgr.getItem(childId); try { - NodeImpl childNode = itemMgr.getNode(childId, getNodeId()); + /* omit the read-permission check upon retrieving the + child item as this is an internal call to remove the + subtree which may contain (protected) child items which + are not visible to the caller of the removal. the actual + validation of the remove permission however is only + executed during Item.save(). + */ + NodeImpl childNode = itemMgr.getNode(childId, getNodeId(), false); childNode.onRemove(thisState.getNodeId()); // remove the child node entry } catch (ItemNotFoundException e) { @@ -715,7 +722,14 @@ public class NodeImpl extends ItemImpl i thisState.removePropertyName(propName); // remove property PropertyId propId = new PropertyId(thisState.getNodeId(), propName); - itemMgr.getItem(propId).setRemoved(); + /* omit the read-permission check upon retrieving the + child item as this is an internal call to remove the + subtree which may contain (protected) child items which + are not visible to the caller of the removal. the actual + validation of the remove permission however is only + executed during Item.save(). + */ + itemMgr.getItem(propId, false).setRemoved(); } // finally remove this node Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/acl/WriteTest.java URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/acl/WriteTest.java?rev=1097363&r1=1097362&r2=1097363&view=diff ============================================================================== --- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/acl/WriteTest.java (original) +++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/authorization/acl/WriteTest.java Thu Apr 28 08:40:43 2011 @@ -422,4 +422,86 @@ public class WriteTest extends AbstractW n.addNode("someChild"); n.save(); } + + public void testRemoveNodeWithPolicy() throws Exception { + Privilege[] privileges = privilegesFromNames(new String[] {Privilege.JCR_READ, Privilege.JCR_WRITE}); + + /* allow READ/WRITE privilege for testUser at 'path' */ + givePrivileges(path, testUser.getPrincipal(), privileges, getRestrictions(superuser, path)); + /* allow READ/WRITE privilege for testUser at 'childPath' */ + givePrivileges(childNPath, testUser.getPrincipal(), privileges, getRestrictions(superuser, path)); + + Session testSession = getTestSession(); + + assertTrue(testSession.nodeExists(childNPath)); + assertTrue(testSession.hasPermission(childNPath, Session.ACTION_REMOVE)); + + Node n = testSession.getNode(childNPath); + + // removing the child node must succeed as both remove-node and + // remove-child-nodes are granted to testsession. + // the policy node underneath childNPath should silently be removed + // as the editing session has no knowledge about it's existence. + n.remove(); + testSession.save(); + } + + public void testRemoveNodeWithInvisibleChild() throws Exception { + Privilege[] privileges = privilegesFromNames(new String[] {Privilege.JCR_READ, Privilege.JCR_WRITE}); + + Node invisible = superuser.getNode(childNPath).addNode(nodeName3); + superuser.save(); + + /* allow READ/WRITE privilege for testUser at 'path' */ + givePrivileges(path, testUser.getPrincipal(), privileges, getRestrictions(superuser, path)); + /* deny READ privilege at invisible node. (removal is still granted) */ + withdrawPrivileges(invisible.getPath(), testUser.getPrincipal(), + privilegesFromNames(new String[] {Privilege.JCR_READ}), + getRestrictions(superuser, path)); + + Session testSession = getTestSession(); + + assertTrue(testSession.nodeExists(childNPath)); + assertTrue(testSession.hasPermission(childNPath, Session.ACTION_REMOVE)); + + Node n = testSession.getNode(childNPath); + + // removing the child node must succeed as both remove-node and + // remove-child-nodes are granted to testsession. + // the policy node underneath childNPath should silently be removed + // as the editing session has no knowledge about it's existence. + n.remove(); + testSession.save(); + } + + public void testRemoveNodeWithInvisibleNonRemovableChild() throws Exception { + Privilege[] privileges = privilegesFromNames(new String[] {Privilege.JCR_READ, Privilege.JCR_WRITE}); + + Node invisible = superuser.getNode(childNPath).addNode(nodeName3); + superuser.save(); + + /* allow READ/WRITE privilege for testUser at 'path' */ + givePrivileges(path, testUser.getPrincipal(), privileges, getRestrictions(superuser, path)); + /* deny READ privilege at invisible node. (removal is still granted) */ + withdrawPrivileges(invisible.getPath(), testUser.getPrincipal(), + privileges, + getRestrictions(superuser, path)); + + Session testSession = getTestSession(); + + assertTrue(testSession.nodeExists(childNPath)); + assertTrue(testSession.hasPermission(childNPath, Session.ACTION_REMOVE)); + + Node n = testSession.getNode(childNPath); + + // removing the child node must fail as a hidden child node cannot + // be removed. + try { + n.remove(); + testSession.save(); + fail(); + } catch (AccessDeniedException e) { + // success + } + } } \ No newline at end of file