docs(07.2): create phase plan — JTI claim + Redis NBF access-token revocation
3 plans (Wave 0 test scaffolding, Wave 1 jti+NBF check, Wave 2 user_nbf writes), VALIDATION.md, RESEARCH.md with resolved open questions. Checker: 0 blockers, 1 warning resolved (RESEARCH.md open questions marked). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
e9b2d88ba0
commit
7ee87c001b
+3
-1
@@ -34,6 +34,7 @@ progress:
|
|||||||
| 6.2 | Close v1 sharing + cloud-delete + CSV export gaps | ✓ Complete (5/5 plans, UAT passed, security gate passed) |
|
| 6.2 | Close v1 sharing + cloud-delete + CSV export gaps | ✓ Complete (5/5 plans, UAT passed, security gate passed) |
|
||||||
| 7 | Redo and Optimize LLM Integration | ✓ Complete (5/5 plans, UAT 11/11 passed, security gate passed) |
|
| 7 | Redo and Optimize LLM Integration | ✓ Complete (5/5 plans, UAT 11/11 passed, security gate passed) |
|
||||||
| 7.1 | Security: session revocation on privilege change (CR-01..03) | ✓ Complete (2/2 plans, 3 new tests, frontend toasts) |
|
| 7.1 | Security: session revocation on privilege change (CR-01..03) | ✓ Complete (2/2 plans, 3 new tests, frontend toasts) |
|
||||||
|
| 7.2 | Security: JTI claim + Redis access-token revocation | ◆ Planned (3 plans, 3 waves) — ready to execute |
|
||||||
|
|
||||||
## Current Position
|
## Current Position
|
||||||
|
|
||||||
@@ -205,6 +206,7 @@ _Updated at each phase transition._
|
|||||||
| Last session | 2026-06-03 — Phase 7 planned: 5 plans (5 waves); system_settings DB + HKDF (Plan 01), ProviderConfig + GenericOpenAIProvider + singleton fix (Plan 02), Anthropic output_config + classifier wiring (Plan 03), Celery retry harness + re-queue endpoint (Plan 04), admin AI panel + DocumentCard badge (Plan 05); verification passed (4 blockers fixed in revision round, 0 remaining) |
|
| Last session | 2026-06-03 — Phase 7 planned: 5 plans (5 waves); system_settings DB + HKDF (Plan 01), ProviderConfig + GenericOpenAIProvider + singleton fix (Plan 02), Anthropic output_config + classifier wiring (Plan 03), Celery retry harness + re-queue endpoint (Plan 04), admin AI panel + DocumentCard badge (Plan 05); verification passed (4 blockers fixed in revision round, 0 remaining) |
|
||||||
| Last session | 2026-06-05 — Phase 7 complete: all 5 plans executed; UAT 11/11 passed; security gate passed (bandit zero HIGH, npm audit zero high/critical); _doc_to_dict status field fix + regression test committed; v1.0 milestone DONE |
|
| Last session | 2026-06-05 — Phase 7 complete: all 5 plans executed; UAT 11/11 passed; security gate passed (bandit zero HIGH, npm audit zero high/critical); _doc_to_dict status field fix + regression test committed; v1.0 milestone DONE |
|
||||||
| Last session | 2026-06-05 — Phase 7.1 complete: CR-01/CR-02/CR-03 implemented; skip_token_hash added to revoke_all_refresh_tokens; change_password, enable_totp, disable_totp now revoke other sessions and return sessions_revoked; 3 new tests passing; frontend toasts in SettingsAccountTab + TotpEnrollment; 373 passed 0 failed; v0.1.1 |
|
| Last session | 2026-06-05 — Phase 7.1 complete: CR-01/CR-02/CR-03 implemented; skip_token_hash added to revoke_all_refresh_tokens; change_password, enable_totp, disable_totp now revoke other sessions and return sessions_revoked; 3 new tests passing; frontend toasts in SettingsAccountTab + TotpEnrollment; 373 passed 0 failed; v0.1.1 |
|
||||||
| Next action | All phases and sub-phases complete. Ready for next milestone. |
|
| Last session | 2026-06-05 — Phase 7.2 planned: 3 plans (Wave 0 test scaffolding, Wave 1 jti+NBF check, Wave 2 user_nbf writes in 4 handlers); verification passed (0 blockers, 1 warning fixed); ready to execute |
|
||||||
|
| Next action | Execute Phase 7.2: /gsd:execute-phase 7.2 |
|
||||||
| Pending decisions | None |
|
| Pending decisions | None |
|
||||||
| Resume file | None |
|
| Resume file | None |
|
||||||
|
|||||||
+337
@@ -0,0 +1,337 @@
|
|||||||
|
---
|
||||||
|
phase: 07.2-security-jti-claim-redis-access-token-revocation-inserted
|
||||||
|
plan: 01
|
||||||
|
type: execute
|
||||||
|
wave: 0
|
||||||
|
depends_on: []
|
||||||
|
files_modified:
|
||||||
|
- backend/tests/test_auth_deps.py
|
||||||
|
- backend/tests/test_task2_auth_service.py
|
||||||
|
- backend/tests/test_auth_api.py
|
||||||
|
- backend/tests/test_admin_api.py
|
||||||
|
autonomous: true
|
||||||
|
requirements:
|
||||||
|
- CONCERNS:JTI-CLAIM
|
||||||
|
- CONCERNS:JTI-REVOKE-REDIS
|
||||||
|
tags:
|
||||||
|
- security
|
||||||
|
- jwt
|
||||||
|
- testing
|
||||||
|
- nyquist
|
||||||
|
|
||||||
|
must_haves:
|
||||||
|
truths:
|
||||||
|
- "Every Phase 7.2 behavior has a failing test scaffold before any implementation runs"
|
||||||
|
- "test_auth_deps.py test app mounts app.state.redis = FakeRedis() so Wave 1 NBF check does not crash existing 34+ auth-dep tests"
|
||||||
|
- "Wave 0 stubs are skipped/xfailed (strict=False); zero existing tests regress"
|
||||||
|
artifacts:
|
||||||
|
- path: "backend/tests/test_auth_deps.py"
|
||||||
|
provides: "FakeRedis attached to make_test_app(); NBF-check xfail stubs (iat<nbf→401, iat>nbf→pass, fail-open)"
|
||||||
|
contains: "FakeRedis"
|
||||||
|
- path: "backend/tests/test_task2_auth_service.py"
|
||||||
|
provides: "test_create_access_token_includes_jti xfail stub"
|
||||||
|
contains: "jti"
|
||||||
|
- path: "backend/tests/test_auth_api.py"
|
||||||
|
provides: "NBF-write xfail stubs for change_password, enable_totp, disable_totp"
|
||||||
|
contains: "user_nbf"
|
||||||
|
- path: "backend/tests/test_admin_api.py"
|
||||||
|
provides: "NBF-write xfail stub for admin deactivation handler"
|
||||||
|
contains: "user_nbf"
|
||||||
|
key_links:
|
||||||
|
- from: "backend/tests/test_auth_deps.py make_test_app()"
|
||||||
|
to: "request.app.state.redis"
|
||||||
|
via: "test_app.state.redis = FakeRedis() set before client yield"
|
||||||
|
pattern: "app\\.state\\.redis\\s*=\\s*FakeRedis"
|
||||||
|
- from: "Wave 0 stubs"
|
||||||
|
to: "Wave 1/2 implementation"
|
||||||
|
via: "pytest.xfail(strict=False) markers promote to xpass when code lands"
|
||||||
|
pattern: "pytest\\.xfail|xfail.*strict=False"
|
||||||
|
---
|
||||||
|
|
||||||
|
<objective>
|
||||||
|
Pre-implementation scaffolding for Phase 7.2 (Nyquist Wave 0).
|
||||||
|
|
||||||
|
Purpose: Two things must be in place before any code in `services/auth.py`,
|
||||||
|
`deps/auth.py`, `api/auth.py`, or `api/admin.py` is touched:
|
||||||
|
|
||||||
|
1. A `FakeRedis` instance attached to `app.state.redis` on the minimal test
|
||||||
|
app in `backend/tests/test_auth_deps.py`. Without this, Wave 1's NBF
|
||||||
|
check in `get_current_user` will raise `AttributeError: 'State' object
|
||||||
|
has no attribute 'redis'` on every existing auth-dep test (Pitfall 4
|
||||||
|
from RESEARCH.md).
|
||||||
|
|
||||||
|
2. Failing/xfailed test stubs that pin every Phase 7.2 behavior listed in
|
||||||
|
`07.2-VALIDATION.md` (JTI presence, NBF write on each security event,
|
||||||
|
NBF check accept/reject, fail-open). Wave 1 and Wave 2 implementation
|
||||||
|
then "flips" these from xfail to passing — verifiable test signal.
|
||||||
|
|
||||||
|
Output: 4 modified test files. Test suite still green (zero new failures);
|
||||||
|
new stubs report as XFAIL with `strict=False`.
|
||||||
|
</objective>
|
||||||
|
|
||||||
|
<execution_context>
|
||||||
|
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
|
||||||
|
@$HOME/.claude/get-shit-done/templates/summary.md
|
||||||
|
</execution_context>
|
||||||
|
|
||||||
|
<context>
|
||||||
|
@.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-CONTEXT.md
|
||||||
|
@.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-RESEARCH.md
|
||||||
|
@.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-VALIDATION.md
|
||||||
|
@backend/tests/test_auth_deps.py
|
||||||
|
@backend/tests/test_auth_api.py
|
||||||
|
@backend/tests/test_task2_auth_service.py
|
||||||
|
@backend/tests/test_admin_api.py
|
||||||
|
|
||||||
|
<interfaces>
|
||||||
|
<!-- Reusable FakeRedis class already exists. DO NOT redefine. -->
|
||||||
|
|
||||||
|
Existing FakeRedis class — backend/tests/test_auth_api.py lines 47–86:
|
||||||
|
- class FakeRedis with async get/set/incr/expire/close
|
||||||
|
- Already used by authed_client fixture (line 102): fake_redis = FakeRedis(); app.state.redis = fake_redis
|
||||||
|
- Supports `await redis.get(key)` returning the stored value (or None), and `await redis.set(key, value, ex=seconds)`
|
||||||
|
- For Phase 7.2 NBF check, the .get() return type must be bytes-like (RESEARCH.md L210: `int(nbf_bytes.decode())`).
|
||||||
|
The existing FakeRedis stores arbitrary value types; tests setting `user_nbf` must store bytes (e.g., `b"1700000000"`) for the .decode() call to work.
|
||||||
|
|
||||||
|
Existing make_test_app — backend/tests/test_auth_deps.py lines 22–37:
|
||||||
|
- Builds a minimal FastAPI with /test/me and /test/admin endpoints
|
||||||
|
- Does NOT currently set app.state.redis
|
||||||
|
- Used by auth_client fixture (line 40–51)
|
||||||
|
|
||||||
|
Existing test_task2_auth_service.py — backend/tests/test_task2_auth_service.py:
|
||||||
|
- test_create_access_token_jwt_format (line 28): existing positive test for create_access_token
|
||||||
|
- test_decode_access_token_valid (line 35): decodes and asserts payload contents — TEMPLATE for jti assertion
|
||||||
|
|
||||||
|
Existing test_auth_api.py change-password test pattern:
|
||||||
|
- Uses `authed_client` fixture (line 89) which sets `app.state.redis = FakeRedis()`
|
||||||
|
- change_password handler at backend/api/auth.py:455 — already uses `request.cookies.get("refresh_token")` and writes audit log
|
||||||
|
|
||||||
|
Existing test_admin_api.py deactivation tests:
|
||||||
|
- test_deactivate_user at line 191 — PATCH /api/admin/users/{id}/status with is_active=False
|
||||||
|
- admin_client fixture (line 72) does NOT currently set app.state.redis — Wave 2 (Plan 03) write site uses request.app.state.redis, so admin tests need FakeRedis too (Task 4 below)
|
||||||
|
|
||||||
|
Phase 7.2 behaviors to stub (from 07.2-VALIDATION.md Per-Task Verification Map):
|
||||||
|
- jti-claim: create_access_token payload contains "jti" key with UUID-format string value
|
||||||
|
- test-fakeredis: get_current_user does not raise AttributeError when app.state.redis is FakeRedis
|
||||||
|
- nbf-write-change-password: POST /api/auth/change-password sets user_nbf:{user_id} in Redis
|
||||||
|
- nbf-write-enable-totp: POST /api/auth/totp/enable sets user_nbf:{user_id} in Redis
|
||||||
|
- nbf-write-disable-totp: DELETE /api/auth/totp sets user_nbf:{user_id} in Redis
|
||||||
|
- nbf-write-deactivation: PATCH /api/admin/users/{id}/status (is_active=False) sets user_nbf:{user_id} in Redis
|
||||||
|
- nbf-check-reject: token with iat < nbf returns 401 "Session invalidated"
|
||||||
|
- nbf-check-allow: token with iat > nbf returns 200
|
||||||
|
- nbf-fail-open: redis.get raises Exception → request still succeeds (200)
|
||||||
|
|
||||||
|
Wave 0 stub convention (from STATE.md key decisions: "Wave 0 stubs: single-line body only"):
|
||||||
|
- Body is ONLY `pytest.xfail("not implemented yet — Phase 7.2 Wave 1/2", strict=False)` or test marker `@pytest.mark.xfail(strict=False, reason="...")` with placeholder assertion
|
||||||
|
- No real assertion logic in Wave 0 — that comes in Wave 1/2 when the same stubs are promoted by replacing the xfail with real assertions
|
||||||
|
</interfaces>
|
||||||
|
</context>
|
||||||
|
|
||||||
|
<tasks>
|
||||||
|
|
||||||
|
<task type="auto" tdd="true">
|
||||||
|
<name>Task 1: Mount FakeRedis on test_auth_deps test app and add NBF-check stubs</name>
|
||||||
|
<files>backend/tests/test_auth_deps.py</files>
|
||||||
|
<read_first>
|
||||||
|
- backend/tests/test_auth_deps.py (current make_test_app + auth_client fixture — lines 22–51)
|
||||||
|
- backend/tests/test_auth_api.py lines 47–86 (FakeRedis class definition — import this, do not redefine)
|
||||||
|
- backend/tests/test_auth_api.py lines 89–123 (authed_client fixture — pattern for app.state.redis assignment)
|
||||||
|
- .planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-RESEARCH.md "Pitfall 4" section (lines 296–301)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- Test 1 (positive guard): existing test_get_current_user_returns_user still passes after FakeRedis is attached (no regression).
|
||||||
|
- Test 2 (xfail): test_get_current_user_rejects_token_when_iat_before_user_nbf — pre-populates fake_redis["user_nbf:{uid}"] = b"<future_ts>", calls /test/me with a token whose iat < future_ts, expects 401 with detail containing "Session invalidated".
|
||||||
|
- Test 3 (xfail): test_get_current_user_allows_token_when_iat_after_user_nbf — pre-populates fake_redis["user_nbf:{uid}"] = b"<past_ts>", calls /test/me with a token whose iat > past_ts, expects 200.
|
||||||
|
- Test 4 (xfail): test_get_current_user_failopen_on_redis_error — sets app.state.redis to an object whose get() raises Exception, calls /test/me, expects 200 (fail-open per D-04).
|
||||||
|
- All three new tests marked `@pytest.mark.xfail(strict=False, reason="Phase 7.2 Wave 1 — NBF check not yet implemented")`.
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Import FakeRedis from tests.test_auth_api at top of file (use relative-style import: `from tests.test_auth_api import FakeRedis` — verify by reading existing `from ...` statements in other Phase tests for the correct module path; conftest uses `from main import app` so absolute `from tests.test_auth_api` is the project convention).
|
||||||
|
|
||||||
|
Modify `make_test_app()` (line 22): before returning test_app, set `test_app.state.redis = FakeRedis()`. Keep the existing route definitions unchanged.
|
||||||
|
|
||||||
|
Modify `auth_client` fixture (line 40): no signature change; ensure each invocation creates a fresh FakeRedis (because make_test_app() instantiates it). Add a fixture-scope comment noting that Phase 7.2 NBF check reads `request.app.state.redis`.
|
||||||
|
|
||||||
|
Append three new test functions after the existing last test:
|
||||||
|
- `test_get_current_user_rejects_token_when_iat_before_user_nbf(auth_client, db_session)`
|
||||||
|
- `test_get_current_user_allows_token_when_iat_after_user_nbf(auth_client, db_session)`
|
||||||
|
- `test_get_current_user_failopen_on_redis_error(auth_client, db_session)`
|
||||||
|
|
||||||
|
Each new test:
|
||||||
|
- decorated with `@pytest.mark.xfail(strict=False, reason="Phase 7.2 Wave 1 — NBF check not yet implemented in get_current_user")`
|
||||||
|
- decorated with `@pytest.mark.asyncio`
|
||||||
|
- Uses _create_user helper (already present, line 54) to insert a user
|
||||||
|
- Uses services.auth.create_access_token to mint the token
|
||||||
|
- Body: 1–5 lines of placeholder assertions (e.g., `assert False, "stub"` after the prep), so when Wave 1 implementation lands and the assertion logic is filled in, the xfail flips to xpass. Keep Wave 0 body minimal per STATE.md convention.
|
||||||
|
|
||||||
|
For Task 4 (failopen), define a small inline `class _BrokenRedis` whose `async def get(self, key)` raises `RuntimeError("simulated redis down")`. Set `auth_client._transport.app.state.redis = _BrokenRedis()` inside the test (or override via a fixture-local app instance) — this is acceptable in Wave 0 because Wave 1 implementation will read the assignment back out.
|
||||||
|
|
||||||
|
Do NOT modify any existing test in the file. Do NOT add real assertion logic for the NBF check beyond placeholder stubs.
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest tests/test_auth_deps.py -v</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `cd backend && pytest tests/test_auth_deps.py -v` exits 0
|
||||||
|
- `grep -c "FakeRedis" backend/tests/test_auth_deps.py` returns >= 2 (import + assignment)
|
||||||
|
- `grep -c "test_app.state.redis" backend/tests/test_auth_deps.py` returns >= 1
|
||||||
|
- `grep -c "test_get_current_user_rejects_token_when_iat_before_user_nbf\|test_get_current_user_allows_token_when_iat_after_user_nbf\|test_get_current_user_failopen_on_redis_error" backend/tests/test_auth_deps.py` returns 3
|
||||||
|
- `grep -E -c "pytest\\.mark\\.xfail|pytest\\.xfail" backend/tests/test_auth_deps.py` returns >= 3
|
||||||
|
- Test output lists at least 3 XFAIL results (or XPASS for any whose body already trivially passes — both acceptable in strict=False)
|
||||||
|
- All previously-existing tests in test_auth_deps.py still PASS (no regressions in the 34+ baseline)
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>FakeRedis mounted on test app; three NBF-check stubs added with xfail(strict=False); zero existing test regressions.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
<task type="auto" tdd="true">
|
||||||
|
<name>Task 2: Add JTI presence stub in test_task2_auth_service.py</name>
|
||||||
|
<files>backend/tests/test_task2_auth_service.py</files>
|
||||||
|
<read_first>
|
||||||
|
- backend/tests/test_task2_auth_service.py lines 28–58 (existing create/decode_access_token tests — TEMPLATE)
|
||||||
|
- backend/services/auth.py lines 86–117 (current create_access_token + decode_access_token implementations — note absence of jti)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- Test (xfail): test_create_access_token_includes_jti_claim — calls `create_access_token("test-uid", "user")`, decodes via `decode_access_token`, asserts the returned payload dict has key `"jti"` whose value is a non-empty string parseable as a UUID (uuid.UUID(payload["jti"]) does not raise).
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Append a single new test function after the existing last create/decode_access_token test in the file:
|
||||||
|
|
||||||
|
`def test_create_access_token_includes_jti_claim():` decorated with `@pytest.mark.xfail(strict=False, reason="Phase 7.2 Wave 1 — jti claim not yet added to create_access_token")`.
|
||||||
|
|
||||||
|
Body: import create_access_token and decode_access_token from services.auth (follow the import style of the surrounding tests — they use function-local imports per test). Mint a token. Decode it. Assert `"jti" in payload` and `uuid.UUID(payload["jti"])` succeeds. Import `uuid` at function scope (matches surrounding style) if not already module-level imported.
|
||||||
|
|
||||||
|
Do NOT modify create_access_token in services/auth.py — that is Wave 1 (Plan 02). Do NOT modify any existing test.
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest tests/test_task2_auth_service.py -v -k jti</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `cd backend && pytest tests/test_task2_auth_service.py -v -k jti` exits 0
|
||||||
|
- `grep -c "test_create_access_token_includes_jti_claim" backend/tests/test_task2_auth_service.py` returns 1
|
||||||
|
- `grep -E -c "xfail.*strict=False" backend/tests/test_task2_auth_service.py` returns >= 1 (new stub)
|
||||||
|
- The new test reports as XFAIL (or XPASS if the placeholder body trivially passes; both acceptable in strict=False)
|
||||||
|
- All existing tests in test_task2_auth_service.py still PASS (zero regressions)
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>JTI presence test stub added with xfail(strict=False); existing tests unaffected.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
<task type="auto" tdd="true">
|
||||||
|
<name>Task 3: Add NBF-write stubs for change_password / enable_totp / disable_totp in test_auth_api.py</name>
|
||||||
|
<files>backend/tests/test_auth_api.py</files>
|
||||||
|
<read_first>
|
||||||
|
- backend/tests/test_auth_api.py lines 45–123 (FakeRedis class + authed_client fixture — already mounts app.state.redis)
|
||||||
|
- backend/api/auth.py lines 452–509 (change_password handler — Wave 2 will add the user_nbf write here)
|
||||||
|
- backend/api/auth.py lines 548–601 (enable_totp handler — Wave 2 write site)
|
||||||
|
- backend/api/auth.py lines 606–641 (disable_totp handler — Wave 2 write site)
|
||||||
|
- Existing test that exercises change_password in test_auth_api.py (grep for `change-password` and copy its login+TOTP setup pattern)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- Test (xfail) 1: test_change_password_writes_user_nbf_to_redis — completes register + login (using existing helpers), then POST /api/auth/change-password with valid current+new passwords. After response, asserts `await app.state.redis.get(f"user_nbf:{user_id}")` returns a non-None bytes value parseable as `int(...)`.
|
||||||
|
- Test (xfail) 2: test_enable_totp_writes_user_nbf_to_redis — registers, logs in, calls /totp/setup, supplies a generated TOTP code via /totp/enable, asserts `user_nbf:{user_id}` is set in Redis.
|
||||||
|
- Test (xfail) 3: test_disable_totp_writes_user_nbf_to_redis — same setup as Test 2 plus enable, then DELETE /api/auth/totp, asserts `user_nbf:{user_id}` is set in Redis.
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Append three new async test functions (after the existing change_password / TOTP tests) named exactly as listed in behavior above. All three:
|
||||||
|
- decorated `@pytest.mark.xfail(strict=False, reason="Phase 7.2 Wave 2 — user_nbf write not yet added to handler")`
|
||||||
|
- decorated `@pytest.mark.asyncio`
|
||||||
|
- Use the `authed_client` fixture (already mounts FakeRedis at app.state.redis)
|
||||||
|
- Reuse the existing `_register` / `_login` helpers (lines 31–43) and the in-file TOTP test helpers (grep for existing TOTP-enable test pattern in the file; copy fixture/imports)
|
||||||
|
- Body keeps Wave 0 minimal: prep + a single `assert False, "stub — Wave 2 will fill in"` or call `pytest.xfail("Phase 7.2 Wave 2 stub")` inside the body. The xfail strict=False allows both.
|
||||||
|
|
||||||
|
Implementation hint (kept in test comments for Wave 2): after the API call returns success, fetch the user_id from the login response payload, then `nbf_bytes = await authed_client._transport.app.state.redis.get(f"user_nbf:{user_id}")` and assert `nbf_bytes is not None` plus `int(nbf_bytes.decode() if isinstance(nbf_bytes, (bytes, bytearray)) else nbf_bytes) > 0`.
|
||||||
|
|
||||||
|
Do NOT modify any handler in backend/api/auth.py — that is Wave 2 (Plan 03). Do NOT modify the FakeRedis class.
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest tests/test_auth_api.py -v -k "nbf or user_nbf"</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `cd backend && pytest tests/test_auth_api.py -v -k "nbf or user_nbf"` exits 0
|
||||||
|
- `grep -c "test_change_password_writes_user_nbf_to_redis\|test_enable_totp_writes_user_nbf_to_redis\|test_disable_totp_writes_user_nbf_to_redis" backend/tests/test_auth_api.py` returns 3
|
||||||
|
- `grep -E -c "user_nbf" backend/tests/test_auth_api.py` returns >= 3 (one per new test)
|
||||||
|
- `grep -E -c "xfail.*strict=False" backend/tests/test_auth_api.py` returns >= 3 (new stubs)
|
||||||
|
- All three new tests report as XFAIL (or XPASS — both acceptable in strict=False)
|
||||||
|
- All existing tests in test_auth_api.py still PASS (zero regressions to register/login/totp/change-password baseline)
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>Three NBF-write stubs added covering change_password, enable_totp, disable_totp; existing auth API tests unaffected.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
<task type="auto" tdd="true">
|
||||||
|
<name>Task 4: Add NBF-write stub + FakeRedis for admin deactivation in test_admin_api.py</name>
|
||||||
|
<files>backend/tests/test_admin_api.py</files>
|
||||||
|
<read_first>
|
||||||
|
- backend/tests/test_admin_api.py lines 71–84 (admin_client fixture — does NOT currently set app.state.redis)
|
||||||
|
- backend/tests/test_admin_api.py lines 191–220 (test_deactivate_user — TEMPLATE for the new test)
|
||||||
|
- backend/tests/test_auth_api.py lines 47–86 (FakeRedis class — import target)
|
||||||
|
- backend/api/admin.py lines 340–380 (deactivation handler — Wave 2 write site)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- admin_client fixture mounts `app.state.redis = FakeRedis()` so the deactivation handler (post-Wave 2) can call `request.app.state.redis.set(...)` without AttributeError.
|
||||||
|
- Test (xfail): test_deactivate_user_writes_user_nbf_to_redis — creates a regular user via make_regular_user, sends PATCH /api/admin/users/{id}/status with `{"is_active": false}`, asserts `await app.state.redis.get(f"user_nbf:{user_id}")` returns a non-None bytes value.
|
||||||
|
- Test (negative guard, can be passing immediately): test_activate_user_does_NOT_write_user_nbf — sends PATCH with `{"is_active": true}` on an already-deactivated user, asserts Redis key is NOT set. Marked `@pytest.mark.xfail(strict=False)` because the handler does not yet write the key in either branch — Wave 2 must preserve this invariant. (Mirrors RESEARCH.md Anti-pattern: "Do not write user_nbf for successful activation".)
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Import FakeRedis at top of file: `from tests.test_auth_api import FakeRedis` (matches project convention for cross-test imports).
|
||||||
|
|
||||||
|
Modify `admin_client` fixture (line 72): before `async with AsyncClient(...)`, add `app.state.redis = FakeRedis()`. In the teardown after fixture yield (after `app.dependency_overrides.clear()`), add `app.state.redis = None` (mirrors authed_client teardown at test_auth_api.py:123).
|
||||||
|
|
||||||
|
Append two new test functions at end of file (after the last existing test):
|
||||||
|
- `test_deactivate_user_writes_user_nbf_to_redis(admin_client)` — async, marked `@pytest.mark.xfail(strict=False, reason="Phase 7.2 Wave 2 — user_nbf write not yet added to admin deactivation handler")`. Body: unpack `client, _admin, session = admin_client`; create a regular user via `make_regular_user(session)`; PATCH /api/admin/users/{user.id}/status with `{"is_active": false}`; assert response 200; access `client._transport.app.state.redis` (the FakeRedis instance) and `await` its `.get(f"user_nbf:{user.id}")`; assert the value is not None. Placeholder body acceptable in Wave 0 (single `assert False, "stub"` line is fine).
|
||||||
|
- `test_activate_user_does_not_write_user_nbf(admin_client)` — same xfail decorator; deactivate first, then PATCH with `{"is_active": true}`, assert `await app.state.redis.get(f"user_nbf:{user.id}")` is None at the end. Placeholder body acceptable.
|
||||||
|
|
||||||
|
Verify no existing admin test regresses: pre-existing admin tests do not call `request.app.state.redis` so the mount is additive and safe.
|
||||||
|
|
||||||
|
Do NOT modify any handler in backend/api/admin.py — that is Wave 2 (Plan 03).
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest tests/test_admin_api.py -v</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `cd backend && pytest tests/test_admin_api.py -v` exits 0
|
||||||
|
- `grep -c "from tests.test_auth_api import FakeRedis\|from tests\\.test_auth_api import FakeRedis" backend/tests/test_admin_api.py` returns >= 1
|
||||||
|
- `grep -c "app.state.redis = FakeRedis" backend/tests/test_admin_api.py` returns >= 1
|
||||||
|
- `grep -c "test_deactivate_user_writes_user_nbf_to_redis\|test_activate_user_does_not_write_user_nbf" backend/tests/test_admin_api.py` returns 2
|
||||||
|
- `grep -E -c "xfail.*strict=False" backend/tests/test_admin_api.py` returns >= 2
|
||||||
|
- All previously-existing admin tests (including test_deactivate_user, test_reactivate_user) still PASS
|
||||||
|
- Full pytest -v reports zero NEW failures vs baseline (XFAILs and XPASSes acceptable)
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>FakeRedis attached to admin_client fixture; deactivation NBF-write stubs (positive + negative-guard) added; existing admin tests unaffected.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
</tasks>
|
||||||
|
|
||||||
|
<threat_model>
|
||||||
|
## Trust Boundaries
|
||||||
|
|
||||||
|
| Boundary | Description |
|
||||||
|
|----------|-------------|
|
||||||
|
| test → app.state | Test fixtures inject fake infrastructure (FakeRedis) onto the live FastAPI app; tests must not leak shared state between test runs |
|
||||||
|
|
||||||
|
## STRIDE Threat Register
|
||||||
|
|
||||||
|
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
||||||
|
|-----------|----------|-----------|-------------|-----------------|
|
||||||
|
| T-7.2-W0-01 | Tampering | test_admin_api.py admin_client fixture | mitigate | Reset `app.state.redis = None` in teardown after yield (mirrors authed_client convention at test_auth_api.py:123) — prevents stale FakeRedis from one test file bleeding into another |
|
||||||
|
| T-7.2-W0-02 | Repudiation | xfail stubs with `strict=False` | accept | Stubs are deliberately permissive in Wave 0 — they document expected behavior and act as failing gates when Wave 1/2 lands. The strict=False convention is established across this project (STATE.md). |
|
||||||
|
| T-7.2-W0-SC | Tampering | npm/pip/cargo installs | n/a | No packages installed in this plan — Phase 7.2 adds zero dependencies (RESEARCH.md "Package Legitimacy Audit" section is intentionally empty) |
|
||||||
|
</threat_model>
|
||||||
|
|
||||||
|
<verification>
|
||||||
|
- `cd backend && pytest tests/test_auth_deps.py tests/test_auth_api.py tests/test_task2_auth_service.py tests/test_admin_api.py -v` — zero new failures vs current baseline (373 passed in Phase 7.1)
|
||||||
|
- `grep -c "FakeRedis" backend/tests/test_auth_deps.py` returns >= 2 (import + assignment)
|
||||||
|
- `grep -c "FakeRedis" backend/tests/test_admin_api.py` returns >= 2 (import + assignment)
|
||||||
|
- All four new stub test sets report as XFAIL (or XPASS — both acceptable under strict=False)
|
||||||
|
</verification>
|
||||||
|
|
||||||
|
<success_criteria>
|
||||||
|
- Wave 0 stubs cover every behavior in 07.2-VALIDATION.md Per-Task Verification Map (jti-claim, test-fakeredis, nbf-write × 4, nbf-check × 2, nbf-fail-open)
|
||||||
|
- FakeRedis is reachable via `request.app.state.redis` in both test_auth_deps and test_admin_api fixtures
|
||||||
|
- Wave 1 (Plan 02) and Wave 2 (Plan 03) can promote each stub to a passing assertion by editing only the test body — no new fixtures, no new files
|
||||||
|
- Zero regressions: existing 373-test baseline from Phase 7.1 passes unchanged
|
||||||
|
</success_criteria>
|
||||||
|
|
||||||
|
<output>
|
||||||
|
Create `.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-01-SUMMARY.md` when done.
|
||||||
|
|
||||||
|
Required fields in SUMMARY: artifacts (4 test files), patterns_established (FakeRedis on test_auth_deps app.state, FakeRedis on admin_client fixture), patterns_to_avoid (do NOT redefine FakeRedis — import from tests.test_auth_api), provides (Wave 0 scaffolds for jti, NBF-check, NBF-write × 4, fail-open).
|
||||||
|
</output>
|
||||||
+292
@@ -0,0 +1,292 @@
|
|||||||
|
---
|
||||||
|
phase: 07.2-security-jti-claim-redis-access-token-revocation-inserted
|
||||||
|
plan: 02
|
||||||
|
type: execute
|
||||||
|
wave: 1
|
||||||
|
depends_on:
|
||||||
|
- 07.2-01
|
||||||
|
files_modified:
|
||||||
|
- backend/services/auth.py
|
||||||
|
- backend/deps/auth.py
|
||||||
|
autonomous: true
|
||||||
|
requirements:
|
||||||
|
- CONCERNS:JTI-CLAIM
|
||||||
|
- CONCERNS:JTI-REVOKE-REDIS
|
||||||
|
tags:
|
||||||
|
- security
|
||||||
|
- jwt
|
||||||
|
- redis
|
||||||
|
- access-control
|
||||||
|
|
||||||
|
must_haves:
|
||||||
|
truths:
|
||||||
|
- "Every access token issued by create_access_token contains a unique jti UUID claim (D-01)"
|
||||||
|
- "get_current_user rejects any access token whose iat predates user_nbf:{user_id} in Redis with HTTP 401 (D-02)"
|
||||||
|
- "get_current_user accepts tokens whose iat post-dates user_nbf (intended recovery path after refresh)"
|
||||||
|
- "A Redis outage during the NBF check does not block requests — fail-open with warning log (D-04)"
|
||||||
|
- "The HTTPException raised by the NBF check is NOT swallowed by the fail-open broad-catch (Pitfall 1)"
|
||||||
|
artifacts:
|
||||||
|
- path: "backend/services/auth.py"
|
||||||
|
provides: "create_access_token with jti=str(uuid.uuid4()) in payload"
|
||||||
|
contains: "jti"
|
||||||
|
- path: "backend/deps/auth.py"
|
||||||
|
provides: "user_nbf Redis check in get_current_user with fail-open + HTTPException re-raise guard"
|
||||||
|
contains: "user_nbf"
|
||||||
|
key_links:
|
||||||
|
- from: "backend/services/auth.py create_access_token"
|
||||||
|
to: "PyJWT encode payload"
|
||||||
|
via: "payload dict with new 'jti' key"
|
||||||
|
pattern: "\"jti\":\\s*str\\(uuid\\.uuid4"
|
||||||
|
- from: "backend/deps/auth.py get_current_user"
|
||||||
|
to: "request.app.state.redis"
|
||||||
|
via: "await redis.get(f\"user_nbf:{payload['sub']}\") + int comparison vs payload['iat']"
|
||||||
|
pattern: "user_nbf:|app\\.state\\.redis"
|
||||||
|
- from: "Wave 1 implementation"
|
||||||
|
to: "Wave 0 xfail stubs (test_auth_deps NBF check, test_task2_auth_service JTI)"
|
||||||
|
via: "stubs flip to XPASS or are promoted by Wave 2/end-of-phase cleanup"
|
||||||
|
pattern: "XPASS|xfail.*strict=False"
|
||||||
|
---
|
||||||
|
|
||||||
|
<objective>
|
||||||
|
Add the jti claim to access token issuance and the user_nbf revocation check
|
||||||
|
to the central auth dependency.
|
||||||
|
|
||||||
|
Purpose: Closes the 15-minute window between Phase 7.1's refresh-token
|
||||||
|
revocation and natural access-token expiry. Implements D-01 (jti claim) and
|
||||||
|
D-02/D-03/D-04 (user-level NBF check with fail-open) verbatim.
|
||||||
|
|
||||||
|
This plan is single-concern (token issuance + validation) and self-contained:
|
||||||
|
no API handler or admin code is touched. Wave 2 (Plan 03) wires the actual
|
||||||
|
`user_nbf` writes into the four security-event handlers.
|
||||||
|
|
||||||
|
Output: Two surgical edits — one to services/auth.py (add jti), one to
|
||||||
|
deps/auth.py (add NBF check block). Wave 0's XFAIL stubs for jti-claim and
|
||||||
|
NBF-check flip to XPASS. Existing 373-test baseline stays green.
|
||||||
|
</objective>
|
||||||
|
|
||||||
|
<execution_context>
|
||||||
|
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
|
||||||
|
@$HOME/.claude/get-shit-done/templates/summary.md
|
||||||
|
</execution_context>
|
||||||
|
|
||||||
|
<context>
|
||||||
|
@.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-CONTEXT.md
|
||||||
|
@.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-RESEARCH.md
|
||||||
|
@backend/services/auth.py
|
||||||
|
@backend/deps/auth.py
|
||||||
|
@.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-01-PLAN.md
|
||||||
|
|
||||||
|
<interfaces>
|
||||||
|
<!-- Current create_access_token signature (services/auth.py:86–99) -->
|
||||||
|
|
||||||
|
def create_access_token(user_id: str, role: str) -> str:
|
||||||
|
- Returns jwt.encode(payload, settings.secret_key, algorithm="HS256")
|
||||||
|
- Current payload keys: sub, role, typ, iat, exp
|
||||||
|
- Wave 1 adds: jti=str(uuid.uuid4())
|
||||||
|
- uuid is ALREADY imported at module top (services/auth.py:25)
|
||||||
|
- Settings used: settings.access_token_expire_minutes (already in scope)
|
||||||
|
|
||||||
|
<!-- Current get_current_user signature (deps/auth.py:38–80) -->
|
||||||
|
|
||||||
|
async def get_current_user(
|
||||||
|
request: Request, # already present, no change
|
||||||
|
credentials: HTTPAuthorizationCredentials = Depends(security),
|
||||||
|
session: AsyncSession = Depends(get_db),
|
||||||
|
) -> User:
|
||||||
|
- Step 1: payload = auth_service.decode_access_token(credentials.credentials) — raises ValueError → 401
|
||||||
|
- Step 2 (NEW Wave 1): user_nbf check via request.app.state.redis
|
||||||
|
- Step 3 (existing): uuid.UUID(payload["sub"]) → 401 on KeyError/ValueError
|
||||||
|
- Step 4 (existing): session.get(User, user_uuid); reject if None or not is_active → 401
|
||||||
|
- Step 5 (existing): request.state.current_user = user; return user
|
||||||
|
|
||||||
|
<!-- Existing fail-open template — services/auth.py check_hibp (~line 394) -->
|
||||||
|
|
||||||
|
try:
|
||||||
|
# ... call ...
|
||||||
|
except Exception as exc:
|
||||||
|
logger.warning("HIBP check failed (fail-open): %s", exc)
|
||||||
|
return False
|
||||||
|
|
||||||
|
<!-- Existing TTL constant convention — config.py -->
|
||||||
|
|
||||||
|
settings.access_token_expire_minutes: int = 15 (confirmed by RESEARCH.md L182)
|
||||||
|
TTL for user_nbf key = settings.access_token_expire_minutes * 60 = 900
|
||||||
|
NOTE: this Wave does NOT write user_nbf — only reads. TTL constant is used by Wave 2 writers.
|
||||||
|
|
||||||
|
<!-- Redis access pattern — already used in api/auth.py:197 and :566 -->
|
||||||
|
|
||||||
|
redis_client = request.app.state.redis
|
||||||
|
nbf_bytes = await redis_client.get(f"user_nbf:{payload['sub']}") # returns bytes | None
|
||||||
|
# Compare:
|
||||||
|
if nbf_bytes is not None and payload["iat"] < int(nbf_bytes.decode()):
|
||||||
|
raise HTTPException(401, "Session invalidated", headers={"WWW-Authenticate": "Bearer"})
|
||||||
|
|
||||||
|
<!-- PyJWT verified behavior (RESEARCH.md L165) -->
|
||||||
|
|
||||||
|
After jwt.decode():
|
||||||
|
- payload["iat"] is type int (Unix timestamp)
|
||||||
|
- payload["jti"] is verbatim the string we passed in (PyJWT does not validate jti format)
|
||||||
|
- Direct integer comparison: payload["iat"] < int(nbf_bytes.decode()) works correctly
|
||||||
|
</interfaces>
|
||||||
|
</context>
|
||||||
|
|
||||||
|
<tasks>
|
||||||
|
|
||||||
|
<task type="auto" tdd="true">
|
||||||
|
<name>Task 1: Add jti claim to create_access_token</name>
|
||||||
|
<files>backend/services/auth.py</files>
|
||||||
|
<read_first>
|
||||||
|
- backend/services/auth.py lines 1–30 (module docstring + imports — confirm `import uuid` already present at line 25)
|
||||||
|
- backend/services/auth.py lines 86–117 (create_access_token + decode_access_token — current implementations)
|
||||||
|
- backend/tests/test_task2_auth_service.py (Wave 0 stub test_create_access_token_includes_jti_claim — this is the test that must flip from XFAIL to XPASS or PASS)
|
||||||
|
- .planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-RESEARCH.md "Pattern 1" section (lines 143–166)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- Test 1 (positive): test_create_access_token_includes_jti_claim — decoded payload contains key "jti"; value is a string parseable as uuid.UUID; promoted from Wave 0 XFAIL to PASS (remove the xfail decorator in this task).
|
||||||
|
- Test 2 (uniqueness): two consecutive calls to create_access_token produce tokens whose decoded jti values differ — add a NEW small test alongside the promotion edit (one-line guard: `assert t1["jti"] != t2["jti"]`).
|
||||||
|
- Test 3 (regression guard): existing test_create_access_token_jwt_format and test_decode_access_token_valid still pass — the payload remains a valid HS256 JWT and existing keys (sub, role, typ, iat, exp) are unchanged.
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Edit `create_access_token` (backend/services/auth.py line 86–99): add exactly one key to the `payload` dict, placed after the existing `exp` line for grep-ability:
|
||||||
|
`"jti": str(uuid.uuid4()),`
|
||||||
|
|
||||||
|
Do NOT change the function signature. Do NOT change algorithm. Do NOT add a TTL constant here (Wave 2 handles user_nbf TTL).
|
||||||
|
|
||||||
|
Promote the Wave 0 stub `test_create_access_token_includes_jti_claim` in `backend/tests/test_task2_auth_service.py`: remove the `@pytest.mark.xfail(...)` decorator and replace any placeholder body with the real assertions:
|
||||||
|
- decoded = decode_access_token(token)
|
||||||
|
- `assert "jti" in decoded`
|
||||||
|
- `assert isinstance(decoded["jti"], str)`
|
||||||
|
- `uuid.UUID(decoded["jti"]) # raises ValueError if not a valid UUID`
|
||||||
|
|
||||||
|
Append one new test alongside it: `def test_create_access_token_jti_is_unique_per_call():` — mint two tokens for the same user, decode both, assert `t1["jti"] != t2["jti"]`. Imports per existing in-function-scope convention in this test file.
|
||||||
|
|
||||||
|
Do NOT modify decode_access_token — PyJWT decodes the jti claim automatically as part of the payload dict.
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest tests/test_task2_auth_service.py -v</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `cd backend && pytest tests/test_task2_auth_service.py -v` exits 0 with zero failures
|
||||||
|
- `grep -E "\"jti\":\\s*str\\(uuid\\.uuid4" backend/services/auth.py` returns 1 match
|
||||||
|
- `grep -v '^\\s*#' backend/services/auth.py | grep -E "\"jti\"" | head -1` shows the new jti line in create_access_token
|
||||||
|
- test_create_access_token_includes_jti_claim reports PASSED (no longer XFAIL)
|
||||||
|
- test_create_access_token_jti_is_unique_per_call reports PASSED
|
||||||
|
- test_create_access_token_jwt_format and test_decode_access_token_valid still PASSED (no regressions)
|
||||||
|
- Python REPL check: `from services.auth import create_access_token, decode_access_token; import uuid; t = create_access_token('u1','user'); p = decode_access_token(t); uuid.UUID(p['jti'])` returns a UUID object without raising
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>Every access token contains a unique jti UUID claim; Wave 0 stub promoted; zero existing test regressions.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
<task type="auto" tdd="true">
|
||||||
|
<name>Task 2: Add user_nbf Redis check to get_current_user with fail-open guard</name>
|
||||||
|
<files>backend/deps/auth.py</files>
|
||||||
|
<read_first>
|
||||||
|
- backend/deps/auth.py (entire file — 115 lines; current get_current_user at lines 38–80)
|
||||||
|
- backend/services/auth.py lines 286–297 (existing TOTP replay pattern — the reference template for `await redis.get / await redis.set` and `bytes | None` return type)
|
||||||
|
- backend/services/auth.py near line 394 (check_hibp fail-open template — confirm exact except-clause pattern with logger.warning)
|
||||||
|
- backend/api/auth.py lines 195–200 and 564–568 (existing `request.app.state.redis` access pattern — confirm direct attribute access without dependency injection)
|
||||||
|
- .planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-RESEARCH.md "Pattern 3" section (lines 186–235) AND "Pitfall 1" section (lines 279–284) AND "Pitfall 2" section (lines 286–290)
|
||||||
|
- backend/tests/test_auth_deps.py (three Wave 0 stubs created in Plan 01 — must flip from XFAIL to PASS in this task)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- Test 1 (reject path): `test_get_current_user_rejects_token_when_iat_before_user_nbf` — pre-populates `app.state.redis` key `user_nbf:{uid}` = `b"<ts_future>"`, calls /test/me with a token whose iat (encoded via `create_access_token`, hence `int(now())`) is less than `ts_future`, expects HTTP 401 with response JSON `{"detail": "Session invalidated"}` and `WWW-Authenticate: Bearer` header. Promote from XFAIL.
|
||||||
|
- Test 2 (allow path): `test_get_current_user_allows_token_when_iat_after_user_nbf` — pre-populates `user_nbf:{uid}` = `b"<ts_past>"`, calls /test/me with a token whose iat > ts_past, expects 200. Promote from XFAIL.
|
||||||
|
- Test 3 (fail-open): `test_get_current_user_failopen_on_redis_error` — mounts a broken Redis (raises RuntimeError on .get()), calls /test/me, expects 200 (no 5xx, no 401). Promote from XFAIL.
|
||||||
|
- Test 4 (regression guard): all 34+ existing tests in test_auth_deps.py still PASS (token decode, missing-user 401, deactivated-user 401, admin role check).
|
||||||
|
- Test 5 (no key path): if `user_nbf:{uid}` key is ABSENT from Redis (the common path), get_current_user must NOT raise — request passes through to the existing DB lookup. This is implicitly covered by the existing passing tests once FakeRedis is mounted (Plan 01 Task 1).
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Add a new block to `get_current_user` in backend/deps/auth.py, inserted AFTER the decode_access_token try/except block (currently lines 50–57) and BEFORE the `try: user_uuid = uuid.UUID(payload["sub"])` block (currently line 59).
|
||||||
|
|
||||||
|
Implementation requirements (concrete; do NOT inline code in this plan — read RESEARCH.md Pattern 3 for the exact shape):
|
||||||
|
|
||||||
|
1. Add module-level imports at top of deps/auth.py:
|
||||||
|
- `import logging`
|
||||||
|
- `_logger = logging.getLogger(__name__)` (module-level, after the imports)
|
||||||
|
|
||||||
|
2. Inside get_current_user, after the existing decode try/except, add a new try/except block:
|
||||||
|
- Inner work: `redis_client = request.app.state.redis`; `nbf_bytes = await redis_client.get(f"user_nbf:{payload['sub']}")`; if `nbf_bytes is not None and payload["iat"] < int(nbf_bytes.decode())` → raise `HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Session invalidated", headers={"WWW-Authenticate": "Bearer"})`.
|
||||||
|
- First except: `except HTTPException: raise` — MUST be present and MUST come BEFORE the broad except. This is Pitfall 1 from RESEARCH.md and is also documented in the threat model below (T-7.2-02).
|
||||||
|
- Second except: `except Exception as exc: _logger.warning("Redis user_nbf check failed (fail-open): %s", exc)` — mirrors the HIBP fail-open template at services/auth.py:~394.
|
||||||
|
|
||||||
|
3. Comparison direction MUST be `payload["iat"] < int(nbf_bytes.decode())` → reject (Pitfall 2). Do NOT use `<=` (a token issued at the exact same second as the event is allowed — edge case, but matches the "issued strictly before" semantic).
|
||||||
|
|
||||||
|
4. The `nbf_bytes` value handling: be tolerant of both `bytes` and `str` returns from Redis (FakeRedis in tests may return either depending on what was stored). Use `nbf_bytes.decode() if isinstance(nbf_bytes, (bytes, bytearray)) else nbf_bytes` before `int(...)`. Comment this with a reference to FakeRedis in test fixtures.
|
||||||
|
|
||||||
|
5. Do NOT add a `get_redis` FastAPI dependency. Per D-03 the access is direct via `request.app.state.redis`.
|
||||||
|
|
||||||
|
6. Do NOT change the function signature (request: Request is already present).
|
||||||
|
|
||||||
|
7. Do NOT touch `get_current_admin` or `get_regular_user` — they wrap get_current_user via Depends and inherit the check transparently.
|
||||||
|
|
||||||
|
Promote the three Wave 0 stubs in backend/tests/test_auth_deps.py:
|
||||||
|
- Remove the `@pytest.mark.xfail(...)` decorators from all three NBF-check tests created in Plan 01.
|
||||||
|
- Fill in the placeholder bodies with concrete assertions:
|
||||||
|
* Reject test: set `auth_client._transport.app.state.redis._store[f"user_nbf:{user.id}"] = (b"9999999999", None)` (FakeRedis tuple format: (value, expiry)) or use `await app.state.redis.set(f"user_nbf:{user.id}", b"9999999999")`. Mint token with create_access_token. GET /test/me with Bearer. Assert resp.status_code == 401 and resp.json()["detail"] == "Session invalidated".
|
||||||
|
* Allow test: set the key to `b"1"` (Unix ts way in the past). Mint token. Assert resp.status_code == 200.
|
||||||
|
* Fail-open test: replace app.state.redis with a small `class _BrokenRedis` whose async get/set raise RuntimeError. Mint a token. Assert resp.status_code == 200 (NOT 5xx, NOT 401).
|
||||||
|
|
||||||
|
Verify by direct grep that the HTTPException re-raise guard is in place — Pitfall 1 is the single highest-impact correctness risk in this plan.
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest tests/test_auth_deps.py tests/test_auth_api.py -v</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `cd backend && pytest tests/test_auth_deps.py tests/test_auth_api.py -v` exits 0 with zero failures
|
||||||
|
- `grep -c "user_nbf:" backend/deps/auth.py` returns >= 1
|
||||||
|
- `grep -E "except HTTPException:\\s*$" backend/deps/auth.py` returns >= 1 line — the re-raise guard is present
|
||||||
|
- `grep -E "except HTTPException" backend/deps/auth.py` line number is LESS than the `grep -n "except Exception" backend/deps/auth.py` line number (re-raise guard appears BEFORE broad catch — confirm with two grep -n calls and inspect ordering)
|
||||||
|
- `grep -E "import logging" backend/deps/auth.py` returns >= 1
|
||||||
|
- `grep -E "_logger.warning|logger.warning" backend/deps/auth.py` returns >= 1
|
||||||
|
- `grep -c "Session invalidated" backend/deps/auth.py` returns >= 1
|
||||||
|
- `grep -c "request.app.state.redis" backend/deps/auth.py` returns >= 1
|
||||||
|
- test_get_current_user_rejects_token_when_iat_before_user_nbf reports PASSED (no longer XFAIL)
|
||||||
|
- test_get_current_user_allows_token_when_iat_after_user_nbf reports PASSED
|
||||||
|
- test_get_current_user_failopen_on_redis_error reports PASSED
|
||||||
|
- All existing test_auth_deps tests (get_current_user_returns_user, deactivated user 401, missing user 401, admin role check) still PASSED
|
||||||
|
- All existing test_auth_api tests still PASSED (the NBF-write stubs from Plan 01 remain XFAIL — they are Wave 2's job to flip)
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>get_current_user enforces user_nbf with fail-open + HTTPException re-raise guard; three NBF-check stubs promoted to PASSED; zero regressions in 34+ baseline auth-dep tests.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
</tasks>
|
||||||
|
|
||||||
|
<threat_model>
|
||||||
|
## Trust Boundaries
|
||||||
|
|
||||||
|
| Boundary | Description |
|
||||||
|
|----------|-------------|
|
||||||
|
| client → API (Authorization: Bearer) | Untrusted JWT crosses here; signature + typ + NBF all validated in get_current_user |
|
||||||
|
| API → Redis (app.state.redis) | Trusted local network within Docker Compose; failures must not deny service (D-04 fail-open) |
|
||||||
|
|
||||||
|
## STRIDE Threat Register
|
||||||
|
|
||||||
|
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
||||||
|
|-----------|----------|-----------|-------------|-----------------|
|
||||||
|
| T-7.2-01 | Elevation of Privilege | get_current_user (NBF check) | mitigate | After successful decode, read `user_nbf:{payload['sub']}` from Redis; if present and `payload["iat"] < int(nbf_bytes.decode())`, raise HTTP 401 "Session invalidated" with WWW-Authenticate header. Closes the 15-minute window where a revoked session's live access token remains valid. |
|
||||||
|
| T-7.2-02 | Elevation of Privilege | fail-open except block in get_current_user | mitigate | Explicit `except HTTPException: raise` MUST precede the broad `except Exception` — otherwise the intentional 401 from the NBF check is swallowed and a revoked token leaks through. Verified by source assertion (grep ordering) in Task 2 acceptance criteria. |
|
||||||
|
| T-7.2-03 | Elevation of Privilege | NBF comparison direction | mitigate | Comparison is `payload["iat"] < int(nbf_bytes.decode())` — token issued BEFORE the event is rejected. Pitfall 2 from RESEARCH.md. Verified by test_get_current_user_rejects_token_when_iat_before_user_nbf. |
|
||||||
|
| T-7.2-04 | Denial of Service / EoP | Redis outage during NBF check | accept | D-04 mandates fail-open: if Redis raises Exception, log a warning and allow the request to proceed. Mirrors the HIBP fail-open pattern (services/auth.py:~394). Availability is prioritized over blocking-during-outage; the surface area is the 15-minute access-token lifetime. |
|
||||||
|
| T-7.2-05 | Information Disclosure | jti claim in JWT body | accept | jti is a UUIDv4 (no PII, no user identifier). RFC 7519 standard. Visible in any base64-decoded JWT — acceptable. |
|
||||||
|
| T-7.2-W1-SC | Tampering | npm/pip/cargo installs | n/a | No packages installed in this plan — uses only stdlib (uuid, logging) already imported in scope (RESEARCH.md "Package Legitimacy Audit" intentionally blank) |
|
||||||
|
</threat_model>
|
||||||
|
|
||||||
|
<verification>
|
||||||
|
- `cd backend && pytest tests/test_auth_deps.py tests/test_task2_auth_service.py -v` — zero failures; 4 stubs from Plan 01 (jti + 3 NBF-check) now PASSED
|
||||||
|
- `cd backend && pytest -v` — full suite green; zero new failures vs Phase 7.1's 373-test baseline (the 3 NBF-write stubs from Plan 01 Task 3 + 2 admin stubs from Plan 01 Task 4 remain XFAIL — they are Wave 2's job)
|
||||||
|
- Source assertions (Task 2 acceptance criteria) prove the Pitfall 1 re-raise guard is in place — this is the single most consequential correctness check in the plan
|
||||||
|
</verification>
|
||||||
|
|
||||||
|
<success_criteria>
|
||||||
|
- Every access token issued after this plan deploys contains a `jti` UUID claim (verifiable: decode any new token)
|
||||||
|
- get_current_user rejects tokens issued before any user_nbf timestamp written for that user (verifiable: pytest tests/test_auth_deps.py -k nbf)
|
||||||
|
- Redis outages produce log warnings but do not block requests (verifiable: test_get_current_user_failopen_on_redis_error)
|
||||||
|
- HTTPException raised by the NBF check is NEVER caught by the fail-open broad-except (verifiable: source ordering grep + test behavior)
|
||||||
|
- Wave 2 (Plan 03) can write `user_nbf:{user_id}` from the four security-event handlers and the check will fire on the next request from any pre-event token
|
||||||
|
</success_criteria>
|
||||||
|
|
||||||
|
<output>
|
||||||
|
Create `.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-02-SUMMARY.md` when done.
|
||||||
|
|
||||||
|
Required fields in SUMMARY: artifacts (services/auth.py + deps/auth.py), patterns_established (NBF check pattern in get_current_user; HTTPException re-raise guard before broad catch; bytes/str tolerance in Redis return values), patterns_to_avoid (do NOT use `<=` in NBF comparison; do NOT add a get_redis dependency; do NOT touch get_current_admin/get_regular_user), provides (jti claim issuance + NBF rejection enforcement — ready for Wave 2 write sites).
|
||||||
|
</output>
|
||||||
+330
@@ -0,0 +1,330 @@
|
|||||||
|
---
|
||||||
|
phase: 07.2-security-jti-claim-redis-access-token-revocation-inserted
|
||||||
|
plan: 03
|
||||||
|
type: execute
|
||||||
|
wave: 2
|
||||||
|
depends_on:
|
||||||
|
- 07.2-02
|
||||||
|
files_modified:
|
||||||
|
- backend/api/auth.py
|
||||||
|
- backend/api/admin.py
|
||||||
|
- backend/tests/test_auth_api.py
|
||||||
|
- backend/tests/test_admin_api.py
|
||||||
|
autonomous: true
|
||||||
|
requirements:
|
||||||
|
- CONCERNS:JTI-CLAIM
|
||||||
|
- CONCERNS:JTI-REVOKE-REDIS
|
||||||
|
tags:
|
||||||
|
- security
|
||||||
|
- jwt
|
||||||
|
- redis
|
||||||
|
- access-control
|
||||||
|
|
||||||
|
must_haves:
|
||||||
|
truths:
|
||||||
|
- "change_password writes user_nbf:{user_id} to Redis before session.commit() (D-02)"
|
||||||
|
- "enable_totp writes user_nbf:{user_id} to Redis before session.commit() (D-02)"
|
||||||
|
- "disable_totp writes user_nbf:{user_id} to Redis before session.commit() (D-02)"
|
||||||
|
- "admin deactivation writes user_nbf:{user_id} to Redis only when is_active=False (D-05)"
|
||||||
|
- "admin activation (is_active=True) does NOT write user_nbf (anti-pattern guard)"
|
||||||
|
- "TTL for all writes is settings.access_token_expire_minutes * 60 (not hardcoded 900)"
|
||||||
|
- "import time added at module level to api/auth.py and api/admin.py"
|
||||||
|
artifacts:
|
||||||
|
- path: "backend/api/auth.py"
|
||||||
|
provides: "user_nbf Redis write in change_password, enable_totp, disable_totp"
|
||||||
|
contains: "user_nbf"
|
||||||
|
- path: "backend/api/admin.py"
|
||||||
|
provides: "user_nbf Redis write in deactivation handler"
|
||||||
|
contains: "user_nbf"
|
||||||
|
key_links:
|
||||||
|
- from: "backend/api/auth.py change_password"
|
||||||
|
to: "request.app.state.redis"
|
||||||
|
via: "await redis.set(f'user_nbf:{current_user.id}', int(time.time()), ex=settings.access_token_expire_minutes * 60)"
|
||||||
|
pattern: "user_nbf.*current_user\\.id"
|
||||||
|
- from: "backend/api/admin.py deactivation handler (line ~351)"
|
||||||
|
to: "request.app.state.redis"
|
||||||
|
via: "conditional write only when not body.is_active"
|
||||||
|
pattern: "user_nbf.*user\\.id"
|
||||||
|
- from: "Wave 2 implementation"
|
||||||
|
to: "Wave 0 xfail stubs (test_auth_api NBF-write × 3, test_admin_api NBF-write × 1)"
|
||||||
|
via: "stubs promoted from XFAIL to PASSED"
|
||||||
|
pattern: "XPASS|xfail.*strict=False"
|
||||||
|
---
|
||||||
|
|
||||||
|
<objective>
|
||||||
|
Wire the user_nbf Redis writes into the four security-event handlers that trigger
|
||||||
|
access-token revocation.
|
||||||
|
|
||||||
|
Purpose: Plan 02 established the check (reader); this plan adds the writers. Until
|
||||||
|
these four writes are in place, the user_nbf:{user_id} key is never set in
|
||||||
|
production (it only exists in tests via direct FakeRedis manipulation), so the
|
||||||
|
15-minute revocation window remains open.
|
||||||
|
|
||||||
|
This plan is the final implementation step for Phase 7.2. After it completes:
|
||||||
|
- Any password change, TOTP enroll, TOTP revoke, or admin deactivation writes
|
||||||
|
user_nbf:{user_id} = now() to Redis with TTL = access token lifetime.
|
||||||
|
- get_current_user (Plan 02) reads the key and blocks pre-event tokens.
|
||||||
|
- All Wave 0 xfail stubs from Plan 01 that weren't promoted in Plan 02 flip to PASSED.
|
||||||
|
|
||||||
|
Output: Two files edited (api/auth.py, api/admin.py), four test stubs promoted.
|
||||||
|
</objective>
|
||||||
|
|
||||||
|
<execution_context>
|
||||||
|
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
|
||||||
|
@$HOME/.claude/get-shit-done/templates/summary.md
|
||||||
|
</execution_context>
|
||||||
|
|
||||||
|
<context>
|
||||||
|
@.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-CONTEXT.md
|
||||||
|
@.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-RESEARCH.md
|
||||||
|
@backend/api/auth.py
|
||||||
|
@backend/api/admin.py
|
||||||
|
@backend/tests/test_auth_api.py
|
||||||
|
@backend/tests/test_admin_api.py
|
||||||
|
@backend/config.py
|
||||||
|
|
||||||
|
<interfaces>
|
||||||
|
<!-- change_password handler — backend/api/auth.py:455 -->
|
||||||
|
|
||||||
|
Handler already calls:
|
||||||
|
1. auth_service.verify_password
|
||||||
|
2. auth_service.check_hibp
|
||||||
|
3. auth_service.validate_password_strength
|
||||||
|
4. auth_service.revoke_all_refresh_tokens(session, current_user.id, skip_token_hash=skip_hash)
|
||||||
|
5. write_audit_log(...)
|
||||||
|
6. await session.commit() ← insert user_nbf write before this line
|
||||||
|
|
||||||
|
The redis_client is NOT yet assigned in change_password — use request.app.state.redis inline.
|
||||||
|
|
||||||
|
<!-- enable_totp handler — backend/api/auth.py:552 -->
|
||||||
|
|
||||||
|
Handler already has at the top:
|
||||||
|
redis_client = request.app.state.redis ← reuse this variable
|
||||||
|
Then calls:
|
||||||
|
1. auth_service.verify_totp(..., redis_client)
|
||||||
|
2. user.totp_enabled = True
|
||||||
|
3. auth_service.generate_backup_codes / store_backup_codes
|
||||||
|
4. auth_service.revoke_all_refresh_tokens(session, current_user.id, skip_token_hash=skip_hash)
|
||||||
|
5. write_audit_log(...)
|
||||||
|
6. await session.commit() ← insert user_nbf write before this line
|
||||||
|
(redis_client already in scope — use it directly)
|
||||||
|
|
||||||
|
<!-- disable_totp handler — backend/api/auth.py:607 -->
|
||||||
|
|
||||||
|
Handler does NOT yet have a redis_client assignment.
|
||||||
|
Calls:
|
||||||
|
1. user.totp_enabled = False; user.totp_secret = None
|
||||||
|
2. delete(BackupCode) for user
|
||||||
|
3. auth_service.revoke_all_refresh_tokens(session, current_user.id, skip_token_hash=skip_hash)
|
||||||
|
4. write_audit_log(...)
|
||||||
|
5. await session.commit() ← insert user_nbf write before this line
|
||||||
|
|
||||||
|
<!-- admin deactivation handler — backend/api/admin.py:326 (PATCH /api/admin/users/{id}/status) -->
|
||||||
|
|
||||||
|
Handler at line ~351:
|
||||||
|
if not body.is_active:
|
||||||
|
# Revoke all refresh tokens on deactivation
|
||||||
|
await revoke_all_refresh_tokens(session, user.id)
|
||||||
|
← INSERT user_nbf write HERE (inside the if not body.is_active block, after revoke_all_refresh_tokens)
|
||||||
|
← Do NOT write if body.is_active is True (anti-pattern from RESEARCH.md)
|
||||||
|
|
||||||
|
admin.py does NOT currently import `time` (grep confirms no `import time` at module level).
|
||||||
|
auth.py does NOT currently import `time` (confirmed by codebase read).
|
||||||
|
|
||||||
|
<!-- TTL constant -->
|
||||||
|
|
||||||
|
settings.access_token_expire_minutes = 15 (from backend/config.py)
|
||||||
|
TTL = settings.access_token_expire_minutes * 60 (= 900 at default config)
|
||||||
|
Use this derived form — do NOT hardcode 900. Import `settings` is already present in api/auth.py.
|
||||||
|
In api/admin.py, verify `settings` is importable (it uses `from config import settings` per existing imports).
|
||||||
|
|
||||||
|
<!-- Redis write pattern (from RESEARCH.md Pattern 2, line 176) -->
|
||||||
|
|
||||||
|
await request.app.state.redis.set(
|
||||||
|
f"user_nbf:{current_user.id}",
|
||||||
|
int(time.time()),
|
||||||
|
ex=settings.access_token_expire_minutes * 60,
|
||||||
|
)
|
||||||
|
|
||||||
|
In enable_totp where redis_client is already assigned:
|
||||||
|
await redis_client.set(
|
||||||
|
f"user_nbf:{current_user.id}",
|
||||||
|
int(time.time()),
|
||||||
|
ex=settings.access_token_expire_minutes * 60,
|
||||||
|
)
|
||||||
|
|
||||||
|
In admin deactivation, the user variable is the target (not current_user):
|
||||||
|
await request.app.state.redis.set(
|
||||||
|
f"user_nbf:{user.id}",
|
||||||
|
int(time.time()),
|
||||||
|
ex=settings.access_token_expire_minutes * 60,
|
||||||
|
)
|
||||||
|
</interfaces>
|
||||||
|
</context>
|
||||||
|
|
||||||
|
<tasks>
|
||||||
|
|
||||||
|
<task type="auto">
|
||||||
|
<name>Task 1: Add import time + user_nbf writes to api/auth.py (change_password, enable_totp, disable_totp)</name>
|
||||||
|
<files>backend/api/auth.py</files>
|
||||||
|
<read_first>
|
||||||
|
- backend/api/auth.py lines 24–35 (module-level imports — add `import time` here, after existing stdlib imports)
|
||||||
|
- backend/api/auth.py lines 455–510 (change_password handler — find the `await session.commit()` line, insert write above it)
|
||||||
|
- backend/api/auth.py lines 552–605 (enable_totp handler — redis_client already in scope; find `await session.commit()`, insert write above it)
|
||||||
|
- backend/api/auth.py lines 607–650 (disable_totp handler — find `await session.commit()`, insert write above it)
|
||||||
|
- backend/config.py (confirm access_token_expire_minutes field name — e.g., `access_token_expire_minutes: int = 15`)
|
||||||
|
- .planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-RESEARCH.md "Pattern 2" section (lines 167–184) and "Anti-Patterns" (do NOT write on activation)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- change_password: after revoke_all_refresh_tokens + write_audit_log and before session.commit(), Redis key user_nbf:{current_user.id} is set with TTL=access_token_expire_minutes*60.
|
||||||
|
- enable_totp: same write, using the already-assigned redis_client variable, before session.commit().
|
||||||
|
- disable_totp: same write via request.app.state.redis, before session.commit().
|
||||||
|
- No write on any other handler (login, logout, register, refresh, etc.).
|
||||||
|
- `import time` is present at module level of api/auth.py.
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Add `import time` to api/auth.py after the existing `import uuid` line (line 26) — alphabetical placement in stdlib imports.
|
||||||
|
|
||||||
|
In change_password (line 455): locate the `await session.commit()` line at end of handler. Insert immediately above it:
|
||||||
|
`await request.app.state.redis.set(f"user_nbf:{current_user.id}", int(time.time()), ex=settings.access_token_expire_minutes * 60)`
|
||||||
|
|
||||||
|
In enable_totp (line 552): locate `await session.commit()` at end of handler. `redis_client` is already assigned near the top of this function (`redis_client = request.app.state.redis`). Insert immediately above session.commit():
|
||||||
|
`await redis_client.set(f"user_nbf:{current_user.id}", int(time.time()), ex=settings.access_token_expire_minutes * 60)`
|
||||||
|
|
||||||
|
In disable_totp (line 607): locate `await session.commit()` at end of handler. Insert immediately above it:
|
||||||
|
`await request.app.state.redis.set(f"user_nbf:{current_user.id}", int(time.time()), ex=settings.access_token_expire_minutes * 60)`
|
||||||
|
|
||||||
|
Do NOT add `settings` import (already present). Do NOT extract a TTL constant — the derived form is readable and stays synchronized with config automatically.
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest tests/test_auth_api.py -v -k "nbf or user_nbf"</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `grep -E "^import time" backend/api/auth.py` returns 1 match
|
||||||
|
- `grep -c "user_nbf" backend/api/auth.py` returns >= 3 (one per handler)
|
||||||
|
- `grep -c "access_token_expire_minutes \* 60" backend/api/auth.py` returns >= 3
|
||||||
|
- `cd backend && pytest tests/test_auth_api.py -v -k "nbf or user_nbf"` exits 0; at least 3 tests report PASSED (was XFAIL)
|
||||||
|
- `cd backend && pytest tests/test_auth_api.py -v` exits 0 with zero failures (zero regressions to existing change_password + totp tests)
|
||||||
|
- The three NBF-write stubs (test_change_password_writes_user_nbf_to_redis, test_enable_totp_writes_user_nbf_to_redis, test_disable_totp_writes_user_nbf_to_redis) are promoted from XFAIL to PASSED in this task — update the test bodies, removing the xfail decorators and replacing placeholder bodies with real assertions (see Task 2 of Plan 02 for the assertion pattern: `await app.state.redis.get(f"user_nbf:{user_id}")` returns a non-None bytes-like value whose `int(decode())` > 0)
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>import time added; user_nbf written in all three auth security-event handlers; stubs promoted to PASSED.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
<task type="auto">
|
||||||
|
<name>Task 2: Add import time + user_nbf write to api/admin.py deactivation handler</name>
|
||||||
|
<files>backend/api/admin.py</files>
|
||||||
|
<read_first>
|
||||||
|
- backend/api/admin.py lines 24–50 (module-level imports — add `import time` here)
|
||||||
|
- backend/api/admin.py lines 326–380 (PATCH /api/admin/users/{id}/status handler — locate the `if not body.is_active:` block at ~line 351; the write goes inside this block, after revoke_all_refresh_tokens)
|
||||||
|
- backend/config.py (confirm access_token_expire_minutes field name)
|
||||||
|
- .planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-RESEARCH.md "Pattern 4" (lines 239–252) and "Anti-Patterns" ("Do not write user_nbf for successful activation")
|
||||||
|
- backend/tests/test_admin_api.py (two Wave 0 stubs: test_deactivate_user_writes_user_nbf_to_redis + test_activate_user_does_not_write_user_nbf — promote both in this task)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- When PATCH /api/admin/users/{id}/status is called with `is_active=false`: after revoke_all_refresh_tokens, user_nbf:{user.id} is written to Redis with TTL=access_token_expire_minutes*60.
|
||||||
|
- When called with `is_active=true` (reactivation): user_nbf is NOT written (the negative guard test must pass).
|
||||||
|
- `import time` is present at module level of api/admin.py.
|
||||||
|
- `settings` must be in scope for the TTL expression — confirm it is already imported in admin.py.
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Add `import time` to api/admin.py after existing stdlib imports — alphabetical placement alongside `import uuid` (line 26).
|
||||||
|
|
||||||
|
Locate the deactivation block in the status handler (around line 351):
|
||||||
|
```
|
||||||
|
if not body.is_active:
|
||||||
|
# Revoke all refresh tokens on deactivation
|
||||||
|
await revoke_all_refresh_tokens(session, user.id)
|
||||||
|
```
|
||||||
|
Add one line immediately after `await revoke_all_refresh_tokens(session, user.id)`, still inside the `if not body.is_active:` block:
|
||||||
|
`await request.app.state.redis.set(f"user_nbf:{user.id}", int(time.time()), ex=settings.access_token_expire_minutes * 60)`
|
||||||
|
|
||||||
|
Verify `settings` is already imported in admin.py (it should be — grep for `from config import settings`). If not present, add `from config import settings` to the imports section.
|
||||||
|
|
||||||
|
Promote the two Wave 0 stubs in backend/tests/test_admin_api.py:
|
||||||
|
- test_deactivate_user_writes_user_nbf_to_redis: remove xfail decorator; replace placeholder body with real assertions: PATCH is_active=false → response 200; then `await client._transport.app.state.redis.get(f"user_nbf:{user.id}")` returns non-None bytes whose `int(decode())` > 0.
|
||||||
|
- test_activate_user_does_not_write_user_nbf: remove xfail decorator; replace body: PATCH is_active=true (on a deactivated user); then assert `await app.state.redis.get(f"user_nbf:{user.id}")` is None (the key should not exist for a reactivation event).
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest tests/test_admin_api.py -v</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `grep -E "^import time" backend/api/admin.py` returns 1 match
|
||||||
|
- `grep -c "user_nbf" backend/api/admin.py` returns >= 1
|
||||||
|
- `grep -c "access_token_expire_minutes \* 60" backend/api/admin.py` returns >= 1
|
||||||
|
- The user_nbf write line is inside the `if not body.is_active:` block (not at module scope or outside the conditional) — verify with grep -n and line-number inspection
|
||||||
|
- `cd backend && pytest tests/test_admin_api.py -v` exits 0 with zero failures
|
||||||
|
- test_deactivate_user_writes_user_nbf_to_redis reports PASSED (promoted from XFAIL)
|
||||||
|
- test_activate_user_does_not_write_user_nbf reports PASSED (promoted from XFAIL; Redis key must be None for activation path)
|
||||||
|
- All existing admin tests (test_deactivate_user, test_reactivate_user, etc.) still PASSED
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>import time added; user_nbf written in admin deactivation handler (conditional on is_active=False only); both stubs promoted to PASSED; zero admin test regressions.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
<task type="auto">
|
||||||
|
<name>Task 3: Full regression run — verify zero new failures across entire test suite</name>
|
||||||
|
<files></files>
|
||||||
|
<read_first>
|
||||||
|
- .planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-VALIDATION.md (Validation Sign-Off checklist)
|
||||||
|
</read_first>
|
||||||
|
<behavior>
|
||||||
|
- Full pytest suite passes with zero new failures vs the Phase 7.1 baseline.
|
||||||
|
- All 9 Phase 7.2 behaviors from VALIDATION.md report green: jti-claim PASSED, test-fakeredis PASSED, nbf-write × 4 PASSED, nbf-check × 2 PASSED, nbf-fail-open PASSED.
|
||||||
|
- No previously-XFAIL stubs remain (all were promoted in Plans 02 and 03).
|
||||||
|
</behavior>
|
||||||
|
<action>
|
||||||
|
Run the full backend test suite. Review output for any unexpected failures. If any test introduced by this plan fails, investigate and fix before marking this task done.
|
||||||
|
</action>
|
||||||
|
<verify>
|
||||||
|
<automated>cd backend && pytest -v 2>&1 | tail -20</automated>
|
||||||
|
</verify>
|
||||||
|
<acceptance_criteria>
|
||||||
|
- `cd backend && pytest -v` exits 0 with zero failures
|
||||||
|
- Output contains no FAILED lines
|
||||||
|
- `cd backend && pytest tests/test_auth_deps.py tests/test_auth_api.py tests/test_admin_api.py tests/test_task2_auth_service.py -v -k "jti or nbf or user_nbf or failopen"` exits 0 with at least 9 PASSED results covering all 9 VALIDATION.md behaviors
|
||||||
|
- `grep -rn "user_nbf" backend/api/auth.py backend/api/admin.py backend/deps/auth.py` returns >= 4 lines (3 writers in api/auth.py + 1 writer in api/admin.py + 1 reader in deps/auth.py)
|
||||||
|
</acceptance_criteria>
|
||||||
|
<done>Full suite green; all Phase 7.2 behaviors verified; no regressions.</done>
|
||||||
|
</task>
|
||||||
|
|
||||||
|
</tasks>
|
||||||
|
|
||||||
|
<threat_model>
|
||||||
|
## Trust Boundaries
|
||||||
|
|
||||||
|
| Boundary | Description |
|
||||||
|
|----------|-------------|
|
||||||
|
| client → API (security events) | Authenticated user or admin triggers password change / TOTP / deactivation; write must fire unconditionally before commit |
|
||||||
|
| API → Redis (app.state.redis) | Trusted local network; write is best-effort — if Redis is down, the session.commit() still completes (no rollback) |
|
||||||
|
|
||||||
|
## STRIDE Threat Register
|
||||||
|
|
||||||
|
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
||||||
|
|-----------|----------|-----------|-------------|-----------------|
|
||||||
|
| T-7.2-01 | Elevation of Privilege | All four security-event handlers | mitigate | user_nbf:{user_id} written with TTL=access_token_expire_minutes*60 before session.commit() on change_password, enable_totp, disable_totp, admin deactivation. Plan 02's get_current_user check fires on the next request from any pre-event token. |
|
||||||
|
| T-7.2-ACT | Elevation of Privilege | Admin activation path (is_active=True) | mitigate | Write is strictly inside `if not body.is_active:` block. Reactivation does NOT write user_nbf — a freshly reactivated user's first token is issued after reactivation and will have iat > any prior nbf. Verified by test_activate_user_does_not_write_user_nbf. |
|
||||||
|
| T-7.2-TTL | Elevation of Privilege | TTL mismatch between write sites | mitigate | All four sites use `settings.access_token_expire_minutes * 60` (not hardcoded 900) — stays synchronized if TTL changes in config. |
|
||||||
|
| T-7.2-W2-SC | Tampering | npm/pip/cargo installs | n/a | No new packages — only `import time` (stdlib) added. RESEARCH.md "Package Legitimacy Audit" is intentionally blank for this phase. |
|
||||||
|
</threat_model>
|
||||||
|
|
||||||
|
<verification>
|
||||||
|
- `cd backend && pytest -v` — zero failures; full suite green
|
||||||
|
- `grep -c "user_nbf" backend/api/auth.py` returns >= 3 (one per security-event handler)
|
||||||
|
- `grep -c "user_nbf" backend/api/admin.py` returns >= 1 (deactivation only)
|
||||||
|
- `grep -E "^import time" backend/api/auth.py backend/api/admin.py` returns 2 lines
|
||||||
|
- All 5 NBF-write/check/fail-open stubs from Plan 01 that were not promoted in Plan 02 now PASSED
|
||||||
|
- `cd backend && bandit -r backend/ --severity-level high` — zero HIGH severity findings introduced by this plan
|
||||||
|
</verification>
|
||||||
|
|
||||||
|
<success_criteria>
|
||||||
|
- All four security-event handlers write user_nbf:{user_id} to Redis with correct TTL
|
||||||
|
- Reactivation path does NOT write user_nbf
|
||||||
|
- TTL expression uses settings.access_token_expire_minutes * 60 in all four write sites
|
||||||
|
- Complete Phase 7.2 test matrix (9 behaviors from VALIDATION.md) reports PASSED
|
||||||
|
- Zero regressions: Phase 7.1's 373-test baseline unchanged
|
||||||
|
</success_criteria>
|
||||||
|
|
||||||
|
<output>
|
||||||
|
Create `.planning/phases/07.2-security-jti-claim-redis-access-token-revocation-inserted/07.2-03-SUMMARY.md` when done.
|
||||||
|
|
||||||
|
Required fields in SUMMARY: artifacts (api/auth.py + api/admin.py), patterns_established (user_nbf write pattern; conditional deactivation-only write; TTL derived from settings), patterns_to_avoid (do NOT write on activation; do NOT hardcode 900), provides (full Phase 7.2 revocation pipeline complete — jti issued + NBF checked + user_nbf written on all four security events).
|
||||||
|
</output>
|
||||||
+3
-1
@@ -387,17 +387,19 @@ except Exception as exc:
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Open Questions
|
## Open Questions (RESOLVED)
|
||||||
|
|
||||||
1. **Does `test_auth_deps.py` need a FakeRedis fixture?**
|
1. **Does `test_auth_deps.py` need a FakeRedis fixture?**
|
||||||
- What we know: `test_auth_deps.py` creates a minimal test app that calls `get_current_user`. After this phase, `get_current_user` will call `request.app.state.redis`. The minimal app has no lifespan handler.
|
- What we know: `test_auth_deps.py` creates a minimal test app that calls `get_current_user`. After this phase, `get_current_user` will call `request.app.state.redis`. The minimal app has no lifespan handler.
|
||||||
- What's unclear: Whether the existing `auth_client` fixture sets `app.state.redis` anywhere.
|
- What's unclear: Whether the existing `auth_client` fixture sets `app.state.redis` anywhere.
|
||||||
- Recommendation: Planner should add `test_app.state.redis = FakeRedis()` in `make_test_app()` or in the fixture. No Redis key will be set so all existing tests continue to pass.
|
- Recommendation: Planner should add `test_app.state.redis = FakeRedis()` in `make_test_app()` or in the fixture. No Redis key will be set so all existing tests continue to pass.
|
||||||
|
- **RESOLVED:** Plan 01 Task 1 adds `test_app.state.redis = FakeRedis()` in `make_test_app()`. FakeRedis is imported from `tests.test_auth_api`.
|
||||||
|
|
||||||
2. **Should `_USER_NBF_TTL` be a constant in `config.py` or a module constant in `api/auth.py`?**
|
2. **Should `_USER_NBF_TTL` be a constant in `config.py` or a module constant in `api/auth.py`?**
|
||||||
- What we know: `access_token_expire_minutes` is already in `config.py`; TTL = `access_token_expire_minutes * 60`.
|
- What we know: `access_token_expire_minutes` is already in `config.py`; TTL = `access_token_expire_minutes * 60`.
|
||||||
- What's unclear: Whether the user prefers `settings.access_token_expire_minutes * 60` (derived at runtime) or a hardcoded `900`.
|
- What's unclear: Whether the user prefers `settings.access_token_expire_minutes * 60` (derived at runtime) or a hardcoded `900`.
|
||||||
- Recommendation: Use `settings.access_token_expire_minutes * 60` in all write sites so that if the TTL ever changes in config, the NBF TTL stays synchronized automatically. This is slightly cleaner than a separate constant.
|
- Recommendation: Use `settings.access_token_expire_minutes * 60` in all write sites so that if the TTL ever changes in config, the NBF TTL stays synchronized automatically. This is slightly cleaner than a separate constant.
|
||||||
|
- **RESOLVED:** Plan 03 Tasks 1-2 use `settings.access_token_expire_minutes * 60` inline in all four write sites. No separate TTL constant extracted.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
+94
@@ -0,0 +1,94 @@
|
|||||||
|
---
|
||||||
|
phase: 7.2
|
||||||
|
slug: security-jti-claim-redis-access-token-revocation-inserted
|
||||||
|
status: draft
|
||||||
|
nyquist_compliant: false
|
||||||
|
wave_0_complete: false
|
||||||
|
created: 2026-06-05
|
||||||
|
---
|
||||||
|
|
||||||
|
# Phase 7.2 — Validation Strategy
|
||||||
|
|
||||||
|
> Per-phase validation contract for feedback sampling during execution.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Test Infrastructure
|
||||||
|
|
||||||
|
| Property | Value |
|
||||||
|
|----------|-------|
|
||||||
|
| **Framework** | pytest + pytest-asyncio |
|
||||||
|
| **Config file** | `backend/pytest.ini` |
|
||||||
|
| **Quick run command** | `pytest tests/test_auth_deps.py tests/test_auth_api.py tests/test_admin_api.py -v` |
|
||||||
|
| **Full suite command** | `cd backend && pytest -v` |
|
||||||
|
| **Estimated runtime** | ~30 seconds |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Sampling Rate
|
||||||
|
|
||||||
|
- **After every task commit:** Run `pytest tests/test_auth_deps.py tests/test_auth_api.py tests/test_admin_api.py -v`
|
||||||
|
- **After every plan wave:** Run `cd backend && pytest -v`
|
||||||
|
- **Before `/gsd:verify-work`:** Full suite must be green
|
||||||
|
- **Max feedback latency:** ~30 seconds
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Per-Task Verification Map
|
||||||
|
|
||||||
|
| Task ID | Plan | Wave | Requirement | Threat Ref | Secure Behavior | Test Type | Automated Command | File Exists | Status |
|
||||||
|
|---------|------|------|-------------|------------|-----------------|-----------|-------------------|-------------|--------|
|
||||||
|
| jti-claim | 01 | 0 | SEC (JTI) | — | Every access token contains a `jti` UUID key | unit | `pytest tests/test_task2_auth_service.py -k jti -v` | ❌ W0 | ⬜ pending |
|
||||||
|
| test-fakeredis | 01 | 0 | SEC (infra) | — | `test_auth_deps.py` test app has `app.state.redis` set | unit | `pytest tests/test_auth_deps.py -v` | ❌ W0 | ⬜ pending |
|
||||||
|
| nbf-write-change-password | 02 | 1 | SEC (NBF-write) | T-7.2-01 | `change_password` writes `user_nbf:{user_id}` to Redis | integration | `pytest tests/test_auth_api.py -k nbf -v` | ❌ W0 | ⬜ pending |
|
||||||
|
| nbf-write-enable-totp | 02 | 1 | SEC (NBF-write) | T-7.2-01 | `enable_totp` writes `user_nbf:{user_id}` to Redis | integration | `pytest tests/test_auth_api.py -k nbf -v` | ❌ W0 | ⬜ pending |
|
||||||
|
| nbf-write-disable-totp | 02 | 1 | SEC (NBF-write) | T-7.2-01 | `disable_totp` writes `user_nbf:{user_id}` to Redis | integration | `pytest tests/test_auth_api.py -k nbf -v` | ❌ W0 | ⬜ pending |
|
||||||
|
| nbf-write-deactivation | 03 | 1 | SEC (NBF-write) | T-7.2-01 | admin deactivation writes `user_nbf:{user_id}` to Redis | integration | `pytest tests/test_admin_api.py -k nbf -v` | ❌ W0 | ⬜ pending |
|
||||||
|
| nbf-check-reject | 04 | 1 | SEC (NBF-check) | T-7.2-02 | Token with `iat < user_nbf` returns HTTP 401 | integration | `pytest tests/test_auth_deps.py -k nbf -v` | ❌ W0 | ⬜ pending |
|
||||||
|
| nbf-check-allow | 04 | 1 | SEC (NBF-check) | T-7.2-03 | Token with `iat > user_nbf` is accepted | integration | `pytest tests/test_auth_deps.py -k nbf_after -v` | ❌ W0 | ⬜ pending |
|
||||||
|
| nbf-fail-open | 04 | 1 | SEC (fail-open) | T-7.2-04 | Redis error in NBF check does not block request | unit | `pytest tests/test_auth_deps.py -k failopen -v` | ❌ W0 | ⬜ pending |
|
||||||
|
|
||||||
|
*Status: ⬜ pending · ✅ green · ❌ red · ⚠️ flaky*
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Wave 0 Requirements
|
||||||
|
|
||||||
|
- [ ] `backend/tests/test_task2_auth_service.py` — add JTI presence test for `create_access_token`
|
||||||
|
- [ ] `backend/tests/test_auth_deps.py` — add `app.state.redis = FakeRedis()` to `make_test_app()` or auth_client fixture; add NBF-check tests (iat < nbf → 401, iat > nbf → pass, Redis error → pass)
|
||||||
|
- [ ] `backend/tests/test_auth_api.py` — add NBF-write stub tests for `change_password`, `enable_totp`, `disable_totp`
|
||||||
|
- [ ] `backend/tests/test_admin_api.py` — add NBF-write stub test for deactivation handler
|
||||||
|
|
||||||
|
*Existing 34+ auth tests in `test_auth_deps.py` and `test_auth_api.py` must remain green with zero regressions.*
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Manual-Only Verifications
|
||||||
|
|
||||||
|
| Behavior | Requirement | Why Manual | Test Instructions |
|
||||||
|
|----------|-------------|------------|-------------------|
|
||||||
|
| Recovering session after password change | SEC (pitfall 3) | Requires live frontend + token refresh flow | 1. Login. 2. Change password via API. 3. Use old access token → expect 401. 4. Use refresh token → expect new access token with `iat > nbf`. 5. Confirm new token works. |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Threat Model
|
||||||
|
|
||||||
|
| Threat | STRIDE | Mitigation in this phase |
|
||||||
|
|--------|--------|--------------------------|
|
||||||
|
| T-7.2-01: Stolen access token replay after security event | EoP | `user_nbf:{user_id}` Redis key blocks all tokens with `iat` before the event |
|
||||||
|
| T-7.2-02: HTTPException swallowed by fail-open catch | EoP | Explicit `except HTTPException: raise` guard before broad `except Exception` in `get_current_user` |
|
||||||
|
| T-7.2-03: Wrong comparison direction in NBF check | EoP | `payload["iat"] < nbf_timestamp` — token issued BEFORE event → reject |
|
||||||
|
| T-7.2-04: Redis outage bypasses revocation check | DoS/EoP | Fail-open accepted (D-04); availability > revocation during outage — known trade-off |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Validation Sign-Off
|
||||||
|
|
||||||
|
- [ ] All tasks have `<automated>` verify or Wave 0 dependencies
|
||||||
|
- [ ] Sampling continuity: no 3 consecutive tasks without automated verify
|
||||||
|
- [ ] Wave 0 covers all MISSING references
|
||||||
|
- [ ] No watch-mode flags
|
||||||
|
- [ ] Feedback latency < 30s
|
||||||
|
- [ ] `nyquist_compliant: true` set in frontmatter
|
||||||
|
|
||||||
|
**Approval:** pending
|
||||||
Reference in New Issue
Block a user