diff --git a/_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md b/_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md index 5e682d6..bce6eb1 100644 --- a/_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md +++ b/_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md @@ -4,7 +4,7 @@ type: 'feature' ticket: '1' epic: 'epic-account-self-service' created: '2026-10-18' -status: 'approved' +status: 'built' baseline_revision: 'ead5670' route: 'full' route_source: 'auto' @@ -102,3 +102,11 @@ context: - `uv run python manage.py makemigrations --check --dry-run` -- expected: no changes detected **Done checkpoint (you, on the Linux host against the test OU):** sign in as `ptest-user1`; the rules match the domain policy; a wrong current password shows the message; five wrong ones block the sixth without touching AD (check that `ptest-user1` is not locked out in AD); a too-short and a history-rejected password show the right messages; a good change works, a second session of the same user in another browser is signed out, and the confirmation mail arrives with the time and no password; `ptest-user1`'s new password works at authentik. Reset the test password afterwards with the test-environment script. + +## Implementation Notes + +- Built inline in the `profile` app. `pytest` 355 passed, 6 skipped; `ruff check` clean; `makemigrations --check` no changes (the app has no tables). +- Judgement calls: the rules are shown above all three fields (the mock-up put them under the new-password field) so they come first in reading order, as FR-5 asks. A form mistake re-reads the policy only to redisplay the rules; it never verifies or changes a password. `ngettext` is used for counts, so the translation test now reads Python with `ast` and checks plural entries. Cancelling and the done page link to the start page. German text added for formal and informal; the confirmation mail is German formal only. +- The mail is rendered once at queue time with the change time in the display zone; the body is removed from the database after sending (see the outbox). +- Not verified: the real directory (the done checkpoint below) and the browser look of the new form, spinner and error summary. +- Not covered by a test: a double submit from two tabs at once (the second attempt fails as a wrong current password, which is harmless). diff --git a/portal/config/settings.py b/portal/config/settings.py index 2a71c90..66f738a 100644 --- a/portal/config/settings.py +++ b/portal/config/settings.py @@ -35,6 +35,7 @@ INSTALLED_APPS = [ "portal.audit", "portal.accounts", "portal.shell", + "portal.profile", "portal.mailer", "portal.scheduler", ] diff --git a/portal/config/urls.py b/portal/config/urls.py index 6933f6e..8ef0658 100644 --- a/portal/config/urls.py +++ b/portal/config/urls.py @@ -6,4 +6,5 @@ urlpatterns = [ path("healthz", healthz, name="healthz"), path("", include("portal.accounts.urls")), path("", include("portal.shell.urls")), + path("", include("portal.profile.urls")), ] diff --git a/portal/locale/de/LC_MESSAGES/django.po b/portal/locale/de/LC_MESSAGES/django.po index 48a6ab1..9c4015b 100644 --- a/portal/locale/de/LC_MESSAGES/django.po +++ b/portal/locale/de/LC_MESSAGES/django.po @@ -113,3 +113,137 @@ msgstr "Test-E-Mail vom Portal" msgid "This is a test e-mail from the portal. If you can read it, sending e-mail works." msgstr "Das ist eine Test-E-Mail vom Portal. Wenn Sie sie lesen können, funktioniert der E-Mail-Versand." + +#, python-format +msgid "At least %(n)s character" +msgid_plural "At least %(n)s characters" +msgstr[0] "Mindestens %(n)s Zeichen" +msgstr[1] "Mindestens %(n)s Zeichen" + +msgid "Characters from at least three of these groups: upper case letters, lower case letters, digits and special characters" +msgstr "Zeichen aus mindestens drei dieser Gruppen: Großbuchstaben, Kleinbuchstaben, Ziffern und Sonderzeichen" + +msgid "No part of your user name or display name" +msgstr "Kein Teil Ihres Benutzernamens oder Anzeigenamens" + +#, python-format +msgid "Not your last %(n)s password" +msgid_plural "Not one of your last %(n)s passwords" +msgstr[0] "Nicht Ihr letztes Passwort (%(n)s gespeichert)" +msgstr[1] "Keines Ihrer letzten %(n)s Passwörter" + +#, python-format +msgid "A new password can be changed again after %(n)s day" +msgid_plural "A new password can be changed again after %(n)s days" +msgstr[0] "Ein neues Passwort kann frühestens nach %(n)s Tag erneut geändert werden" +msgstr[1] "Ein neues Passwort kann frühestens nach %(n)s Tagen erneut geändert werden" + +#, python-format +msgid "A new password can be changed again after %(n)s hour" +msgid_plural "A new password can be changed again after %(n)s hours" +msgstr[0] "Ein neues Passwort kann frühestens nach %(n)s Stunde erneut geändert werden" +msgstr[1] "Ein neues Passwort kann frühestens nach %(n)s Stunden erneut geändert werden" + +#, python-format +msgid "A password is valid for %(n)s day" +msgid_plural "A password is valid for %(n)s days" +msgstr[0] "Ein Passwort ist %(n)s Tag gültig" +msgstr[1] "Ein Passwort ist %(n)s Tage gültig" + +msgid "The password is too short." +msgstr "Das Passwort ist zu kurz." + +msgid "The password needs characters from more groups: upper case, lower case, digits, special." +msgstr "Dem Passwort fehlen Zeichengruppen: Großbuchstaben, Kleinbuchstaben, Ziffern, Sonderzeichen." + +msgid "The current password is not correct." +msgstr "Das aktuelle Passwort ist nicht korrekt." + +msgid "The new password does not meet the rules above, or it was used or changed too recently." +msgstr "Das neue Passwort erfüllt die oben genannten Regeln nicht, wurde schon einmal verwendet oder kommt zu kurz nach der letzten Änderung." + +msgid "Please fill in this field." +msgstr "Bitte füllen Sie dieses Feld aus." + +msgid "This password is too long." +msgstr "Dieses Passwort ist zu lang." + +msgid "The two new passwords are not the same." +msgstr "Die beiden neuen Passwörter stimmen nicht überein." + +msgid "The new password must be different." +msgstr "Das neue Passwort muss sich vom aktuellen unterscheiden." + +msgid "Change password" +msgstr "Passwort ändern" + +msgid "This password applies to every service that uses your account. After the change you are signed out on your other devices and you receive a confirmation by e-mail." +msgstr "Dieses Passwort gilt für alle Dienste, die Ihr Konto verwenden. Nach der Änderung werden Sie auf anderen Geräten abgemeldet, und Sie erhalten eine Bestätigung per E-Mail." + +msgid "Too many attempts. Please wait a few minutes." +msgstr "Zu viele Versuche. Bitte warten Sie einige Minuten." + +msgid "Back to the start page" +msgstr "Zurück zur Startseite" + +msgid "The password was not changed." +msgstr "Das Passwort wurde nicht geändert." + +msgid "Current password" +msgstr "Aktuelles Passwort" + +msgid "We check it before anything is changed." +msgstr "Wir prüfen es, bevor etwas geändert wird." + +msgid "New password" +msgstr "Neues Passwort" + +msgid "Your new password must follow these rules:" +msgstr "Ihr neues Passwort muss diese Regeln erfüllen:" + +msgid "A stricter rule may also apply to your account." +msgstr "Für Ihr Konto kann zusätzlich eine strengere Regel gelten." + +msgid "Repeat new password" +msgstr "Neues Passwort wiederholen" + +msgid "Cancel" +msgstr "Abbrechen" + +msgid "Forgotten or expired password? Please contact the administrator." +msgstr "Passwort vergessen oder abgelaufen? Wenden Sie sich bitte an die Administration." + +msgid "Password changed" +msgstr "Passwort geändert" + +msgid "Your password was changed." +msgstr "Ihr Passwort wurde geändert." + +msgid "You were signed out on your other devices." +msgstr "Sie wurden auf Ihren anderen Geräten abgemeldet." + +msgid "A confirmation was sent to your e-mail address." +msgstr "Eine Bestätigung wurde an Ihre E-Mail-Adresse gesendet." + +msgid "Result unclear" +msgstr "Ergebnis unklar" + +msgid "Your password may have been changed, but the portal could not finish. Try to sign in with the new password. If that does not work, contact the administrator." +msgstr "Ihr Passwort wurde möglicherweise geändert, aber das Portal konnte den Vorgang nicht abschließen. Versuchen Sie, sich mit dem neuen Passwort anzumelden. Wenn das nicht klappt, wenden Sie sich an die Administration." + +msgid "Your password was changed" +msgstr "Ihr Passwort wurde geändert" + +#, python-format +msgid "Hello %(name)s," +msgstr "Guten Tag %(name)s," + +#, python-format +msgid "your password was changed on %(when)s (%(zone)s)." +msgstr "Ihr Passwort wurde am %(when)s (%(zone)s) geändert." + +msgid "If you did this, you do not need to do anything. If you did not, contact the administrator at once." +msgstr "Wenn Sie das waren, müssen Sie nichts tun. Wenn nicht, wenden Sie sich bitte sofort an die Administration." + +msgid "This e-mail never contains your password." +msgstr "Diese E-Mail enthält niemals Ihr Passwort." diff --git a/portal/locale/de_DU/LC_MESSAGES/django.po b/portal/locale/de_DU/LC_MESSAGES/django.po index 5b3d32d..4bd4a05 100644 --- a/portal/locale/de_DU/LC_MESSAGES/django.po +++ b/portal/locale/de_DU/LC_MESSAGES/django.po @@ -29,3 +29,42 @@ msgstr "Das ist gerade nicht möglich. Bitte versuche es später erneut." msgid "This is a test e-mail from the portal. If you can read it, sending e-mail works." msgstr "Das ist eine Test-E-Mail vom Portal. Wenn du sie lesen kannst, funktioniert der E-Mail-Versand." + +msgid "No part of your user name or display name" +msgstr "Kein Teil deines Benutzernamens oder Anzeigenamens" + +#, python-format +msgid "Not your last %(n)s password" +msgid_plural "Not one of your last %(n)s passwords" +msgstr[0] "Nicht dein letztes Passwort (%(n)s gespeichert)" +msgstr[1] "Keines deiner letzten %(n)s Passwörter" + +msgid "Please fill in this field." +msgstr "Bitte fülle dieses Feld aus." + +msgid "This password applies to every service that uses your account. After the change you are signed out on your other devices and you receive a confirmation by e-mail." +msgstr "Dieses Passwort gilt für alle Dienste, die dein Konto verwenden. Nach der Änderung wirst du auf anderen Geräten abgemeldet, und du erhältst eine Bestätigung per E-Mail." + +msgid "Too many attempts. Please wait a few minutes." +msgstr "Zu viele Versuche. Bitte warte einige Minuten." + +msgid "Your new password must follow these rules:" +msgstr "Dein neues Passwort muss diese Regeln erfüllen:" + +msgid "A stricter rule may also apply to your account." +msgstr "Für dein Konto kann zusätzlich eine strengere Regel gelten." + +msgid "Forgotten or expired password? Please contact the administrator." +msgstr "Passwort vergessen oder abgelaufen? Wende dich bitte an die Administration." + +msgid "Your password was changed." +msgstr "Dein Passwort wurde geändert." + +msgid "You were signed out on your other devices." +msgstr "Du wurdest auf deinen anderen Geräten abgemeldet." + +msgid "A confirmation was sent to your e-mail address." +msgstr "Eine Bestätigung wurde an deine E-Mail-Adresse gesendet." + +msgid "Your password may have been changed, but the portal could not finish. Try to sign in with the new password. If that does not work, contact the administrator." +msgstr "Dein Passwort wurde möglicherweise geändert, aber das Portal konnte den Vorgang nicht abschließen. Versuche, dich mit dem neuen Passwort anzumelden. Wenn das nicht klappt, wende dich an die Administration." diff --git a/portal/profile/__init__.py b/portal/profile/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/portal/profile/apps.py b/portal/profile/apps.py new file mode 100644 index 0000000..7bd94c8 --- /dev/null +++ b/portal/profile/apps.py @@ -0,0 +1,6 @@ +from django.apps import AppConfig + + +class ProfileConfig(AppConfig): + name = "portal.profile" + label = "profile" diff --git a/portal/profile/forms.py b/portal/profile/forms.py new file mode 100644 index 0000000..d464913 --- /dev/null +++ b/portal/profile/forms.py @@ -0,0 +1,35 @@ +from django import forms +from django.utils.translation import gettext_lazy as _ + +MAX_PASSWORD_LENGTH = 256 # Active Directory's limit + + +def _password_field(): + return forms.CharField( + max_length=MAX_PASSWORD_LENGTH, + strip=False, # spaces can be part of a password + widget=forms.PasswordInput(render_value=False), + error_messages={ + "required": _("Please fill in this field."), + "max_length": _("This password is too long."), + }, + ) + + +class PasswordChangeForm(forms.Form): + current_password = _password_field() + new_password = _password_field() + confirm_password = _password_field() + + def clean(self): + data = super().clean() + current, new, confirm = ( + data.get("current_password"), + data.get("new_password"), + data.get("confirm_password"), + ) + if new and confirm and new != confirm: + self.add_error("confirm_password", _("The two new passwords are not the same.")) + elif new and current and new == current: + self.add_error("new_password", _("The new password must be different.")) + return data diff --git a/portal/profile/services.py b/portal/profile/services.py new file mode 100644 index 0000000..49736bf --- /dev/null +++ b/portal/profile/services.py @@ -0,0 +1,254 @@ +"""Password change for the signed-in user (FR-5, FR-6, FR-7; AD-3, AD-4, AD-8). + +The order is fixed: throttle check, policy read, verification of the current password, local +hints, then the directory change, the ended sessions, the audit entry and the queued mail in +one transaction. Passwords live only in local variables here: they are never logged, audited, +stored or put into an exception. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from datetime import timedelta +from zoneinfo import ZoneInfo + +from django.conf import settings +from django.db import transaction +from django.utils import timezone +from django.utils.translation import gettext as _ +from django.utils.translation import ngettext + +from portal.accounts import throttle +from portal.accounts.sessions import SessionUser, end_sessions +from portal.adapters.directory import DirectoryUnavailable, PasswordPolicy, PasswordRejected +from portal.adapters.registry import get_directory +from portal.audit import services as audit +from portal.mailer import services as mailer + +logger = logging.getLogger(__name__) + +ENDPOINT = "password-change" +CHARACTER_GROUPS_REQUIRED = 3 # AD complexity: three of upper, lower, digit, other + + +class WrongCurrentPassword(Exception): + def __init__(self, policy: PasswordPolicy | None = None) -> None: + self.policy = policy + super().__init__("The current password is not correct") + + +class PasswordNotAccepted(Exception): + """The new password breaks a rule. `reasons` holds keys for rules the portal could name.""" + + def __init__(self, reasons: list[str], policy: PasswordPolicy | None = None) -> None: + self.reasons = reasons + self.policy = policy + super().__init__("The new password is not accepted") + + +class OutcomeUnknown(Exception): + """The directory accepted the new password but the portal could not finish its own part.""" + + +@dataclass(frozen=True) +class Changed: + mail_queued: bool + other_sessions_ended: int + + +# -- the rules, in words ---------------------------------------------------------------------- + + +def describe_policy(policy: PasswordPolicy) -> list[str]: + """The user's password rules as sentences in the active language.""" + rules = [ + ngettext( + "At least %(n)s character", + "At least %(n)s characters", + policy.min_length, + ) + % {"n": policy.min_length} + ] + if policy.complexity: + rules.append( + _( + "Characters from at least three of these groups: upper case letters, " + "lower case letters, digits and special characters" + ) + ) + rules.append(_("No part of your user name or display name")) + if policy.history_length: + rules.append( + ngettext( + "Not your last %(n)s password", + "Not one of your last %(n)s passwords", + policy.history_length, + ) + % {"n": policy.history_length} + ) + if policy.min_age: + rules.append(_min_age_rule(policy.min_age)) + if policy.max_age: + days = max(1, round(policy.max_age.total_seconds() / 86400)) + rules.append( + ngettext( + "A password is valid for %(n)s day", + "A password is valid for %(n)s days", + days, + ) + % {"n": days} + ) + return rules + + +def _min_age_rule(age: timedelta) -> str: + if age >= timedelta(days=1): + days = round(age.total_seconds() / 86400) + return ngettext( + "A new password can be changed again after %(n)s day", + "A new password can be changed again after %(n)s days", + days, + ) % {"n": days} + hours = max(1, round(age.total_seconds() / 3600)) + return ngettext( + "A new password can be changed again after %(n)s hour", + "A new password can be changed again after %(n)s hours", + hours, + ) % {"n": hours} + + +def _character_groups(password: str) -> int: + groups = set() + for char in password: + if char.isupper(): + groups.add("upper") + elif char.islower(): + groups.add("lower") + elif char.isdigit(): + groups.add("digit") + else: + groups.add("other") + return len(groups) + + +def local_hints(new: str, policy: PasswordPolicy) -> list[str]: + """Rule keys the portal can check itself; the directory stays the authority.""" + reasons = [] + if len(new) < policy.min_length: + reasons.append("too_short") + if policy.complexity and _character_groups(new) < CHARACTER_GROUPS_REQUIRED: + reasons.append("too_few_groups") + return reasons + + +# -- the limit ----------------------------------------- + + +def failure_limit(policy: PasswordPolicy) -> int: + """Wrong current passwords allowed: below the directory's lockout threshold (FR-6).""" + threshold = policy.lockout_threshold + if threshold <= 0: + return throttle.THROTTLE_MAX_FAILURES + return max(1, min(throttle.THROTTLE_MAX_FAILURES, threshold - 1)) + + +def pause_seconds(policy: PasswordPolicy) -> int: + window = int(policy.lockout_window.total_seconds()) if policy.lockout_window else 0 + return max(throttle.THROTTLE_PAUSE_SECONDS, window) + + +# -- the change ----------------------------------------- + + +def _record_failure(user: SessionUser, ip: str, reason: str, **detail) -> None: + audit.record_failure( + user.key, "password.change_failed", ip=ip, detail={"reason": reason, **detail} + ) + + +def change_password( + user: SessionUser, current: str, new: str, *, ip: str, session_key: str | None +) -> Changed: + """Change the signed-in user's password. + + Raises Throttled, WrongCurrentPassword, PasswordNotAccepted, DirectoryUnavailable or + OutcomeUnknown. Must be called outside an open transaction. + """ + try: + throttle.check(ENDPOINT, user=user.key, ip=ip or None) + except throttle.Throttled: + audit.record_failure(user.key, "password.throttled", ip=ip) + raise + + directory = get_directory() + try: + policy = directory.password_policy(user.sam) + if not directory.verify_password(user.sam, current): + throttle.hit( + ENDPOINT, + user=user.key, + ip=ip or None, + max_failures=failure_limit(policy), + pause_seconds=pause_seconds(policy), + ) + _record_failure(user, ip, "wrong_current") + raise WrongCurrentPassword(policy) + account = directory.find_user(user.sam) + except DirectoryUnavailable: + _record_failure(user, ip, "unavailable") + raise + + reasons = local_hints(new, policy) + if reasons: + _record_failure(user, ip, "policy", rules=reasons) + raise PasswordNotAccepted(reasons, policy) + + changed_in_directory = False + try: + with transaction.atomic(): + directory.change_password(user.sam, current, new) + changed_in_directory = True + ended = end_sessions(user, except_session_key=session_key) + mail_queued = _queue_mail(account) + audit.record( + user.key, + "password.changed", + ip=ip, + detail={ + "other_sessions_ended": ended, + "mail": "queued" if mail_queued else "skipped", + }, + ) + except PasswordRejected as exc: + if exc.reason == "wrong_current": # the password changed between the check and the change + raise WrongCurrentPassword(policy) from exc + _record_failure(user, ip, "policy") + raise PasswordNotAccepted([], policy) from exc + except DirectoryUnavailable: + _record_failure(user, ip, "unavailable") + raise + except Exception as exc: + if changed_in_directory: + # AD-4: the directory changed but the portal's part did not commit. + logger.error("Password changed in the directory but the portal did not record it") + raise OutcomeUnknown("The password was changed but not recorded") from exc + raise + + throttle.reset(ENDPOINT, user=user.key) + return Changed(mail_queued=mail_queued, other_sessions_ended=ended) + + +def _queue_mail(account) -> bool: + """Queue the confirmation e-mail inside the caller's transaction; False if there is none.""" + address = (account.mail if account else "") or "" + if not mailer.is_valid_address(address): + return False + name = account.display_name or " ".join(p for p in (account.given_name, account.surname) if p) + now = timezone.now().astimezone(ZoneInfo(settings.DISPLAY_TIME_ZONE)) + mailer.enqueue( + "password_changed", + address, + {"name": name or account.sam, "changed_at": now, "zone_name": settings.DISPLAY_TIME_ZONE}, + ) + return True diff --git a/portal/profile/urls.py b/portal/profile/urls.py new file mode 100644 index 0000000..e6e59a1 --- /dev/null +++ b/portal/profile/urls.py @@ -0,0 +1,8 @@ +from django.urls import path + +from . import views + +urlpatterns = [ + path("password", views.password, name="password"), + path("password/done", views.password_done, name="password-done"), +] diff --git a/portal/profile/views.py b/portal/profile/views.py new file mode 100644 index 0000000..ead21af --- /dev/null +++ b/portal/profile/views.py @@ -0,0 +1,109 @@ +"""The password page (FR-5..FR-7). No business logic here: see services.py (AD-17).""" + +from __future__ import annotations + +import logging + +from django.shortcuts import redirect, render +from django.utils.translation import gettext as _ +from django.views.decorators.debug import sensitive_post_parameters +from django.views.decorators.http import require_http_methods + +from portal.accounts.decorators import login_required +from portal.accounts.throttle import Throttled +from portal.adapters.directory import DirectoryUnavailable +from portal.adapters.registry import get_directory + +from . import services +from .forms import PasswordChangeForm + +logger = logging.getLogger(__name__) + +DONE_FLAG = "password_changed" + +# Rule keys from services.local_hints, in plain words. +HINT_MESSAGES = { + "too_short": lambda: _("The password is too short."), + "too_few_groups": lambda: _( + "The password needs characters from more groups: upper case, lower case, digits, special." + ), +} + + +def _page(request, form, *, policy=None, status=200, throttled=False): + rules = services.describe_policy(policy) if policy is not None else [] + partly_known = policy is not None and policy.fine_grained and not policy.fine_grained_readable + context = { + "form": form, + "rules": rules, + "partly_known": partly_known, + "throttled": throttled, + } + return render(request, "profile/password.html", context, status=status) + + +def _unavailable(request): + return render(request, "accounts/unavailable.html", status=503) + + +@login_required +@sensitive_post_parameters("current_password", "new_password", "confirm_password") +@require_http_methods(["GET", "POST"]) +def password(request): + user = request.portal_user.user + if request.method == "GET": + try: + policy = get_directory().password_policy(user.sam) + except DirectoryUnavailable: + return _unavailable(request) + return _page(request, PasswordChangeForm(), policy=policy) + + form = PasswordChangeForm(request.POST) + if not form.is_valid(): + return _page(request, form, policy=_policy_or_none(user), status=400) + + try: + result = services.change_password( + user, + form.cleaned_data["current_password"], + form.cleaned_data["new_password"], + ip=request.client_ip, + session_key=request.session.session_key, + ) + except Throttled: + return _page(request, PasswordChangeForm(), status=429, throttled=True) + except services.WrongCurrentPassword as exc: + form.add_error("current_password", _("The current password is not correct.")) + return _page(request, form, policy=exc.policy, status=400) + except services.PasswordNotAccepted as exc: + messages = [HINT_MESSAGES[key]() for key in exc.reasons] or [ + _( + "The new password does not meet the rules above, " + "or it was used or changed too recently." + ) + ] + for message in messages: + form.add_error("new_password", message) + return _page(request, form, policy=exc.policy, status=400) + except DirectoryUnavailable: + return _unavailable(request) + except services.OutcomeUnknown: + return render(request, "profile/password_unknown.html", status=500) + + request.session[DONE_FLAG] = {"mail": result.mail_queued} + return redirect("password-done") + + +def _policy_or_none(user): + try: + return get_directory().password_policy(user.sam) + except DirectoryUnavailable: + return None + + +@login_required +def password_done(request): + flag = request.session.pop(DONE_FLAG, None) + if flag is None: # opened directly: nothing to report + return redirect("password") + return render(request, "profile/password_done.html", {"mail": flag["mail"]}) diff --git a/portal/shell/menu.py b/portal/shell/menu.py index 1292f04..0470755 100644 --- a/portal/shell/menu.py +++ b/portal/shell/menu.py @@ -22,7 +22,7 @@ class Group: MENU = ( - Group(None, None, (Entry(_("Home"), "home"),)), + Group(None, None, (Entry(_("Home"), "home"), Entry(_("Change password"), "password"))), Group( _("Publishing"), "publisher", diff --git a/portal/static/portal/shell.css b/portal/static/portal/shell.css index 23dcc6f..ac7a286 100644 --- a/portal/static/portal/shell.css +++ b/portal/static/portal/shell.css @@ -14,6 +14,8 @@ --text-secondary: #616161; --line: #e0e0e0; --control-line: #8a8a8a; + --danger: #b10e1c; + --success: #107c10; --font: "Segoe UI Variable", "Segoe UI", system-ui, sans-serif; --radius-sm: 4px; --radius-lg: 8px; @@ -37,6 +39,8 @@ --text-secondary: #c8c8c8; --line: #3d3d3d; --control-line: #9a9a9a; + --danger: #ff99a4; + --success: #6ccb5f; color-scheme: dark; } @@ -55,6 +59,8 @@ --text-secondary: #c8c8c8; --line: #3d3d3d; --control-line: #9a9a9a; + --danger: #ff99a4; + --success: #6ccb5f; color-scheme: dark; } } @@ -430,3 +436,145 @@ main button { display: block; } } + +/* Forms, notices and buttons */ + +.card { + max-width: 520px; + margin: 0 0 14px; + padding: 16px 18px; + background: var(--surface); + border: 1px solid var(--line); + border-radius: var(--radius-lg); +} + +.field { + margin-bottom: 14px; +} + +.field label { + display: block; + margin-bottom: 4px; + font-weight: 600; +} + +.field input { + width: 100%; + min-height: 36px; + padding: 6px 10px; + border: 1px solid var(--control-line); + border-bottom-width: 2px; + border-radius: var(--radius-sm); + background: var(--bg); + color: var(--text); + font: inherit; +} + +.field.has-error input { + border-color: var(--danger); +} + +.hint { + margin: 4px 0 0; + font-size: 12px; + color: var(--text-secondary); +} + +.error { + margin: 4px 0 0; + font-size: 13px; + color: var(--danger); +} + +.error::before { + content: "\26A0\FE0E "; +} + +.rules ul { + margin: 4px 0 14px; + padding-left: 20px; + font-size: 13px; + color: var(--text-secondary); +} + +.error-summary, +.notice { + max-width: 520px; + margin: 0 0 14px; + padding: 12px 16px; + border: 1px solid var(--danger); + border-left-width: 5px; + border-radius: 6px; + background: var(--surface); +} + +.notice.success { + border-color: var(--success); +} + +.error-summary p, +.notice p { + margin: 0; +} + +.error-summary ul { + margin: 6px 0 0; + padding-left: 20px; +} + +.actions { + margin: 0; +} + +a.button, +main button.primary { + display: inline-flex; + align-items: center; + justify-content: center; + gap: 8px; + min-height: var(--touch); + padding: 0 16px; + border: 1px solid var(--control-line); + border-radius: var(--radius-sm); + color: var(--text); + font: inherit; + text-decoration: none; +} + +main button.primary { + border-color: var(--brand); + background: var(--brand); + color: var(--on-brand); + font-weight: 600; +} + +main button.primary:disabled { + opacity: 0.7; + cursor: progress; +} + +.spinner { + display: none; + width: 14px; + height: 14px; + border: 2px solid currentcolor; + border-top-color: transparent; + border-radius: 50%; + animation: spin 0.8s linear infinite; +} + +.busy .spinner { + display: inline-block; +} + +@keyframes spin { + to { + transform: rotate(360deg); + } +} + +@media (prefers-reduced-motion: reduce) { + .spinner { + animation-duration: 3s; + } +} diff --git a/portal/static/portal/shell.js b/portal/static/portal/shell.js index 529020e..6a6cdd7 100644 --- a/portal/static/portal/shell.js +++ b/portal/static/portal/shell.js @@ -50,6 +50,25 @@ } }); + // A form that is being sent shows a spinner and cannot be sent twice. + document.addEventListener("submit", function (event) { + var form = event.target; + if (!form.hasAttribute("data-busy-on-submit")) { + return; + } + form.classList.add("busy"); + setTimeout(function () { + form.querySelectorAll("button[type=submit]").forEach(function (button) { + button.disabled = true; + }); + }, 0); + }); + + var focusTarget = document.querySelector("[data-focus-on-load]"); + if (focusTarget) { + focusTarget.focus(); + } + if (menuButton) { menuButton.addEventListener("click", function () { var open = root.classList.toggle("menu-open"); diff --git a/portal/templates/mail/password_changed/body.txt b/portal/templates/mail/password_changed/body.txt new file mode 100644 index 0000000..6c7b0db --- /dev/null +++ b/portal/templates/mail/password_changed/body.txt @@ -0,0 +1,7 @@ +{% load i18n tz %}{% autoescape off %}{% blocktrans with name=name %}Hello {{ name }},{% endblocktrans %} + +{% timezone zone_name %}{% blocktrans with when=changed_at|date:"j. F Y, H:i" zone=changed_at|date:"T" %}your password was changed on {{ when }} ({{ zone }}).{% endblocktrans %}{% endtimezone %} + +{% trans "If you did this, you do not need to do anything. If you did not, contact the administrator at once." %} + +{% trans "This e-mail never contains your password." %}{% endautoescape %} diff --git a/portal/templates/mail/password_changed/subject.txt b/portal/templates/mail/password_changed/subject.txt new file mode 100644 index 0000000..02b3edb --- /dev/null +++ b/portal/templates/mail/password_changed/subject.txt @@ -0,0 +1 @@ +{% load i18n %}{% autoescape off %}{% trans "Your password was changed" %}{% endautoescape %} diff --git a/portal/templates/profile/_field.html b/portal/templates/profile/_field.html new file mode 100644 index 0000000..0319d86 --- /dev/null +++ b/portal/templates/profile/_field.html @@ -0,0 +1,7 @@ +{% load i18n %} +
+ + + {% if hint %}

{{ hint }}

{% else %}{% endif %} + {% for error in field.errors %}

{{ error }}

{% endfor %} +
diff --git a/portal/templates/profile/password.html b/portal/templates/profile/password.html new file mode 100644 index 0000000..e54a5c6 --- /dev/null +++ b/portal/templates/profile/password.html @@ -0,0 +1,47 @@ +{% extends "base.html" %} +{% load i18n %} +{% block title %}{% trans "Change password" %}{% endblock %} +{% block content %} +

{% trans "Change password" %}

+

{% trans "This password applies to every service that uses your account. After the change you are signed out on your other devices and you receive a confirmation by e-mail." %}

+{% if throttled %} + +

{% trans "Back to the start page" %}

+{% else %} +{% if form.errors %} + +{% endif %} +
+ {% csrf_token %} + {% if rules %} +
+

{% trans "Your new password must follow these rules:" %}

+ + {% if partly_known %}

{% trans "A stricter rule may also apply to your account." %}

{% endif %} +
+ {% endif %} + {% trans "Current password" as label %}{% trans "We check it before anything is changed." as hint %} + {% include "profile/_field.html" with field=form.current_password label=label hint=hint autocomplete="current-password" %} + {% trans "New password" as label %} + {% include "profile/_field.html" with field=form.new_password label=label hint="" autocomplete="new-password" %} + {% trans "Repeat new password" as label %} + {% include "profile/_field.html" with field=form.confirm_password label=label hint="" autocomplete="new-password" %} +

+ + {% trans "Cancel" %} +

+
+

{% trans "Forgotten or expired password? Please contact the administrator." %}

+{% endif %} +{% endblock %} diff --git a/portal/templates/profile/password_done.html b/portal/templates/profile/password_done.html new file mode 100644 index 0000000..2eb5c51 --- /dev/null +++ b/portal/templates/profile/password_done.html @@ -0,0 +1,12 @@ +{% extends "base.html" %} +{% load i18n %} +{% block title %}{% trans "Password changed" %}{% endblock %} +{% block content %} +

{% trans "Password changed" %}

+
+

{% trans "Your password was changed." %}

+
+

{% trans "You were signed out on your other devices." %}

+{% if mail %}

{% trans "A confirmation was sent to your e-mail address." %}

{% endif %} +

{% trans "Back to the start page" %}

+{% endblock %} diff --git a/portal/templates/profile/password_unknown.html b/portal/templates/profile/password_unknown.html new file mode 100644 index 0000000..f7f28d3 --- /dev/null +++ b/portal/templates/profile/password_unknown.html @@ -0,0 +1,10 @@ +{% extends "base.html" %} +{% load i18n %} +{% block title %}{% trans "Result unclear" %}{% endblock %} +{% block content %} +

{% trans "Result unclear" %}

+ +

{% trans "Back to the start page" %}

+{% endblock %} diff --git a/portal/tests/test_password_change.py b/portal/tests/test_password_change.py new file mode 100644 index 0000000..18fafd9 --- /dev/null +++ b/portal/tests/test_password_change.py @@ -0,0 +1,437 @@ +"""Password change: the page, the ordered service, limits, sessions, audit and mail (FR-5..FR-7).""" + +import logging +import re +from datetime import timedelta + +import pytest +from django.test import Client + +from portal.accounts.models import ThrottleCounter, UserSession +from portal.adapters.directory import DirectoryUnavailable, PasswordPolicy, PasswordRejected +from portal.audit.models import AuditEntry +from portal.mailer.models import OutboxMail +from portal.profile import services +from portal.tests.helpers import sign_in + +pytestmark = pytest.mark.django_db(transaction=True) + +CURRENT = "Correct-Horse-9" +NEW = "Brand-New-Secret-77" +WRONG = "Not-The-Password-1" + + +class Spy: + """Counts calls to the directory methods the order of the flow is about.""" + + def __init__(self, directory): + self.calls = [] + for name in ("password_policy", "verify_password", "change_password", "find_user"): + self._wrap(directory, name) + + def _wrap(self, directory, name): + original = getattr(directory, name) + + def wrapper(*args, **kwargs): + self.calls.append(name) + return original(*args, **kwargs) + + setattr(directory, name, wrapper) + + def count(self, name): + return self.calls.count(name) + + +@pytest.fixture +def people(adapters): + d = adapters.directory + d.add_user( + "alice", + password=CURRENT, + given_name="Alice", + surname="Anders", + display_name="Alice Anders", + mail="alice@example.test", + ) + d.add_user("bob", mail="bob@example.test", display_name="Bob Brown") + d.add_user("nomail", password=CURRENT, display_name="No Mail") + return d + + +def change(client, current=CURRENT, new=NEW, confirm=None, **extra): + data = { + "current_password": current, + "new_password": new, + "confirm_password": new if confirm is None else confirm, + **extra, + } + return client.post("/password", data) + + +def signed_in(adapters, name="alice"): + client, _ = sign_in(adapters, name) + return client + + +def audit_actions(): + return list(AuditEntry.objects.values_list("action", flat=True)) + + +# -- the page ----------------------------------------- + + +def test_the_page_lists_the_rules_before_the_fields_and_states_the_effect(people, adapters): + html = signed_in(adapters).get("/password").content.decode() + assert "At least 8 characters" in html and "at least three of these groups" in html + assert "Not one of your last 24 passwords" in html and "valid for 90 days" in html + assert "signed out on your other devices" in html and "confirmation by e-mail" in html + assert html.index("At least 8 characters") < html.index('name="new_password"') + assert 'autocomplete="current-password"' in html + assert html.count('autocomplete="new-password"') == 2 + + +def test_the_page_speaks_german_with_the_chosen_register(people, adapters): + client = signed_in(adapters) + client.cookies["pm_lang"] = "de" + assert "Mindestens 8 Zeichen" in client.get("/password").content.decode() + assert "Ihr neues Passwort muss" in client.get("/password").content.decode() + client.cookies["pm_form"] = "du" + assert "Dein neues Passwort muss" in client.get("/password").content.decode() + + +def test_a_fine_grained_policy_that_cannot_be_read_is_flagged(people, adapters): + adapters.directory.policy = PasswordPolicy( + 8, True, 0, timedelta(0), None, fine_grained=True, fine_grained_readable=False + ) + html = signed_in(adapters).get("/password").content.decode() + assert "A stricter rule may also apply" in html + + +def test_the_menu_has_the_entry_and_marks_it_current(people, adapters): + html = signed_in(adapters).get("/password").content.decode() + assert re.search(r'', html) + + +def test_the_page_is_unavailable_when_the_directory_is_down(people, adapters): + client = signed_in(adapters) + adapters.directory.unavailable = True + assert client.get("/password").status_code == 503 + + +def test_the_page_needs_a_sign_in_and_only_get_and_post(people, adapters): + anonymous = Client() + assert anonymous.get("/password").status_code == 302 + assert anonymous.post("/password", {"current_password": "x"}).status_code == 302 + assert signed_in(adapters).put("/password").status_code == 405 + + +def test_the_form_needs_a_csrf_token(people, adapters): + client = signed_in(adapters) + strict = Client(enforce_csrf_checks=True) + strict.cookies = client.cookies + data = {"current_password": CURRENT, "new_password": NEW, "confirm_password": NEW} + assert strict.post("/password", data).status_code == 403 + assert adapters.directory.verify_password("alice", CURRENT) + + +# -- success ----------------------------------------- + + +def test_a_change_updates_the_directory_and_ends_only_the_other_sessions(people, adapters): + first = signed_in(adapters) + second = signed_in(adapters) + bob = signed_in(adapters, "bob") + response = change(first) + assert response.status_code == 302 and response["Location"] == "/password/done" + assert adapters.directory.verify_password("alice", NEW) + assert not adapters.directory.verify_password("alice", CURRENT) + assert first.get("/").status_code == 200 # this session stays + assert second.get("/").status_code == 302 # the other one is gone + assert bob.get("/").status_code == 200 # other users are untouched + assert UserSession.objects.filter(username="alice").count() == 1 + + +def test_a_change_is_audited_with_the_address_and_no_secrets(people, adapters): + signed_in(adapters) + client = signed_in(adapters) + change(client) + entry = AuditEntry.objects.get(action="password.changed") + assert entry.actor == "alice" and entry.ip == "127.0.0.1" + assert entry.detail == {"other_sessions_ended": 1, "mail": "queued"} + + +def test_a_confirmation_mail_is_queued_in_german_with_the_time_and_no_password(people, adapters): + change(signed_in(adapters)) + mail = OutboxMail.objects.get() + assert mail.recipient == "alice@example.test" and mail.template == "password_changed" + assert mail.subject == "Ihr Passwort wurde geändert" + assert "Guten Tag Alice Anders," in mail.body_text + assert re.search(r"am \d{1,2}\. \w+ \d{4}, \d{2}:\d{2} \(CES?T\) geändert", mail.body_text) + assert NEW not in mail.body_text and CURRENT not in mail.body_text + + +def test_the_done_page_reports_once_and_mentions_the_mail(people, adapters): + client = signed_in(adapters) + response = change(client) + page = client.get(response["Location"]) + assert "Your password was changed." in page.content.decode() + assert "confirmation was sent" in page.content.decode() + assert client.get("/password/done")["Location"] == "/password" # nothing left to report + + +def test_without_a_mail_address_the_password_still_changes(people, adapters): + client = signed_in(adapters, "nomail") + response = change(client) + assert response.status_code == 302 and adapters.directory.verify_password("nomail", NEW) + assert OutboxMail.objects.count() == 0 + assert AuditEntry.objects.get(action="password.changed").detail["mail"] == "skipped" + assert "confirmation was sent" not in client.get(response["Location"]).content.decode() + + +def test_a_success_clears_the_failed_attempts_of_the_user(people, adapters): + client = signed_in(adapters) + change(client, current=WRONG) + assert ThrottleCounter.objects.filter(subject="user:alice").exists() + change(client) + assert not ThrottleCounter.objects.filter(subject="user:alice").exists() + + +def test_the_user_is_always_the_session_user(people, adapters): + client = signed_in(adapters) + change(client, username="bob", sam="bob", user="bob") + assert adapters.directory.verify_password("alice", NEW) + assert adapters.directory.verify_password("bob", "Correct-Horse-9") + + +# -- mistakes in the form ----------------------------------------- + + +@pytest.mark.parametrize( + ("fields", "message"), + [ + ({"confirm": "Something-Else-12"}, "not the same"), + ({"new": CURRENT}, "must be different"), + ({"current": ""}, "Please fill in this field."), + ({"new": "", "confirm": ""}, "Please fill in this field."), + ({"new": "A" * 257, "confirm": "A" * 257}, "too long"), + ], +) +def test_form_mistakes_are_explained_and_never_reach_the_directory( + people, adapters, fields, message +): + client = signed_in(adapters) + spy = Spy(adapters.directory) + response = change( + client, + current=fields.get("current", CURRENT), + new=fields.get("new", NEW), + confirm=fields.get("confirm"), + ) + assert response.status_code == 400 and message in response.content.decode() + assert spy.count("verify_password") == 0 and spy.count("change_password") == 0 + assert not ThrottleCounter.objects.exists() + assert adapters.directory.verify_password("alice", CURRENT) + + +def test_errors_are_next_to_the_field_and_in_a_summary(people, adapters): + html = change(signed_in(adapters), confirm="Other-Password-12").content.decode() + assert 'class="error-summary"' in html and "The password was not changed." in html + assert 'href="#id_confirm_password"' in html and 'id="id_confirm_password-error"' in html + assert 'aria-invalid="true"' in html and "data-focus-on-load" in html + + +# -- wrong current password and the limit ----------------------------------------- + + +def test_a_wrong_current_password_changes_nothing_and_counts(people, adapters): + spy = Spy(adapters.directory) + client = signed_in(adapters) + other = signed_in(adapters) + response = change(client, current=WRONG) + assert response.status_code == 400 + assert "The current password is not correct." in response.content.decode() + assert spy.count("change_password") == 0 + assert adapters.directory.verify_password("alice", CURRENT) + assert other.get("/").status_code == 200 and OutboxMail.objects.count() == 0 + assert ThrottleCounter.objects.get(subject="user:alice").count == 1 + entry = AuditEntry.objects.get(action="password.change_failed") + assert entry.detail == {"reason": "wrong_current"} + + +def test_the_limit_stays_below_the_directory_lockout_threshold(people, adapters): + client = signed_in(adapters) + for _ in range(3): # the fake domain locks after 5, so 4 are allowed + assert change(client, current=WRONG).status_code == 400 + assert change(client, current=WRONG).status_code == 400 # the 4th wrong one blocks + spy = Spy(adapters.directory) + response = change(client) # even the right password is refused now + assert response.status_code == 429 + assert "Too many attempts. Please wait a few minutes." in response.content.decode() + assert spy.calls == [] # refused before any directory call + assert adapters.directory.verify_password("alice", CURRENT) + assert "password.throttled" in audit_actions() + + +def test_the_pause_is_over_after_the_lockout_window(people, adapters): + client = signed_in(adapters) + for _ in range(4): + change(client, current=WRONG) + ThrottleCounter.objects.update(blocked_until=None, count=0) # the pause is over + assert change(client).status_code == 302 + + +def test_the_limit_for_other_thresholds(people): + def policy(threshold, window=timedelta(minutes=30)): + return PasswordPolicy( + 8, True, 0, timedelta(0), None, lockout_threshold=threshold, lockout_window=window + ) + + assert services.failure_limit(policy(0)) == 5 # the domain never locks + assert services.failure_limit(policy(1)) == 1 + assert services.failure_limit(policy(3)) == 2 + assert services.failure_limit(policy(5)) == 4 + assert services.failure_limit(policy(10)) == 5 + assert services.pause_seconds(policy(5, timedelta(minutes=30))) == 30 * 60 + assert services.pause_seconds(policy(5, timedelta(minutes=5))) == 15 * 60 + assert services.pause_seconds(policy(0, None)) == 15 * 60 + + +def test_the_limit_is_per_ip_too(people, adapters): + client = signed_in(adapters) + for _ in range(4): + change(client, current=WRONG) + other_user = signed_in(adapters, "bob") # same address, different user + assert change(other_user).status_code == 429 + + +# -- the directory refuses the new password ----------------------------------------- + + +@pytest.mark.parametrize( + ("new", "message"), + [("Ab1", "too short"), ("alllowercase-only", "more groups")], +) +def test_rules_the_portal_can_name_are_named_without_asking_the_directory( + people, adapters, new, message +): + spy = Spy(adapters.directory) + response = change(signed_in(adapters), new=new) + assert response.status_code == 400 and message in response.content.decode() + assert spy.count("change_password") == 0 + assert adapters.directory.verify_password("alice", CURRENT) + assert AuditEntry.objects.get(action="password.change_failed").detail["reason"] == "policy" + + +def test_a_rule_failure_does_not_count_as_a_wrong_password(people, adapters): + client = signed_in(adapters) + for _ in range(8): + assert change(client, new="Ab1").status_code == 400 + assert not ThrottleCounter.objects.exists() + assert change(client).status_code == 302 + + +def test_a_rejection_by_the_directory_is_explained_and_changes_nothing(people, adapters): + client = signed_in(adapters) + other = signed_in(adapters) + adapters.directory.change_password("alice", CURRENT, NEW) # CURRENT is now an old password + response = change(client, current=NEW, new=CURRENT) + html = response.content.decode() + assert response.status_code == 400 and "does not meet the rules" in html + assert other.get("/").status_code == 200 and OutboxMail.objects.count() == 0 + assert not ThrottleCounter.objects.exists() + assert "password.changed" not in audit_actions() + + +def test_the_password_changed_by_someone_else_in_between_counts_as_wrong( + people, adapters, monkeypatch +): + def raced(*args): + raise PasswordRejected("wrong_current") + + monkeypatch.setattr(adapters.directory, "change_password", raced) + response = change(signed_in(adapters)) + assert response.status_code == 400 and "not correct" in response.content.decode() + + +# -- failures after the first directory call ----------------------------------------- + + +def test_the_directory_going_down_before_the_change_changes_nothing(people, adapters): + client = signed_in(adapters) + adapters.directory.unavailable = True + assert change(client).status_code == 503 + assert AuditEntry.objects.get(action="password.change_failed").detail["reason"] == "unavailable" + + +def test_the_directory_failing_during_the_change_rolls_everything_back( + people, adapters, monkeypatch +): + client = signed_in(adapters) + other = signed_in(adapters) + + def down(*args): + raise DirectoryUnavailable("down") + + monkeypatch.setattr(adapters.directory, "change_password", down) + assert change(client).status_code == 503 + assert other.get("/").status_code == 200 + assert OutboxMail.objects.count() == 0 and "password.changed" not in audit_actions() + assert AuditEntry.objects.get(action="password.change_failed").detail["reason"] == "unavailable" + + +def test_a_failure_after_the_directory_changed_is_reported_as_unclear( + people, adapters, monkeypatch, caplog +): + client = signed_in(adapters) + other = signed_in(adapters) + + def broken(*args, **kwargs): + raise RuntimeError("database trouble") + + monkeypatch.setattr(services.mailer, "enqueue", broken) + with caplog.at_level(logging.ERROR): + response = change(client) + assert response.status_code == 500 and "Result unclear" in response.content.decode() + assert adapters.directory.verify_password("alice", NEW) # the directory did change + assert other.get("/").status_code == 200 # the portal's part rolled back + assert "password.changed" not in audit_actions() + assert "changed in the directory but the portal did not record it" in caplog.text + + +# -- no secrets anywhere ----------------------------------------- + + +def test_no_password_appears_in_logs_audit_session_database_or_pages(people, adapters, caplog): + client = signed_in(adapters) + other = signed_in(adapters) + pages = [] + with caplog.at_level(logging.DEBUG): + pages.append(change(client, current=WRONG, new=NEW + "x").content.decode()) + pages.append(change(client, new="Ab1").content.decode()) + pages.append(change(client, confirm="Different-One-99").content.decode()) + done = change(client) + pages.append(client.get(done["Location"]).content.decode()) + secrets = (CURRENT, NEW, NEW + "x", WRONG, "Different-One-99") + stored = " ".join(str(e.detail) + e.action + e.target for e in AuditEntry.objects.all()) + stored += " ".join(m.body_text + m.subject for m in OutboxMail.objects.all()) + stored += str(dict(client.session)) + str(dict(other.session)) + for secret in secrets: + assert secret not in caplog.text and secret not in stored, secret + assert all(secret not in page for page in pages), secret + + +# -- the rules in words ----------------------------------------- + + +def test_the_rules_are_written_out_for_each_part_of_the_policy(): + policy = PasswordPolicy(1, False, 1, timedelta(hours=1), None) + assert services.describe_policy(policy) == [ + "At least 1 character", + "Not your last 1 password", + "A new password can be changed again after 1 hour", + ] + long_policy = PasswordPolicy(12, True, 5, timedelta(days=2), timedelta(days=42)) + rules = services.describe_policy(long_policy) + assert "At least 12 characters" in rules and "Not one of your last 5 passwords" in rules + assert "A new password can be changed again after 2 days" in rules + assert "A password is valid for 42 days" in rules diff --git a/portal/tests/test_translations.py b/portal/tests/test_translations.py index be477b8..f8e57f8 100644 --- a/portal/tests/test_translations.py +++ b/portal/tests/test_translations.py @@ -1,5 +1,6 @@ """Every user-visible string has German text (AD-15); the informal catalogue only differs.""" +import ast import re from pathlib import Path @@ -12,29 +13,65 @@ LOCALE = PORTAL / "locale" TRANS_TAG = re.compile(r"""\{%\s*trans\s+(?:"([^"]+)"|'([^']+)')""") BLOCKTRANS = re.compile(r"\{%\s*blocktrans[^%]*%\}(.*?)\{%\s*endblocktrans\s*%\}", re.S) VARIABLE = re.compile(r"\{\{\s*(\w+)\s*\}\}") -PY_CALL = re.compile(r"""(? set[str]: - found = set() +def _call_name(node: ast.Call) -> str: + func = node.func + return func.id if isinstance(func, ast.Name) else getattr(func, "attr", "") + + +def _strings(node: ast.Call, count: int) -> tuple[str, ...] | None: + args = node.args[:count] + if len(args) == count and all( + isinstance(a, ast.Constant) and isinstance(a.value, str) for a in args + ): + return tuple(a.value for a in args) + return None + + +def messages_in_code() -> set[tuple[str, str | None]]: + """(msgid, msgid_plural) for every translatable string in templates and Python code.""" + found: set[tuple[str, str | None]] = set() templates = [*(PORTAL / "templates").rglob("*.html"), *(PORTAL / "templates").rglob("*.txt")] for template in templates: text = template.read_text(encoding="utf-8") - found.update(a or b for a, b in TRANS_TAG.findall(text)) - found.update(VARIABLE.sub(r"%(\1)s", body).strip() for body in BLOCKTRANS.findall(text)) + found.update((a or b, None) for a, b in TRANS_TAG.findall(text)) + found.update( + (VARIABLE.sub(r"%(\1)s", body).strip(), None) for body in BLOCKTRANS.findall(text) + ) for source in PORTAL.rglob("*.py"): if "tests" in source.parts or "migrations" in source.parts: continue - found.update(a or b for a, b in PY_CALL.findall(source.read_text(encoding="utf-8"))) + for node in ast.walk(ast.parse(source.read_text(encoding="utf-8"))): + if not isinstance(node, ast.Call): + continue + name = _call_name(node) + if name in SINGLE and (strings := _strings(node, 1)): + found.add((strings[0], None)) + elif name in PLURAL and (strings := _strings(node, 2)): + found.add((strings[0], strings[1])) return found +def msgids_in_code() -> set[str]: + return {msgid for msgid, _ in messages_in_code()} + + def catalogue(name: str) -> polib.POFile: return polib.pofile(str(LOCALE / name / "LC_MESSAGES" / "django.po")) +def texts(entry: polib.POEntry) -> list[str]: + """Every translated text of an entry: one, or one per plural form.""" + return list(entry.msgstr_plural.values()) if entry.msgid_plural else [entry.msgstr] + + def test_the_scan_finds_the_strings(): + messages = messages_in_code() assert {"Sign out", "Home", "Hello, %(name)s", "Audit log"} <= msgids_in_code() + assert ("At least %(n)s character", "At least %(n)s characters") in messages def test_every_string_in_the_code_has_german_text(): @@ -42,33 +79,42 @@ def test_every_string_in_the_code_has_german_text(): missing = sorted(msgids_in_code() - german.keys()) assert not missing, f"no German entry for: {missing}" for entry in german.values(): - assert entry.msgstr.strip(), f"empty German text for {entry.msgid!r}" + assert all(t.strip() for t in texts(entry)), f"empty German text for {entry.msgid!r}" assert "fuzzy" not in entry.flags, f"fuzzy entry {entry.msgid!r}" +def test_plural_strings_are_plural_entries_with_both_forms(): + german = {e.msgid: e for e in catalogue("de")} + for msgid, plural in messages_in_code(): + if plural: + entry = german[msgid] + assert entry.msgid_plural == plural and len(entry.msgstr_plural) == 2, msgid + + def test_the_german_catalogue_has_no_stale_entries(): stale = sorted({e.msgid for e in catalogue("de")} - msgids_in_code()) assert not stale, f"entries no longer used: {stale}" def test_the_informal_catalogue_only_holds_differences_to_known_strings(): - german = {e.msgid: e.msgstr for e in catalogue("de")} + german = {e.msgid: texts(e) for e in catalogue("de")} for entry in catalogue("de_DU"): assert entry.msgid in german, f"unknown string {entry.msgid!r}" - assert entry.msgstr.strip() and "fuzzy" not in entry.flags - assert entry.msgstr != german[entry.msgid], f"{entry.msgid!r} does not differ" + assert all(t.strip() for t in texts(entry)) and "fuzzy" not in entry.flags + assert texts(entry) != german[entry.msgid], f"{entry.msgid!r} does not differ" @pytest.mark.parametrize("name", ["de", "de_DU"]) def test_placeholders_survive_translation(name): + pattern = re.compile(r"%\(\w+\)s") for entry in catalogue(name): - assert set(re.findall(r"%\(\w+\)s", entry.msgid)) == set( - re.findall(r"%\(\w+\)s", entry.msgstr) - ), entry.msgid + wanted = set(pattern.findall(entry.msgid)) + for text in texts(entry): + assert set(pattern.findall(text)) == wanted, entry.msgid def test_the_formal_text_does_not_use_du_and_the_informal_does_not_use_sie(): - formal = " ".join(e.msgstr for e in catalogue("de")) + formal = " ".join(t for e in catalogue("de") for t in texts(e)) assert not re.search(r"\b(du|dir|dich|dein|deine)\b", formal, re.I) - informal = " ".join(e.msgstr for e in catalogue("de_DU")) + informal = " ".join(t for e in catalogue("de_DU") for t in texts(e)) assert not re.search(r"\b(Sie|Ihnen|Ihr|Ihre)\b", informal)