feat(filesharing): require signer fullname; fix canEncrypt reactivity#239
feat(filesharing): require signer fullname; fix canEncrypt reactivity#239rubenhensen wants to merge 1 commit into
Conversation
Two changes in SendButton.svelte:
1. Flip `optional: true` → `optional: false` on
`pbdf.gemeente.personalData.fullname` in the `pg.sign.yivi` config so
the Yivi disclosure session refuses to complete without a name. The
cryptify recipient mail now reliably has a real name to show in place
of the bare sender email (paired change in cryptify).
Consequence: senders without a fullname credential in their Yivi app
can no longer sign. That's the intended outcome.
2. Fix `canEncrypt`: was `\$derived(() => { ... })` which stored the
arrow function itself as the derived value (never recomputed, no
reactive deps tracked). Only the single call site `canEncrypt()`
inside `startEncryption` hid the bug. Now `\$derived.by(() => ...)`
so the body is treated as the computation, and the call site reads
`canEncrypt` as a bool. Caught by the Svelte autofixer.
|
/dobby review |
|
Dobby has received the request! Routing to the right specialist now... |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Both edits are correct on their face. The $derived.by swap is semantically right but cosmetic — canEncrypt is only consumed once inside startEncryption, never in a reactive (template) context, so no user-visible behaviour changes from that hunk. Early-return at line 141 strands encryptionState at EncryptionState.Sign with no recovery path (pre-existing, but the PR touches the line). PR description's manual-test checkbox is unchecked, so the breaking signing-fails-without-fullname path isn't verified end-to-end.
Rule compliance
- [Rule: tests-required-on-fixes] Repo rule says
npm run test(Playwright) must run before claiming green; PR ran onlynpm run check. At minimum re-run Playwright; ideally add a smoke for the signing path. - [Rule: docs-drift] User-facing copy at
src/lib/locales/en.json:205("any additional personal data") and:213("in the Yivi app you can add optional data") still frames the signer attributes as optional. Making fullname required without updating that copy (plusnl.json) will surprise senders whose Yivi disclosure now silently fails. - [Rule: no-justification-paragraphs-for-simple-changes] PR body narrates ~250 words for a 2-hunk change. The fullname rationale is fine context; the
$derived.byautofixer paragraph is excess.
| /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/ | ||
|
|
||
| let canEncrypt = $derived(() => { | ||
| let canEncrypt = $derived.by(() => { |
There was a problem hiding this comment.
[Code review] $derived → $derived.by swap is semantically correct, but canEncrypt is never read in a reactive context — only consumed once inside startEncryption. The PR-description claim of a real reactivity bug overstates the impact; no user-visible behaviour changes from this hunk.
| let canEncrypt = $derived.by(() => { | ||
| if (encryptState.files.length === 0) return false | ||
| const totalSize = encryptState.files.reduce((a, f) => a + f.size, 0) | ||
| if (totalSize >= MAX_UPLOAD_SIZE) return false |
There was a problem hiding this comment.
[Code review] Threshold mismatch with getValidationErrors on line 77: here totalSize >= MAX_UPLOAD_SIZE, there totalSize > effectiveLimit. At exactly MAX_UPLOAD_SIZE validation passes but canEncrypt is false, triggering the stranded-UI path at line 141. Pre-existing, worth fixing while here (flip to >).
|
|
||
| try { | ||
| if (!canEncrypt()) return | ||
| if (!canEncrypt) return |
There was a problem hiding this comment.
[Code review] Early return when !canEncrypt leaves encryptState.encryptionState stuck at EncryptionState.Sign (set on line 135) with no reset — the user sees the Yivi popup with no QR and no error. Either reset the state here or remove the check (it's already redundant with getValidationErrors() in onSign).
| { | ||
| t: 'pbdf.gemeente.personalData.fullname', | ||
| optional: true, | ||
| optional: false, |
There was a problem hiding this comment.
[Code review] Making pbdf.gemeente.personalData.fullname required locks out any sender without a Dutch municipality credential (foreign users, anyone who hasn't enrolled BRP). Intended per PR description, but there is no pre-flight i18n notice — affected users only see a generic Yivi disclosure failure with no explanation. Pair this with a src/lib/locales/{en,nl}.json copy update before merge.
|
Superseded by #240, which keeps the mandatory-name intent while letting the signer satisfy it from any of four credentials (gemeente fullname OR firstName+lastName from passport / idcard / drivinglicence). Addresses dobby's review here directly. |
Remove the mandatory name disjunction from SIGN_ATTRIBUTES so senders only need to prove their email address (the pre-#239 UX). The condiscon infrastructure in postguard/pg-js remains available for future use. Update locale strings to no longer mention the name requirement.
…crypt reactivity (#240) * feat(filesharing): accept signer name from passport/idcard/drivinglicence Replace the previous (PR #239) mandatory pbdf.gemeente.personalData.fullname disclosure with a Yivi disjunction-of-conjunctions: the signer must disclose a name, but they may satisfy that from any one of four credentials -- gemeente fullname, OR firstName+lastName from pbdf.pbdf.{passport,idcard,drivinglicence}. This addresses dobby's review of #239: requiring only the gemeente credential silently locked out everyone without a Dutch municipality attestation. The disjunction is mandatory -- disclosure refuses to complete unless at least one option is satisfied. Optional mobilenumber and dateofbirth remain unchanged. The disjunction lives in a new signAttributes.ts module exporting a typed AttrConItem[] consumed by SendButton.svelte. Splitting it out keeps the component file focused and the attribute list reviewable in isolation. Locale copy updates flagged by dobby on en.json/nl.json: - emailSenderSubHeading describes the four-credential rule. - yiviTip no longer frames the name as optional. The $derived.by + (!canEncrypt) call-site fixes from #239 are preserved. Depends on encryption4all/postguard#198 (PKG), postguard-js#78 (sign.yivi attrs), and cryptify#170 (render firstName+lastName). Supersedes #239 in this repo. npm run check: 0 errors, 0 warnings. * feat(filesharing): only require email on sign step; name is optional Remove the mandatory name disjunction from SIGN_ATTRIBUTES so senders only need to prove their email address (the pre-#239 UX). The condiscon infrastructure in postguard/pg-js remains available for future use. Update locale strings to no longer mention the name requirement. * fix: remove AttrConItem import (not yet in published pg-js) * feat(filesharing): optional name disjunction via pg-js 1.11.0 condiscon Bump @e4a/pg-js to 1.11.0 which adds AttrDiscon support. Use it to request the sender's name optionally from any of four credentials (gemeente fullname, passport, ID card, or driving licence). The leading empty alternative keeps the group skippable for senders without any of these credentials.
1 participant
Summary
Two changes in
src/lib/components/filesharing/SendButton.svelte:Make signer fullname required. Flip
optional: true→optional: falseonpbdf.gemeente.personalData.fullnamein thepg.sign.yiviconfig. The Yivi disclosure session now refuses to complete without a name, so the cryptify recipient mail can reliably show a real name in place of the bare sender email — companion to cryptify#170.Consequence: senders without a fullname credential in their Yivi app can no longer sign. Intended.
Fix
canEncryptreactivity. Was$derived(() => { ... }), which stores the arrow function itself as the derived value — no reactive deps tracked, never recomputes. Only the single call sitecanEncrypt()insidestartEncryptionhid the bug by invoking it. Now$derived.by(() => ...)so the body is the computation, and the call site readscanEncryptas a bool. Caught by the official Svelte autofixer.Test plan
npm run check— 0 errors, 0 warnings.$derived.bysuggestion gone. Remaining suggestions are unrelated false positives ($effectcalling DOM methods ondialogRef;bind:thisdiscretionary "consider attachments").fullname— signing should fail to complete. With a credential that hasfullname, signing should succeed and the recipient mail (once cryptify#170 is also deployed) should show the disclosed name in place of the email.Companion change
The matching cryptify change that consumes the disclosed name and uses it in the mail body (plus other deliverability fixes from postguard#197): encryption4all/cryptify#170.