Skip to content

Commit c49f164

Browse files
author
Damian Mooyman
committed
Merge pull request #3781 from tractorcow/pulls/3.1/fix-file-canedit
Fix file and uploadfield permissions
2 parents 146b468 + 7816875 commit c49f164

File tree

5 files changed

+136
-40
lines changed

5 files changed

+136
-40
lines changed

‎filesystem/File.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ public function canEdit($member = null) {
305305
$result = $this->extendedCan('canEdit', $member);
306306
if($result !== null) return $result;
307307

308-
return true;
308+
return Permission::checkMember($member, 'CMS_ACCESS_AssetAdmin');
309309
}
310310

311311
/**

‎forms/UploadField.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1431,6 +1431,7 @@ public function delete(SS_HTTPRequest $request) {
14311431
// Check item permissions
14321432
$item = $this->getItem();
14331433
if(!$item) return $this->httpError(404);
1434+
if($item instanceof Folder) return $this->httpError(403);
14341435
if(!$item->canDelete()) return $this->httpError(403);
14351436

14361437
// Delete the file from the filesystem. The file will be removed
@@ -1452,6 +1453,7 @@ public function edit(SS_HTTPRequest $request) {
14521453
// Check item permissions
14531454
$item = $this->getItem();
14541455
if(!$item) return $this->httpError(404);
1456+
if($item instanceof Folder) return $this->httpError(403);
14551457
if(!$item->canEdit()) return $this->httpError(403);
14561458

14571459
Requirements::css(FRAMEWORK_DIR . '/css/UploadField.css');
@@ -1495,6 +1497,8 @@ public function doEdit(array $data, Form $form, SS_HTTPRequest $request) {
14951497
// Check item permissions
14961498
$item = $this->getItem();
14971499
if(!$item) return $this->httpError(404);
1500+
if($item instanceof Folder) return $this->httpError(403);
1501+
if(!$item->canEdit()) return $this->httpError(403);
14981502

14991503
$form->saveInto($item);
15001504
$item->write();

‎tests/filesystem/FileTest.php

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,30 @@ public function testSetsOwnerOnFirstWrite() {
381381
$file2->write();
382382
$this->assertEquals($member1->ID, $file2->OwnerID, 'Owner not overwritten on existing files');
383383
}
384+
385+
public function testCanEdit() {
386+
$file = $this->objFromFixture('File', 'gif');
387+
388+
// Test anonymous permissions
389+
Session::set('loggedInAs', null);
390+
$this->assertFalse($file->canEdit(), "Anonymous users can't edit files");
391+
392+
// Test permissionless user
393+
$this->objFromFixture('Member', 'frontend')->logIn();
394+
$this->assertFalse($file->canEdit(), "Permissionless users can't edit files");
395+
396+
// Test cms non-asset user
397+
$this->objFromFixture('Member', 'cms')->logIn();
398+
$this->assertFalse($file->canEdit(), "Basic CMS users can't edit files");
399+
400+
// Test asset-admin user
401+
$this->objFromFixture('Member', 'assetadmin')->logIn();
402+
$this->assertTrue($file->canEdit(), "Asset admin users can edit files");
403+
404+
// Test admin
405+
$this->objFromFixture('Member', 'admin')->logIn();
406+
$this->assertTrue($file->canEdit(), "Admins can edit files");
407+
}
384408

385409
/////////////////////////////////////////////////////////////////////////////////////////////////////////////
386410

‎tests/filesystem/FileTest.yml

Lines changed: 57 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,59 @@
11
Folder:
2-
subfolder:
3-
Name: FileTest-subfolder
4-
folder1:
5-
Name: FileTest-folder1
6-
folder2:
7-
Name: FileTest-folder2
8-
folder1-subfolder1:
9-
Name: FileTest-folder1-subfolder1
10-
ParentID: =>Folder.folder1
2+
subfolder:
3+
Name: FileTest-subfolder
4+
folder1:
5+
Name: FileTest-folder1
6+
folder2:
7+
Name: FileTest-folder2
8+
folder1-subfolder1:
9+
Name: FileTest-folder1-subfolder1
10+
ParentID: =>Folder.folder1
1111
File:
12-
asdf:
13-
Filename: assets/FileTest.txt
14-
gif:
15-
Filename: assets/FileTest.gif
16-
pdf:
17-
Filename: assets/FileTest.pdf
18-
setfromname:
19-
Name: FileTest.png
20-
ParentID: 0
21-
subfolderfile:
22-
Filename: assets/FileTest-subfolder/FileTestSubfolder.txt
23-
ParentID: =>Folder.subfolder
24-
subfolderfile-setfromname:
25-
Name: FileTestSubfolder2.txt
26-
ParentID: =>Folder.subfolder
27-
file1-folder1:
28-
Filename: assets/FileTest-folder1/File1.txt
29-
Name: File1.txt
30-
ParentID: =>Folder.folder1
12+
asdf:
13+
Filename: assets/FileTest.txt
14+
gif:
15+
Filename: assets/FileTest.gif
16+
pdf:
17+
Filename: assets/FileTest.pdf
18+
setfromname:
19+
Name: FileTest.png
20+
ParentID: 0
21+
subfolderfile:
22+
Filename: assets/FileTest-subfolder/FileTestSubfolder.txt
23+
ParentID: =>Folder.subfolder
24+
subfolderfile-setfromname:
25+
Name: FileTestSubfolder2.txt
26+
ParentID: =>Folder.subfolder
27+
file1-folder1:
28+
Filename: assets/FileTest-folder1/File1.txt
29+
Name: File1.txt
30+
ParentID: =>Folder.folder1
31+
Permission:
32+
admin:
33+
Code: ADMIN
34+
cmsmain:
35+
Code: CMS_ACCESS_LeftAndMain
36+
assetadmin:
37+
Code: CMS_ACCESS_AssetAdmin
38+
Group:
39+
admins:
40+
Title: Administrators
41+
Permissions: =>Permission.admin
42+
cmsusers:
43+
Title: 'CMS Users'
44+
Permissions: =>Permission.cmsmain
45+
assetusers:
46+
Title: 'Asset Users'
47+
Permissions: =>Permission.cmsmain, =>Permission.assetadmin
48+
Member:
49+
frontend:
50+
Email: frontend@example.com
51+
cms:
52+
Email: cms@silverstripe.com
53+
Groups: =>Group.cmsusers
54+
admin:
55+
Email: admin@silverstripe.com
56+
Groups: =>Group.admins
57+
assetadmin:
58+
Email: assetadmin@silverstripe.com
59+
Groups: =>Group.assetusers

‎tests/forms/uploadfield/UploadFieldTest.php

Lines changed: 50 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,12 @@ public function testDeleteFromManyMany() {
404404
$this->assertFileNotExists($file4->FullPath, 'File is also removed from filesystem');
405405

406406
// Test record-based permissions
407-
$response = $this->mockFileDelete('ManyManyFiles/', $fileNoDelete->ID);
407+
$response = $this->mockFileDelete('ManyManyFiles', $fileNoDelete->ID);
408+
$this->assertEquals(403, $response->getStatusCode());
409+
410+
// Test that folders can't be deleted
411+
$folder = $this->objFromFixture('Folder', 'folder1-subfolder1');
412+
$response = $this->mockFileDelete('ManyManyFiles', $folder->ID);
408413
$this->assertEquals(403, $response->getStatusCode());
409414
}
410415

@@ -441,25 +446,31 @@ public function testEdit() {
441446

442447
$record = $this->objFromFixture('UploadFieldTest_Record', 'record1');
443448
$file4 = $this->objFromFixture('File', 'file4');
444-
$file5 = $this->objFromFixture('File', 'file5');
445449
$fileNoEdit = $this->objFromFixture('File', 'file-noedit');
446-
$baseUrl = 'UploadFieldTest_Controller/Form/field/ManyManyFiles/item/' . $file4->ID;
450+
$folder = $this->objFromFixture('Folder', 'folder1-subfolder1');
447451

448-
$response = $this->get($baseUrl . '/edit');
452+
$response = $this->mockFileEditForm('ManyManyFiles', $file4->ID);
449453
$this->assertFalse($response->isError());
450454

451-
$response = $this->post($baseUrl . '/EditForm', array('Title' => 'File 4 modified'));
455+
$response = $this->mockFileEdit('ManyManyFiles', $file4->ID, array('Title' => 'File 4 modified'));
452456
$this->assertFalse($response->isError());
453457

454458
$record = DataObject::get_by_id($record->class, $record->ID, false);
455459
$file4 = DataObject::get_by_id($file4->class, $file4->ID, false);
456460
$this->assertEquals('File 4 modified', $file4->Title);
457461

458462
// Test record-based permissions
459-
$response = $this->post(
460-
'UploadFieldTest_Controller/Form/field/ManyManyFiles/item/' . $fileNoEdit->ID . '/edit',
461-
array()
462-
);
463+
$response = $this->mockFileEditForm('ManyManyFiles', $fileNoEdit->ID);
464+
$this->assertEquals(403, $response->getStatusCode());
465+
466+
$response = $this->mockFileEdit('ManyManyFiles', $fileNoEdit->ID, array());
467+
$this->assertEquals(403, $response->getStatusCode());
468+
469+
// Test folder permissions
470+
$response = $this->mockFileEditForm('ManyManyFiles', $folder->ID);
471+
$this->assertEquals(403, $response->getStatusCode());
472+
473+
$response = $this->mockFileEdit('ManyManyFiles', $folder->ID, array());
463474
$this->assertEquals(403, $response->getStatusCode());
464475
}
465476

@@ -791,7 +802,35 @@ protected function mockFileExists($fileField, $fileName) {
791802
"UploadFieldTest_Controller/Form/field/{$fileField}/fileexists?filename=".urlencode($fileName)
792803
);
793804
}
794-
805+
806+
/**
807+
* Gets the edit form for the given file
808+
*
809+
* @param string $fileField Name of the field
810+
* @param integer $fileID ID of the file to delete
811+
* @return SS_HTTPResponse form response
812+
*/
813+
protected function mockFileEditForm($fileField, $fileID) {
814+
return $this->get(
815+
"UploadFieldTest_Controller/Form/field/{$fileField}/item/{$fileID}/edit"
816+
);
817+
}
818+
819+
/**
820+
* Mocks edit submissions to a file
821+
*
822+
* @param string $fileField Name of the field
823+
* @param integer $fileID ID of the file to delete
824+
* @param array $fields Fields to update
825+
* @return SS_HTTPResponse form response
826+
*/
827+
protected function mockFileEdit($fileField, $fileID, $fields = array()) {
828+
return $this->post(
829+
"UploadFieldTest_Controller/Form/field/{$fileField}/item/{$fileID}/EditForm",
830+
$fields
831+
);
832+
}
833+
795834
/**
796835
* Simulates a physical file deletion
797836
*
@@ -801,7 +840,7 @@ protected function mockFileExists($fileField, $fileName) {
801840
*/
802841
protected function mockFileDelete($fileField, $fileID) {
803842
return $this->post(
804-
"UploadFieldTest_Controller/Form/field/HasOneFile/item/{$fileID}/delete",
843+
"UploadFieldTest_Controller/Form/field/{$fileField}/item/{$fileID}/delete",
805844
array()
806845
);
807846
}

0 commit comments

Comments
 (0)