fix(user): password change silently lost when notification email fails to send#1808
Open
miaulalala wants to merge 3 commits into
Open
fix(user): password change silently lost when notification email fails to send#1808miaulalala wants to merge 3 commits into
miaulalala wants to merge 3 commits into
Conversation
…med sent password_changed_signal is a pre_save hook on User, so previously it called update_token() before send_mail() with no error handling. If send_mail() raised (e.g. a mail outage), the exception propagated out of the signal, aborting the save entirely -- so the password change was silently lost, but the API token had already been regenerated as a side effect. Reorder so the token is only regenerated once the notification email has actually been sent, and wrap send_mail() so a mail failure raises a clear ValidationError instead of a raw SMTP/socket exception. The save is still aborted on failure by design: users should not have their password changed without being notified. Signed-off-by: Anna Larch <anna@nextcloud.com>
…e failure password_changed_signal can now raise ValidationError (previous commit) when it can't notify the user by email, aborting the password save. Both places that call user.save() as part of a password change -- the logged-in /account/password/ form and the forgot-password /password/reset/key/... flow -- left that exception unhandled, so users hit a raw error page instead of a clear message. Override PasswordView.form_valid() and add a PasswordResetFromKeyView (wired ahead of allauth's own URL) that catch the ValidationError and re-render the form with a messages.error() warning instead. Signed-off-by: Anna Larch <anna@nextcloud.com>
for more information, see https://pre-commit.ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
password_changed_signal(nextcloudappstore/user/signals.py) is apre_savehook onUserthat regenerates the API token and emails the user whenever their password changes. It calledupdate_token()beforesend_mail(), with no error handling. Ifsend_mail()raised (e.g. a mail outage), the exception propagated out of thepre_savesignal and aborted the.save()entirely — so the password change was silently lost, even though the token had already been regenerated as a side effect./account/password/change form and the forgot-password/password/reset/key/...flow, since both ultimately calluser.save().send_mail()so a failure raises a clearValidationErrorinstead of a raw SMTP/socket exception. The save is still aborted on failure by design — a password should never change without the user being notified.PasswordView.form_valid()handling and a newPasswordResetFromKeyView(wired ahead of allauth's own URL foraccount_reset_password_from_key) so this failure now shows a friendly warning banner instead of an unhandled error page.Test plan
nextcloudappstore/user/tests.pycovering: password persists when mail succeeds, is rejected when mail fails, token isn't regenerated on failure, no mail sent for unrelated field changes, and both password-change views show a warning (not a crash) on failure while leaving the old password intactpoetry run python manage.py test nextcloudappstore.user nextcloudappstore.core nextcloudappstore.api.v1 --settings nextcloudappstore.settings.development --exclude-tag=e2e— 184 tests pass🤖 Generated with Claude Code