From 8f7153f89bf8316fdbd8214e40f64ec31b7bf536 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 27 Apr 2026 12:39:36 +0200 Subject: [PATCH] chore: Improve getPermissions typing Signed-off-by: Carl Schwan --- apps/dav/lib/Connector/Sabre/Node.php | 21 ++++--- apps/files_sharing/lib/External/Storage.php | 55 ++++++++++--------- apps/files_sharing/lib/SharedStorage.php | 1 + apps/files_trashbin/lib/Trash/TrashItem.php | 2 +- lib/private/Files/FileInfo.php | 5 +- lib/private/Files/Node/LazyFolder.php | 5 +- lib/private/Files/Node/Node.php | 3 +- lib/private/Files/Node/NonExistingFile.php | 2 +- lib/private/Files/Node/NonExistingFolder.php | 2 +- lib/private/Files/Node/Root.php | 20 ++----- lib/private/Files/Storage/DAV.php | 3 + .../Files/Storage/Wrapper/PermissionsMask.php | 4 +- lib/private/Share20/Manager.php | 5 -- lib/private/Share20/Share.php | 14 ++--- lib/public/Files/FileInfo.php | 4 +- lib/public/Files/Node.php | 4 +- lib/public/Files/Storage/IStorage.php | 3 +- lib/public/Share/IShare.php | 10 ++-- tests/lib/Share20/ManagerTest.php | 6 +- 19 files changed, 73 insertions(+), 96 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php index b2ecf9b2f6a22..8282ebb2378db 100644 --- a/apps/dav/lib/Connector/Sabre/Node.php +++ b/apps/dav/lib/Connector/Sabre/Node.php @@ -236,11 +236,17 @@ public function getInternalPath(): string { return $this->info->getInternalPath(); } + /** + * @param string|null $user + * @return int-mask-of + */ public function getSharePermissions(?string $user): int { // check of we access a federated share if ($user !== null) { try { - return $this->shareManager->getShareByToken($user)->getPermissions(); + /** @var int-mask-of $permissions */ + $permissions = $this->shareManager->getShareByToken($user)->getPermissions(); + return $permissions; } catch (ShareNotFound) { // ignore } @@ -259,21 +265,21 @@ public function getSharePermissions(?string $user): int { } /* - * We can always share non moveable mount points with DELETE and UPDATE + * We can always share non-moveable mount points with DELETE and UPDATE * Eventually we need to do this properly */ $mountpoint = $this->info->getMountPoint(); if (!($mountpoint instanceof IMovableMount)) { /** * @psalm-suppress UnnecessaryVarAnnotation Rector doesn't trust the return type annotation - * @var string $mountpointpath + * @var string $mountpointPath */ - $mountpointpath = $mountpoint->getMountPoint(); - if (str_ends_with($mountpointpath, '/')) { - $mountpointpath = substr($mountpointpath, 0, -1); + $mountpointPath = $mountpoint->getMountPoint(); + if (str_ends_with($mountpointPath, '/')) { + $mountpointPath = substr($mountpointPath, 0, -1); } - if (!$mountpoint->getOption('readonly', false) && $mountpointpath === $this->info->getPath()) { + if (!$mountpoint->getOption('readonly', false) && $mountpointPath === $this->info->getPath()) { $permissions |= Constants::PERMISSION_DELETE | Constants::PERMISSION_UPDATE; } } @@ -285,6 +291,7 @@ public function getSharePermissions(?string $user): int { $permissions &= ~(Constants::PERMISSION_CREATE | Constants::PERMISSION_DELETE); } + /** @var int-mask-of $permissions */ return $permissions; } diff --git a/apps/files_sharing/lib/External/Storage.php b/apps/files_sharing/lib/External/Storage.php index f074d86d54fd4..3389ade834bd4 100644 --- a/apps/files_sharing/lib/External/Storage.php +++ b/apps/files_sharing/lib/External/Storage.php @@ -362,20 +362,27 @@ public function getPermissions(string $path): int { $ocsPermissions = $response['{http://open-collaboration-services.org/ns}share-permissions'] ?? null; $ocmPermissions = $response['{http://open-cloud-mesh.org/ns}share-permissions'] ?? null; $ocPermissions = $response['{http://owncloud.org/ns}permissions'] ?? null; + // old federated sharing permissions if ($ocsPermissions !== null) { - $permissions = (int)$ocsPermissions; - } elseif ($ocmPermissions !== null) { + $permission = (int)$ocsPermissions; + if ($permission < 0 || $permission > Constants::PERMISSION_ALL) { + throw new \RuntimeException('Got invalid permissions: ' . $ocsPermissions); + } + return $permission; + } + + if ($ocmPermissions !== null) { // permissions provided by the OCM API - $permissions = $this->ocmPermissions2ncPermissions($ocmPermissions, $path); - } elseif ($ocPermissions !== null) { + return $this->ocmPermissions2ncPermissions($ocmPermissions, $path); + } + + if ($ocPermissions !== null) { return $this->parsePermissions($ocPermissions); - } else { - // use default permission if remote server doesn't provide the share permissions - $permissions = $this->getDefaultPermissions($path); } - return $permissions; + // use default permission if remote server doesn't provide the share permissions + return $this->getDefaultPermissions($path); } #[\Override] @@ -388,36 +395,30 @@ public function needsPartFile(): bool { * * @param string $ocmPermissions json encoded OCM permissions * @param string $path path to file - * @return int + * @return int-mask-of */ protected function ocmPermissions2ncPermissions(string $ocmPermissions, string $path): int { try { - $ocmPermissions = json_decode($ocmPermissions); + $ocmPermissions = json_decode($ocmPermissions, flags: JSON_THROW_ON_ERROR); $ncPermissions = 0; foreach ($ocmPermissions as $permission) { - switch (strtolower($permission)) { - case 'read': - $ncPermissions += Constants::PERMISSION_READ; - break; - case 'write': - $ncPermissions += Constants::PERMISSION_CREATE + Constants::PERMISSION_UPDATE; - break; - case 'share': - $ncPermissions += Constants::PERMISSION_SHARE; - break; - default: - throw new \Exception(); - } + $ncPermissions |= match (strtolower($permission)) { + 'read' => Constants::PERMISSION_READ, + 'write' => Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE, + 'share' => Constants::PERMISSION_SHARE, + default => throw new \Exception(), + }; } - } catch (\Exception $e) { - $ncPermissions = $this->getDefaultPermissions($path); + /** @var int-mask-of $ncPermissions */ + return $ncPermissions; + } catch (\Exception) { + return $this->getDefaultPermissions($path); } - - return $ncPermissions; } /** * Calculate the default permissions in case no permissions are provided + * @return int-mask-of */ protected function getDefaultPermissions(string $path): int { if ($this->is_dir($path)) { diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index c1e8faeae338b..a2423fe8a6c47 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/apps/files_sharing/lib/SharedStorage.php @@ -249,6 +249,7 @@ public function getPermissions(string $path = ''): int { if (!$this->isValid()) { return 0; } + /** @var int-mask-of $permissions */ $permissions = parent::getPermissions($path) & $this->superShare->getPermissions(); // part files and the mount point always have delete permissions diff --git a/apps/files_trashbin/lib/Trash/TrashItem.php b/apps/files_trashbin/lib/Trash/TrashItem.php index 9e1a28a5ee76d..dffab8fb17c50 100644 --- a/apps/files_trashbin/lib/Trash/TrashItem.php +++ b/apps/files_trashbin/lib/Trash/TrashItem.php @@ -109,7 +109,7 @@ public function isEncrypted() { } #[\Override] - public function getPermissions() { + public function getPermissions(): int { return $this->fileInfo->getPermissions(); } diff --git a/lib/private/Files/FileInfo.php b/lib/private/Files/FileInfo.php index 65280b0f31580..bc6d554031cd0 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -207,11 +207,8 @@ public function getEncryptedVersion(): int { return isset($this->data['encryptedVersion']) ? (int)$this->data['encryptedVersion'] : 1; } - /** - * @return int - */ #[\Override] - public function getPermissions() { + public function getPermissions(): int { return (int)$this->data['permissions']; } diff --git a/lib/private/Files/Node/LazyFolder.php b/lib/private/Files/Node/LazyFolder.php index cd67de7c33a97..bd564252897bb 100644 --- a/lib/private/Files/Node/LazyFolder.php +++ b/lib/private/Files/Node/LazyFolder.php @@ -256,11 +256,8 @@ public function getEtag() { return $this->__call(__FUNCTION__, func_get_args()); } - /** - * @inheritDoc - */ #[\Override] - public function getPermissions() { + public function getPermissions(): int { if (isset($this->data['permissions'])) { return $this->data['permissions']; } diff --git a/lib/private/Files/Node/Node.php b/lib/private/Files/Node/Node.php index 63e275d1f476d..8687cd6e7bdfa 100644 --- a/lib/private/Files/Node/Node.php +++ b/lib/private/Files/Node/Node.php @@ -228,12 +228,11 @@ public function getEtag() { } /** - * @return int * @throws InvalidPathException * @throws NotFoundException */ #[\Override] - public function getPermissions() { + public function getPermissions(): int { return $this->getFileInfo(false)->getPermissions(); } diff --git a/lib/private/Files/Node/NonExistingFile.php b/lib/private/Files/Node/NonExistingFile.php index 356ba5e47857c..74c89b9b043f4 100644 --- a/lib/private/Files/Node/NonExistingFile.php +++ b/lib/private/Files/Node/NonExistingFile.php @@ -84,7 +84,7 @@ public function getEtag() { } #[\Override] - public function getPermissions() { + public function getPermissions(): int { if ($this->fileInfo) { return parent::getPermissions(); } else { diff --git a/lib/private/Files/Node/NonExistingFolder.php b/lib/private/Files/Node/NonExistingFolder.php index 487170c98fade..e41eec5c4d5bf 100644 --- a/lib/private/Files/Node/NonExistingFolder.php +++ b/lib/private/Files/Node/NonExistingFolder.php @@ -86,7 +86,7 @@ public function getEtag() { } #[\Override] - public function getPermissions() { + public function getPermissions(): int { if ($this->fileInfo) { return parent::getPermissions(); } else { diff --git a/lib/private/Files/Node/Root.php b/lib/private/Files/Node/Root.php index 095db425ed606..112a21e5027a6 100644 --- a/lib/private/Files/Node/Root.php +++ b/lib/private/Files/Node/Root.php @@ -254,35 +254,23 @@ public function getSize($includeMounts = true): int|float { return 0; } - /** - * @return string - */ #[\Override] - public function getEtag() { + public function getEtag(): string { return ''; } - /** - * @return int - */ #[\Override] - public function getPermissions() { + public function getPermissions(): int { return Constants::PERMISSION_CREATE; } - /** - * @return bool - */ #[\Override] - public function isReadable() { + public function isReadable(): bool { return false; } - /** - * @return bool - */ #[\Override] - public function isUpdateable() { + public function isUpdateable(): bool { return false; } diff --git a/lib/private/Files/Storage/DAV.php b/lib/private/Files/Storage/DAV.php index 0c74abdfee1a2..86155c91947ec 100644 --- a/lib/private/Files/Storage/DAV.php +++ b/lib/private/Files/Storage/DAV.php @@ -759,6 +759,9 @@ public function getETag(string $path): string|false { return $meta ? $meta['etag'] : false; } + /** + * @return int-mask-of + */ protected function parsePermissions(string $permissionsString): int { $permissions = Constants::PERMISSION_READ; if (str_contains($permissionsString, 'R')) { diff --git a/lib/private/Files/Storage/Wrapper/PermissionsMask.php b/lib/private/Files/Storage/Wrapper/PermissionsMask.php index 15061b7c02397..9fd981f990127 100644 --- a/lib/private/Files/Storage/Wrapper/PermissionsMask.php +++ b/lib/private/Files/Storage/Wrapper/PermissionsMask.php @@ -23,12 +23,12 @@ */ class PermissionsMask extends Wrapper { /** - * @var int the permissions bits we want to keep + * @var int-mask-of the permissions bits we want to keep */ protected readonly int $mask; /** - * @param array{storage: IStorage, mask: int, ...} $parameters + * @param array{storage: IStorage, mask: int-mask-of, ...} $parameters * * $storage: The storage the permissions mask should be applied on * $mask: The permission bits that should be kept, a combination of the \OCP\Constant::PERMISSION_ constants diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 9d68aa23653c5..4e7d2762205f5 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -223,11 +223,6 @@ protected function generalCreateChecks(IShare $share, bool $isUpdate = false): v throw new GenericShareException($this->l->t('You are not allowed to share %s', [$share->getNode()->getName()]), code: 404); } - // Permissions should be set - if ($share->getPermissions() === null) { - throw new \InvalidArgumentException($this->l->t('Valid permissions are required for sharing')); - } - // Permissions must be valid if ($share->getPermissions() < 0 || $share->getPermissions() > Constants::PERMISSION_ALL) { throw new \InvalidArgumentException($this->l->t('Valid permissions are required for sharing')); diff --git a/lib/private/Share20/Share.php b/lib/private/Share20/Share.php index 2ededbc7c361a..6015d2abcf163 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -42,8 +42,8 @@ class Share implements IShare { private $sharedBy; /** @var string */ private $shareOwner; - /** @var int */ - private $permissions; + /** @var int-mask-of */ + private int $permissions = 0; /** @var IAttributes */ private $attributes; /** @var int */ @@ -286,22 +286,16 @@ public function getSharedWithAvatar() { return $this->sharedWithAvatar; } - /** - * @inheritdoc - */ #[\Override] - public function setPermissions($permissions) { + public function setPermissions(int $permissions): self { //TODO checks $this->permissions = $permissions; return $this; } - /** - * @inheritdoc - */ #[\Override] - public function getPermissions() { + public function getPermissions(): int { return $this->permissions; } diff --git a/lib/public/Files/FileInfo.php b/lib/public/Files/FileInfo.php index 76b2083085daf..c65e1ae7f77ff 100644 --- a/lib/public/Files/FileInfo.php +++ b/lib/public/Files/FileInfo.php @@ -153,10 +153,10 @@ public function isEncrypted(); * \OCP\Constants::PERMISSION_SHARE * \OCP\Constants::PERMISSION_ALL * - * @return int + * @return int-mask-of * @since 7.0.0 - namespace of constants has changed in 8.0.0 */ - public function getPermissions(); + public function getPermissions(): int; /** * Check whether this is a file or a folder diff --git a/lib/public/Files/Node.php b/lib/public/Files/Node.php index b3b0ffb5c01fc..4f7190e96dcda 100644 --- a/lib/public/Files/Node.php +++ b/lib/public/Files/Node.php @@ -160,13 +160,13 @@ public function getEtag(); * - \OCP\Constants::PERMISSION_DELETE * - \OCP\Constants::PERMISSION_SHARE * - * @return int + * @return int-mask-of * @throws InvalidPathException * @throws NotFoundException * @since 6.0.0 - namespace of constants has changed in 8.0.0 */ #[\Override] - public function getPermissions(); + public function getPermissions(): int; /** * Check if the file or folder is readable diff --git a/lib/public/Files/Storage/IStorage.php b/lib/public/Files/Storage/IStorage.php index ab88404b70c94..1a37dd3825484 100644 --- a/lib/public/Files/Storage/IStorage.php +++ b/lib/public/Files/Storage/IStorage.php @@ -10,6 +10,7 @@ namespace OCP\Files\Storage; +use OCP\Constants; use OCP\Files\Cache\ICache; use OCP\Files\Cache\IPropagator; use OCP\Files\Cache\IScanner; @@ -147,7 +148,7 @@ public function isSharable(string $path); * get the full permissions of a path. * Should return a combination of the PERMISSION_ constants defined in lib/public/constants.php * - * @return int + * @return int-mask-of * @since 9.0.0 */ public function getPermissions(string $path); diff --git a/lib/public/Share/IShare.php b/lib/public/Share/IShare.php index 7bd1b8c5ebda3..5ebfabdb1b599 100644 --- a/lib/public/Share/IShare.php +++ b/lib/public/Share/IShare.php @@ -275,22 +275,20 @@ public function getSharedWithAvatar(); /** * Set the permissions. - * See \OCP\Constants::PERMISSION_* * - * @param int $permissions + * @param int-mask-of $permissions * @return IShare The modified object * @since 9.0.0 */ - public function setPermissions($permissions); + public function setPermissions(int $permissions): self; /** * Get the share permissions - * See \OCP\Constants::PERMISSION_* * - * @return int + * @return int-mask-of * @since 9.0.0 */ - public function getPermissions(); + public function getPermissions(): int; /** * Create share attributes object diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index be9e817e8fb27..8ecc5605a7165 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -911,7 +911,7 @@ public function testVerifyPasswordHookFails(): void { } public function createShare($id, int $type, ?Node $node, $sharedWith, $sharedBy, $shareOwner, - $permissions, $expireDate = null, $password = null, $attributes = null): IShare&MockObject { + int $permissions, $expireDate = null, $password = null, $attributes = null): IShare&MockObject { $share = $this->createMock(IShare::class); $share->method('getShareType')->willReturn($type); @@ -1013,10 +1013,6 @@ public static function dataGeneralChecks(): array { 'default', ]; - $data[] = [[null, IShare::TYPE_USER, $limitedPermssions, $user2, $user0, $user0, null, null, null], 'Valid permissions are required for sharing', true]; - $data[] = [[null, IShare::TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, null, null, null], 'Valid permissions are required for sharing', true]; - $data[] = [[null, IShare::TYPE_LINK, $limitedPermssions, null, $user0, $user0, null, null, null], 'Valid permissions are required for sharing', true]; - $limitedPermssions[1]['getMountPoint'] = IMovableMount::class; // increase permissions of a re-share