Merge pull request #1167 from creative-commoners/pulls/5/access-submitted-file

ENH Protect access to the uploaded file without permission

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(
+            if ($file->canView()) {
-                '%s - <a href="%s" target="_blank">%s</a>',
+                return DBField::create_field('HTMLText', sprintf(
-                $name,
+                    '%s - <a href="%s" target="_blank">%s</a>',
-                $link,
+                    htmlspecialchars($name, ENT_QUOTES),
-                $title
+                    htmlspecialchars($link, ENT_QUOTES),
-            ));
+                    htmlspecialchars($title, ENT_QUOTES)
+                ));
+            } else {
+                return DBField::create_field('HTMLText', sprintf(
+                    '<i class="icon font-icon-lock"></i> %s - <em>%s</em>',
+                    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);
             }
         }
     }

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'

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();
+        $this->assertStringContainsString('test-SubmittedFileFieldTest', $this->submittedFile->getFileName(), 'Submitted file is linked');
-        $file->setFromString('ABC', 'test-SubmittedFileFieldTest.txt');
-        $file->write();
 
-        $submittedForm = SubmittedForm::create();
+        $this->submittedForm->delete();
-        $submittedForm->write();
+        $fileId = $this->file->ID;
-
-        $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;
 
         $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 - <a href="/assets/3c01bdbb26/test-SubmittedFileFieldTest.txt" target="_blank">Download File</a>',
+                $fileName
+            ),
+            $this->submittedFile->getFormattedValue()->value
+        );
+            
+        $this->loginWithPermission('CMS_ACCESS_CMSMain');
+        $this->assertEquals(
+            sprintf(
+                '<i class="icon font-icon-lock"></i> %s - <em>%s</em>',
+                $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'
+        );
+    }
 }