feat: add Keycloak JWT auth and role-based access control

forgejo_admin commented

2026-03-16 01:50:09 +00:00

Author

Contributor

PR #5 Review

DOMAIN REVIEW

Tech stack: Python 3.12+ / FastAPI / SQLAlchemy / Pydantic / python-jose / httpx

This PR adds a Keycloak OIDC JWT validation module (auth.py) and a keycloak_realm_url config setting. No routes are wired to auth yet -- this is dependency-only groundwork. The auth module follows the established basketball-api pattern: JWKS-based RS256 validation, User dataclass, get_current_user dependency, require_role factory.

Python/FastAPI findings:

PEP 484 type hints: Good. Callable return type on require_role, User dataclass fields typed, dict | None union syntax used correctly.
PEP 257 docstrings: All public functions and the module have docstrings. Clean.
FastAPI dependency injection: get_current_user and require_role follow the standard Depends() pattern correctly.
Pydantic settings: keycloak_realm_url correctly added to Settings with env_prefix inheritance from MCD_TRACKER_. Will read from MCD_TRACKER_KEYCLOAK_REALM_URL env var.
OWASP/Security: JWT validation uses RS256 (asymmetric), issuer verification is enabled, verify_aud: False is documented with rationale, JWKS cache-bust-and-retry handles key rotation. WWW-Authenticate: Bearer header included on 401s per RFC 6750.
Synchronous httpx in async context: _fetch_jwks() uses synchronous httpx.get() inside an async def dependency chain. This blocks the event loop during the first request (and any cache-bust). The comment justifies it ("cached after the first call, in-cluster round-trip is sub-millisecond") -- acceptable for now but will become a problem under concurrent load if Keycloak is slow.

BLOCKERS

1. DRY violation in auth/security path (BLOCKER)

src/mcd_tracker_api/auth.py (176 lines) is a verbatim copy of basketball-api/src/basketball_api/auth.py (177 lines, the extra line being a trailing newline). The only differences are:

Line 1: "mcd-tracker-api" vs "basketball-api" in the module docstring
Line 16: from mcd_tracker_api.config import settings vs from basketball_api.config import settings

Everything else -- the User dataclass, _fetch_jwks, _extract_roles, get_current_user, require_role, the JWKS cache-bust logic, error messages, HTTP status codes, logging -- is identical.

Per the BLOCKER criteria: "DRY violation in auth/security paths (duplicated auth logic = divergence risk)". If a security fix is needed (e.g., adding exp validation, fixing a JWT parsing edge case), it must be applied to every copy independently. This is divergence risk.

Recommended fix: Extract the shared auth module into a reusable package (e.g., pal-e-auth or a shared keycloak-auth library) that both basketball-api and mcd-tracker-api depend on. The only per-app configuration is the keycloak_realm_url setting, which can be injected. Alternatively, if a shared package is premature, acknowledge this as tech debt and file a tracking issue -- but the code must not ship without a plan to converge.

Note for the author: I recognize this is a deliberate pattern-follow from basketball-api, and the copy is clean. The blocker is about the organizational risk of maintaining identical security logic in two places, not about the quality of the code itself.

NITS

Hardcoded default URL in config.py (line 8): keycloak_realm_url defaults to "https://keycloak.tail5b443a.ts.net/realms/mcd-tracker". While not a secret, hardcoding the Tailscale hostname means the default only works in one environment. Consider making this field required (no default) so missing config fails fast, or use a sentinel that clearly signals "not configured."
test_wrong_issuer_returns_401 is fragile (test_auth.py lines 149-167): This test re-patches _fetch_jwks and settings inside an autouse fixture that already patches those same things. The nested with block creates a double-mock situation. The test works because the inner patch wins, but the comments inside the test reveal confusion about the mock layering ("We need to let the real jwt.decode run, so unpatch _fetch_jwks briefly / Actually the mock is already in place from the fixture"). Consider restructuring: either skip the autouse fixture for this test (using a marker) or simplify the mock setup.
noqa: E402 imports in test_auth.py (lines 42-46, 56): The cryptography and base64 imports are placed after the test app definition, requiring noqa suppressions. Moving these imports to the top of the file (standard position) would eliminate the suppressions. The test app definition does not depend on them.
Module-level global keyword usage: The global _jwks_cache in _fetch_jwks and get_current_user works but is a code smell in larger codebases. A class-based cache (e.g., JWKSCache singleton) would be more testable and avoid the global state. Non-blocking -- the current approach matches basketball-api.
Missing exp claim test: The tests do not verify that expired tokens are rejected. python-jose validates exp by default, so this works implicitly, but an explicit test for expired token rejection would document the expected behavior and catch regressions if options ever changes.

SOP COMPLIANCE

Branch named after issue (4-add-keycloak-realm-jwt-auth-and-role-bas -- references issue #4)
PR body has: Summary, Changes, Test Plan, Related
Related section references plan slug -- PR body says plan-pal-e-platform, but the dispatch specified plan-mcd-tracker. If plan-mcd-tracker exists, the Related section should reference it instead.
Tests exist and pass (15 new auth tests + 9 existing = 24 total per PR body)
No secrets committed (realm URL is not a secret; no client secrets, no passwords)
No unnecessary file changes (4 files, all directly related to auth)
Commit messages are descriptive
Review Checklist section in PR body -- present but not the standard template format (## Review Checklist vs expected ## Related). Minor.

PROCESS OBSERVATIONS

Deployment frequency: This is dependency-only groundwork (no routes use auth yet). Clean separation of concerns -- auth infrastructure ships first, route wiring ships separately. Good for incremental review.
Change failure risk: LOW for the auth module itself (no routes consume it yet, so it cannot break existing behavior). The DRY concern is about future maintenance risk, not immediate failure.
Cross-repo duplication: This is the second Keycloak auth module in the platform (basketball-api was first). A third app will make extraction unavoidable. Filing a tech-debt issue now prevents the third copy from happening silently.
Test quality: The mocked JWKS approach is solid -- no live Keycloak dependency in CI. The RSA key generation in tests is well-structured.

VERDICT: NOT APPROVED

The auth module is well-written and the test coverage is strong, but the verbatim copy of basketball-api/auth.py is a BLOCKER per DRY-in-security-paths criteria. The recommended path forward is one of:

Extract a shared keycloak-auth package (best)
File a tracking issue acknowledging the duplication as tech debt with a concrete plan to converge before a third consumer appears (acceptable)

Either approach unblocks this PR. The code quality itself is not in question -- the organizational risk of maintaining identical security logic across repos is.

## PR #5 Review ### DOMAIN REVIEW **Tech stack:** Python 3.12+ / FastAPI / SQLAlchemy / Pydantic / python-jose / httpx This PR adds a Keycloak OIDC JWT validation module (`auth.py`) and a `keycloak_realm_url` config setting. No routes are wired to auth yet -- this is dependency-only groundwork. The auth module follows the established basketball-api pattern: JWKS-based RS256 validation, `User` dataclass, `get_current_user` dependency, `require_role` factory. **Python/FastAPI findings:** - **PEP 484 type hints**: Good. `Callable` return type on `require_role`, `User` dataclass fields typed, `dict | None` union syntax used correctly. - **PEP 257 docstrings**: All public functions and the module have docstrings. Clean. - **FastAPI dependency injection**: `get_current_user` and `require_role` follow the standard `Depends()` pattern correctly. - **Pydantic settings**: `keycloak_realm_url` correctly added to `Settings` with `env_prefix` inheritance from `MCD_TRACKER_`. Will read from `MCD_TRACKER_KEYCLOAK_REALM_URL` env var. - **OWASP/Security**: JWT validation uses RS256 (asymmetric), issuer verification is enabled, `verify_aud: False` is documented with rationale, JWKS cache-bust-and-retry handles key rotation. `WWW-Authenticate: Bearer` header included on 401s per RFC 6750. - **Synchronous httpx in async context**: `_fetch_jwks()` uses synchronous `httpx.get()` inside an `async def` dependency chain. This blocks the event loop during the first request (and any cache-bust). The comment justifies it ("cached after the first call, in-cluster round-trip is sub-millisecond") -- acceptable for now but will become a problem under concurrent load if Keycloak is slow. ### BLOCKERS **1. DRY violation in auth/security path (BLOCKER)** `src/mcd_tracker_api/auth.py` (176 lines) is a verbatim copy of `basketball-api/src/basketball_api/auth.py` (177 lines, the extra line being a trailing newline). The only differences are: - Line 1: `"mcd-tracker-api"` vs `"basketball-api"` in the module docstring - Line 16: `from mcd_tracker_api.config import settings` vs `from basketball_api.config import settings` Everything else -- the `User` dataclass, `_fetch_jwks`, `_extract_roles`, `get_current_user`, `require_role`, the JWKS cache-bust logic, error messages, HTTP status codes, logging -- is identical. Per the BLOCKER criteria: *"DRY violation in auth/security paths (duplicated auth logic = divergence risk)"*. If a security fix is needed (e.g., adding `exp` validation, fixing a JWT parsing edge case), it must be applied to every copy independently. This is divergence risk. **Recommended fix:** Extract the shared auth module into a reusable package (e.g., `pal-e-auth` or a shared `keycloak-auth` library) that both `basketball-api` and `mcd-tracker-api` depend on. The only per-app configuration is the `keycloak_realm_url` setting, which can be injected. Alternatively, if a shared package is premature, acknowledge this as tech debt and file a tracking issue -- but the code must not ship without a plan to converge. **Note for the author:** I recognize this is a deliberate pattern-follow from basketball-api, and the copy is clean. The blocker is about the organizational risk of maintaining identical security logic in two places, not about the quality of the code itself. ### NITS 1. **Hardcoded default URL in config.py (line 8):** `keycloak_realm_url` defaults to `"https://keycloak.tail5b443a.ts.net/realms/mcd-tracker"`. While not a secret, hardcoding the Tailscale hostname means the default only works in one environment. Consider making this field required (no default) so missing config fails fast, or use a sentinel that clearly signals "not configured." 2. **`test_wrong_issuer_returns_401` is fragile (test_auth.py lines 149-167):** This test re-patches `_fetch_jwks` and `settings` inside an `autouse` fixture that already patches those same things. The nested `with` block creates a double-mock situation. The test works because the inner patch wins, but the comments inside the test reveal confusion about the mock layering ("We need to let the real jwt.decode run, so unpatch _fetch_jwks briefly / Actually the mock is already in place from the fixture"). Consider restructuring: either skip the autouse fixture for this test (using a marker) or simplify the mock setup. 3. **`noqa: E402` imports in test_auth.py (lines 42-46, 56):** The `cryptography` and `base64` imports are placed after the test app definition, requiring `noqa` suppressions. Moving these imports to the top of the file (standard position) would eliminate the suppressions. The test app definition does not depend on them. 4. **Module-level `global` keyword usage:** The `global _jwks_cache` in `_fetch_jwks` and `get_current_user` works but is a code smell in larger codebases. A class-based cache (e.g., `JWKSCache` singleton) would be more testable and avoid the global state. Non-blocking -- the current approach matches basketball-api. 5. **Missing `exp` claim test:** The tests do not verify that expired tokens are rejected. `python-jose` validates `exp` by default, so this works implicitly, but an explicit test for expired token rejection would document the expected behavior and catch regressions if `options` ever changes. ### SOP COMPLIANCE - [x] Branch named after issue (`4-add-keycloak-realm-jwt-auth-and-role-bas` -- references issue #4) - [x] PR body has: Summary, Changes, Test Plan, Related - [ ] Related section references plan slug -- PR body says `plan-pal-e-platform`, but the dispatch specified `plan-mcd-tracker`. If `plan-mcd-tracker` exists, the Related section should reference it instead. - [x] Tests exist and pass (15 new auth tests + 9 existing = 24 total per PR body) - [x] No secrets committed (realm URL is not a secret; no client secrets, no passwords) - [x] No unnecessary file changes (4 files, all directly related to auth) - [x] Commit messages are descriptive - [ ] Review Checklist section in PR body -- present but not the standard template format (`## Review Checklist` vs expected `## Related`). Minor. ### PROCESS OBSERVATIONS - **Deployment frequency:** This is dependency-only groundwork (no routes use auth yet). Clean separation of concerns -- auth infrastructure ships first, route wiring ships separately. Good for incremental review. - **Change failure risk:** LOW for the auth module itself (no routes consume it yet, so it cannot break existing behavior). The DRY concern is about future maintenance risk, not immediate failure. - **Cross-repo duplication:** This is the second Keycloak auth module in the platform (basketball-api was first). A third app will make extraction unavoidable. Filing a tech-debt issue now prevents the third copy from happening silently. - **Test quality:** The mocked JWKS approach is solid -- no live Keycloak dependency in CI. The RSA key generation in tests is well-structured. ### VERDICT: NOT APPROVED The auth module is well-written and the test coverage is strong, but the verbatim copy of `basketball-api/auth.py` is a BLOCKER per DRY-in-security-paths criteria. The recommended path forward is one of: 1. Extract a shared `keycloak-auth` package (best) 2. File a tracking issue acknowledging the duplication as tech debt with a concrete plan to converge before a third consumer appears (acceptable) Either approach unblocks this PR. The code quality itself is not in question -- the organizational risk of maintaining identical security logic across repos is.

2026-03-16 01:47:03 +00:00

Summary

Set up Keycloak mcd-tracker realm with roles (user, admin), two OIDC clients (mcd-tracker-app confidential, mcd-tracker-ios public PKCE), and test users (testuser with user role, testadmin with admin role)
Add auth.py following basketball-api pattern: JWKS-based JWT validation, User dataclass, get_current_user dependency, and require_role factory
No API routes use auth yet -- this PR only adds the auth dependencies

Changes

src/mcd_tracker_api/auth.py -- new module: Keycloak OIDC JWT validation, User dataclass, get_current_user, require_role
src/mcd_tracker_api/config.py -- add keycloak_realm_url setting (external URL)
pyproject.toml -- add python-jose[cryptography] and httpx dependencies
tests/test_auth.py -- 15 tests with mocked JWKS (no live Keycloak in CI): token validation, missing/invalid tokens, wrong kid/issuer, role enforcement, case insensitivity

Test Plan

24 tests pass (15 auth + 9 existing)
ruff check and ruff format --check pass
Auth tests use mocked RSA keys and JWKS -- no Keycloak needed in CI
Keycloak realm verified live: testuser token contains user role, issuer is external URL

Review Checklist

No secrets committed
No unnecessary file changes
Commit messages are descriptive
Client secret not in code (stored in Keycloak only)

Closes forgejo_admin/mcd-tracker-api #4
Deployment env var: forgejo_admin/pal-e-deployments #17
plan-pal-e-platform

Closes #4

## Summary - Set up Keycloak `mcd-tracker` realm with roles (`user`, `admin`), two OIDC clients (`mcd-tracker-app` confidential, `mcd-tracker-ios` public PKCE), and test users (`testuser` with user role, `testadmin` with admin role) - Add auth.py following basketball-api pattern: JWKS-based JWT validation, User dataclass, `get_current_user` dependency, and `require_role` factory - No API routes use auth yet -- this PR only adds the auth dependencies ## Changes - `src/mcd_tracker_api/auth.py` -- new module: Keycloak OIDC JWT validation, User dataclass, get_current_user, require_role - `src/mcd_tracker_api/config.py` -- add `keycloak_realm_url` setting (external URL) - `pyproject.toml` -- add `python-jose[cryptography]` and `httpx` dependencies - `tests/test_auth.py` -- 15 tests with mocked JWKS (no live Keycloak in CI): token validation, missing/invalid tokens, wrong kid/issuer, role enforcement, case insensitivity ## Test Plan - [x] 24 tests pass (15 auth + 9 existing) - [x] `ruff check` and `ruff format --check` pass - [x] Auth tests use mocked RSA keys and JWKS -- no Keycloak needed in CI - [x] Keycloak realm verified live: testuser token contains `user` role, issuer is external URL ## Review Checklist - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive - [x] Client secret not in code (stored in Keycloak only) ## Related - Closes forgejo_admin/mcd-tracker-api #4 - Deployment env var: forgejo_admin/pal-e-deployments #17 - `plan-pal-e-platform` Closes #4

forgejo_admin added 1 commit

feat: add Keycloak JWT auth module with role-based access control

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

6de879c08b

Add auth.py following basketball-api pattern: JWKS-based JWT validation,
User dataclass, get_current_user dependency, and require_role factory.
Add keycloak_realm_url to config. Add python-jose and httpx dependencies.
15 auth tests with mocked JWKS (no live Keycloak needed in CI).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

2026-03-16 01:47:34 +00:00

Self-Review

Code Quality: Clean. auth.py is a 1:1 match of the basketball-api pattern adapted for mcd-tracker-api.

Tests: 15 tests covering valid tokens, missing/invalid tokens, wrong kid, wrong issuer, multiple roles, role enforcement (403), case insensitivity, and User dataclass defaults. All use mocked RSA keys and JWKS -- no Keycloak dependency in CI.

Findings: None. No secrets in code, no unnecessary changes, ruff passes clean.

Keycloak realm verified live:

Realm: mcd-tracker with roles user, admin
Clients: mcd-tracker-app (confidential), mcd-tracker-ios (public PKCE)
Test users: testuser (user role), testadmin (admin role)
Token flow confirmed: issuer is https://keycloak.tail5b443a.ts.net/realms/mcd-tracker (external URL)

Full test suite: 24/24 passing (15 auth + 9 existing).

## Self-Review **Code Quality:** Clean. auth.py is a 1:1 match of the basketball-api pattern adapted for mcd-tracker-api. **Tests:** 15 tests covering valid tokens, missing/invalid tokens, wrong kid, wrong issuer, multiple roles, role enforcement (403), case insensitivity, and User dataclass defaults. All use mocked RSA keys and JWKS -- no Keycloak dependency in CI. **Findings:** None. No secrets in code, no unnecessary changes, ruff passes clean. **Keycloak realm verified live:** - Realm: `mcd-tracker` with roles `user`, `admin` - Clients: `mcd-tracker-app` (confidential), `mcd-tracker-ios` (public PKCE) - Test users: `testuser` (user role), `testadmin` (admin role) - Token flow confirmed: issuer is `https://keycloak.tail5b443a.ts.net/realms/mcd-tracker` (external URL) **Full test suite:** 24/24 passing (15 auth + 9 existing).