Normal user can only change properties but not add child nodes.

- Fix to only check ancestor existence instead of getting each ancestor that might raise AccessDeniedException if the user lacks permission to an ancestor. Resolves: https://www.pivotaltracker.com/story/show/72982948
fcrepo · Nov 8, 2014 · 538d455 · 538d455
1 parent 3aa9e1f
commit 538d455
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 26 deletions.
diff --git a/...o-auth-common/src/test/java/org/fcrepo/auth/integration/ModeShapeHonorsFADResponseIT.java b/...o-auth-common/src/test/java/org/fcrepo/auth/integration/ModeShapeHonorsFADResponseIT.java
@@ -103,7 +103,7 @@ public void testPermissiveFAD() throws RepositoryException {
         }
         final ContainerService os = new ContainerServiceImpl();
         os.findOrCreate(session, "/myobject");
-        verify(fad, times(4)).hasPermission(any(Session.class), any(Path.class), any(String[].class));
+        verify(fad, times(5)).hasPermission(any(Session.class), any(Path.class), any(String[].class));
     }
 
     @Test(expected = AccessDeniedException.class)

diff --git a/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/utils/FedoraTypesUtils.java b/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/utils/FedoraTypesUtils.java
@@ -301,22 +301,29 @@ public static Node getClosestExistingAncestor(final Session session,
                                                   final String path) throws RepositoryException {
         final String[] pathSegments = path.replaceAll("^/+", "").replaceAll("/+$", "").split("/");
 
-        Node node = session.getRootNode();
+        final StringBuilder existingAncestorPath = new StringBuilder(path.length());
+        existingAncestorPath.append("/");
 
         final int len = pathSegments.length;
         for (int i = 0; i != len; ++i) {
             final String pathSegment = pathSegments[i];
 
-            if (node.hasNode(pathSegment)) {
-                // Find the existing node ...
-                node = node.getNode(pathSegment);
+            if (session.nodeExists(existingAncestorPath.toString() + pathSegment)) {
+                // Add to existingAncestorPath  ...
+                existingAncestorPath.append(pathSegment);
+                if (i != (len - 1)) {
+                existingAncestorPath.append("/");
+                }
             } else {
-                return node;
+                if (i != 0) {
+                    existingAncestorPath.deleteCharAt(existingAncestorPath.length() - 1);
+                }
+                break;
             }
 
         }
 
-        return node;
+        return session.getNode(existingAncestorPath.toString());
     }
 
 }
diff --git a/fcrepo-kernel-impl/src/test/java/org/fcrepo/kernel/impl/rdf/JcrRdfToolsTest.java b/fcrepo-kernel-impl/src/test/java/org/fcrepo/kernel/impl/rdf/JcrRdfToolsTest.java
@@ -342,8 +342,8 @@ public void shouldCreateHashUriSubjects() throws RepositoryException {
         testObj.jcrTools = mock(JcrTools.class);
         when(mockNode.getParent()).thenReturn(mockHashNode);
         when(mockHashNode.getParent()).thenReturn(mockChildNode);
-        when(mockRootNode.hasNode("some")).thenReturn(true);
-        when(mockRootNode.getNode("some")).thenReturn(mockChildNode);
+        when(mockSession.nodeExists("/some")).thenReturn(true);
+        when(mockSession.getNode("/some")).thenReturn(mockChildNode);
         when(mockChildNode.isNew()).thenReturn(false);
         when(testObj.jcrTools.findOrCreateNode(mockSession, "/some/#/abc", NT_FOLDER)).thenReturn(mockNode);
         when(mockHashNode.isNew()).thenReturn(true);
@@ -363,8 +363,8 @@ public void shouldCreateHashUriSubjectsWithExistingHashUri() throws RepositoryEx
         testObj.jcrTools = mock(JcrTools.class);
         when(mockNode.getParent()).thenReturn(mockHashNode);
         when(mockHashNode.getParent()).thenReturn(mockChildNode);
-        when(mockRootNode.hasNode("some")).thenReturn(true);
-        when(mockRootNode.getNode("some")).thenReturn(mockChildNode);
+        when(mockSession.nodeExists("/some")).thenReturn(true);
+        when(mockSession.getNode("/some")).thenReturn(mockChildNode);
         when(mockChildNode.isNew()).thenReturn(false);
         when(mockChildNode.hasNode("#")).thenReturn(true);
         when(mockChildNode.getNode("#")).thenReturn(mockHashNode);
@@ -385,7 +385,7 @@ public void shouldNotAllowHashUriSubjectsForResourcesThatDontExist() throws Repo
         testObj.jcrTools = mock(JcrTools.class);
         when(mockNode.getParent()).thenReturn(mockHashNode);
         when(mockHashNode.getParent()).thenReturn(mockChildNode);
-        when(mockRootNode.hasNode("some")).thenReturn(false);
+        when(mockSession.nodeExists("/some")).thenReturn(false);
         when(testObj.jcrTools.findOrCreateNode(mockSession, "/some/#/abc", NT_FOLDER)).thenReturn(mockNode);
         testObj.skolemize(testSubjects, x);
     }
@@ -400,8 +400,8 @@ public void shouldCreateHashUriObjects() throws RepositoryException {
         testObj.jcrTools = mock(JcrTools.class);
         when(mockNode.getParent()).thenReturn(mockHashNode);
         when(mockHashNode.getParent()).thenReturn(mockChildNode);
-        when(mockRootNode.hasNode("some")).thenReturn(true);
-        when(mockRootNode.getNode("some")).thenReturn(mockChildNode);
+        when(mockSession.nodeExists("/some")).thenReturn(true);
+        when(mockSession.getNode("/some")).thenReturn(mockChildNode);
         when(testObj.jcrTools.findOrCreateNode(mockSession, "/some/#/abc", NT_FOLDER)).thenReturn(mockNode);
         final Statement statement = testObj.skolemize(testSubjects, x);
         assertEquals(x, statement);

diff --git a/fcrepo-kernel-impl/src/test/java/org/fcrepo/kernel/impl/services/BinaryServiceImplTest.java b/fcrepo-kernel-impl/src/test/java/org/fcrepo/kernel/impl/services/BinaryServiceImplTest.java
@@ -64,6 +64,7 @@ public void testFindOrCreateBinary() throws Exception {
         final String testPath = "/foo/bar";
         when(mockRoot.getNode(testPath.substring(1))).thenReturn(mockDsNode);
         when(mockNode.isNodeType(FEDORA_BINARY)).thenReturn(true);
+        when(mockSession.getNode("/")).thenReturn(mockRoot);
         testObj.findOrCreate(mockSession, testPath);
         verify(mockRoot).getNode(testPath.substring(1));
     }

diff --git a/...o-kernel-impl/src/test/java/org/fcrepo/kernel/impl/services/ContainerServiceImplTest.java b/...o-kernel-impl/src/test/java/org/fcrepo/kernel/impl/services/ContainerServiceImplTest.java
@@ -70,6 +70,8 @@ public void setUp() throws RepositoryException {
         initMocks(this);
         testObj = new ContainerServiceImpl();
         when(mockSession.getRootNode()).thenReturn(mockRoot);
+        when(mockSession.nodeExists("/")).thenReturn(true);
+        when(mockSession.getNode("/")).thenReturn(mockRoot);
         when(mockRoot.getNode(testPath.substring(1))).thenReturn(mockNode);
         when(mockNode.getParent()).thenReturn(mockRoot);
         when(mockRoot.isNew()).thenReturn(false);
@@ -127,8 +129,8 @@ public void testThrowsTombstoneExceptionOnCreateOnTombstone() throws RepositoryE
         when(mockParent.getParent()).thenReturn(mockRoot);
         when(mockParent.isNew()).thenReturn(false);
         when(mockParent.isNodeType(FEDORA_TOMBSTONE)).thenReturn(true);
-        when(mockRoot.hasNode("foo")).thenReturn(true);
-        when(mockRoot.getNode("foo")).thenReturn(mockParent);
+        when(mockSession.nodeExists("/foo")).thenReturn(true);
+        when(mockSession.getNode("/foo")).thenReturn(mockParent);
         when(mockRoot.getNode("foo/bar")).thenReturn(mockNode);
         when(mockNode.isNew()).thenReturn(true);
 

diff --git a/fcrepo-kernel-impl/src/test/java/org/fcrepo/kernel/impl/utils/FedoraTypesUtilsTest.java b/fcrepo-kernel-impl/src/test/java/org/fcrepo/kernel/impl/utils/FedoraTypesUtilsTest.java
@@ -337,30 +337,30 @@ public void testProperty2values() throws RepositoryException {
 
     @Test
     public void testGetClosestExistingAncestorRoot() throws RepositoryException {
-        when(mockSession.getRootNode()).thenReturn(mockRootNode);
-        when(mockRootNode.hasNode(anyString())).thenReturn(false);
+        when(mockSession.getNode("/")).thenReturn(mockRootNode);
+        when(mockSession.nodeExists(anyString())).thenReturn(false);
 
         final Node closestExistingAncestor = getClosestExistingAncestor(mockSession, "/some/path");
         assertEquals(mockRootNode, closestExistingAncestor);
     }
 
     @Test
     public void testGetClosestExistingAncestorContainer() throws RepositoryException {
-        when(mockSession.getRootNode()).thenReturn(mockRootNode);
-        when(mockRootNode.hasNode("some")).thenReturn(true);
-        when(mockRootNode.getNode("some")).thenReturn(mockContainer);
+        when(mockSession.getNode("/")).thenReturn(mockRootNode);
+        when(mockSession.nodeExists("/some")).thenReturn(true);
+        when(mockSession.getNode("/some")).thenReturn(mockContainer);
 
         final Node closestExistingAncestor = getClosestExistingAncestor(mockSession, "/some/path");
         assertEquals(mockContainer, closestExistingAncestor);
     }
 
     @Test
     public void testGetClosestExistingAncestorNode() throws RepositoryException {
-        when(mockSession.getRootNode()).thenReturn(mockRootNode);
-        when(mockRootNode.hasNode("some")).thenReturn(true);
-        when(mockRootNode.getNode("some")).thenReturn(mockContainer);
-        when(mockContainer.hasNode("path")).thenReturn(true);
-        when(mockContainer.getNode("path")).thenReturn(mockNode);
+        when(mockSession.getNode("/")).thenReturn(mockRootNode);
+        when(mockSession.nodeExists("/some")).thenReturn(true);
+        when(mockSession.getNode("/some")).thenReturn(mockContainer);
+        when(mockSession.nodeExists("/some/path")).thenReturn(true);
+        when(mockSession.getNode("/some/path")).thenReturn(mockNode);
 
         final Node closestExistingAncestor = getClosestExistingAncestor(mockSession, "/some/path");
         assertEquals(mockNode, closestExistingAncestor);