Strengthen TRE API authentication and clean up codebase#4989
Conversation
…256_algorithm_only
There was a problem hiding this comment.
Pull request overview
This PR refactors TRE API authentication by introducing a new layered api_app/auth/ package (token validation, typed user model, RBAC dependency factories) and removes the unused AccessService abstraction, while updating API routes and tests to use the new RBAC dependencies.
Changes:
- Added
api_app/auth/withTokenValidator(JWKS viaPyJWKClient), typed auth exceptions, an immutableAuthenticatedUser, and composable RBAC dependency factories. - Removed
services/access_service.pyand updated legacy AAD auth/service plumbing to useAzureADAuthorizationdirectly for Graph calls (viaget_aad_service()). - Migrated many route-level dependencies and tests from legacy
services.authenticationdependencies toauth.rbacdependencies.
Reviewed changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| CHANGELOG.md | Documents the auth refactor and AccessService removal. |
| api_app/tests_ma/test_services/test_aad_access_service.py | Updates imports after moving exceptions into aad_authentication.py. |
| api_app/tests_ma/test_db/test_repositories/test_airlock_request_repository.py | Updates patches to get_aad_service in repository tests. |
| api_app/tests_ma/test_api/test_routes/test_workspaces.py | Switches route auth overrides to new auth.rbac dependencies. |
| api_app/tests_ma/test_api/test_routes/test_workspace_users.py | Switches route auth overrides to new auth.rbac dependencies. |
| api_app/tests_ma/test_api/test_routes/test_workspace_templates.py | Updates admin/user dependency overrides to auth.rbac. |
| api_app/tests_ma/test_api/test_routes/test_workspace_service_templates.py | Updates admin/user dependency overrides to auth.rbac. |
| api_app/tests_ma/test_api/test_routes/test_user_resource_templates.py | Updates admin/user dependency overrides to auth.rbac. |
| api_app/tests_ma/test_api/test_routes/test_shared_services.py | Updates admin/user dependency overrides to auth.rbac. |
| api_app/tests_ma/test_api/test_routes/test_shared_service_templates.py | Updates admin/user dependency overrides to auth.rbac. |
| api_app/tests_ma/test_api/test_routes/test_requests.py | Updates route auth overrides to auth.rbac. |
| api_app/tests_ma/test_api/test_routes/test_migrations.py | Updates route auth overrides to auth.rbac. |
| api_app/tests_ma/test_api/test_routes/test_api_access.py | Updates access-control tests to override auth.rbac dependencies. |
| api_app/tests_ma/test_api/test_routes/test_airlock.py | Updates route auth overrides to auth.rbac and adjusts role param generation. |
| api_app/tests_ma/test_api/conftest.py | Reworks global auth patching for new validator/dependency structure. |
| api_app/tests_ma/auth/test_token_validator.py | Adds unit tests for TokenValidator. |
| api_app/tests_ma/auth/test_rbac.py | Adds unit tests for new RBAC dependency factories and AuthenticatedUser helpers. |
| api_app/tests_ma/auth/init.py | Adds auth tests package marker. |
| api_app/services/authentication.py | Removes get_access_service; adds get_aad_service and updates extract_auth_information. |
| api_app/services/airlock.py | Replaces get_access_service usage with get_aad_service. |
| api_app/services/access_service.py | Removes unused AccessService abstraction and its exceptions. |
| api_app/services/aad_authentication.py | Refactors JWT validation to use new auth.registry validators and keeps Graph role logic. |
| api_app/db/repositories/airlock_requests.py | Uses get_aad_service for role assignment lookups. |
| api_app/auth/token_validator.py | Implements TokenValidator backed by PyJWKClient. |
| api_app/auth/registry.py | Adds cached validator registry (get_core_validator, get_workspace_validator). |
| api_app/auth/rbac.py | Adds require_roles / require_workspace_roles factories and prebuilt role dependencies. |
| api_app/auth/models.py | Introduces AuthenticatedUser and role enums. |
| api_app/auth/exceptions.py | Adds typed auth exception hierarchy. |
| api_app/auth/dependencies.py | Adds FastAPI deps for core/workspace token validation and HTTP exception mapping. |
| api_app/auth/init.py | Adds auth package marker. |
| api_app/api/routes/workspaces.py | Migrates router dependencies to auth.rbac; updates AAD service usage for workspace filtering. |
| api_app/api/routes/workspace_users.py | Migrates router dependencies to auth.rbac and get_aad_service. |
| api_app/api/routes/workspace_templates.py | Migrates admin router dependencies to auth.rbac. |
| api_app/api/routes/workspace_service_templates.py | Migrates router dependencies to auth.rbac. |
| api_app/api/routes/user_resource_templates.py | Migrates router dependencies to auth.rbac. |
| api_app/api/routes/shared_services.py | Migrates router dependencies to auth.rbac. |
| api_app/api/routes/shared_service_templates.py | Migrates router dependencies to auth.rbac. |
| api_app/api/routes/resource_helpers.py | Switches role assignment lookup to get_aad_service and removes dead helper. |
| api_app/api/routes/requests.py | Migrates router dependencies to auth.rbac. |
| api_app/api/routes/operations.py | Migrates router dependencies to auth.rbac. |
| api_app/api/routes/migrations.py | Migrates router dependencies to auth.rbac. |
| api_app/api/routes/costs.py | Migrates router dependencies to auth.rbac. |
| api_app/api/routes/airlock.py | Migrates router dependencies to auth.rbac. |
|
@copilot address PR review comments (and replies) and fix merge conflicts. |
…ck, role normalization
…e-api-auth # Conflicts: # CHANGELOG.md
Unit Test Results711 tests 711 ✅ 10s ⏱️ Results for commit 07e77ce. ♻️ This comment has been updated with latest results. |
- Remove unused get_workspace_authenticated_user (had broken Depends(lambda: None)) - Remove dead AzureADAuthorization.__call__ and helpers; make it a plain Graph service class - Share a single PyJWKClient across token validators via registry
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 43 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
CHANGELOG.md:12
- Changelog entries in this file consistently include an issue/PR reference link (e.g. "(#4950)"). This new entry is missing a reference, which makes it harder to trace the change back to the discussion/PR.
* Strengthen TRE API authentication: introduce layered `auth/` package with typed exceptions, `PyJWKClient`-backed token validation with issuer checking, immutable `AuthenticatedUser` model, and composable RBAC factories; remove the `AccessService` abstraction that is no longer needed now that Entra ID is the only auth provider.
- Store AuthenticatedUser.roles as an immutable tuple so roles cannot be escalated via in-place mutation (frozen only blocked reassignment) - Ignore E231 in flake8 config (Python 3.12 f-string tokenisation false positives) - Fix E306 nested-def blank line in test_migrations.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 45 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
CHANGELOG.md:12
- The new CHANGELOG entry does not include an issue/PR reference link, while other entries in this section consistently include one (e.g. "(#4920)"). The contribution guidelines in this repo require changelog entries to include an issue and/or PR reference.
* Strengthen TRE API authentication: introduce layered `auth/` package with typed exceptions, `PyJWKClient`-backed token validation with issuer checking, immutable `AuthenticatedUser` model, and composable RBAC factories; remove the `AccessService` abstraction that is no longer needed now that Entra ID is the only auth provider.
api_app/auth/dependencies.py:17
- 401 responses raised from the auth dependency currently omit the
WWW-Authenticate: Bearerheader. The previous auth layer included this header on 401s, and omitting it can break standards-compliant clients and auth middleware that relies on it.
def _to_http_exception(exc: AuthError) -> HTTPException:
if isinstance(exc, TokenExpired):
return HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail=strings.EXPIRED_SIGNATURE,
- Add PR reference to CHANGELOG auth entry - Add WWW-Authenticate: Bearer header to 401 responses in auth dependencies - Guard get_required_roles against endpoint.__defaults__ being None
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
/test-extended 4b63744 |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/30081182980 (with refid (in response to this comment from @marrobi) |
There was a problem hiding this comment.
Looks good, Would suggest also fixing #4797 so #4796 can be closed.
Issue
While PR #4989 successfully migrated the dependency references from the old services.authentication singletons to the new auth.rbac package
(require_tre_user_or_tre_admin), it preserved the duplicate dependency structure:
- Router level: APIRouter(dependencies=[Depends(...)]) applies the check to all routes on shared_service_templates_core_router.
- Route level (Line 25): @router.get(..., dependencies=[Depends(...)]) declares the exact same check again.
- Parameter level (Line 20): Endpoint functions like get_shared_service_templates(..., user=Depends(...)) inject the dependency a third time.
Similar duplicate declarations remain across other route modules as well, including:
• api_app/api/routes/shared_service_templates.py (Line 25)
• api_app/api/routes/user_resource_templates.py (Lines 21 & 27)
• api_app/api/routes/workspace_service_templates.py (Lines 19 & 25)
• api_app/api/routes/shared_services.py (Lines 35 & 45)
──────
Suggest keep those in seperate PR as if any issues introduced can track it easier. This PR is large already. |
…r level auth and remove duplicate route decorator auth
|
@copilot resolve the merge conflicts in this pull request |
The TRE API auth layer was a single god-object (
AzureADAuthorization) mixing JWT validation, JWKS key management, Graph API calls, and role checking, with an unusedAccessServiceabstraction sitting on top.New
api_app/auth/packageIntroduces a clean, layered auth package:
models.py—AuthenticatedUser(frozen Pydantic;rolesis an immutable tuple so roles cannot be reassigned or mutated in place),TRERole/WorkspaceAccessRoleStrEnums,has_any_role()/is_tre_admin()helpersexceptions.py— Typed exception hierarchy (TokenExpired,TokenSignatureInvalid,TokenInvalid,InsufficientPermissions,WorkspaceNotFound); no silent swallowingtoken_validator.py—TokenValidatorusingPyJWKClientfor automatic JWKS key management; validates issuer + audience + expiry + signatureregistry.py—@lru_cachesingletons:get_core_validator()for the TRE core app,get_workspace_validator(client_id)per workspace, all sharing a singlePyJWKClientdependencies.py—require_bearer_credentials(HTTPBearer withauto_error=False→ consistent 401 +WWW-Authenticate: Beareron missing/malformed headers) andget_authenticated_userrbac.py—require_roles()/require_workspace_roles()dependency factories + pre-built named checks. Workspace-scoped checks validate against the workspace app registration first and only fall back to the core app registration for TREAdmin (no cross-audience elevation for other core tokens)Route migration to the new auth layer
auth.rbacdependencies (require_tre_admin,require_workspace_owner, etc.) instead of the legacyservices/authentication.pysingletonsget_current_*dependencies have been removedRemoval of
AccessServiceabstractionservices/access_service.pydeleted — Entra ID is the only auth provider; the abstraction was never variedAzureADAuthorizationis now a plain Microsoft Graph service wrapper (noOAuth2AuthorizationCodeBearerbase class and no__call__); it is used only for Graph API calls (role-assignment lookups) viaget_aad_service()AuthConfigValidationError/UserRoleAssignmentErrormoved intoaad_authentication.pyget_access_service()replaced byget_aad_service()across routes, airlock service, and repositoriesget_app_user_roles_assignments_emails(unreachable) removed fromresource_helpers.pyTests
TokenValidator, RBAC/AuthenticatedUserhelpers, the workspace-validator + TREAdmin-only fallback paths, model immutability, and missing-credential handlingapi_appunit test suite passes (706 tests)conftest.pyauth fixtures updated for the new validator/dependency structureNotes
api_app/_version.pybumped to0.26.0