From e5e5656cc0e73934f028371e62166a3f3789d4e5 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 2 Jun 2025 13:18:16 +1000 Subject: [PATCH 01/10] Add pagination params for user queries --- auth0/client.py | 51 +++++++++++++++++++++++++++++++++++------------- routers/admin.py | 42 +++++++++++++++++++++++++++++++-------- 2 files changed, 71 insertions(+), 22 deletions(-) diff --git a/auth0/client.py b/auth0/client.py index 569ce5d4..59eb5b14 100644 --- a/auth0/client.py +++ b/auth0/client.py @@ -1,5 +1,7 @@ __all__ = ["Auth0Client"] +from typing import Optional + import httpx from auth0.schemas import Auth0UserResponse @@ -17,9 +19,16 @@ def _convert_users(resp: httpx.Response): """Convert a list of Auth0UserResponse objects from a response.""" return [Auth0UserResponse(**user) for user in resp.json()] - def get_users(self) -> list[Auth0UserResponse]: + def get_users(self, page: Optional[int] = None, per_page: Optional[int] = None) -> list[Auth0UserResponse]: + params = {} + if page is not None: + # Convert from 1-based pagination to 0-based. + page = page - 1 + params["page"] = page + if per_page is not None: + params["per_page"] = per_page url = f"https://{self.domain}/api/v2/users" - resp = self._client.get(url) + resp = self._client.get(url, params=params) return self._convert_users(resp) def get_user(self, user_id: str) -> Auth0UserResponse: @@ -27,23 +36,37 @@ def get_user(self, user_id: str) -> Auth0UserResponse: resp = self._client.get(url) return Auth0UserResponse(**resp.json()) - def get_approved_users(self) -> list[Auth0UserResponse]: - # TODO: also search for approved resources? (with OR) - approved_query = 'app_metadata.services.status:"approved"' + def _search_users(self, query: str, page: Optional[int] = None, per_page: Optional[int] = None) -> list[Auth0UserResponse]: + params = {"q": query, "search_engine": "v3"} + if page is not None: + # Convert from 1-based pagination to 0-based. + page = page - 1 + params["page"] = page + if per_page is not None: + params["per_page"] = per_page url = f"https://{self.domain}/api/v2/users" # TODO: set primary_order=false for faster search? # https://auth0.com/docs/manage-users/user-search/user-search-best-practices - resp = self._client.get(url, params={"q": approved_query, "search_engine": "v3"}) + resp = self._client.get( + url, + params={ + "q": query, + "search_engine": "v3", + "page": page, + "per_page": per_page + } + ) return self._convert_users(resp) - def get_pending_users(self) -> list[Auth0UserResponse]: + def get_approved_users(self, page: Optional[int] = None, per_page: Optional[int] = None) -> list[Auth0UserResponse]: + # TODO: also search for approved resources? (with OR) + approved_query = 'app_metadata.services.status:"approved"' + return self._search_users(approved_query, page, per_page) + + def get_pending_users(self, page: Optional[int] = None, per_page: Optional[int] = None) -> list[Auth0UserResponse]: pending_query = 'app_metadata.services.status:"pending"' - url = f"https://{self.domain}/api/v2/users" - resp = self._client.get(url, params={"q": pending_query, "search_engine": "v3"}) - return [Auth0UserResponse(**user) for user in resp.json()] + return self._search_users(pending_query, page, per_page) - def get_revoked_users(self) -> list[Auth0UserResponse]: + def get_revoked_users(self, page: Optional[int] = None, per_page: Optional[int] = None) -> list[Auth0UserResponse]: revoked_query = 'app_metadata.services.status:"revoked"' - url = f"https://{self.domain}/api/v2/users" - resp = self._client.get(url, params={"q": revoked_query, "search_engine": "v3"}) - return [Auth0UserResponse(**user) for user in resp.json()] + return self._search_users(revoked_query, page, per_page) diff --git a/routers/admin.py b/routers/admin.py index 875c7bf1..6e95a0f2 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -3,6 +3,8 @@ from typing import Annotated from fastapi import APIRouter, Depends, Path +from fastapi.params import Query +from pydantic import BaseModel from auth.config import Settings, get_settings from auth.management import get_management_token @@ -19,6 +21,26 @@ ServiceIdParam = Path(..., pattern=r"^[-a-zA-Z0-9_]+$") ResourceIdParam = Path(..., pattern=r"^[-a-zA-Z0-9_]+$") + +class PaginationParams(BaseModel): + """ + Query parameters for paginated endpoints. Page starts at 1. + """ + page: int = Query(1, ge=1) + per_page: int = Query(100, ge=1, le=100) + + @property + def start_index(self): + return (self.page - 1) * self.per_page + + +def get_pagination_params(page: int = 1, per_page: int = 100): + return PaginationParams( + page=page, + per_page=per_page + ) + + router = APIRouter(prefix="/admin", tags=["admin"], dependencies=[Depends(user_is_admin)]) @@ -32,27 +54,31 @@ def get_auth0_client(settings: Settings = Depends(get_settings), # of them @router.get("/users", response_model=list[Auth0UserResponse]) -def get_users(client: Auth0Client = Depends(get_auth0_client)): - resp = client.get_users() +def get_users(client: Annotated[Auth0Client, Depends(get_auth0_client)], + pagination: Annotated[PaginationParams, Depends(get_pagination_params)]): + resp = client.get_users(page=pagination.page, per_page=pagination.per_page) return resp # NOTE: This must appear before /users/{user_id} so it takes precedence @router.get("/users/approved") -def get_approved_users(client: Annotated[Auth0Client, Depends(get_auth0_client)]): - resp = client.get_approved_users() +def get_approved_users(client: Annotated[Auth0Client, Depends(get_auth0_client)], + pagination: Annotated[PaginationParams, Depends(get_pagination_params)]): + resp = client.get_approved_users(page=pagination.page, per_page=pagination.per_page) return resp @router.get("/users/pending") -def get_pending_users(client: Annotated[Auth0Client, Depends(get_auth0_client)]): - resp = client.get_pending_users() +def get_pending_users(client: Annotated[Auth0Client, Depends(get_auth0_client)], + pagination: Annotated[PaginationParams, Depends(get_pagination_params)]): + resp = client.get_pending_users(page=pagination.page, per_page=pagination.per_page) return resp @router.get("/users/revoked") -def get_revoked_users(client: Annotated[Auth0Client, Depends(get_auth0_client)]): - resp = client.get_revoked_users() +def get_revoked_users(client: Annotated[Auth0Client, Depends(get_auth0_client)], + pagination: Annotated[PaginationParams, Depends(get_pagination_params)]): + resp = client.get_revoked_users(page=pagination.page, per_page=pagination.per_page) return resp From 81ff3c8f033526b69d3fc1a6c6248ecac4a76055 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 2 Jun 2025 13:44:58 +1000 Subject: [PATCH 02/10] Add respx to deps --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index 1ecc984f..92fb9cff 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -20,6 +20,7 @@ dev = [ "polyfactory>=2.21.0", "pre-commit>=3.7.0", "freezegun>=1.5.2", + "respx>=0.22.0", ] [tool.pytest.ini_options] From 34c14ed1a63c4151734a67dd4398ce1af6d6aba4 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 2 Jun 2025 13:58:08 +1000 Subject: [PATCH 03/10] Update uv lockfile --- uv.lock | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/uv.lock b/uv.lock index 17a5bcaa..2840c9b1 100644 --- a/uv.lock +++ b/uv.lock @@ -21,6 +21,7 @@ dev = [ { name = "pytest" }, { name = "pytest-cov" }, { name = "pytest-mock" }, + { name = "respx" }, { name = "ruff" }, ] @@ -36,6 +37,7 @@ requires-dist = [ { name = "pytest-cov", marker = "extra == 'dev'", specifier = ">=4.1.0" }, { name = "pytest-mock", marker = "extra == 'dev'", specifier = ">=3.14.0" }, { name = "python-jose", specifier = ">=3.4.0" }, + { name = "respx", marker = "extra == 'dev'", specifier = ">=0.22.0" }, { name = "ruff", marker = "extra == 'dev'", specifier = ">=0.4.4" }, ] provides-extras = ["dev"] @@ -630,6 +632,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/fa/de/02b54f42487e3d3c6efb3f89428677074ca7bf43aae402517bc7cca949f3/PyYAML-6.0.2-cp313-cp313-win_amd64.whl", hash = "sha256:8388ee1976c416731879ac16da0aff3f63b286ffdd57cdeb95f3f2e085687563", size = 156446, upload-time = "2024-08-06T20:33:04.33Z" }, ] +[[package]] +name = "respx" +version = "0.22.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "httpx" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/f4/7c/96bd0bc759cf009675ad1ee1f96535edcb11e9666b985717eb8c87192a95/respx-0.22.0.tar.gz", hash = "sha256:3c8924caa2a50bd71aefc07aa812f2466ff489f1848c96e954a5362d17095d91", size = 28439, upload-time = "2024-12-19T22:33:59.374Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/8e/67/afbb0978d5399bc9ea200f1d4489a23c9a1dad4eee6376242b8182389c79/respx-0.22.0-py2.py3-none-any.whl", hash = "sha256:631128d4c9aba15e56903fb5f66fb1eff412ce28dd387ca3a81339e52dbd3ad0", size = 25127, upload-time = "2024-12-19T22:33:57.837Z" }, +] + [[package]] name = "rich" version = "14.0.0" From 770076df9fb29356a94c99605429ac40dde0b06c Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 2 Jun 2025 13:58:37 +1000 Subject: [PATCH 04/10] Add tests of pagination for Auth0Client --- tests/test_auth0_client.py | 82 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 tests/test_auth0_client.py diff --git a/tests/test_auth0_client.py b/tests/test_auth0_client.py new file mode 100644 index 00000000..71ca0a4a --- /dev/null +++ b/tests/test_auth0_client.py @@ -0,0 +1,82 @@ +import pytest +import respx +from httpx import Response + +from auth0.client import Auth0Client +from auth0.schemas import Auth0UserResponse +from tests.datagen import Auth0UserResponseFactory + + +@pytest.fixture +def auth0_client(): + return Auth0Client(domain="example.auth0.com", management_token="dummy-token") + + +@respx.mock +def test_get_users_no_pagination(auth0_client): + user = Auth0UserResponseFactory.build() + route = respx.get("https://example.auth0.com/api/v2/users").mock( + return_value=Response(200, json=[user.model_dump(mode="json")]) + ) + + result = auth0_client.get_users() + + assert route.called + assert result[0].model_dump(mode="json") == user.model_dump(mode="json") + + +@respx.mock +def test_get_users_with_pagination(auth0_client): + user = Auth0UserResponseFactory.build() + route = respx.get("https://example.auth0.com/api/v2/users").respond( + 200, json=[user.model_dump(mode="json")] + ) + + result = auth0_client.get_users(page=2, per_page=25) + + # Validate the actual request + request = route.calls[0].request + assert route.called + assert request.url.params["page"] == "1" + assert request.url.params["per_page"] == "25" + assert result[0].model_dump(mode="json") == user.model_dump(mode="json") + + +@respx.mock +def test_get_user_by_id(auth0_client): + user_id = "auth0|789" + expected = {"user_id": user_id, "email": "one@example.com"} + route = respx.get(f"https://example.auth0.com/api/v2/users/{user_id}").mock( + return_value=Response(200, json=expected) + ) + + result = auth0_client.get_user(user_id) + + assert route.called + assert result == Auth0UserResponse(**expected) + + +@pytest.mark.parametrize( + "method,query", + [ + ("get_approved_users", 'app_metadata.services.status:"approved"'), + ("get_pending_users", 'app_metadata.services.status:"pending"'), + ("get_revoked_users", 'app_metadata.services.status:"revoked"'), + ] +) +@respx.mock +def test_search_users_methods(auth0_client, method, query): + user = Auth0UserResponseFactory.build() + route = respx.get("https://example.auth0.com/api/v2/users").respond( + 200, json=[user.model_dump(mode="json")] + ) + + result = getattr(auth0_client, method)(page=3, per_page=50) + + assert route.called + request = route.calls[0].request + assert request.url.params["q"] == query + assert request.url.params["search_engine"] == "v3" + assert request.url.params["page"] == "2" + assert request.url.params["per_page"] == "50" + assert result[0].model_dump(mode="json") == user.model_dump(mode="json") From 16913e10945367f446377333f2a58765a2d2f393 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 2 Jun 2025 16:01:29 +1000 Subject: [PATCH 05/10] Fix test for get_user_by_id --- tests/test_auth0_client.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test_auth0_client.py b/tests/test_auth0_client.py index 71ca0a4a..1889d8c2 100644 --- a/tests/test_auth0_client.py +++ b/tests/test_auth0_client.py @@ -3,7 +3,6 @@ from httpx import Response from auth0.client import Auth0Client -from auth0.schemas import Auth0UserResponse from tests.datagen import Auth0UserResponseFactory @@ -45,15 +44,15 @@ def test_get_users_with_pagination(auth0_client): @respx.mock def test_get_user_by_id(auth0_client): user_id = "auth0|789" - expected = {"user_id": user_id, "email": "one@example.com"} + user = Auth0UserResponseFactory.build(user_id=user_id) route = respx.get(f"https://example.auth0.com/api/v2/users/{user_id}").mock( - return_value=Response(200, json=expected) + return_value=Response(200, json=user.model_dump(mode="json")) ) result = auth0_client.get_user(user_id) assert route.called - assert result == Auth0UserResponse(**expected) + assert result.model_dump(mode="json") == user.model_dump(mode="json") @pytest.mark.parametrize( From 048296fa576a29c7e2a262e04c572ff8d794499b Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 2 Jun 2025 16:07:55 +1000 Subject: [PATCH 06/10] Test pagination params --- tests/test_admin.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_admin.py b/tests/test_admin.py index 6a051fc4..1eb1e9f6 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -7,6 +7,7 @@ from auth.validator import get_current_user, user_is_admin from main import app +from routers.admin import PaginationParams from schemas import Resource, Service from tests.datagen import ( AccessTokenPayloadFactory, @@ -33,6 +34,15 @@ def mock_auth0_client(mocker): return mock_client() +def test_pagination_params_start_index(): + """ + Test we can get the current start index given the page number and per_page. + """ + params = PaginationParams(page=2, per_page=10) + # start index for page 1 is 0, for page 2 is 0 + per_page = 10 + assert params.start_index == 10 + + def test_get_users_requires_admin_unauthorized(test_client, mocker): def get_nonadmin_user(): payload = AccessTokenPayloadFactory.build(biocommons_roles=["User"]) From dca191f7733f3a53eede7dfc614a838e48f69344 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Tue, 3 Jun 2025 09:43:15 +1000 Subject: [PATCH 07/10] Raise a HTTPException when validating PaginationParams --- routers/admin.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/routers/admin.py b/routers/admin.py index 6e95a0f2..184abf38 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -2,9 +2,9 @@ import logging from typing import Annotated -from fastapi import APIRouter, Depends, Path +from fastapi import APIRouter, Depends, HTTPException, Path from fastapi.params import Query -from pydantic import BaseModel +from pydantic import BaseModel, ValidationError from auth.config import Settings, get_settings from auth.management import get_management_token @@ -35,10 +35,10 @@ def start_index(self): def get_pagination_params(page: int = 1, per_page: int = 100): - return PaginationParams( - page=page, - per_page=per_page - ) + try: + return PaginationParams(page=page, per_page=per_page) + except ValidationError: + raise HTTPException(status_code=422, detail="Invalid page params: page should be >= 1, per_page should be >= 1 and <= 100") router = APIRouter(prefix="/admin", tags=["admin"], From 4bf0909bfd80217827fa1c6ef8492a5448c79924 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Tue, 3 Jun 2025 09:43:30 +1000 Subject: [PATCH 08/10] Add tests of paginated endpoints --- tests/test_admin.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/test_admin.py b/tests/test_admin.py index 1eb1e9f6..aefcf384 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -77,6 +77,23 @@ def test_get_users(test_client, as_admin_user, mock_auth0_client): assert len(resp.json()) == 3 +def test_get_users_pagination_params(test_client, as_admin_user, mock_auth0_client): + users = Auth0UserResponseFactory.batch(3) + mock_auth0_client.get_users.return_value = users + resp = test_client.get("/admin/users?page=2&per_page=10") + assert resp.status_code == 200 + assert len(resp.json()) == 3 + + +def test_get_users_invalid_params(test_client, as_admin_user, mock_auth0_client): + users = Auth0UserResponseFactory.batch(3) + mock_auth0_client.get_users.return_value = users + resp = test_client.get("/admin/users?page=0&per_page=500") + assert resp.status_code == 422 + error_msg = resp.json()["detail"] + assert "Invalid page params" in error_msg + + def test_get_user(test_client, as_admin_user, mock_auth0_client): user = Auth0UserResponseFactory.build() mock_auth0_client.get_user.return_value = user From e2d08d23321d62a116d00c6fba988577f57861d1 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Tue, 3 Jun 2025 09:44:00 +1000 Subject: [PATCH 09/10] Style fix --- routers/admin.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/routers/admin.py b/routers/admin.py index 184abf38..9241dea8 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -38,7 +38,10 @@ def get_pagination_params(page: int = 1, per_page: int = 100): try: return PaginationParams(page=page, per_page=per_page) except ValidationError: - raise HTTPException(status_code=422, detail="Invalid page params: page should be >= 1, per_page should be >= 1 and <= 100") + raise HTTPException( + status_code=422, + detail="Invalid page params: page should be >= 1, per_page should be >= 1 and <= 100" + ) router = APIRouter(prefix="/admin", tags=["admin"], From b9c287af44ae07872d5812a8ffde4802c9faf0fd Mon Sep 17 00:00:00 2001 From: marius-mather Date: Tue, 3 Jun 2025 09:55:28 +1000 Subject: [PATCH 10/10] Test that resources are revoked when revoking a service --- tests/test_admin.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test_admin.py b/tests/test_admin.py index aefcf384..e8311e07 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -202,13 +202,15 @@ def test_revoke_service(test_client, as_admin_user, mock_auth0_client, mocker): Note this is currently pretty clunky due to the need to mock out asyncio.run. """ - # Build test user and metadata + resource1 = Resource(name="Test Resource", id="resource1", status="approved") + resource2 = Resource(name="Test Resource", id="resource2", status="approved") service = Service( name="Test Service", id="service1", status="approved", last_updated=FROZEN_TIME - timedelta(hours=1), - updated_by="" + updated_by="", + resources=[resource1, resource2] ) app_metadata = AppMetadataFactory.build(services=[service]) user = Auth0UserResponseFactory.build(app_metadata=app_metadata.model_dump(mode="json")) @@ -242,6 +244,8 @@ def test_revoke_service(test_client, as_admin_user, mock_auth0_client, mocker): assert service_data["status"] == "revoked" assert service_data["id"] == service.id assert service_data["updated_by"] == revoking_user.email + for resource in service_data["resources"]: + assert resource["status"] == "revoked" def test_approve_resource(test_client, as_admin_user, mock_auth0_client, mocker):