Files
Ebbe BaßandClaude Sonnet 5.5 ae4b30ec22 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FocPJ75XPma7TcuYEMy8zm
2026-10-19 08:23:33 +02:00

17 KiB

title, type, ticket, epic, created, status, baseline_revision, route, route_source, risk, review, review_source, context
title type ticket epic created status baseline_revision route route_source risk review review_source context
Password change feature 1 epic-account-self-service 2026-10-18 built ead5670 full auto high full pinned
{project-root}/_bmad-output/architecture/architecture-ping-mee-self-service-platform-2026-10-15/ARCHITECTURE-SPINE.md
{project-root}/_bmad-output/ux-designs/ux-ping-mee-self-service-platform-2026-10-15/EXPERIENCE.md
{project-root}/_bmad-output/ux-designs/ux-ping-mee-self-service-platform-2026-10-15/mockups/key-password.html

Intent

Problem: Every password change still goes through the admin. A signed-in user has no way to change their own AD password, see the rules first, or be told what happened.

Approach: A new profile app (spine: password change and Password Policy live in profile) with a service change_password and one page at /password. The page lists the user's password policy read from AD, asks for the current password and the new one twice, and applies the change over LDAPS through the directory adapter. After a success the user's other sessions end and a confirmation e-mail is queued, all in one transaction with the audit entry.

Boundaries & Constraints

Always: AD-2, AD-3, AD-4, AD-8, AD-17 apply. The acting user is read from the session only; the view and service accept no username. The order is fixed: validate the form locally, throttle.check (before any AD call), verify_password, local policy hints, change_password inside the transaction with audit, queued mail and ended sessions. A failed AD call rolls back and writes a failure entry after the rollback. Passwords exist only in local variables: never logged, audited, stored, put in an exception message, echoed back into the form, or kept in the session. The view is marked sensitive_post_parameters. Messages are plain words with no technical terms and the pause is never shown in numbers. Every new string has German formal text, informal where it differs, in portal/locale. The mail names the time of the change with its time zone and never contains the password.

Never: No change to another user, no username in the URL or form. No password reset for forgotten or expired passwords (the page says to contact the Admin). No new AD write path: the adapter's change_password is the only one. No storage of the current password for later steps. No JavaScript requirement: the form works as a plain POST.

I/O & Edge-Case Matrix

Scenario Input / State Expected Output / Behavior Error Handling
Open the page Signed-in user Effect text, the policy rules in words, three empty fields; menu entry "Change password" active Directory down: the page says it is not possible now, no form
Policy shown Domain policy, or a fine-grained policy that applies Minimum length, character kinds when complexity is on, how many old passwords are remembered, minimum age, expiry Fine-grained policy not readable: rules shown with a note that a stricter rule may apply
Success Correct current, new password accepted by AD AD changed; other sessions of the user ended, this one kept; password.changed audited; e-mail queued; redirect to a done page n/a
Wrong current Current password does not match "The current password is not correct." Nothing changed; counts toward the limit; failure audited No AD change is attempted
Limit reached The effective number of failed current-password checks (5, or the AD lockout threshold minus 1 when that is lower) "Too many attempts. Please wait a few minutes." (HTTP 429), no AD call, no mail; throttled attempt audited Recovers after the pause or after a success
Mismatch or empty New and repeat differ, a field empty, over 256 characters Error under the field and in a summary at the top; nothing sent to AD; not counted n/a
Same as current New equals current "The new password must be different." Not sent to AD n/a
Locally detectable rule Shorter than the minimum, or too few character kinds Specific reason ("The password is too short.") after the current password was verified; not sent to AD; not counted n/a
AD rejects the new password History, age or another rule "The new password does not meet the rules above, or it was used or changed too recently." Nothing changed; audited as policy; not counted n/a
No usable e-mail address AD mail empty or invalid Password changes anyway; no mail; the done page omits the e-mail sentence; audit detail records mail: skipped n/a
Directory down during the change AD unavailable "This is not possible right now. Please try again later." Nothing changes; failure audited Transaction rolled back
Not signed in Any request Redirect to sign-in n/a
Wrong method or CSRF GET to the change, POST without token 405 or 403; nothing changes n/a

Code Map

  • portal/profile/ -- new app: services.py (change_password, describe_policy, local_hints; typed errors), forms.py (PasswordChangeForm, three password inputs with render_value=False), views.py (password, password_done), urls.py, apps.py. No models.
  • portal/templates/profile/password.html, password_done.html, portal/templates/mail/password_changed/{subject,body}.txt.
  • portal/shell/menu.py -- add the entry "Change password" for everyone.
  • portal/static/portal/shell.js and shell.css -- on submit the primary button gets a spinner and is disabled (without blocking the submit); the error summary takes focus on load. Both optional enhancements.
  • portal/config/settings.py, urls.py -- register the app and the routes.
  • Reuse, unchanged: accounts.throttle (check, hit, reset), accounts.sessions.end_sessions, audit.services, mailer.services.enqueue, adapters.directory, accounts.decorators.login_required.
  • portal/tests/ -- test_password_change.py (service and view with the fake directory and fake mailer), additions to test_translations.py as needed.

Tasks & Acceptance

Execution:

  • profile/services.py -- the ordered flow, throttle counting only for a wrong current password with the limit derived from the policy's lockout threshold, one transaction for AD call, audit, mail and session ending -- AD-4, AD-8
  • profile/services.py -- policy in words and the local hints (minimum length, character kinds) -- FR-5
  • profile/forms.py, views.py, templates, menu entry -- the page, the done page, the error mapping to messages and status codes -- AD-17
  • mail template password_changed in German formal with the time and zone, no password
  • shell.js and shell.css -- spinner and focus on the summary
  • German and informal German text for every new string
  • Tests for every matrix row, plus: no password in logs, audit, session or response; another user's sessions untouched; a rolled-back change queues no mail and ends no session

Acceptance Criteria:

  • Given a signed-in user, when they open /password, then the policy rules from the directory are listed before any field and the page states the effect.
  • Given the correct current password and an acceptable new one, when they submit, then the directory password changed, their other sessions are gone and this one stays, an audit entry exists and one mail is queued.
  • Given a wrong current password as often as the effective limit (5, or lower when AD locks sooner), when the next request arrives, then it is refused with 429 before any directory call and the attempt is audited.
  • Given AD refuses the new password, when they submit, then nothing changed, no session ended, no mail queued, and the message is in plain words.
  • Given any request, then no password appears in a log line, an audit entry, the session, the database or the rendered response.
  • Given a failure after the AD call inside the transaction, then the error is logged at error level with the change details (AD-4) and the user is told the outcome is unknown.

Design Notes

Counting. Only a wrong current password counts toward the limit (FR-6). Policy rejections and local mistakes do not: they happen after the current password was proven, and punishing them would lock out people who are only choosing a password. The limit uses the existing password-change endpoint counters per user and per IP with the default of 5 failures and a 15-minute pause.

Account lockout. verify_password binds as the user, so every wrong attempt also raises AD's own bad-password count, and attempts made elsewhere (for example at authentik) count too. The directory adapter therefore reads lockoutThreshold and lockoutObservationWindow (or the fine-grained msDS-Lockout* values) with the password policy (built first, on its own branch, and merged before this ticket). The portal's limit is min(5, threshold - 1) and at least 1; with a threshold of 0 (the domain never locks) it stays 5. The pause is at least the observation window. A threshold of 1 cannot be protected from the portal; the page still works and the audit notes it.

One verification per change. The portal verifies the current password once, then calls the adapter's change_password, which authenticates as the user again. That is a second correct bind, not a second chance for an attacker.

Local hints. The hints (too short, too few character kinds) only improve the message; AD stays the authority. Rules the portal cannot know (history, minimum age, parts of the name) collapse into the generic policy message.

Status codes. Validation and policy errors re-render the form with 400, the limit gives 429, the directory being down gives 503, success redirects (POST, redirect, GET) to the done page, which reads a one-time session flag for the e-mail sentence.

Expired passwords. A user whose password has expired cannot bind, so their current-password check fails and counts. The sign-in already sends such users to the Admin; the page text says the same.

Verification

Commands:

  • uv run pytest -- expected: all tests pass, including the new ones
  • uv run ruff check portal manage.py docker -- expected: no findings
  • 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).

Review Findings

Code review of ead5670..49c7fd4 on 2026-10-19 (blind hunter, edge case hunter, verification gap, acceptance auditor).

  • [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)
  • [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
  • [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
  • [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
  • [Review][Patch] Wrong current password found at change time (race) is neither counted toward the limit nor audited [portal/profile/services.py:224]
  • [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]
  • [Review][Patch] Lockout threshold 1 is not noted in the audit, as the design notes require [portal/profile/services.py:148]
  • [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]
  • [Review][Patch] No test checks that sensitive_post_parameters masks all three password fields [portal/profile/views.py:50]
  • [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]
  • [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]
  • [Review][Patch] maxlength="256" is hard-coded in the field template instead of coming from the form [portal/templates/profile/_field.html:5]
  • [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]
  • [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]
  • [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
  • [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
  • [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
  • [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.