Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use smtp to send email #4339

Closed
wants to merge 6 commits into from
Closed

fix: use smtp to send email #4339

wants to merge 6 commits into from

Conversation

AnoyiX
Copy link

@AnoyiX AnoyiX commented May 13, 2024

Description

邀请用户,发送注册邮件时,邮件一直无法发出,经过本地代码测试排查,使用 smtplib 发送邮件,会卡主,同样的 SMTP 配置,使用 aiosmtplib 能正常发送。SMTP 配置的是飞书邮箱。

另外,在部署时,dify-api 在 MODE 为 api 模式时,配置以下参数是无效的,故删除:

      # Mail configuration, support: resend, smtp
      MAIL_TYPE: ''
      # default send from email address, if not specified
      MAIL_DEFAULT_SEND_FROM: 'YOUR EMAIL FROM (eg: no-reply <no-reply@dify.ai>)'
      SMTP_SERVER: ''
      SMTP_PORT: 587
      SMTP_USERNAME: ''
      SMTP_PASSWORD: ''
      SMTP_USE_TLS: 'true'
      # the api-key for resend (https://resend.com)
      RESEND_API_KEY: ''
      RESEND_API_URL: https://api.resend.com

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update, included: Dify Document
  • Improvement, including but not limited to code refactoring, performance optimization, and UI/UX improvement
  • Dependency upgrade

How Has This Been Tested?

使用 aiosmtplib 发送邮件的测试代码:

import asyncio
from email.message import EmailMessage

import aiosmtplib

message = EmailMessage()
message["From"] = "root@localhost"
message["To"] = "somebody@example.com"
message["Subject"] = "Hello World!"
message.set_content("Sent via aiosmtplib")

asyncio.run(aiosmtplib.send(message, hostname="127.0.0.1", port=25))

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods
  • optional I have made corresponding changes to the documentation
  • optional I have added tests that prove my fix is effective or that my feature works
  • optional New and existing unit tests pass locally with my changes

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. 🐞 bug Something isn't working labels May 13, 2024
@takatost
Copy link
Collaborator

aiosmtplib is the asynchronous version of smtplib, but Dify's current flask/celery doesn't support asynchronous.
Have you investigated the reason for this stuck?

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels May 14, 2024
@AnoyiX
Copy link
Author

AnoyiX commented May 14, 2024

aiosmtplib is the asynchronous version of smtplib, but Dify's current flask/celery doesn't support asynchronous. Have you investigated the reason for this stuck?

已经知道问题所在了,如果 SMTP 服务器使用 SSL,使用 smtplib.SMTP() 初始化的时候会卡主,需要使用 smtplib.SMTP_SSL() 初始化。

@chazzhou
Copy link
Contributor

I was able to reproduce this issue with an Implicit TLS server on port 465. Changing the SMTP initialization to the following code resolved the problem:

if self._use_tls:
    smtp = smtplib.SMTP_SSL(self.server, self.port, timeout=10)
else:
    smtp = smtplib.SMTP(self.server, self.port, timeout=10)

However, this might cause issues for Explicit TLS servers using port 587. To address this, we may need an additional flag to distinguish between these scenarios. Since custom ports can be used, it's not always clear whether the connection should use STARTTLS or Implicit TLS.

ref rfc8314

self.server = server
self.port = port
self.port = int(port)
self._from = _from
self.username = username
self.password = password
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like the assignment of self._use_tls is missing here.

Copy link
Author

Choose a reason for hiding this comment

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

self._use_tls = use_tls is after self.password = password, no changed

@chazzhou
Copy link
Contributor

@takatost Do you have any thoughts on how to distinguish between Explicit and Implicit TLS? Implementing current fixes may cause issues for Explicit TLS servers, as they require STARTTLS implementation. However, Explicit TLS is deprecated and potentially insecure, so Dify might consider not supporting it at all.

@AnoyiX AnoyiX closed this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants