Skip to content

feat: configurable email attribute type for the finalize sender check#193

Merged
rubenhensen merged 1 commit into
mainfrom
feat/email-attribute
Jul 17, 2026
Merged

feat: configurable email attribute type for the finalize sender check#193
rubenhensen merged 1 commit into
mainfrom
feat/email-attribute

Conversation

@rubenhensen

Copy link
Copy Markdown
Contributor

Part 2 of encryption4all/postguard#236 (Phase 2 of EPIC encryption4all/postguard#201) — the cryptify leg; companion to encryption4all/postguard#244 (PKG) and a pg-js PR to follow.

Problem: upload_finalize rejects any upload whose sender identity lacks exactly pbdf.sidn-pbdf.email.email — a hidden contract the e2e harness discovered, and one of the three hardcodings that make it impossible to run real client flows in a test environment (no test env can issue pbdf.* credentials).

Change: new email_attribute config key (defaults to the production value — deployed configs unaffected). Finalize's sender lookup and the sender-attributes filter both use it. The e2e stack will set irma-demo.sidn-pbdf.email.email — the public demo scheme mirrors the email credential with published issuer keys, so the harness gets real email semantics on the identical code path.

Tests: config default + override parse tests; full suite 149 + 2 green; clippy clean (-D warnings).

Part 2 of encryption4all/postguard#236 (the cryptify leg). Finalize required
exactly pbdf.sidn-pbdf.email.email in the sender's signing identity; test
environments cannot issue pbdf credentials, blocking real end-to-end flows.
The `email_attribute` config key (default: the production value, so deployed
configs are unaffected) now names the attribute type carrying the sender's
email. Companion changes: pg-pkg (encryption4all/postguard#244) and pg-js.
@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR, Ruben! I'm on it — kicking off a review of the email_attribute config change, checking the upload_finalize sender lookup and attribute filter for correctness and rule-compliance, then I'll leave a single consolidated review with a clear verdict.

@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.

Approving — clean, well-scoped change with no rule or review findings.

Rule check (Rust backend diff): all green.

  • feat: title passes the conventional-commit gate (pr-title.yml).
  • Cross-repo refs use the full encryption4all/postguard#NNN form, so they auto-link.
  • No closing keyword — correct, since this is Part 2 of a multi-part epic and must not auto-close #236.
  • Parse tests added for both the default and the override.

Verification: cargo fmt --all -- --check clean; tests 151/151 and clippy -D warnings clean (per the review step).

Correctness notes:

  • Both finalize call sites (sender lookup and the sender-attributes filter) route through config.email_attribute(), so they stay consistent — no risk of the filter and the lookup disagreeing.
  • The unwrap_or_else default (pbdf.sidn-pbdf.email.email) preserves current production behaviour, so deployed configs are unaffected.
  • The only remaining pbdf.sidn-pbdf.email.email literal in src/ is a test-fixture comment, not a live code path — the finalize hardcoding is fully removed.

Looks good to merge.

@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Reviewed and approved: encryption4all/cryptify#193. The new email_attribute config key defaults to the production value, so existing deployments are unaffected, and both finalize call sites (sender lookup and sender-attributes filter) go through the same accessor, keeping them from drifting apart. cargo fmt, clippy (-D warnings), and all 151 tests are clean, and the PR correctly avoids auto-closing postguard#236 since this is just Part 2 of the epic. Nice clean, well-scoped change — nothing else needed from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant