feat(email): carry the tenant on the port, and ship a default provider - #136
Merged
msalvatti merged 2 commits intoAug 9, 2026
Merged
Conversation
Reaches parity with nest-auth's #88, #95 and #96, the functional changes it took since the last alignment (#134 here, #74 there). Every EmailProvider method now takes the account's tenant_id first, so a multi-tenant delivery channel can attribute and route each message rather than guessing from the recipient. A platform admin is cross-tenant and carries no tenant, so MFA notices on that plane arrive under the reserved PLATFORM_EMAIL_TENANT ("platform"), held byte-identical to nest-auth's constant. Breaking for a hand-written EmailProvider, and that is the outcome to want: Rust refuses to compile an implementation that has not been updated, where a structurally-typed language would keep compiling one that now passes the recipient where the tenant goes. Nothing is published yet, so no released consumer breaks. Adds DefaultAuthEmailProvider over AuthEmailSink, a one-method delivery port, so a deployment does not hand-write ten methods to send a verification code. It carries the policy each of those would otherwise have to get right alone: HTML escaping on a body path that holds caller-chosen text, CR/LF stripping on the subject so that text cannot inject a header, the NIST SP 800-63B notification catalogue, and a swallow-and-log failure policy so a down channel never turns "enable MFA" into a failed request. The copy is replaceable through AuthEmailCatalogue; the escaping and the failure policy are not. OutgoingEmail and AuthEmailMessage redact both bodies in Debug and mask the recipient: a rendered body is where the reset token and the OTP sit in the clear, and a sink logging its input at debug would put every one of them in a log pipeline. tenantId becomes optional wherever a TenantIdResolver can supply it. A configured resolver already ignores the body's value, so a client under one may now omit the field instead of sending something the server discards. With no resolver the body is the only thing that can scope the request, and one naming no tenant is refused with auth.validation rather than defaulted: inventing a tenant name would gather into one scope every account a misconfigured deployment created, and that scope keys the user lookup, the Redis records and the HMAC identifiers. The reset helpers now take the resolved tenant as an argument instead of reading it back off the input struct. With both values in scope, reading the wrong one is a one-character mistake no test would notice, and it is the mistake nest-auth had to fix on this same flow. Verified with fmt, clippy -D warnings, the full suite, and llvm-cov at 100% lines and 100% functions. A cargo-mutants sweep scoped to the diff reports 59 caught, 15 unviable, 0 survivors; the two survivors that were dead code were deleted rather than excluded, and the two that were real test gaps (nothing asserted the password-changed notice was ever sent) are now covered.
There was a problem hiding this comment.
Pull request overview
Adds tenant-aware email delivery, a secure default email provider, and optional tenant inputs when a resolver is configured.
Changes:
- Passes resolved tenant IDs through all email operations.
- Adds configurable email rendering, sanitization, redaction, and error handling.
- Makes tenant inputs optional and refactors reset flows around resolved tenants.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents email and tenant APIs. |
docs/technical_specification.md |
Updates behavioral specification. |
crates/bymax-auth-redis/tests/redis_stores.rs |
Adapts Redis tests to optional tenants. |
crates/bymax-auth-redis/tests/mfa_lifecycle_e2e.rs |
Updates MFA E2E inputs. |
crates/bymax-auth-core/tests/engine_assembly.rs |
Updates assembly tests. |
crates/bymax-auth-core/src/traits/mod.rs |
Exports new email APIs. |
crates/bymax-auth-core/src/traits/email.rs |
Adds tenant-aware email methods. |
crates/bymax-auth-core/src/traits/default_email.rs |
Implements the default email provider. |
crates/bymax-auth-core/src/services/platform.rs |
Updates platform tests. |
crates/bymax-auth-core/src/services/oauth.rs |
Accepts optional tenant input. |
crates/bymax-auth-core/src/services/mfa/tests.rs |
Tests tenant-attributed MFA notices. |
crates/bymax-auth-core/src/services/mfa/setup.rs |
Forwards tenant on MFA enablement. |
crates/bymax-auth-core/src/services/mfa/mod.rs |
Resolves MFA email attribution. |
crates/bymax-auth-core/src/services/mfa/manage.rs |
Forwards tenant on MFA disablement. |
crates/bymax-auth-core/src/services/auth/session_ops.rs |
Updates optional-tenant fixtures. |
crates/bymax-auth-core/src/services/auth/register.rs |
Resolves optional registration tenants. |
crates/bymax-auth-core/src/services/auth/password_reset.rs |
Refactors reset tenant handling. |
crates/bymax-auth-core/src/services/auth/mod.rs |
Implements optional tenant resolution. |
crates/bymax-auth-core/src/services/auth/login.rs |
Resolves optional login tenants. |
crates/bymax-auth-core/src/services/auth/invitation.rs |
Attributes invitations to tenants. |
crates/bymax-auth-core/src/services/auth/email_verification.rs |
Supports resolver-supplied tenants. |
crates/bymax-auth-core/src/services/auth/email_change.rs |
Attributes email-change messages. |
crates/bymax-auth-core/src/services/auth/detached.rs |
Forwards tenants through email tasks. |
crates/bymax-auth-core/src/services/adapter_api.rs |
Updates adapter tests. |
crates/bymax-auth-axum/src/validation.rs |
Tests optional OAuth tenants. |
crates/bymax-auth-axum/src/routes/oauth.rs |
Forwards optional OAuth tenants. |
crates/bymax-auth-axum/src/routes/auth.rs |
Forwards optional verification tenants. |
crates/bymax-auth-axum/src/dto.rs |
Makes resolver-compatible tenant fields optional. |
CHANGELOG.md |
Records new and breaking behavior. |
.cargo/mutants.toml |
Expands equivalent-mutant exclusions. |
Suppressed comments (1)
crates/bymax-auth-axum/src/dto.rs:210
- The existing
resend_otphandler discards every engine error, including the new missing-tenantAuthError::Validation. Thus omitting this now-optional field without a resolver returns a successful 200 but cannot send an OTP, despite the documented refusal. Surface validation failures while retaining the uniform anti-enumeration response for account-dependent outcomes.
pub tenant_id: Option<String>,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… two docs Three findings, each verified against the code before acting. The anti-enumerating routes discarded the new AuthError::Validation. Making tenantId optional opened a path where a caller who names no tenant, with no resolver configured, got 200/204 and no mail was ever sent — a failure that looks like success from every side, and one this change introduced. The three mail-triggering routes now go through one anti_enumerating_outcome helper that surfaces that error and still collapses every account-dependent outcome: existence, blocked status and an unreachable store stay indistinguishable. Surfacing it leaks nothing, because the refusal is decided before any lookup runs and cannot vary by account. Centralised rather than patched per route — one rule with three implementations is how this reopens. The escaping guarantee was too broad. AuthEmailMessage::html is passed through verbatim, so a renderer that returns it owns the escaping of every dynamic value it interpolates; only what the provider renders is escaped. Stated as a security boundary in the trait doc, the CHANGELOG and the README, since an override trusting a protection that is not there is the failure mode. The Rethrow tradeoff was inaccurate. The MFA and password-changed notices go out through spawn_guarded and the invitation flow catches its error and leaves the invitation standing, so Rethrow fails neither. It buys the cleanup on the reset and email-change flows and costs nothing elsewhere. Also fixes the rustdoc gate: five public doc comments linked the pub(crate) resolve_tenant, and one linked an unresolvable OutgoingEmail::fmt. Verified in a clean worktree carrying only these changes, because the shared checkout has another agent's in-progress OtpService work: fmt, clippy -D warnings, rustdoc -D warnings and the full suite all pass there.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
crates/bymax-auth-axum/src/dto.rs:52
- The server now accepts an omitted
tenantId, but both shipped clients still make it mandatory (crates/bymax-auth-client/src/lib.rs:109,120,146,304andpackages/rust-auth/src/client/index.ts:269,281,289,323). The React wrapper also substitutes"default"atpackages/rust-auth/src/react/index.ts:186,210, so it keeps inventing a tenant instead of allowing resolver-backed omission. Update the client request types/serialization and remove that fallback so this new contract is usable through the first-party clients.
/// The tenant scope. Optional: a deployment that configures a `TenantIdResolver`
/// ignores this value, so a client there may omit it. With no resolver configured it is
/// the only thing that can scope the request, and omitting it is refused rather than
/// defaulted (`auth.validation`) — see `AuthEngine::resolve_tenant`.
README.md:240
- The preceding sentence says the library passes structured data and “never rendered HTML,” but this newly documented default path renders the message and passes
html/texttoAuthEmailSink. Qualify the statement by port so readers do not implement the sink expecting tokens/OTPs as structured fields.
**Most deployments should not write this trait by hand.** `DefaultAuthEmailProvider` already implements all ten methods over a one-method delivery port, with the escaping, header sanitization and failure policy described below:
docs/technical_specification.md:3714
- This new built-in rendering section leaves the specification internally contradictory: §10's introduction (lines 3535–3540) still says rendering and copy are entirely adapter-owned, while §22.2 (lines 6942–6943) says the library ships only NoOp/recording implementations and that rendered HTML is out of scope. Update those sections to distinguish
EmailProvider's structured event port fromDefaultAuthEmailProvider/AuthEmailSink.
msalvatti
added a commit
that referenced
this pull request
Aug 9, 2026
…log (#138) DefaultAuthEmailProvider logged the rendered subject when a send failed, and the subject comes from AuthEmailCatalogue -- which is the host's. Putting the code in the subject is an ordinary product decision, not a misuse, so a host doing something entirely reasonable would have had this library copy their OTP or reset token into a log pipeline. The failure now records a library-owned event name and a generic delivery error. The transport's underlying cause stays out too: it is a Box<dyn Error> the host's sink built, so its text is as unconstrained as the subject was. Pinned with the repo's log_capture subscriber: a catalogue whose subject carries the OTP, a forced delivery failure, and assertions that neither the code, the recipient nor the sink's error text reaches the event. Raised by Copilot on #136, after that PR had already merged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reaches parity with nest-auth's
#88,#95and#96— the functional changes it took since the last alignment (#134here,#74there).An audit of the full delta found four functional PRs on that side; two were real gaps here, and two were the other side catching up:
#95tenant on everyIEmailProvidermethod#96DefaultAuthEmailProviderNoOpEmailProvider#88optionaltenantIdtenantId#94AuthRevocationService#89absoluteSessionLifetimeDays= 30Two of those deserve their reasoning stated, because the headline is misleading:
#88on nest-auth was fixing a bug this side never had.password-reset.service.tsresolved the tenant and then went on readingdto.tenantId, so with a resolver configured the reset flow ran under the tenant the caller asked for. This side already rebound the input to the resolved tenant on every flow. What was missing here was the other half:tenantIdwas required, so a deployment with aTenantIdResolverhad to send a value the server discards.#94this side was already ahead. nest-auth published the service to de-duplicate three copies of the same two Redis reads across its guards; this side never had the duplication —verify_access/verify_platform_accessconsultrv:{jti}and the epoch in one place, andverify_access_tokenis already public for a host bridging a realtime transport.Changes
The email port carries the tenant. Every
EmailProvidermethod now takes the account'stenant_idfirst, so a multi-tenant delivery channel can attribute and route each message rather than guessing from the recipient. A platform admin is cross-tenant and carries no tenant of its own, so MFA notices on that plane arrive under the reservedPLATFORM_EMAIL_TENANT("platform"), held byte-identical to nest-auth's constant and pinned by a test.Important
Breaking for a hand-written
EmailProvider— and that is the outcome to want here. nest-auth documented that in TypeScript an implementation with fewer parameters stays assignable, so an old provider keeps compiling while passing the recipient where the tenant now goes. Rust refuses to compile it. Nothing is published on crates.io or npm yet, so no released consumer breaks.DefaultAuthEmailProvider. Sends throughAuthEmailSink, a one-method delivery port, so a deployment does not hand-write ten methods to send a verification code. It ships the policy each of those hand-written providers would otherwise have to get right independently:DeliveryErrorPolicy::Rethrowrestores the error for the two flows that react to one.The copy is fully replaceable through
AuthEmailCatalogue(ten renderers, each with a secure default). The escaping, the subject sanitization and the failure policy are not — a catalogue chooses words, never behaviour.Both message types redact their bodies in
Debug. A rendered body is where the reset token, the verification OTP and the invitation token actually sit in the clear. A sink logging its input at debug — which is what one does while wiring a new adapter — would otherwise put every one of them into a log pipeline.OutgoingEmailandAuthEmailMessageredact both bodies and mask the recipient, with a test.tenantIdis optional wherever aTenantIdResolvercan supply it. A configured resolver already ignores the body's value, so a client under one may now omit the field entirely. With no resolver the body is the only thing that can scope the request, and one naming no tenant is refused withauth.validationnaming the field rather than defaulted: inventing a tenant name would silently gather into one scope every account a misconfigured deployment created, and that scope keys the user lookup, the Redis records and the HMAC identifiers built from it.The reset helpers take the resolved tenant as an argument instead of reading it back off the input struct. With both values in scope, reading the wrong one is a one-character mistake no test would notice — and it is exactly the mistake nest-auth had to fix on this same flow. Not carrying the unresolved value is what makes it unwritable rather than merely unlikely.
Verification
cargo fmt --all --checkcargo clippy --workspace --all-targets --all-features -- -D warningsllvm-covlinesllvm-covfunctionsllvm-covregionsdefault_email.rs,email.rsanddto.rsare at 100%cargo mutants --in-diffTwo coverage misses appeared on the first run — both the repo's known multi-line
else { return; }trap, and both caused by these edits pushing rustfmt to wrap those lines. Rewritten to the single-line-else idiom.Of the four initial mutation survivors: two were dead code (a "defensive"
tenant_id: Nonenobody read) and were deleted rather than excluded inmutants.toml; two were real, pre-existing test gaps — nothing asserted the NIST §4.6 "your password changed" notice was ever sent — and are now covered, each confirmed by a red-check.PLATFORM_EMAIL_TENANTis deliberately not added toconformance/wire-contract.json: that file is byte-identical between the two repos and nest-auth did not add it. It is pinned by a unit test instead, and adding it to both is a follow-up for the nest-auth side.