From 2750bc3a071faddfd1a902d5b70974f0c6c0a555 Mon Sep 17 00:00:00 2001 From: Sabina Talipova Date: Thu, 11 Aug 2022 15:35:39 +1200 Subject: [PATCH] ENH Protect access to the uploaded file without permission --- code/Model/Submission/SubmittedFileField.php | 28 +++++--- lang/en.yml | 1 + tests/php/Model/SubmittedFileFieldTest.php | 71 +++++++++++++++----- 3 files changed, 75 insertions(+), 25 deletions(-) diff --git a/code/Model/Submission/SubmittedFileField.php b/code/Model/Submission/SubmittedFileField.php index 1d8e078..94829d9 100755 --- a/code/Model/Submission/SubmittedFileField.php +++ b/code/Model/Submission/SubmittedFileField.php @@ -39,16 +39,26 @@ class SubmittedFileField extends SubmittedFormField public function getFormattedValue() { $name = $this->getFileName(); - $link = $this->getLink(); + $link = $this->getLink(false); $title = _t(__CLASS__ . '.DOWNLOADFILE', 'Download File'); + $message = _t(__CLASS__ . '.INSUFFICIENTRIGHTS', 'You don\'t have the right permissions to download this file'); + $file = $this->getUploadedFileFromDraft(); if ($link) { - return DBField::create_field('HTMLText', sprintf( - '%s - %s', - $name, - $link, - $title - )); + if ($file->canView()) { + return DBField::create_field('HTMLText', sprintf( + '%s - %s', + htmlspecialchars($name, ENT_QUOTES), + htmlspecialchars($link, ENT_QUOTES), + htmlspecialchars($title, ENT_QUOTES) + )); + } else { + return DBField::create_field('HTMLText', sprintf( + ' %s - %s', + htmlspecialchars($name, ENT_QUOTES), + htmlspecialchars($message, ENT_QUOTES) + )); + } } return false; @@ -69,11 +79,11 @@ class SubmittedFileField extends SubmittedFormField * * @return string */ - public function getLink() + public function getLink($grant = true) { if ($file = $this->getUploadedFileFromDraft()) { if ($file->exists()) { - return $file->getAbsoluteURL(); + return $file->getURL($grant); } } } diff --git a/lang/en.yml b/lang/en.yml index 6cb479b..52ea84b 100644 --- a/lang/en.yml +++ b/lang/en.yml @@ -246,6 +246,7 @@ en: SINGULARNAME: 'Email Recipient Condition' SilverStripe\UserForms\Model\Submission\SubmittedFileField: DOWNLOADFILE: 'Download File' + INSUFFICIENTRIGHTS: 'You don''t have the right permissions to download this file' PLURALNAME: 'Submitted File Fields' PLURALS: one: 'A Submitted File Field' diff --git a/tests/php/Model/SubmittedFileFieldTest.php b/tests/php/Model/SubmittedFileFieldTest.php index 397dcc0..9db30dc 100644 --- a/tests/php/Model/SubmittedFileFieldTest.php +++ b/tests/php/Model/SubmittedFileFieldTest.php @@ -4,6 +4,8 @@ namespace SilverStripe\UserForms\Tests\Model; use SilverStripe\Assets\Dev\TestAssetStore; use SilverStripe\Assets\File; +use SilverStripe\Assets\Storage\AssetStore; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\SapphireTest; use SilverStripe\UserForms\Model\Submission\SubmittedFileField; use SilverStripe\UserForms\Model\Submission\SubmittedForm; @@ -11,11 +13,27 @@ use SilverStripe\Versioned\Versioned; class SubmittedFileFieldTest extends SapphireTest { + protected $file; + protected $submittedForm; + protected function setUp(): void { parent::setUp(); TestAssetStore::activate('SubmittedFileFieldTest'); + + $this->file = File::create(); + $this->file->setFromString('ABC', 'test-SubmittedFileFieldTest.txt'); + $this->file->write(); + + $this->submittedForm = SubmittedForm::create(); + $this->submittedForm->write(); + + $this->submittedFile = SubmittedFileField::create(); + $this->submittedFile->UploadedFileID = $this->file->ID; + $this->submittedFile->Name = 'File'; + $this->submittedFile->ParentID = $this->submittedForm->ID; + $this->submittedFile->write(); } protected function tearDown(): void @@ -27,23 +45,10 @@ class SubmittedFileFieldTest extends SapphireTest public function testDeletingSubmissionRemovesFile() { - $file = File::create(); - $file->setFromString('ABC', 'test-SubmittedFileFieldTest.txt'); - $file->write(); + $this->assertStringContainsString('test-SubmittedFileFieldTest', $this->submittedFile->getFileName(), 'Submitted file is linked'); - $submittedForm = SubmittedForm::create(); - $submittedForm->write(); - - $submittedFile = SubmittedFileField::create(); - $submittedFile->UploadedFileID = $file->ID; - $submittedFile->Name = 'File'; - $submittedFile->ParentID = $submittedForm->ID; - $submittedFile->write(); - - $this->assertStringContainsString('test-SubmittedFileFieldTest', $submittedFile->getFileName(), 'Submitted file is linked'); - - $submittedForm->delete(); - $fileId = $file->ID; + $this->submittedForm->delete(); + $fileId = $this->file->ID; $draftVersion = Versioned::withVersionedMode(function () use ($fileId) { Versioned::set_stage(Versioned::DRAFT); @@ -61,4 +66,38 @@ class SubmittedFileFieldTest extends SapphireTest $this->assertNull($liveVersion, 'Live file has been deleted'); } + + public function testGetFormattedValue() + { + $fileName = $this->submittedFile->getFileName(); + $message = "You don't have the right permissions to download this file"; + + $this->file->CanViewType = 'OnlyTheseUsers'; + $this->file->write(); + + $this->loginWithPermission('ADMIN'); + $this->assertEquals( + sprintf( + '%s - Download File', + $fileName + ), + $this->submittedFile->getFormattedValue()->value + ); + + $this->loginWithPermission('CMS_ACCESS_CMSMain'); + $this->assertEquals( + sprintf( + ' %s - %s', + $fileName, + $message + ), + $this->submittedFile->getFormattedValue()->value + ); + + $store = Injector::inst()->get(AssetStore::class); + $this->assertFalse( + $store->canView($fileName, $this->file->getHash()), + 'Users without canView rights on the file should not have been session granted access to it' + ); + } }