Skip to content

feat: configurable email attribute types (emailAttributes option)#115

Merged
rubenhensen merged 2 commits into
mainfrom
feat/email-attribute
Jul 16, 2026
Merged

feat: configurable email attribute types (emailAttributes option)#115
rubenhensen merged 2 commits into
mainfrom
feat/email-attribute

Conversation

@rubenhensen

Copy link
Copy Markdown
Contributor

Part 3 of encryption4all/postguard#236 (Phase 2 of EPIC encryption4all/postguard#201) — the pg-js leg; companions: encryption4all/postguard#244 (PKG, merged pending) and encryption4all/cryptify#193.

Problem: pbdf.sidn-pbdf.email.{email,domain} was hardcoded in 10 places across 6 files (recipient builders, key requests, decryption hints, includeSender policies, yivi/session signing bodies). No test environment can issue pbdf.* credentials, so the real SDK could never be driven end to end in a test — the single blocker to Phase 2's real-SDK harness flows.

Change: new PostGuard({ …, emailAttributes: { email, domain } }) — resolved once against the production defaults and threaded through both pipelines and all signing/decrypt paths. Zero behavior change without the option: all 212 existing tests pass untouched.

The e2e stack will set irma-demo.sidn-pbdf.email.email (the public demo scheme mirrors the email credential with published issuer keys), matching the PKG's PKG_EMAIL_ATTRIBUTE and cryptify's email_attribute.

Notes:

  • The JWT sender-email extraction heuristic (.endsWith('.email.email')) already matches demo-scheme types — untouched.
  • 6 new tests (tests/email-attributes.test.ts) pin default + override behavior through builders, key requests and signing start bodies.
  • tsc --noEmit clean, build clean, 218/218 tests.

Part 3 of encryption4all/postguard#236 (the pg-js leg). The pbdf email and
email-domain attribute types were hardcoded across recipient builders, key
requests, decryption hints, includeSender policies and all signing flows —
making it impossible to run the real SDK in any test environment (pbdf
credentials cannot be issued outside production) and coupling every layer to
one scheme.

PostGuardConfig gains `emailAttributes?: { email?, domain? }`, resolved once
against the production defaults and threaded through the pipelines; the seam
helpers (buildEncryptionPolicy, buildKeyRequest, buildStartRequestBody,
session/yivi signing) accept the resolved types with unchanged defaults.
Existing callers are unaffected — all 212 existing tests pass untouched; 6
new tests pin the override and default behavior.

Companions: encryption4all/postguard#244 (PKG) and
encryption4all/cryptify#193 (cryptify).
@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR, @rubenhensen! I'm on it — starting a review of this pg-js leg now (the emailAttributes refactor replacing the hardcoded pbdf.sidn-pbdf.email references), and I'll follow up with a consolidated review shortly.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the abstraction is clean (resolveEmailAttributes merging partial overrides once, threaded through both pipelines) and the 6 new tests pin default + override behaviour at exactly the right seams. tsc/build/tests all green.

One gap before merge, on the exact path this PR makes configurable:

The apiKey signing path is not threaded with the override. In src/crypto/signing.ts, the yivi (line 23) and session (line 26) cases both receive emailAttributes, but the apiKey case (line 16) calls resolveSigningKeysFromApiKey(pkgUrl, sign.apiKey, headers) without it — and fetchSigningKeysWithApiKey (src/api/pkg.ts:44) hardcodes pubSignId: [{ t: 'pbdf.sidn-pbdf.email.email' }]. Under an override (e.g. the e2e harness's irma-demo.*), an API-key sender's signing-key request would ask the PKG for the production pbdf type while recipient policies and decrypt hints use the overridden type — a silent mismatch, which is the class of bug this change is meant to remove. The PR body says the option is "threaded through … all signing/decrypt paths"; the apiKey leg is the one that isn't.

This is a small fix either way:

  • Thread emailAttributes through resolveSigningKeysFromApiKeyfetchSigningKeysWithApiKey's pubSignId, mirroring the yivi/session legs, or
  • If API-key signing is intentionally out of scope for the demo harness, add a one-line comment at the apiKey case (and/or in pkg.ts) stating it always uses the production pbdf.* type, so the omission is deliberate and documented.

Everything else looks good — happy to re-approve once this is resolved one way or the other.

Comment thread src/crypto/signing.ts
sign: SignMethod,
headers?: HeadersInit
headers?: HeadersInit,
emailAttributes?: EmailAttributes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This emailAttributes param reaches the yivi (line 23) and session (line 26) cases but the apiKey case at line 16 (resolveSigningKeysFromApiKey(pkgUrl, sign.apiKey, headers)) drops it, and fetchSigningKeysWithApiKey (src/api/pkg.ts:44) hardcodes pubSignId: [{ t: 'pbdf.sidn-pbdf.email.email' }]. Under an emailAttributes override an API-key sender would request the production type while recipient policies use the overridden type — a mismatch on the exact path this PR makes configurable. Either thread it through here too, or document that api-key signing intentionally stays on the production pbdf type.

@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Reviewed and left a Request Changes on #115. The resolveEmailAttributes abstraction and new tests look solid, and everything (tsc, build, 218/218 tests) is green.

One gap before merge: the apiKey signing leg in src/crypto/signing.ts doesn't get the emailAttributes override threaded through — fetchSigningKeysWithApiKey (src/api/pkg.ts:44) still hardcodes the production pbdf.sidn-pbdf.email.email type, unlike the yivi and session cases. Under an irma-demo.* override, an API-key sender would sign with the production type while recipients use the overridden one — exactly the mismatch this PR is meant to eliminate.

Small fix either way: thread emailAttributes into pubSignId, or add a comment noting api-key signing intentionally stays on pbdf.*. Happy to re-approve once that's sorted.

Review catch: the apiKey dispatch case dropped the configured attributes and
fetchSigningKeysWithApiKey hardcoded the production type in its request body
- under an override, an api-key sender would request the production type
while recipient policies used the overridden one. Threaded through
(dispatch -> resolveSigningKeysFromApiKey -> fetchSigningKeysWithApiKey) with
a wire-level test capturing the request body. Note: the PKG derives the
api-key signing identity from the business database and ignores this body
today; the configured type is sent anyway for wire-level consistency.
@rubenhensen

Copy link
Copy Markdown
Contributor Author

Good catch, /dobby — that was a genuinely dropped site (my sweep truncated its grep output; re-swept the whole tree, it was the only one left). Threaded through rather than documented-around: resolveSigningKeysresolveSigningKeysFromApiKeyfetchSigningKeysWithApiKey all carry emailAttributes, and the request body uses the configured type. New wire-level test captures the POST body and asserts pubSignId[0].t under an override.

One server-side note for the record: under API-key auth the PKG currently derives the signing identity from the business database and ignores this body (pg-pkg signing_key.rs), so the mismatch was inert today — but sending the configured type keeps the wire contract coherent and future-proof, which is why threading beats documenting. Noted in the function's doc comment. tsc clean, 219/219 tests.

@rubenhensen
rubenhensen merged commit 854d5c0 into main Jul 16, 2026
5 checks passed
@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Thanks for the detail! I'm on it — kicking off a review pass on this PR now to check the signing-key threading changes and the new wire-level test for correctness and rule-compliance, then I'll post one consolidated review.

@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Nice catch on the re-sweep — glad that was the last one. Since #115 is already merged there's no formal review to post (nothing left to gate), but for the record everything here checks out: threading emailAttributes through resolveSigningKeysresolveSigningKeysFromApiKeyfetchSigningKeysWithApiKey is the right call over documenting around a stale default, the wire-level test asserting pubSignId[0].t locks in the behavior that matters, and noting the pg-pkg signing_key.rs server-side reality in the doc comment is exactly the kind of "inert today, coherent tomorrow" detail worth capturing. tsc clean and 219/219 green — good place to land it. No action needed on my end.

#115

@dobby-coder
dobby-coder Bot deleted the feat/email-attribute branch July 16, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant