From 73505ac7170ff4d960ab4cd1961897e4c030c389 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ebbe=20Ba=C3=9F?= Date: Thu, 15 Oct 2026 15:43:43 +0200 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01Sk72AxLwQnYttF6VFSxQLt --- .../deferred-work.md | 3 + ...t-throttle-and-session-foundations-plan.md | 108 +++++++++++++++ portal/accounts/__init__.py | 0 portal/accounts/apps.py | 6 + portal/accounts/middleware.py | 20 +++ portal/accounts/migrations/0001_initial.py | 44 +++++++ portal/accounts/migrations/__init__.py | 0 portal/accounts/models.py | 31 +++++ portal/accounts/session_store.py | 29 +++++ portal/accounts/sessions.py | 52 ++++++++ portal/accounts/throttle.py | 98 ++++++++++++++ portal/audit/__init__.py | 0 portal/audit/apps.py | 6 + portal/audit/migrations/0001_initial.py | 30 +++++ portal/audit/migrations/__init__.py | 0 portal/audit/models.py | 16 +++ portal/audit/services.py | 79 +++++++++++ portal/config/settings.py | 8 +- portal/tests/test_audit.py | 59 +++++++++ portal/tests/test_sessions.py | 123 ++++++++++++++++++ portal/tests/test_throttle.py | 91 +++++++++++++ pyproject.toml | 2 +- 22 files changed, 803 insertions(+), 2 deletions(-) create mode 100644 _bmad-output/initiative-ping-mee-portal/epic-platform-and-sign-in/story-audit-throttle-and-session-foundations-plan.md create mode 100644 portal/accounts/__init__.py create mode 100644 portal/accounts/apps.py create mode 100644 portal/accounts/middleware.py create mode 100644 portal/accounts/migrations/0001_initial.py create mode 100644 portal/accounts/migrations/__init__.py create mode 100644 portal/accounts/models.py create mode 100644 portal/accounts/session_store.py create mode 100644 portal/accounts/sessions.py create mode 100644 portal/accounts/throttle.py create mode 100644 portal/audit/__init__.py create mode 100644 portal/audit/apps.py create mode 100644 portal/audit/migrations/0001_initial.py create mode 100644 portal/audit/migrations/__init__.py create mode 100644 portal/audit/models.py create mode 100644 portal/audit/services.py create mode 100644 portal/tests/test_audit.py create mode 100644 portal/tests/test_sessions.py create mode 100644 portal/tests/test_throttle.py diff --git a/_bmad-output/initiative-ping-mee-portal/deferred-work.md b/_bmad-output/initiative-ping-mee-portal/deferred-work.md index a7a70d9..363bf0a 100644 --- a/_bmad-output/initiative-ping-mee-portal/deferred-work.md +++ b/_bmad-output/initiative-ping-mee-portal/deferred-work.md @@ -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. diff --git a/_bmad-output/initiative-ping-mee-portal/epic-platform-and-sign-in/story-audit-throttle-and-session-foundations-plan.md b/_bmad-output/initiative-ping-mee-portal/epic-platform-and-sign-in/story-audit-throttle-and-session-foundations-plan.md new file mode 100644 index 0000000..1c819d1 --- /dev/null +++ b/_bmad-output/initiative-ping-mee-portal/epic-platform-and-sign-in/story-audit-throttle-and-session-foundations-plan.md @@ -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' +--- + + + +## 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 | + + + +## 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) diff --git a/portal/accounts/__init__.py b/portal/accounts/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/portal/accounts/apps.py b/portal/accounts/apps.py new file mode 100644 index 0000000..f864ce7 --- /dev/null +++ b/portal/accounts/apps.py @@ -0,0 +1,6 @@ +from django.apps import AppConfig + + +class AccountsConfig(AppConfig): + name = "portal.accounts" + label = "accounts" diff --git a/portal/accounts/middleware.py b/portal/accounts/middleware.py new file mode 100644 index 0000000..9af228e --- /dev/null +++ b/portal/accounts/middleware.py @@ -0,0 +1,20 @@ +"""Absolute session limit; the idle limit is SESSION_COOKIE_AGE with save-every-request.""" + +import time + +from django.conf import settings + +from .session_store import CREATED_AT + + +class AbsoluteSessionLimitMiddleware: + def __init__(self, get_response): + self.get_response = get_response + + def __call__(self, request): + session = request.session + created = session.get(CREATED_AT) + if created is not None and time.time() - created > settings.SESSION_ABSOLUTE_SECONDS: + # flush() deletes the row; the empty session makes SessionMiddleware clear the cookie. + session.flush() + return self.get_response(request) diff --git a/portal/accounts/migrations/0001_initial.py b/portal/accounts/migrations/0001_initial.py new file mode 100644 index 0000000..a688992 --- /dev/null +++ b/portal/accounts/migrations/0001_initial.py @@ -0,0 +1,44 @@ +# Generated by Django 5.2.18 on 2026-10-15 13:40 + +import django.utils.timezone +from django.db import migrations, models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ] + + operations = [ + migrations.CreateModel( + name='UserSession', + fields=[ + ('session_key', models.CharField(max_length=40, primary_key=True, serialize=False, verbose_name='session key')), + ('session_data', models.TextField(verbose_name='session data')), + ('expire_date', models.DateTimeField(db_index=True, verbose_name='expire date')), + ('username', models.CharField(blank=True, db_index=True, default='', max_length=256)), + ('created_at', models.DateTimeField(default=django.utils.timezone.now)), + ], + options={ + 'verbose_name': 'session', + 'verbose_name_plural': 'sessions', + 'abstract': False, + }, + ), + migrations.CreateModel( + name='ThrottleCounter', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('endpoint', models.CharField(max_length=64)), + ('subject', models.CharField(max_length=256)), + ('count', models.PositiveIntegerField(default=0)), + ('window_started', models.DateTimeField(default=django.utils.timezone.now)), + ('blocked_until', models.DateTimeField(blank=True, null=True)), + ], + options={ + 'constraints': [models.UniqueConstraint(fields=('endpoint', 'subject'), name='uniq_throttle_subject')], + }, + ), + ] diff --git a/portal/accounts/migrations/__init__.py b/portal/accounts/migrations/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/portal/accounts/models.py b/portal/accounts/models.py new file mode 100644 index 0000000..f9c6ddc --- /dev/null +++ b/portal/accounts/models.py @@ -0,0 +1,31 @@ +from django.contrib.sessions.base_session import AbstractBaseSession +from django.db import models +from django.utils import timezone + + +class UserSession(AbstractBaseSession): + """Database session with the user key in its own column (AD-6).""" + + username = models.CharField(max_length=256, blank=True, default="", db_index=True) + created_at = models.DateTimeField(default=timezone.now) # UTC + + @classmethod + def get_session_store_class(cls): + from .session_store import SessionStore + + return SessionStore + + +class ThrottleCounter(models.Model): + """One row per (endpoint, subject); subject is `user:` or `ip:
` (AD-7).""" + + endpoint = models.CharField(max_length=64) + subject = models.CharField(max_length=256) + count = models.PositiveIntegerField(default=0) + window_started = models.DateTimeField(default=timezone.now) + blocked_until = models.DateTimeField(null=True, blank=True) + + class Meta: + constraints = [ + models.UniqueConstraint(fields=["endpoint", "subject"], name="uniq_throttle_subject") + ] diff --git a/portal/accounts/session_store.py b/portal/accounts/session_store.py new file mode 100644 index 0000000..c0aeb39 --- /dev/null +++ b/portal/accounts/session_store.py @@ -0,0 +1,29 @@ +"""Database session store that fills the username and created_at columns.""" + +from __future__ import annotations + +from datetime import UTC, datetime + +from django.contrib.sessions.backends.db import SessionStore as DBSessionStore +from django.utils import timezone + +from .models import UserSession + +USER_KEY = "session_user_key" +USER_SAM = "session_user_sam" +CREATED_AT = "session_created_at" # epoch seconds, UTC + + +class SessionStore(DBSessionStore): + @classmethod + def get_model_class(cls): + return UserSession + + def create_model_instance(self, data): + obj = super().create_model_instance(data) + obj.username = str(data.get(USER_KEY, "")).lower() + created = data.get(CREATED_AT) + obj.created_at = ( + datetime.fromtimestamp(created, tz=UTC) if created is not None else timezone.now() + ) + return obj diff --git a/portal/accounts/sessions.py b/portal/accounts/sessions.py new file mode 100644 index 0000000..0f2a0ef --- /dev/null +++ b/portal/accounts/sessions.py @@ -0,0 +1,52 @@ +"""Session user and the only way to end sessions (AD-3, AD-6).""" + +from __future__ import annotations + +import time +from dataclasses import dataclass + +from .models import UserSession +from .session_store import CREATED_AT, USER_KEY, USER_SAM + + +@dataclass(frozen=True) +class SessionUser: + key: str # lower-case, used for every database key + sam: str # exact sAMAccountName from AD + + @classmethod + def from_sam(cls, sam: str) -> SessionUser: + return cls(key=sam.lower(), sam=sam) + + +def start_session(request, sam: str) -> SessionUser: + """Sign a user in on this request; the session key is rotated and old data dropped.""" + user = SessionUser.from_sam(sam) + request.session.flush() + request.session[USER_KEY] = user.key + request.session[USER_SAM] = user.sam + request.session[CREATED_AT] = time.time() + return user + + +def current_user(request) -> SessionUser | None: + session = getattr(request, "session", None) + if session is None: + return None + key = session.get(USER_KEY) + sam = session.get(USER_SAM) + if not key or not sam: + return None + return SessionUser(key=key, sam=sam) + + +def end_sessions(user: SessionUser | str, except_session_key: str | None = None) -> int: + """Delete every session of one user, optionally keeping one. Returns how many were ended.""" + key = (user.key if isinstance(user, SessionUser) else str(user)).lower() + if not key: + return 0 + qs = UserSession.objects.filter(username=key) + if except_session_key: + qs = qs.exclude(session_key=except_session_key) + deleted, _ = qs.delete() + return deleted diff --git a/portal/accounts/throttle.py b/portal/accounts/throttle.py new file mode 100644 index 0000000..76e525f --- /dev/null +++ b/portal/accounts/throttle.py @@ -0,0 +1,98 @@ +"""Database throttle counters, checked before any outbound call (AD-7).""" + +from __future__ import annotations + +import math +from collections.abc import Callable +from datetime import datetime, timedelta + +from django.db import connection, transaction +from django.db.models import F, Q +from django.utils import timezone + +from .models import ThrottleCounter + +# Defaults, ready to move into the settings registry (AD-13). +THROTTLE_MAX_FAILURES = 5 +THROTTLE_PAUSE_SECONDS = 15 * 60 + +Clock = Callable[[], datetime] + + +class Throttled(Exception): + def __init__(self, retry_after: int) -> None: + self.retry_after = retry_after + super().__init__(f"Too many attempts; retry in {retry_after} seconds") + + +def _subjects(user: str | None, ip: str | None) -> list[str]: + if not user and not ip: + raise ValueError("throttle needs a user or an ip; refusing to do nothing") + subjects = [] + if user: + subjects.append(f"user:{user.lower()}") + if ip: + subjects.append(f"ip:{ip}") + return subjects + + +def check( + endpoint: str, + *, + user: str | None = None, + ip: str | None = None, + clock: Clock = timezone.now, +) -> None: + """Raise Throttled if the user or the IP is paused at this endpoint. Never writes.""" + now = clock() + blocked = ThrottleCounter.objects.filter( + endpoint=endpoint, + subject__in=_subjects(user, ip), + blocked_until__gt=now, + ).values_list("blocked_until", flat=True) + latest = max(blocked, default=None) + if latest is not None: + raise Throttled(max(1, math.ceil((latest - now).total_seconds()))) + + +def hit( + endpoint: str, + *, + user: str | None = None, + ip: str | None = None, + clock: Clock = timezone.now, + max_failures: int = THROTTLE_MAX_FAILURES, + pause_seconds: int = THROTTLE_PAUSE_SECONDS, +) -> None: + """Count one failure for the user and the IP; pause a subject that reaches the limit.""" + subjects = _subjects(user, ip) + if connection.in_atomic_block: + raise RuntimeError( + "throttle.hit() must be called after the failed work was rolled back, " + "outside any open transaction" + ) + now = clock() + window = timedelta(seconds=pause_seconds) + with transaction.atomic(): + for subject in subjects: + counter, _ = ThrottleCounter.objects.get_or_create( + endpoint=endpoint, subject=subject, defaults={"window_started": now} + ) + qs = ThrottleCounter.objects.filter(pk=counter.pk) + # Start a fresh window when the old one ran out or the pause is over. + qs.filter(Q(window_started__lte=now - window) | Q(blocked_until__lte=now)).update( + count=0, window_started=now, blocked_until=None + ) + qs.update(count=F("count") + 1) + qs.filter(count__gte=max_failures, blocked_until__isnull=True).update( + blocked_until=now + window + ) + + +def reset(endpoint: str, *, user: str) -> None: + """Clear the user's counter (call after a success). + + IP counters expire on their own and are never reset: a valid sign-in must not let an + attacker with one account clear the IP counter. + """ + ThrottleCounter.objects.filter(endpoint=endpoint, subject__in=_subjects(user, None)).delete() diff --git a/portal/audit/__init__.py b/portal/audit/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/portal/audit/apps.py b/portal/audit/apps.py new file mode 100644 index 0000000..073e51d --- /dev/null +++ b/portal/audit/apps.py @@ -0,0 +1,6 @@ +from django.apps import AppConfig + + +class AuditConfig(AppConfig): + name = "portal.audit" + label = "audit" diff --git a/portal/audit/migrations/0001_initial.py b/portal/audit/migrations/0001_initial.py new file mode 100644 index 0000000..7d94227 --- /dev/null +++ b/portal/audit/migrations/0001_initial.py @@ -0,0 +1,30 @@ +# Generated by Django 5.2.18 on 2026-10-15 13:40 + +import django.utils.timezone +from django.db import migrations, models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ] + + operations = [ + migrations.CreateModel( + name='AuditEntry', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('actor', models.CharField(db_index=True, max_length=256)), + ('action', models.CharField(db_index=True, max_length=128)), + ('target', models.CharField(blank=True, default='', max_length=256)), + ('ip', models.CharField(blank=True, default='', max_length=64)), + ('created_at', models.DateTimeField(db_index=True, default=django.utils.timezone.now)), + ('detail', models.JSONField(blank=True, default=dict)), + ], + options={ + 'ordering': ['-created_at', '-id'], + }, + ), + ] diff --git a/portal/audit/migrations/__init__.py b/portal/audit/migrations/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/portal/audit/models.py b/portal/audit/models.py new file mode 100644 index 0000000..46c3a7b --- /dev/null +++ b/portal/audit/models.py @@ -0,0 +1,16 @@ +from django.db import models +from django.utils import timezone + + +class AuditEntry(models.Model): + """Who did what to whom, when and from where. Written only by `audit.services` (AD-4, AD-19).""" + + actor = models.CharField(max_length=256, db_index=True) # lower-case key + action = models.CharField(max_length=128, db_index=True) + target = models.CharField(max_length=256, blank=True, default="") + ip = models.CharField(max_length=64, blank=True, default="") + created_at = models.DateTimeField(default=timezone.now, db_index=True) # UTC + detail = models.JSONField(default=dict, blank=True) + + class Meta: + ordering = ["-created_at", "-id"] diff --git a/portal/audit/services.py b/portal/audit/services.py new file mode 100644 index 0000000..c4ae673 --- /dev/null +++ b/portal/audit/services.py @@ -0,0 +1,79 @@ +"""Audit service: the only writer of audit entries (AD-4, AD-19).""" + +from __future__ import annotations + +from typing import Any + +from django.db import connection, transaction + +from .models import AuditEntry + +_FORBIDDEN_FRAGMENTS = ("password", "passwd", "pwd", "token", "secret") + + +class AuditOutsideTransaction(RuntimeError): + """`record` was called without an open transaction, so it cannot share the change's fate.""" + + +class AuditInsideTransaction(RuntimeError): + """`record_failure` was called in an open transaction, where a rollback would erase it.""" + + +def _check_detail(value: Any, path: str = "detail") -> None: + if isinstance(value, dict): + for key, inner in value.items(): + lowered = str(key).lower() + if any(fragment in lowered for fragment in _FORBIDDEN_FRAGMENTS): + raise ValueError(f"{path}: key {key!r} may hold a secret and is not allowed") + _check_detail(inner, f"{path}.{key}") + elif isinstance(value, list | tuple): + for item in value: + _check_detail(item, path) + + +def _build(actor: str, action: str, target: str, ip: str, detail: dict | None) -> AuditEntry: + detail = detail or {} + _check_detail(detail) + return AuditEntry( + actor=(actor or "").lower(), + action=action, + target=target or "", + ip=ip or "", + detail=detail, + ) + + +def record( + actor: str, + action: str, + *, + target: str = "", + ip: str = "", + detail: dict | None = None, +) -> AuditEntry: + """Write an entry inside the caller's open transaction; it rolls back with the change.""" + entry = _build(actor, action, target, ip, detail) + if not connection.in_atomic_block: + raise AuditOutsideTransaction("audit.record() must run inside transaction.atomic()") + entry.save() + return entry + + +def record_failure( + actor: str, + action: str, + *, + target: str = "", + ip: str = "", + detail: dict | None = None, +) -> AuditEntry: + """Write a failure entry in its own transaction, after the failed change was rolled back.""" + entry = _build(actor, action, target, ip, detail) + if connection.in_atomic_block: + raise AuditInsideTransaction( + "audit.record_failure() must run after the failed change was rolled back, " + "outside any open transaction" + ) + with transaction.atomic(): + entry.save() + return entry diff --git a/portal/config/settings.py b/portal/config/settings.py index 74800ff..8e552ca 100644 --- a/portal/config/settings.py +++ b/portal/config/settings.py @@ -29,6 +29,8 @@ INSTALLED_APPS = [ "django.contrib.auth", "django.contrib.sessions", "django.contrib.staticfiles", + "portal.audit", + "portal.accounts", ] MIDDLEWARE = [ @@ -37,6 +39,7 @@ MIDDLEWARE = [ "django.middleware.security.SecurityMiddleware", "whitenoise.middleware.WhiteNoiseMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", + "portal.accounts.middleware.AbsoluteSessionLimitMiddleware", "django.middleware.locale.LocaleMiddleware", "django.middleware.common.CommonMiddleware", "django.middleware.csrf.CsrfViewMiddleware", @@ -91,7 +94,10 @@ STORAGES = { } # Sessions in the database; cookies secure, HttpOnly, SameSite=Lax (AD-6). -SESSION_ENGINE = "django.contrib.sessions.backends.db" +SESSION_ENGINE = "portal.accounts.session_store" +SESSION_COOKIE_AGE = 60 * 60 # idle limit: 1 hour, renewed on every request +SESSION_SAVE_EVERY_REQUEST = True +SESSION_ABSOLUTE_SECONDS = 4 * 60 * 60 # absolute limit: 4 hours SESSION_COOKIE_SECURE = not DEBUG SESSION_COOKIE_HTTPONLY = True SESSION_COOKIE_SAMESITE = "Lax" diff --git a/portal/tests/test_audit.py b/portal/tests/test_audit.py new file mode 100644 index 0000000..ba617eb --- /dev/null +++ b/portal/tests/test_audit.py @@ -0,0 +1,59 @@ +import pytest +from django.db import transaction + +from portal.audit import services +from portal.audit.models import AuditEntry + + +@pytest.mark.django_db(transaction=True) +def test_record_rolls_back_with_the_change(): + with pytest.raises(RuntimeError), transaction.atomic(): + services.record("Alice", "profile.update", target="alice", ip="10.0.0.1") + raise RuntimeError("change failed") + assert AuditEntry.objects.count() == 0 + + +@pytest.mark.django_db(transaction=True) +def test_record_persists_on_commit_and_lowercases_actor(): + with transaction.atomic(): + services.record("Alice", "profile.update", target="alice", ip="10.0.0.1", detail={"a": 1}) + entry = AuditEntry.objects.get() + assert (entry.actor, entry.action, entry.ip, entry.detail) == ( + "alice", + "profile.update", + "10.0.0.1", + {"a": 1}, + ) + + +@pytest.mark.django_db(transaction=True) +def test_record_outside_transaction_raises_and_writes_nothing(): + with pytest.raises(services.AuditOutsideTransaction): + services.record("alice", "x") + assert AuditEntry.objects.count() == 0 + + +@pytest.mark.django_db(transaction=True) +@pytest.mark.parametrize("key", ["password", "New_Token", "client_secret", "pwd", "oldPassword"]) +def test_secret_keys_are_refused(key): + with pytest.raises(ValueError), transaction.atomic(): + services.record("alice", "x", detail={"ok": 1, "nested": {key: "v"}}) + with pytest.raises(ValueError): + services.record_failure("alice", "x", detail={key: "v"}) + assert AuditEntry.objects.count() == 0 + + +@pytest.mark.django_db(transaction=True) +def test_record_failure_inside_transaction_raises_and_writes_nothing(): + with pytest.raises(services.AuditInsideTransaction), transaction.atomic(): + services.record_failure("alice", "x.failed") + assert AuditEntry.objects.count() == 0 + + +@pytest.mark.django_db(transaction=True) +def test_record_failure_persists_after_rollback(): + with pytest.raises(RuntimeError), transaction.atomic(): + services.record("alice", "x") + raise RuntimeError("boom") + services.record_failure("alice", "x.failed", detail={"reason": "ad"}) + assert list(AuditEntry.objects.values_list("action", flat=True)) == ["x.failed"] diff --git a/portal/tests/test_sessions.py b/portal/tests/test_sessions.py new file mode 100644 index 0000000..5c847d0 --- /dev/null +++ b/portal/tests/test_sessions.py @@ -0,0 +1,123 @@ +import time +from datetime import timedelta + +import pytest +from django.conf import settings +from django.test import Client, RequestFactory +from django.utils import timezone + +from portal.accounts.middleware import AbsoluteSessionLimitMiddleware +from portal.accounts.models import UserSession +from portal.accounts.session_store import CREATED_AT, SessionStore +from portal.accounts.sessions import SessionUser, current_user, end_sessions, start_session + +pytestmark = pytest.mark.django_db + + +def make_session(sam): + request = RequestFactory().get("/") + request.session = SessionStore() + start_session(request, sam) + request.session.save() + return request + + +def test_username_column_is_lowercase_key_and_sam_keeps_case(): + request = make_session("Alice") + row = UserSession.objects.get(session_key=request.session.session_key) + assert row.username == "alice" + assert current_user(request) == SessionUser(key="alice", sam="Alice") + + +def test_start_session_rotates_key_and_drops_old_data(): + request = RequestFactory().get("/") + request.session = SessionStore() + request.session["stale"] = 1 + request.session.save() + old_key = request.session.session_key + start_session(request, "alice") + request.session.save() + assert request.session.session_key != old_key + assert "stale" not in request.session + assert not UserSession.objects.filter(session_key=old_key).exists() + + +def test_end_sessions_only_target_user(): + for _ in range(3): + make_session("a") + for _ in range(2): + make_session("b") + assert end_sessions("a") == 3 + assert UserSession.objects.filter(username="a").count() == 0 + assert UserSession.objects.filter(username="b").count() == 2 + + +def test_end_sessions_except_session_key_and_unknown_user(): + keep = make_session("a").session.session_key + make_session("a") + assert end_sessions(SessionUser("a", "A"), except_session_key=keep) == 1 + assert list(UserSession.objects.values_list("session_key", flat=True)) == [keep] + assert end_sessions("nobody") == 0 + + +def test_idle_limit_expired_session_is_gone(): + request = make_session("a") + UserSession.objects.update(expire_date=timezone.now() - timedelta(seconds=1)) + assert SessionStore(request.session.session_key).get(CREATED_AT) is None + + +def test_settings_defaults(): + assert settings.SESSION_COOKIE_AGE == 3600 + assert settings.SESSION_ABSOLUTE_SECONDS == 14400 + assert settings.SESSION_SAVE_EVERY_REQUEST is True + + +def _client_with_session(created_at): + store = SessionStore() + request = RequestFactory().get("/") + request.session = store + start_session(request, "alice") + store[CREATED_AT] = created_at + store.save() + client = Client() + client.cookies[settings.SESSION_COOKIE_NAME] = store.session_key + return client, store.session_key + + +def test_absolute_limit_removes_session_and_clears_cookie(): + assert any(m.endswith("AbsoluteSessionLimitMiddleware") for m in settings.MIDDLEWARE) + client, key = _client_with_session(time.time() - settings.SESSION_ABSOLUTE_SECONDS - 5) + response = client.get("/healthz") + assert not UserSession.objects.filter(session_key=key).exists() + assert response.cookies[settings.SESSION_COOKIE_NAME].value == "" + + +def test_session_within_limits_survives_a_request(): + client, key = _client_with_session(time.time() - 60) + client.get("/healthz") + assert UserSession.objects.filter(session_key=key).exists() + + +def test_middleware_callable_directly(): + request = make_session("alice") + AbsoluteSessionLimitMiddleware(lambda r: None)(request) + assert current_user(request) is not None + + +def test_request_renews_expire_date_to_one_hour_ahead(): + client, key = _client_with_session(time.time() - 60) + UserSession.objects.filter(session_key=key).update( + expire_date=timezone.now() + timedelta(minutes=5) + ) + client.get("/healthz") + remaining = UserSession.objects.get(session_key=key).expire_date - timezone.now() + assert timedelta(minutes=59) < remaining <= timedelta(hours=1) + + +def test_request_with_idle_expired_session_has_no_session_and_clears_cookie(): + client, key = _client_with_session(time.time() - 60) + UserSession.objects.filter(session_key=key).update( + expire_date=timezone.now() - timedelta(seconds=1) + ) + response = client.get("/healthz") + assert response.cookies[settings.SESSION_COOKIE_NAME].value == "" diff --git a/portal/tests/test_throttle.py b/portal/tests/test_throttle.py new file mode 100644 index 0000000..5d8955f --- /dev/null +++ b/portal/tests/test_throttle.py @@ -0,0 +1,91 @@ +from datetime import UTC, datetime, timedelta + +import pytest +from django.db import transaction + +from portal.accounts import throttle +from portal.accounts.models import ThrottleCounter + +T0 = datetime(2026, 1, 1, 12, 0, tzinfo=UTC) +EP = "sign-in" + +pytestmark = pytest.mark.django_db(transaction=True) + + +def fail(n, at=T0, **kw): + for _ in range(n): + throttle.hit(EP, clock=lambda: at, **kw) + + +def test_below_limit_passes(): + fail(4, user="alice") + throttle.check(EP, user="alice", clock=lambda: T0) + + +def test_user_is_throttled_but_others_pass(): + fail(5, user="Alice", ip="1.1.1.1") + with pytest.raises(throttle.Throttled) as exc: + throttle.check(EP, user="alice", clock=lambda: T0) + assert exc.value.retry_after == 900 + throttle.check(EP, user="bob", ip="2.2.2.2", clock=lambda: T0) + + +def test_ip_is_throttled_across_users(): + fail(5, user="a", ip="1.1.1.1") + with pytest.raises(throttle.Throttled): + throttle.check(EP, user="other", ip="1.1.1.1", clock=lambda: T0) + throttle.check(EP, user="other", ip="3.3.3.3", clock=lambda: T0) + + +def test_other_endpoint_is_unaffected(): + fail(5, user="alice") + throttle.check("password-change", user="alice", clock=lambda: T0) + + +def test_pause_elapses(): + fail(5, user="alice") + later = T0 + timedelta(seconds=901) + throttle.check(EP, user="alice", clock=lambda: later) + # a new failure after the pause starts a fresh count + throttle.hit(EP, user="alice", clock=lambda: later) + assert ThrottleCounter.objects.get(subject="user:alice").count == 1 + + +def test_stale_window_restarts_count(): + fail(4, user="alice") + later = T0 + timedelta(minutes=16) + throttle.hit(EP, user="alice", clock=lambda: later) + assert ThrottleCounter.objects.get().count == 1 + + +def test_reset_clears_user_but_never_the_ip(): + fail(5, user="alice", ip="1.1.1.1") + throttle.reset(EP, user="alice") + throttle.check(EP, user="alice", clock=lambda: T0) + with pytest.raises(throttle.Throttled): + throttle.check(EP, ip="1.1.1.1", clock=lambda: T0) + + +def test_reset_has_no_ip_parameter(): + with pytest.raises(TypeError): + throttle.reset(EP, user="alice", ip="1.1.1.1") + + +def test_hit_inside_transaction_raises_and_counts_nothing(): + with pytest.raises(RuntimeError), transaction.atomic(): + throttle.hit(EP, user="alice", clock=lambda: T0) + assert ThrottleCounter.objects.count() == 0 + + +def test_no_subject_raises_for_all_functions(): + with pytest.raises(ValueError): + throttle.check(EP) + with pytest.raises(ValueError): + throttle.hit(EP) + with pytest.raises(ValueError): + throttle.reset(EP, user="") + + +def test_check_never_writes(): + throttle.check(EP, user="alice", ip="1.1.1.1", clock=lambda: T0) + assert ThrottleCounter.objects.count() == 0 diff --git a/pyproject.toml b/pyproject.toml index f4cba1f..3daffb7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -34,7 +34,7 @@ env = [ [tool.ruff] line-length = 100 target-version = "py313" -extend-exclude = ["_bmad", "_bmad-output", ".agents", "context-resources"] +extend-exclude = ["**/migrations", "_bmad", "_bmad-output", ".agents", "context-resources"] [tool.ruff.lint] select = ["E", "F", "I", "B", "UP", "S"]