From 7816875e92c13b6e02d647113a64011b0d8a8b8a Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 16 Dec 2014 15:43:24 +1300 Subject: [PATCH] Fix file and uploadfield permissions --- filesystem/File.php | 2 +- forms/UploadField.php | 4 + tests/filesystem/FileTest.php | 24 ++++++ tests/filesystem/FileTest.yml | 85 ++++++++++++++------- tests/forms/uploadfield/UploadFieldTest.php | 61 ++++++++++++--- 5 files changed, 136 insertions(+), 40 deletions(-) diff --git a/filesystem/File.php b/filesystem/File.php index 822790d5c..dd394208b 100644 --- a/filesystem/File.php +++ b/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'); } /** diff --git a/forms/UploadField.php b/forms/UploadField.php index 015661a69..c7431d1bc 100644 --- a/forms/UploadField.php +++ b/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(); diff --git a/tests/filesystem/FileTest.php b/tests/filesystem/FileTest.php index ed716eedc..1466810a8 100644 --- a/tests/filesystem/FileTest.php +++ b/tests/filesystem/FileTest.php @@ -381,6 +381,30 @@ class FileTest extends SapphireTest { $file2->write(); $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"); + } ///////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/tests/filesystem/FileTest.yml b/tests/filesystem/FileTest.yml index e4eeab45c..636339f15 100644 --- a/tests/filesystem/FileTest.yml +++ b/tests/filesystem/FileTest.yml @@ -1,30 +1,59 @@ Folder: - subfolder: - Name: FileTest-subfolder - folder1: - Name: FileTest-folder1 - folder2: - Name: FileTest-folder2 - folder1-subfolder1: - Name: FileTest-folder1-subfolder1 - ParentID: =>Folder.folder1 + subfolder: + Name: FileTest-subfolder + folder1: + Name: FileTest-folder1 + folder2: + Name: FileTest-folder2 + folder1-subfolder1: + Name: FileTest-folder1-subfolder1 + ParentID: =>Folder.folder1 File: - asdf: - Filename: assets/FileTest.txt - gif: - Filename: assets/FileTest.gif - pdf: - Filename: assets/FileTest.pdf - setfromname: - Name: FileTest.png - ParentID: 0 - subfolderfile: - Filename: assets/FileTest-subfolder/FileTestSubfolder.txt - ParentID: =>Folder.subfolder - subfolderfile-setfromname: - Name: FileTestSubfolder2.txt - ParentID: =>Folder.subfolder - file1-folder1: - Filename: assets/FileTest-folder1/File1.txt - Name: File1.txt - ParentID: =>Folder.folder1 + asdf: + Filename: assets/FileTest.txt + gif: + Filename: assets/FileTest.gif + pdf: + Filename: assets/FileTest.pdf + setfromname: + Name: FileTest.png + ParentID: 0 + subfolderfile: + Filename: assets/FileTest-subfolder/FileTestSubfolder.txt + ParentID: =>Folder.subfolder + subfolderfile-setfromname: + Name: FileTestSubfolder2.txt + ParentID: =>Folder.subfolder + file1-folder1: + Filename: assets/FileTest-folder1/File1.txt + Name: File1.txt + 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 diff --git a/tests/forms/uploadfield/UploadFieldTest.php b/tests/forms/uploadfield/UploadFieldTest.php index 27b37b02f..734716b45 100644 --- a/tests/forms/uploadfield/UploadFieldTest.php +++ b/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( - 'UploadFieldTest_Controller/Form/field/ManyManyFiles/item/' . $fileNoEdit->ID . '/edit', - array() - ); + $response = $this->mockFileEditForm('ManyManyFiles', $fileNoEdit->ID); + $this->assertEquals(403, $response->getStatusCode()); + + $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()); } @@ -791,7 +802,35 @@ class UploadFieldTest extends FunctionalTest { "UploadFieldTest_Controller/Form/field/{$fileField}/fileexists?filename=".urlencode($fileName) ); } - + + /** + * 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() ); }