diff --git a/apps/files_sharing/lib/Cache.php b/apps/files_sharing/lib/Cache.php index 08a48c398f07f..23fad70b0909a 100644 --- a/apps/files_sharing/lib/Cache.php +++ b/apps/files_sharing/lib/Cache.php @@ -15,6 +15,7 @@ use OC\Files\Search\SearchComparison; use OC\Files\Storage\Wrapper\Jail; use OC\User\DisplayNameCache; +use OCP\Constants; use OCP\Files\Cache\ICache; use OCP\Files\Cache\ICacheEntry; use OCP\Files\Search\ISearchBinaryOperator; @@ -154,6 +155,14 @@ protected function formatCacheEntry($entry, $path = null) { $entry['permissions'] = $this->storage->getPermissions($entry['path']); } + if (!SharedStorage::hasValidSourcePermissions($this->sourceRootInfo)) { + // SharedStorage disables a share whose source lost the share permission: + // every write is refused while reading still works. Report the same here, + // otherwise the file list advertises write permissions that every + // operation then denies with a 403. + $entry['permissions'] &= Constants::PERMISSION_READ; + } + if ($this->share->getNodeId() === $entry['fileid']) { $entry['name'] = basename($this->share->getTarget()); } diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index 7a2909c3ad839..608eac24af739 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/apps/files_sharing/lib/SharedStorage.php @@ -63,6 +63,8 @@ class SharedStorage extends Jail implements LegacyISharedStorage, ISharedStorage private $initialized = false; + private bool $invalidSourceLogged = false; + /** * @var ICacheEntry */ @@ -236,8 +238,58 @@ public function getShareId() { return $this->superShare->getId(); } + /** + * A share only works as long as its source still carries the share permission. + * Both this storage and its cache have to agree on that, so keep the check in + * one place. + * + * @param ICacheEntry|false|null $sourceRootInfo + */ + public static function hasValidSourcePermissions($sourceRootInfo): bool { + return $sourceRootInfo instanceof ICacheEntry + && ($sourceRootInfo->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE; + } + private function isValid(): bool { - return $this->getSourceRootInfo() && ($this->getSourceRootInfo()->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE; + $sourceRootInfo = $this->getSourceRootInfo(); + if (self::hasValidSourcePermissions($sourceRootInfo)) { + return true; + } + + $this->logInvalidSource($sourceRootInfo); + + return false; + } + + /** + * An invalid share is disabled entirely: every permission check returns 0 while + * reading keeps working, which is easy to mistake for arbitrary breakage such as + * failing uploads or renames that revert. Leave a trace naming the share and the + * source that has to be repaired. + * + * @param ICacheEntry|false|null $sourceRootInfo + */ + private function logInvalidSource($sourceRootInfo): void { + if ($this->invalidSourceLogged) { + return; + } + $this->invalidSourceLogged = true; + + if (!($sourceRootInfo instanceof ICacheEntry) || $sourceRootInfo->getId() < 0) { + // The source could not be resolved at all, in which case `init()` has + // already swapped in a `FailedStorage` and `FailedCache` and the root info + // is only a read-only placeholder. That is a different problem from a + // source whose cached permissions are wrong, so don't report it as one. + return; + } + + $this->logger->warning('Share {shareId} is disabled because its source is missing the share permission. Repair the cached permissions of {shareOwner} with "occ files:scan", or check whether the source is masked by a read-only mount or access control rule.', [ + 'app' => 'files_sharing', + 'shareId' => $this->superShare->getId(), + 'shareOwner' => $this->superShare->getShareOwner(), + 'sourceFileId' => $sourceRootInfo->getId(), + 'sourcePermissions' => $sourceRootInfo->getPermissions(), + ]); } #[\Override] diff --git a/apps/files_sharing/tests/SharedStorageTest.php b/apps/files_sharing/tests/SharedStorageTest.php index 26e1e0a5e81c0..39ee3c1069c73 100644 --- a/apps/files_sharing/tests/SharedStorageTest.php +++ b/apps/files_sharing/tests/SharedStorageTest.php @@ -443,6 +443,53 @@ public function testOwnerPermissions(): void { $this->shareManager->deleteShare($share); } + public function testInvalidSourcePermissionsAreReportedAsReadOnly(): void { + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + + $share = $this->share( + IShare::TYPE_USER, + $this->folder, + self::TEST_FILES_SHARING_API_USER1, + self::TEST_FILES_SHARING_API_USER2, + Constants::PERMISSION_ALL + ); + + // Drop the share permission of the source in the file cache, the way a scan + // through a permission mask used to persist masked permissions. This disables + // the share in `SharedStorage::isValid()`. + $sourceInfo = $this->view->getFileInfo($this->folder); + $sourceCache = $sourceInfo->getStorage()->getCache(); + $sourceCache->update($sourceInfo->getId(), [ + 'permissions' => Constants::PERMISSION_ALL & ~Constants::PERMISSION_SHARE, + ]); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + $user2View = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + + // the storage refuses every write, so the cache must not advertise more. + // The mount point keeps its delete permission: `View::getFileInfo()` adds that + // for every movable mount, so the recipient can always remove the share from + // their own view. + $folderInfo = $user2View->getFileInfo($this->folder); + $this->assertNotFalse($folderInfo); + $this->assertTrue($folderInfo->isReadable()); + $this->assertFalse($folderInfo->isCreatable()); + $this->assertFalse($folderInfo->isUpdateable()); + $this->assertFalse($folderInfo->isShareable()); + + $fileInfo = $user2View->getFileInfo($this->folder . $this->filename); + $this->assertNotFalse($fileInfo); + $this->assertSame(Constants::PERMISSION_READ, $fileInfo->getPermissions()); + + // and what it does advertise still holds + $this->assertSame('file in subfolder', $user2View->file_get_contents($this->folder . $this->filename)); + $this->assertFalse($user2View->fopen($this->folder . '/blocked.txt', 'w')); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + $sourceCache->update($sourceInfo->getId(), ['permissions' => Constants::PERMISSION_ALL]); + $this->shareManager->deleteShare($share); + } + public function testInitWithNonExistingUser(): void { $share = $this->createMock(IShare::class); $share->method('getShareOwner')->willReturn('unexist');