feat: add audit, throttle and session foundations
Audit entries written only inside the caller's transaction, database throttle counters that refuse to run inside a transaction that could roll back, and a database session store with the user key in its own column, end_sessions and idle and absolute limits. Ticket 1.3. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sk72AxLwQnYttF6VFSxQLt
This commit is contained in:
Ebbe Baß
co-authored by
Claude Sonnet 5.5
Claude Sonnet 5.5
parent
003ce6e0c2
commit
73505ac717
@@ -1,3 +1,6 @@
|
||||
- source_plan: `_bmad-output/initiative-ping-mee-portal/epic-platform-and-sign-in/story-prepare-the-test-environment-plan.md`
|
||||
summary: ARCHITECTURE-SPINE AD-2 says the service account's read rights are limited to a list, but Active Directory gives authenticated users broad read by default, so the statement is not enforced by the test-environment setup.
|
||||
evidence: The setup script grants only write rights and the Change Password right; read access is the default. Either soften AD-2 ("needs no more than this list") or enforce it with deny entries; decide when the architecture is next updated.
|
||||
- source_plan: `_bmad-output/initiative-ping-mee-portal/epic-platform-and-sign-in/story-audit-throttle-and-session-foundations-plan.md`
|
||||
summary: ARCHITECTURE-SPINE AD-6 names `end_sessions(username, except_current=bool)`, but the built function is `end_sessions(user, except_session_key=None)`.
|
||||
evidence: A function that keeps the current session needs that session's key; a boolean cannot say which one. Reword AD-6 to match when the architecture is next updated; callers use the key form.
|
||||
|
||||
+108
@@ -0,0 +1,108 @@
|
||||
---
|
||||
title: 'Audit, throttle and session foundations'
|
||||
type: 'feature'
|
||||
ticket: '3'
|
||||
created: '2026-10-15'
|
||||
status: 'built'
|
||||
baseline_revision: '003ce6e0c2e4e228f8e93df030b78396b861b613'
|
||||
route: 'full'
|
||||
route_source: 'auto'
|
||||
risk: 'medium'
|
||||
review: 'quick'
|
||||
review_source: 'pinned'
|
||||
lenses_ran: ['quick']
|
||||
review_loop_iteration: 0
|
||||
context:
|
||||
- '{project-root}/_bmad-output/architecture/architecture-ping-mee-self-service-platform-2026-10-15/ARCHITECTURE-SPINE.md'
|
||||
---
|
||||
|
||||
<frozen-after-approval reason="human-owned intent — do not modify unless human renegotiates">
|
||||
|
||||
## Intent
|
||||
|
||||
**Problem:** Every later feature must write an audit entry with its change, refuse floods before calling Active Directory, and end a user's sessions on demand. None of these shared services exists yet.
|
||||
|
||||
**Approach:** Add two Django apps. `audit` owns audit entries and a service that records them only inside the caller's transaction. `accounts` owns throttle counters and a database session store that keeps the username in its own column, with `end_sessions`, idle and absolute limits, and the `SessionUser` object.
|
||||
|
||||
## Boundaries & Constraints
|
||||
|
||||
**Always:** Spine rules AD-4, AD-6, AD-7 and AD-19 apply: only the owning app writes its tables; other code calls its service. Usernames are stored lower-case as `key`; `sam` keeps the exact case. Timestamps are UTC. Idle limit 1 hour and absolute limit 4 hours are named settings with those defaults. A throttle default of 5 failures and a 15-minute pause is a named constant, ready for the settings registry. Entries and counters never hold passwords, tokens or picture content.
|
||||
|
||||
**Never:** No sign-in, OIDC, AD, mail or Admin-page code. No client-IP derivation (the sign-in entry owns it; here the IP is a string passed in). No other way to end sessions than `end_sessions`. No raw SQL.
|
||||
|
||||
## I/O & Edge-Case Matrix
|
||||
|
||||
| Scenario | Input / State | Expected Output / Behavior | Error Handling |
|
||||
|----------|--------------|---------------------------|----------------|
|
||||
| Audit in transaction | `record()` inside `transaction.atomic()`, then the block fails | Entry is rolled back with the change | No error expected |
|
||||
| Audit outside transaction | `record()` called with no open transaction | Raises `AuditOutsideTransaction` | Nothing is written |
|
||||
| Secret in detail | detail key like `password`, `token`, `secret` or `pwd` | Raises `ValueError` | Nothing is written |
|
||||
| Failure entry | `record_failure()` after a rolled-back change | Entry persists in its own transaction | No error expected |
|
||||
| Throttle refuses | Limit failures reached for endpoint plus user, or plus IP | `check()` raises `Throttled` with seconds to wait | No AD call is made by the caller |
|
||||
| Throttle recovers | Pause elapsed, or `reset()` after success | `check()` passes again | No error expected |
|
||||
| Sessions by user | Two users, several sessions each | `end_sessions(user)` deletes all of that user's sessions only; `except_session_key` keeps one | Unknown user returns 0 |
|
||||
| Session limits | Idle over 1 hour, or older than 4 hours | Next request has no session | Cookie cleared |
|
||||
|
||||
</frozen-after-approval>
|
||||
|
||||
## Code Map
|
||||
|
||||
- `portal/config/settings.py` -- add the two apps, `SESSION_ENGINE = "portal.accounts.session_store"`, `SESSION_COOKIE_AGE` (idle), `SESSION_SAVE_EVERY_REQUEST = True`, `SESSION_ABSOLUTE_SECONDS`, and the middleware after `SessionMiddleware`. Existing: DB sessions, secure cookies, UTC.
|
||||
- `portal/audit/` -- new app: `models.py` (`AuditEntry`: actor key, action, target, ip, created_at, detail JSON), `services.py` (`record`, `record_failure`), migration.
|
||||
- `portal/accounts/` -- new app: `models.py` (`UserSession(AbstractBaseSession)` with `username`, `created_at`; `ThrottleCounter`), `session_store.py`, `sessions.py` (`SessionUser`, `start_session`, `current_user`, `end_sessions`), `throttle.py` (`check`, `hit`, `reset`, `Throttled`), `middleware.py` (absolute limit), migration.
|
||||
- `portal/tests/` -- `test_audit.py`, `test_throttle.py`, `test_sessions.py`.
|
||||
- Reuse: Django `AbstractBaseSession` and `SessionStore`; do not change `portal/config/middleware.py`.
|
||||
|
||||
## Tasks & Acceptance
|
||||
|
||||
**Execution:**
|
||||
- [ ] `portal/audit/` -- model, service with the transaction guard and secret-key guard, migration -- AD-4
|
||||
- [ ] `portal/accounts/models.py`, `session_store.py`, `sessions.py`, `middleware.py` -- session store with a username column, `SessionUser`, `start_session` (rotates the key), `end_sessions`, absolute limit -- AD-6
|
||||
- [ ] `portal/accounts/throttle.py` -- atomic counters keyed by endpoint plus subject (user or IP), pause, reset, injectable clock -- AD-7
|
||||
- [ ] `portal/config/settings.py` -- register apps, store and middleware -- wiring
|
||||
- [ ] `portal/tests/` -- unit-test every matrix row
|
||||
|
||||
**Acceptance Criteria:**
|
||||
- Given a failing change, when it runs in a transaction that records an audit entry, then neither is stored.
|
||||
- Given 5 recorded failures for one user at one endpoint, when `check` runs, then it raises `Throttled`, while another user and another IP at that endpoint still pass.
|
||||
- Given three sessions of user A and two of user B, when `end_sessions("a")` runs, then A has none and B keeps two.
|
||||
- Given a session older than the absolute limit, when a request arrives, then the session is gone.
|
||||
- Given the migrations, when `makemigrations --check` runs, then no changes are reported.
|
||||
|
||||
## Implementation Notes
|
||||
|
||||
- Implemented by a coding subagent; the orchestrator read the code and re-ran `uv run pytest` (40 passed, none skipped), `ruff check .` (clean) and `makemigrations --check --dry-run` (no changes).
|
||||
- Judgement calls: `**/migrations` added to ruff's `extend-exclude`; `reset()` clears only the subjects passed in, so the caller decides about the IP counter; the failure window equals the pause (15 minutes) because the plan did not fix it; `django.contrib.sessions` stays installed (its unused `django_session` table is migrated, harmless); audit tests use `transaction=True` so the "outside a transaction" case is real.
|
||||
- Not tested: concurrent writers on the throttle counters.
|
||||
- After review: `hit()` and `record_failure()` now refuse to run inside an open transaction, `reset()` takes only a user, and the throttle functions refuse an empty subject. Callers must therefore count failures after the failed work was rolled back, not inside it.
|
||||
|
||||
## Plan Change Log
|
||||
|
||||
## Review Triage Log
|
||||
|
||||
Pass 1 (quick lens). Counts: high 0, medium 5, low 5, false 1.
|
||||
|
||||
| # | Finding | Verdict | Route | Evidence |
|
||||
|---|---------|---------|-------|----------|
|
||||
| 1 | `record_failure` inside an open transaction is lost on rollback | medium | patch | Nested `atomic()` is a savepoint; now raises `AuditInsideTransaction`, tested. |
|
||||
| 2 | `hit()` inside a transaction that rolls back erases the count, so the throttle never trips | medium | patch | Same savepoint effect on a failed AD call (AD-4); now raises when a transaction is open, tested. |
|
||||
| 3 | Throttle with no user and no IP silently does nothing | medium | patch | `_subjects` returned `[]`; now raises `ValueError` in `check`, `hit` and `reset`, tested. |
|
||||
| 4 | `reset(ip=...)` lets an attacker with one valid account clear the IP counter | medium | patch | `reset` now takes only `user`; IP counters expire on their own; tested. |
|
||||
| 5 | `end_sessions` takes `except_session_key`, the spine says `except_current=bool` | low | defer | The implementation is the workable form (a session needs its key); the fix is to reword AD-6, a spec edit. See deferred-work. |
|
||||
| 6 | Lower-casing of the key is duplicated | low | rejected | Harmless defence; both lower-case the same value. |
|
||||
| 7 | Absolute limit only covers sessions made by `start_session` | low | rejected | `start_session` is the only sign-in path. |
|
||||
| 8 | Unused session table; limits are constants, not settings | false | rejected | They are named Django settings; the table is harmless and the settings registry is a later entry. |
|
||||
| 9 | `**/migrations` excluded from lint | low | rejected | Generated files; disclosed. |
|
||||
| 10 | Idle limit tested only on a store, not through a request | medium | patch | Two request-level tests added (renewal, expired session with cleared cookie). |
|
||||
| 11 | No concurrency test for `hit()` | low | rejected | Disclosed; SQLite serializes writers (`IMMEDIATE`), a concurrent test needs threads and a file database. |
|
||||
|
||||
## Design Notes
|
||||
|
||||
Throttle storage is one row per (endpoint, subject) with `count`, `window_started` and `blocked_until`; updates use database expressions inside a transaction so concurrent requests count correctly (the database serializes writes). `check` never writes. The clock is a parameter so tests need no sleeping. The settings registry that later feeds the limits is a different entry.
|
||||
|
||||
## Verification
|
||||
|
||||
**Commands:**
|
||||
- `uv run pytest` -- expected: all tests pass, including the new ones
|
||||
- `uv run ruff check .` -- expected: no findings
|
||||
- `uv run python manage.py makemigrations --check --dry-run` -- expected: no changes detected (with a valid test environment)
|
||||
Reference in New Issue
Block a user