Add authentication audit logging and metrics by Tim020 · Pull Request #1465 · dreamteamprod/DigiScript

Tim020 · 2026-10-22T17:35:25Z

Records login, logout, password change/reset, token refresh, API token generate/revoke (v1 and v2) and user create/update/delete (v1 and v2) on DigiScript.Audit (INFO success, WARNING failure) with username, user_id and remote_ip, and counts them in digiscript_auth_events_total{event,result} (fixed event set, bounded labels). Failed logins log unknown_user vs bad_password reasons; HTTP responses are unchanged. User management events log the acting admin plus target_user_id. Usernames are sanitised (control chars stripped, 64 cap, JSON-quoted); secrets are never logged. Docs updated in user_config.md.

Tests: full pytest 1176 passed; full Playwright 225 passed (chromium); audit lines inspected in the server log.

🤖 Generated with Claude Code

Log login, logout, password change/reset, token refresh and API token generate/revoke on DigiScript.Audit with sanitised usernames, never including secrets, and count them in a bounded Prometheus counter. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Adds user_create, user_update and user_delete audit events and covers the v2 users and token controllers plus v1 user create/delete. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Tim020

Review round 1

Head SHA: 68ff3fb6a87b0ad318404a60ee3fa3600775c98d

Verdict: Changes needed, but non-blocking — the audit-logging/metrics design itself is solid (fixed-set metric labels, sanitised + JSON-quoted usernames, reST docstrings, strong existing test discipline around secret-leakage). The issues below are accuracy/polish and test-coverage gaps, not correctness bugs that break the feature.

Previous round follow-up: N/A — this is round 1.

SonarQube: Quality gate PASSED. All 5 configured conditions pass (new reliability/security/maintainability ratings, 2.5% duplication vs 3% threshold, 100% security hotspots reviewed — no coverage condition is configured in this gate, so the check-run's "0.0% Coverage on New Code" is informational only and doesn't gate the PR). 4 open MINOR/MAJOR code-smell issues were flagged by Sonar's new-code window (python:S7504 ×2 in server/utils/web/app_metrics.py, python:S5906 and python:S8997 in server/test/digi_server/test_logger.py) — none of these files are touched by this PR's diff (base is the stacked branch feature/obs-monitoring-docs), so they're pre-existing debt from an earlier PR in the stack, surfaced here only by Sonar's new-code period. Not actionable in this PR.

New findings: 6 Important, 9 Suggestion. 9 posted inline; 6 couldn't be anchored to a changed line and are listed below.

Not anchored to a diff line:

Important — Every audit() call on a success path sits unguarded between the DB commit and self.finish() (e.g. server/controllers/api/v1/auth/user.py:104,244,334,359,477, token.py:28,68, and the v2 equivalents). If audit() ever raised, the handler would 500 even though the real mutation (login, user create/delete, password change, token issue) already succeeded — and for user_create/user_delete it would also skip the GET_USERS broadcast. Not reachable today (every call site passes a hardcoded literal from EVENTS/RESULTS, and audit()'s internals can't currently throw), but it's an unguarded single point of failure that will get copied to future call sites. Consider a narrow try/except inside audit() itself that logs+swallows rather than propagates.
Suggestion — AdminPasswordResetController doesn't audit two of its refusal branches: "User not found" (server/controllers/api/v1/auth/user.py:523, 404) and "Cannot reset your own password" (:529, 400) — both pre-existing, unchanged by this PR, hence not inline-commentable here. user_delete's only_admin refusal is audited; for parity, consider auditing these too with reason=not_found / reason=self_reset.
Important — No test asserts that auth_events_total's label set stays restricted to {event, result} (excludes username or other per-request values) — the cardinality-safety claim in auth_audit.py's docstring is never directly checked, e.g. via auth_events_total._labelnames or two different usernames collapsing to the same series.
Suggestion — The only_admin failure branches for user_delete (v1 user.py:145) and user_update/user_delete (v2 users.py:140,198) have no audit-assertion test (plausibly hard to reach given the admin-count guard, but untested either way).
Suggestion — The "first user, no actor" case (test_auth.py and test_users.py bootstrap-create tests) only asserts the message contains "AUDIT user_create success"; it never asserts username/user_id are absent. A regression that attributed the bootstrap event to a stale actor would pass.
Suggestion — password_reset failure via reason=invalid_password (triggered by change_password raising ValueError, user.py:546) has no audit-assertion test; existing reset tests cover not-found/self-reset/missing-id but not this branch.

Strengths: Thorough existing test coverage pairs HTTP-response assertions with assertLogs + Prometheus counter-delta checks at nearly all ~28 call sites, not just status codes; _assert_no_secrets / _no_secrets helpers give a solid regression guard against accidentally logging credentials; the sanitisation design (control-char stripping + truncation + JSON-quoting for usernames, fixed label sets for metrics) is the right shape for this kind of feature; reST docstrings are properly formatted throughout; test_auth_audit.py correctly stays on plain unittest.TestCase since it touches no DB/settings/temp paths.

🤖 Generated with Claude Code

audit() now logs and swallows failures instead of raising, every (event, result) series is pre-initialised, ip is quoted unless a valid address, reset-password refusals are audited, v1 token controllers use actor(), and docs/tests are tightened. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Tim020 · 2026-10-22T22:20:46Z

Review round 1 response (fixes in df45bda). pytest 1216 passed, ruff check and format --check clean.

Decision for Tim to confirm: on an unknown_user login failure the attempted username is still logged (sanitised, capped at 64, JSON-quoted), like sshd, because that is the audit's value. A password mistyped into the username field will therefore appear in the log. The docs no longer claim secrets are never logged; they now say "never deliberately logged", that the username is recorded as typed, and that logs should be treated as sensitive.

Unanchored findings

audit() could fail a request after the mutation succeeded: fixed. audit() now never raises. Internal failures are logged with exc_info and swallowed. Unknown event/result is logged at ERROR and not counted, so labels stay bounded. Tests added for both.
Label-set cardinality not tested: fixed. Tests assert _labelnames == (event, result) and that different usernames create no new series.
reset-password refusal branches unaudited: fixed. reason=not_found and reason=self_reset, with tests.
only_admin branches for user_delete (v1) and user_update/user_delete (v2) untested: rejected. They are unreachable through the API. require_admin means the caller is an admin, so a different admin target implies at least two admins. They stay as defensive guards that are audited.
Bootstrap create only checked the message: fixed. The v1 and v2 tests now assert the line has no username=/ user_id= and the record has no such attributes.
password_reset reason=invalid_password untested: fixed, by patching change_password to raise ValueError.

Inline findings

Docs example for bad_password lacked user_id: fixed. Added an unknown_user example without user_id.
Stale module docstring: fixed. It now points at EVENTS.
Raw username claim contradicting the docs: see the decision above.
Weak log-injection test: fixed. It now asserts the exact rendered message, that the lookalike ip= stays inside the quoted username, and that the real ip= appears once.
Pre-initialise label pairs: fixed, with a test.
bool() in the user_update changed field: fixed. It now logs data[field]!r.
v1 token.py hand-spelled actor fields: fixed. It uses actor(self).
Stray blank line in the docs list: fixed.
ip not quoted: fixed. ip is bare only when it parses as an IP address, otherwise it is JSON-quoted, with tests. The module docstring claim was narrowed to match.

…ing conflicts with dev's login hardening LoginHandler now follows dev's throttled flow (no per-username lock, dummy bcrypt for unknown users) with the audit calls re-applied to each outcome, including a new 'reason=throttled' audit line for lockouts. Test imports are the union of both sides, plus a test for the throttled audit line. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

github-actions · 2026-10-22T22:33:22Z