Skip to content

feat(settings): SMTP e-mail configuration and credential notifications - #400

Open
vhervatin wants to merge 1 commit into
Paca-AI:masterfrom
vhervatin:feat/smtp-email-config
Open

feat(settings): SMTP e-mail configuration and credential notifications#400
vhervatin wants to merge 1 commit into
Paca-AI:masterfrom
vhervatin:feat/smtp-email-config

Conversation

@vhervatin

Copy link
Copy Markdown
Contributor

Summary

Adds an admin-only E-mail configuration section under workspace settings so operators can wire up an SMTP server from the UI, and optional transactional e-mails when a user account is created or a password is reset.

Everything is off until configured, and disabled installs behave exactly as before.

Backend

  • migration 000037 — SMTP columns on workspace_settings + nullable users.email
  • migration 000038 — opt-in smtp_skip_verify flag
  • platform/mail — a small net/smtp sender (implicit TLS / STARTTLS / plain) and a multipart/alternative message builder with an HTML→text fallback
  • service/email — settings CRUD (password encrypted at rest, never returned to the client), test-send, and best-effort user-created / password-reset notifications
  • endpointsGET/PATCH /admin/settings/email, POST /admin/settings/email/test (all behind settings.write)
  • user create / reset-password hooks deliver credentials when enabled and configured (best-effort; account creation never fails because e-mail failed)

Frontend

  • E-mail configuration form (SMTP fields, SSL/TLS switches, send on user creation toggle, send test e-mail button) and an e-mail field on user creation
  • i18n across all 9 locales

Security notes

  • The SMTP password is stored encrypted (AES-256-GCM) and is never sent back to the client; the API only reports whether one is set.
  • smtp_skip_verify is off by default. It exists only for shared mail hosting that presents a wildcard certificate not matching the SMTP hostname (Go's x509 hostname check otherwise fails). The admin turns it on knowingly, behind an in-UI "insecure" warning.

Testing

  • go build ./..., go test ./internal/platform/mail/... ./internal/service/email/... ./internal/transport/http/... — green
  • Frontend tsc -b, biome check, and vitest (admin-api) — green
  • Manually verified end-to-end against a live SMTP server (send test + create-user credential e-mail), including the skip-verify path on a mismatched-certificate host

🤖 Generated with Claude Code

…tions

Adds an admin-only "E-mail configuration" section under workspace settings
so operators can wire up an SMTP server directly from the UI, plus optional
transactional e-mails when accounts are created or passwords are reset.

Backend
- migration 000037: SMTP columns on workspace_settings + users.email
- migration 000038: opt-in smtp_skip_verify flag
- platform/mail: minimal net/smtp sender (implicit TLS / STARTTLS / plain),
  multipart/alternative message builder with HTML→text fallback
- service/email: settings CRUD (password encrypted at rest, never returned),
  test-send, and best-effort user-created / password-reset notifications
- endpoints: GET/PATCH /admin/settings/email, POST /admin/settings/email/test
- user create/reset hooks send credentials when enabled and configured

Frontend
- E-mail configuration form (SMTP fields, SSL/TLS, send-on-create toggle,
  send-test button) and an e-mail field on user creation
- i18n across all 9 locales

The password is stored encrypted (AES-256-GCM) and never sent back to the
client. smtp_skip_verify is off by default and exists only for shared mail
hosting that presents a wildcard certificate not matching the SMTP hostname;
the admin enables it knowingly with an in-UI insecure warning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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

Important

Two issues in the new SMTP sender are worth addressing before merging — a plaintext TLS-downgrade path for credential e-mails and an unbounded SMTP conversation inside synchronous HTTP handlers. Details inline and below.

Reviewed changes

  • SMTP sender (platform/mail) — new net/smtp client supporting implicit TLS / STARTTLS / plain, plus a multipart/alternative message builder with an HTML→text fallback. Password never appears in error or log strings.
  • E-mail service (service/email) — CRUD for SMTP settings with the password AES-256-GCM encrypted at rest and never returned to the client, a test-send, and best-effort user-created / password-reset notifications gated behind the send_user_created_email toggle.
  • HTTP surfaceGET|PATCH /admin/settings/email and POST /admin/settings/email/test, all behind settings.write; user_handler wires the notifier into create-user and reset-password without ever failing those flows.
  • Data layer — migrations 000037 (SMTP cols + nullable users.email) and 000038 (smtp_skip_verify); email now required on the admin create-user endpoint.
  • Frontend — e-mail configuration form, an e-mail field on user creation, and i18n across all 9 locales.

⚠️ SMTP conversation has no deadline after the dial

The 20s Timeout bounds only connection establishment (dial: net.Dialer{Timeout: ...} / tls.DialWithDialer). Once smtp.NewClient returns, no deadline is set on the connection, so the Mail / Rcpt / Data / Quit steps can block indefinitely, and the request context is never tied to the connection. Because all three send paths run synchronously inside HTTP handlers — create-user (user_handler.go), reset-password, and send-test-email (email_settings_handler.go) — a server that accepts the TCP connection and then stalls holds those requests open and exhausts handler goroutines/connections, with no cancellation escape. User creation itself can hang for an unbounded time because a misbehaving SMTP server never answers EHLO.

Technical details
# SMTP conversation has no deadline

## Affected sites
- services/api/internal/platform/mail/mailer.go:90,114-130 — Send() dials with `timeout` but the
  subsequent Extension/StartTLS/Auth/Mail/Rcpt/Data/Write/Quit steps have no deadline; the only
  deadline in the file is net.Dialer's.
- services/api/internal/transport/http/handler/user_handler.go:236-241 (create), :334-347 (reset)
- services/api/internal/transport/http/handler/email_settings_handler.go:103-113 (test-send)

## Required outcome
- Bound the whole SMTP round-trip, not just the connect: set `client.SetDeadline(time.Now().Add(timeout))`
  (or SetRead/SetWriteDeadline) after dialing and re-arm after StartTLS, and/or honor the caller's
  context so a cancelled HTTP request aborts the in-flight send. The notification is documented as
  best-effort — it should never be able to pin a request open for longer than the configured bound.
Technical details
# STARTTLS downgrade details

## Affected sites
- services/api/internal/platform/mail/mailer.go:99-105 — `ok, _ := client.Extension("STARTTLS")`
  drops the error and silently skips TLS when the server doesn't advertise STARTTLS.

## Required outcome
- When `UseTLS` (or `UseSSL`) is set and TLS cannot be negotiated, return an explicit error
  ("server does not advertise STARTTLS") instead of continuing in plaintext, so an admin who
  enabled TLS is never silently downgraded. Optionally also reject after `client.StartTLS` when
  `cfg.tlsConfig().InsecureSkipVerify` is false but the negotiated TLS state isn't confirmed.

ℹ️ Breaking change: email is now required on admin create-user

CreateUserRequest.Email is binding:"required,email" plus a handler-level looksLikeEmail guard, so any existing integration that created users without an e-mail now receives 400. This is a deliberate part of the feature, but worth confirming that no first-party path (e.g. seed scripts, plugin/webhook user provisioning) relies on creating users without an e-mail.

ℹ️ Nitpicks

  • The e-mail form cannot clear a stored SMTP password: blank keeps it, and there's no explicit "clear" action even though the API (""nil) supports it. A stored credential can never be removed from the UI.
  • email_sent: false is overloaded — the backend returns false both when the send failed and when e-mail is simply disabled/unconfigured (a no-op). The frontend TypeScript comment describes it as "sending was on but failed", which is inaccurate; the field isn't currently consumed, so this is only a doc nit.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +99 to +105
if cfg.UseTLS && !cfg.UseSSL {
if ok, _ := client.Extension("STARTTLS"); ok {
if err := client.StartTLS(cfg.tlsConfig()); err != nil {
return fmt.Errorf("mail: starttls: %w", err)
}
}
}

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.

When UseTLS is set but the server doesn't advertise STARTTLS, the ok, _ result is silently dropped and the send continues over the plaintext connection. With an unauthenticated relay (empty Username) the message body — including the freshly-created user's plaintext password for credential e-mails — is transmitted in the clear; when a username is set, Go's net/smtp.PlainAuth happens to fail-closed with "unencrypted connection", but that's incidental to this path. When the admin explicitly enabled TLS, failing with an explicit error is the safer contract than silently downgrading.

@pikann

pikann commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Hi @vhervatin

Great work on this PR! 🚀 This feature is super useful and definitely something we've been needing. Thank you for putting so much effort into implementing the whole flow end-to-end ( backend, frontend, security, and i18n).

That said, since Paca is designed to be as flexible as possible for many different teams, I think we should consider implementing this via a plugin architecture. Different teams might prefer using services like AWS SES, Mailgun, or SendGrid instead of a raw SMTP server.

What do you think about:

  • Keeping the core change (like adding the email field to the users table).
  • Extracting the SMTP logic into an SMTP plugin?

This way, core Paca stays lightweight, and other teams can easily write/use plugins for SES, SendGrid, etc., in the future.

Thanks again for the awesome contribution!

@pikann

pikann commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

By the way, feel free to connect with me on LinkedIn (linkedin.com/in/pikann22) if you’d like to discuss this further or brainstorm implementation details!

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.

2 participants