fix: apply the rejection visibility rules to signer status and to the validation API - #8398
maia-andre wants to merge 2 commits into
Conversation
|
Codecov Report❌ Patch coverage is
... and 226 files with indirect coverage changes 🚀 New features to boost your workflow:
|
da8f062 to
ab26e3e
Compare
| bool $hiddenRejectionInFile, | ||
| ): SignerPresentation { | ||
| $status = $signer->getStatusEnum(); | ||
| if ($hiddenRejectionInFile && $status !== SignRequestStatus::SIGNED && !$privileged) { |
There was a problem hiding this comment.
I think this !$privileged condition does not follow the rule defined in #8388.
When a rejection is hidden, all unsigned signers must have the same not_signed presentation. Otherwise it is still possible to find who rejected by comparing the signers.
For example, if signer A rejected and signer B can still sign, B currently sees:
- A as
not_signed - B as
ready_to_sign
Since B knows that they did not reject, they can know that A rejected.
The privilege to see your own rejection should be separate from the rule that hides the status of all unsigned signers.
Please change this so a pending signer does not bypass the not_signed redaction, while the signer who rejected can still see their own rejection details. The PHPUnit and Behat tests that currently expect the pending signer to stay as ready_to_sign also need to be updated.
There was a problem hiding this comment.
Done in 54ea5d2: being the signer no longer lifts the redaction of a pending entry. presentSigner() keeps only the viewer's own rejection; with a hidden rejection every other unsigned entry, the viewer's own pending one included, is not_signed without the real status. The PHPUnit matrix (visibility service, SignersLoader, FileListService, EnvelopeAssembler) and the Behat scenario were updated — for the other signer the scenario now asserts that no entry differs from not_signed on the validation payload, the file list and the child summary. Infection on the service stays at 100% MSI.
One consequence I had to handle in the same commit: the sign page (/p/sign/{uuid}) feeds useSignStore().ableToSign from these same signers, and that gate required mySigner.status === ABLE_TO_SIGN. With the own entry redacted, the pending signer would get "Unable to sign." whenever another signer had rejected privately. The store now treats an own entry without status as signable and leaves the signing order to the backend, which already enforces it (SignerValidator: "You are not allowed to sign this document yet"). Residual corner: sequential flow + private rejection + viewer not yet on turn → the button shows and the backend answers with that message instead of the pre-emptive "Unable to sign.". If you want that closed too, the natural place is settings.canSign reflecting the viewer's own real state (today it is me && !signed, which also feeds "You need to sign this document") — I left it out to keep this PR to the contract.
One more observation, not blocking: even with every unsigned entry redacted, a viewer who knows the behaviour can still infer that a rejection exists, because their own entry reads not_signed instead of ready_to_sign only when something is hidden — with two signers that identifies the rejecter. Closing it fully would mean presenting unsigned signers as not_signed whenever public_status is false, rejection or not. That is a product decision, so I did not touch it; happy to do it here or in a follow-up if you want.
There was a problem hiding this comment.
The implementation here is consistent with #8388, but the discussion exposed a broader product and architecture question around rejection behavior.
Before merging this PR, I would like to define that contract in #8405, especially around:
- whether rejection cancels or continues the workflow;
- who can see the rejection during the workflow;
- public validation visibility;
- comment visibility;
- how admin policy and requester choices interact;
- separation between
SignRequestStatus,displayStatus, andcanSign.
This is not a problem with the implementation in this PR. The contract itself may need to change before the feature is released.
I created #8405 so we can define the expected behavior and decide whether this PR should be adapted or merged as-is.
It would be useful if you could review the proposal there as well.
There was a problem hiding this comment.
I added a first version of the proposal to #8405, so I think we already have enough there to start reviewing the model properly.
The problem turned out to be a bit more complex than I first expected, especially around workflow behavior, visibility and policy inheritance.
Your feedback there is welcome.
54ea5d2 to
0659786
Compare
|
@maia-andre could you make a rebase to solve conflicts? I think that you will need to run again the command |
vitormattos
left a comment
vitormattos
left a comment
There was a problem hiding this comment.
After reviewing this again with the decisions from #8405, I think most of the presentation work here can still be kept.
The main changes will be to make the visibility service use the new policy model instead of public_status, and to remove the frontend fallback where a missing signer status is treated as signable. settings.canSign should handle that.
I think it makes sense to keep this PR open and adapt it after the policy model is in place.
For now, the rebase is the first step so we can get the branch back in sync with main.
… validation API The rejection record was already filtered by SignatureRejectionVisibilityService, but only on the file list, and the signer status went out untouched everywhere: with `public_status: false` another signer or an anonymous reader of the validation page still got `status: 3` / "Rejected" for the signer who rejected, and the validation payload (FileService → SignersLoader) never carried the `rejection` object at all, not even for the requester. One shared decision now drives every serializer: - `SignatureRejectionVisibilityService::hasHiddenRejection()` says whether the file holds a rejection the viewer may not know about (privileged viewers are the requester of the file and the signer who rejected, for their own entry), and `presentSigner()` returns a `SignerPresentation`: `displayStatus`, the real `status` or null, the `statusText` and the `rejection` object under the existing comment rules. When a rejection is hidden, every unsigned signer of that file is presented as `not_signed` / "Not signed" without the real status, so the rejecter cannot be told apart by comparison; signed signers are unchanged, and a viewer keeps their own entry. - `SignersLoader` (validation), `FileListService` (detailed file, file with children, child summaries), `EnvelopeAssembler` (child documents) and `FileService::mapSignerDetailsToSummary()` (which used to map REJECTED to DRAFT) consume that presentation. `FileResponseOptions` gained `isViewerOfSigner()`, the identity check SignersLoader used to do inline, so the validation and envelope paths resolve the viewer the same way. - `SignerDisplayStatus` is the API presentation enum (draft, ready_to_sign, signed, rejected, not_signed); `LibresignSignerSummary` and the contracts built on it declare `displayStatus` as required and the numeric `status` as optional. Nothing is persisted and no authorization reads the presentation: SignRequestStatus stays the workflow state. Tests: unit coverage of the decision and the presentation (Infection 100% MSI on the visibility service, the enum and the value object), the viewer matrix on each serializer (anonymous, another user, the pending signer, the requester, the rejecter), the summaries, and Behat scenarios on the validation endpoint and the file list for a private status, a public status and a canceled workflow. An observer (LibreSign#8143) is presented as `observing` and is never redacted: like a signed signer, they could not have rejected, so their real state says nothing about who did. Resolves: LibreSign#8388 Assisted-by: Claude Code:claude-opus-5 Signed-off-by: André Maia <andrefnkmm@gmail.com>
Being the signer no longer lifts the `not_signed` redaction of a pending
entry: with A rejected and B still able to sign, B saw themselves as
`ready_to_sign` next to A as `not_signed` and, knowing they did not
reject, could tell that A did. `presentSigner()` now keeps only the
viewer's own rejection, which they already know about; every other
unsigned entry of a file with a hidden rejection, the viewer's own
pending one included, is presented as `not_signed` without the real
status.
The sign page fed its "able to sign" gate from that very entry, so the
store no longer requires the real status when the own entry comes
redacted; the backend keeps enforcing the signing order ("You are not
allowed to sign this document yet").
Tests: the visibility service (own pending entry redacted for draft and
ready-to-sign, own rejection kept; Infection 100% MSI), the pending
viewer on SignersLoader, FileListService and EnvelopeAssembler, the
sign store gate, and the Behat scenario for the other signer, which now
asserts that no entry differs from `not_signed` on the validation
payload, the file list and the child summary.
Assisted-by: Claude Code:claude-opus-5
Signed-off-by: André Maia <andrefnkmm@gmail.com>
0659786 to
14aff24
Compare
|
Rebased on The conflicts came from #8143 (observer role) and #8412 (ValidateHelper removal). One of them needed a decision rather than a merge: an observer has Everything else is unchanged and stays as agreed in #8405: once the policy model lands, the visibility service reads the effective |
|
I reviewed the changes after the rebase. The And yes, keeping the other parts as they are for now is fine. We can adapt |
3 participants
Resolves: #8388
📝 Summary
Backend contract for rejection visibility, as specified in #8388 after the discussion in #8162 (option (a): redact every unsigned signer of the file when the viewer may not know about a rejection, so nobody is singled out by comparison).
One shared decision —
SignatureRejectionVisibilityService:hasHiddenRejection(file, signers, privilegedSignRequestIds): whether the file holds a rejection the viewer may not know about. Privileged = the requester of the file (for every entry) and the signer who rejected (for their own entry); everybody else only sees it whenpublic_statusis true.presentSigner(signer, file, privileged, hiddenRejectionInFile)→SignerPresentation(displayStatus, realstatusornull,statusText,rejectionunder the existing comment rules). With a hidden rejection every unsigned signer (DRAFT,ABLE_TO_SIGN,REJECTED) becomesnot_signed/ Not signed without the real status; signed signers keepsigned; the viewer's own pending entry is redacted like the others (a real status there would tell them who rejected by comparison), and the only entry a viewer keeps is their own rejection, which they already know about.SignerDisplayStatusenum:draft | ready_to_sign | signed | rejected | not_signed— presentation only, not persisted, never read for authorization.SignRequestStatusis untouched.Every response path consumes it — no serializer reconstructs the rules:
File\SignersLoader(validation endpoint) — gains therejectionobject it never had, andFileResponseOptions::isViewerOfSigner()(the identity check the loader used to do inline: identify method of the session, uid or e-mail of the authenticated user) so the envelope path resolves the viewer the same way;FileListService— detailed file, file with children (authenticated and anonymous) and the child summaries; the child summary used to encodestatus: 1for a signed signer and0otherwise, it now carries the real state anddisplayStatuslike the contract says;EnvelopeAssembler— child documents, same rules;FileService::mapSignerDetailsToSummary()— passes the presentation through instead of mappingREJECTED(and anything unknown) toDRAFT.Contracts:
LibresignSignerDisplayStatus;LibresignSignerSummary(andLibresignSignerDetail/LibresignValidatedChildSignerbuilt on it) declaredisplayStatusas required andstatus?: 0|1|2|3as optional. Generated schema:SignerDisplayStatusenum,statusno longer required; TypeScriptdisplayStatus: components["schemas"]["SignerDisplayStatus"],status?: 0 | 1 | 2 | 3.The
buildSignerRejection()comment branch was folded into one return with identical behaviour (it was an equivalent mutant).🧪 How to test
Behat (
tests/integration/features/sign/reject.feature, 3 new scenarios, 90 steps, run locally):rejection.comment), the rejecter (own entryrejected, the otherready_to_sign), the other signer (every unsigned entrynot_signed, their own included — no entry differs fromnot_signed— nostatus, norejection, on validation, on the list and on the child summary), an authenticated bystander and an anonymous reader ([.signers[].displayStatus] | unique == ["not_signed"], nostatus, norejection); then the rejected signer still cannot reject/sign again and the requester still sees3— redaction is presentation only;rejected/3/rejectedAtand no comment (show_comment_on_validation: false), the requester gets the comment;file.status: 6and every signernot_signedwithoutstatus; the requester sees who rejected.PHPUnit (all run locally):
SignatureRejectionVisibilityServiceTest(26 — hidden decision incl. privileged/public/multiple rejections, the four mappings, redaction of each unsigned state, signed never redacted, own pending entry redacted, own rejection kept, rejection object),SignerDisplayStatusTest,SignerPresentationTest,FileResponseOptionsTest(viewer identity), and the viewer matrix — anonymous, another user, the pending signer, the requester, the rejecter — onSignersLoaderTest,FileListServiceTest(detailed, with children, child summary) andEnvelopeAssemblerTest;FileServiceTestsummaries (visible rejection stays3, redacted entry has no status, unknown status is no longerDRAFT).Infection on
SignatureRejectionVisibilityService,SignerPresentationandSignerDisplayStatus: 68 mutants, 68 killed, MSI 100%, no errors.Full unit suite: 3857 tests; the only failures are the 4 order-dependent ones in
AEngineHandlerTestthat also fail onmain. php-cs-fixer clean; psalm on the changed files: no new errors. OpenAPI and TypeScript types regenerated.⚙️ API / Back‑end changes
displayStatuson every signer entry; numericstatusbecomes optional (absent for a redacted entry); new neutralstatusText"Not signed"rejectionto privileged viewers2for signed) instead of1/0FileResponseOptions::isViewerOfSigner();SignatureRejectionVisibilityServicegainsIL10NFrontend: the regenerated
SignerSummaryRecordrequiresdisplayStatus, so the three places that build such records locally (VisibleElements.vue,SignPDF.vue—draftplaceholders — andnormalizeCertificateSigner()invalidationDocument.ts,signed/draftfrom the certificate status) and five specs set it; no behaviour change there. One behaviour change insrc/store/sign.js: the sign page'sableToSigngate used to require the realstatuson the viewer's own entry, which is now redacted while a rejection is hidden, so an own entry withoutstatusno longer blocks signing — the backend keeps enforcing the signing order ("You are not allowed to sign this document yet"). Vitest (incl. a new store case),vue-tscand ESLint clean.Note for #8162
The validation frontend is out of scope here, as the issue says. Two facts for whoever picks it up (me, next):
validationDocument.tsaccepts only signerstatus0|1|2, so onmainthe validation page already fails to parse any file with a rejected signer (3) — this PR does not change that (redacted entries omitstatus, which the parser rejects the same way); and the list/sidebar components readstatusText, so they show Not signed for a redacted entry without further change.🚧 Backport
None (
mainonly; the rejection feature is not released).✅ Checklist
🤖 AI (if applicable)