diff --git a/.github/workflows/behat-mariadb.yml b/.github/workflows/behat-mariadb.yml index 2dead08ac0..1c571e83ce 100644 --- a/.github/workflows/behat-mariadb.yml +++ b/.github/workflows/behat-mariadb.yml @@ -108,7 +108,7 @@ jobs: php-version: ${{ matrix.php-versions }} tools: phpunit # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation - extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, mysql, pdo_mysql + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, mysql, pdo_mysql, imagick coverage: none ini-file: development # Temporary workaround for missing pcntl_* in PHP 8.3 diff --git a/.github/workflows/behat-mysql.yml b/.github/workflows/behat-mysql.yml index 1494cd2c75..3befb63258 100644 --- a/.github/workflows/behat-mysql.yml +++ b/.github/workflows/behat-mysql.yml @@ -112,7 +112,7 @@ jobs: php-version: ${{ matrix.php-versions }} tools: phpunit # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation - extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite, imagick coverage: none ini-file: development # Temporary workaround for missing pcntl_* in PHP 8.3 diff --git a/.github/workflows/behat-pgsql.yml b/.github/workflows/behat-pgsql.yml index 2b7b2a2b92..e843ed7b60 100644 --- a/.github/workflows/behat-pgsql.yml +++ b/.github/workflows/behat-pgsql.yml @@ -111,7 +111,7 @@ jobs: php-version: ${{ matrix.php-versions }} tools: phpunit # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation - extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, pgsql, pdo_pgsql + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, pgsql, pdo_pgsql, imagick coverage: none ini-file: development # Temporary workaround for missing pcntl_* in PHP 8.3 diff --git a/.github/workflows/behat-sqlite.yml b/.github/workflows/behat-sqlite.yml index f3b0316263..aefff0f921 100644 --- a/.github/workflows/behat-sqlite.yml +++ b/.github/workflows/behat-sqlite.yml @@ -102,7 +102,7 @@ jobs: php-version: ${{ matrix.php-versions }} tools: phpunit # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation - extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite, imagick coverage: none ini-file: development # Temporary workaround for missing pcntl_* in PHP 8.3 diff --git a/.github/workflows/phpunit-mariadb.yml b/.github/workflows/phpunit-mariadb.yml index 33b007a0c8..d8c2a3e01a 100644 --- a/.github/workflows/phpunit-mariadb.yml +++ b/.github/workflows/phpunit-mariadb.yml @@ -113,7 +113,7 @@ jobs: with: php-version: ${{ matrix.php-versions }} # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation - extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, mysql, pdo_mysql + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, mysql, pdo_mysql, imagick coverage: xdebug ini-file: development # Temporary workaround for missing pcntl_* in PHP 8.3 diff --git a/.github/workflows/phpunit-mysql.yml b/.github/workflows/phpunit-mysql.yml index 1beecc3684..4062207116 100644 --- a/.github/workflows/phpunit-mysql.yml +++ b/.github/workflows/phpunit-mysql.yml @@ -111,7 +111,7 @@ jobs: with: php-version: ${{ matrix.php-versions }} # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation - extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, mysql, pdo_mysql + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, mysql, pdo_mysql, imagick coverage: xdebug ini-file: development # Temporary workaround for missing pcntl_* in PHP 8.3 diff --git a/.github/workflows/phpunit-pgsql.yml b/.github/workflows/phpunit-pgsql.yml index 50d8a7cb33..5394fb9545 100644 --- a/.github/workflows/phpunit-pgsql.yml +++ b/.github/workflows/phpunit-pgsql.yml @@ -114,7 +114,7 @@ jobs: with: php-version: ${{ matrix.php-versions }} # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation - extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, pgsql, pdo_pgsql + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, pgsql, pdo_pgsql, imagick coverage: xdebug ini-file: development # Temporary workaround for missing pcntl_* in PHP 8.3 diff --git a/.github/workflows/phpunit-sqlite.yml b/.github/workflows/phpunit-sqlite.yml index a53dbaeb11..33cfdea35e 100644 --- a/.github/workflows/phpunit-sqlite.yml +++ b/.github/workflows/phpunit-sqlite.yml @@ -110,7 +110,7 @@ jobs: with: php-version: ${{ matrix.php-versions }} # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation - extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite, imagick coverage: xdebug ini-file: development # Temporary workaround for missing pcntl_* in PHP 8.3 diff --git a/lib/Controller/FileController.php b/lib/Controller/FileController.php index 55101552fe..ecc3d707a5 100644 --- a/lib/Controller/FileController.php +++ b/lib/Controller/FileController.php @@ -18,6 +18,7 @@ use OCA\Libresign\Helper\JSActions; use OCA\Libresign\Helper\ValidateHelper; use OCA\Libresign\Middleware\Attribute\PrivateValidation; +use OCA\Libresign\Middleware\Attribute\RequireFileAccess; use OCA\Libresign\Middleware\Attribute\RequireManager; use OCA\Libresign\Service\AccountService; use OCA\Libresign\Service\File\FileListService; @@ -353,6 +354,7 @@ public function list( */ #[NoAdminRequired] #[NoCSRFRequired] + #[RequireFileAccess('nodeId')] #[ApiRoute(verb: 'GET', url: '/api/{apiVersion}/file/thumbnail/{nodeId}', requirements: ['apiVersion' => '(v1)'])] public function getThumbnail( int $nodeId = -1, @@ -369,9 +371,6 @@ public function getThumbnail( try { $libreSignFile = $this->fileMapper->getByNodeId($nodeId); - if ($libreSignFile->getUserId() !== $this->userSession->getUser()->getUID()) { - return new DataResponse([], Http::STATUS_FORBIDDEN); - } if ($libreSignFile->getNodeType() === 'envelope') { if ($mimeFallback) { @@ -411,6 +410,7 @@ public function getThumbnail( */ #[NoAdminRequired] #[NoCSRFRequired] + #[RequireFileAccess('fileId')] #[ApiRoute(verb: 'GET', url: '/api/{apiVersion}/file/thumbnail/file_id/{fileId}', requirements: ['apiVersion' => '(v1)'])] public function getThumbnailByFileId( int $fileId = -1, @@ -427,9 +427,6 @@ public function getThumbnailByFileId( try { $libreSignFile = $this->fileMapper->getById($fileId); - if ($libreSignFile->getUserId() !== $this->userSession->getUser()->getUID()) { - return new DataResponse([], Http::STATUS_FORBIDDEN); - } if ($libreSignFile->getNodeType() === 'envelope') { if ($mimeFallback) { diff --git a/lib/Middleware/Attribute/RequireFileAccess.php b/lib/Middleware/Attribute/RequireFileAccess.php new file mode 100644 index 0000000000..a047edcc18 --- /dev/null +++ b/lib/Middleware/Attribute/RequireFileAccess.php @@ -0,0 +1,23 @@ +identifier; + } +} diff --git a/lib/Middleware/InjectionMiddleware.php b/lib/Middleware/InjectionMiddleware.php index 9fc59fa415..1f0d9b58e9 100644 --- a/lib/Middleware/InjectionMiddleware.php +++ b/lib/Middleware/InjectionMiddleware.php @@ -22,11 +22,13 @@ use OCA\Libresign\Helper\ValidateHelper; use OCA\Libresign\Middleware\Attribute\CanSignRequestUuid; use OCA\Libresign\Middleware\Attribute\PrivateValidation; +use OCA\Libresign\Middleware\Attribute\RequireFileAccess; use OCA\Libresign\Middleware\Attribute\RequireManager; use OCA\Libresign\Middleware\Attribute\RequireSetupOk; use OCA\Libresign\Middleware\Attribute\RequireSigner; use OCA\Libresign\Middleware\Attribute\RequireSignerUuid; use OCA\Libresign\Middleware\Attribute\RequireSignRequestUuid; +use OCA\Libresign\Service\FileAccessService; use OCA\Libresign\Service\SignFileService; use OCA\Libresign\Service\UuidResolverService; use OCP\AppFramework\Controller; @@ -58,6 +60,7 @@ public function __construct( private CertificateEngineFactory $certificateEngineFactory, private FileMapper $fileMapper, private IInitialState $initialState, + private FileAccessService $fileAccessService, private SignFileService $signFileService, private UuidResolverService $uuidResolverService, private IL10N $l10n, @@ -93,6 +96,9 @@ public function beforeController(Controller $controller, string $methodName) { if (!empty($reflectionMethod->getAttributes(RequireSignerUuid::class))) { $this->requireSignerUuid(); } + if (!empty($reflectionMethod->getAttributes(RequireFileAccess::class))) { + $this->requireFileAccess($reflectionMethod); + } $this->requireSetupOk($reflectionMethod); @@ -232,6 +238,26 @@ private function requireSignerUuid(): void { } } + private function requireFileAccess(\ReflectionMethod $reflectionMethod): void { + $attributes = $reflectionMethod->getAttributes(RequireFileAccess::class); + $attribute = current($attributes); + /** @var RequireFileAccess $requirement */ + $requirement = $attribute->newInstance(); + + $identifier = $requirement->getIdentifier(); + $hasAccess = match ($identifier) { + 'nodeId' => $this->fileAccessService->userCanAccessFileByNodeId((int)$this->request->getParam('nodeId', -1)), + 'fileId' => $this->fileAccessService->userCanAccessFileById((int)$this->request->getParam('fileId', -1)), + default => throw new \InvalidArgumentException('Unsupported file access identifier: ' . $identifier), + }; + + if ($hasAccess) { + return; + } + + throw new LibresignException(json_encode([]), Http::STATUS_FORBIDDEN); + } + private function redirectSignedToValidationIfNeeded(RequireSignRequestUuid $requirement): void { if (!$requirement->redirectIfSignedToValidation()) { return; diff --git a/lib/Search/FileSearchProvider.php b/lib/Search/FileSearchProvider.php index c6678b34bc..ac43288a58 100644 --- a/lib/Search/FileSearchProvider.php +++ b/lib/Search/FileSearchProvider.php @@ -92,11 +92,12 @@ private function formatResult(File $file, IUser $user): SearchResultEntry { $icon = $this->mimeTypeDetector->mimeTypeIcon($node->getMimetype()); $thumbnailUrl = $this->urlGenerator->linkToRouteAbsolute( - 'core.Preview.getPreviewByFileId', + 'ocs.libresign.File.getThumbnailByFileId', [ + 'apiVersion' => 'v1', 'x' => 32, 'y' => 32, - 'fileId' => $node->getId() + 'fileId' => $file->getId() ] ); diff --git a/lib/Service/FileAccessService.php b/lib/Service/FileAccessService.php new file mode 100644 index 0000000000..cdbb1bfcb6 --- /dev/null +++ b/lib/Service/FileAccessService.php @@ -0,0 +1,62 @@ +resolveUser($user); + if (!$user) { + return false; + } + + return $this->userCanAccessFile($this->fileMapper->getById($fileId), $user); + } + + public function userCanAccessFileByNodeId(int $nodeId, ?IUser $user = null): bool { + $user = $this->resolveUser($user); + if (!$user) { + return false; + } + + return $this->userCanAccessFile($this->fileMapper->getByNodeId($nodeId), $user); + } + + private function resolveUser(?IUser $user): ?IUser { + return $user ?? $this->userSession->getUser(); + } + + private function userCanAccessFile(FileEntity $file, IUser $user): bool { + if ($file->getUserId() === $user->getUID()) { + return true; + } + + return $this->userCanSignFile($file, $user); + } + + private function userCanSignFile(FileEntity $file, IUser $user): bool { + try { + $this->signFileService->getSignRequestToSign($file, null, $user); + return true; + } catch (\Exception) { + return false; + } + } +} diff --git a/psalm.xml b/psalm.xml index 5fe94c6b98..d0fa2c9718 100644 --- a/psalm.xml +++ b/psalm.xml @@ -45,5 +45,6 @@ + diff --git a/src/components/File/File.vue b/src/components/File/File.vue index 296d3410f8..8afa71b006 100644 --- a/src/components/File/File.vue +++ b/src/components/File/File.vue @@ -61,14 +61,14 @@ const previewUrl = computed(() => { } let filePreviewUrl = '' - if (currentFile.value.nodeId) { - filePreviewUrl = generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/{nodeId}', { - nodeId: currentFile.value.nodeId, - }) - } else if (currentFile.value.id) { + if (currentFile.value.id) { filePreviewUrl = generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/file_id/{fileId}', { fileId: currentFile.value.id, }) + } else if (currentFile.value.nodeId) { + filePreviewUrl = generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/{nodeId}', { + nodeId: currentFile.value.nodeId, + }) } else { filePreviewUrl = window.location.origin + generateUrl('/core/preview?fileId={fileid}', { fileid: currentFile.value.id, diff --git a/src/components/RightSidebar/EnvelopeFilesList.vue b/src/components/RightSidebar/EnvelopeFilesList.vue index bcc125366a..dac9a3f02f 100644 --- a/src/components/RightSidebar/EnvelopeFilesList.vue +++ b/src/components/RightSidebar/EnvelopeFilesList.vue @@ -412,11 +412,17 @@ function validateMaxFileUploads(filesCount: number) { return true } -function getPreviewUrl(file: Partial & { nodeId?: number }) { - if (!file.nodeId) return null - const url = new URL(generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/{nodeId}', { - nodeId: file.nodeId, - })) +function getPreviewUrl(file: Partial & { id?: number; nodeId?: number }) { + if (!file.id && !file.nodeId) return null + + const previewUrl = file.id + ? generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/file_id/{fileId}', { + fileId: file.id, + }) + : generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/{nodeId}', { + nodeId: file.nodeId, + }) + const url = new URL(previewUrl) url.searchParams.set('x', '32') url.searchParams.set('y', '32') url.searchParams.set('mimeFallback', 'true') diff --git a/src/tests/components/File/File.spec.ts b/src/tests/components/File/File.spec.ts index 450387c3f9..4c86d252e3 100644 --- a/src/tests/components/File/File.spec.ts +++ b/src/tests/components/File/File.spec.ts @@ -9,7 +9,7 @@ import { mount } from '@vue/test-utils' import File from '../../../components/File/File.vue' type FileEntry = { - id: number + id?: number nodeId?: number name: string status: number @@ -83,23 +83,23 @@ describe('File.vue', () => { sidebarStoreMock.activeRequestSignatureTab.mockReset() }) - it('renders the selected file preview using the node id thumbnail endpoint', () => { + it('renders the selected file preview using the file id thumbnail endpoint', () => { const wrapper = createWrapper() const image = wrapper.find('img') expect(image.exists()).toBe(true) expect(wrapper.find('h1').text()).toBe('contract.pdf') - expect(image.attributes('src')).toContain('/apps/libresign/api/v1/file/thumbnail/99') + expect(image.attributes('src')).toContain('/apps/libresign/api/v1/file/thumbnail/file_id/13') expect(image.attributes('src')).toContain('x=128') expect(image.attributes('src')).toContain('y=128') expect(image.attributes('src')).toContain('mimeFallback=true') expect(image.attributes('src')).toContain('a=0') }) - it('falls back to the file id thumbnail endpoint when node id is absent', () => { + it('falls back to the node id thumbnail endpoint when the file id is absent', () => { filesStoreMock.files[7] = { - id: 13, + nodeId: 99, name: 'fallback.pdf', status: 1, statusText: 'Pending', @@ -107,7 +107,7 @@ describe('File.vue', () => { const wrapper = createWrapper() - expect(wrapper.find('img').attributes('src')).toContain('/apps/libresign/api/v1/file/thumbnail/file_id/13') + expect(wrapper.find('img').attributes('src')).toContain('/apps/libresign/api/v1/file/thumbnail/99') }) it('selects the file and opens the request signature sidebar on click', async () => { diff --git a/src/tests/components/RightSidebar/EnvelopeFilesList.spec.ts b/src/tests/components/RightSidebar/EnvelopeFilesList.spec.ts index 40d4226bd7..045755642b 100644 --- a/src/tests/components/RightSidebar/EnvelopeFilesList.spec.ts +++ b/src/tests/components/RightSidebar/EnvelopeFilesList.spec.ts @@ -26,7 +26,7 @@ vi.mock('@nextcloud/l10n', () => globalThis.mockNextcloudL10n()) vi.mock('@nextcloud/capabilities') vi.mock('@nextcloud/router', () => ({ - generateOcsUrl: vi.fn((url) => `https://example.com${url}`), + generateOcsUrl: vi.fn((url, params) => `https://example.com${url.replace('{fileId}', String(params?.fileId ?? '')).replace('{nodeId}', String(params?.nodeId ?? ''))}`), generateUrl: vi.fn((url, params) => url.replace('{uuid}', params.uuid).replace('{nodeId}', params.nodeId)), })) vi.mock('../../../utils/viewer.js', () => ({ @@ -364,18 +364,26 @@ describe('EnvelopeFilesList', () => { }) describe('RULE: getPreviewUrl constructs thumbnail URL with parameters', () => { - it('builds URL with nodeId and parameters', () => { + it('prefers file id URLs and appends preview parameters', () => { wrapper = createWrapper() - const url = wrapper.vm.getPreviewUrl({ nodeId: 123 }) + const url = wrapper.vm.getPreviewUrl({ id: 123, nodeId: 456 }) - expect(url).toContain('nodeId') + expect(url).toContain('/apps/libresign/api/v1/file/thumbnail/file_id/123') expect(url).toContain('x=32') expect(url).toContain('y=32') expect(url).toContain('mimeFallback=true') expect(url).toContain('a=1') }) + it('falls back to node id URLs when file id is absent', () => { + wrapper = createWrapper() + + const url = wrapper.vm.getPreviewUrl({ nodeId: 123 }) + + expect(url).toContain('/apps/libresign/api/v1/file/thumbnail/123') + }) + it('returns null when no nodeId', () => { wrapper = createWrapper() diff --git a/src/tests/views/FilesList/FileEntryPreview.spec.ts b/src/tests/views/FilesList/FileEntryPreview.spec.ts index 347f000a1b..c248f43bcd 100644 --- a/src/tests/views/FilesList/FileEntryPreview.spec.ts +++ b/src/tests/views/FilesList/FileEntryPreview.spec.ts @@ -56,16 +56,23 @@ describe('FileEntryPreview.vue', () => { expect(wrapper.vm.isEnvelope).toBe(true) }) - it('builds a thumbnail URL from the node id with list-size previews', () => { - const wrapper = createWrapper({ nodeId: 42 }) + it('prefers the file id thumbnail URL with list-size previews', () => { + const wrapper = createWrapper({ id: 7, nodeId: 42 }) const url = wrapper.vm.previewUrl as URL - expect(url.toString()).toContain('/apps/libresign/api/v1/file/thumbnail/42') + expect(url.toString()).toContain('/apps/libresign/api/v1/file/thumbnail/file_id/7') expect(url.searchParams.get('x')).toBe('32') expect(url.searchParams.get('y')).toBe('32') expect(url.searchParams.get('a')).toBe('1') }) + it('falls back to the node id thumbnail URL when the file id is absent', () => { + const wrapper = createWrapper({ nodeId: 42 }) + const url = wrapper.vm.previewUrl as URL + + expect(url.toString()).toContain('/apps/libresign/api/v1/file/thumbnail/42') + }) + it('uses grid preview sizes when the user config enables grid mode', () => { userConfigStoreMock.files_list_grid_view = true const wrapper = createWrapper({ id: 7 }) diff --git a/src/views/FilesList/FileEntry/FileEntryPreview.vue b/src/views/FilesList/FileEntry/FileEntryPreview.vue index 7ebd9b512e..8ea21ffd61 100644 --- a/src/views/FilesList/FileEntry/FileEntryPreview.vue +++ b/src/views/FilesList/FileEntry/FileEntryPreview.vue @@ -65,14 +65,14 @@ const previewUrl = computed(() => { } let nextPreviewUrl = '' - if (props.source?.nodeId) { - nextPreviewUrl = generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/{nodeId}', { - nodeId: props.source.nodeId, - }) - } else if (props.source?.id) { + if (props.source?.id) { nextPreviewUrl = generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/file_id/{fileId}', { fileId: props.source.id, }) + } else if (props.source?.nodeId) { + nextPreviewUrl = generateOcsUrl('/apps/libresign/api/v1/file/thumbnail/{nodeId}', { + nodeId: props.source.nodeId, + }) } else { nextPreviewUrl = window.location.origin + generateUrl('/core/preview?fileId={fileid}', { fileid: props.source.nodeId, diff --git a/tests/integration/features/file/thumbnail.feature b/tests/integration/features/file/thumbnail.feature new file mode 100644 index 0000000000..4a40958f81 --- /dev/null +++ b/tests/integration/features/file/thumbnail.feature @@ -0,0 +1,28 @@ +Feature: file-thumbnail + Scenario: Thumbnail endpoint access by file_id should enforce signer authorization + Given as user "admin" + And user "signer1" exists + And user "nonsigner" exists + And sending "post" to ocs "/apps/provisioning_api/api/v1/config/apps/libresign/identify_methods" + | value | (string)[{"name":"account","enabled":true,"mandatory":true}] | + And sending "post" to ocs "/apps/libresign/api/v1/request-signature" + | file | {"url":"/apps/libresign/develop/pdf"} | + | signers | [{"identifyMethods":[{"method":"account","value":"signer1"}]}] | + | name | document | + And the response should have a status code 200 + And sending "get" to ocs "/apps/libresign/api/v1/file/list?details=1" + And fetch field "(FILE_ID)ocs.data.data.0.id" from previous JSON response + And as user "nonsigner" + When sending "get" to ocs "/apps/libresign/api/v1/file/thumbnail/file_id/" + Then the response should have a status code 403 + # x=0 triggers a deterministic 400 in controller for authorized users, + # while unauthorized users are blocked earlier by middleware with 403. + And as user "admin" + When sending "get" to ocs "/apps/libresign/api/v1/file/thumbnail/file_id/?x=0" + Then the response should have a status code 400 + And as user "signer1" + When sending "get" to ocs "/apps/libresign/api/v1/file/thumbnail/file_id/?x=0" + Then the response should have a status code 400 + And as user "nonsigner" + When sending "get" to ocs "/apps/libresign/api/v1/file/thumbnail/file_id/?x=0" + Then the response should have a status code 403 diff --git a/tests/php/Api/Controller/FileControllerTest.php b/tests/php/Api/Controller/FileControllerTest.php index 2e752d4f9e..04ca43c4de 100644 --- a/tests/php/Api/Controller/FileControllerTest.php +++ b/tests/php/Api/Controller/FileControllerTest.php @@ -16,6 +16,7 @@ * @group DB */ final class FileControllerTest extends ApiTestCase { + /** * @runInSeparateProcess */ @@ -159,4 +160,68 @@ public function testSendNewFile():void { $this->assertRequest(); } + + /** + * @runInSeparateProcess + */ + public function testGetThumbnailByNonSigner(): void { + $owner = $this->createAccount('owner', 'password'); + $this->createAccount('signer', 'password'); + $this->createAccount('nonsigner', 'password'); + $this->getMockAppConfig(); + + $file = $this->requestSignFile([ + 'file' => ['base64' => base64_encode(file_get_contents(__DIR__ . '/../../fixtures/pdfs/small_valid.pdf'))], + 'name' => 'test.pdf', + 'signers' => [[ + 'identifyMethods' => [[ + 'method' => 'account', + 'mandatory' => 0, + 'value' => 'signer', + ]], + ]], + 'userManager' => $owner, + ]); + + $this->request + ->withRequestHeader([ + 'Authorization' => 'Basic ' . base64_encode('nonsigner:password'), + ]) + ->withPath('/api/v1/file/thumbnail/file_id/' . $file->getId()) + ->assertResponseCode(403); + + $this->assertRequest(); + } + + /** + * @runInSeparateProcess + */ + public function testGetThumbnailByNodeIdForNonSigner(): void { + $owner = $this->createAccount('owner', 'password'); + $this->createAccount('signer', 'password'); + $this->createAccount('nonsigner', 'password'); + $this->getMockAppConfig(); + + $file = $this->requestSignFile([ + 'file' => ['base64' => base64_encode(file_get_contents(__DIR__ . '/../../fixtures/pdfs/small_valid.pdf'))], + 'name' => 'test.pdf', + 'signers' => [[ + 'identifyMethods' => [[ + 'method' => 'account', + 'mandatory' => 0, + 'value' => 'signer', + ]], + ]], + 'userManager' => $owner, + ]); + + $this->request + ->withRequestHeader([ + 'Authorization' => 'Basic ' . base64_encode('nonsigner:password'), + ]) + ->withPath('/api/v1/file/thumbnail/' . $file->getNodeId()) + ->assertResponseCode(403); + + $this->assertRequest(); + } } diff --git a/tests/php/Unit/Middleware/InjectionMiddlewareTest.php b/tests/php/Unit/Middleware/InjectionMiddlewareTest.php index 3ae14dc650..900b5adfa2 100644 --- a/tests/php/Unit/Middleware/InjectionMiddlewareTest.php +++ b/tests/php/Unit/Middleware/InjectionMiddlewareTest.php @@ -15,6 +15,7 @@ use OCA\Libresign\Handler\CertificateEngine\CertificateEngineFactory; use OCA\Libresign\Helper\ValidateHelper; use OCA\Libresign\Middleware\InjectionMiddleware; +use OCA\Libresign\Service\FileAccessService; use OCA\Libresign\Service\SignFileService; use OCA\Libresign\Service\UuidResolverService; use OCP\AppFramework\Controller; @@ -46,11 +47,12 @@ final class InjectionMiddlewareTest extends \OCA\Libresign\Tests\Unit\TestCase { private CertificateEngineFactory $certificateEngineFactory; private FileMapper&MockObject $fileMapper; private IInitialState $initialState; + private FileAccessService&MockObject $fileAccessService; private SignFileService&MockObject $signFileService; private UuidResolverService&MockObject $uuidResolverService; private IL10N&MockObject $l10n; - private IappConfig&MockObject $appConfig; - private IurlGenerator&MockObject $urlGenerator; + private IAppConfig&MockObject $appConfig; + private IURLGenerator&MockObject $urlGenerator; private ?string $userId = null; private InitialStateService $initialStateService; @@ -72,6 +74,7 @@ public function setUp(): void { $this->createMock(IServerContainer::class) ); $this->initialState = new InitialState($this->initialStateService, 'libresign'); + $this->fileAccessService = $this->createMock(FileAccessService::class); $this->signFileService = $this->createMock(SignFileService::class); $this->uuidResolverService = $this->createMock(UuidResolverService::class); $this->l10n = $this->createMock(IL10N::class); @@ -88,6 +91,7 @@ public function getInjectionMiddleware(): InjectionMiddleware { $this->certificateEngineFactory, $this->fileMapper, $this->initialState, + $this->fileAccessService, $this->signFileService, $this->uuidResolverService, $this->l10n, diff --git a/tests/php/Unit/Service/FileAccessServiceTest.php b/tests/php/Unit/Service/FileAccessServiceTest.php new file mode 100644 index 0000000000..02643748b2 --- /dev/null +++ b/tests/php/Unit/Service/FileAccessServiceTest.php @@ -0,0 +1,115 @@ +fileMapper = $this->createMock(FileMapper::class); + $this->signFileService = $this->createMock(SignFileService::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->service = new FileAccessService( + $this->fileMapper, + $this->signFileService, + $this->userSession, + ); + } + + #[DataProvider('provideFileAccessScenarios')] + public function testUserCanAccessFileUsesOwnershipAndSignerRules( + string $method, + string $mapperMethod, + int $identifier, + string $ownerId, + string $userId, + bool $canSign, + bool $expected, + ): void { + $user = $this->mockUser($userId); + $file = $this->createFileEntity(ownerId: $ownerId); + + $this->fileMapper->expects($this->once()) + ->method($mapperMethod) + ->with($identifier) + ->willReturn($file); + + $expectsSignerLookup = $ownerId !== $userId; + if (!$expectsSignerLookup) { + $this->signFileService->expects($this->never())->method('getSignRequestToSign'); + } elseif ($canSign) { + $this->signFileService->expects($this->once()) + ->method('getSignRequestToSign') + ->with($file, null, $user); + } else { + $this->signFileService->expects($this->once()) + ->method('getSignRequestToSign') + ->with($file, null, $user) + ->willThrowException(new \RuntimeException('not a signer')); + } + + $this->assertSame($expected, $this->service->{$method}($identifier, $user)); + } + + #[DataProvider('provideMissingUserScenarios')] + public function testUserCanAccessFileReturnsFalseWithoutResolvedUser( + string $method, + string $mapperMethod, + int $identifier, + ): void { + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn(null); + $this->fileMapper->expects($this->never())->method($mapperMethod); + + $this->assertFalse($this->service->{$method}($identifier)); + } + + public static function provideFileAccessScenarios(): array { + return [ + 'owner by file id' => ['userCanAccessFileById', 'getById', 10, 'owner', 'owner', false, true], + 'signer by file id' => ['userCanAccessFileById', 'getById', 20, 'owner', 'signer', true, true], + 'outsider by file id' => ['userCanAccessFileById', 'getById', 30, 'owner', 'outsider', false, false], + 'signer by node id' => ['userCanAccessFileByNodeId', 'getByNodeId', 99, 'owner', 'signer', true, true], + 'outsider by node id' => ['userCanAccessFileByNodeId', 'getByNodeId', 100, 'owner', 'outsider', false, false], + ]; + } + + public static function provideMissingUserScenarios(): array { + return [ + 'file id without user' => ['userCanAccessFileById', 'getById', 40], + 'node id without user' => ['userCanAccessFileByNodeId', 'getByNodeId', 41], + ]; + } + + private function mockUser(string $uid): IUser&MockObject { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + return $user; + } + + private function createFileEntity(string $ownerId): FileEntity { + $file = new FileEntity(); + $file->setUserId($ownerId); + return $file; + } +} diff --git a/tests/stubs/psr_http_client.php b/tests/stubs/psr_http_client.php new file mode 100644 index 0000000000..6b82b9157a --- /dev/null +++ b/tests/stubs/psr_http_client.php @@ -0,0 +1,15 @@ +