Skip to content

Add system email notifications and password recovery#178

Open
Atlas-SZ wants to merge 5 commits intodataelement:mainfrom
Atlas-SZ:main
Open

Add system email notifications and password recovery#178
Atlas-SZ wants to merge 5 commits intodataelement:mainfrom
Atlas-SZ:main

Conversation

@Atlas-SZ
Copy link
Contributor

@Atlas-SZ Atlas-SZ commented Mar 24, 2026

Summary

This PR adds platform-owned email delivery for password recovery and optional company broadcast emails.

What Changed

  • Added env-driven system SMTP configuration
  • Added forgot-password and reset-password backend endpoints
  • Added single-use password reset tokens with persistence
  • Added frontend forgot-password and reset-password pages
  • Added optional email delivery for company broadcast notifications
  • Localized the new password recovery and broadcast email UI for English and Simplified Chinese
  • Hardened email delivery so SMTP latency or failures do not block the main request path

Validation

  • cd backend && .venv/bin/python -m pytest tests/test_password_reset_and_notifications.py
  • cd backend && .venv/bin/python -m ruff check app/api/auth.py app/api/notification.py app/services/system_email_service.py tests/test_password_reset_and_notifications.py
  • cd frontend && npm run build
  • Manual local validation with Docker PostgreSQL/Redis
  • Manual SMTP validation with a real 163 mailbox
  • Manual browser E2E for reset-password -> login
  • Manual browser validation for English and Simplified Chinese UI copy

Notes

  • SMTP configuration is environment-variable based only
  • emails_sent in broadcast responses now means queued recipients, not guaranteed final SMTP success
  • Background email delivery is in-process, not a durable queue

Risk

  • Background email tasks can still be lost if the server process exits immediately after the response
  • Other languages were intentionally left unchanged; this PR only updates English and Simplified Chinese

@yaojin3616
Copy link
Collaborator

yaojin3616 commented Mar 26, 2026

PR 178 评审意见：

  1. 安全性：密码重置 Token 采用了哈希存储、单次使用及过期机制，符合安全规范。
  2. 性能：邮件发送采用异步 + BackgroundTasks，避免了网络延迟对响应速度的影响。
  3. 扩展性：组织架构同步服务重构为多 Provider 模式（飞书 + 企微），架构清晰。
  4. 完整性：包含了数据库迁移、前后端实现、国际化及单元测试，代码质量很高。

建议 (LGTM)：

  • 提醒管理员在生产环境配置 PUBLIC_BASE_URL，以确保重置链接有效。
  • 未来可考虑将 SMTP 超时时间配置化。

Atlas-SZ added a commit to Atlas-SZ/Clawith that referenced this pull request Mar 26, 2026
@Atlas-SZ
Improve production readiness of password reset email delivery
36297d9 
Address PR dataelement#178 review follow-ups by making system SMTP timeout configurable and clarifying PUBLIC_BASE_URL expectations for production reset links.\n\nConstraint: Keep SMTP env-driven and avoid introducing queue infrastructure or behavior changes to auth responses\nRejected: Leaving SMTP timeout hardcoded at 15s | blocks operator tuning for slow providers\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Keep reset-link generation on PUBLIC_BASE_URL/public_base_url and avoid localhost values in production\nTested: backend/.venv/bin/python -m pytest tests/test_password_reset_and_notifications.py\nTested: backend/.venv/bin/python -m ruff check app/config.py app/services/system_email_service.py app/services/password_reset_service.py tests/test_password_reset_and_notifications.py\nNot-tested: live SMTP provider behavior under high latency
@Atlas-SZ
Copy link
Contributor Author

Atlas-SZ commented Mar 26, 2026

已根据建议完成两项改进并推送：

  • 新增 SYSTEM_SMTP_TIMEOUT_SECONDS，可配置 SMTP 超时（默认 15s）
  • README/.env.example 明确生产环境必须配置 PUBLIC_BASE_URL（公网 HTTPS）

验证：

  • backend/.venv/bin/python -m pytest tests/test_password_reset_and_notifications.py
  • backend/.venv/bin/python -m ruff check app/config.py app/services/system_email_service.py app/services/password_reset_service.py tests/
    test_password_reset_and_notifications.py

@wisdomqin
Copy link
Contributor

wisdomqin commented Mar 26, 2026

Thanks for this comprehensive PR! The password recovery flow and system email service are exactly what we need.

However, this PR currently bundles two independent features together:

  1. System email + password recovery (the main feature)
  2. WeCom org sync (org_sync_service.py, add_wecom_org_sync_fields.py, etc.)

Since the WeCom org sync is already covered separately in #183, could you please split this PR into two?

This way we can review and merge each feature independently without risk of conflicts.

Additionally, since you've already built the SMTP infrastructure and email verification token pattern, it would be great if you could also add email verification during registration in the same PR (or a follow-up):

  • When a user registers with email, send a verification code/link
  • User must verify their email before the account becomes active
  • This reuses the same system_email_service.py and token pattern you've already implemented

This would complete the email lifecycle: registration verification + password recovery + broadcast notifications.

Looking forward to the updated PR!

@Atlas-SZ
Copy link
Contributor Author

Atlas-SZ commented Mar 27, 2026

很抱歉造成了混淆，应该是我本地代码提交导致的混乱，我将彻底拆分两个独立功能分别对应不同 issues 进行提交。

Atlas-SZ added 4 commits March 27, 2026 08:56
@Atlas-SZ
Add platform email delivery for password recovery and broadcast notices
be33d48 
This change adds a platform-owned SMTP delivery path and wires it into
forgot-password plus optional enterprise broadcast email delivery.
The reset flow now issues single-use database-backed tokens, exposes
public reset endpoints, and adds frontend pages for requesting and
consuming reset links. The README and env example now document the
required SMTP and public URL configuration for local and production
setups.

Constraint: SMTP configuration must come from environment variables rather than admin-managed secrets
Constraint: Forgot-password responses must not reveal whether an email address exists
Rejected: Reusing per-agent email tooling | wrong trust boundary for system-owned auth mail
Rejected: Stateless reset JWTs | harder to revoke and audit than DB-backed single-use tokens
Confidence: high
Scope-risk: moderate
Reversibility: clean
Directive: Do not move reset-link generation away from PUBLIC_BASE_URL without verifying frontend route compatibility
Tested: backend pytest tests/test_password_reset_and_notifications.py
Tested: frontend npm run build
Tested: manual local validation of forgot-password, reset-password, and SMTP delivery with Docker PostgreSQL/Redis
Not-tested: full end-to-end browser automation for clicking the emailed reset link
@Atlas-SZ
Localize password recovery and broadcast email UI copy
ca3c2f0 
This change removes English-only fallback text from the new password
recovery flow and broadcast email controls so the feature matches the
existing bilingual frontend behavior. The new auth pages now read from
i18n, and both English and Simplified Chinese dictionaries include the
new strings.

Constraint: New user-facing copy must follow the existing frontend i18n structure instead of introducing page-local string tables
Rejected: Leaving default English fallbacks in component code | inconsistent with the rest of the localized UI
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Add future auth and enterprise UI copy to locale files at the same time as the component change
Tested: frontend npm run build
Tested: manual browser check of /forgot-password Chinese rendering
Not-tested: full language-switch regression across all newly added strings
@Atlas-SZ
Prevent SMTP latency from blocking password recovery and broadcasts
e8203b8 
This change moves system email delivery off the main request path for
forgot-password and broadcast email sends. Password recovery now queues
email delivery after persisting the reset token, and broadcast email
sends are isolated per recipient so one SMTP failure does not abort the
entire operation.

Constraint: Keep the current env-driven SMTP configuration and avoid adding queue infrastructure
Constraint: Requests must remain responsive even when SMTP is slow or misconfigured
Rejected: Leave SMTP on the request path | risks slow or stalled user-facing requests
Rejected: Add a durable mail queue now | too much scope for a targeted hardening pass
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Treat emails_sent in broadcast responses as queued recipients, not guaranteed SMTP success
Tested: backend pytest tests/test_password_reset_and_notifications.py
Tested: backend ruff check app/api/auth.py app/api/notification.py app/services/system_email_service.py tests/test_password_reset_and_notifications.py
Tested: manual browser E2E reset-password -> login flow after the hardening change
Not-tested: process-crash behavior between response return and background email completion
@Atlas-SZ
Improve production readiness of password reset email delivery
d1f2a0b 
Address PR dataelement#178 review follow-ups by making system SMTP timeout configurable and clarifying PUBLIC_BASE_URL expectations for production reset links.\n\nConstraint: Keep SMTP env-driven and avoid introducing queue infrastructure or behavior changes to auth responses\nRejected: Leaving SMTP timeout hardcoded at 15s | blocks operator tuning for slow providers\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Keep reset-link generation on PUBLIC_BASE_URL/public_base_url and avoid localhost values in production\nTested: backend/.venv/bin/python -m pytest tests/test_password_reset_and_notifications.py\nTested: backend/.venv/bin/python -m ruff check app/config.py app/services/system_email_service.py app/services/password_reset_service.py tests/test_password_reset_and_notifications.py\nNot-tested: live SMTP provider behavior under high latency
@Atlas-SZ
Keep password reset migration on the mainline Alembic chain
c15b8f0 
The password reset migration was anchored to an older branch point, which created a second Alembic head alongside add_daily_token_usage. Reattach it to the current mainline so upgrades remain linear for PR dataelement#178.\n\nConstraint: Preserve the password reset schema change without introducing a merge migration in this PR\nRejected: Add a separate Alembic merge revision | extra migration noise for a single isolated table addition\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: New migrations in feature PRs must be rebased onto the current Alembic head before review\nTested: cd backend && .venv/bin/alembic heads\nTested: cd backend && .venv/bin/python -m pytest tests/test_password_reset_and_notifications.py\nNot-tested: full backend migration upgrade/downgrade cycle on a populated database
@Atlas-SZ
Copy link
Contributor Author

Atlas-SZ commented Mar 27, 2026

本次提交只涉及“忘记密码” 功能，不再涉及其他功能。

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Atlas-SZ @yaojin3616 @wisdomqin