feat: add password change

Password page with the AD policy listed first, ordered service (throttle limit below the lockout threshold, verification, local hints, directory change, ended sessions, audit and confirmation mail in one transaction), plain-language errors, German text and tests.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N3t4WL7bd9bdHHNjEsxzqC

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'<a href="/password" class="current" aria-current="page">', 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

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"""(?<![\w.])(?:_|gettext|gettext_lazy)\(\s*(?:"([^"]+)"|'([^']+)')""")
+SINGLE = {"_", "gettext", "gettext_lazy"}
+PLURAL = {"ngettext", "ngettext_lazy"}
 
 
-def msgids_in_code() -> 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)