Repository navigation
fix: freeze the signature flow once the request is sent - #9062
paulo-raoni wants to merge 4 commits into
Conversation
Only a node that is not registered yet falls through to the new-file path; policy failures now reach the caller. Signed-off-by: Paulo Raoni <raoni_jkd@hotmail.com>
A frozen request keeps its stored flow: the same value is accepted, another one is refused with 422. Signed-off-by: Paulo Raoni <raoni_jkd@hotmail.com>
…is frozen The file list and request-signature responses carry policySnapshotFrozen; validation responses are unchanged. Signed-off-by: Paulo Raoni <raoni_jkd@hotmail.com>
The sidebar sends the stored flow, none included, and no longer offers the signing order switch. Signed-off-by: Paulo Raoni <raoni_jkd@hotmail.com>
YvesCesar
left a comment
There was a problem hiding this comment.
Thanks, @paulo-raoni !
I have a few revisions. Can you take a look?
| ]; | ||
| // The sign page formats files without a user and may be public. | ||
| if ($user !== null) { | ||
| $file['policySnapshotFrozen'] = FileStatusService::isPolicySnapshotFrozen($fileEntity); |
There was a problem hiding this comment.
policySnapshotFrozen here only looks at the file itself, but the backend also treats a document as frozen when its envelope is frozen (AbstractFilePolicyApplier::isPolicySnapshotFrozen checks $envelope too).
So a child document that is still in DRAFT inside a frozen envelope comes back with false. The sidebar then takes the non-frozen path, where toRequestSignatureFlowOverride turns none into parallel, and the save fails with 422.
Could you make the flag use the same rule as the backend (file or parent envelope frozen)? Lines 392 and 839 need the same change. A shared method that takes the envelope into account would keep both sides from drifting apart.
| if (!is_null($nodeId)) { | ||
| try { | ||
| $file = $this->fileMapper->getByNodeId($nodeId); | ||
| } catch (DoesNotExistException) { |
There was a problem hiding this comment.
Non-blocking: node_id has no unique index, and the old catch (\Throwable) could create a second row for the same node when policy sync failed. On installs that already have such duplicates, getByNodeId now throws MultipleObjectsReturnedException and the save returns 500. It may be worth catching it and returning a clear error, or handling it in a follow-up.
Refs: #8715
📝 Summary
Next slice of #8715:
signature_flowfollows the request-snapshot freeze. This is option 1 from the issue discussion: strict backend freeze, the request sidebar updated in the same PR, and the broad catch insaveFile()narrowed.One commit per step:
fix: stop creating a second file when updating by node id fails– inRequestSignatureService::saveFile()onlyDoesNotExistExceptionfromgetByNodeId()leads to the new-file path. Policy and status failures now reach the caller instead of being swallowed into a secondFilerow for the same node.fix: freeze the signature flow once the request is sent–SignatureFlowFilePolicyApplier::sync()returns before any resolution whenisPolicySnapshotFrozen()is true. The submitted value is compared with the stored column, strictly:noneis notparallel.feat: tell authenticated clients whether the request policy snapshot is frozen– new booleanpolicySnapshotFrozenonLibresignFileSummaryandLibresignDetailedFile(file list and request-signature / file-save responses). The status alone cannot tell, because a request that returned to DRAFT stays frozen.LibresignValidatedFileand the validation metadata are unchanged, so thefile/validateendpoints do not carry it and the marker itself is still not exposed there. The sign page payloads (...ForSignRequest, including theenvelopeFilesinitial state) do not carry it either;FileListServiceTestcovers that.fix: keep the stored signature flow in the request sidebar once frozen– once frozen, the sidebar shows and sends the stored flow (noneincluded), hides "Sign in order" and "Use this as my default signing order" the same way it does when the policy forbids the override, and no longer rewrites the file flow from a policy forced later.signature_flowABLE_TO_SIGNor later without marker, or document of a frozen envelope)noneincludedparallelover a storednoneincludedSigners added later use the stored flow: every consumer reads the column, which no longer moves.
Behaviour change for API clients: a client that PATCHes a different
signature_flowafter the request was sent now gets 422. A request created through the API with the default status isABLE_TO_SIGN, so it is frozen at creation. On the node id path, any failure other than "node not registered yet" is now returned instead of creating another file.Not in this PR:
add_footer,docmdp, the other snapshot keys, and the envelope rule for files added after the freeze. An envelope entity only receives its flow through a PATCH by uuid; that is unchanged here and belongs with the envelope rule.🧪 How to test
composer test:unit -- --filter 'SignatureFlowFilePolicyApplierTest|RequestSignatureServiceTest|FileStatusServiceTest|FileListServiceTest' vendor/bin/phpunit -c tests/php/phpunit.xml tests/php/Api/Controller/RequestSignatureControllerTest.php npx vitest run src/tests/components/RightSidebar/RequestSignatureTab.spec.ts src/tests/views/Settings/PolicyWorkbench/settings/signature-flow/model.spec.tsSignatureFlowFilePolicyApplierTest: a frozen request keeps its flow with nothing submitted, with the stored flow resent, after returning to DRAFT and without a marker; a different flow gets 422 (parallelovernoneincluded); a signer added after the freeze is ordered by the stored flow.RequestSignatureServiceTest: a refused update of a registered node does not create another file; an unregistered node is still created.FileStatusServiceTest,FileListServiceTest: the flag on the three payload paths, for a draft, a sent request back in DRAFT and a request sent before the marker existed.RequestSignatureControllerTest: PATCH with another flow on a sent request returns 422 with the message above.Manually: send a request with two signers and reopen it in the sidebar: the signing order switch is gone and adding a signer saves without error.
PATCH /api/v1/request-signaturewith anotherpolicy.overrides.signature_flowon that uuid returns 422.Validation evidence
composer psalm, php-cs-fixer dry-run on the touched files,npm run lint,npm run ts:checkRan locally:
tests/php/Unit/Service/Policy(952 tests);--filter 'RequestSignatureService|FileStatusService|Envelope|SequentialSigning|FileListService'(324 tests);tests/php/Api/Controller/RequestSignatureControllerTest.phpandFileControllerTest.php; Behatfeatures/sign/sequential_signing.feature(2 scenarios); the focused Vitest files;npm testonce.Not run locally: the whole PHPUnit
unitandapisuites, the other Behat features, and Playwright (signature-flow-policy-request-sidebar.spec.tsonly toggles the switch on a draft, which is unchanged; it was read, not run).npm testhas one failure, insrc/tests/views/Validation.spec.ts("updates the policies store when requester effective policies are returned"). It is not caused by this PR: the spec expectspoliciesStore.setPoliciesto be called, andValidation.vuestopped calling it in 37d92dd. This PR does not touch that view, its spec or the policies store.tests/php/Integration/Controller/PageControllerTest.phphas 11 errors (ArgumentCountError: the test buildsPageControllerwithout$validationEffectivePolicyService). They are the same with everylib/file of this branch restored tomain, so they are not caused by this PR either.🎨 UI / Front-end changes
On a request that already entered the signing flow, the request sidebar no longer shows "Sign in order" and "Use this as my default signing order", and it saves with the stored flow.
Screenshots before/after added for user-visible changes – not yet
Tested in relevant browsers when applicable – not yet
Component/unit (Vitest) and/or E2E (Playwright) tests added or updated – Vitest
Accessibility verified when applicable – no new control; two existing switches are hidden
Design review approved when applicable
Documentation updated when applicable
⚙️ API / Back-end changes
signature_flowis frozen with the request policy snapshot; a different value after the freeze returns 422.policySnapshotFrozenonLibresignFileSummaryandLibresignDetailedFile.saveFile()by node id returns policy failures instead of creating another file.composer openapiwhen necessary – andnpm run typescript:generate; the generated diff is only the new field✅ Checklist
🤖 AI (if applicable)