chore: Improve getPermissions typing

CarlSchwan · CarlSchwan · commit 8f7153f89bf8 · 2026-05-06T14:54:33.000+02:00
Signed-off-by: Carl Schwan &lt;carlschwan@kde.org&gt;
diff --git 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<Constants::PERMISSION_*>
+	 */
 	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<Constants::PERMISSION_*> $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<Constants::PERMISSION_*> $permissions */
 		return $permissions;
 	}
 
diff --git 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<Constants::PERMISSION_*>
 	 */
 	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<Constants::PERMISSION_*> $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<Constants::PERMISSION_*>
 	 */
 	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
@@ -249,6 +249,7 @@ public function getPermissions(string $path = ''): int {
 		if (!$this->isValid()) {
 			return 0;
 		}
+		/** @var int-mask-of<Constants::PERMISSION_*> $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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -759,6 +759,9 @@ public function getETag(string $path): string|false {
 		return $meta ? $meta['etag'] : false;
 	}
 
+	/**
+	 * @return int-mask-of<Constants::PERMISSION_*>
+	 */
 	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
@@ -23,12 +23,12 @@
  */
 class PermissionsMask extends Wrapper {
 	/**
-	 * @var int the permissions bits we want to keep
+	 * @var int-mask-of<Constants::PERMISSION_*> 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<Constants::PERMISSION_*>, ...} $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
@@ -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
@@ -42,8 +42,8 @@ class Share implements IShare {
 	private $sharedBy;
 	/** @var string */
 	private $shareOwner;
-	/** @var int */
-	private $permissions;
+	/** @var int-mask-of<Constants::PERMISSION_*> */
+	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
@@ -153,10 +153,10 @@ public function isEncrypted();
 	 * \OCP\Constants::PERMISSION_SHARE
 	 * \OCP\Constants::PERMISSION_ALL
 	 *
-	 * @return int
+	 * @return int-mask-of<Constants::PERMISSION_*>
 	 * @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
@@ -160,13 +160,13 @@ public function getEtag();
 	 *  - \OCP\Constants::PERMISSION_DELETE
 	 *  - \OCP\Constants::PERMISSION_SHARE
 	 *
-	 * @return int
+	 * @return int-mask-of<Constants::PERMISSION_*>
 	 * @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
@@ -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<Constants::PERMISSION_*>
 	 * @since 9.0.0
 	 */
 	public function getPermissions(string $path);
diff --git a/lib/public/Share/IShare.php b/lib/public/Share/IShare.php
diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php

Original file line number	Diff line number	Diff line change
`@@ -249,6 +249,7 @@ public function getPermissions(string $path = ''): int {`
`249`	`249`	`if (!$this->isValid()) {`
`250`	`250`	`return 0;`
`251`	`251`	`}`
	`252`	`+ /** @var int-mask-of<Constants::PERMISSION_> $permissions /`
`252`	`253`	`$permissions = parent::getPermissions($path) & $this->superShare->getPermissions();`
`253`	`254`
`254`	`255`	`// part files and the mount point always have delete permissions`
Original file line number	Diff line number	Diff line change
`@@ -109,7 +109,7 @@ public function isEncrypted() {`
`109`	`109`	`}`
`110`	`110`
`111`	`111`	`#[\Override]`
`112`		`- public function getPermissions() {`
	`112`	`+ public function getPermissions(): int {`
`113`	`113`	`return $this->fileInfo->getPermissions();`
`114`	`114`	`}`
`115`	`115`
Original file line number	Diff line number	Diff line change
`@@ -207,11 +207,8 @@ public function getEncryptedVersion(): int {`
`207`	`207`	`return isset($this->data['encryptedVersion']) ? (int)$this->data['encryptedVersion'] : 1;`
`208`	`208`	`}`
`209`	`209`
`210`		`- /**`
`211`		`- * @return int`
`212`		`- */`
`213`	`210`	`#[\Override]`
`214`		`- public function getPermissions() {`
	`211`	`+ public function getPermissions(): int {`
`215`	`212`	`return (int)$this->data['permissions'];`
`216`	`213`	`}`
`217`	`214`
Original file line number	Diff line number	Diff line change
`@@ -256,11 +256,8 @@ public function getEtag() {`
`256`	`256`	`return $this->__call(__FUNCTION__, func_get_args());`
`257`	`257`	`}`
`258`	`258`
`259`		`- /**`
`260`		`- * @inheritDoc`
`261`		`- */`
`262`	`259`	`#[\Override]`
`263`		`- public function getPermissions() {`
	`260`	`+ public function getPermissions(): int {`
`264`	`261`	`if (isset($this->data['permissions'])) {`
`265`	`262`	`return $this->data['permissions'];`
`266`	`263`	`}`
Original file line number	Diff line number	Diff line change
`@@ -228,12 +228,11 @@ public function getEtag() {`
`228`	`228`	`}`
`229`	`229`
`230`	`230`	`/**`
`231`		`- * @return int`
`232`	`231`	`* @throws InvalidPathException`
`233`	`232`	`* @throws NotFoundException`
`234`	`233`	`*/`
`235`	`234`	`#[\Override]`
`236`		`- public function getPermissions() {`
	`235`	`+ public function getPermissions(): int {`
`237`	`236`	`return $this->getFileInfo(false)->getPermissions();`
`238`	`237`	`}`
`239`	`238`
Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,7 @@ public function getEtag() {`
`84`	`84`	`}`
`85`	`85`
`86`	`86`	`#[\Override]`
`87`		`- public function getPermissions() {`
	`87`	`+ public function getPermissions(): int {`
`88`	`88`	`if ($this->fileInfo) {`
`89`	`89`	`return parent::getPermissions();`
`90`	`90`	`} else {`
Original file line number	Diff line number	Diff line change
`@@ -86,7 +86,7 @@ public function getEtag() {`
`86`	`86`	`}`
`87`	`87`
`88`	`88`	`#[\Override]`
`89`		`- public function getPermissions() {`
	`89`	`+ public function getPermissions(): int {`
`90`	`90`	`if ($this->fileInfo) {`
`91`	`91`	`return parent::getPermissions();`
`92`	`92`	`} else {`