[BUGFIX] Properly check permissions with read only file mounts 94/34694/6
authorHelmut Hummel <helmut.hummel@typo3.org>
Thu, 27 Nov 2014 22:14:45 +0000 (23:14 +0100)
committerMarkus Klein <klein.t3@reelworx.at>
Wed, 3 Dec 2014 00:10:00 +0000 (01:10 +0100)
Previously the permission check returned false if the
file or folder was within a read only file mount, but
also within a writable file mount.

Properly check this now and centralize the check in
isWithinFileMountBoundaries, which now has an additional
argument whether write access to file mounts should be checked or not.

Resolves: #63364
Related: #49391
Releases: master, 6.2
Change-Id: If90958b7d6e4d5aad1fbd172af06462ca2b9764f
Reviewed-on: http://review.typo3.org/34694
Reviewed-by: Helmut Hummel <helmut.hummel@typo3.org>
Reviewed-by: Wouter Wolters <typo3@wouterwolters.nl>
Tested-by: Wouter Wolters <typo3@wouterwolters.nl>
Tested-by: Markus Klein <klein.t3@reelworx.at>
typo3/sysext/core/Classes/Resource/ResourceStorage.php
typo3/sysext/core/Tests/Unit/Resource/ResourceStorageTest.php

index bd20f94..b4d49d1 100644 (file)
@@ -435,35 +435,43 @@ class ResourceStorage implements ResourceStorageInterface {
 
        /**
         * Checks if the given subject is within one of the registered user
-        * filemounts. If not, working with the file is not permitted for the user.
+        * file mounts. If not, working with the file is not permitted for the user.
         *
-        * @param Folder $subject
+        * @param ResourceInterface $subject file or folder
+        * @param bool $checkWriteAccess If true, it is not only checked if the subject is within the file mount but also whether it isn't a read only file mount
         * @return bool
         */
-       public function isWithinFileMountBoundaries($subject) {
+       public function isWithinFileMountBoundaries($subject, $checkWriteAccess = FALSE) {
                if (!$this->evaluatePermissions) {
                        return TRUE;
                }
-               $isWithinFilemount = FALSE;
+               $isWithinFileMount = FALSE;
                if (!$subject) {
                        $subject = $this->getRootLevelFolder();
                }
                $identifier = $subject->getIdentifier();
 
                // Allow access to processing folder
-               if ($this->driver->isWithin($this->getProcessingFolder()->getIdentifier(), $identifier)) {
-                       $isWithinFilemount = TRUE;
+               if ($this->isWithinProcessingFolder($identifier)) {
+                       $isWithinFileMount = TRUE;
                } else {
                        // Check if the identifier of the subject is within at
                        // least one of the file mounts
+                       $writableFileMountAvailable = FALSE;
                        foreach ($this->fileMounts as $fileMount) {
                                if ($this->driver->isWithin($fileMount['folder']->getIdentifier(), $identifier)) {
-                                       $isWithinFilemount = TRUE;
-                                       break;
+                                       $isWithinFileMount = TRUE;
+                                       if (!$checkWriteAccess) {
+                                               break;
+                                       } elseif (empty($fileMount['read_only'])) {
+                                               $writableFileMountAvailable = TRUE;
+                                               break;
+                                       }
                                }
                        }
+                       $isWithinFileMount = $checkWriteAccess ? $writableFileMountAvailable : $isWithinFileMount;
                }
-               return $isWithinFilemount;
+               return $isWithinFileMount;
        }
 
        /**
@@ -540,11 +548,6 @@ class ResourceStorage implements ResourceStorageInterface {
                if (!$this->checkFileExtensionPermission($file->getName())) {
                        return FALSE;
                }
-               // Check 3: Does the user have the right to perform the action?
-               // (= is he within the file mount borders)
-               if (!$isProcessedFile && !$this->isWithinFileMountBoundaries($file)) {
-                       return FALSE;
-               }
                $isReadCheck = FALSE;
                if (in_array($action, array('read', 'copy', 'move'), TRUE)) {
                        $isReadCheck = TRUE;
@@ -553,17 +556,10 @@ class ResourceStorage implements ResourceStorageInterface {
                if (in_array($action, array('add', 'write', 'move', 'rename', 'unzip', 'delete'), TRUE)) {
                        $isWriteCheck = TRUE;
                }
-
-               // Check 4: "Read-only" filemount
-               if ($isWriteCheck) {
-                       foreach ($this->fileMounts as $fileMount) {
-                               if ($this->driver->isWithin($fileMount['folder']->getIdentifier(), $file->getIdentifier())) {
-                                       if (!empty($fileMount['read_only'])) {
-                                               return FALSE;
-                                       }
-                                       break;
-                               }
-                       }
+               // Check 3: 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;
                }
 
                $isMissing = FALSE;
@@ -571,11 +567,11 @@ class ResourceStorage implements ResourceStorageInterface {
                        $isMissing = $file->isMissing();
                }
 
-               // Check 5: Check the capabilities of the storage (and the driver)
+               // Check 4: Check the capabilities of the storage (and the driver)
                if ($isWriteCheck && ($isMissing || !$this->isWritable())) {
                        return FALSE;
                }
-               // Check 6: "File permissions" of the driver (only when file isn't marked as missing)
+               // Check 5: "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']) {
@@ -609,11 +605,6 @@ class ResourceStorage implements ResourceStorageInterface {
                        return TRUE;
                }
 
-               // Check 2: Does the user has the right to perform the action?
-               // (= is he within the file mount borders)
-               if (!$this->isWithinFileMountBoundaries($folder)) {
-                       return FALSE;
-               }
                $isReadCheck = FALSE;
                if (in_array($action, array('read', 'copy'), TRUE)) {
                        $isReadCheck = TRUE;
@@ -622,6 +613,11 @@ class ResourceStorage implements ResourceStorageInterface {
                if (in_array($action, array('add', 'move', 'write', 'delete', 'rename'), TRUE)) {
                        $isWriteCheck = TRUE;
                }
+               // Check 2: Does the user has the right to perform the action?
+               // (= is he within the file mount borders)
+               if (!$this->isWithinFileMountBoundaries($folder, $isWriteCheck)) {
+                       return FALSE;
+               }
                // Check 3: Check the capabilities of the storage (and the driver)
                if ($isReadCheck && !$this->isBrowsable()) {
                        return FALSE;
@@ -630,19 +626,7 @@ class ResourceStorage implements ResourceStorageInterface {
                        return FALSE;
                }
 
-               // Check 4: "Read-only" filemount
-               if ($isWriteCheck) {
-                       foreach ($this->fileMounts as $fileMount) {
-                               if ($this->driver->isWithin($fileMount['folder']->getIdentifier(), $folder->getIdentifier())) {
-                                       if (!empty($fileMount['read_only'])) {
-                                               return FALSE;
-                                       }
-                                       break;
-                               }
-                       }
-               }
-
-               // Check 5: "Folder permissions" of the driver
+               // Check 4: "Folder permissions" of the driver
                $folderPermissions = $this->driver->getPermissions($folder->getIdentifier());
                if ($isReadCheck && !$folderPermissions['r']) {
                        return FALSE;
index 4b0416d..e4c4c73 100644 (file)
@@ -141,6 +141,95 @@ class ResourceStorageTest extends BaseTestCase {
        /**
         * @return array
         */
+       public function isWithinFileMountBoundariesDataProvider() {
+               return array(
+                       'Access to file in ro file mount denied for write request' => array(
+                               '$fileIdentifier' => '/fooBaz/bar.txt',
+                               '$fileMountFolderIdentifier' => '/fooBaz/',
+                               '$isFileMountReadOnly' => TRUE,
+                               '$checkWriteAccess' => TRUE,
+                               '$expectedResult' => FALSE,
+                       ),
+                       'Access to file in ro file mount allowed for read request' => array(
+                               '$fileIdentifier' => '/fooBaz/bar.txt',
+                               '$fileMountFolderIdentifier' => '/fooBaz/',
+                               '$isFileMountReadOnly' => TRUE,
+                               '$checkWriteAccess' => FALSE,
+                               '$expectedResult' => TRUE,
+                       ),
+                       'Access to file in rw file mount allowed for write request' => array(
+                               '$fileIdentifier' => '/fooBaz/bar.txt',
+                               '$fileMountFolderIdentifier' => '/fooBaz/',
+                               '$isFileMountReadOnly' => FALSE,
+                               '$checkWriteAccess' => TRUE,
+                               '$expectedResult' => TRUE,
+                       ),
+                       'Access to file in rw file mount allowed for read request' => array(
+                               '$fileIdentifier' => '/fooBaz/bar.txt',
+                               '$fileMountFolderIdentifier' => '/fooBaz/',
+                               '$isFileMountReadOnly' => FALSE,
+                               '$checkWriteAccess' => FALSE,
+                               '$expectedResult' => TRUE,
+                       ),
+                       'Access to file not in file mount denied for write request' => array(
+                               '$fileIdentifier' => '/fooBaz/bar.txt',
+                               '$fileMountFolderIdentifier' => '/barBaz/',
+                               '$isFileMountReadOnly' => FALSE,
+                               '$checkWriteAccess' => TRUE,
+                               '$expectedResult' => FALSE,
+                       ),
+                       'Access to file not in file mount denied for read request' => array(
+                               '$fileIdentifier' => '/fooBaz/bar.txt',
+                               '$fileMountFolderIdentifier' => '/barBaz/',
+                               '$isFileMountReadOnly' => FALSE,
+                               '$checkWriteAccess' => FALSE,
+                               '$expectedResult' => FALSE,
+                       ),
+               );
+       }
+
+       /**
+        * @param string $fileIdentifier
+        * @param string $fileMountFolderIdentifier
+        * @param bool $isFileMountReadOnly
+        * @param bool $checkWriteAccess
+        * @param bool $expectedResult
+        * @throws \TYPO3\CMS\Core\Resource\Exception\FolderDoesNotExistException
+        * @test
+        * @dataProvider isWithinFileMountBoundariesDataProvider
+        */
+       public function isWithinFileMountBoundariesRespectsReadOnlyFileMounts($fileIdentifier, $fileMountFolderIdentifier, $isFileMountReadOnly, $checkWriteAccess, $expectedResult) {
+               /** @var AbstractDriver|\PHPUnit_Framework_MockObject_MockObject $driverMock */
+               $driverMock = $this->getMockForAbstractClass(AbstractDriver::class, array(), '', FALSE);
+               $driverMock->expects($this->any())
+                       ->method('getFolderInfoByIdentifier')
+                       ->willReturnCallback(function($identifier) use ($isFileMountReadOnly) {
+                               return array(
+                                       'identifier' => $identifier,
+                                       'name' => trim($identifier, '/'),
+                               );
+                       });
+               $driverMock->expects($this->any())
+                       ->method('isWithin')
+                       ->willReturnCallback(function($folderIdentifier, $fileIdentifier)  {
+                               if ($fileIdentifier === ResourceStorageInterface::DEFAULT_ProcessingFolder . '/') {
+                                       return FALSE;
+                               } else {
+                                       return strpos($fileIdentifier, $folderIdentifier) === 0;
+                               }
+                       });
+               $this->prepareSubject(array(), FALSE, $driverMock);
+               $fileMock = $this->getSimpleFileMock($fileIdentifier);
+               $this->subject->setEvaluatePermissions(TRUE);
+               $this->subject->addFileMount('/' . uniqid('random') . '/', array('read_only' => FALSE));
+               $this->subject->addFileMount($fileMountFolderIdentifier, array('read_only' => $isFileMountReadOnly));
+               $this->subject->addFileMount('/' . uniqid('random') . '/', array('read_only' => FALSE));
+               $this->assertSame($expectedResult, $this->subject->isWithinFileMountBoundaries($fileMock, $checkWriteAccess));
+       }
+
+       /**
+        * @return array
+        */
        public function capabilitiesDataProvider() {
                return array(
                        'only public' => array(
@@ -627,4 +716,4 @@ class ResourceStorageTest extends BaseTestCase {
 
                $this->assertSame(FolderInterface::ROLE_DEFAULT, $role);
        }
-}
\ No newline at end of file
+}