Skip to content

fix(api): readable marketing email + live-DB admin check - #290

Merged
thomasluizon merged 1 commit into
mainfrom
fix/marketing-email-and-admin-dbcheck
Jul 6, 2026
Merged

fix(api): readable marketing email + live-DB admin check#290
thomasluizon merged 1 commit into
mainfrom
fix/marketing-email-and-admin-dbcheck

Conversation

@thomasluizon

Copy link
Copy Markdown
Owner

Summary

Follow-ups to the marketing-consent / code-sender work (#287), from real testing:

  • Marketing email was unreadable — the caller's body HTML rendered black-on-dark (the branded card is dark, the body had no text color). Now wrapped in a readable light-colored container.
  • Banned dashes in the footer — removed the em dash + en dash from the LTDA legal line.
  • Admin authorization now checks the DB live. Previously the Admin policy trusted a JWT admin claim minted at login — which meant revoking admin didn't take effect until the token expired. AdminAuthorizationHandler now reads User.IsAdmin from the DB on every request (immediate revocation, DB is the sole authority). The JWT carries identity only; the vestigial claim minting and AdminPolicy claim constants are removed.

Tests

AdminAuthorizationHandler unit tests (admin allowed / non-admin denied / missing id denied); JWT no longer contains an admin claim; auth session/token call-sites updated. Full suite green (Domain 492, Application 2602, Infrastructure 1387).

Refs thomasluizon/orbit-ui-mobile#397

🤖 Generated with Claude Code

Two marketing-email fixes and an admin-authorization hardening:
- Marketing email body was black-on-dark (invisible). Wrap the caller's body
  HTML in a readable light-colored container before the branded shell.
- Remove the em dash and en dash from the LTDA footer (banned in Orbit copy).
- Admin authorization now reads User.IsAdmin LIVE from the DB on every request
  (AdminAuthorizationHandler) instead of trusting a JWT claim, so revoking admin
  takes effect immediately rather than lingering until the token expires. The
  JWT carries identity only and no longer asserts admin; the now-vestigial claim
  minting + AdminPolicy claim constants are removed.

Refs thomasluizon/orbit-ui-mobile#397

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Jul 6, 2026

Copy link
Copy Markdown

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Complete

Scope: PR #290, thomasluizon/orbit-api, branch fix/marketing-email-and-admin-dbcheck
Recommendation: APPROVE

Severity Count
Critical (incl. ⚠️ old-client breaks) 0
High 0
Medium 0
Low / Info 1 (informational only, not a defect)

Summary

Two independent, small, well-scoped fixes: (1) the marketing-broadcast email body is now wrapped in a light-colored, legible <div> and the footer's em/en dashes are removed; (2) admin authorization moves from a JWT claim minted at login to a live IsAdmin DB check via a new AdminAuthorizationHandler, closing the "revoked admin still works until token expiry" gap. Both changes are internal (no DTO, endpoint, or packages/shared surface touched), so backward-compat and contract-drift checks don't apply. Tests were updated in lockstep across all call-sites removed (ITokenService, IAuthSessionService, AdminPolicy), and the new handler has direct unit-test coverage for all four branches (admin, non-admin, missing claim, unparseable claim).

Findings

Critical / High / Medium

None.

Low / Info

  • src/Orbit.Infrastructure/Services/ResendEmailService.cs:107-116SendMarketingEmailAsync wraps the admin-supplied bodyHtml in a new <div> without additional HTML encoding. Confirmed this is not a new trust-boundary change: EmailTemplateRenderer.RenderLayout already injected bodyHtml verbatim before this PR, and the content is gated behind [Authorize(Policy = AdminPolicy.Name)] on AdminController — trusted-author HTML, not user input. No action required.

Subagents

Agent Verdict
security-reviewer PASS
contract-aligner N/A — no DTO, Controller route, or packages/shared surface changed in this diff

Validation

Check Result
Build / Tests (dotnet) N/A — covered by required CI checks (Build / Unit Tests / SonarCloud)

What's good

  • Fail-closed authorization: AdminAuthorizationHandler never calls context.Succeed when the NameIdentifier claim is missing or fails Guid.TryParse, directly unit-tested including asserting the repository is never queried in those cases.
  • Full removal of the dead admin-claim path: AdminPolicy.ClaimType/ClaimValue deleted, and every call-site of ITokenService.GenerateToken / IAuthSessionService.CreateSessionAsync was updated in the same PR — no orphaned signatures.
  • The email-contrast fix is verifiably correct: the layout's dark canvas (EmailTemplateRenderer.CanvasColor) is paired with a light wrapper text color — genuinely fixes the reported black-on-dark unreadability.
  • Backward-compat guard: no DTO, endpoint, or packages/shared type changed, so there is no old-mobile-client break surface to guard here.

Recommendation

Merge as-is. No follow-ups required.

🤖 Generated with Claude Code

@thomasluizon
thomasluizon merged commit a2ba339 into main Jul 6, 2026
8 of 9 checks passed
@thomasluizon
thomasluizon deleted the fix/marketing-email-and-admin-dbcheck branch July 6, 2026 19:57
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