refactor(Helper,TokenManager): migrate guest cookie access to IRequest add typing throughout

lib/Controller/WopiController.php

@@ -594,7 +594,7 @@ public function putFile(
 			if ($isPutRelative) {
 				// the new file needs to be installed in the current user dir
 				$userFolder = $this->rootFolder->getUserFolder($wopi->getEditorUid());
-				$file = $userFolder->getFirstNodeById($fileId);
+				$file = $userFolder->getFirstNodeById((int)$fileId);
 				if ($file === null) {
 					return new JSONResponse([], Http::STATUS_NOT_FOUND);
 				}

lib/Helper.php

@@ -1,5 +1,6 @@
 <?php
 
+declare(strict_types=1);
 /**
  * SPDX-FileCopyrightText: 2018 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -16,78 +17,100 @@
 
 class Helper {
 	/**
-	 * @param string|null $userId
+	 * @param string|null $userId Current user ID, or null for guest sessions.
 	 */
 	public function __construct(
-		private $userId,
+		private ?string $userId,
 	) {
 	}
 
 	/**
-	 * @param string $fileId
-	 * @return array
-	 * @throws \Exception
+	 * Parse a WOPI file identifier string into its components.
+	 *
+	 * Supports standard and template-based identifiers. Standard formats include:
+	 * - {fileId}
+	 * - {fileId}_{instanceId}
+	 * - {fileId}_{instanceId}_{version}
+	 *
+	 * For template-based documents, the file part contains a template marker and is expected to be
+	 * encoded as "{fileId}/{templateId}".
+	 *
+	 * @param string $fileId The WOPI-encoded file identifier string to parse.
+	 * @return array{0: string, 1: string, 2: string, 3: string|null} [fileId, instanceId, version, templateId]
+	 * @throws \InvalidArgumentException If the identifier does not match the expected format.
 	 */
-	public static function parseFileId(string $fileId) {
-		$arr = explode('_', $fileId);
-		$templateId = null;
-		if (count($arr) === 1) {
-			$fileId = $arr[0];
-			$instanceId = '';
-			$version = '0';
-		} elseif (count($arr) === 2) {
-			[$fileId, $instanceId] = $arr;
-			$version = '0';
-		} elseif (count($arr) === 3) {
-			[$fileId, $instanceId, $version] = $arr;
-		} else {
-			throw new \Exception('$fileId has not the expected format');
+	public static function parseFileId(string $fileId): array {
+		$parts = explode('_', $fileId);
+
+		if (count($parts) > 3) {
+			throw new \InvalidArgumentException('$fileId does not match the expected format');
 		}
 
-		if (str_contains($fileId, '-')) {
-			[$fileId, $templateId] = array_pad(explode('/', $fileId), 2, null);
+		$fileIdPart = $parts[0];
+		$instanceId = $parts[1] ?? '';
+		$version = $parts[2] ?? '0';
+		$templateId = null;
+
+		$hasTemplateMarker = str_contains($fileIdPart, '-');
+		if ($hasTemplateMarker) {
+			[$fileIdPart, $templateId] = array_pad(explode('/', $fileIdPart, 2), 2, null);
 		}
 
-		return [
-			$fileId,
-			$instanceId,
-			$version,
-			$templateId
-		];
+		return [$fileIdPart, $instanceId, $version, $templateId];
 	}
 
 	/**
-	 * WOPI helper function to convert to ISO 8601 round-trip format.
-	 * @param integer $time Must be seconds since unix epoch
+	 * Convert a Unix timestamp to WOPI's ISO 8601 round-trip format.
+	 *
+	 * @param int $time Seconds since the Unix epoch.
+	 * @return string|false Formatted timestamp, or false if conversion fails.
 	 */
-	public static function toISO8601($time) {
-		// TODO: Be more precise and don't ignore milli, micro seconds ?
-		$datetime = DateTime::createFromFormat('U', $time, new DateTimeZone('UTC'));
+	public static function toISO8601(int $time): string|false {
+		// TODO: Be more precise and don't ignore milli, micro seconds?
+		// 		 e.g. Accept float|DateTimeInterface to support sub-second precision
+		//		 (.u is always 000000 with int input).
+		$datetime = DateTime::createFromFormat('U', (string)$time, new DateTimeZone('UTC'));
 		if ($datetime) {
 			return $datetime->format('Y-m-d\TH:i:s.u\Z');
 		}
 
 		return false;
 	}
 
-	public static function getNewFileName(Folder $folder, $filename) {
+	/**
+	 * Return a unique file name by appending an incrementing suffix if needed.
+	 *
+	 * @param Folder $folder Folder to check for name collisions.
+	 * @param string $filename Proposed file name.
+	 * @return string Collision-free file name.
+	 * @throws \RuntimeException If regex/PCRE outright fails (unlikely).
+	 */
+	public static function getNewFileName(Folder $folder, string $filename): string {
 		$fileNum = 1;
 
 		while ($folder->nodeExists($filename)) {
 			$fileNum++;
-			$filename = preg_replace('/(\.| \(\d+\)\.)([^.]*)$/', ' (' . $fileNum . ').$2', $filename);
-		}
+			$newFilename = preg_replace('/(\.| \(\d+\)\.)([^.]*)$/', ' (' . $fileNum . ').$2', $filename);
 
-		return $filename;
-	}
+			if ($newFilename === null) {
+				// unlikely unless regex is broken
+				throw new \RuntimeException('Failed to generate a unique filename');
+			}
 
-	public function getGuestNameFromCookie() {
-		if ($this->userId !== null || !isset($_COOKIE['guestUser']) || $_COOKIE['guestUser'] === '') {
-			return null;
+			$filename = $newFilename;
 		}
-		return $_COOKIE['guestUser'];
+
+		return $filename;
 	}
 
+	/**
+	 * Resolve the share associated with a node from shared storage.
+	 *
+	 * TODO: Put this elsewhere.
+	 *
+	 * @param Node $node File node to inspect.
+	 * @return IShare|null The share backing the node, or null if it is not shared.
+	 */
 	public function getShareFromNode(Node $node): ?IShare {
 		try {
 			$storage = $node->getStorage();

lib/TokenManager.php

@@ -1,5 +1,6 @@
 <?php
 
+declare(strict_types=1);
 /**
  * SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -21,6 +22,7 @@
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
 use OCP\IL10N;
+use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Share\IManager;
@@ -40,13 +42,19 @@ public function __construct(
 		private PermissionManager $permissionManager,
 		private IEventDispatcher $eventDispatcher,
 		private LoggerInterface $logger,
+		private IRequest $request,
 	) {
 	}
 
 	/**
 	 * @throws Exception
 	 */
-	public function generateWopiToken(string $fileId, ?string $shareToken = null, ?string $editoruid = null, bool $direct = false): Wopi {
+	public function generateWopiToken(
+		string $fileId,
+		?string $shareToken = null,
+		?string $editoruid = null,
+		bool $direct = false,
+	): Wopi {
 		[$fileId, , $version] = Helper::parseFileId($fileId);
 		$owneruid = null;
 		$hideDownload = false;
@@ -94,7 +102,7 @@ public function generateWopiToken(string $fileId, ?string $shareToken = null, ?s
 		}
 
 		/** @var File $file */
-		$file = $userFolder->getFirstNodeById($fileId);
+		$file = $userFolder->getFirstNodeById((int)$fileId);
 
 		// Check node readability (for storage wrapper overwrites like terms of services)
 		if ($file === null || !$file->isReadable()) {
@@ -121,15 +129,50 @@ public function generateWopiToken(string $fileId, ?string $shareToken = null, ?s
 		$this->eventDispatcher->dispatchTyped(new BeforeNodeReadEvent($file));
 
 		$serverHost = $this->urlGenerator->getAbsoluteURL('/');
-		$guestName = $editoruid === null ? $this->prepareGuestName($this->helper->getGuestNameFromCookie()) : null;
-		return $this->wopiMapper->generateFileToken($fileId, $owneruid, $editoruid, $version, $updatable, $serverHost, $guestName, $hideDownload, $direct, 0, $shareToken);
+
+		$guestName = $editoruid === null ? $this->prepareGuestName($this->getGuestNameFromCookie()) : null;
+
+		return $this->wopiMapper->generateFileToken(
+			(int)$fileId,
+			$owneruid,
+			$editoruid,
+			$version,
+			$updatable,
+			$serverHost,
+			$guestName,
+			$hideDownload,
+			$direct,
+			0,
+			$shareToken
+		);
+	}
+
+	private function getGuestNameFromCookie(): ?string {
+		// prevent a logged-in user from being treated as a guest via the cookie
+		if ($this->userId === null) {
+			return null;
+		}
+
+		$guestUser = $this->request->getCookie('guestUser');
+
+		if ($guestUser === '' || $guestUser === null) {
+			return null;
+		}
+
+		return $guestUser;
 	}
 
 	/**
 	 * This method is receiving the results from the TOKEN_TYPE_FEDERATION generated on the opener server
 	 * that is created in {@link newInitiatorToken}
 	 */
-	public function upgradeToRemoteToken(Wopi $wopi, Wopi $remoteWopi, string $shareToken, string $remoteServer, string $remoteServerToken): Wopi {
+	public function upgradeToRemoteToken(
+		Wopi $wopi,
+		Wopi $remoteWopi,
+		string $shareToken,
+		string $remoteServer,
+		string $remoteServerToken,
+	): Wopi {
 		if ($remoteWopi->getTokenType() !== Wopi::TOKEN_TYPE_INITIATOR) {
 			return $wopi;
 		}
@@ -150,7 +193,7 @@ public function upgradeToRemoteToken(Wopi $wopi, Wopi $remoteWopi, string $share
 		return $wopi;
 	}
 
-	public function upgradeFromDirectInitiator(Direct $direct, Wopi $wopi) {
+	public function upgradeFromDirectInitiator(Direct $direct, Wopi $wopi): Wopi {
 		$wopi->setTokenType(Wopi::TOKEN_TYPE_REMOTE_GUEST);
 		$wopi->setEditorUid(null);
 		$wopi->setRemoteServer($direct->getInitiatorHost());
@@ -207,7 +250,13 @@ public function generateWopiTokenForTemplate(
 		);
 	}
 
-	public function newInitiatorToken($sourceServer, ?Node $node = null, $shareToken = null, bool $direct = false, $userId = null): Wopi {
+	public function newInitiatorToken(
+		string $sourceServer,
+		?Node $node = null,
+		?string $shareToken = null,
+		bool $direct = false,
+		?string $userId = null,
+	): Wopi {
 		if ($node !== null) {
 			$wopi = $this->generateWopiToken((string)$node->getId(), $shareToken, $userId, $direct);
 			$wopi->setServerHost($sourceServer);
@@ -226,7 +275,7 @@ public function extendWithInitiatorUserToken(Wopi $wopi, string $initiatorUserHo
 		return $wopi;
 	}
 
-	public function prepareGuestName(?string $guestName = null) {
+	public function prepareGuestName(?string $guestName = null): string {
 		if (empty($guestName)) {
 			return $this->trans->t('Anonymous guest');
 		}
@@ -244,13 +293,10 @@ public function prepareGuestName(?string $guestName = null) {
 	}
 
 	/**
-	 * @param string $accessToken
-	 * @param string $guestName
-	 * @return void
 	 * @throws Exceptions\ExpiredTokenException
 	 * @throws Exceptions\UnknownTokenException
 	 */
-	public function updateGuestName(string $accessToken, string $guestName) {
+	public function updateGuestName(string $accessToken, string $guestName): void {
 		$wopi = $this->wopiMapper->getWopiForToken($accessToken);
 		$wopi->setGuestDisplayname($this->prepareGuestName($guestName));
 		$this->wopiMapper->update($wopi);

tests/psalm-baseline.xml

@@ -80,11 +80,6 @@
       <code><![CDATA[putContent]]></code>
     </UndefinedInterfaceMethod>
   </file>
-  <file src="lib/Helper.php">
-    <InvalidScalarArgument>
-      <code><![CDATA[$time]]></code>
-    </InvalidScalarArgument>
-  </file>
   <file src="lib/PermissionManager.php">
     <RedundantCondition>
       <code><![CDATA[$share && method_exists($share, 'getAttributes')]]></code>