Fix file and uploadfield permissions

filesystem/File.php

@@ -305,7 +305,7 @@ class File extends DataObject {
 		$result = $this->extendedCan('canEdit', $member);
 		if($result !== null) return $result;
 		
-		return true;
+		return Permission::checkMember($member, 'CMS_ACCESS_AssetAdmin');
 	}
 	
 	/**

forms/UploadField.php

@@ -1431,6 +1431,7 @@ class UploadField_ItemHandler extends RequestHandler {
 		// Check item permissions
 		$item = $this->getItem();
 		if(!$item) return $this->httpError(404);
+		if($item instanceof Folder) return $this->httpError(403);
 		if(!$item->canDelete()) return $this->httpError(403);
 
 		// Delete the file from the filesystem. The file will be removed
@@ -1452,6 +1453,7 @@ class UploadField_ItemHandler extends RequestHandler {
 		// Check item permissions
 		$item = $this->getItem();
 		if(!$item) return $this->httpError(404);
+		if($item instanceof Folder) return $this->httpError(403);
 		if(!$item->canEdit()) return $this->httpError(403);
 
 		Requirements::css(FRAMEWORK_DIR . '/css/UploadField.css');
@@ -1495,6 +1497,8 @@ class UploadField_ItemHandler extends RequestHandler {
 		// Check item permissions
 		$item = $this->getItem();
 		if(!$item) return $this->httpError(404);
+		if($item instanceof Folder) return $this->httpError(403);
+		if(!$item->canEdit()) return $this->httpError(403);
 
 		$form->saveInto($item);
 		$item->write();

tests/filesystem/FileTest.php

@@ -382,6 +382,30 @@ class FileTest extends SapphireTest {
 		$this->assertEquals($member1->ID, $file2->OwnerID, 'Owner not overwritten on existing files');
 	}
 
+	public function testCanEdit() {
+		$file = $this->objFromFixture('File', 'gif');
+
+		// Test anonymous permissions
+		Session::set('loggedInAs', null);
+		$this->assertFalse($file->canEdit(), "Anonymous users can't edit files");
+
+		// Test permissionless user
+		$this->objFromFixture('Member', 'frontend')->logIn();
+		$this->assertFalse($file->canEdit(), "Permissionless users can't edit files");
+
+		// Test cms non-asset user
+		$this->objFromFixture('Member', 'cms')->logIn();
+		$this->assertFalse($file->canEdit(), "Basic CMS users can't edit files");
+
+		// Test asset-admin user
+		$this->objFromFixture('Member', 'assetadmin')->logIn();
+		$this->assertTrue($file->canEdit(), "Asset admin users can edit files");
+
+		// Test admin
+		$this->objFromFixture('Member', 'admin')->logIn();
+		$this->assertTrue($file->canEdit(), "Admins can edit files");
+	}
+		
 	/////////////////////////////////////////////////////////////////////////////////////////////////////////////
 	
 	public function setUp() {

tests/filesystem/FileTest.yml

@@ -1,30 +1,59 @@
 Folder:
-   subfolder:
+  subfolder:
-      Name: FileTest-subfolder
+    Name: FileTest-subfolder
-   folder1:
+  folder1:
-      Name: FileTest-folder1
+    Name: FileTest-folder1
-   folder2:
+  folder2:
-      Name: FileTest-folder2
+    Name: FileTest-folder2
-   folder1-subfolder1:
+  folder1-subfolder1:
-      Name: FileTest-folder1-subfolder1
+    Name: FileTest-folder1-subfolder1
-      ParentID: =>Folder.folder1
+    ParentID: =>Folder.folder1
 File:
-   asdf:
+  asdf:
-      Filename: assets/FileTest.txt
+    Filename: assets/FileTest.txt
-   gif:
+  gif:
-      Filename: assets/FileTest.gif
+    Filename: assets/FileTest.gif
-   pdf:
+  pdf:
-      Filename: assets/FileTest.pdf
+    Filename: assets/FileTest.pdf
-   setfromname:
+  setfromname:
-      Name: FileTest.png
+    Name: FileTest.png
-      ParentID: 0
+    ParentID: 0
-   subfolderfile:
+  subfolderfile:
-      Filename: assets/FileTest-subfolder/FileTestSubfolder.txt
+    Filename: assets/FileTest-subfolder/FileTestSubfolder.txt
-      ParentID: =>Folder.subfolder
+    ParentID: =>Folder.subfolder
-   subfolderfile-setfromname:
+  subfolderfile-setfromname:
-      Name: FileTestSubfolder2.txt
+    Name: FileTestSubfolder2.txt
-      ParentID: =>Folder.subfolder
+    ParentID: =>Folder.subfolder
-   file1-folder1:
+  file1-folder1:
-      Filename: assets/FileTest-folder1/File1.txt
+    Filename: assets/FileTest-folder1/File1.txt
-      Name: File1.txt
+    Name: File1.txt
-      ParentID: =>Folder.folder1
+    ParentID: =>Folder.folder1
+Permission:
+  admin:
+    Code: ADMIN
+  cmsmain:
+    Code: CMS_ACCESS_LeftAndMain
+  assetadmin:
+    Code: CMS_ACCESS_AssetAdmin
+Group:
+  admins:
+    Title: Administrators
+    Permissions: =>Permission.admin
+  cmsusers:
+    Title: 'CMS Users'
+    Permissions: =>Permission.cmsmain
+  assetusers:
+    Title: 'Asset Users'
+    Permissions: =>Permission.cmsmain, =>Permission.assetadmin
+Member:
+  frontend:
+    Email: frontend@example.com
+  cms:
+    Email: cms@silverstripe.com
+    Groups: =>Group.cmsusers
+  admin:
+    Email: admin@silverstripe.com
+    Groups: =>Group.admins
+  assetadmin:
+    Email: assetadmin@silverstripe.com
+    Groups: =>Group.assetusers

tests/forms/uploadfield/UploadFieldTest.php

@@ -404,7 +404,12 @@ class UploadFieldTest extends FunctionalTest {
 		$this->assertFileNotExists($file4->FullPath, 'File is also removed from filesystem');
 
 		// Test record-based permissions
-		$response = $this->mockFileDelete('ManyManyFiles/', $fileNoDelete->ID);
+		$response = $this->mockFileDelete('ManyManyFiles', $fileNoDelete->ID);
+		$this->assertEquals(403, $response->getStatusCode());
+
+		// Test that folders can't be deleted
+		$folder = $this->objFromFixture('Folder', 'folder1-subfolder1');
+		$response = $this->mockFileDelete('ManyManyFiles', $folder->ID);
 		$this->assertEquals(403, $response->getStatusCode());
 	}
 
@@ -441,14 +446,13 @@ class UploadFieldTest extends FunctionalTest {
 
 		$record = $this->objFromFixture('UploadFieldTest_Record', 'record1');
 		$file4 = $this->objFromFixture('File', 'file4');
-		$file5 = $this->objFromFixture('File', 'file5');
 		$fileNoEdit = $this->objFromFixture('File', 'file-noedit');
-		$baseUrl = 'UploadFieldTest_Controller/Form/field/ManyManyFiles/item/' . $file4->ID;
+		$folder = $this->objFromFixture('Folder', 'folder1-subfolder1');
 
-		$response = $this->get($baseUrl . '/edit');
+		$response = $this->mockFileEditForm('ManyManyFiles', $file4->ID);
 		$this->assertFalse($response->isError());
 
-		$response = $this->post($baseUrl . '/EditForm', array('Title' => 'File 4 modified'));
+		$response = $this->mockFileEdit('ManyManyFiles', $file4->ID, array('Title' => 'File 4 modified'));
 		$this->assertFalse($response->isError());
 
 		$record = DataObject::get_by_id($record->class, $record->ID, false);
@@ -456,10 +460,17 @@ class UploadFieldTest extends FunctionalTest {
 		$this->assertEquals('File 4 modified', $file4->Title);
 
 		// Test record-based permissions
-		$response = $this->post(
+		$response = $this->mockFileEditForm('ManyManyFiles', $fileNoEdit->ID);
-			'UploadFieldTest_Controller/Form/field/ManyManyFiles/item/' . $fileNoEdit->ID . '/edit',
+		$this->assertEquals(403, $response->getStatusCode());
-			array()
+
-		);
+		$response = $this->mockFileEdit('ManyManyFiles', $fileNoEdit->ID, array());
+		$this->assertEquals(403, $response->getStatusCode());
+
+		// Test folder permissions
+		$response = $this->mockFileEditForm('ManyManyFiles', $folder->ID);
+		$this->assertEquals(403, $response->getStatusCode());
+
+		$response = $this->mockFileEdit('ManyManyFiles', $folder->ID, array());
 		$this->assertEquals(403, $response->getStatusCode());
 	}
 
@@ -792,6 +803,34 @@ class UploadFieldTest extends FunctionalTest {
 		);
 	}
 
+	/**
+	 * Gets the edit form for the given file
+	 *
+	 * @param string $fileField Name of the field
+	 * @param integer $fileID ID of the file to delete
+	 * @return SS_HTTPResponse form response
+	 */
+	protected function mockFileEditForm($fileField, $fileID) {
+		return $this->get(
+			"UploadFieldTest_Controller/Form/field/{$fileField}/item/{$fileID}/edit"
+		);
+	}
+
+	/**
+	 * Mocks edit submissions to a file
+	 *
+	 * @param string $fileField Name of the field
+	 * @param integer $fileID ID of the file to delete
+	 * @param array $fields Fields to update
+	 * @return SS_HTTPResponse form response
+	 */
+	protected function mockFileEdit($fileField, $fileID, $fields = array()) {
+		return $this->post(
+			"UploadFieldTest_Controller/Form/field/{$fileField}/item/{$fileID}/EditForm",
+			$fields
+		);
+	}
+
 	/**
 	 * Simulates a physical file deletion
 	 * 
@@ -801,7 +840,7 @@ class UploadFieldTest extends FunctionalTest {
 	 */
 	protected function mockFileDelete($fileField, $fileID) {
 		return $this->post(
-			"UploadFieldTest_Controller/Form/field/HasOneFile/item/{$fileID}/delete",
+			"UploadFieldTest_Controller/Form/field/{$fileField}/item/{$fileID}/delete",
 			array()
 		);
 	}