Merge pull request #56967 from nextcloud/fix/share-download

fix(files_sharing): make legacy `downloadShare` endpoint compatible with legacy behavior

apps/files_sharing/lib/Controller/ShareController.php

@@ -15,6 +15,7 @@ use OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent;
 use OCA\Files_Sharing\Event\ShareLinkAccessedEvent;
 use OCP\Accounts\IAccountManager;
 use OCP\AppFramework\AuthPublicShareController;
+use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
 use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired;
 use OCP\AppFramework\Http\Attribute\OpenAPI;
@@ -30,6 +31,7 @@ use OCP\Files\File;
 use OCP\Files\Folder;
 use OCP\Files\IRootFolder;
 use OCP\Files\NotFoundException;
+use OCP\Files\NotPermittedException;
 use OCP\HintException;
 use OCP\IConfig;
 use OCP\IL10N;
@@ -356,49 +358,75 @@ class ShareController extends AuthPublicShareController {
 		$share = $this->shareManager->getShareByToken($token);
 
 		if (!($share->getPermissions() & Constants::PERMISSION_READ)) {
-			return new DataResponse('Share has no read permission');
+			return new DataResponse('Share has no read permission', Http::STATUS_FORBIDDEN);
 		}
 
 		$attributes = $share->getAttributes();
 		if ($attributes?->getAttribute('permissions', 'download') === false) {
-			return new DataResponse('Share has no download permission');
+			return new DataResponse('Share has no download permission', Http::STATUS_FORBIDDEN);
 		}
 
 		if (!$this->validateShare($share)) {
 			throw new NotFoundException();
 		}
 
-		$node = $share->getNode();
-		if ($node instanceof Folder) {
-			// Directory share
+		if ($share->getHideDownload()) {
+			// download API does not work if hidden - use the DAV endpoint for previews
+			throw new NotFoundException();
+		}
 
-			// Try to get the path
-			if ($path !== '') {
+		$node = $share->getNode();
+		if ($path !== '') {
+			if (!$node instanceof Folder) {
+				return new NotFoundResponse();
+			}
+
+			try {
+				$node = $node->get($path);
+			} catch (NotFoundException|NotPermittedException) {
+				$this->emitAccessShareHook($share, 404, 'Share not found');
+				$this->emitShareAccessEvent($share, self::SHARE_DOWNLOAD, 404, 'Share not found');
+				return new NotFoundResponse();
+			}
+		}
+
+		if ($files !== null) {
+			if (!$node instanceof Folder) {
+				return new NotFoundResponse();
+			}
+
+			$filesParam = json_decode($files, true);
+			if (!is_array($filesParam)) {
 				try {
-					$node = $node->get($path);
-				} catch (NotFoundException $e) {
+					// legacy wise this allows also passing the filename
+					$node = $node->get($files);
+					$files = null;
+				} catch (NotFoundException|NotPermittedException) {
 					$this->emitAccessShareHook($share, 404, 'Share not found');
 					$this->emitShareAccessEvent($share, self::SHARE_DOWNLOAD, 404, 'Share not found');
 					return new NotFoundResponse();
 				}
 			}
-
-			if ($node instanceof Folder) {
-				if ($files === null || $files === '') {
-					if ($share->getHideDownload()) {
-						throw new NotFoundException('Downloading a folder');
-					}
-				}
-			}
 		}
 
 		$this->emitAccessShareHook($share);
 		$this->emitShareAccessEvent($share, self::SHARE_DOWNLOAD);
 
-		$davUrl = '/public.php/dav/files/' . $token . '/?accept=zip';
-		if ($files !== null) {
-			$davUrl .= '&files=' . $files;
+		$davPath = '';
+		if ($node !== $share->getNode()) {
+			$davPath = substr($node->getPath(), strlen($share->getNode()->getPath()));
 		}
+
+		$params = [];
+		if ($files !== null) {
+			$params['files'] = $files;
+		}
+		if ($node instanceof Folder) {
+			$params['accept'] = 'zip';
+		}
+
+		$davUrl = '/public.php/dav/files/' . $token . $davPath;
+		$davUrl .= '?' . http_build_query($params);
 		return new RedirectResponse($this->urlGenerator->getAbsoluteURL($davUrl));
 	}
 }

apps/files_sharing/tests/Controller/ShareControllerTest.php

@@ -18,6 +18,7 @@ use OCP\Accounts\IAccount;
 use OCP\Accounts\IAccountManager;
 use OCP\Accounts\IAccountProperty;
 use OCP\Activity\IManager;
+use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\ContentSecurityPolicy;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\Template\ExternalShareMenuAction;
@@ -691,7 +692,9 @@ class ShareControllerTest extends \Test\TestCase {
 			->with('token')
 			->willReturn($share);
 
-		$this->userManager->method('get')->with('ownerUID')->willReturn($owner);
+		$this->userManager->method('get')
+			->with('ownerUID')
+			->willReturn($owner);
 
 		$this->shareController->showShare();
 	}
@@ -712,7 +715,7 @@ class ShareControllerTest extends \Test\TestCase {
 
 		// Test with a password protected share and no authentication
 		$response = $this->shareController->downloadShare('validtoken');
-		$expectedResponse = new DataResponse('Share has no read permission');
+		$expectedResponse = new DataResponse('Share has no read permission', Http::STATUS_FORBIDDEN);
 		$this->assertEquals($expectedResponse, $response);
 	}
 
@@ -740,7 +743,7 @@ class ShareControllerTest extends \Test\TestCase {
 
 		// Test with a password protected share and no authentication
 		$response = $this->shareController->downloadShare('validtoken');
-		$expectedResponse = new DataResponse('Share has no download permission');
+		$expectedResponse = new DataResponse('Share has no download permission', Http::STATUS_FORBIDDEN);
 		$this->assertEquals($expectedResponse, $response);
 	}
 

build/psalm-baseline.xml

@@ -1518,6 +1518,7 @@
       <code><![CDATA[emitAccessShareHook]]></code>
       <code><![CDATA[emitAccessShareHook]]></code>
       <code><![CDATA[emitAccessShareHook]]></code>
+      <code><![CDATA[emitAccessShareHook]]></code>
     </DeprecatedMethod>
   </file>
   <file src="apps/files_sharing/lib/External/Manager.php">