From dc7f74cc10ba414d56f066553e3f8becbabcc621 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Thu, 2 Jul 2026 16:35:26 +0200 Subject: [PATCH 1/3] fix(user): don't regenerate token before notification email is confirmed 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 --- nextcloudappstore/user/signals.py | 30 +++++++++++++++------ nextcloudappstore/user/tests.py | 43 +++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/nextcloudappstore/user/signals.py b/nextcloudappstore/user/signals.py index 8863273b8db..5ba58a69b2b 100644 --- a/nextcloudappstore/user/signals.py +++ b/nextcloudappstore/user/signals.py @@ -8,6 +8,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.contrib.auth.models import User +from django.core.exceptions import ValidationError from django.core.mail import send_mail from django.db.models.signals import post_save, pre_save from django.dispatch import receiver @@ -30,7 +31,11 @@ def post_save_create_token(sender, **kwargs): @receiver(pre_save, sender=get_user_model()) def password_changed_signal(sender, instance, **kwargs): """ - Regenerate token on password change + Notify the user by email and regenerate their token on password change. + This is a pre_save hook: raising here aborts the save, so the password + change is only persisted once the notification email is confirmed sent. + If it can't be sent (e.g. a mail outage), we'd rather block the change + than silently persist it without notifying the user. :param sender: :param instance: :param kwargs: @@ -41,14 +46,23 @@ def password_changed_signal(sender, instance, **kwargs): try: user = get_user_model().objects.get(pk=instance.pk) - if user.password != instance.password and instance._password is not None: - # make sure we only send an email when user changed their - # password and _password is set and not when password hash - # was changed due to a new default hashing algorithm - update_token(user.username) - send_mail(mail_subect, mail_message, settings.DEFAULT_FROM_EMAIL, [user.email], False) except User.DoesNotExist: - pass + return + + if user.password == instance.password or instance._password is None: + # only act when the user changed their password and _password is + # set, not when the password hash changed due to a new default + # hashing algorithm + return + + try: + send_mail(mail_subect, mail_message, settings.DEFAULT_FROM_EMAIL, [user.email], False) + except Exception as exc: + raise ValidationError( + _("Could not send the password change notification email. Your password was not changed.") + ) from exc + + update_token(user.username) @receiver(email_confirmed) diff --git a/nextcloudappstore/user/tests.py b/nextcloudappstore/user/tests.py index 27a76780d02..aaeb13e1779 100644 --- a/nextcloudappstore/user/tests.py +++ b/nextcloudappstore/user/tests.py @@ -9,9 +9,11 @@ from allauth.core import ratelimit from django.contrib.auth import get_user_model from django.core.cache import cache +from django.core.exceptions import ValidationError from django.test import RequestFactory, TestCase, override_settings from django.urls import reverse from email_validator import validate_email as _real_validate_email +from rest_framework.authtoken.models import Token from nextcloudappstore.user.facades import create_user @@ -184,3 +186,44 @@ def test_budget_shared_with_allauth_email_view(self): email_url = reverse("account_email") r = self.client.post(email_url, {"email": "viaemail@nextcloud.com", "action_add": ""}) self.assertEqual(r.status_code, 429) + + +class PasswordChangedSignalTest(TestCase): + def setUp(self): + self.user = create_user(username="pwuser", password=PASSWORD, email=PRIMARY_EMAIL) + + def test_password_change_persists_when_email_succeeds(self): + old_hash = self.user.password + self.user.set_password("New-Sup3rS3cret!") + self.user.save() + + refreshed = get_user_model().objects.get(pk=self.user.pk) + self.assertNotEqual(old_hash, refreshed.password) + + def test_password_change_not_persisted_when_email_fails(self): + old_hash = self.user.password + self.user.set_password("New-Sup3rS3cret!") + + with patch("nextcloudappstore.user.signals.send_mail", side_effect=Exception("SMTP down")): + with self.assertRaises(ValidationError): + self.user.save() + + refreshed = get_user_model().objects.get(pk=self.user.pk) + self.assertEqual(old_hash, refreshed.password) + + def test_token_not_regenerated_when_email_fails(self): + old_token = Token.objects.get(user=self.user).key + self.user.set_password("New-Sup3rS3cret!") + + with patch("nextcloudappstore.user.signals.send_mail", side_effect=Exception("SMTP down")): + with self.assertRaises(ValidationError): + self.user.save() + + self.assertEqual(old_token, Token.objects.get(user=self.user).key) + + def test_no_email_sent_when_password_unchanged(self): + with patch("nextcloudappstore.user.signals.send_mail") as mock_send_mail: + self.user.first_name = "Changed" + self.user.save() + + mock_send_mail.assert_not_called() From 5ca90d9a1669948afcec29cf9e845cdbdf65cba1 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Thu, 2 Jul 2026 16:48:47 +0200 Subject: [PATCH 2/3] fix(user): show a friendly warning instead of a crash on password save 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 --- nextcloudappstore/urls.py | 6 +++ nextcloudappstore/user/tests.py | 87 +++++++++++++++++++++++++++++++++ nextcloudappstore/user/views.py | 24 +++++++++ 3 files changed, 117 insertions(+) diff --git a/nextcloudappstore/urls.py b/nextcloudappstore/urls.py index 16a9c079646..cbe79a89063 100644 --- a/nextcloudappstore/urls.py +++ b/nextcloudappstore/urls.py @@ -28,6 +28,7 @@ AppScaffoldingView, IntegrationScaffoldingView, ) +from nextcloudappstore.user.views import PasswordResetFromKeyView admin.site.login = login_required(admin.site.login) @@ -36,6 +37,11 @@ path("featured", CategoryAppListView.as_view(), {"id": None, "is_featured_category": True}, name="featured"), path("signup/", csp_update(**settings.CSP_SIGNUP)(signup), name="account_signup"), path("social/signup/", csp_update(**settings.CSP_SIGNUP)(social_signup), name="socialaccount_signup"), + re_path( + r"^password/reset/key/(?P[0-9A-Za-z]+)-(?P.+)/$", + PasswordResetFromKeyView.as_view(), + name="account_reset_password_from_key", + ), path("", include("allauth.urls")), re_path(r"^categories/(?P[\w]*)/?$", CategoryAppListView.as_view(), name="category-app-list"), re_path(r"^developer/apps/generate/?$", AppScaffoldingView.as_view(), name="app-scaffold"), diff --git a/nextcloudappstore/user/tests.py b/nextcloudappstore/user/tests.py index aaeb13e1779..40750a74788 100644 --- a/nextcloudappstore/user/tests.py +++ b/nextcloudappstore/user/tests.py @@ -3,11 +3,13 @@ SPDX-License-Identifier: AGPL-3.0-or-later """ +import re from unittest.mock import patch from allauth.account.models import EmailAddress from allauth.core import ratelimit from django.contrib.auth import get_user_model +from django.core import mail from django.core.cache import cache from django.core.exceptions import ValidationError from django.test import RequestFactory, TestCase, override_settings @@ -227,3 +229,88 @@ def test_no_email_sent_when_password_unchanged(self): self.user.save() mock_send_mail.assert_not_called() + + +class PasswordChangeViewFriendlyErrorTest(TestCase): + """The logged-in /account/password/ form must show a warning, not crash, + when password_changed_signal aborts the save.""" + + def setUp(self): + self.user = create_user(username="pwchangeuser", password=PASSWORD, email=PRIMARY_EMAIL) + self.client.login(username="pwchangeuser", password=PASSWORD) + self.url = reverse("user:account-password") + + def _post(self, new_password="New-Sup3rS3cret!"): + return self.client.post( + self.url, + { + "oldpassword": PASSWORD, + "password1": new_password, + "password2": new_password, + }, + ) + + def test_change_persists_when_email_succeeds(self): + response = self._post() + self.assertEqual(response.status_code, 302) + refreshed = get_user_model().objects.get(pk=self.user.pk) + self.assertTrue(refreshed.check_password("New-Sup3rS3cret!")) + + def test_change_shows_warning_instead_of_crashing_when_email_fails(self): + with patch("nextcloudappstore.user.signals.send_mail", side_effect=Exception("SMTP down")): + response = self._post() + + self.assertEqual(response.status_code, 200) + shown_messages = [str(m) for m in response.context["messages"]] + self.assertTrue(any("not changed" in m for m in shown_messages)) + refreshed = get_user_model().objects.get(pk=self.user.pk) + self.assertTrue(refreshed.check_password(PASSWORD)) + + +class PasswordResetFromKeyViewFriendlyErrorTest(TestCase): + """The forgot-password flow must show a warning, not crash, when + password_changed_signal aborts the save.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls._validate_patcher = patch( + "nextcloudappstore.user.adapters.validate_email", + side_effect=_validate_email_no_dns, + ) + cls._validate_patcher.start() + + @classmethod + def tearDownClass(cls): + cls._validate_patcher.stop() + super().tearDownClass() + + def setUp(self): + cache.clear() + self.user = create_user(username="resetuser", password=PASSWORD, email=PRIMARY_EMAIL) + + def _get_set_password_url(self): + self.client.post(reverse("account_reset_password"), {"email": PRIMARY_EMAIL}) + body = mail.outbox[-1].body + match = re.search(r"https?://[^\s]+(/password/reset/key/\S+?)/?(?:\s|$)", body) + response = self.client.get(match.group(1) + "/", follow=True) + self.assertEqual(response.status_code, 200) + return response.redirect_chain[-1][0] + + def test_reset_persists_when_email_succeeds(self): + url = self._get_set_password_url() + response = self.client.post(url, {"password1": "New-Sup3rS3cret!", "password2": "New-Sup3rS3cret!"}) + self.assertEqual(response.status_code, 302) + refreshed = get_user_model().objects.get(pk=self.user.pk) + self.assertTrue(refreshed.check_password("New-Sup3rS3cret!")) + + def test_reset_shows_warning_instead_of_crashing_when_email_fails(self): + url = self._get_set_password_url() + with patch("nextcloudappstore.user.signals.send_mail", side_effect=Exception("SMTP down")): + response = self.client.post(url, {"password1": "New-Sup3rS3cret!", "password2": "New-Sup3rS3cret!"}) + + self.assertEqual(response.status_code, 200) + shown_messages = [str(m) for m in response.context["messages"]] + self.assertTrue(any("not changed" in m for m in shown_messages)) + refreshed = get_user_model().objects.get(pk=self.user.pk) + self.assertTrue(refreshed.check_password(PASSWORD)) diff --git a/nextcloudappstore/user/views.py b/nextcloudappstore/user/views.py index 0f237f6defc..164311d167a 100644 --- a/nextcloudappstore/user/views.py +++ b/nextcloudappstore/user/views.py @@ -5,10 +5,12 @@ from allauth.account.models import EmailAddress from allauth.account.views import PasswordChangeView +from allauth.account.views import PasswordResetFromKeyView as AllauthPasswordResetFromKeyView from allauth.core import ratelimit from django.contrib import messages from django.contrib.auth import logout from django.contrib.auth.mixins import LoginRequiredMixin +from django.core.exceptions import ValidationError from django.db.models import Q from django.http import HttpResponseRedirect from django.shortcuts import get_object_or_404, redirect, render @@ -193,12 +195,34 @@ class PasswordView(LoginRequiredMixin, PasswordChangeView): template_name = "user/password.html" success_url = reverse_lazy("user:account-password") + def form_valid(self, form): + try: + return super().form_valid(form) + except ValidationError as exc: + # Raised by password_changed_signal when the change couldn't be + # persisted (e.g. the notification email failed to send). Show + # it as a warning instead of an unhandled error page. + messages.error(self.request, " ".join(exc.messages)) + return self.form_invalid(form) + def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) context["acc_page"] = "password" return context +class PasswordResetFromKeyView(AllauthPasswordResetFromKeyView): + """Same as allauth's view, but shows a friendly warning instead of an + unhandled error page if password_changed_signal aborts the save.""" + + def form_valid(self, form): + try: + return super().form_valid(form) + except ValidationError as exc: + messages.error(self.request, " ".join(exc.messages)) + return self.form_invalid(form) + + @method_decorator(never_cache, name="dispatch") class APITokenView(LoginRequiredMixin, TemplateView): """Display the user's API token, and allow it to be regenerated.""" From 128f189f7e14864825e3f17757f2092ff4d5fca3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:50:47 +0000 Subject: [PATCH 3/3] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- nextcloudappstore/user/views.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nextcloudappstore/user/views.py b/nextcloudappstore/user/views.py index 164311d167a..83bbc9e11b1 100644 --- a/nextcloudappstore/user/views.py +++ b/nextcloudappstore/user/views.py @@ -5,7 +5,9 @@ from allauth.account.models import EmailAddress from allauth.account.views import PasswordChangeView -from allauth.account.views import PasswordResetFromKeyView as AllauthPasswordResetFromKeyView +from allauth.account.views import ( + PasswordResetFromKeyView as AllauthPasswordResetFromKeyView, +) from allauth.core import ratelimit from django.contrib import messages from django.contrib.auth import logout