KNL-1513 collectionId validation

kernel/api/src/main/java/org/sakaiproject/content/api/ContentHostingService.java

@@ -438,8 +438,9 @@ public ContentCollectionEdit addCollection(String id) throws IdUsedException, Id
 	 * Access a List of all the ContentResource objects in this path (and below) which the current user has access.
 	 * 
 	 * @param id
-	 *        A collection id.
+	 *        A collection id. This cannot be the root collection.
 	 * @return a List of the ContentResource objects.
+	 * @throws IllegalArgumentException If the getting all the resources for this collection isn't allowed (eg root).
 	 */
 	public List<ContentResource> getAllResources(String id);
 

kernel/kernel-impl/src/main/java/org/sakaiproject/content/impl/BaseContentService.java

@@ -1560,6 +1560,22 @@ public boolean isInsideIndividualDropbox(String id)
 		return isIndividualDropbox;
 	}
 
+	public boolean isInSiteCollection(String entityId)
+	{
+		return entityId.startsWith(COLLECTION_SITE);
+	}
+
+	public boolean isSiteLevelCollection(String id)
+	{
+		boolean isSiteLevelCollection = (id != null) && isInSiteCollection(id);
+		if(isSiteLevelCollection)
+		{
+			String[] parts = id.split(Entity.SEPARATOR);
+			isSiteLevelCollection = parts.length == 3 ;
+		}
+		return isSiteLevelCollection;
+	}
+
 	public String getSiteLevelDropboxId(String id)
 	{
 		String dropboxId = null;
@@ -2490,6 +2506,14 @@ public List<ContentResource> getAllResources(String id)
 	{
 		List<ContentResource> rv = new ArrayList<ContentResource>();
 
+		if (isRootCollection(id))
+		{
+			// There are performance issues with returning every single resources in one collection as well
+			// as issues in Sakai where actions incorrectly happen for the whole of the content service
+			// instead of just those of a site.
+			throw new IllegalArgumentException("Fetching of "+ id+ " is not allowed");
+		}
+
 		// get the collection members
 		try
 		{
@@ -9277,7 +9301,7 @@ protected boolean overQuota(ContentResourceEdit edit)
 
 		// some quick exits, if we are not doing user quota, or if this is not a user or group resource
 		// %%% These constants should be from somewhere else -ggolden
-		if (!((edit.getId().startsWith(COLLECTION_USER)) || (edit.getId().startsWith(COLLECTION_SITE)) || edit.getId().startsWith(COLLECTION_DROPBOX))) return false;
+		if (!((edit.getId().startsWith(COLLECTION_USER)) || isInSiteCollection(edit.getId()) || edit.getId().startsWith(COLLECTION_DROPBOX))) return false;
 
 		// expect null, "user" | "group", user/groupid, rest...
 		String[] parts = StringUtil.split(edit.getId(), Entity.SEPARATOR);
@@ -9393,7 +9417,7 @@ protected void removeSizeCache(ContentResourceEdit edit)
 
 		// some quick exits, if we are not doing user quota, or if this is not a user or group resource
 		// %%% These constants should be from somewhere else -ggolden
-		if (!((edit.getId().startsWith("/user/")) || (edit.getId().startsWith("/group/")))) return;
+		if (!((edit.getId().startsWith("/user/")) || isInSiteCollection(edit.getId()))) return;
 
 		// expect null, "user" | "group", user/groupid, rest...
 		String[] parts = StringUtil.split(edit.getId(), Entity.SEPARATOR);
@@ -9426,7 +9450,7 @@ protected void addSizeCache(ContentResourceEdit edit)
 
 		// some quick exits, if we are not doing user quota, or if this is not a user or group resource
 		// %%% These constants should be from somewhere else -ggolden
-		if (!((edit.getId().startsWith("/user/")) || (edit.getId().startsWith("/group/")))) return;
+		if (!((edit.getId().startsWith("/user/")) || isInSiteCollection(edit.getId()))) return;
 
 		// expect null, "user" | "group", user/groupid, rest...
 		String[] parts = StringUtil.split(edit.getId(), Entity.SEPARATOR);
@@ -14397,23 +14421,29 @@ public void hardDelete(String siteId) {
 		 * 
 		 * Therefore we need to delete the files (1 handled), then get any backed up files and delete them also (2 and 3 handled). Then delete the collection to finalise things.
 		 */
-
-    	//get collection for the site
-    	String collectionId = getSiteCollection(siteId);
-		M_log.debug("collectionId: " + collectionId);
-
-
-    	//handle 1
+
+		if (m_siteService.isSpecialSite(siteId)) {
+			M_log.error("hardDelete rejected special site: {}", siteId);
+			return;
+		}
+		// Get collection for the site and check validity
+		String collectionId = getSiteCollection(siteId);
+		if (!isSiteLevelCollection(collectionId)) {
+			M_log.error("hardDelete rejected on non site collection: {}", collectionId);
+			return;
+		}
+		M_log.info("hardDelete proceeding on collectionId: {}", collectionId);
+
+		//handle 1
 		try {
 			List<ContentResource> resources = getAllResources(collectionId);
 	    	for(ContentResource resource: resources) {
 				M_log.debug("Removing resource: " + resource.getId());
 	    		removeResource(resource.getId());
 	    	}
 		} catch (Exception e) {
-			e.printStackTrace();
-			//ignore and try to proceed
-		} 
+			M_log.warn("Failed to remove content.", e);
+		}
 
     	//handle2
 		//only for 2.10 - comment this out for 2.9 and below
@@ -14424,22 +14454,19 @@ public void hardDelete(String siteId) {
 	    		removeDeletedResource(deletedResource.getId());
 	    	}
 		} catch (Exception e) {
-			e.printStackTrace();
-			//ignore and try to proceed
-		} 
+			M_log.warn("Failed to remove some content.", e);
+		}
 
 		//cleanup
 		try {
 			M_log.debug("Removing collection: " + collectionId);
 			removeCollection(collectionId);
-
 		} catch (Exception e) {
-			e.printStackTrace();
-			//ignore and try to proceed
-		} 
-
-
+			M_log.warn("Failed to remove collection {}.", collectionId, e);
+		}
     }
+
+
 	private String getDisplayName(User userIn) {
 		User user = (userIn== null)?userDirectoryService.getCurrentUser():userIn ;
 		String displayId = user.getDisplayId();

kernel/kernel-impl/src/test/java/org/sakaiproject/content/impl/BaseContentServiceTest.java

@@ -0,0 +1,51 @@
+package org.sakaiproject.content.impl;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Simple checks for BaseContentService
+ */
+public class BaseContentServiceTest {
+
+    private BaseContentService baseContentService;
+
+    @Before
+    public void setUp() {
+        baseContentService = new DbContentService();
+    }
+
+    @Test
+    public void  testIsSiteLevelDropbox() {
+        assertFalse(baseContentService.isSiteLevelDropbox(""));
+        assertFalse(baseContentService.isSiteLevelDropbox("/"));
+        assertFalse(baseContentService.isSiteLevelDropbox("/group"));
+        assertFalse(baseContentService.isSiteLevelDropbox("/group/siteId"));
+        assertFalse(baseContentService.isSiteLevelDropbox("/group-user"));
+        assertFalse(baseContentService.isSiteLevelDropbox("/group-user/"));
+        assertFalse(baseContentService.isSiteLevelDropbox("/group-user//"));
+        assertFalse(baseContentService.isSiteLevelDropbox("/group-user//other"));
+        assertFalse(baseContentService.isSiteLevelDropbox("/group-user/siteId/other/"));
+
+        assertTrue(baseContentService.isSiteLevelDropbox("/group-user/siteId"));
+        assertTrue(baseContentService.isSiteLevelDropbox("/group-user/siteId/"));
+	}
+
+    @Test
+    public void  testIsSiteLevelCollection() {
+        assertFalse(baseContentService.isSiteLevelCollection(""));
+        assertFalse(baseContentService.isSiteLevelCollection("/"));
+        assertFalse(baseContentService.isSiteLevelCollection("/group"));
+        assertFalse(baseContentService.isSiteLevelCollection("/group/"));
+        assertFalse(baseContentService.isSiteLevelCollection("/group//"));
+        assertFalse(baseContentService.isSiteLevelCollection("/group///"));
+        assertFalse(baseContentService.isSiteLevelCollection("/group//other/"));
+        assertFalse(baseContentService.isSiteLevelCollection("/group/siteId/other/"));
+
+        assertTrue(baseContentService.isSiteLevelCollection("/group/siteId"));
+        assertTrue(baseContentService.isSiteLevelCollection("/group/siteId/"));
+    }
+}

webservices/cxf/src/java/org/sakaiproject/webservices/ContentHosting.java

@@ -861,6 +861,10 @@ public String siteHideResources(
 		Site site = null;
 		try {
 			 site = siteService.getSite(siteId);
+			 if (siteService.isSpecialSite(site.getId())) {
+				LOG.warn("siteHideResources: cannot run on a special site: {}", siteId);
+				return "failure";
+			 }
 		} catch (IdUnusedException iue) {
 			LOG.warn("WebService siteHideResources: site " + siteId + " doesn't exist");
 			return "failure";