Skip to content

feat: add admin endpoint to send email announcements - #2236

Open
dragosp1011 wants to merge 5 commits into
mainfrom
dragos/int1-688-email-announcements
Open

feat: add admin endpoint to send email announcements#2236
dragosp1011 wants to merge 5 commits into
mainfrom
dragos/int1-688-email-announcements

Conversation

@dragosp1011

Copy link
Copy Markdown
Contributor

Context

  • fixes #

Changes

  • Add opt-in admin endpoint POST /admin/notifications/email (enabled only when ADMIN_NOTIFICATION_SECRET is set), gated by x-admin-secret
  • Support configurable subject/body, with either an explicit recipient list (for preview/testing) or sendToAll: true for all verified users
  • Validate explicit recipients are registered users; reject unknown addresses before sending
  • Send via SendGrid personalizations (batched), with per-recipient fallback and failedRecipients returned for manual retry
  • Wire env/Helm/.env.example config

@github-actions github-actions Bot added package: wallet/backend Wallet backend implementations type: test Improvements or additions to tests type: source Source changes labels Sep 7, 2026
@adrianboros
adrianboros requested a lite review from Copilot September 7, 2026 16:05

Copilot AI 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.

🟡 Changes recommended

There are confirmed issues in the new announcement email template and security/behavior concerns in the recipient resolution and secret-checking logic that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds an opt-in wallet-backend admin endpoint to send email announcements, gated by ADMIN_NOTIFICATION_SECRET and x-admin-secret, with SendGrid batching support and Helm/env wiring so it can be enabled safely in deployed environments.

Changes:

  • Introduces /admin/notifications/email (only registered when ADMIN_NOTIFICATION_SECRET is set) with request validation for sendToAll vs explicit recipients.
  • Implements EmailService.sendAnnouncementBatch() using SendGrid personalizations with per-recipient fallback and failure reporting.
  • Adds controller/service unit tests and wires new configuration into .env.example and the wallet Helm chart.
File summaries
File Description
packages/wallet/backend/tests/admin-notification/service.test.ts Adds service-level tests for recipient resolution and SendGrid batch/fallback behavior.
packages/wallet/backend/tests/admin-notification/controller.test.ts Adds controller + middleware tests, including “disabled when secret missing” behavior.
packages/wallet/backend/src/user/service.ts Adds getVerifiedUserEmails() to support broadcasting to verified users.
packages/wallet/backend/src/middleware/isAdminSecret.ts Adds admin-secret middleware to gate the endpoint.
packages/wallet/backend/src/email/templates/announcementEmail.ts Adds HTML template for announcement emails.
packages/wallet/backend/src/email/service.ts Adds batched announcement sending via SendGrid personalizations and fallback sending.
packages/wallet/backend/src/createContainer.ts Conditionally registers the admin notification controller/service when enabled.
packages/wallet/backend/src/config/env.ts Adds ADMIN_NOTIFICATION_SECRET env parsing/normalization.
packages/wallet/backend/src/app.ts Wires the new admin route behind the middleware and env flag.
packages/wallet/backend/src/admin-notification/validation.ts Adds request body validation schema and limits (e.g., max body length).
packages/wallet/backend/src/admin-notification/service.ts Implements recipient resolution, validation, and send orchestration.
packages/wallet/backend/src/admin-notification/controller.ts Adds controller handler that validates input and returns success responses.
packages/wallet/backend/.env.example Documents the new env var used to enable the endpoint.
helm/testnet-wallet/values.yaml Wires ADMIN_NOTIFICATION_SECRET as an optional secret-driven env var in Helm.
Review details
  • Files reviewed: 14/14 changed files
  • Comments generated: 5
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/wallet/backend/src/admin-notification/service.ts
Comment thread packages/wallet/backend/src/admin-notification/service.ts Outdated
Comment thread packages/wallet/backend/src/email/service.ts
Comment thread packages/wallet/backend/src/email/templates/announcementEmail.ts Outdated
Comment thread packages/wallet/backend/src/middleware/isAdminSecret.ts

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

@dragosp1011

This might be scope creep so I'm just commenting

  • Perhaps a parameter to add a regex to filter email addresses to target. This way it can be easy to test with a specific domain like @interledger.foundation
  • AI flagged that there is no "unsubscribe" link, but I think thats fine
  • Consider adding a mechanism to prevent accidental repeating of call. You don't want to send announcements twice by accident
  • Consider making the response body return the full list of email addresses that will be targeted, and adding a dry run more.

Copilot AI 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.

🔵 Needs a closer look

There are security/maintainability issues that should be addressed before approval (notably HTML injection risk in the new announcement email template).

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

packages/wallet/backend/src/email/service.ts:160

  • sendAnnouncementBatch sends emails synchronously in a loop (with delays) on the request path; for large recipient counts this can keep the HTTP request open for a long time, increase memory/connection pressure, and risk reverse-proxy/app timeouts. Consider queuing the send work (e.g., background job) and returning 202 with a job id/status endpoint instead of doing the full send inline.
    packages/wallet/backend/src/email/templates/announcementEmail.ts:26
  • The announcement email template injects subject directly into HTML without escaping, so a subject containing <, &, quotes, etc. can break the markup or inject unintended HTML into email clients. Escaping the subject before interpolation keeps the email structure predictable.
    packages/wallet/backend/src/user/service.ts:62
  • getByEmails is typed as returning Promise<User[]>, but the query selects only the email column, so callers may assume other User fields are populated when they aren't. Either return a narrower type or fetch full rows; given this method is public, returning full User records avoids future misuse.
  • Files reviewed: 14/14 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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

Labels

package: wallet/backend Wallet backend implementations type: source Source changes type: test Improvements or additions to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants