From 8ca6fcace7dc22230a7afe84936ca01470f50d00 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 16 Sep 2024 16:00:30 +0200 Subject: [PATCH 1/2] fix(Storage): Document getOwner() can return false Signed-off-by: provokateurin --- apps/dav/lib/Storage/PublicOwnerWrapper.php | 2 +- apps/files_sharing/lib/External/Storage.php | 2 +- apps/files_sharing/lib/SharedStorage.php | 2 +- lib/private/Files/ObjectStore/HomeObjectStoreStorage.php | 8 +------- lib/private/Files/Storage/Common.php | 8 +------- lib/private/Files/Storage/Home.php | 8 +------- lib/private/Files/Storage/Wrapper/Availability.php | 4 ++-- lib/private/Files/Storage/Wrapper/Jail.php | 8 +------- lib/private/Files/Storage/Wrapper/Wrapper.php | 9 +-------- lib/private/Files/View.php | 4 +--- lib/private/Lockdown/Filesystem/NullStorage.php | 4 ++-- lib/public/Files/Storage/IStorage.php | 2 +- tests/lib/Lockdown/Filesystem/NullStorageTest.php | 2 +- 13 files changed, 15 insertions(+), 48 deletions(-) diff --git a/apps/dav/lib/Storage/PublicOwnerWrapper.php b/apps/dav/lib/Storage/PublicOwnerWrapper.php index 6523c4c7cad9c..40ef81ab06d54 100644 --- a/apps/dav/lib/Storage/PublicOwnerWrapper.php +++ b/apps/dav/lib/Storage/PublicOwnerWrapper.php @@ -26,7 +26,7 @@ public function __construct($arguments) { $this->owner = $arguments['owner']; } - public function getOwner($path) { + public function getOwner($path): string|false { $owner = parent::getOwner($path); if ($owner === null || $owner === false) { diff --git a/apps/files_sharing/lib/External/Storage.php b/apps/files_sharing/lib/External/Storage.php index 169054fd1d964..9172ff932027a 100644 --- a/apps/files_sharing/lib/External/Storage.php +++ b/apps/files_sharing/lib/External/Storage.php @@ -345,7 +345,7 @@ public function getShareInfo(int $depth = -1) { return json_decode($response->getBody(), true); } - public function getOwner($path) { + public function getOwner($path): string|false { return $this->cloudId->getDisplayId(); } diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index 62f5880294cf1..cecc1d5cb985a 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/apps/files_sharing/lib/SharedStorage.php @@ -446,7 +446,7 @@ public function getScanner($path = '', $storage = null) { return new \OCA\Files_Sharing\Scanner($storage); } - public function getOwner($path): string { + public function getOwner($path): string|false { return $this->superShare->getShareOwner(); } diff --git a/lib/private/Files/ObjectStore/HomeObjectStoreStorage.php b/lib/private/Files/ObjectStore/HomeObjectStoreStorage.php index b543d223f4c26..feca1f6b4f544 100644 --- a/lib/private/Files/ObjectStore/HomeObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/HomeObjectStoreStorage.php @@ -32,13 +32,7 @@ public function getId(): string { return 'object::user:' . $this->user->getUID(); } - /** - * get the owner of a path - * - * @param string $path The path to get the owner - * @return string uid - */ - public function getOwner($path): string { + public function getOwner($path): string|false { return $this->user->getUID(); } diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index eb93ab89bc7e8..181da79cebec5 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -390,13 +390,7 @@ public function getStorageCache($storage = null) { return $this->getCache($storage)->getStorageCache(); } - /** - * get the owner of a path - * - * @param string $path The path to get the owner - * @return string|false uid or false - */ - public function getOwner($path) { + public function getOwner($path): string|false { if ($this->owner === null) { $this->owner = \OC_User::getUser(); } diff --git a/lib/private/Files/Storage/Home.php b/lib/private/Files/Storage/Home.php index 0e53e7b28d431..f7151f4a0058b 100644 --- a/lib/private/Files/Storage/Home.php +++ b/lib/private/Files/Storage/Home.php @@ -83,13 +83,7 @@ public function getUser(): IUser { return $this->user; } - /** - * get the owner of a path - * - * @param string $path The path to get the owner - * @return string uid or false - */ - public function getOwner($path) { + public function getOwner($path): string|false { return $this->user->getUID(); } } diff --git a/lib/private/Files/Storage/Wrapper/Availability.php b/lib/private/Files/Storage/Wrapper/Availability.php index 6bd622e1c1bb1..0f1abada168ad 100644 --- a/lib/private/Files/Storage/Wrapper/Availability.php +++ b/lib/private/Files/Storage/Wrapper/Availability.php @@ -373,12 +373,12 @@ public function hasUpdated($path, $time) { } } - /** {@inheritdoc} */ - public function getOwner($path) { + public function getOwner($path): string|false { try { return parent::getOwner($path); } catch (StorageNotAvailableException $e) { $this->setUnavailable($e); + return false; } } diff --git a/lib/private/Files/Storage/Wrapper/Jail.php b/lib/private/Files/Storage/Wrapper/Jail.php index 5673caf834ad8..46b40b922445d 100644 --- a/lib/private/Files/Storage/Wrapper/Jail.php +++ b/lib/private/Files/Storage/Wrapper/Jail.php @@ -370,13 +370,7 @@ public function getCache($path = '', $storage = null) { return new CacheJail($sourceCache, $this->rootPath); } - /** - * get the user id of the owner of a file or folder - * - * @param string $path - * @return string - */ - public function getOwner($path) { + public function getOwner($path): string|false { return $this->getWrapperStorage()->getOwner($this->getUnjailedPath($path)); } diff --git a/lib/private/Files/Storage/Wrapper/Wrapper.php b/lib/private/Files/Storage/Wrapper/Wrapper.php index f0420f4f16a4d..2a6d04da849ea 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -381,14 +381,7 @@ public function getScanner($path = '', $storage = null) { return $this->getWrapperStorage()->getScanner($path, $storage); } - - /** - * get the user id of the owner of a file or folder - * - * @param string $path - * @return string - */ - public function getOwner($path) { + public function getOwner($path): string|false { return $this->getWrapperStorage()->getOwner($path); } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 336349c680bab..9ac74461ca81b 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1676,11 +1676,9 @@ private function searchCommon($method, $args) { /** * Get the owner for a file or folder * - * @param string $path - * @return string the user id of the owner * @throws NotFoundException */ - public function getOwner($path) { + public function getOwner(string $path): string { $info = $this->getFileInfo($path); if (!$info) { throw new NotFoundException($path . ' not found while trying to get owner'); diff --git a/lib/private/Lockdown/Filesystem/NullStorage.php b/lib/private/Lockdown/Filesystem/NullStorage.php index 7175594a01dc6..00124877e9d59 100644 --- a/lib/private/Lockdown/Filesystem/NullStorage.php +++ b/lib/private/Lockdown/Filesystem/NullStorage.php @@ -155,8 +155,8 @@ public function test() { return true; } - public function getOwner($path) { - return null; + public function getOwner($path): string|false { + return false; } public function getCache($path = '', $storage = null) { diff --git a/lib/public/Files/Storage/IStorage.php b/lib/public/Files/Storage/IStorage.php index d2fd3b7555302..2016c273f9103 100644 --- a/lib/public/Files/Storage/IStorage.php +++ b/lib/public/Files/Storage/IStorage.php @@ -414,7 +414,7 @@ public function needsPartFile(); /** * @param string $path path for which to retrieve the owner - * @return string + * @return string|false * @since 9.0.0 */ public function getOwner($path); diff --git a/tests/lib/Lockdown/Filesystem/NullStorageTest.php b/tests/lib/Lockdown/Filesystem/NullStorageTest.php index cc65221d0a3a7..a0b9b04ad1adb 100644 --- a/tests/lib/Lockdown/Filesystem/NullStorageTest.php +++ b/tests/lib/Lockdown/Filesystem/NullStorageTest.php @@ -219,7 +219,7 @@ public function testTest() { } public function testGetOwner(): void { - $this->assertNull($this->storage->getOwner('foo')); + $this->assertFalse($this->storage->getOwner('foo')); } public function testGetCache(): void { From dc13f9cc1e3d8dce1c18d2c7920afe99b7bfa475 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 16 Sep 2024 16:00:46 +0200 Subject: [PATCH 2/2] fix(Files): Handle getOwner() returning false Signed-off-by: provokateurin --- apps/dav/lib/Storage/PublicOwnerWrapper.php | 10 ++--- .../lib/Versions/LegacyVersionsBackend.php | 4 ++ lib/private/Files/Node/Folder.php | 2 +- lib/private/Files/Node/Root.php | 19 ++++++++- lib/private/Files/View.php | 40 +++++++++++++++---- lib/private/legacy/OC_Helper.php | 3 +- 6 files changed, 60 insertions(+), 18 deletions(-) diff --git a/apps/dav/lib/Storage/PublicOwnerWrapper.php b/apps/dav/lib/Storage/PublicOwnerWrapper.php index 40ef81ab06d54..91c1916dc39b4 100644 --- a/apps/dav/lib/Storage/PublicOwnerWrapper.php +++ b/apps/dav/lib/Storage/PublicOwnerWrapper.php @@ -12,8 +12,7 @@ class PublicOwnerWrapper extends Wrapper { - /** @var string */ - private $owner; + private string $owner; /** * @param array $arguments ['storage' => $storage, 'owner' => $owner] @@ -28,11 +27,10 @@ public function __construct($arguments) { public function getOwner($path): string|false { $owner = parent::getOwner($path); - - if ($owner === null || $owner === false) { - return $this->owner; + if ($owner !== false) { + return $owner; } - return $owner; + return $this->owner; } } diff --git a/apps/files_versions/lib/Versions/LegacyVersionsBackend.php b/apps/files_versions/lib/Versions/LegacyVersionsBackend.php index fc37ecead95e9..bc46da857525a 100644 --- a/apps/files_versions/lib/Versions/LegacyVersionsBackend.php +++ b/apps/files_versions/lib/Versions/LegacyVersionsBackend.php @@ -49,6 +49,10 @@ public function getVersionsForFile(IUser $user, FileInfo $file): array { if ($storage->instanceOfStorage(ISharedStorage::class)) { $owner = $storage->getOwner(''); + if ($owner === false) { + throw new NotFoundException('No owner for ' . $file->getPath()); + } + $user = $this->userManager->get($owner); $fileId = $file->getId(); diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index ca256b09d336d..d8344f3d5bd16 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -253,7 +253,7 @@ private function cacheEntryToFileInfo(IMountPoint $mount, string $appendRoot, IC $owner = null; $ownerId = $storage->getOwner($cacheEntry['internalPath']); - if (!empty($ownerId)) { + if ($ownerId !== false) { // Cache the user manager (for performance) if ($this->userManager === null) { $this->userManager = \OCP\Server::get(IUserManager::class); diff --git a/lib/private/Files/Node/Root.php b/lib/private/Files/Node/Root.php index 416adc7f374ac..dbe03a4cfbfb6 100644 --- a/lib/private/Files/Node/Root.php +++ b/lib/private/Files/Node/Root.php @@ -28,6 +28,7 @@ use OCP\ICacheFactory; use OCP\IUser; use OCP\IUserManager; +use OCP\Server; use Psr\Log\LoggerInterface; /** @@ -477,9 +478,23 @@ public function getByIdInPath(int $id, string $path): array { $pathRelativeToMount = substr($internalPath, strlen($rootInternalPath)); $pathRelativeToMount = ltrim($pathRelativeToMount, '/'); $absolutePath = rtrim($mount->getMountPoint() . $pathRelativeToMount, '/'); + $storage = $mount->getStorage(); + if ($storage === null) { + return null; + } + $ownerId = $storage->getOwner($pathRelativeToMount); + if ($ownerId !== false) { + $owner = Server::get(IUserManager::class)->get($ownerId); + } else { + $owner = null; + } return $this->createNode($absolutePath, new FileInfo( - $absolutePath, $mount->getStorage(), $cacheEntry->getPath(), $cacheEntry, $mount, - \OC::$server->getUserManager()->get($mount->getStorage()->getOwner($pathRelativeToMount)) + $absolutePath, + $storage, + $cacheEntry->getPath(), + $cacheEntry, + $mount, + $owner, )); }, $mountsContainingFile); diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 9ac74461ca81b..80679a9481fa5 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -28,6 +28,7 @@ use OCP\Files\NotFoundException; use OCP\Files\ReservedWordException; use OCP\IUser; +use OCP\IUserManager; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use OCP\Server; @@ -1364,7 +1365,7 @@ public function getFileInfo($path, $includeMountPoints = true) { $ownerId = $storage->getOwner($internalPath); $owner = null; - if ($ownerId !== null && $ownerId !== false) { + if ($ownerId !== false) { // ownerId might be null if files are accessed with an access token without file system access $owner = $this->getUserObjectForOwner($ownerId); } @@ -1450,7 +1451,12 @@ public function getDirectoryContent($directory, $mimetype_filter = '', ?\OCP\Fil if ($sharingDisabled) { $content['permissions'] = $content['permissions'] & ~\OCP\Constants::PERMISSION_SHARE; } - $owner = $this->getUserObjectForOwner($storage->getOwner($content['path'])); + $ownerId = $storage->getOwner($content['path']); + if ($ownerId !== false) { + $owner = $this->getUserObjectForOwner($ownerId); + } else { + $owner = null; + } return new FileInfo($path . '/' . $content['name'], $storage, $content['path'], $content, $mount, $owner); }, $contents); $files = array_combine($fileNames, $fileInfos); @@ -1527,7 +1533,12 @@ public function getDirectoryContent($directory, $mimetype_filter = '', ?\OCP\Fil $rootEntry['permissions'] = $rootEntry['permissions'] & ~\OCP\Constants::PERMISSION_SHARE; } - $owner = $this->getUserObjectForOwner($subStorage->getOwner('')); + $ownerId = $subStorage->getOwner(''); + if ($ownerId !== false) { + $owner = $this->getUserObjectForOwner($ownerId); + } else { + $owner = null; + } $files[$rootEntry->getName()] = new FileInfo($path . '/' . $rootEntry['name'], $subStorage, '', $rootEntry, $mount, $owner); } } @@ -1644,7 +1655,12 @@ private function searchCommon($method, $args) { $internalPath = $result['path']; $path = $mountPoint . $result['path']; $result['path'] = substr($mountPoint . $result['path'], $rootLength); - $owner = $userManager->get($storage->getOwner($internalPath)); + $ownerId = $storage->getOwner($internalPath); + if ($ownerId !== false) { + $owner = $userManager->get($ownerId); + } else { + $owner = null; + } $files[] = new FileInfo($path, $storage, $internalPath, $result, $mount, $owner); } } @@ -1663,7 +1679,12 @@ private function searchCommon($method, $args) { $internalPath = $result['path']; $result['path'] = rtrim($relativeMountPoint . $result['path'], '/'); $path = rtrim($mountPoint . $internalPath, '/'); - $owner = $userManager->get($storage->getOwner($internalPath)); + $ownerId = $storage->getOwner($internalPath); + if ($ownerId !== false) { + $owner = $userManager->get($ownerId); + } else { + $owner = null; + } $files[] = new FileInfo($path, $storage, $internalPath, $result, $mount, $owner); } } @@ -1811,7 +1832,12 @@ private function getPartFileInfo(string $path): \OC\Files\FileInfo { $mount = $this->getMount($path); $storage = $mount->getStorage(); $internalPath = $mount->getInternalPath($this->getAbsolutePath($path)); - $owner = \OC::$server->getUserManager()->get($storage->getOwner($internalPath)); + $ownerId = $storage->getOwner($internalPath); + if ($ownerId !== false) { + $owner = Server::get(IUserManager::class)->get($ownerId); + } else { + $owner = null; + } return new FileInfo( $this->getAbsolutePath($path), $storage, @@ -1846,7 +1872,7 @@ public function verifyPath($path, $fileName, $readonly = false): void { // Short cut for read-only validation if ($readonly) { - $validator = \OCP\Server::get(FilenameValidator::class); + $validator = Server::get(FilenameValidator::class); if ($validator->isForbidden($fileName)) { $l = \OCP\Util::getL10N('lib'); throw new InvalidPathException($l->t('Filename is a reserved word')); diff --git a/lib/private/legacy/OC_Helper.php b/lib/private/legacy/OC_Helper.php index 33cc966da2a6e..6da063ef2ce58 100644 --- a/lib/private/legacy/OC_Helper.php +++ b/lib/private/legacy/OC_Helper.php @@ -541,10 +541,9 @@ public static function getStorageInfo($path, $rootInfo = null, $includeMountPoin $relative = 0; } - /** @var string $ownerId */ $ownerId = $storage->getOwner($path); $ownerDisplayName = ''; - if ($ownerId) { + if ($ownerId !== false) { $ownerDisplayName = \OC::$server->getUserManager()->getDisplayName($ownerId) ?? ''; }