From c62bd6d3ba82656c4510fb082103d70ca398a37f Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:00:23 +1000 Subject: [PATCH 01/21] Define admin roles in config --- auth/config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/auth/config.py b/auth/config.py index f9682bca..26f6e50b 100644 --- a/auth/config.py +++ b/auth/config.py @@ -11,6 +11,7 @@ class Settings(BaseSettings): auth0_audience: str jwt_secret_key: str auth0_algorithms: list[str] = ["RS256"] + admin_roles: list[str] = [] # Note we process this separately in app startup as it needs # to be available before the app starts cors_allowed_origins: str From 8c9eaee2d7afe354869b84153257b3b17a000f7a Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:03:21 +1000 Subject: [PATCH 02/21] Update .env example with admin_roles --- .env.example | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.env.example b/.env.example index e972e7b4..8bf1758a 100644 --- a/.env.example +++ b/.env.example @@ -6,6 +6,8 @@ AUTH0_AUDIENCE=https://audience.com/api # JWT secret key: used to provide some protection around registration # Generate with: python -c "import secrets; print(secrets.token_urlsafe(32))" JWT_SECRET_KEY=secret-key +# Note the list syntax pydantic-settings uses +ADMIN_ROLES='["Admin", "GalaxyAdmin"]' # Comma-separated list of allowed origins. Note we # don't process this with pydantic-settings as it needs # to be used before the FastAPI app loads From cc2a1518960a95a5fe84c023b8940311079612a0 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:08:39 +1000 Subject: [PATCH 03/21] Add a user_is_admin dependency for checking admin roles --- auth/validator.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/auth/validator.py b/auth/validator.py index 46ebacff..7bad560b 100644 --- a/auth/validator.py +++ b/auth/validator.py @@ -1,10 +1,12 @@ +from typing import Annotated + import httpx -from fastapi import Depends, HTTPException +from fastapi import Depends, HTTPException, status from fastapi.security import OAuth2PasswordBearer from jose import jwk, jwt from jose.exceptions import JWTError -from auth.config import Settings +from auth.config import Settings, get_settings from schemas.tokens import AccessTokenPayload from schemas.user import User @@ -63,6 +65,17 @@ def get_rsa_key(token: str, settings: Settings) -> jwk.RSAKey | None: # type: i return None -def get_current_user(token: str = Depends(oauth2_scheme)) -> User: - access_token = verify_jwt(token) +def get_current_user(token: str = Depends(oauth2_scheme), + settings: Settings = Depends(get_settings)) -> User: + access_token = verify_jwt(token, settings=settings) return User(access_token=access_token) + + +def user_is_admin(current_user: Annotated[User, Depends(get_current_user)], + settings: Annotated[Settings, Depends(get_settings)]) -> User: + if not current_user.is_admin(settings=settings): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="You must be an admin to access this endpoint." + ) + return current_user From 1a2d5cd4a896cb812d2dc70dd51900bae36885cd Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:09:21 +1000 Subject: [PATCH 04/21] Fix roles claim name: we now prefix with HTTPS (as recommended by Auth0) --- auth/validator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auth/validator.py b/auth/validator.py index 7bad560b..2509db0b 100644 --- a/auth/validator.py +++ b/auth/validator.py @@ -35,7 +35,7 @@ def verify_jwt(token: str, settings: Settings) -> AccessTokenPayload: except JWTError as e: raise HTTPException(status_code=401, detail=f"Invalid token: {e}") - roles_claim = "biocommons.org.au/roles" + roles_claim = "https://biocommons.org.au/roles" if roles_claim not in payload: raise HTTPException( status_code=403, detail=f"Missing required claim: {roles_claim}" From 79710737768aacc1f7c70de698d2398114ce5004 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:10:54 +1000 Subject: [PATCH 05/21] Remove old admin check --- auth/validator.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/auth/validator.py b/auth/validator.py index 2509db0b..e8a6bac6 100644 --- a/auth/validator.py +++ b/auth/validator.py @@ -41,14 +41,6 @@ def verify_jwt(token: str, settings: Settings) -> AccessTokenPayload: status_code=403, detail=f"Missing required claim: {roles_claim}" ) - roles = payload[roles_claim] - if not isinstance(roles, list) or not any( - "admin" in role.lower() for role in roles - ): - raise HTTPException( - status_code=403, detail="Access denied: Insufficient permissions" - ) - return AccessTokenPayload(**payload) From e88afdc9122dbcb1d5951babcc9433f935257232 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:11:52 +1000 Subject: [PATCH 06/21] Update AccessTokenPayload so it's easier to specify roles --- schemas/tokens.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/schemas/tokens.py b/schemas/tokens.py index a605580f..35cee689 100644 --- a/schemas/tokens.py +++ b/schemas/tokens.py @@ -1,6 +1,6 @@ from typing import Optional -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field class AccessTokenPayload(BaseModel): @@ -9,7 +9,7 @@ class AccessTokenPayload(BaseModel): """ biocommons_roles: list[str] = Field( - alias="biocommons.org.au/roles", + alias="https://biocommons.org.au/roles", description="BioCommons-specific roles assigned to the user", ) email: Optional[str] = Field(None, description="Email address") @@ -20,3 +20,6 @@ class AccessTokenPayload(BaseModel): iat: int = Field(description="Issued at time (as Unix timestamp)") azp: Optional[str] = Field(None, description="Authorized party") permissions: list[str] = Field(description="Permissions granted to the user") + + # Set populate_by_name so we can specify biocommons_roles as an argument + model_config = ConfigDict(populate_by_name=True) From 1ef747b099be769a1d95c03d866344f525d4a9f6 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:12:51 +1000 Subject: [PATCH 07/21] Update User.is_admin() to check configured roles --- schemas/user.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/schemas/user.py b/schemas/user.py index ec6434fe..68f24e3e 100644 --- a/schemas/user.py +++ b/schemas/user.py @@ -1,5 +1,7 @@ from pydantic import BaseModel +from auth.config import Settings + from .tokens import AccessTokenPayload @@ -12,13 +14,13 @@ class User(BaseModel): access_token: AccessTokenPayload - def is_admin(self) -> bool: + def is_admin(self, settings: Settings) -> bool: """ Checks if the user has an admin role. """ # TODO: Need to finalize exactly what roles make # a user an admin for role in self.access_token.biocommons_roles: - if "admin" in role.lower(): + if role in settings.admin_roles: return True return False From 243d5481f89fe67b19971acb6b92bd69370d55d2 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:40:01 +1000 Subject: [PATCH 08/21] Add a fixture for acting as an admin --- tests/conftest.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index a4689e9b..d64fb869 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,7 +2,9 @@ from fastapi.testclient import TestClient from auth.config import Settings, get_settings +from auth.validator import get_current_user from main import app +from tests.datagen import AccessTokenPayloadFactory, UserFactory @pytest.fixture @@ -34,3 +36,17 @@ def override_settings(): # Reset override app.dependency_overrides.clear() + + +@pytest.fixture +def as_admin_user(): + """ + Override the get_current_user dependency to return a User object with admin role, + so admin check will pass. + """ + def override_user(): + token = AccessTokenPayloadFactory.build(biocommons_roles=["Admin"]) + return UserFactory.build(access_token=token) + app.dependency_overrides[get_current_user] = override_user + yield + app.dependency_overrides.clear() From 7dc7578a006fe5d13f2a05fa24eeb352e7411a6d Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:40:37 +1000 Subject: [PATCH 09/21] Add admin_roles to mock settings --- tests/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/conftest.py b/tests/conftest.py index d64fb869..d75d37d1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -17,6 +17,7 @@ def mock_settings(): auth0_audience="mock-audience", jwt_secret_key="mock-secret-key", cors_allowed_origins="https://test", + admin_roles=["Admin"], auth0_algorithms=["HS256"] ) From 8800f50e772203ca8c7f7a3b3bacd260cf97f1fb Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:42:33 +1000 Subject: [PATCH 10/21] Rename client_with_settings_override: too clunky for something we use everywhere --- tests/conftest.py | 6 ++++-- tests/test_bpa_register.py | 36 ++++++++++++++++++------------------ tests/test_galaxy.py | 14 +++++++------- tests/test_main.py | 4 ++-- tests/test_user.py | 32 ++++++++++++++++---------------- 5 files changed, 47 insertions(+), 45 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index d75d37d1..ef92d887 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -21,9 +21,11 @@ def mock_settings(): auth0_algorithms=["HS256"] ) - @pytest.fixture -def client_with_settings_override(mock_settings): +def test_client(mock_settings): + """ + Override the get_settings dependency to return a mocked Settings object. + """ # Define override def override_settings(): return mock_settings diff --git a/tests/test_bpa_register.py b/tests/test_bpa_register.py index a494cf5a..08c15ad1 100644 --- a/tests/test_bpa_register.py +++ b/tests/test_bpa_register.py @@ -31,7 +31,7 @@ def mock_auth_token(mocker): def test_successful_registration( - client_with_settings_override, mock_auth_token, mocker, valid_registration_data + test_client, mock_auth_token, mocker, valid_registration_data ): """Test successful user registration with BPA service""" mock_response = MagicMock() @@ -40,7 +40,7 @@ def test_successful_registration( mock_post = mocker.patch("httpx.AsyncClient.post", return_value=mock_response) - response = client_with_settings_override.post( + response = test_client.post( "/bpa/register", json=valid_registration_data ) @@ -66,14 +66,14 @@ def test_successful_registration( def test_registration_duplicate_user( - client_with_settings_override, mock_auth_token, mocker, valid_registration_data + test_client, mock_auth_token, mocker, valid_registration_data ): """Test registration with duplicate user""" mock_response = MagicMock() mock_response.status_code = 409 mocker.patch("httpx.AsyncClient.post", return_value=mock_response) - response = client_with_settings_override.post( + response = test_client.post( "/bpa/register", json=valid_registration_data ) @@ -82,7 +82,7 @@ def test_registration_duplicate_user( def test_registration_auth0_error( - client_with_settings_override, mock_auth_token, mocker, valid_registration_data + test_client, mock_auth_token, mocker, valid_registration_data ): """Test registration with Auth0 API error""" mock_response = MagicMock() @@ -90,7 +90,7 @@ def test_registration_auth0_error( mock_response.text = "Invalid request" mocker.patch("httpx.AsyncClient.post", return_value=mock_response) - response = client_with_settings_override.post( + response = test_client.post( "/bpa/register", json=valid_registration_data ) @@ -99,19 +99,19 @@ def test_registration_auth0_error( def test_registration_with_invalid_organization( - client_with_settings_override, mock_auth_token, mocker, valid_registration_data + test_client, mock_auth_token, mocker, valid_registration_data ): """Test registration with invalid organization ID""" data = valid_registration_data.copy() data["organizations"] = {"invalid-org-id": True} - response = client_with_settings_override.post("/bpa/register", json=data) + response = test_client.post("/bpa/register", json=data) assert response.status_code == 400 assert "Invalid organization ID" in response.json()["detail"] -def test_registration_request_validation(client_with_settings_override): +def test_registration_request_validation(test_client): """Test request validation""" invalid_data = { "username": "testuser", @@ -119,13 +119,13 @@ def test_registration_request_validation(client_with_settings_override): "organizations": {}, } - response = client_with_settings_override.post("/bpa/register", json=invalid_data) + response = test_client.post("/bpa/register", json=invalid_data) assert response.status_code == 422 def test_no_selected_organizations( - client_with_settings_override, mock_auth_token, mocker, valid_registration_data + test_client, mock_auth_token, mocker, valid_registration_data ): """Test registration with no organizations selected""" data = valid_registration_data.copy() @@ -141,7 +141,7 @@ def test_no_selected_organizations( mock_post = mocker.patch("httpx.AsyncClient.post", return_value=mock_response) - response = client_with_settings_override.post("/bpa/register", json=data) + response = test_client.post("/bpa/register", json=data) assert response.status_code == 200 called_data = mock_post.call_args[1]["json"] @@ -150,7 +150,7 @@ def test_no_selected_organizations( def test_empty_organizations_dict( - client_with_settings_override, mock_auth_token, mocker, valid_registration_data + test_client, mock_auth_token, mocker, valid_registration_data ): """Test registration with empty organizations dictionary""" data = valid_registration_data.copy() @@ -162,7 +162,7 @@ def test_empty_organizations_dict( mock_post = mocker.patch("httpx.AsyncClient.post", return_value=mock_response) - response = client_with_settings_override.post("/bpa/register", json=data) + response = test_client.post("/bpa/register", json=data) assert response.status_code == 200 called_data = mock_post.call_args[1]["json"] @@ -171,20 +171,20 @@ def test_empty_organizations_dict( def test_registration_email_format( - client_with_settings_override, valid_registration_data + test_client, valid_registration_data ): """Test email format validation""" data = valid_registration_data.copy() data["email"] = "invalid-email" - response = client_with_settings_override.post("/bpa/register", json=data) + response = test_client.post("/bpa/register", json=data) assert response.status_code == 422 assert "email" in response.json()["detail"][0]["loc"] def test_all_organizations_selected( - client_with_settings_override, + test_client, mock_auth_token, mock_settings, mocker, @@ -200,7 +200,7 @@ def test_all_organizations_selected( mock_post = mocker.patch("httpx.AsyncClient.post", return_value=mock_response) - response = client_with_settings_override.post("/bpa/register", json=data) + response = test_client.post("/bpa/register", json=data) assert response.status_code == 200 called_data = mock_post.call_args[1]["json"] diff --git a/tests/test_galaxy.py b/tests/test_galaxy.py index d4755a79..11ce3898 100644 --- a/tests/test_galaxy.py +++ b/tests/test_galaxy.py @@ -33,11 +33,11 @@ def test_galaxy_registration_data_password_match(): public_name="valid_username") -def test_get_registration_token(client_with_settings_override, mock_settings): +def test_get_registration_token(test_client, mock_settings): """ Test get-registration-token endpoint returns a valid JWT token. """ - response = client_with_settings_override.get("/galaxy/get-registration-token") + response = test_client.get("/galaxy/get-registration-token") assert response.status_code == 200 jwt.decode(response.json()["token"], mock_settings.jwt_secret_key, algorithms=mock_settings.auth0_algorithms) @@ -78,7 +78,7 @@ def test_to_auth0_create_user_data_valid(): assert auth0_data.user_metadata.galaxy_username == "valid_username" -def test_register(mocker, mock_auth_token, mock_settings, client_with_settings_override): +def test_register(mocker, mock_auth_token, mock_settings, test_client): """ Try to test our register endpoint. Since we don't want to call an actual Auth0 API, test that: @@ -92,9 +92,9 @@ def test_register(mocker, mock_auth_token, mock_settings, client_with_settings_o mock_resp.status_code = 201 mock_post = mocker.patch("httpx.post", return_value=mock_resp) user_data = GalaxyRegistrationDataFactory.build() - token_resp = client_with_settings_override.get("/galaxy/get-registration-token") + token_resp = test_client.get("/galaxy/get-registration-token") headers = {"registration-token": token_resp.json()["token"]} - resp = client_with_settings_override.post("/galaxy/register", json=user_data.model_dump(), headers=headers) + resp = test_client.post("/galaxy/register", json=user_data.model_dump(), headers=headers) assert resp.status_code == 200 assert resp.json()["message"] == "User registered successfully" assert resp.json()["user"] == {"user_id": "abc123"} @@ -108,8 +108,8 @@ def test_register(mocker, mock_auth_token, mock_settings, client_with_settings_o ) -def test_register_requires_token(client_with_settings_override): +def test_register_requires_token(test_client): user_data = GalaxyRegistrationDataFactory.build() - resp = client_with_settings_override.post("/galaxy/register", json=user_data.model_dump()) + resp = test_client.post("/galaxy/register", json=user_data.model_dump()) assert resp.status_code == 400 assert resp.json()["detail"] == "Missing registration token" diff --git a/tests/test_main.py b/tests/test_main.py index edf35b97..2ede746c 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -5,7 +5,7 @@ client = TestClient(app) -def test_root(client_with_settings_override): - response = client_with_settings_override.get("/") +def test_root(test_client): + response = test_client.get("/") assert response.status_code == 200 assert response.json() == {"message": "AAI Backend API"} diff --git a/tests/test_user.py b/tests/test_user.py index dbed25dd..a1414a50 100644 --- a/tests/test_user.py +++ b/tests/test_user.py @@ -282,7 +282,7 @@ def test_get_resources_no_metadata( # --- Service Request Endpoints (POST) --- def test_request_service_success( mock_auth_token, auth_headers, mock_user_data, mocker, - client_with_settings_override + test_client ): """Test successful service request""" mocker.patch("routers.user.get_user_data", return_value=mock_user_data) @@ -297,7 +297,7 @@ def test_request_service_success( "user_id": mock_auth_token.sub, } - response = client_with_settings_override.post( + response = test_client.post( "/me/request/service", json=new_service, headers=auth_headers ) assert response.status_code == 200 @@ -307,7 +307,7 @@ def test_request_service_success( def test_request_service_duplicate( mock_auth_token, auth_headers, mock_user_data, mocker, - client_with_settings_override + test_client ): """Test duplicate service request""" mocker.patch("routers.user.get_user_data", return_value=mock_user_data) @@ -321,7 +321,7 @@ def test_request_service_duplicate( "user_id": mock_auth_token.sub, } - response = client_with_settings_override.post( + response = test_client.post( "/me/request/service", json=existing_service, headers=auth_headers ) assert response.status_code == 400 @@ -332,7 +332,7 @@ def test_request_service_duplicate( def test_request_service_user_mismatch( mock_auth_token, auth_headers, mock_user_data, - client_with_settings_override + test_client ): """Test service request with mismatched user""" request_payload = { @@ -341,7 +341,7 @@ def test_request_service_user_mismatch( "user_id": "auth0|WRONG_USER", } - response = client_with_settings_override.post( + response = test_client.post( "/me/request/service", json=request_payload, headers=auth_headers ) assert response.status_code == 403 @@ -354,7 +354,7 @@ def test_request_service_user_mismatch( # --- Resource Request Endpoints (POST) --- def test_request_resource_success( mock_auth_token, auth_headers, mock_user_data, mocker, - client_with_settings_override + test_client ): """Test successful resource request""" mocker.patch("routers.user.get_user_data", return_value=mock_user_data) @@ -370,7 +370,7 @@ def test_request_resource_success( "service_id": "service1", } - response = client_with_settings_override.post( + response = test_client.post( "/me/request/service1/resource-new", json=request_payload, headers=auth_headers ) assert response.status_code == 200 @@ -379,7 +379,7 @@ def test_request_resource_success( def test_request_resource_user_mismatch( mock_auth_token, auth_headers, mock_user_data, - client_with_settings_override + test_client ): """Test resource request with mismatched user""" request_payload = { @@ -389,7 +389,7 @@ def test_request_resource_user_mismatch( "service_id": "service1", } - response = client_with_settings_override.post( + response = test_client.post( "/me/request/service1/res-invalid", json=request_payload, headers=auth_headers ) assert response.status_code == 403 @@ -401,7 +401,7 @@ def test_request_resource_user_mismatch( def test_request_resource_non_approved_service( mock_auth_token, auth_headers, mock_user_data, mocker, - client_with_settings_override + test_client ): """Test resource request for non-approved service""" mocker.patch("routers.user.get_user_data", return_value=mock_user_data) @@ -416,7 +416,7 @@ def test_request_resource_non_approved_service( "service_id": "service2", } - response = client_with_settings_override.post( + response = test_client.post( "/me/request/service2/blocked-resource", json=request_payload, headers=auth_headers, @@ -430,7 +430,7 @@ def test_request_resource_non_approved_service( def test_request_resource_duplicate( mock_auth_token, auth_headers, mock_user_data, mocker, - client_with_settings_override + test_client ): """Test duplicate resource request""" mocker.patch("routers.user.get_user_data", return_value=mock_user_data) @@ -445,7 +445,7 @@ def test_request_resource_duplicate( "service_id": "service1", } - response = client_with_settings_override.post( + response = test_client.post( "/me/request/service1/resource1", json=existing_resource, headers=auth_headers ) assert response.status_code == 400 @@ -456,7 +456,7 @@ def test_request_resource_duplicate( def test_request_resource_invalid_service( mock_auth_token, auth_headers, mock_user_data, mocker, - client_with_settings_override + test_client ): """Test resource request for non-existent service""" mocker.patch("routers.user.get_user_data", return_value=mock_user_data) @@ -471,7 +471,7 @@ def test_request_resource_invalid_service( "service_id": "non-existent-service", } - response = client_with_settings_override.post( + response = test_client.post( "/me/request/non-existent-service/resource-invalid", json=request_payload, headers=auth_headers, From 5d4530b6e92a75fbd63ce6ec52f9188faa6b57cf Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:43:13 +1000 Subject: [PATCH 11/21] Add data generation for Auth0 user API data --- tests/datagen.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/datagen.py b/tests/datagen.py index fd283ff2..87e965a7 100644 --- a/tests/datagen.py +++ b/tests/datagen.py @@ -1,6 +1,7 @@ from polyfactory.decorators import post_generated from polyfactory.factories.pydantic_factory import ModelFactory +from auth0.schemas import Auth0UserResponse from routers.bpa_register import BPARegistrationRequest from schemas.galaxy import GalaxyRegistrationData from schemas.service import Auth0User @@ -11,6 +12,9 @@ class AccessTokenPayloadFactory(ModelFactory[AccessTokenPayload]): ... +class Auth0UserResponseFactory(ModelFactory[Auth0UserResponse]): ... + + class UserFactory(ModelFactory[User]): ... From ffdbbdb4d047fad684a8f886f2d9d0f025b2421e Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:43:56 +1000 Subject: [PATCH 12/21] Move user schema tests to test_user_schema --- tests/test_user.py | 66 --------------------------------------- tests/test_user_schema.py | 51 +++++++++++++----------------- 2 files changed, 21 insertions(+), 96 deletions(-) diff --git a/tests/test_user.py b/tests/test_user.py index a1414a50..9537d075 100644 --- a/tests/test_user.py +++ b/tests/test_user.py @@ -6,8 +6,6 @@ from main import app from schemas.service import AppMetadata, Group, Resource, Service -from schemas.tokens import AccessTokenPayload -from schemas.user import User from tests.datagen import AccessTokenPayloadFactory, Auth0UserFactory client = TestClient(app) @@ -478,67 +476,3 @@ def test_request_resource_invalid_service( ) assert response.status_code == 404 assert response.json()["detail"] == "Service with ID non-existent-service not found" - -def test_is_admin_true(): - payload = AccessTokenPayload( - exp=9999999999, - iat=9999999000, - iss="https://example.com/", - sub="abc123", - aud=["client_id"], - scope="read:all", - **{ - "biocommons.org.au/roles": ["PlatformAdmin"], - "permissions": ["read"] - } - ) - user = User(access_token=payload) - assert user.is_admin() is True - -def test_is_admin_false(): - payload = AccessTokenPayload( - exp=9999999999, - iat=9999999000, - iss="https://example.com/", - sub="abc123", - aud=["client_id"], - scope="read:all", - **{ - "biocommons.org.au/roles": ["guest"], - "permissions": ["read"] - } - ) - user = User(access_token=payload) - assert user.is_admin() is False - -def test_is_admin_empty_roles(): - payload = AccessTokenPayload( - exp=9999999999, - iat=9999999000, - iss="https://example.com/", - sub="abc123", - aud=["client_id"], - scope="read:all", - **{ - "biocommons.org.au/roles": [], - "permissions": ["read"] - } - ) - user = User(access_token=payload) - assert user.is_admin() is False - -def test_is_admin_multiple_roles_with_admin(): - payload = AccessTokenPayload( - exp=9999999999, - iat=9999999000, - iss="https://example.com/", - sub="abc123", - aud=["client_id"], - scope="read:all", - **{ - "biocommons.org.au/roles": ["editor", "admin_viewer"], - "permissions": ["read"] - } - ) - user = User(access_token=payload) - assert user.is_admin() is True diff --git a/tests/test_user_schema.py b/tests/test_user_schema.py index b8d83a69..16047fa2 100644 --- a/tests/test_user_schema.py +++ b/tests/test_user_schema.py @@ -1,35 +1,26 @@ -from schemas.tokens import AccessTokenPayload from schemas.user import User +from tests.datagen import AccessTokenPayloadFactory -def test_is_admin_true(): - payload = AccessTokenPayload( - exp=9999999999, - iat=9999999000, - iss="https://example.com/", - sub="abc123", - aud=["client_id"], - scope="read:all", - **{ - "biocommons.org.au/roles": ["PlatformAdmin"], # ✅ match schema key - "permissions": ["read"] - } - ) +def test_is_admin_true(mock_settings): + payload = AccessTokenPayloadFactory.build(biocommons_roles=["Admin"]) user = User(access_token=payload) - assert user.is_admin() is True - -def test_is_admin_false(): - payload = AccessTokenPayload( - exp=9999999999, - iat=9999999000, - iss="https://example.com/", - sub="abc123", - aud=["client_id"], - scope="read:all", - **{ - "biocommons.org.au/roles": ["guest"], # ✅ match schema key - "permissions": ["read"] - } - ) + assert user.is_admin(settings=mock_settings) is True + + +def test_is_admin_false(mock_settings): + payload = AccessTokenPayloadFactory.build(biocommons_roles=["User"]) + user = User(access_token=payload) + assert user.is_admin(settings=mock_settings) is False + + +def test_is_admin_empty_roles(mock_settings): + payload = AccessTokenPayloadFactory.build(biocommons_roles=[]) + user = User(access_token=payload) + assert user.is_admin(settings=mock_settings) is False + + +def test_is_admin_multiple_roles_with_admin(mock_settings): + payload = AccessTokenPayloadFactory.build(biocommons_roles=["Admin", "Editor"]) user = User(access_token=payload) - assert user.is_admin() is False + assert user.is_admin(settings=mock_settings) is True From f72c89adec5da2bce967e162323a7c99381f054f Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:49:37 +1000 Subject: [PATCH 13/21] Initial Auth0 client for making requests to the management API --- auth0/__init__.py | 0 auth0/client.py | 17 +++++++++++++++++ auth0/schemas.py | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+) create mode 100644 auth0/__init__.py create mode 100644 auth0/client.py create mode 100644 auth0/schemas.py diff --git a/auth0/__init__.py b/auth0/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/auth0/client.py b/auth0/client.py new file mode 100644 index 00000000..902244b6 --- /dev/null +++ b/auth0/client.py @@ -0,0 +1,17 @@ +__all__ = ["Auth0Client"] + +import httpx + +from auth0.schemas import Auth0UserResponse + + +class Auth0Client: + + def __init__(self, domain: str): + self.domain = domain + + def get_users(self, access_token: str) -> list[Auth0UserResponse]: + url = f"https://{self.domain}/api/v2/users" + headers = {"Authorization": f"Bearer {access_token}"} + resp = httpx.get(url, headers=headers) + return resp.json() diff --git a/auth0/schemas.py b/auth0/schemas.py new file mode 100644 index 00000000..a0313c79 --- /dev/null +++ b/auth0/schemas.py @@ -0,0 +1,34 @@ +from datetime import datetime +from typing import List, Optional + +from pydantic import BaseModel, ConfigDict, EmailStr + + +class Auth0UserResponse(BaseModel): + """ + Response returned by Auth0's /users endpoint. + Note we have our own Auth0User model + that includes specifying the metadata fields we use. + """ + user_id: str + email: EmailStr + email_verified: bool + username: Optional[str] = None + phone_number: Optional[str] = None + phone_verified: Optional[bool] = None + created_at: datetime + updated_at: datetime + identities: List[dict] + app_metadata: Optional[dict] = None + user_metadata: Optional[dict] = None + picture: Optional[str] = None + name: Optional[str] = None + nickname: Optional[str] = None + last_ip: Optional[str] = None + last_login: Optional[datetime] = None + logins_count: Optional[int] = None + blocked: Optional[bool] = None + given_name: Optional[str] = None + family_name: Optional[str] = None + + model_config = ConfigDict(extra="allow") From 0c22725c8ccc160f2924fa2499a7d8b0feae960f Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:52:43 +1000 Subject: [PATCH 14/21] Initial admin router with one route defined (and all routes protected by user_is_admin) --- routers/admin.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 routers/admin.py diff --git a/routers/admin.py b/routers/admin.py new file mode 100644 index 00000000..5381e659 --- /dev/null +++ b/routers/admin.py @@ -0,0 +1,23 @@ +from fastapi import APIRouter, Depends + +from auth.config import Settings, get_settings +from auth.management import get_management_token +from auth.validator import user_is_admin +from auth0.client import Auth0Client +from auth0.schemas import Auth0UserResponse + +router = APIRouter(prefix="/admin", tags=["admin"], + dependencies=[Depends(user_is_admin)]) + + +def get_auth0_client(settings: Settings = Depends(get_settings)): + return Auth0Client(settings.auth0_domain) + + +@router.get("/users", + response_model=list[Auth0UserResponse]) +def get_users(settings: Settings = Depends(get_settings), + client: Auth0Client = Depends(get_auth0_client)): + token = get_management_token(settings=settings) + resp = client.get_users(token) + return resp From be557e1a928d8b2022ce5a11936a9aaae3bd24fa Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 11:53:39 +1000 Subject: [PATCH 15/21] Add admin router to app --- main.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/main.py b/main.py index a9d29a0c..7f142c33 100644 --- a/main.py +++ b/main.py @@ -4,7 +4,7 @@ from fastapi import FastAPI from starlette.middleware.cors import CORSMiddleware -from routers import bpa_register, galaxy_register, user +from routers import admin, bpa_register, galaxy_register, user # Load .env to get CORS_ALLOWED_ORIGINS. # Note that for most env variables, we use pydantic-settings @@ -30,6 +30,7 @@ def public_route(): return {"message": "AAI Backend API"} +app.include_router(admin.router) app.include_router(user.router) app.include_router(bpa_register.router) app.include_router(galaxy_register.router) From 8e6de4d27ac3bc7b2839152d0350b1f4ef0dc10b Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 13:46:20 +1000 Subject: [PATCH 16/21] Add tests for admin endpoints --- tests/test_admin.py | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 tests/test_admin.py diff --git a/tests/test_admin.py b/tests/test_admin.py new file mode 100644 index 00000000..90041b7e --- /dev/null +++ b/tests/test_admin.py @@ -0,0 +1,46 @@ +import pytest +from fastapi import HTTPException + +from auth.validator import get_current_user, user_is_admin +from main import app +from tests.datagen import ( + AccessTokenPayloadFactory, + Auth0UserResponseFactory, + UserFactory, +) + + +def test_get_users_requires_admin_unauthorized(test_client, mocker): + def get_nonadmin_user(): + payload = AccessTokenPayloadFactory.build(biocommons_roles=["User"]) + return UserFactory.build(access_token=payload) + + app.dependency_overrides[get_current_user] = get_nonadmin_user + mocker.patch("routers.admin.get_management_token", return_value="mock_token") + resp = test_client.get("/admin/users") + assert resp.status_code == 403 + assert resp.json() == {"detail": "You must be an admin to access this endpoint."} + app.dependency_overrides.clear() + + +def test_user_is_admin(mock_settings): + payload = AccessTokenPayloadFactory.build(biocommons_roles=["Admin"]) + admin_user = UserFactory.build(access_token=payload) + assert user_is_admin(current_user=admin_user, settings=mock_settings) + + +def test_user_is_admin_nonadmin_user(mock_settings): + payload = AccessTokenPayloadFactory.build(biocommons_roles=["User"]) + user = UserFactory.build(access_token=payload) + with pytest.raises(HTTPException, match="You must be an admin to access this endpoint."): + user_is_admin(current_user=user, settings=mock_settings) + + +def test_get_users(mocker, test_client, as_admin_user): + mocker.patch("routers.admin.get_management_token", return_value="mock_token") + mock_client = mocker.patch("routers.admin.Auth0Client") + users = Auth0UserResponseFactory.batch(3) + mock_client().get_users.return_value = users + resp = test_client.get("/admin/users") + assert resp.status_code == 200 + assert len(resp.json()) == 3 From 56a074d3c3e5b3180926acef86fdc43960a81731 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 14:51:12 +1000 Subject: [PATCH 17/21] Force .env file to be ignored in tests --- tests/conftest.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index ef92d887..1c771601 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,6 +7,15 @@ from tests.datagen import AccessTokenPayloadFactory, UserFactory +def pytest_configure(config: pytest.Config) -> None: + """ + Force the app to ignore the .env file while testing. + Otherwise we get different results when the .env + file is present or not. + """ + Settings.model_config["env_file"] = "" + + @pytest.fixture def mock_settings(): """Fixture that returns mocked Settings object.""" From 9b984d799c9c2de6d1a86e582ffcf38909e614ec Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 16:00:16 +1000 Subject: [PATCH 18/21] Don't load env variables in main, just grab the needed value from the file --- main.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/main.py b/main.py index 7f142c33..6156d1e6 100644 --- a/main.py +++ b/main.py @@ -1,6 +1,5 @@ -import os -from dotenv import load_dotenv +from dotenv import dotenv_values from fastapi import FastAPI from starlette.middleware.cors import CORSMiddleware @@ -10,9 +9,9 @@ # Note that for most env variables, we use pydantic-settings # and load them via auth.config. But we need the # allowed_origins before we load the app -load_dotenv() +env_values = dotenv_values(".env") ALLOWED_ORIGINS = [ - origin.strip() for origin in os.getenv("CORS_ALLOWED_ORIGINS", "").split(",") + origin.strip() for origin in env_values.get("CORS_ALLOWED_ORIGINS", "").split(",") ] app = FastAPI() From 184bfa3a7d3cc84f854d700a339a1ca5d48f9529 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 16:02:13 +1000 Subject: [PATCH 19/21] Rework how we force the env file to be ignored in tests --- tests/conftest.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 1c771601..ac053514 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,13 +7,15 @@ from tests.datagen import AccessTokenPayloadFactory, UserFactory -def pytest_configure(config: pytest.Config) -> None: +@pytest.fixture(autouse=True) +def ignore_env_file(): """ - Force the app to ignore the .env file while testing. - Otherwise we get different results when the .env - file is present or not. + Always ignore the .env file when running tests, + so we get the same behaviour when the .env file is present or not. """ - Settings.model_config["env_file"] = "" + def get_settings_no_env_file(): + return Settings(_env_file=None) + app.dependency_overrides[get_settings] = get_settings_no_env_file @pytest.fixture From bb082770329594ca364562998ee0e065f34a6e8a Mon Sep 17 00:00:00 2001 From: marius-mather Date: Fri, 23 May 2025 16:02:40 +1000 Subject: [PATCH 20/21] Update tests to use the test client --- tests/test_user.py | 56 +++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/tests/test_user.py b/tests/test_user.py index 9537d075..8cc22be6 100644 --- a/tests/test_user.py +++ b/tests/test_user.py @@ -2,14 +2,10 @@ import pytest from fastapi import HTTPException -from fastapi.testclient import TestClient -from main import app from schemas.service import AppMetadata, Group, Resource, Service from tests.datagen import AccessTokenPayloadFactory, Auth0UserFactory -client = TestClient(app) - # --- Test Fixtures --- @pytest.fixture @@ -76,16 +72,17 @@ def mock_user_data(): "/me/all/pending", ], ) -def test_endpoints_require_auth(endpoint): +def test_endpoints_require_auth(endpoint, test_client): """Test that all endpoints require authentication""" - response = client.get(endpoint) + response = test_client.get(endpoint) assert response.status_code == 401 assert response.json() == {"detail": "Not authenticated"} # --- Service Endpoints (GET) --- def test_get_all_services( - mock_auth_token, auth_headers, mock_user_data, mocker + mock_auth_token, auth_headers, mock_user_data, mocker, + test_client ): """Test getting all services""" mocker.patch( @@ -96,7 +93,7 @@ def test_get_all_services( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/services", headers=auth_headers) + response = test_client.get("/me/services", headers=auth_headers) assert response.status_code == 200 expected_services = [ @@ -106,7 +103,8 @@ def test_get_all_services( def test_get_approved_services( - mock_auth_token, auth_headers, mock_user_data, mocker + mock_auth_token, auth_headers, mock_user_data, mocker, + test_client ): """Test getting approved services""" mocker.patch( @@ -117,7 +115,7 @@ def test_get_approved_services( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/services/approved", headers=auth_headers) + response = test_client.get("/me/services/approved", headers=auth_headers) assert response.status_code == 200 approved_services = [ @@ -129,7 +127,8 @@ def test_get_approved_services( def test_get_pending_services( - mock_auth_token, auth_headers, mock_user_data, mocker + mock_auth_token, auth_headers, mock_user_data, mocker, + test_client ): """Test getting pending services""" mocker.patch( @@ -140,7 +139,7 @@ def test_get_pending_services( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/services/pending", headers=auth_headers) + response = test_client.get("/me/services/pending", headers=auth_headers) assert response.status_code == 200 pending_services = [ @@ -152,7 +151,8 @@ def test_get_pending_services( def test_get_services_failed_fetch( - mock_auth_token, auth_headers, mocker + mock_auth_token, auth_headers, mocker, + test_client ): """Test handling of failed API calls""" mocker.patch( @@ -163,13 +163,13 @@ def test_get_services_failed_fetch( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/services", headers=auth_headers) + response = test_client.get("/me/services", headers=auth_headers) assert response.status_code == 403 assert response.json() == {"detail": "Failed to fetch user data"} def test_get_services_empty_metadata( - mock_auth_token, auth_headers, mocker + mock_auth_token, auth_headers, mocker, test_client ): """Test handling of empty metadata""" empty_user = Auth0UserFactory.build( @@ -180,13 +180,14 @@ def test_get_services_empty_metadata( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/services", headers=auth_headers) + response = test_client.get("/me/services", headers=auth_headers) assert response.status_code == 200 assert response.json() == {"services": []} def test_get_services_no_metadata( - mock_auth_token, auth_headers, mocker + mock_auth_token, auth_headers, mocker, + test_client ): """Test handling of missing metadata""" no_metadata_user = Auth0UserFactory.build(app_metadata=AppMetadata()) @@ -195,14 +196,15 @@ def test_get_services_no_metadata( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/services", headers=auth_headers) + response = test_client.get("/me/services", headers=auth_headers) assert response.status_code == 200 assert response.json() == {"services": []} # --- Resource Endpoints (GET) --- def test_get_all_resources( - mock_auth_token, auth_headers, mock_user_data, mocker + mock_auth_token, auth_headers, mock_user_data, mocker, + test_client ): """Test getting all resources""" mocker.patch( @@ -213,7 +215,7 @@ def test_get_all_resources( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/resources", headers=auth_headers) + response = test_client.get("/me/resources", headers=auth_headers) assert response.status_code == 200 all_resources = [ r.model_dump() @@ -224,7 +226,8 @@ def test_get_all_resources( def test_get_approved_resources( - mock_auth_token, auth_headers, mock_user_data, mocker + mock_auth_token, auth_headers, mock_user_data, mocker, + test_client ): """Test getting approved resources""" mocker.patch( @@ -235,7 +238,7 @@ def test_get_approved_resources( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/resources/approved", headers=auth_headers) + response = test_client.get("/me/resources/approved", headers=auth_headers) assert response.status_code == 200 approved_resources = [ r.model_dump() @@ -247,7 +250,7 @@ def test_get_approved_resources( def test_get_resources_empty_metadata( - mock_auth_token, auth_headers, mocker + mock_auth_token, auth_headers, mocker, test_client ): """Test handling of empty resource metadata""" empty_user = Auth0UserFactory.build(app_metadata=AppMetadata(services=[], groups=[]), @@ -257,13 +260,14 @@ def test_get_resources_empty_metadata( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/resources", headers=auth_headers) + response = test_client.get("/me/resources", headers=auth_headers) assert response.status_code == 200 assert response.json() == {"resources": []} def test_get_resources_no_metadata( - mock_auth_token, auth_headers, mocker + mock_auth_token, auth_headers, mocker, + test_client ): """Test handling of missing resource metadata""" no_metadata_user = Auth0UserFactory.build(app_metadata=AppMetadata()) @@ -272,7 +276,7 @@ def test_get_resources_no_metadata( "routers.user.get_management_token", return_value="mock_management_token" ) - response = client.get("/me/resources", headers=auth_headers) + response = test_client.get("/me/resources", headers=auth_headers) assert response.status_code == 200 assert response.json() == {"resources": []} From 99bbacac020e64d289d8f96f6b4952d7b63cfa2e Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 26 May 2025 09:35:14 +1000 Subject: [PATCH 21/21] Style fix --- tests/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/conftest.py b/tests/conftest.py index ac053514..7d589985 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -61,6 +61,7 @@ def as_admin_user(): def override_user(): token = AccessTokenPayloadFactory.build(biocommons_roles=["Admin"]) return UserFactory.build(access_token=token) + app.dependency_overrides[get_current_user] = override_user yield app.dependency_overrides.clear()