fix smtp mail sending #26
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EmailService
participant SMTPClient
User->>EmailService: send_email()
EmailService->>SMTPClient: connect(email_smtp_port)
SMTPClient->>SMTPClient: starttls()
alt STARTTLS not supported
SMTPClient-->>EmailService: SMTPNotSupportedError
else STARTTLS supported
SMTPClient->>SMTPClient: login(from_username)
SMTPClient->>User: Email sent successfully
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
| @@ -60,13 +60,18 @@ def email_task( | |||
| try: | |||
| # 是否使用ssl | |||
| if email_use_ssl: | |||
| client = smtplib.SMTP_SSL(email_smtp_host) | |||
| client = smtplib.SMTP_SSL(email_smtp_host, email_smtp_port) | |||
There was a problem hiding this comment.
SMTP_SSL and SMTP constructors already do connect() inside. port must be given before that connect call.
| # 开启DEBUG模式 | ||
| client.set_debuglevel(0) | ||
| client.login(from_address, email_password) | ||
| client.login(from_username, email_password) |
There was a problem hiding this comment.
this should be the smtp auth user
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/tasks/email.py (1)
Line range hint
81-92: Add logging for better error trackingWhile the error handling is comprehensive, adding logging would help with debugging and monitoring in production.
Consider adding logging:
+import logging + +logger = logging.getLogger(__name__) + def email_task(...): try: # ... existing code ... except smtplib.SMTPConnectError as e: + logger.error(f"SMTP connection error: {e.smtp_code} - {e.smtp_error}") return "发送失败，连接失败:", str(e.smtp_code), str(e.smtp_error) # Add similar logging for other exceptions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/tasks/email.py (1 hunks)
🔇 Additional comments (3)
app/tasks/email.py (3)
63-63: LGTM: Explicit port specification improves reliabilityThe addition of
email_smtp_portparameter to SMTP constructors ensures consistent port usage regardless of the server configuration.Also applies to: 65-65
66-71: Review STARTTLS implementation and security implicationsWhile the error handling for STARTTLS is correct, there are a few considerations:
- The commented out
ehlo_or_helo_if_needed()should either be implemented or removed, as it's typically required after STARTTLS.- Silently continuing without STARTTLS could expose credentials over an unencrypted connection.
Consider this improved implementation:
if not email_use_ssl: try: client.starttls() - # client.ehlo_or_helo_if_needed() + client.ehlo() # Required after STARTTLS except smtplib.SMTPNotSupportedError: - pass + if celery.conf.app_config.get('EMAIL_REQUIRE_TLS', True): + raise # Don't proceed without encryption if TLS is required
74-74: LGTM: Correct use of from_username for authenticationUsing
from_usernamefor SMTP authentication is the right approach. The error handling for authentication failures is also properly implemented.Let's verify the username validation:
✅ Verification successful
Let me check how the from_username is validated and used in the email sending logic.
Let me check the email task implementation to see how from_username is handled.
LGTM: Username validation is properly implemented
The code correctly handles the
from_usernameparameter:
- Falls back to
EMAIL_USERNAMEconfig iffrom_usernameis None- Uses the username for both SMTP authentication and email "From" header
- Configuration validation is done via the
ENABLE_USER_EMAILcheck🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if from_username is properly validated before use rg -g '!*.pyc' -g '!*test*' "from_username.*=.*None" -A 5Length of output: 590
Script:
#!/bin/bash # Check email sending implementation for username validation and usage rg -g '!*.pyc' -g '!*test*' "def.*send.*mail" -A 20 app/tasks/email.pyLength of output: 598
Script:
#!/bin/bash # Check email task implementation rg -g '!*.pyc' -g '!*test*' "@celery.*task.*email" -A 30 app/tasks/email.pyLength of output: 1248
Summary by CodeRabbit
New Features
Bug Fixes
Improvements