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

Failure to send a password reset email should not 500 #19754

Open
alexmv opened this issue Sep 14, 2021 · 4 comments
Open

Failure to send a password reset email should not 500 #19754

alexmv opened this issue Sep 14, 2021 · 4 comments

Comments

@alexmv
Copy link
Collaborator

alexmv commented Sep 14, 2021

The password reset form calls send_email synchronously:

zulip/zerver/forms.py

Lines 347 to 376 in e7c62c4

if user is not None:
context["active_account_in_realm"] = True
context["reset_url"] = generate_password_reset_url(user, token_generator)
send_email(
"zerver/emails/password_reset",
to_user_ids=[user.id],
from_name=FromAddress.security_email_from_name(user_profile=user),
from_address=FromAddress.tokenized_no_reply_address(),
context=context,
realm=realm,
request=request,
)
else:
context["active_account_in_realm"] = False
active_accounts_in_other_realms = UserProfile.objects.filter(
delivery_email__iexact=email, is_active=True
)
if active_accounts_in_other_realms:
context["active_accounts_in_other_realms"] = active_accounts_in_other_realms
language = request.LANGUAGE_CODE
send_email(
"zerver/emails/password_reset",
to_emails=[email],
from_name=FromAddress.security_email_from_name(language=language),
from_address=FromAddress.tokenized_no_reply_address(),
language=language,
context=context,
realm=realm,
request=request,
)

This means that if that emails fails to send, it raises an EmailNotDeliveredException:

try:
# This will call .open() for us, which is a no-op if it's already open;
# it will only call .close() if it was not open to begin with
if connection.send_messages([mail]) == 0:
logger.error("Unknown error sending %s email to %s", template, mail.to)
raise EmailNotDeliveredException
except smtplib.SMTPResponseException as e:
logger.exception(
"Error sending %s email to %s with error code %s: %s",
template,
mail.to,
e.smtp_code,
e.smtp_error,
stack_info=True,
)
raise EmailNotDeliveredException
except smtplib.SMTPException as e:
logger.exception(
"Error sending %s email to %s: %s", template, mail.to, str(e), stack_info=True
)
raise EmailNotDeliveredException

We should catch those in the password reset form, and display a message that says something like:

Something went wrong sending an email to that address. Double-check that you typed it correctly, and try again in a few minutes!

@zulipbot
Copy link
Member

Hello @zulip/server-development members, this issue was labeled with the "area: emails" label, so you may want to check it out!

@timabbott
Copy link
Sponsor Member

Hmm, I'm not sure what precise semantics we want here. I think if the failure indicates that the server is unable to send outgoing emails at all, we do want a 500 error. (One thing to consider is that the signup/password reset flows are often where self-hosted administrators might be when they are first trying to get email working).

So we perhaps want the behavior to depend on what type of exception we get when trying to send the email?

@alexmv
Copy link
Collaborator Author

alexmv commented Sep 16, 2021

SMTP error codes are an unregulated mess, which makes them very hard to act on. For instance, a failure we recently saw in production was:

454 Temporary service failure

...and code 454 is technically a transient failure of the receiver system, which is accurate here. But it's also used for declining to relay mail or needing authentication and those are just the first two hits that turn up on Google.

@alexmv
Copy link
Collaborator Author

alexmv commented Jan 27, 2022

This also applies to send_confirm_registration_email, which is also synchronous, and can lead to a 500.

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

No branches or pull requests

3 participants