[BUGFIX] Make meta data editable for non-writable storages

Decouple check for writable files/storage from permission
to edit meta data. Permission to edit meta data is now
only denied when users have only access to the file
via a readonly file mount.

Resolves: #65636
Resolves: #66507
Releases: master, 8.7
Change-Id: I25a0fbc9cf761898dbdb95dec1d3d39bb2f4b7fd
Reviewed-on: https://review.typo3.org/42874
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>
Reviewed-by: Joerg Kummer <typo3@enobe.de>
Reviewed-by: Josef Glatz <josef.glatz@typo3.org>
Reviewed-by: Nicole Cordes <typo3@cordes.co>
Tested-by: Nicole Cordes <typo3@cordes.co>
Reviewed-by: Benni Mack <benni@typo3.org>
Tested-by: Benni Mack <benni@typo3.org>

diff --git a/typo3/sysext/core/Classes/Resource/ResourceStorage.php b/typo3/sysext/core/Classes/Resource/ResourceStorage.php
index f310660..d9d6649 100644 (file)
--- a/typo3/sysext/core/Classes/Resource/ResourceStorage.php
+++ b/typo3/sysext/core/Classes/Resource/ResourceStorage.php
@@ -604,18 +604,22 @@ class ResourceStorage implements ResourceStorageInterface
      * the Filelist UI to check whether an action is allowed and whether action
      * related UI elements should thus be shown (move icon, edit icon, etc.)
      *
-     * @param string $action action, can be read, write, delete
+     * @param string $action action, can be read, write, delete, editMeta
      * @param FileInterface $file
      * @return bool
      */
     public function checkFileActionPermission($action, FileInterface $file)
     {
         $isProcessedFile = $file instanceof ProcessedFile;
-        // Check 1: Does the user have permission to perform the action? e.g. "readFile"
+        // Check 1: Allow editing meta data of a file if it is in mount boundaries of a writable file mount
+        if ($action === 'editMeta') {
+            return !$isProcessedFile && $this->isWithinFileMountBoundaries($file, true);
+        }
+        // Check 2: Does the user have permission to perform the action? e.g. "readFile"
         if (!$isProcessedFile && $this->checkUserActionPermission($action, 'File') === false) {
             return false;
         }
-        // Check 2: No action allowed on files for denied file extensions
+        // Check 3: No action allowed on files for denied file extensions
         if (!$this->checkFileExtensionPermission($file->getName())) {
             return false;
         }
@@ -627,7 +631,7 @@ class ResourceStorage implements ResourceStorageInterface
         if (in_array($action, ['add', 'write', 'move', 'rename', 'replace', 'delete'], true)) {
             $isWriteCheck = true;
         }
-        // Check 3: Does the user have the right to perform the action?
+        // Check 4: Does the user have the right to perform the action?
         // (= is he within the file mount borders)
         if (!$isProcessedFile && !$this->isWithinFileMountBoundaries($file, $isWriteCheck)) {
             return false;
@@ -643,12 +647,12 @@ class ResourceStorage implements ResourceStorageInterface
             $isMissing = true;
         }
 
-        // Check 4: Check the capabilities of the storage (and the driver)
+        // Check 5: Check the capabilities of the storage (and the driver)
         if ($isWriteCheck && ($isMissing || !$this->isWritable())) {
             return false;
         }
 
-        // Check 5: "File permissions" of the driver (only when file isn't marked as missing)
+        // Check 6: "File permissions" of the driver (only when file isn't marked as missing)
         if (!$isMissing) {
             $filePermissions = $this->driver->getPermissions($file->getIdentifier());
             if ($isReadCheck && !$filePermissions['r']) {

diff --git a/typo3/sysext/core/Classes/Resource/Security/FileMetadataPermissionsAspect.php b/typo3/sysext/core/Classes/Resource/Security/FileMetadataPermissionsAspect.php
index 22102f4..d8308cf 100644 (file)
--- a/typo3/sysext/core/Classes/Resource/Security/FileMetadataPermissionsAspect.php
+++ b/typo3/sysext/core/Classes/Resource/Security/FileMetadataPermissionsAspect.php
@@ -163,7 +163,7 @@ class FileMetadataPermissionsAspect implements DataHandlerCheckModifyAccessListH
                 $file = substr($file, strlen('sys_file_'));
             }
             $fileObject = ResourceFactory::getInstance()->getFileObject((int)$file);
-            $accessAllowed = $fileObject->checkActionPermission('write');
+            $accessAllowed = $fileObject->checkActionPermission('editMeta');
         }
         return $accessAllowed;
     }

diff --git a/typo3/sysext/core/Documentation/Changelog/9.5.x/Important-65636-AllowMetaDataEditingOnReadOnlyStorages.rst b/typo3/sysext/core/Documentation/Changelog/9.5.x/Important-65636-AllowMetaDataEditingOnReadOnlyStorages.rst
new file mode 100644 (file)
index 0000000..f45221e
--- /dev/null
+++ b/typo3/sysext/core/Documentation/Changelog/9.5.x/Important-65636-AllowMetaDataEditingOnReadOnlyStorages.rst
@@ -0,0 +1,28 @@
+.. include:: ../../Includes.txt
+
+==========================================================================
+Important: #65636 - File meta data can now be edited on read only storages
+==========================================================================
+
+See :issue:`65636`
+
+Description
+===========
+
+Whether meta data editing of files is allowed or not, must not be bound to whether a file is
+physically writable in a storage, or whether the storage itself is set read only.
+
+Editing meta data should on the other hand be forbidden, when the file is within a read only
+file mount.
+
+Allowing meta data editing on read only storage
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+To allow to distinguish between read access to a file and write access to file meta data,
+a new file action permission `editMeta` is introduced, which is automatically checked and
+enforced when saving a meta data record.
+
+When having to check for editing meta data permission in userland code, it is recommended
+to use the new file action permission instead of the previously used permission `read`.
+
+.. index:: ext:core, NotScanned

diff --git a/typo3/sysext/core/Tests/Unit/Resource/ResourceStorageTest.php b/typo3/sysext/core/Tests/Unit/Resource/ResourceStorageTest.php
index 4a6a30e..c1ebb56 100644 (file)
--- a/typo3/sysext/core/Tests/Unit/Resource/ResourceStorageTest.php
+++ b/typo3/sysext/core/Tests/Unit/Resource/ResourceStorageTest.php
@@ -444,6 +444,71 @@ class ResourceStorageTest extends BaseTestCase
         $this->assertFalse($this->subject->checkUserActionPermission('write', 'folder'));
     }
 
+    /**
+     * @test
+     */
+    public function metaDataEditIsNotAllowedWhenWhenNoFileMountsAreSet(): void
+    {
+        $this->prepareSubject([], false, null, [], ['isWithinProcessingFolder']);
+        $this->subject->setEvaluatePermissions(true);
+        $this->assertFalse($this->subject->checkFileActionPermission('editMeta', new File(['identifier' => '/foo/bar.jpg'], $this->subject)));
+    }
+
+    /**
+     * @test
+     */
+    public function metaDataEditIsAllowedWhenWhenInFileMount(): void
+    {
+        $driverMock = $this->getMockForAbstractClass(AbstractDriver::class, [], '', false);
+        $this->prepareSubject([], false, $driverMock, [], ['isWithinProcessingFolder']);
+
+        $fileStub = new File(['identifier' => '/foo/bar.jpg'], $this->subject);
+        $folderStub = new Folder($this->subject, '/foo/', 'foo');
+        $driverMock->expects($this->once())
+            ->method('isWithin')
+            ->with($folderStub->getIdentifier(), $fileStub->getIdentifier())
+            ->willReturn(true);
+
+        $this->subject->setEvaluatePermissions(true);
+        $fileMounts = [
+            '/foo/' => [
+                'path' => '/foo/',
+                'title' => 'Foo',
+                'folder' => $folderStub,
+            ]
+        ];
+        $this->inject($this->subject, 'fileMounts', $fileMounts);
+        $this->assertTrue($this->subject->checkFileActionPermission('editMeta', $fileStub));
+    }
+
+    /**
+     * @test
+     */
+    public function metaDataEditIsNotAllowedWhenWhenInReadOnlyFileMount(): void
+    {
+        $driverMock = $this->getMockForAbstractClass(AbstractDriver::class, [], '', false);
+        $this->prepareSubject([], false, $driverMock, [], ['isWithinProcessingFolder']);
+
+        $fileStub = new File(['identifier' => '/foo/bar.jpg'], $this->subject);
+        $folderStub = new Folder($this->subject, '/foo/', 'foo');
+        $driverMock->expects($this->once())
+            ->method('isWithin')
+            ->with($folderStub->getIdentifier(), $fileStub->getIdentifier())
+            ->willReturn(true);
+
+        $this->subject->setEvaluatePermissions(true);
+        $fileMounts = [
+            '/foo/' => [
+                'path' => '/foo/',
+                'title' => 'Foo',
+                'folder' => $folderStub,
+                'read_only' => true,
+            ]
+        ];
+        $this->inject($this->subject, 'fileMounts', $fileMounts);
+        $this->assertFalse($this->subject->checkFileActionPermission('editMeta', $fileStub));
+    }
+
     /**
      * @test
      */

diff --git a/typo3/sysext/filelist/Classes/FileList.php b/typo3/sysext/filelist/Classes/FileList.php
index e099432..5ab126a 100644 (file)
--- a/typo3/sysext/filelist/Classes/FileList.php
+++ b/typo3/sysext/filelist/Classes/FileList.php
@@ -966,7 +966,7 @@ class FileList
     public function linkWrapFile($code, File $fileObject)
     {
         try {
-            if ($fileObject instanceof File && $fileObject->isIndexed() && $fileObject->checkActionPermission('write') && $this->getBackendUser()->check('tables_modify', 'sys_file_metadata')) {
+            if ($fileObject instanceof File && $fileObject->isIndexed() && $fileObject->checkActionPermission('editMeta') && $this->getBackendUser()->check('tables_modify', 'sys_file_metadata')) {
                 $metaData = $fileObject->_getMetaData();
                 $urlParameters = [
                     'edit' => [
@@ -1060,7 +1060,7 @@ class FileList
                         $theData[$field] = $this->makeClip($fileObject);
                         break;
                     case '_LOCALIZATION_':
-                        if (!empty($systemLanguages) && $fileObject->isIndexed() && $fileObject->checkActionPermission('write') && $this->getBackendUser()->check('tables_modify', 'sys_file_metadata')) {
+                        if (!empty($systemLanguages) && $fileObject->isIndexed() && $fileObject->checkActionPermission('editMeta') && $this->getBackendUser()->check('tables_modify', 'sys_file_metadata')) {
                             $metaDataRecord = $fileObject->_getMetaData();
                             $translations = $this->getTranslationsForMetaData($metaDataRecord);
                             $languageCode = '';
@@ -1333,7 +1333,7 @@ class FileList
         }
 
         // Edit metadata of file
-        if ($fileOrFolderObject instanceof File && $fileOrFolderObject->checkActionPermission('write') && $this->getBackendUser()->check('tables_modify', 'sys_file_metadata')) {
+        if ($fileOrFolderObject instanceof File && $fileOrFolderObject->checkActionPermission('editMeta') && $this->getBackendUser()->check('tables_modify', 'sys_file_metadata')) {
             $metaData = $fileOrFolderObject->_getMetaData();
             $urlParameters = [
                 'edit' => [