feat: add Keycloak OIDC auth middleware with tenant-scoped access control #5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "4-keycloak-oidc-auth-middleware"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Adds Keycloak OIDC Bearer token authentication to all 15 REST endpoints. Admin users get unrestricted access. Stakeholder users are scoped to the
assetsbucket under their Keycloak group prefix (e.g. group/westsidemaps toassets/westside/), with no delete permission.AUTH_DISABLED=truebypasses all auth for local development.Changes
src/minio_api/auth.py-- NEW: Keycloak OIDC module (JWKS fetching with TTL cache, JWT validation via PyJWT,TokenUserdataclass with role/group extraction,AuthErrorexception)src/minio_api/permissions.py-- NEW: Access control logic (check_bucket_access,check_key_access,check_prefix_access,check_delete_permission,check_bucket_write_permission,filter_buckets_for_user,get_allowed_prefix)src/minio_api/dependencies.py-- Addedget_current_userFastAPI dependency that validates Bearer tokens or returns anonymous admin when auth disabledsrc/minio_api/routes/buckets.py-- Injectedget_current_userdependency on all 5 endpoints; admin-only create/delete; stakeholder bucket filtering on listsrc/minio_api/routes/objects.py-- Injected auth dependency on all 6 endpoints; tenant prefix scoping on list/get/put; admin-only delete/batch-deletesrc/minio_api/routes/presign.py-- Injected auth dependency on both endpoints; tenant prefix scopingsrc/minio_api/routes/multipart.py-- Injected auth dependency on all 3 endpoints; tenant prefix scoping on initiate/complete; admin-only aborttests/test_auth.py-- NEW: 15 unit tests for JWT validation with mock RSA keys (expired, bad signature, wrong audience/issuer, missing groups)tests/test_permissions.py-- NEW: 28 unit tests for access control logic (admin vs stakeholder vs no-group for all permission checks)tests/test_auth_middleware.py-- NEW: 20 HTTP-level tests verifying 401 without token and 403 for stakeholder-forbidden operationstests/conftest.py-- Added_disable_authautouse fixture so existing integration tests continue to workpyproject.toml-- AddedPyJWT[crypto]>=2.8andhttpx>=0.28dependencies; addedallow-direct-referencesfor hatchCLAUDE.md-- Updated auth docs, env vars table, project layoutTest Plan
pytest tests/test_auth.py tests/test_permissions.py tests/test_auth_middleware.py -v)ruff checkandruff formatcleanReview Checklist
assetsbucket, scoped to group prefixRelated
plan-minio-mobile(traceability)Self-Review
Reviewed all 13 changed files (1,179 lines added). Two issues found and fixed in
da19479:Fixed
Requestimport and parameter independencies.py--get_current_useraccepted arequest: Requestparameter that was never used. Removed.auth.py--decode_tokencaught JWT exceptions and re-raisedAuthErrorwithoutfrom exc, losing the original traceback. Added properfrom excchaining on all 6 except branches.Verified Clean
ruff checkandruff formatcleanAUTH_DISABLED=truebypass works for local dev and integration testsPR #5 Review
DOMAIN REVIEW
Tech stack: Python 3.12 / FastAPI / Pydantic v2 / PyJWT / Keycloak OIDC. No SQLAlchemy or database layer. This is a pure auth middleware + access control PR on top of an existing REST service.
Architecture assessment: Clean separation of concerns. Auth logic (
auth.py) handles JWT validation and user model. Permissions logic (permissions.py) handles access control decisions. Dependencies (dependencies.py) provides the FastAPI DI glue. Route files consume both viaDepends(). This is textbook FastAPI pattern -- no anti-patterns detected.Security review (OIDC / JWT):
JWKS caching: Correctly implemented.
_get_jwk_client()usestime.monotonic()with configurableJWKS_CACHE_TTL(default 3600s). ThePyJWKClientis also constructed withcache_keys=Truefor its own internal caching. JWKS is NOT fetched per-request. Acceptance criterion met.Token expiration: Enforced.
jwt.decode()is called withoptions={"require": ["exp", "iss", "sub"]}, andExpiredSignatureErroris caught and mapped toAuthError("Token has expired"). Verified bytest_expired_tokentest.Signature validation: Correct. Signing key is retrieved from the JWKS client via
get_signing_key_from_jwt(token), then passed tojwt.decode()withalgorithms=["RS256"]. Bad signatures are tested viatest_bad_signaturewhich signs with a different RSA key.Audience and issuer validation: Both validated in
jwt.decode()call viaaudience=KEYCLOAK_CLIENT_IDandissuer=_issuer_url(). Teststest_wrong_audienceandtest_wrong_issuerconfirm 401 behavior.Group claim extraction: Groups extracted from
payload.get("groups", [])with defensive type checking (if not isinstance(groups, list): groups = []). Groups are stripped of leading slashes and mapped toassets/<group>/prefixes. Test coverage confirms missing group claim produces empty list.Admin bypass:
user.is_adminchecks"admin" in self.rolesfromrealm_access.roles. All permission checks short-circuit onis_admin. Correct.Stakeholder scoping: Thoroughly implemented:
STAKEHOLDER_ALLOWED_BUCKETS = frozenset({"assets"})-- immutable, good.tenant_prefixes.check_delete_permission().check_bucket_write_permission().filter_buckets_for_user().allowed_prefixin route handler.401 vs 403 separation: Clean. Missing/invalid/expired token = 401 (from
get_current_userindependencies.py). Valid token but insufficient permissions = 403 (fromAccessDeniedexceptions caught in route handlers).WWW-Authenticate: Bearerheader correctly included on 401 responses.AUTH_DISABLED bypass: Reads
AUTH_DISABLEDfrom env var at module load. When true,get_current_userreturnsmake_anonymous_admin()(synthetic admin, no real token needed). The conftest_disable_authfixture patches this for existing integration tests. Correct pattern.No hardcoded credentials: Verified.
KEYCLOAK_URL,KEYCLOAK_REALM,KEYCLOAK_CLIENT_IDcome from env vars with sensible defaults. No API keys, passwords, or tokens in code. The defaultKEYCLOAK_URLvalue (https://keycloak.tail5b443a.ts.net) is a Tailscale hostname, not a secret.All 15 endpoints protected: Verified by reading every route file. Every endpoint (5 bucket + 5 object + 2 presign + 3 multipart) has
user: TokenUser = Depends(get_current_user). The/healthendpoint correctly does NOT require auth (test_health_no_auth_requiredconfirms this).Exception chaining: All
raise HTTPException(...) from excpatterns use proper exception chaining. No swallowed errors.PEP compliance:
from __future__ import annotationsused consistently for forward references.FastAPI patterns:
Depends()-- correct.HTTPBearer(auto_error=False)-- correct choice sinceget_current_userhandles the None case explicitly (needed forAUTH_DISABLEDmode where no token is sent).Test coverage:
test_auth.py: TokenUser model tests (3), decode_token tests (7 including valid admin, valid stakeholder, expired, bad signature, missing groups, wrong audience, wrong issuer), make_anonymous_admin tests (2). Total: ~12 tests.test_permissions.py: Role checks (5), bucket access (3), allowed prefix (4), key access (5), prefix access (4), delete permission (2), bucket write permission (2), bucket filtering (3). Total: ~28 tests.test_auth_middleware.py: Auth required tests (8 including health bypass and valid token pass-through), stakeholder restriction tests (12 covering create/delete bucket, access private bucket, delete object, batch delete, cross-prefix access for get/list/upload/presign-get/presign-put/multipart-init/multipart-abort). Total: ~20 tests.BLOCKERS
None found. All blocker criteria pass:
auth.py, permissions inpermissions.py, and injected via a singleget_current_userdependency. No duplicated auth logic.NITS
Content-Disposition header injection (
objects.pyline withf'attachment; filename="{key.split("/")[-1]}"'): A key containing a double-quote character would break the header. This is a pre-existing issue already tracked as issue #3. Not introduced by this PR, not blocking.Module-level config evaluation (
auth.py):AUTH_DISABLED,KEYCLOAK_URL,KEYCLOAK_REALM,KEYCLOAK_CLIENT_IDare evaluated at import time fromos.environ. This means they cannot be changed after the module is imported without patching the module attribute (which is exactly what the test fixture does viamonkeypatch.setattr). This is a common FastAPI pattern and works correctly here, but worth noting for future refactoring if dynamic config is needed.Multi-group determinism (
permissions.pyget_allowed_prefix): When a stakeholder belongs to multiple groups, the function sorts prefixes alphabetically and takes the first. This is deterministic but means a user in both/westsideand/eastsidecan only accessassets/eastside/(alphabetically first), not both. The docstring documents this behavior. If multi-group access is needed later, this will need to change. Not a blocker for current requirements.RSA key generation duplicated across test files: Both
test_auth.pyandtest_auth_middleware.pyindependently generate RSA key pairs and define_make_token()/_mock_jwk_client()helpers. These could be extracted to a shared test fixture inconftest.py. Non-blocking since the tests are correct as-is.SOP COMPLIANCE
4-keycloak-oidc-auth-middlewarereferences issue #4plan-minio-mobilereferencedPROCESS OBSERVATIONS
Deployment frequency: This is a clean, well-scoped feature PR. The auth middleware is additive --
AUTH_DISABLED=truepreserves backward compatibility for local dev and existing integration tests. Low change failure risk.Change failure risk: The
_disable_authconftest fixture isautouse=True, which means all existing integration tests automatically bypass auth. This is correct for the integration tests that hit real MinIO, but means those tests do not exercise the auth path. The auth path is separately tested viatest_auth_middleware.pywith mock MinIO. This is an acceptable trade-off.Documentation:
CLAUDE.mdupdated with auth configuration.pyproject.tomlupdated with new dependencies (PyJWT[crypto]). No gaps.VERDICT: APPROVED