From ae4b30ec22d927bf1fb9e2a9ba32d6727c6935bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ebbe=20Ba=C3=9F?= Date: Mon, 19 Oct 2026 08:23:33 +0200 Subject: [PATCH] fix: harden password change after code review Count and audit a wrong current password found at change time, log the unclear outcome with its details and end the other sessions as far as possible, note a lockout threshold of 1 in the audit, skip the local hints when the fine-grained policy cannot be read, round the minimum age up, reset the form after back navigation, give error messages unique ids, and fix the German rejection text. Adds tests for the block length, the masked password fields and the new cases; the review findings are recorded in the plan and the deferred work. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01FocPJ75XPma7TcuYEMy8zm --- .../deferred-work.md | 18 ++++ .../story-password-change-plan.md | 38 +++++++++ portal/locale/de/LC_MESSAGES/django.po | 2 +- portal/profile/services.py | 81 +++++++++++++++--- portal/static/portal/shell.js | 13 +++ portal/templates/profile/_field.html | 4 +- portal/tests/test_password_change.py | 82 +++++++++++++++++-- 7 files changed, 218 insertions(+), 20 deletions(-) diff --git a/_bmad-output/initiative-ping-mee-portal/deferred-work.md b/_bmad-output/initiative-ping-mee-portal/deferred-work.md index 2eb2ae1..996ef2c 100644 --- a/_bmad-output/initiative-ping-mee-portal/deferred-work.md +++ b/_bmad-output/initiative-ping-mee-portal/deferred-work.md @@ -1,3 +1,21 @@ - source_plan: `_bmad-output/initiative-ping-mee-portal/epic-platform-and-sign-in/story-audit-throttle-and-session-foundations-plan.md` summary: Concurrent writers on the throttle counters are not tested. evidence: The test database is SQLite in memory with a shared cache, which answers "table is locked" to a second writer at once, so a threaded test fails for reasons that say nothing about production (a WAL file database with a 20 s busy timeout and IMMEDIATE transactions). A real test needs a file database; tried and dropped in the refactor sweep. +- source_plan: `_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md` + summary: Code review 2026-10-19: the throttle check and the failure count are separate steps, so parallel requests can all pass the check before any failure is counted and more wrong binds reach AD than the limit allows. + evidence: Pre-existing design of the throttle foundation (AD-7); `portal/accounts/throttle.py` `check` and `hit`. Needs a reserve-then-confirm step to fix, for every endpoint at once. +- source_plan: `_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md` + summary: Code review 2026-10-19 (unverified, would be medium): a timeout after AD applied the password change is reported as "nothing changed", so sessions are not ended and no mail is queued. + evidence: Settle by checking whether `LdapDirectory._service` / `modify_s` turns a lost response into `DirectoryUnavailable`, and whether `verify_password` with the new password after a timeout would tell. +- source_plan: `_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md` + summary: Code review 2026-10-19: no signed-in page sends `Cache-Control: no-store`, so the back button can show a page after sign-out. + evidence: Pre-existing; only the health check uses `never_cache`. Decide once for every authenticated page (middleware), not per page. +- source_plan: `_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md` + summary: Code review 2026-10-19: AD-2 in the architecture spine names a `msDS-Lockout*` wildcard, but the adapter reads only `msDS-LockoutThreshold` and `msDS-LockoutObservationWindow`. + evidence: Fix edits the architecture document; reword AD-2 to name the two attributes. +- source_plan: `_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md` + summary: Known limitation, accepted in the code review of 2026-10-19: when AD never forgets failed attempts (a lockout observation window of 0), the portal's pause is still 15 minutes, so its limit does not keep up with AD's count. + evidence: Unusual AD setup; not the owner's. Revisit if a deployment sets the observation window to never. +- source_plan: `_bmad-output/initiative-ping-mee-portal/epic-account-self-service/story-password-change-plan.md` + summary: Known limitation, accepted in the code review of 2026-10-19: failed binds made elsewhere (authentik, other devices) count toward AD's lockout but not toward the portal's limit. + evidence: Already stated in the plan's design notes. Fixing it needs a new directory read of the bad-password count (new adapter surface). 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 bce6eb1..0e2a612 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 @@ -110,3 +110,41 @@ context: - 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). + +### Review Findings + +Code review of `ead5670..49c7fd4` on 2026-10-19 (blind hunter, edge case hunter, verification gap, acceptance auditor). + +- [x] [Review][Decision] Unreadable fine-grained policy: the attempt limit, the pause and the local hints use the domain values — a stricter policy can lock the account before the portal blocks; a looser one can be refused by the local hints [portal/profile/services.py:148] — resolved: the local hints are skipped when the policy cannot be read; the limit keeps the domain threshold minus one (never below 1) +- [x] [Review][Decision] Lockout window "never forgotten" (`lockout_window=None`): the pause falls back to 15 minutes while AD keeps counting, so the limit protects nothing [portal/profile/services.py:156] — resolved: kept as a known limitation (unusual AD setup), recorded in deferred-work.md +- [x] [Review][Decision] Failed binds made elsewhere (authentik, other devices) count toward AD's lockout but not toward the portal's limit; the design notes accept this, a fix needs a new directory read [portal/profile/services.py:148] — resolved: accepted, recorded in deferred-work.md +- [x] [Review][Decision] Unclear outcome (AD changed, portal did not commit): the user's other sessions stay alive and no failure entry is written; a test asserts the sessions stay [portal/profile/services.py:231] — resolved: the other sessions are ended and a failure entry is written on a best-effort basis +- [x] [Review][Patch] Wrong current password found at change time (race) is neither counted toward the limit nor audited [portal/profile/services.py:224] +- [x] [Review][Patch] The error log for a failure after the AD call has no change details (AD-4 asks for them: user, IP, time, step, exception) [portal/profile/services.py:234] +- [x] [Review][Patch] Lockout threshold 1 is not noted in the audit, as the design notes require [portal/profile/services.py:148] +- [x] [Review][Patch] No test checks how long the block lasts (`blocked_until` about the observation window); `test_the_pause_is_over_after_the_lockout_window` clears the block by hand and is misnamed [portal/tests/test_password_change.py] +- [x] [Review][Patch] No test checks that `sensitive_post_parameters` masks all three password fields [portal/profile/views.py:50] +- [x] [Review][Patch] Back-navigation restores the page from the back/forward cache with a disabled button and a running spinner [portal/static/portal/shell.js:53] +- [x] [Review][Patch] Several errors on one field repeat the same element id and `aria-describedby` points to only one of them [portal/templates/profile/_field.html:6] +- [x] [Review][Patch] `maxlength="256"` is hard-coded in the field template instead of coming from the form [portal/templates/profile/_field.html:5] +- [x] [Review][Patch] German formal text "kommt zu kurz nach der letzten Änderung" reads as "too short"; it means "changed too recently" [portal/locale/de/LC_MESSAGES/django.po:163] +- [x] [Review][Patch] The minimum age is rounded down (1.4 days shows as 1 day) and a minimum length of 0 shows "At least 0 characters" [portal/profile/services.py:63] +- [x] [Review][Defer] Throttle check and failure count are separate steps, so parallel requests can all pass the check before any is counted [portal/accounts/throttle.py:39] — deferred: pre-existing design of the throttle foundation (AD-7), not changed by this ticket +- [x] [Review][Defer] A timeout after AD applied the change is reported as "nothing changed" (sessions and mail skipped) [portal/profile/services.py:228] — deferred: unverified medium; settle by checking whether the adapter turns a lost response after `modify_s` into `DirectoryUnavailable` +- [x] [Review][Defer] Authenticated pages carry no `Cache-Control: no-store`, so the back button can show a page after sign-out [portal/config/settings.py] — deferred: pre-existing, applies to every signed-in page; decide once for the whole portal +- [x] [Review][Defer] AD-2 in the spine names a `msDS-Lockout*` wildcard while the code reads two attributes [ARCHITECTURE-SPINE.md] — deferred: the fix edits the architecture document + +#### Rejected + +- Invalid form while throttled re-reads the policy from AD (low): unlikely, and the fix adds a branch for no user harm; no password is checked. +- 429 page shows no form (false): the spec asks only for the message and status 429. +- "Please wait a few minutes" can be shorter than the real pause (reject): the message text is prescribed by the spec. +- Extra rule "No part of your user name or display name" (low): accurate for AD complexity, no harm. +- Invalid form with the directory down shows no notice (low): unlikely, fix adds a branch. +- User deleted mid-request gives a bare 500 (low): not reachable in normal use, fix adds a branch. +- Submit handler ignores `defaultPrevented` (false): no other submit listener exists in `shell.js`. +- Spinner has no accessible text, reduced-motion only slows it (low): cosmetic. +- German "klappt", "Administration" vs "Admin", singular wording of the history rule (low): cosmetic. +- Rules shown above the fields, "Result unclear" page added (false): disclosed judgement calls that meet the first acceptance criterion and the "outcome unknown" criterion. +- Plan task boxes unticked, `tickets.toml` header miscount, plan notes wording (reject): fix edits the spec or planning documents. +- No JavaScript tests (reject): no JS test setup exists; the plan lists the browser look as not verified. diff --git a/portal/locale/de/LC_MESSAGES/django.po b/portal/locale/de/LC_MESSAGES/django.po index 9c4015b..1368552 100644 --- a/portal/locale/de/LC_MESSAGES/django.po +++ b/portal/locale/de/LC_MESSAGES/django.po @@ -160,7 +160,7 @@ 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." +msgstr "Das neue Passwort erfüllt die oben genannten Regeln nicht, wurde schon einmal verwendet oder das Passwort wurde erst vor Kurzem geändert." msgid "Please fill in this field." msgstr "Bitte füllen Sie dieses Feld aus." diff --git a/portal/profile/services.py b/portal/profile/services.py index 49736bf..4a7435b 100644 --- a/portal/profile/services.py +++ b/portal/profile/services.py @@ -9,6 +9,7 @@ stored or put into an exception. from __future__ import annotations import logging +import math from dataclasses import dataclass from datetime import timedelta from zoneinfo import ZoneInfo @@ -62,14 +63,16 @@ class Changed: 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, + rules = [] + if policy.min_length: + rules.append( + ngettext( + "At least %(n)s character", + "At least %(n)s characters", + policy.min_length, + ) + % {"n": policy.min_length} ) - % {"n": policy.min_length} - ] if policy.complexity: rules.append( _( @@ -104,13 +107,13 @@ def describe_policy(policy: PasswordPolicy) -> list[str]: def _min_age_rule(age: timedelta) -> str: if age >= timedelta(days=1): - days = round(age.total_seconds() / 86400) + days = math.ceil(age.total_seconds() / 86400) # round up: too early is refused 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)) + hours = max(1, math.ceil(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", @@ -133,7 +136,13 @@ def _character_groups(password: str) -> int: def local_hints(new: str, policy: PasswordPolicy) -> list[str]: - """Rule keys the portal can check itself; the directory stays the authority.""" + """Rule keys the portal can check itself; the directory stays the authority. + + Nothing is claimed when a fine-grained policy applies but could not be read: the domain + values may be stricter or looser than the rules that really apply. + """ + if policy.fine_grained and not policy.fine_grained_readable: + return [] reasons = [] if len(new) < policy.min_length: reasons.append("too_short") @@ -161,6 +170,13 @@ def pause_seconds(policy: PasswordPolicy) -> int: # -- the change ----------------------------------------- +def _threshold_note(policy: PasswordPolicy | None) -> dict: + """Audit note for a lockout threshold of 1, which the portal cannot protect against.""" + if policy is not None and policy.lockout_threshold == 1: + return {"lockout_protection": "none"} + return {} + + 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} @@ -192,7 +208,7 @@ def change_password( max_failures=failure_limit(policy), pause_seconds=pause_seconds(policy), ) - _record_failure(user, ip, "wrong_current") + _record_failure(user, ip, "wrong_current", **_threshold_note(policy)) raise WrongCurrentPassword(policy) account = directory.find_user(user.sam) except DirectoryUnavailable: @@ -205,12 +221,16 @@ def change_password( raise PasswordNotAccepted(reasons, policy) changed_in_directory = False + step = "directory" try: with transaction.atomic(): directory.change_password(user.sam, current, new) changed_in_directory = True + step = "end_sessions" ended = end_sessions(user, except_session_key=session_key) + step = "queue_mail" mail_queued = _queue_mail(account) + step = "audit" audit.record( user.key, "password.changed", @@ -218,10 +238,19 @@ def change_password( detail={ "other_sessions_ended": ended, "mail": "queued" if mail_queued else "skipped", + **_threshold_note(policy), }, ) except PasswordRejected as exc: if exc.reason == "wrong_current": # the password changed between the check and the change + 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", **_threshold_note(policy)) raise WrongCurrentPassword(policy) from exc _record_failure(user, ip, "policy") raise PasswordNotAccepted([], policy) from exc @@ -231,7 +260,7 @@ def change_password( 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") + _after_unclear_outcome(user, ip, session_key, step, exc) raise OutcomeUnknown("The password was changed but not recorded") from exc raise @@ -239,6 +268,34 @@ def change_password( return Changed(mail_queued=mail_queued, other_sessions_ended=ended) +def _after_unclear_outcome( + user: SessionUser, ip: str, session_key: str | None, step: str, exc: Exception +) -> None: + """The directory has the new password but the portal's part rolled back. + + Log it with the change details (AD-4, never the exception text, which could hold input), + then do what can still be done: end the other sessions and write a failure entry. The + database may be the very thing that failed, so each attempt stands alone. + """ + logger.error( + "Password changed in the directory but the portal did not record it: " + "user=%s ip=%s time=%s step=%s error=%s", + user.key, + ip, + timezone.now().isoformat(), + step, + type(exc).__name__, + ) + try: + end_sessions(user, except_session_key=session_key) + except Exception: + logger.error("The other sessions of %s could not be ended", user.key) + try: + _record_failure(user, ip, "unclear", step=step) + except Exception: + logger.error("The failure entry for %s could not be written", user.key) + + 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 "" diff --git a/portal/static/portal/shell.js b/portal/static/portal/shell.js index 6a6cdd7..302db93 100644 --- a/portal/static/portal/shell.js +++ b/portal/static/portal/shell.js @@ -64,6 +64,19 @@ }, 0); }); + // A page restored from the back/forward cache must not come back stuck in the sending state. + window.addEventListener("pageshow", function (event) { + if (!event.persisted) { + return; + } + document.querySelectorAll("form.busy").forEach(function (form) { + form.classList.remove("busy"); + form.querySelectorAll("button[type=submit]").forEach(function (button) { + button.disabled = false; + }); + }); + }); + var focusTarget = document.querySelector("[data-focus-on-load]"); if (focusTarget) { focusTarget.focus(); diff --git a/portal/templates/profile/_field.html b/portal/templates/profile/_field.html index 0319d86..815ffd8 100644 --- a/portal/templates/profile/_field.html +++ b/portal/templates/profile/_field.html @@ -1,7 +1,7 @@ {% load i18n %}
- + {% if hint %}

{{ hint }}

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

{{ error }}

{% endfor %} + {% if field.errors %}
{% for error in field.errors %}

{{ error }}

{% endfor %}
{% endif %}
diff --git a/portal/tests/test_password_change.py b/portal/tests/test_password_change.py index 18fafd9..c9759e6 100644 --- a/portal/tests/test_password_change.py +++ b/portal/tests/test_password_change.py @@ -6,6 +6,8 @@ from datetime import timedelta import pytest from django.test import Client +from django.utils import timezone +from django.views.debug import SafeExceptionReporterFilter from portal.accounts.models import ThrottleCounter, UserSession from portal.adapters.directory import DirectoryUnavailable, PasswordPolicy, PasswordRejected @@ -238,6 +240,13 @@ def test_errors_are_next_to_the_field_and_in_a_summary(people, adapters): 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 + assert 'maxlength="256"' in html + + +def test_two_errors_on_one_field_share_one_description(people, adapters): + html = change(signed_in(adapters), new="a").content.decode() # too short and too few groups + assert html.count('id="id_new_password-error"') == 1 + assert html.count('class="error"') >= 2 # -- wrong current password and the limit ----------------------------------------- @@ -272,11 +281,21 @@ def test_the_limit_stays_below_the_directory_lockout_threshold(people, adapters) assert "password.throttled" in audit_actions() -def test_the_pause_is_over_after_the_lockout_window(people, adapters): +def test_the_block_lasts_as_long_as_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 + # the fake domain forgets failed attempts after 30 minutes: the pause must not end sooner + for subject in ("user:alice", "ip:127.0.0.1"): + left = ThrottleCounter.objects.get(subject=subject).blocked_until - timezone.now() + assert timedelta(minutes=29) < left <= timedelta(minutes=30), subject + + +def test_a_user_can_try_again_once_the_block_is_cleared(people, adapters): + client = signed_in(adapters) + for _ in range(4): + change(client, current=WRONG) + ThrottleCounter.objects.update(blocked_until=None, count=0) # as if the pause had run out assert change(client).status_code == 302 @@ -296,6 +315,18 @@ def test_the_limit_for_other_thresholds(people): assert services.pause_seconds(policy(0, None)) == 15 * 60 +def test_a_lockout_threshold_of_one_is_noted_in_the_audit(people, adapters): + adapters.directory.policy = PasswordPolicy( + 8, True, 0, timedelta(0), None, lockout_threshold=1, lockout_window=timedelta(minutes=30) + ) + change(signed_in(adapters), current=WRONG) + entry = AuditEntry.objects.get(action="password.change_failed") + assert entry.detail == {"reason": "wrong_current", "lockout_protection": "none"} + ThrottleCounter.objects.all().delete() + change(signed_in(adapters)) + assert AuditEntry.objects.get(action="password.changed").detail["lockout_protection"] == "none" + + def test_the_limit_is_per_ip_too(people, adapters): client = signed_in(adapters) for _ in range(4): @@ -322,6 +353,16 @@ def test_rules_the_portal_can_name_are_named_without_asking_the_directory( assert AuditEntry.objects.get(action="password.change_failed").detail["reason"] == "policy" +def test_without_a_readable_policy_the_portal_leaves_the_rules_to_the_directory(people, adapters): + adapters.directory.policy = PasswordPolicy( + 8, True, 0, timedelta(0), None, fine_grained=True, fine_grained_readable=False + ) + spy = Spy(adapters.directory) + response = change(signed_in(adapters), new="Ab1") + assert spy.count("change_password") == 1 # not refused by domain values that may not apply + assert "too short" not in response.content.decode() + + def test_a_rule_failure_does_not_count_as_a_wrong_password(people, adapters): client = signed_in(adapters) for _ in range(8): @@ -351,6 +392,10 @@ def test_the_password_changed_by_someone_else_in_between_counts_as_wrong( monkeypatch.setattr(adapters.directory, "change_password", raced) response = change(signed_in(adapters)) assert response.status_code == 400 and "not correct" in response.content.decode() + assert ThrottleCounter.objects.get(subject="user:alice").count == 1 + entry = AuditEntry.objects.get(action="password.change_failed") + assert entry.detail == {"reason": "wrong_current"} + assert "password.changed" not in audit_actions() # -- failures after the first directory call ----------------------------------------- @@ -393,14 +438,34 @@ def test_a_failure_after_the_directory_changed_is_reported_as_unclear( 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 + assert client.get("/").status_code == 200 # this session stays + assert other.get("/").status_code == 302 # the other one is ended as far as possible + assert "password.changed" not in audit_actions() and OutboxMail.objects.count() == 0 + entry = AuditEntry.objects.get(action="password.change_failed") + assert entry.detail == {"reason": "unclear", "step": "queue_mail"} + record = next(r for r in caplog.records if "did not record it" in r.getMessage()) + assert record.levelno == logging.ERROR + assert "user=alice" in record.getMessage() and "step=queue_mail" in record.getMessage() + assert "RuntimeError" in record.getMessage() and "database trouble" not in caplog.text # -- no secrets anywhere ----------------------------------------- +def test_the_password_fields_are_masked_in_error_reports(people, adapters, monkeypatch): + def crash(*args, **kwargs): + raise RuntimeError("crash") + + monkeypatch.setattr(services, "change_password", crash) + client = signed_in(adapters) + client.raise_request_exception = False + response = change(client) + assert response.status_code == 500 + shown = SafeExceptionReporterFilter().get_post_parameters(response.wsgi_request) + for name in ("current_password", "new_password", "confirm_password"): + assert shown[name] == SafeExceptionReporterFilter.cleansed_substitute, name + + def test_no_password_appears_in_logs_audit_session_database_or_pages(people, adapters, caplog): client = signed_in(adapters) other = signed_in(adapters) @@ -423,6 +488,13 @@ def test_no_password_appears_in_logs_audit_session_database_or_pages(people, ada # -- the rules in words ----------------------------------------- +def test_the_minimum_age_is_rounded_up_and_a_minimum_length_of_zero_is_not_listed(): + policy = PasswordPolicy(0, False, 0, timedelta(days=1, hours=10), None) + assert services.describe_policy(policy) == ["A new password can be changed again after 2 days"] + soon = PasswordPolicy(0, False, 0, timedelta(minutes=90), None) + assert services.describe_policy(soon) == ["A new password can be changed again after 2 hours"] + + 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) == [