diff --git a/README.md b/README.md index 33eae4c03b..0059138155 100644 --- a/README.md +++ b/README.md @@ -108,7 +108,7 @@ To work on admin's JavaScript or CSS, `cd` to `misago-admin` and install depende ### E-mails -Misago uses [Mailpit](https://github.com/axllent/mailpit) to capture emails sent from development instance. +Misago uses [Mailpit](https://github.com/axllent/mailpit) to capture emails sent from the development instance. To browse those emails, visit the in your browser for the web interface. diff --git a/misago/account/emailchange.py b/misago/account/emailchange.py index c3551f1049..9024164daf 100644 --- a/misago/account/emailchange.py +++ b/misago/account/emailchange.py @@ -2,10 +2,12 @@ import hashlib from base64 import urlsafe_b64decode, urlsafe_b64encode from datetime import datetime, timedelta +from enum import StrEnum from typing import TYPE_CHECKING from django.conf import settings from django.utils import timezone +from django.utils.translation import pgettext if TYPE_CHECKING: from ..users.models import User @@ -13,30 +15,72 @@ TIMESTAMP_FORMAT = "%Y%m%d%H%S" +class EmailChangeTokenErrorCode(StrEnum): + PAYLOAD_MISSING = "PAYLOAD-MISSING" + SIGNATURE_INVALID = "SIGNATURE-INVALID" + SIGNATURE_MISSING = "SIGNATURE-MISSING" + TOKEN_EXPIRED = "TOKEN-EXPIRED" + TOKEN_INVALID = "TOKEN-INVALID" + + class EmailChangeTokenError(ValueError): - pass + code: EmailChangeTokenErrorCode + + def __init__(self, code: EmailChangeTokenErrorCode): + self.code = code + + def __str__(self): + if self.code == EmailChangeTokenErrorCode.PAYLOAD_MISSING: + return pgettext( + "email change token error", + "Mail change confirmation link is missing a payload.", + ) + if self.code == EmailChangeTokenErrorCode.TOKEN_EXPIRED: + return pgettext( + "email change token error", + "Mail change confirmation link has expired.", + ) + if self.code == EmailChangeTokenErrorCode.TOKEN_INVALID: + return pgettext( + "email change token error", + "Mail change confirmation link is invalid.", + ) + if self.code == EmailChangeTokenErrorCode.SIGNATURE_INVALID: + return pgettext( + "email change token error", + "Mail change confirmation link has invalid signature.", + ) + if self.code == EmailChangeTokenErrorCode.SIGNATURE_MISSING: + return pgettext( + "email change token error", + "Mail change confirmation link is missing a signature.", + ) + + return self.code.value def create_email_change_token(user: "User", new_email: str) -> str: timestamp = timezone.now().strftime(TIMESTAMP_FORMAT) - message = urlsafe_b64encode(f"{timestamp}:{new_email}".encode("utf-8")) - signature = get_email_change_signature(user, message) + payload = urlsafe_b64encode(f"{timestamp}:{new_email}".encode("utf-8")) + signature = get_email_change_signature(user, payload) - return f"{signature}-{message.decode('utf-8')}" + return f"{signature}-{payload.decode('utf-8')}" def read_email_change_token(user: "User", token: str) -> str: if "-" not in token: - raise EmailChangeTokenError("missing signature or message") + raise EmailChangeTokenError(EmailChangeTokenErrorCode.TOKEN_INVALID) - signature, message = token.split("-", 1) - if not signature or not message: - raise EmailChangeTokenError("missing signature or message") + signature, payload = token.split("-", 1) + if not signature: + raise EmailChangeTokenError(EmailChangeTokenErrorCode.SIGNATURE_MISSING) + if not payload: + raise EmailChangeTokenError(EmailChangeTokenErrorCode.PAYLOAD_MISSING) - if signature != get_email_change_signature(user, message.encode("utf-8")): - raise EmailChangeTokenError("invalid signature for message") + if signature != get_email_change_signature(user, payload.encode("utf-8")): + raise EmailChangeTokenError(EmailChangeTokenErrorCode.SIGNATURE_INVALID) - timestamp, email = urlsafe_b64decode(message).decode("utf-8").split(":", 1) + timestamp, email = urlsafe_b64decode(payload).decode("utf-8").split(":", 1) created = datetime.strptime(timestamp, TIMESTAMP_FORMAT).replace( tzinfo=timezone.utc @@ -44,7 +88,7 @@ def read_email_change_token(user: "User", token: str) -> str: expires = created + timedelta(hours=settings.MISAGO_EMAIL_CHANGE_TOKEN_EXPIRES) if timezone.now() > expires: - raise EmailChangeTokenError("token expired") + raise EmailChangeTokenError(EmailChangeTokenErrorCode.TOKEN_EXPIRED) return email @@ -55,5 +99,5 @@ def get_email_change_secret(user: "User") -> bytes: ).encode("utf-8") -def get_email_change_signature(user: "User", message: bytes) -> str: - return hmac.new(get_email_change_secret(user), message, hashlib.sha256).hexdigest() +def get_email_change_signature(user: "User", payload: bytes) -> str: + return hmac.new(get_email_change_secret(user), payload, hashlib.sha256).hexdigest() diff --git a/misago/account/forms.py b/misago/account/forms.py index c4338feb1e..e0d1870638 100644 --- a/misago/account/forms.py +++ b/misago/account/forms.py @@ -10,6 +10,7 @@ from ..permissions.accounts import allow_delete_own_account from ..profile.profilefields import profile_fields +from ..users.utils import hash_email from ..users.validators import validate_email, validate_username from .namechanges import get_available_username_changes @@ -339,7 +340,7 @@ def clean_current_password(self): def clean_new_email(self): data = self.cleaned_data["new_email"] - if data == self.instance.email: + if hash_email(data) == self.instance.email_hash: raise forms.ValidationError( pgettext( "account email form", diff --git a/misago/account/tests/test_account_email.py b/misago/account/tests/test_account_email.py new file mode 100644 index 0000000000..d8166b0c30 --- /dev/null +++ b/misago/account/tests/test_account_email.py @@ -0,0 +1,124 @@ +from django.urls import reverse + +from ...conf.test import override_dynamic_settings +from ...test import assert_contains + + +@override_dynamic_settings(enable_oauth2_client=True) +def test_account_email_returns_error_if_oauth_client_is_enabled(db, client): + response = client.get(reverse("misago:account-email")) + assert response.status_code == 404 + + +def test_account_email_returns_error_for_guests(db, client): + response = client.get(reverse("misago:account-email")) + assert_contains(response, "You need to be signed in", status_code=403) + + +def test_account_email_renders_form(user_client): + response = user_client.get(reverse("misago:account-email")) + assert_contains(response, "Change email address") + + +def test_account_email_form_sends_email_confirmation_link_on_change( + user_client, user_password, mailoutbox +): + response = user_client.post( + reverse("misago:account-email"), + { + "current_password": user_password, + "new_email": "new@example.com", + "confirm_email": "new@example.com", + }, + ) + assert response.status_code == 302 + + assert len(mailoutbox) == 1 + + +def test_account_email_form_redirects_to_message_page_on_change( + user_client, user_password +): + response = user_client.post( + reverse("misago:account-email"), + { + "current_password": user_password, + "new_email": "new@example.com", + "confirm_email": "new@example.com", + }, + ) + assert response.status_code == 302 + + response = user_client.get(response.headers["location"]) + assert_contains(response, "Confirm email address change") + + +def test_account_email_form_validates_current_password(user_client): + response = user_client.post( + reverse("misago:account-email"), + { + "current_password": "invalid", + "new_email": "new@example.com", + "confirm_email": "new@example.com", + }, + ) + assert response.status_code == 200 + assert_contains(response, "Change email address") + assert_contains(response, "Password is incorrect.") + + +def test_account_email_form_validates_new_email(user_client, user_password): + response = user_client.post( + reverse("misago:account-email"), + { + "current_password": user_password, + "new_email": "invalid", + "confirm_email": "invalid", + }, + ) + assert response.status_code == 200 + assert_contains(response, "Change email address") + assert_contains(response, "Enter a valid email address.") + + +def test_account_email_form_validates_email_is_new(user, user_client, user_password): + response = user_client.post( + reverse("misago:account-email"), + { + "current_password": user_password, + "new_email": user.email, + "confirm_email": user.email, + }, + ) + assert response.status_code == 200 + assert_contains(response, "This email address is the same as the current one.") + + +def test_account_email_form_validates_new_emails_match(user_client, user_password): + response = user_client.post( + reverse("misago:account-email"), + { + "current_password": user_password, + "new_email": "new@example.com", + "confirm_email": "other@example.com", + }, + ) + assert response.status_code == 200 + assert_contains(response, "Change email address") + assert_contains(response, "New email addresses don't match.") + + +def test_account_email_form_displays_message_page_on_change_in_htmx( + user_client, user_password +): + response = user_client.post( + reverse("misago:account-email"), + { + "current_password": user_password, + "new_email": "new@example.com", + "confirm_email": "new@example.com", + }, + headers={"hx-request": "true"}, + ) + assert response.status_code == 200 + assert_contains(response, "Confirm email address change") diff --git a/misago/account/tests/test_account_password.py b/misago/account/tests/test_account_password.py index e5d5497b6d..fda6bdaee1 100644 --- a/misago/account/tests/test_account_password.py +++ b/misago/account/tests/test_account_password.py @@ -1,7 +1,7 @@ from django.urls import reverse from ...conf.test import override_dynamic_settings -from ...test import assert_contains, assert_has_success_message, assert_not_contains +from ...test import assert_contains, assert_has_success_message @override_dynamic_settings(enable_oauth2_client=True) diff --git a/misago/account/tests/test_email_change_token.py b/misago/account/tests/test_email_change_token.py index 234da23385..642aa49625 100644 --- a/misago/account/tests/test_email_change_token.py +++ b/misago/account/tests/test_email_change_token.py @@ -3,6 +3,7 @@ from ..emailchange import ( EmailChangeTokenError, + EmailChangeTokenErrorCode, create_email_change_token, read_email_change_token, ) @@ -24,7 +25,16 @@ def test_email_change_token_is_invalidated_by_user_email_change(user): with pytest.raises(EmailChangeTokenError) as exc_info: read_email_change_token(user, token) - assert exc_info.value.args[0] == "invalid signature for message" + assert str(exc_info.value) == "Mail change confirmation link has invalid signature." + assert exc_info.value.code == EmailChangeTokenErrorCode.SIGNATURE_INVALID + + +def test_read_email_change_token_raises_error_if_token_is_invalid(user): + with pytest.raises(EmailChangeTokenError) as exc_info: + read_email_change_token(user, "invalid") + + assert str(exc_info.value) == "Mail change confirmation link is invalid." + assert exc_info.value.code == EmailChangeTokenErrorCode.TOKEN_INVALID def test_email_change_token_is_invalidated_by_user_password_change(user): @@ -36,7 +46,8 @@ def test_email_change_token_is_invalidated_by_user_password_change(user): with pytest.raises(EmailChangeTokenError) as exc_info: read_email_change_token(user, token) - assert exc_info.value.args[0] == "invalid signature for message" + assert str(exc_info.value) == "Mail change confirmation link has invalid signature." + assert exc_info.value.code == EmailChangeTokenErrorCode.SIGNATURE_INVALID def test_email_change_token_is_invalidated_by_secret_key_change(user): @@ -46,35 +57,34 @@ def test_email_change_token_is_invalidated_by_secret_key_change(user): with pytest.raises(EmailChangeTokenError) as exc_info: read_email_change_token(user, token) - assert exc_info.value.args[0] == "invalid signature for message" - - -def test_read_email_change_token_raises_error_if_token_is_invalid(user): - with pytest.raises(EmailChangeTokenError) as exc_info: - read_email_change_token(user, "invalid") - - assert exc_info.value.args[0] == "missing signature or message" + assert str(exc_info.value) == "Mail change confirmation link has invalid signature." + assert exc_info.value.code == EmailChangeTokenErrorCode.SIGNATURE_INVALID -def test_read_email_change_token_raises_error_if_token_misses_signature(user): +def test_read_email_change_token_raises_error_if_token_is_missing_signature(user): with pytest.raises(EmailChangeTokenError) as exc_info: read_email_change_token(user, "-invalid") - assert exc_info.value.args[0] == "missing signature or message" + assert ( + str(exc_info.value) == "Mail change confirmation link is missing a signature." + ) + assert exc_info.value.code == EmailChangeTokenErrorCode.SIGNATURE_MISSING -def test_read_email_change_token_raises_error_if_token_misses_message(user): +def test_read_email_change_token_raises_error_if_token_is_missing_payload(user): with pytest.raises(EmailChangeTokenError) as exc_info: read_email_change_token(user, "invalid-") - assert exc_info.value.args[0] == "missing signature or message" + assert str(exc_info.value) == "Mail change confirmation link is missing a payload." + assert exc_info.value.code == EmailChangeTokenErrorCode.PAYLOAD_MISSING def test_read_email_change_token_raises_error_if_token_signature_is_invalid(user): with pytest.raises(EmailChangeTokenError) as exc_info: - read_email_change_token(user, "invalid-message") + read_email_change_token(user, "invalid-payload") - assert exc_info.value.args[0] == "invalid signature for message" + assert str(exc_info.value) == "Mail change confirmation link has invalid signature." + assert exc_info.value.code == EmailChangeTokenErrorCode.SIGNATURE_INVALID @override_settings(MISAGO_EMAIL_CHANGE_TOKEN_EXPIRES=0) @@ -84,4 +94,5 @@ def test_read_email_change_token_raises_error_if_token_is_expired(user): with pytest.raises(EmailChangeTokenError) as exc_info: read_email_change_token(user, token) - assert exc_info.value.args[0] == "token expired" + assert str(exc_info.value) == "Mail change confirmation link has expired." + assert exc_info.value.code == EmailChangeTokenErrorCode.TOKEN_EXPIRED diff --git a/misago/account/urls.py b/misago/account/urls.py index efc59a63b5..c784c8289d 100644 --- a/misago/account/urls.py +++ b/misago/account/urls.py @@ -30,6 +30,16 @@ settings.AccountEmailView.as_view(), name="account-email", ), + path( + "email/confirm/", + settings.AccountEmailConfirm.as_view(), + name="account-email-confirm-sent", + ), + path( + "email/confirm///", + settings.account_email_confirm_change, + name="account-email-confirm-change", + ), path( "download-data/", settings.AccountDownloadDataView.as_view(), diff --git a/misago/account/views/settings.py b/misago/account/views/settings.py index 320de18f1c..daa840fda7 100644 --- a/misago/account/views/settings.py +++ b/misago/account/views/settings.py @@ -4,9 +4,10 @@ from django.contrib import messages from django.contrib.auth import get_user_model, logout from django.core.exceptions import PermissionDenied -from django.forms import Form +from django.forms import Form, ValidationError from django.http import Http404, HttpRequest, HttpResponse from django.shortcuts import get_object_or_404, redirect, render +from django.urls import reverse from django.utils.translation import gettext as _, pgettext, pgettext_lazy from django.views import View @@ -19,6 +20,7 @@ from ...users.models import DataDownload from ...users.online.tracker import clear_tracking from ...users.tasks import delete_user +from ...users.validators import validate_email from ..forms import ( AccountDeleteForm, AccountDetailsForm, @@ -329,7 +331,7 @@ def handle_valid_form(self, request: HttpRequest, form: Form) -> HttpResponse: ) request.session["misago_new_email"] = form.cleaned_data["new_email"] - return redirect(request.path_info) + return redirect(reverse("misago:account-email-confirm-sent")) def save_form(self, request: HttpRequest, form: Form) -> None: new_email = form.cleaned_data["new_email"] @@ -355,39 +357,57 @@ def save_form(self, request: HttpRequest, form: Form) -> None: mail.send(fail_silently=True) -def account_email_change_confirm_sent(request): - new_email = request.session.pop("misago_new_email", None) - if not new_email: - raise Http404() +class AccountEmailConfirm(AccountSettingsView): + def dispatch(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse: + if request.settings.enable_oauth2_client: + raise Http404() - return render( - request, - "misago/account/settings/email_confirm_sent.html", - {"new_email": new_email}, - ) + return super().dispatch(request, *args, **kwargs) + def get(self, request: HttpRequest) -> HttpResponse: + new_email = request.session.pop("misago_new_email", None) + if not new_email: + return redirect(reverse("misago:account-email")) + + return self.render( + request, + "misago/account/settings/email_confirm.html", + {"new_email": new_email}, + ) -def account_email_change_confirm(request, user_id, token): + +def account_email_confirm_change(request, user_id, token): user = get_object_or_404(User.objects, id=user_id, is_active=True) try: new_email = read_email_change_token(user, token) - except EmailChangeTokenError: - raise NotImplementedError("TODO") - - # TODO: verify user is not banned - # TODO: verify new email is still valid + validate_email(new_email, user) + except EmailChangeTokenError as e: + return render( + request, + "misago/account/settings/email_change_error.html", + { + "message": str(e), + "error_code": str(e.code), + }, + status=400, + ) + except ValidationError as e: + return render( + request, + "misago/account/settings/email_change_error.html", + {"message": e.error_list[0]}, + status=400, + ) if new_email != user.email: user.set_email(new_email) user.save() - # TODO: write template - # TODO: write tests return render( request, "misago/account/settings/email_changed.html", - {"username": user.username}, + {"username": user.username, "new_email": new_email}, ) diff --git a/misago/templates/misago/account/settings/email_change_error.html b/misago/templates/misago/account/settings/email_change_error.html new file mode 100644 index 0000000000..0f06513178 --- /dev/null +++ b/misago/templates/misago/account/settings/email_change_error.html @@ -0,0 +1,33 @@ +{% extends "misago/base.html" %} +{% load i18n %} + + +{% block title %}{% trans "Confirm email change" context "account email change page" %} | {{ block.super }}{% endblock %} + + +{% block content %} +
+
+
+ +
+ error_outline +
+ +
+

+ {% trans "Couldn't confirm email change" context "account email change error page" %} +

+

{{ message }}

+ {% if error_code == "SIGNATURE_INVALID" %} +

+ {% blocktrans trimmed context "account email change error page" %} + Link's signature will also become invalid when valid link is used for the first time. + {% endblocktrans %} +

+ {% endif %} +
+
+
+
+{% endblock content %} diff --git a/misago/templates/misago/account/settings/email_changed.html b/misago/templates/misago/account/settings/email_changed.html new file mode 100644 index 0000000000..c3d23687dd --- /dev/null +++ b/misago/templates/misago/account/settings/email_changed.html @@ -0,0 +1,38 @@ +{% extends "misago/base.html" %} +{% load i18n misago_capture %} + + +{% block title %}{% trans "Confirm email change" context "account email change page" %} | {{ block.super }}{% endblock %} + + +{% block content %} +
+
+
+ +
+ check +
+ +
+

+ {% blocktrans trimmed with username=username new_email=new_email context "account email changed page" %} + {{ username }}, your email has been changed to {{ new_email }} + {% endblocktrans %} +

+

+ {% if user.is_authenticated %} + + {% trans "Go to account settings" context "account email changed page" %} + + {% endif %} + + {% trans "Go to home page" context "account email changed page" %} + +

+
+ +
+
+
+{% endblock content %} diff --git a/misago/templates/misago/account/settings/email_confirm.html b/misago/templates/misago/account/settings/email_confirm.html new file mode 100644 index 0000000000..2f26793a0a --- /dev/null +++ b/misago/templates/misago/account/settings/email_confirm.html @@ -0,0 +1,10 @@ +{% extends "misago/account/settings/base.html" %} +{% load i18n %} + + +{% block title %}{% trans "Email address" context "account email page" %} | {{ block.super }}{% endblock %} + + +{% block page %} +{% include "misago/account/settings/email_form_completed.html" %} +{% endblock page %} \ No newline at end of file diff --git a/misago/templates/misago/emails/email_confirm_change.html b/misago/templates/misago/emails/email_confirm_change.html index 69234508e4..b9a2a3de6d 100644 --- a/misago/templates/misago/emails/email_confirm_change.html +++ b/misago/templates/misago/emails/email_confirm_change.html @@ -4,17 +4,19 @@ {% block content %} {% blocktrans trimmed with user=user context "email confirm change email" %} -{{ user }}, you are receiving this message because you are changing your e-mail address. + {{ user }}, you are receiving this message because you are changing your e-mail address. {% endblocktrans %}

-{% trans "Confirm change" context "email confirm change email cta" %} + + {% trans "Confirm change" context "email confirm change email cta" %} +

{% blocktrans trimmed count expires_in=expires_in context "email confirm change email" %} -This link will remain active for {{ expires_in }} hour from the time this message has been sent. + This link will remain active for {{ expires_in }} hour from the time this message has been sent. {% plural %} -This link will remain active for {{ expires_in }} hours from the time this message has been sent. + This link will remain active for {{ expires_in }} hours from the time this message has been sent. {% endblocktrans %}
{% endblock content %} diff --git a/misago/templates/misago/emails/email_confirm_change.txt b/misago/templates/misago/emails/email_confirm_change.txt index 4fa97547c5..03c58bdd1a 100644 --- a/misago/templates/misago/emails/email_confirm_change.txt +++ b/misago/templates/misago/emails/email_confirm_change.txt @@ -4,17 +4,17 @@ {% block content %} {% blocktrans trimmed with user=user context "email confirm change email" %} -{{ user }}, you are receiving this message because you are changing your e-mail address. + {{ user }}, you are receiving this message because you are changing your e-mail address. {% endblocktrans %} {% blocktrans trimmed context "email confirm change email" %} -To confirm the change, click the following link: + To confirm the change, click the following link: {% endblocktrans %} -{{ settings.forum_address }}{{ token }} +{% absoluteurl 'misago:account-email-confirm-change' user_id=user.id token=token %} {% blocktrans trimmed count expires_in=expires_in context "email confirm change email" %} -This link will remain active for {{ expires_in }} hour from the time this message has been sent. + This link will remain active for {{ expires_in }} hour from the time this message has been sent. {% plural %} -This link will remain active for {{ expires_in }} hours from the time this message has been sent. + This link will remain active for {{ expires_in }} hours from the time this message has been sent. {% endblocktrans %} {% endblock content %}