From 8da8a80e19c4cb431dabde0c28b1c54a54e53273 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 10 Dec 2025 16:26:14 +0100 Subject: [PATCH] fix(files_sharing): make legacy `downloadShare` endpoint compatible with legacy behavior This needs to be able to directly download files if specified to only download a single file and not a folder. Also it was possible to either pass a files array json encoded or a single file not encoded at all. Signed-off-by: Ferdinand Thiessen --- .../lib/Controller/ShareController.php | 64 +++++++++++++------ .../tests/Controller/ShareControllerTest.php | 9 ++- 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 3c51a95f1d5..bc078cb0b2b 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -14,6 +14,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\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; @@ -29,6 +30,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; @@ -360,49 +362,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(); } + if ($share->getHideDownload()) { + // download API does not work if hidden - use the DAV endpoint for previews + throw new NotFoundException(); + } + $node = $share->getNode(); - if ($node instanceof Folder) { - // Directory share + if ($path !== '') { + if (!$node instanceof Folder) { + return new NotFoundResponse(); + } - // Try to get the path - if ($path !== '') { + 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'; + $davPath = ''; + if ($node !== $share->getNode()) { + $davPath = substr($node->getPath(), strlen($share->getNode()->getPath())); + } + + $params = []; if ($files !== null) { - $davUrl .= '&files=' . $files; + $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)); } } diff --git a/apps/files_sharing/tests/Controller/ShareControllerTest.php b/apps/files_sharing/tests/Controller/ShareControllerTest.php index a8381e70fd7..393b5de898d 100644 --- a/apps/files_sharing/tests/Controller/ShareControllerTest.php +++ b/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); }