remove duplicate dependencies in API#4796
Conversation
Unit Test Results711 tests 711 ✅ 8s ⏱️ Results for commit 07e77ce. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR resolves duplicate authentication dependency declarations in FastAPI endpoints by removing redundant dependency specifications at the endpoint level when they're already declared at the router level. According to FastAPI behavior, dependencies declared both at router and endpoint levels are executed twice, causing unnecessary duplicate authentication checks.
Key Changes:
- Removed duplicate auth dependencies from endpoint decorators across multiple route files
- Incremented API version from 0.25.7 to 0.25.8 (PATCH version)
- Updated CHANGELOG.md with bug fix entry
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| api_app/api/routes/workspaces.py | Removed duplicate get_current_workspace_owner_or_researcher_user_or_airlock_manager from two GET endpoints that already inherit this dependency from the router |
| api_app/api/routes/workspace_service_templates.py | Removed duplicate get_current_tre_user_or_tre_admin from two GET endpoints that already inherit this dependency from the router |
| api_app/api/routes/user_resource_templates.py | Removed duplicate get_current_tre_user_or_tre_admin from two GET endpoints that already inherit this dependency from the router |
| api_app/api/routes/shared_services.py | Removed duplicate get_current_tre_user_or_tre_admin from two GET endpoints while preserving it in function signatures where needed for business logic |
| api_app/api/routes/shared_service_templates.py | Removed duplicate get_current_tre_user_or_tre_admin from one GET endpoint that already inherits this dependency from the router |
| api_app/_version.py | Incremented API version from 0.25.7 to 0.25.8 following semantic versioning for bug fixes |
| CHANGELOG.md | Added bug fix entry documenting the removal of duplicate auth dependencies |
| * Add timeouts to Graph requests in API ([#4723](https://github.com/microsoft/AzureTRE/issues/4723)) | ||
| * Fix missing metastoreDomains for Databricks, which caused metastore outages for some domains ([#4779](https://github.com/microsoft/AzureTRE/issues/4779)) | ||
| * Fix cost display duplication when user resource is deleted - UI incorrectly reused cost data for remaining resources ([#4783](https://github.com/microsoft/AzureTRE/issues/4783)) | ||
| * Remove duplicate auth dependencies in API ([#4796](https://github.com/microsoft/AzureTRE/pull/4796)) |
There was a problem hiding this comment.
The reference should point to issue #4797 instead of PR #4796. The PR description states "Resolves #4797", so the changelog entry should reference the issue number to maintain consistency with the format used in other entries.
| * Remove duplicate auth dependencies in API ([#4796](https://github.com/microsoft/AzureTRE/pull/4796)) | |
| * Remove duplicate auth dependencies in API ([#4797](https://github.com/microsoft/AzureTRE/issues/4797)) |
Analysis: Does Removing the Duplicate dependency from the Route Add/Remove Permissions?Short answer: No, removing these dependencies does NOT change the effective permissions. The auth checks are still applied — they were just duplicated, causing them to run twice. Here's the file-by-file breakdown: 1.
|
| Endpoint | Dependency Removed from Route | Router-Level Dependency | Still Protected? |
|---|---|---|---|
GET /shared-service-templates/{name} |
Depends(get_current_tre_user_or_tre_admin) |
shared_service_templates_core_router has dependencies=[Depends(get_current_tre_user_or_tre_admin)] |
✅ Yes — same dependency on router |
No permission change. The router already enforces get_current_tre_user_or_tre_admin.
2. shared_services.py
| Endpoint | Dependency Removed from Route | Router-Level Dependency | Still Protected? |
|---|---|---|---|
GET /shared-services |
Depends(get_current_tre_user_or_tre_admin) |
shared_services_router has this at router level + user=Depends(get_current_tre_user_or_tre_admin) as a function param |
✅ Yes — router-level + function param |
GET /shared-services/{id} |
Depends(get_current_tre_user_or_tre_admin) |
Same router-level dep + user=Depends(get_current_tre_user_or_tre_admin) as function param |
✅ Yes — router-level + function param |
No permission change. Both endpoints still have the auth dependency via the router and as a function parameter (since user=Depends(get_current_tre_user_or_tre_admin) remains in the function signature).
3. user_resource_templates.py
| Endpoint | Dependency Removed from Route | Router-Level Dependency | Still Protected? |
|---|---|---|---|
GET .../user-resource-templates |
Depends(get_current_tre_user_or_tre_admin) |
user_resource_templates_core_router = APIRouter(dependencies=[Depends(get_current_tre_user_or_tre_admin)]) |
✅ Yes |
GET .../user-resource-templates/{name} |
Depends(get_current_tre_user_or_tre_admin) |
Same router-level dep | ✅ Yes |
No permission change.
4. workspace_service_templates.py
| Endpoint | Dependency Removed from Route | Router-Level Dependency | Still Protected? |
|---|---|---|---|
GET /workspace-service-templates |
Depends(get_current_tre_user_or_tre_admin) |
workspace_service_templates_core_router = APIRouter(dependencies=[Depends(get_current_tre_user_or_tre_admin)]) |
✅ Yes |
GET /workspace-service-templates/{name} |
Depends(get_current_tre_user_or_tre_admin) |
Same router-level dep | ✅ Yes |
No permission change.
5. workspaces.py
| Endpoint | Dependency Removed from Route | Router-Level Dependency | Still Protected? |
|---|---|---|---|
GET /workspaces/{id}/workspace-services |
Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager) |
workspace_services_workspace_router = APIRouter(dependencies=[Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager)]) |
✅ Yes |
GET /workspaces/{id}/workspace-services/{service_id} |
Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager) |
Same router-level dep | ✅ Yes |
No permission change. Note that Depends(get_workspace_by_id_from_path) is kept in the route-level dependencies for the second endpoint because it's a different dependency (not an auth duplicate — it fetches the workspace resource).
Summary
Every dependency that was removed from an individual route was already present on the parent APIRouter. In FastAPI, router-level dependencies are automatically applied to all routes under that router. The PR correctly identifies that having the same Depends(...) at both the router and route level causes the auth check to execute twice per request, which is redundant.
No permissions are added or removed by this PR — the effective authorization behavior remains identical. The only change is that auth checks now run once instead of twice per request.
|
@copilot please fix the merge conflicts in this pull request. |
…256_algorithm_only
…ck, role normalization
…e-api-auth # Conflicts: # CHANGELOG.md
- 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
- 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
- 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
…ng creds - require_workspace_roles core fallback now only grants access to TREAdmin; any other valid core token is rejected with 401 (no cross-audience elevation) - HTTPBearer(auto_error=False) + require_bearer_credentials returns 401 + WWW-Authenticate for missing/malformed Authorization headers (was FastAPI 403) - Update workspace-roles tests to exercise the workspace-validator path and the hardened fallback; add missing-credentials tests
The migration inadvertently granted TREAdmin access to ALL workspace-scoped endpoints (require_workspace_roles always allowed TREAdmin). Pre-PR, only endpoints explicitly using the *_or_tre_admin dependencies allowed TREAdmin; owner/researcher/airlock-manager-only endpoints did not. - Make TREAdmin access opt-in via require_workspace_roles(..., allow_tre_admin) - When allow_tre_admin=False, no core-token fallback occurs (wrong-audience token -> 401), preserving separation of platform admin vs workspace access - Add require_workspace_owner_or_tre_admin and require_workspace_owner_or_researcher_or_airlock_manager_or_tre_admin, and re-map the exact endpoints that used *_or_tre_admin in main (costs, workspace_users router, workspaces shared router + operations/history + template listing) - Update route tests' dependency overrides and rbac tests accordingly
- token_validator: add require=[exp,iss,aud] so tokens missing these claims are rejected (PyJWT only validates a claim when present; a token without exp was otherwise accepted indefinitely) - test_rbac: use asyncio.run() instead of get_event_loop().run_until_complete() - add tests for the required-claim behavior
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 46 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (3)
api_app/api/routes/workspaces.py:245
- This endpoint declares require_workspace_owner twice (once via dependencies=[Depends(require_workspace_owner)] on the decorator and again as user=Depends(require_workspace_owner)). This can lead to duplicated auth evaluation; drop the decorator-level dependency and keep the injected user parameter (same pattern applies to patch/delete/invoke workspace-service endpoints below).
@workspace_services_workspace_router.post("/workspaces/{workspace_id}/workspace-services", status_code=status.HTTP_202_ACCEPTED, response_model=OperationInResponse, name=strings.API_CREATE_WORKSPACE_SERVICE, dependencies=[Depends(require_workspace_owner)])
async def create_workspace_service(response: Response, workspace_service_input: WorkspaceServiceInCreate, user=Depends(require_workspace_owner), workspace_service_repo=Depends(get_repository(WorkspaceServiceRepository)), workspace_repo=Depends(get_repository(WorkspaceRepository)), resource_template_repo=Depends(get_repository(ResourceTemplateRepository)), operations_repo=Depends(get_repository(OperationRepository)), resource_history_repo=Depends(get_repository(ResourceHistoryRepository)), workspace=Depends(get_deployed_workspace_by_id_from_path)) -> OperationInResponse:
api_app/api/routes/shared_services.py:85
- This route lists both require_tre_admin and get_shared_service_by_id_from_path in the decorator dependencies, but it also injects both as parameters (user=Depends(require_tre_admin), shared_service=Depends(get_shared_service_by_id_from_path)). That duplication can execute dependencies twice. Remove the decorator-level dependencies and rely on the parameters.
@shared_services_router.patch("/shared-services/{shared_service_id}",
status_code=status.HTTP_202_ACCEPTED,
response_model=OperationInResponse,
name=strings.API_UPDATE_SHARED_SERVICE,
dependencies=[Depends(require_tre_admin), Depends(get_shared_service_by_id_from_path)])
CHANGELOG.md:12
- The PR metadata says this change resolves #4797 (duplicate auth dependencies causing checks to run twice), but the new changelog entry references PR #4989 and describes a broader auth refactor. If this PR is specifically fixing #4797, the changelog item should reference #4797 (and ideally describe the duplication fix) so release notes align with the actual change/issue being resolved.
* 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. ([#4989](https://github.com/microsoft/AzureTRE/pull/4989))
|
|
||
| @workspaces_core_router.post("/workspaces", status_code=status.HTTP_202_ACCEPTED, response_model=OperationInResponse, name=strings.API_CREATE_WORKSPACE, dependencies=[Depends(get_current_admin_user)]) | ||
| async def create_workspace(workspace_create: WorkspaceInCreate, response: Response, user=Depends(get_current_admin_user), workspace_repo=Depends(get_repository(WorkspaceRepository)), resource_template_repo=Depends(get_repository(ResourceTemplateRepository)), operations_repo=Depends(get_repository(OperationRepository)), resource_history_repo=Depends(get_repository(ResourceHistoryRepository))) -> OperationInResponse: | ||
| @workspaces_core_router.post("/workspaces", status_code=status.HTTP_202_ACCEPTED, response_model=OperationInResponse, name=strings.API_CREATE_WORKSPACE, dependencies=[Depends(require_tre_admin)]) |
|
|
||
|
|
||
| shared_services_router = APIRouter(dependencies=[Depends(get_current_tre_user_or_tre_admin)]) | ||
| shared_services_router = APIRouter(dependencies=[Depends(require_tre_user_or_admin)]) |
| from auth.rbac import require_tre_user_or_admin | ||
|
|
||
| router = APIRouter(dependencies=[Depends(get_current_tre_user_or_tre_admin)]) | ||
| router = APIRouter(dependencies=[Depends(require_tre_user_or_admin)]) |
|
|
||
|
|
||
| operations_router = APIRouter(dependencies=[Depends(get_current_tre_user_or_tre_admin)]) | ||
| operations_router = APIRouter(dependencies=[Depends(require_tre_user_or_admin)]) |
| workspace_templates_admin_router = APIRouter(dependencies=[Depends(require_tre_admin)]) | ||
|
|
||
|
|
||
| @workspace_templates_admin_router.get("/workspace-templates", response_model=ResourceTemplateInformationInList, name=strings.API_GET_WORKSPACE_TEMPLATES) | ||
| async def get_workspace_templates(authorized_only: bool = False, template_repo=Depends(get_repository(ResourceTemplateRepository)), user=Depends(get_current_admin_user)) -> ResourceTemplateInformationInList: | ||
| async def get_workspace_templates(authorized_only: bool = False, template_repo=Depends(get_repository(ResourceTemplateRepository)), user=Depends(require_tre_admin)) -> ResourceTemplateInformationInList: |
| shared_service_templates_core_router = APIRouter(dependencies=[Depends(require_tre_user_or_admin)]) | ||
|
|
||
|
|
||
| @shared_service_templates_core_router.get("/shared-service-templates", response_model=ResourceTemplateInformationInList, name=strings.API_GET_SHARED_SERVICE_TEMPLATES) | ||
| async def get_shared_service_templates(authorized_only: bool = False, template_repo=Depends(get_repository(ResourceTemplateRepository)), user=Depends(get_current_tre_user_or_tre_admin)) -> ResourceTemplateInformationInList: | ||
| async def get_shared_service_templates(authorized_only: bool = False, template_repo=Depends(get_repository(ResourceTemplateRepository)), user=Depends(require_tre_user_or_admin)) -> ResourceTemplateInformationInList: |
| dependencies = list(filter(lambda x: hasattr(x.dependency, 'require_one_of_roles'), defaults)) | ||
| if dependencies: | ||
| return dependencies[0].dependency.require_one_of_roles | ||
| # New-style deps: check for _role_names attribute on the closure | ||
| dependencies = list(filter(lambda x: hasattr(x.dependency, '_role_names'), defaults)) |
…r level auth and remove duplicate route decorator auth
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 46 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (8)
api_app/api/routes/airlock.py:38
- This route declares
require_workspace_owner_or_researchertwice (once independencies=[...]and again asuser=Depends(...)), which can cause the auth check to run multiple times.
@airlock_workspace_router.post("/workspaces/{workspace_id}/requests", status_code=status_code.HTTP_201_CREATED,
response_model=AirlockRequestWithAllowedUserActions, name=strings.API_CREATE_AIRLOCK_REQUEST,
dependencies=[Depends(require_workspace_owner_or_researcher), Depends(get_workspace_by_id_from_path)])
async def create_draft_request(airlock_request_input: AirlockRequestInCreate, user=Depends(require_workspace_owner_or_researcher),
api_app/api/routes/airlock.py:81
- This endpoint repeats
require_workspace_owner_or_researcher_or_airlock_managerin both the decoratordependencies=[...]and theuser=Depends(...)parameter (and also via the router), which can lead to multiple auth evaluations per request.
@airlock_workspace_router.get("/workspaces/{workspace_id}/requests/{airlock_request_id}", status_code=status_code.HTTP_200_OK,
response_model=AirlockRequestWithAllowedUserActions, name=strings.API_GET_AIRLOCK_REQUEST,
dependencies=[Depends(require_workspace_owner_or_researcher_or_airlock_manager), Depends(get_workspace_by_id_from_path)])
async def retrieve_airlock_request_by_id(airlock_request=Depends(get_airlock_request_by_id_from_path),
airlock_request_repo=Depends(get_repository(AirlockRequestRepository)),
user=Depends(require_workspace_owner_or_researcher_or_airlock_manager)) -> AirlockRequestWithAllowedUserActions:
allowed_actions = get_allowed_actions(airlock_request, user, airlock_request_repo)
api_app/api/routes/airlock.py:90
- This route declares
require_workspace_owner_or_researchertwice (decoratordependencies=[...]anduser=Depends(...)), which can cause duplicate auth checks.
@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/submit", status_code=status_code.HTTP_200_OK,
response_model=AirlockRequestWithAllowedUserActions, name=strings.API_SUBMIT_AIRLOCK_REQUEST,
dependencies=[Depends(require_workspace_owner_or_researcher), Depends(get_workspace_by_id_from_path)])
async def create_submit_request(airlock_request=Depends(get_airlock_request_by_id_from_path),
user=Depends(require_workspace_owner_or_researcher),
airlock_request_repo=Depends(get_repository(AirlockRequestRepository)),
api_app/api/routes/airlock.py:103
- This route declares
require_workspace_owner_or_researchertwice (decoratordependencies=[...]anduser=Depends(...)), which can cause duplicate auth checks.
@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/cancel", status_code=status_code.HTTP_200_OK,
response_model=AirlockRequestWithAllowedUserActions, name=strings.API_CANCEL_AIRLOCK_REQUEST,
dependencies=[Depends(require_workspace_owner_or_researcher), Depends(get_workspace_by_id_from_path)])
async def create_cancel_request(airlock_request=Depends(get_airlock_request_by_id_from_path),
user=Depends(require_workspace_owner_or_researcher),
workspace=Depends(get_workspace_by_id_from_path),
api_app/api/routes/airlock.py:121
- This route declares
require_airlock_managertwice (decoratordependencies=[...]anduser=Depends(...)), which can cause duplicate auth checks.
@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/revoke", status_code=status_code.HTTP_200_OK,
response_model=AirlockRequestWithAllowedUserActions, name=strings.API_REVOKE_AIRLOCK_REQUEST,
dependencies=[Depends(require_airlock_manager), Depends(get_workspace_by_id_from_path)])
async def create_revoke_request(revoke_input: AirlockRevokeInCreate,
airlock_request=Depends(get_airlock_request_by_id_from_path),
user=Depends(require_airlock_manager),
workspace=Depends(get_workspace_by_id_from_path),
api_app/api/routes/airlock.py:136
- This route declares
require_airlock_managertwice (decoratordependencies=[...]anduser=Depends(...)), which can cause duplicate auth checks.
@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/review-user-resource",
status_code=status_code.HTTP_202_ACCEPTED, response_model=AirlockRequestAndOperationInResponse,
name=strings.API_CREATE_AIRLOCK_REVIEW_USER_RESOURCE,
dependencies=[Depends(require_airlock_manager), Depends(get_workspace_by_id_from_path)])
async def create_review_user_resource(
response: Response,
airlock_request=Depends(get_airlock_request_by_id_from_path),
user=Depends(require_airlock_manager),
workspace=Depends(get_deployed_workspace_by_id_from_path),
api_app/api/routes/airlock.py:174
- This route declares
require_airlock_managertwice (decoratordependencies=[...]anduser=Depends(...)), which can cause duplicate auth checks.
@airlock_workspace_router.post("/workspaces/{workspace_id}/requests/{airlock_request_id}/review",
status_code=status_code.HTTP_200_OK, response_model=AirlockRequestWithAllowedUserActions,
name=strings.API_REVIEW_AIRLOCK_REQUEST, dependencies=[Depends(require_airlock_manager),
Depends(get_workspace_by_id_from_path)])
async def create_airlock_review(
airlock_review_input: AirlockReviewInCreate,
airlock_request=Depends(get_airlock_request_by_id_from_path),
user=Depends(require_airlock_manager),
workspace=Depends(get_deployed_workspace_by_id_from_path),
api_app/api/routes/airlock.py:196
- This route declares
require_workspace_owner_or_researcher_or_airlock_managertwice (decoratordependencies=[...]anduser=Depends(...)), which can cause duplicate auth checks.
@airlock_workspace_router.get("/workspaces/{workspace_id}/requests/{airlock_request_id}/link",
status_code=status_code.HTTP_200_OK, response_model=AirlockRequestTokenInResponse,
name=strings.API_AIRLOCK_REQUEST_LINK,
dependencies=[Depends(require_workspace_owner_or_researcher_or_airlock_manager)])
async def get_airlock_container_link_method(workspace=Depends(get_deployed_workspace_by_id_from_path),
airlock_request=Depends(get_airlock_request_by_id_from_path),
user=Depends(require_workspace_owner_or_researcher_or_airlock_manager)) -> AirlockRequestTokenInResponse:
| @migrations_core_router.post("/migrations", | ||
| status_code=status.HTTP_202_ACCEPTED, | ||
| name=strings.API_MIGRATE_DATABASE, | ||
| response_model=MigrationOutList, | ||
| dependencies=[Depends(get_current_admin_user)]) | ||
| dependencies=[Depends(require_tre_admin)]) |
| from services.logging import logger | ||
|
|
||
| airlock_workspace_router = APIRouter(dependencies=[Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager)]) | ||
| airlock_workspace_router = APIRouter(dependencies=[Depends(require_workspace_owner_or_researcher_or_airlock_manager)]) |
| * Add Windows Server 2025 image support to Guacamole. ([#4890](https://github.com/microsoft/AzureTRE/issues/4890)) | ||
| * Add support for setting resource processor VMSS SKU via environment variables ([#4936](https://github.com/microsoft/AzureTRE/issues/4936)) | ||
| * Exclude recovery service vaults from e2e tests ([#4920](https://github.com/microsoft/AzureTRE/issues/4920)) | ||
| * 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. ([#4989](https://github.com/microsoft/AzureTRE/pull/4989)) |
Resolves #4797
What is being addressed
Dependencies are duplicated which would cause auth checks to run twice
How is this addressed