diff --git a/lib/Controller/FileController.php b/lib/Controller/FileController.php index ad3f0fec9a..2cfc25f5f0 100644 --- a/lib/Controller/FileController.php +++ b/lib/Controller/FileController.php @@ -27,6 +27,7 @@ use OCA\Libresign\Service\Policy\ValidationEffectivePolicyService; use OCA\Libresign\Service\RequestSignatureService; use OCA\Libresign\Service\SessionService; +use OCA\Libresign\Service\Validation\FileInputValidator; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\ApiRoute; @@ -84,6 +85,7 @@ public function __construct( private ValidateHelper $validateHelper, private SettingsLoader $settingsLoader, private IURLGenerator $urlGenerator, + private FileInputValidator $fileInputValidator, ) { parent::__construct(Application::APP_ID, $request); } @@ -742,12 +744,18 @@ private function prepareFilesForSaving(array $file, array $files, array $setting } if (!empty($files)) { - /** @var list $files */ - return $files; + /** @var list $normalizedFiles */ + $normalizedFiles = array_map( + fn (mixed $each): mixed => is_array($each) ? $this->fileInputValidator->normalizeNodeId($each) : $each, + $files, + ); + return $normalizedFiles; } if (!empty($file)) { - return [$file]; + /** @var array{fileNode?: Node, name?: string} $normalizedFile */ + $normalizedFile = $this->fileInputValidator->normalizeNodeId($file); + return [$normalizedFile]; } // TRANSLATORS Error shown when creating or updating a signature request without a file. diff --git a/lib/Controller/RequestSignatureController.php b/lib/Controller/RequestSignatureController.php index c46e0aa692..952f0c3e39 100644 --- a/lib/Controller/RequestSignatureController.php +++ b/lib/Controller/RequestSignatureController.php @@ -62,7 +62,7 @@ public function __construct( * @param LibresignNewSigner[] $signers Collection of signers who must sign the document. Use identifyMethods as the canonical format. Other supported fields: displayName, description, notify, signingOrder, status, geolocationRequired * @param string $name The name of file to sign * @param LibresignFolderSettings $settings Settings to define how and where the file should be stored - * @param LibresignNewFile $file File object. Supports nodeId, url, base64 or path. + * @param LibresignNewFile $file File object. Supports nodeId (a non-negative integer or its canonical decimal string, as Nextcloud node ids can exceed a JavaScript number), url, base64 or path. * @param list $files Multiple files to create an envelope (optional, use either file or files). Each file supports nodeId, url, base64 or path. * @param string|null $callback URL that will receive a POST after the document is signed * @param integer|null $status Numeric code of status * 0 - no signers * 1 - signed * 2 - pending @@ -134,7 +134,7 @@ public function requestSignature( * @param LibresignNewSigner[]|null $signers Collection of signers who must sign the document. Use identifyMethods as the canonical format. * @param string|null $uuid UUID of sign request. The signer UUID is what the person receives via email when asked to sign. This is not the file UUID. * @param LibresignVisibleElement[]|null $visibleElements Visible elements on document - * @param LibresignNewFile|null $file File object. Supports nodeId, url, base64 or path when creating a new request. + * @param LibresignNewFile|null $file File object. Supports nodeId (a non-negative integer or its canonical decimal string), url, base64 or path when creating a new request. * @param integer|null $status Numeric code of status * 0 - no signers * 1 - signed * 2 - pending * @param array|null $policy Structured policy payload with request-level overrides and active context. * @param string|null $name The name of file to sign diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index 7bda4b0a0e..a021e58731 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -72,7 +72,7 @@ * } * @psalm-type LibresignNewFile = array{ * base64?: string, - * nodeId?: non-negative-int, + * nodeId?: non-negative-int|numeric-string, * path?: string, * url?: string, * name?: string, diff --git a/lib/Service/IdDocsService.php b/lib/Service/IdDocsService.php index 95e0354839..88c2de7d43 100644 --- a/lib/Service/IdDocsService.php +++ b/lib/Service/IdDocsService.php @@ -82,6 +82,28 @@ private function validateIdDoc(int $fileIndex, array $file, IUser $user): void { } } + /** + * `file` of each entry is the HTTP `LibresignNewFile` payload; its node + * id is normalized once here, the same boundary the signature request has. + */ + private function normalizeNodeIds(array $files): array { + foreach ($files as $fileIndex => $fileData) { + if (!is_array($fileData) || !is_array($fileData['file'] ?? null)) { + continue; + } + try { + $files[$fileIndex]['file'] = $this->fileInputValidator->normalizeNodeId($fileData['file'], FileInputValidator::TYPE_ACCOUNT_DOCUMENT); + } catch (LibresignException $e) { + throw new LibresignException(json_encode([ + 'type' => 'danger', + 'file' => $fileIndex, + 'message' => $e->getMessage(), + ])); + } + } + return $files; + } + public function validateIdDocs(array $files, IUser $user): void { foreach ($files as $fileIndex => $file) { $this->validateTypeOfFile($fileIndex, $file); @@ -90,6 +112,7 @@ public function validateIdDocs(array $files, IUser $user): void { } public function addIdDocs(array $files, IUser $user): void { + $files = $this->normalizeNodeIds($files); $this->validateIdDocs($files, $user); foreach ($files as $fileData) { $dataToSave = $fileData; @@ -119,6 +142,7 @@ public function addFilesToDocumentFolder( array $files, SignRequest $signRequest, ): void { + $files = $this->normalizeNodeIds($files); foreach ($files as $fileIndex => $file) { $this->validateTypeOfFile($fileIndex, $file); } diff --git a/lib/Service/RequestSignatureService.php b/lib/Service/RequestSignatureService.php index 88b465e600..2ad694cc09 100644 --- a/lib/Service/RequestSignatureService.php +++ b/lib/Service/RequestSignatureService.php @@ -361,15 +361,15 @@ public function saveFile(array $data): FileEntity { } return $this->fileStatusService->updateFileStatusIfUpgrade($file, $data['status'] ?? 0); } - $fileId = null; + $nodeId = null; if (isset($data['file']['fileNode']) && $data['file']['fileNode'] instanceof Node) { - $fileId = $data['file']['fileNode']->getId(); + $nodeId = $data['file']['fileNode']->getId(); } elseif (!empty($data['file']['nodeId'])) { - $fileId = $data['file']['nodeId']; + $nodeId = $data['file']['nodeId']; } - if (!is_null($fileId)) { + if (!is_null($nodeId)) { try { - $file = $this->fileMapper->getByNodeId($fileId); + $file = $this->fileMapper->getByNodeId($nodeId); $this->filePolicyApplier->syncAllPolicies($file, $data); return $this->fileStatusService->updateFileStatusIfUpgrade($file, $data['status'] ?? 0); } catch (\Throwable) { diff --git a/lib/Service/RequestSignatureWorkflowService.php b/lib/Service/RequestSignatureWorkflowService.php index 6c5096ae76..cf65e87e82 100644 --- a/lib/Service/RequestSignatureWorkflowService.php +++ b/lib/Service/RequestSignatureWorkflowService.php @@ -26,6 +26,7 @@ public function __construct( private SignerValidator $signerValidator, private VisibleElementValidator $visibleElementValidator, private FileMapper $fileMapper, + private FileInputValidator $fileInputValidator, ) { } @@ -67,6 +68,8 @@ public function createRequest( throw new LibresignException($this->l10n->t('File or files parameter is required')); } + $file = $this->fileInputValidator->normalizeNodeId($file); + $files = $this->normalizeNodeIds($files); $resolvedPolicy = $this->resolvePolicyPayload($policy); $data = [ 'file' => $file, @@ -128,6 +131,7 @@ public function updateExistingRequest( ?string $name = null, array $settings = [], ): array { + $file = $this->fileInputValidator->normalizeNodeId($file); $resolvedPolicy = $this->resolvePolicyPayload($policy); $data = [ 'uuid' => $uuid, @@ -159,6 +163,17 @@ public function updateExistingRequest( ]; } + /** + * @param list> $files + * @return list> + */ + private function normalizeNodeIds(array $files): array { + return array_map( + fn (mixed $file): mixed => is_array($file) ? $this->fileInputValidator->normalizeNodeId($file) : $file, + $files, + ); + } + /** @return list */ private function loadChildFilesIfEnvelope(FileEntity $fileEntity): array { return $fileEntity->getParentFileId() === null || $fileEntity->isEnvelope() diff --git a/lib/Service/Validation/FileInputValidator.php b/lib/Service/Validation/FileInputValidator.php index 02c76ae253..691eba7012 100644 --- a/lib/Service/Validation/FileInputValidator.php +++ b/lib/Service/Validation/FileInputValidator.php @@ -38,6 +38,38 @@ public function __construct( ) { } + /** + * Normalize the node id of a file payload received over HTTP. + * + * The API accepts `nodeId` as a non-negative int or as its canonical + * decimal string (digits only, no sign, no leading zeros, within the int + * range): the Files app exposes node ids as strings (`Node.id` of + * `@nextcloud/files`) and, since Nextcloud 33, they can exceed what a + * JavaScript number holds. Call it once at the boundary: past it, + * `nodeId` is either absent or the non-negative `int` the Nextcloud Files + * API works with, and anything else is rejected here instead of reaching + * a later cast. + * + * @param array $file + * @return array + * @throws LibresignException when `nodeId` is present and is neither a non-negative int nor its canonical decimal string + */ + public function normalizeNodeId(array $file, int $type = self::TYPE_TO_SIGN): array { + $nodeId = $file['nodeId'] ?? null; + if ($nodeId === null) { + return $file; + } + if (is_string($nodeId) && ctype_digit($nodeId)) { + // FILTER_VALIDATE_INT also rejects leading zeros and overflow. + $nodeId = filter_var($nodeId, FILTER_VALIDATE_INT); + } + if (is_int($nodeId) && $nodeId >= 0) { + $file['nodeId'] = $nodeId; + return $file; + } + throw new LibresignException($this->l10n->t('File type: %s. Invalid fileID.', [$this->getTypeOfFile($type)])); + } + public function validateNewFile(array $data, int $type = self::TYPE_TO_SIGN, ?IUser $user = null): void { $this->validateFile($data, $type, $user); if (!empty($data['file']['nodeId'])) { diff --git a/openapi-full.json b/openapi-full.json index b63f319c65..c15632786a 100644 --- a/openapi-full.json +++ b/openapi-full.json @@ -2119,9 +2119,16 @@ "type": "string" }, "nodeId": { - "type": "integer", - "format": "int64", - "minimum": 0 + "oneOf": [ + { + "type": "integer", + "format": "int64", + "minimum": 0 + }, + { + "type": "string" + } + ] }, "path": { "type": "string" @@ -10146,7 +10153,7 @@ "file": { "$ref": "#/components/schemas/NewFile", "default": [], - "description": "File object. Supports nodeId, url, base64 or path." + "description": "File object. Supports nodeId (a non-negative integer or its canonical decimal string, as Nextcloud node ids can exceed a JavaScript number), url, base64 or path." }, "files": { "type": "array", @@ -10322,7 +10329,7 @@ "file": { "$ref": "#/components/schemas/NewFile", "nullable": true, - "description": "File object. Supports nodeId, url, base64 or path when creating a new request." + "description": "File object. Supports nodeId (a non-negative integer or its canonical decimal string), url, base64 or path when creating a new request." }, "status": { "type": "integer", diff --git a/openapi.json b/openapi.json index ee97f68c77..687a58df74 100644 --- a/openapi.json +++ b/openapi.json @@ -1583,9 +1583,16 @@ "type": "string" }, "nodeId": { - "type": "integer", - "format": "int64", - "minimum": 0 + "oneOf": [ + { + "type": "integer", + "format": "int64", + "minimum": 0 + }, + { + "type": "string" + } + ] }, "path": { "type": "string" @@ -9444,7 +9451,7 @@ "file": { "$ref": "#/components/schemas/NewFile", "default": [], - "description": "File object. Supports nodeId, url, base64 or path." + "description": "File object. Supports nodeId (a non-negative integer or its canonical decimal string, as Nextcloud node ids can exceed a JavaScript number), url, base64 or path." }, "files": { "type": "array", @@ -9620,7 +9627,7 @@ "file": { "$ref": "#/components/schemas/NewFile", "nullable": true, - "description": "File object. Supports nodeId, url, base64 or path when creating a new request." + "description": "File object. Supports nodeId (a non-negative integer or its canonical decimal string), url, base64 or path when creating a new request." }, "status": { "type": "integer", diff --git a/src/components/RightSidebar/AppFilesTab.vue b/src/components/RightSidebar/AppFilesTab.vue index 0e716b7c42..f0072f0e66 100644 --- a/src/components/RightSidebar/AppFilesTab.vue +++ b/src/components/RightSidebar/AppFilesTab.vue @@ -47,7 +47,9 @@ type PendingEnvelope = { } type FileInfo = { - id: number + // Nextcloud node id as tab.ts sends it: the numeric `fileid` when the node + // has one, otherwise the string `Node.id` of `@nextcloud/files`. + id: number | string type?: string name?: string path?: string diff --git a/src/store/files.js b/src/store/files.js index 1784b9e1d8..8736bf8bd0 100644 --- a/src/store/files.js +++ b/src/store/files.js @@ -961,6 +961,24 @@ const _filesStore = defineStore('files', () => { .filter((signer) => signer && signer.identifyMethods?.length) } + /** + * Whether a value identifies a Nextcloud node. Besides the historical + * positive number, `@nextcloud/files` exposes `Node.id` as a string and + * that is what the Files sidebar hands to AppFilesTab (#8363). The string + * is kept as is: node ids are 64-bit and converting them with Number() + * could change the value above Number.MAX_SAFE_INTEGER. The API accepts + * both representations. + * + * @param {unknown} value + * @return {value is number | string} + */ + function isNodeId(value) { + if (typeof value === 'number') { + return Number.isInteger(value) && value > 0 + } + return typeof value === 'string' && /^[1-9][0-9]*$/.test(value) + } + /** @param {EditableFileReferenceDraft | ApiFileRecord | EditableFileDraft | string | null | undefined} file */ function serializeRequestFile(file, { preferNodeId = false } = {}) { if (typeof file === 'string') { @@ -972,7 +990,7 @@ const _filesStore = defineStore('files', () => { if (typeof file.path === 'string' && file.path.length > 0) { return { path: file.path } } - if (preferNodeId && typeof file.nodeId === 'number' && file.nodeId > 0) { + if (preferNodeId && isNodeId(file.nodeId)) { return { nodeId: file.nodeId } } if (typeof file.fileId === 'number' && file.fileId > 0) { @@ -983,7 +1001,7 @@ const _filesStore = defineStore('files', () => { return { fileId: file.id } } } - if (typeof file.nodeId === 'number' && file.nodeId > 0) { + if (isNodeId(file.nodeId)) { return { nodeId: file.nodeId } } if (typeof file.url === 'string' && file.url.length > 0) { diff --git a/src/tests/components/RightSidebar/AppFilesTab.spec.ts b/src/tests/components/RightSidebar/AppFilesTab.spec.ts index 4c2c7402b2..7f154d5828 100644 --- a/src/tests/components/RightSidebar/AppFilesTab.spec.ts +++ b/src/tests/components/RightSidebar/AppFilesTab.spec.ts @@ -72,7 +72,7 @@ type TitleObserver = { } type FileInfo = { - id: number + id: number | string type?: string name?: string path?: string @@ -286,6 +286,28 @@ describe('AppFilesTab', () => { expect(sidebarStore.activeRequestSignatureTab).toHaveBeenCalled() }) + it('passes a string node id through unchanged when adding the file (#8363)', async () => { + filesStore.selectFileByNodeId = vi.fn().mockResolvedValue(null) + filesStore.addFile = vi.fn() + filesStore.selectFile = vi.fn() + sidebarStore.activeRequestSignatureTab = vi.fn() + wrapper = createWrapper() + + // tab.ts sends `Node.id` (a string) when the node has no numeric fileid + await wrapper.vm.update({ + id: '9007199254740993', + name: 'copy of contract.pdf', + path: '/Documents', + }) + + expect(filesStore.selectFileByNodeId).toHaveBeenCalledWith('9007199254740993') + expect(filesStore.addFile).toHaveBeenCalledWith(expect.objectContaining({ + nodeId: '9007199254740993', + name: 'copy of contract.pdf', + })) + expect(sidebarStore.activeRequestSignatureTab).toHaveBeenCalled() + }) + it('returns early when pending envelope processed', async () => { window.OCA = { Libresign: { diff --git a/src/tests/store/files.spec.ts b/src/tests/store/files.spec.ts index e5eb4d12fe..d110cdd1c4 100644 --- a/src/tests/store/files.spec.ts +++ b/src/tests/store/files.spec.ts @@ -1522,6 +1522,79 @@ describe('files store - critical business rules', () => { expect(config.data.file).toEqual({ nodeId }) }) + /** + * Regression #8363: `@nextcloud/files` exposes `Node.id` as a string and + * the Files sidebar hands it to AppFilesTab as is (a file copied in the + * Files app, or any node id above Number.MAX_SAFE_INTEGER). The store + * keeps it as nodeId; the request must carry it unchanged instead of + * dropping the whole "file" (422 "File or files parameter is required"). + */ + it('includes file.nodeId as the string the Files sidebar provided', async () => { + const store = useFilesStore() + const nodeId = '9007199254740993' + const tempId = -Number(nodeId) + store.files[tempId] = { + id: tempId, + nodeId, + name: 'copy of contract.pdf', + signers: [{ email: 'signer@example.com', identifyMethods: [{ method: 'email', value: 'signer@example.com', requirement: 'optional' }] }], + signatureFlow: 'parallel', + } + store.selectedFileId = tempId + + axiosMock.mockResolvedValue({ + data: { ocs: { data: { id: 77, nodeId: 77, signatureFlow: 'parallel', signers: [] } } }, + }) + + await store.saveOrUpdateSignatureRequest({}) + + const config = axiosMock.mock.calls[0][0] + expect(config.data.file).toEqual({ nodeId: '9007199254740993' }) + }) + + it('serializes envelope files with string and number node ids as they are', async () => { + const store = useFilesStore() + store.selectedFileId = -1 + store.files[-1] = { + id: -1, + name: 'Envelope', + files: [ + { id: -7, nodeId: '9007199254740993', name: 'first.pdf' }, + { id: -22, nodeId: 22, name: 'second.pdf' }, + ], + signers: [{ email: 'signer@example.com' }], + signatureFlow: 'parallel', + } + axiosMock.mockResolvedValue({ + data: { ocs: { data: { id: 12, nodeId: 'real-node', signatureFlow: 'parallel', signers: [] } } }, + }) + + await store.saveOrUpdateSignatureRequest({}) + + const config = axiosMock.mock.calls[0][0] + expect(config.data.files).toEqual([{ nodeId: '9007199254740993' }, { nodeId: 22 }]) + }) + + it('does not send the empty node id tab.ts falls back to when the node has none', async () => { + const store = useFilesStore() + store.files[-1] = { + id: -1, + nodeId: '', + name: 'unknown.pdf', + signers: [{ email: 'signer@example.com' }], + signatureFlow: 'parallel', + } + store.selectedFileId = -1 + axiosMock.mockResolvedValue({ + data: { ocs: { data: { id: 12, nodeId: 12, signatureFlow: 'parallel', signers: [] } } }, + }) + + await store.saveOrUpdateSignatureRequest({}) + + const config = axiosMock.mock.calls[0][0] + expect(config.data.file).toBeNull() + }) + it('serializes envelope files with nodeId-based references for creation flows', async () => { const store = useFilesStore() store.selectedFileId = -1 diff --git a/src/tests/tab.spec.ts b/src/tests/tab.spec.ts index 1e033964ef..08886d1ea0 100644 --- a/src/tests/tab.spec.ts +++ b/src/tests/tab.spec.ts @@ -137,6 +137,33 @@ describe('tab.ts', () => { }) }) + it('enabled() keeps the string node id of a PDF whose node has no numeric fileid', async () => { + await loadTabModule('complete') + const tabConfig = getRegisteredTabConfig<{ + enabled: (context: { node: Record }) => boolean + }>() + + // `@nextcloud/files` Node: `id` is always a string and `fileid` is + // undefined when the id does not fit a JavaScript number (#8363). + const enabled = tabConfig.enabled({ + node: { + id: '9007199254740993', + fileid: undefined, + basename: 'copy of contract.pdf', + dirname: '/Documents', + type: 'file', + mime: 'application/pdf', + }, + }) + + expect(enabled).toBe(true) + expect(window.OCA.Libresign.fileInfo).toMatchObject({ + id: '9007199254740993', + name: 'copy of contract.pdf', + path: '/Documents', + }) + }) + it('lazy mounts Vue only when custom element is connected and unmounts on disconnect', async () => { await loadTabModule('complete') diff --git a/src/types/openapi/openapi-full.ts b/src/types/openapi/openapi-full.ts index 82300ace97..4d5be161ec 100644 --- a/src/types/openapi/openapi-full.ts +++ b/src/types/openapi/openapi-full.ts @@ -1780,8 +1780,7 @@ export type components = { }; NewFile: { base64?: string; - /** Format: int64 */ - nodeId?: number; + nodeId?: number | string; path?: string; url?: string; name?: string; @@ -4878,7 +4877,7 @@ export interface operations { */ settings?: components["schemas"]["FolderSettings"]; /** - * @description File object. Supports nodeId, url, base64 or path. + * @description File object. Supports nodeId (a non-negative integer or its canonical decimal string, as Nextcloud node ids can exceed a JavaScript number), url, base64 or path. * @default [] */ file?: components["schemas"]["NewFile"]; @@ -4957,7 +4956,7 @@ export interface operations { uuid?: string | null; /** @description Visible elements on document */ visibleElements?: components["schemas"]["VisibleElement"][] | null; - /** @description File object. Supports nodeId, url, base64 or path when creating a new request. */ + /** @description File object. Supports nodeId (a non-negative integer or its canonical decimal string), url, base64 or path when creating a new request. */ file?: components["schemas"]["NewFile"]; /** * Format: int64 diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index eea99c2963..fadaab3a8a 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1360,8 +1360,7 @@ export type components = { }; NewFile: { base64?: string; - /** Format: int64 */ - nodeId?: number; + nodeId?: number | string; path?: string; url?: string; name?: string; @@ -4417,7 +4416,7 @@ export interface operations { */ settings?: components["schemas"]["FolderSettings"]; /** - * @description File object. Supports nodeId, url, base64 or path. + * @description File object. Supports nodeId (a non-negative integer or its canonical decimal string, as Nextcloud node ids can exceed a JavaScript number), url, base64 or path. * @default [] */ file?: components["schemas"]["NewFile"]; @@ -4496,7 +4495,7 @@ export interface operations { uuid?: string | null; /** @description Visible elements on document */ visibleElements?: components["schemas"]["VisibleElement"][] | null; - /** @description File object. Supports nodeId, url, base64 or path when creating a new request. */ + /** @description File object. Supports nodeId (a non-negative integer or its canonical decimal string), url, base64 or path when creating a new request. */ file?: components["schemas"]["NewFile"]; /** * Format: int64 diff --git a/tests/php/Unit/Controller/RequestSignatureControllerTest.php b/tests/php/Unit/Controller/RequestSignatureControllerTest.php index 60b436ceaa..605de17bc9 100644 --- a/tests/php/Unit/Controller/RequestSignatureControllerTest.php +++ b/tests/php/Unit/Controller/RequestSignatureControllerTest.php @@ -12,14 +12,18 @@ use OCA\Libresign\Controller\RequestSignatureController; use OCA\Libresign\Db\File as FileEntity; use OCA\Libresign\Db\FileMapper; +use OCA\Libresign\Db\SignRequestMapper; use OCA\Libresign\Exception\LibresignException; use OCA\Libresign\Service\File\FileListService; +use OCA\Libresign\Service\FolderService; use OCA\Libresign\Service\RequestSignatureService; use OCA\Libresign\Service\RequestSignatureWorkflowService; +use OCA\Libresign\Service\Validation\FileInputValidator; use OCA\Libresign\Service\Validation\SignerValidator; use OCA\Libresign\Service\Validation\SigningRequestValidator; use OCA\Libresign\Service\Validation\VisibleElementValidator; use OCP\AppFramework\Http; +use OCP\Files\IMimeTypeDetector; use OCP\IL10N; use OCP\IRequest; use OCP\IUser; @@ -63,6 +67,13 @@ protected function setUp(): void { $this->signerValidator, $this->visibleElementValidator, $this->fileMapper, + new FileInputValidator( + $this->l10n, + $this->createMock(SignRequestMapper::class), + $this->fileMapper, + $this->createMock(IMimeTypeDetector::class), + $this->createMock(FolderService::class), + ), ); $this->controller = new RequestSignatureController( diff --git a/tests/php/Unit/Service/IdDocsServiceTest.php b/tests/php/Unit/Service/IdDocsServiceTest.php index 7b94e64372..5937744e29 100644 --- a/tests/php/Unit/Service/IdDocsServiceTest.php +++ b/tests/php/Unit/Service/IdDocsServiceTest.php @@ -15,6 +15,7 @@ use OCA\Libresign\Db\IdentifyMethodMapper; use OCA\Libresign\Db\SignRequest; use OCA\Libresign\Db\SignRequestMapper; +use OCA\Libresign\Exception\LibresignException; use OCA\Libresign\Service\IdDocsService; use OCA\Libresign\Service\RequestSignatureService; use OCA\Libresign\Service\Validation\FileInputValidator; @@ -241,6 +242,71 @@ public function testAddFilesToDocumentFolderStoresFilesUnderTheOwnerOfTheSignedF ); } + /** + * The `file` entry is the same HTTP payload as the signature request's, + * so its node id is normalized at this boundary too and reaches + * saveFile() as an int. + */ + public function testAddFilesToDocumentFolderNormalizesTheNodeIdOfEachFile(): void { + $signRequest = new SignRequest(); + $signRequest->setId(55); + $signRequest->setFileId(10); + $this->fileMapper->method('getById')->willThrowException(new \OCP\AppFramework\Db\DoesNotExistException('no')); + $this->fileTypeMapper->method('getTypes') + ->willReturn(['IDENTIFICATION' => ['type' => 'IDENTIFICATION']]); + $this->fileInputValidator->expects($this->once()) + ->method('normalizeNodeId') + ->with(['nodeId' => '9007199254740993'], FileInputValidator::TYPE_ACCOUNT_DOCUMENT) + ->willReturn(['nodeId' => 9007199254740993]); + + $savedFile = new \OCA\Libresign\Db\File(); + $savedFile->setId(77); + $this->requestSignatureService->expects($this->once()) + ->method('saveFile') + ->with($this->callback(function (array $data): bool { + $this->assertSame(9007199254740993, $data['file']['nodeId']); + return true; + })) + ->willReturn($savedFile); + + $service = $this->getIdDocsService(); + $service->addFilesToDocumentFolder( + [['type' => 'IDENTIFICATION', 'name' => 'id-front.pdf', 'file' => ['nodeId' => '9007199254740993']]], + $signRequest, + ); + } + + public function testAddIdDocsReportsAnInvalidNodeIdWithTheIndexOfTheFile(): void { + $user = $this->createMock(IUser::class); + $this->fileInputValidator->expects($this->exactly(2)) + ->method('normalizeNodeId') + ->with($this->anything(), FileInputValidator::TYPE_ACCOUNT_DOCUMENT) + ->willReturnCallback(static function (array $file): array { + if (($file['nodeId'] ?? null) === 'temp-node') { + throw new LibresignException('File type: Account document. Invalid fileID.'); + } + return $file; + }); + $this->requestSignatureService->expects($this->never())->method('saveFile'); + + $service = $this->getIdDocsService(); + try { + $service->addIdDocs( + [ + ['type' => 'IDENTIFICATION', 'file' => ['base64' => 'ZmFrZQ==']], + ['type' => 'IDENTIFICATION', 'file' => ['nodeId' => 'temp-node']], + ], + $user, + ); + $this->fail('An invalid node id must be rejected'); + } catch (LibresignException $e) { + $this->assertSame( + ['type' => 'danger', 'file' => 1, 'message' => 'File type: Account document. Invalid fileID.'], + json_decode($e->getMessage(), true), + ); + } + } + public function testAddFilesToDocumentFolderWithoutResolvableOwnerKeepsCurrentBehaviour(): void { $signRequest = new SignRequest(); $signRequest->setId(55); diff --git a/tests/php/Unit/Service/RequestSignatureServiceTest.php b/tests/php/Unit/Service/RequestSignatureServiceTest.php index 231813bb80..8a9cb3396f 100644 --- a/tests/php/Unit/Service/RequestSignatureServiceTest.php +++ b/tests/php/Unit/Service/RequestSignatureServiceTest.php @@ -195,6 +195,39 @@ private function getService(array $methods = []): RequestSignatureService|MockOb ); } + /** + * saveFile() receives the node id already normalized to int by the + * workflow boundary and looks the existing LibreSign file up with it. + */ + public function testSaveFileReusesTheFileRegisteredForTheNodeId(): void { + $service = $this->getService(); + + $existing = new \OCA\Libresign\Db\File(); + $existing->setId(7); + $existing->setNodeId(9007199254740993); + $this->fileMapper->expects($this->once()) + ->method('getByNodeId') + ->with(9007199254740993) + ->willReturn($existing); + $this->filePolicyApplier->expects($this->once()) + ->method('syncAllPolicies') + ->with($existing, $this->anything()); + $this->fileStatusService->expects($this->once()) + ->method('updateFileStatusIfUpgrade') + ->with($existing, 1) + ->willReturn($existing); + $this->fileService->expects($this->never())->method('getNodeFromData'); + + $result = $service->saveFile([ + 'file' => ['nodeId' => 9007199254740993], + 'name' => 'contract', + 'status' => 1, + 'userManager' => $this->user, + ]); + + $this->assertSame($existing, $result); + } + public function testSaveFilesUsesSaveForSingleFile(): void { $service = $this->getService(['save']); diff --git a/tests/php/Unit/Service/RequestSignatureWorkflowServiceTest.php b/tests/php/Unit/Service/RequestSignatureWorkflowServiceTest.php index f4916e3ac2..03aadebd32 100644 --- a/tests/php/Unit/Service/RequestSignatureWorkflowServiceTest.php +++ b/tests/php/Unit/Service/RequestSignatureWorkflowServiceTest.php @@ -10,13 +10,16 @@ use OCA\Libresign\Db\File as FileEntity; use OCA\Libresign\Db\FileMapper; +use OCA\Libresign\Db\SignRequestMapper; use OCA\Libresign\Exception\LibresignException; +use OCA\Libresign\Service\FolderService; use OCA\Libresign\Service\RequestSignatureService; use OCA\Libresign\Service\RequestSignatureWorkflowService; use OCA\Libresign\Service\Validation\FileInputValidator; use OCA\Libresign\Service\Validation\SignerValidator; use OCA\Libresign\Service\Validation\SigningRequestValidator; use OCA\Libresign\Service\Validation\VisibleElementValidator; +use OCP\Files\IMimeTypeDetector; use OCP\IL10N; use OCP\IUser; use PHPUnit\Framework\Attributes\DataProvider; @@ -52,6 +55,13 @@ protected function setUp(): void { $this->signerValidator, $this->visibleElementValidator, $this->fileMapper, + new FileInputValidator( + $this->l10n, + $this->createMock(SignRequestMapper::class), + $this->fileMapper, + $this->createMock(IMimeTypeDetector::class), + $this->createMock(FolderService::class), + ), ); } @@ -155,6 +165,137 @@ public function testCreateRequestUsesEnvelopeSaveFilesAndReturnsProvidedChildren $this->assertSame([$child], $result['children']); } + /** + * Regression #8363: the Files sidebar sends the node id as a string. The + * workflow is the boundary between the HTTP payload and the services, so + * everything after it must already see an int. + */ + public function testCreateRequestNormalizesTheStringNodeIdOnceAtTheBoundary(): void { + $fileEntity = new FileEntity(); + $fileEntity->setId(9); + + $this->requestSignatureService->expects($this->once()) + ->method('validateNewRequestToFile') + ->with($this->callback(static fn (array $payload): bool => $payload['file']['nodeId'] === 9007199254740993)); + $this->requestSignatureService->expects($this->once()) + ->method('save') + ->with($this->callback(static fn (array $payload): bool => $payload['file']['nodeId'] === 9007199254740993)) + ->willReturn($fileEntity); + + $result = $this->service->createRequest( + $this->user, + ['nodeId' => '9007199254740993'], + [], + 'copy of contract.pdf', + [], + [['identifyMethods' => [['method' => 'email', 'value' => 'user@example.test']]]], + 1, + null, + ); + + $this->assertSame($fileEntity, $result['file']); + } + + public function testCreateRequestKeepsAnIntegerNodeIdAsItIs(): void { + $fileEntity = new FileEntity(); + $fileEntity->setId(9); + + $this->requestSignatureService->expects($this->once()) + ->method('validateNewRequestToFile') + ->with($this->callback(static fn (array $payload): bool => $payload['file']['nodeId'] === 11)); + $this->requestSignatureService->expects($this->once()) + ->method('save') + ->with($this->callback(static fn (array $payload): bool => $payload['file']['nodeId'] === 11)) + ->willReturn($fileEntity); + + $this->service->createRequest( + $this->user, + ['nodeId' => 11], + [], + 'contract.pdf', + [], + [['identifyMethods' => [['method' => 'email', 'value' => 'user@example.test']]]], + 1, + null, + ); + } + + public function testCreateRequestNormalizesTheNodeIdOfEachEnvelopeFile(): void { + $envelope = new FileEntity(); + $envelope->setId(30); + $envelope->setNodeType('envelope'); + + $this->requestSignatureService->expects($this->once()) + ->method('validateNewRequestToFile') + ->with($this->callback(static fn (array $payload): bool => $payload['files'][0]['nodeId'] === 9007199254740993 + && $payload['files'][1]['nodeId'] === 22 + && $payload['files'][2] === ['base64' => 'abc', 'name' => 'part-c.pdf'])); + $this->requestSignatureService->expects($this->once()) + ->method('saveFiles') + ->with($this->callback(static fn (array $payload): bool => $payload['files'][0]['nodeId'] === 9007199254740993 + && $payload['files'][1]['nodeId'] === 22)) + ->willReturn(['file' => $envelope, 'children' => []]); + + $this->service->createRequest( + $this->user, + [], + [ + ['nodeId' => '9007199254740993', 'name' => 'part-a.pdf'], + ['nodeId' => 22, 'name' => 'part-b.pdf'], + ['base64' => 'abc', 'name' => 'part-c.pdf'], + ], + 'Envelope', + [], + [['identifyMethods' => [['method' => 'email', 'value' => 'user@example.test']]]], + 0, + null, + ); + } + + public function testCreateRequestRejectsAnInvalidNodeIdBeforeAnyService(): void { + $this->requestSignatureService->expects($this->never())->method('validateNewRequestToFile'); + $this->requestSignatureService->expects($this->never())->method('save'); + + $this->expectException(LibresignException::class); + $this->expectExceptionMessage('Invalid fileID'); + + $this->service->createRequest( + $this->user, + ['nodeId' => 'temp-node'], + [], + 'contract.pdf', + [], + [['identifyMethods' => [['method' => 'email', 'value' => 'user@example.test']]]], + 1, + null, + ); + } + + public function testUpdateExistingRequestNormalizesTheStringNodeId(): void { + $fileEntity = new FileEntity(); + $fileEntity->setId(21); + $fileEntity->setParentFileId(20); + + $this->signingRequestValidator->expects($this->once()) + ->method('validateExistingFile') + ->with($this->callback(static fn (array $payload): bool => $payload['file']['nodeId'] === 9007199254740993)); + $this->requestSignatureService->expects($this->once()) + ->method('save') + ->with($this->callback(static fn (array $payload): bool => $payload['file']['nodeId'] === 9007199254740993)) + ->willReturn($fileEntity); + + $result = $this->service->updateExistingRequest( + $this->user, + [['identifyMethods' => [['method' => 'email', 'value' => 'user@example.test']]]], + 'uuid-21', + null, + ['nodeId' => '9007199254740993'], + null, + ); + + $this->assertSame($fileEntity, $result['file']); + } + public function testUpdateExistingRequestValidatesAndLoadsEnvelopeChildren(): void { $fileEntity = new FileEntity(); $fileEntity->setId(21); diff --git a/tests/php/Unit/Service/Validation/FileInputValidatorTest.php b/tests/php/Unit/Service/Validation/FileInputValidatorTest.php index 733d9f3d8f..330ed0e533 100644 --- a/tests/php/Unit/Service/Validation/FileInputValidatorTest.php +++ b/tests/php/Unit/Service/Validation/FileInputValidatorTest.php @@ -47,6 +47,57 @@ public function setUp(): void { ); } + /** + * The Files app sends node ids as strings (`Node.id` of `@nextcloud/files`) + * and, since Nextcloud 33, they can exceed a JavaScript number. The API + * accepts an int or a string of digits and the services only see an int. + */ + #[DataProvider('nodeIdNormalizationCases')] + public function testNormalizeNodeId(array $file, array $expected): void { + $this->assertSame($expected, $this->validator->normalizeNodeId($file)); + } + + public static function nodeIdNormalizationCases(): array { + return [ + 'int stays int' => [['nodeId' => 42, 'name' => 'a.pdf'], ['nodeId' => 42, 'name' => 'a.pdf']], + 'zero stays int' => [['nodeId' => 0], ['nodeId' => 0]], + 'canonical decimal string becomes int' => [['nodeId' => '42'], ['nodeId' => 42]], + 'zero as string' => [['nodeId' => '0'], ['nodeId' => 0]], + 'node id above Number.MAX_SAFE_INTEGER' => [['nodeId' => '9007199254740993'], ['nodeId' => 9007199254740993]], + '64-bit node id' => [['nodeId' => '9223372036854775807'], ['nodeId' => PHP_INT_MAX]], + 'null is absent' => [['nodeId' => null], ['nodeId' => null]], + 'without nodeId' => [['base64' => 'abc'], ['base64' => 'abc']], + ]; + } + + /** + * The boundary either hands an int to the services or stops here: an + * invalid value must not reach a later cast that could read it differently. + */ + #[DataProvider('invalidNodeIdCases')] + public function testNormalizeNodeIdRejectsAnythingElse(mixed $nodeId): void { + $this->expectException(LibresignException::class); + $this->expectExceptionMessage('Invalid fileID'); + + $this->validator->normalizeNodeId(['nodeId' => $nodeId]); + } + + public static function invalidNodeIdCases(): array { + return [ + 'negative int' => [-1], + 'string above PHP_INT_MAX' => ['9223372036854775808'], + 'non numeric string' => ['temp-node'], + 'signed string' => ['-42'], + 'leading zeros' => ['0042'], + 'decimal separator' => ['42.0'], + 'surrounding spaces' => [' 42 '], + 'empty string' => [''], + 'float' => [42.0], + 'bool' => [true], + 'array' => [['42']], + ]; + } + #[DataProvider('mimeTypeCases')] public function testValidatesMimeTypeForFileRole(string $mimeType, int $type, bool $valid): void { if (!$valid) {