From b1d7b76d16a9869216e94d1c960f18dfa2b56ee7 Mon Sep 17 00:00:00 2001 From: mishaschwartz <4380924+mishaschwartz@users.noreply.github.com> Date: Wed, 24 Jun 2026 14:59:25 -0400 Subject: [PATCH] add auth --- .github/workflows/test.yml | 2 +- .pre-commit-config.yaml | 4 +- README.md | 35 ++++++-- docker-compose.dev.yml | 10 ++- docker-compose.yml | 2 +- marble_api/_config.py | 24 ++++++ marble_api/app.py | 2 +- marble_api/database/__init__.py | 6 +- marble_api/utils/auth.py | 47 ++++++++++ marble_api/versions/v1/__init__.py | 16 ++++ marble_api/versions/v1/app.py | 9 -- marble_api/versions/v1/data_request/routes.py | 37 ++++---- pyproject.toml | 6 +- test/conftest.py | 18 ++++ test/integration/conftest.py | 13 +++ .../versions/v1/data_request/test_routes.py | 86 +++++++++++++++++-- 16 files changed, 260 insertions(+), 57 deletions(-) create mode 100644 marble_api/_config.py create mode 100644 marble_api/utils/auth.py delete mode 100644 marble_api/versions/v1/app.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e2e22eb..a64d797 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -3,7 +3,7 @@ on: pull_request: types: [opened, synchronize, reopened, ready_for_review] env: - MONGODB_URI: mongodb://localhost:27017 + MARBLE_API_MONGODB_URI: mongodb://localhost:27017 jobs: test: if: github.event.pull_request.draft == false diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 8a9e74d..0771f28 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,10 +1,10 @@ repos: - repo: https://github.com/astral-sh/ruff-pre-commit # Ruff version. - rev: v0.13.2 + rev: v0.15.9 hooks: # Run the linter. - - id: ruff + - id: ruff-check # Run the formatter. - id: ruff-format \ No newline at end of file diff --git a/README.md b/README.md index b6a968b..20aed5d 100644 --- a/README.md +++ b/README.md @@ -8,19 +8,36 @@ An API for the Marble platform. ## Authentication and Authorization -Marble API does not do any authentication or authorization (authn/z). That is left to other -applications (such as [Magpie](https://github.com/ouranosinc/magpie)). +Marble API uses [Magpie](https://github.com/ouranosinc/magpie) for authentication. -Marble API assumes that only users with administrator access should be able to access all routes -prefixed with `/vX/admin/` (where `X` is a version number). +Authentication is enforced for the following routes: -Marble API also assumes that only users with a given user name or id `Y` should be able to access -all routes prefixed with `/vX/users/Y/` (where `X` is a version number). +- admin routes: `/vX/admin/` (where `X` is a version number) +- user routes: `/vX/users/Y/` (where `X` is a version number and `Y` is a user name) + +Only users who belong to the group named "administrators" in Magpie will have access to +the admin routes. Only users whose Magpie user name matches the `Y` in user routes will +have access to the given user route. + +Authn/z can be configured with the following environment variables: + +- `MARBLE_API_MAGPIE_AUTH_ENABLED` + - default: `True` + - type: boolean + - set to `False` to disable authentication entirely (this is not recommended in a production environment) +- `MARBLE_API_MAGPIE_URL` + - default: `None` + - type: string (URL format) + - set to the URL for the Magpie instance used to authenticate users +- `MARBLE_API_MAGPIE_ADMIN_GROUP` + - default: `administrators` + - type: string + - change this if you want a different Magpie group to be have access to the admin routes When integrating Marble API with the [birdhouse](https://github.com/bird-house/birdhouse-deploy/) platform we recommend enabling it with the [Marble API component](https://github.com/DACCS-Climate/marble-config/tree/main/components/marble-api). -This enables the basic authn/z rules described above through [Magpie](https://github.com/ouranosinc/magpie). +This sets default environment variables that will work with most birdhouse deployments. ## Developing @@ -28,7 +45,7 @@ To start a development server: ```sh python3 -m pip install .[dev] -MONGODB_URI="mongodb://localhost:27017" fastapi dev marble_api +MARBLE_API_MONGODB_URI="mongodb://localhost:27017" fastapi dev marble_api ``` This assumes that you have a mongodb service running at `mongodb://localhost:27017`. @@ -86,7 +103,7 @@ To run tests: ```sh python3 -m pip install .[dev] -MONGODB_URI="mongodb://localhost:27017" pytest ./test +MARBLE_API_MONGODB_URI="mongodb://localhost:27017" pytest ./test ``` This assumes that you have a mongodb service running at `mongodb://localhost:27017`. diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index e1d5f3d..ae5dd3c 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -3,11 +3,17 @@ services: image: python:3.13-alpine volumes: - .:/app + - marble-api-venv:/marble-api/venv working_dir: /app - command: ["sh", "-c", "pip install -e .[dev,test] && fastapi dev marble_api --host 0.0.0.0"] + command: ["sh", "-c", "python -m venv /marble-api/venv && pip install -e .[dev,test] && fastapi dev marble_api --host 0.0.0.0"] environment: - - MONGODB_URI=mongodb://mongo:27017 + - PATH=/marble-api/venv/bin:/usr/local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin + - MARBLE_API_MONGODB_URI=mongodb://mongo:27017 + - MARBLE_API_MAGPIE_AUTH_ENABLED=false ports: - 8000:8000 mongo: image: mongo:5.0.4 + +volumes: + marble-api-venv: diff --git a/docker-compose.yml b/docker-compose.yml index bc35c87..572c907 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -2,7 +2,7 @@ services: marble_api: image: marbleclimate/marble-api:latest environment: - - MONGODB_URI=mongodb://mongo:27017 + - MARBLE_API_MONGODB_URI=mongodb://mongo:27017 ports: - 8000:8000 mongo: diff --git a/marble_api/_config.py b/marble_api/_config.py new file mode 100644 index 0000000..ef388ee --- /dev/null +++ b/marble_api/_config.py @@ -0,0 +1,24 @@ +from typing import Self + +from pydantic import HttpUrl, MongoDsn, model_validator +from pydantic_settings import BaseSettings, SettingsConfigDict + + +class Config(BaseSettings): + mongodb_uri: MongoDsn + magpie_auth_enabled: bool = True + magpie_url: HttpUrl | None = None + magpie_admin_group: str = "administrators" + + model_config = SettingsConfigDict(env_prefix="marble_api_") + + @model_validator(mode="after") + def require_auth_settings(self) -> Self: + if self.magpie_auth_enabled: + not_set = [field for field, value in self if field.startswith("magpie_") and not value] + if not_set: + raise ValueError(f"The following fields are required if 'magpie_auth_enabled' is set: {not_set}") + return self + + +config = Config() diff --git a/marble_api/app.py b/marble_api/app.py index 55cbf02..b151e30 100644 --- a/marble_api/app.py +++ b/marble_api/app.py @@ -2,7 +2,7 @@ from fastapi import FastAPI, Request -from marble_api.versions.v1.app import router as v1_router +from marble_api.versions.v1 import router as v1_router _metadata = metadata.metadata("marble_api").json diff --git a/marble_api/database/__init__.py b/marble_api/database/__init__.py index e494b44..9e65410 100644 --- a/marble_api/database/__init__.py +++ b/marble_api/database/__init__.py @@ -1,8 +1,8 @@ -import os - from pymongo import AsyncMongoClient from pymongo.asynchronous.database import AsyncDatabase +from marble_api._config import config + class Client(AsyncMongoClient): """AsyncMongoClient with different defaults.""" @@ -17,4 +17,4 @@ def db(self) -> AsyncDatabase: return self.get_default_database() -client = Client(os.environ["MONGODB_URI"], tz_aware=True) +client = Client(str(config.mongodb_uri), tz_aware=True) diff --git a/marble_api/utils/auth.py b/marble_api/utils/auth.py new file mode 100644 index 0000000..f092df1 --- /dev/null +++ b/marble_api/utils/auth.py @@ -0,0 +1,47 @@ +from collections.abc import Callable +from functools import wraps +from typing import Any + +import httpx +from fastapi import HTTPException, Request + +from marble_api._config import config + + +def _if_auth_enabled(func: Callable) -> Callable: + """Only run function if magpie authentication is enabled.""" + + @wraps(func) + async def _(*args, **kwargs) -> Any: # noqa: ANN401 + if config.magpie_auth_enabled: + return await func(*args, **kwargs) + + return _ + + +async def _get_authenticated_magpie_user_session(cookies: dict[str, str]) -> dict[str, Any]: + async with httpx.AsyncClient(cookies=cookies) as client: + try: + response = await client.get(f"{config.magpie_url}/session") + except httpx.HTTPError: + raise HTTPException(status_code=403, detail="Forbidden: unable to authenticate") + json = response.json() + if response.status_code == 200 and json["authenticated"]: + return json + raise HTTPException(status_code=403, detail="Forbidden") + + +@_if_auth_enabled +async def authenticate_magpie_user(request: Request, user: str) -> None: + """Raise exception if the user's username does not match the request path.""" + magpie_session = await _get_authenticated_magpie_user_session(request.cookies) + if magpie_session.get("user", {}).get("user_name") != user: + raise HTTPException(status_code=403, detail="Forbidden") + + +@_if_auth_enabled +async def authenticate_magpie_admin(request: Request) -> None: + """Raise exception if the user is not part of the admin group.""" + magpie_session = await _get_authenticated_magpie_user_session(request.cookies) + if config.magpie_admin_group not in magpie_session.get("user", {}).get("group_names", []): + raise HTTPException(status_code=403, detail="Forbidden") diff --git a/marble_api/versions/v1/__init__.py b/marble_api/versions/v1/__init__.py index e69de29..8b822e2 100644 --- a/marble_api/versions/v1/__init__.py +++ b/marble_api/versions/v1/__init__.py @@ -0,0 +1,16 @@ +from fastapi import APIRouter, Depends + +from marble_api.utils.auth import authenticate_magpie_admin, authenticate_magpie_user +from marble_api.versions.v1.data_request.routes import admin_router as data_request_admin_router +from marble_api.versions.v1.data_request.routes import user_router as data_request_user_router + +router = APIRouter(prefix="/v1") + +user_router = APIRouter(prefix="/users/{user}", tags=["User"], dependencies=[Depends(authenticate_magpie_user)]) +admin_router = APIRouter(prefix="/admin", tags=["Admin"], dependencies=[Depends(authenticate_magpie_admin)]) + +user_router.include_router(data_request_user_router) +admin_router.include_router(data_request_admin_router) + +router.include_router(user_router) +router.include_router(admin_router) diff --git a/marble_api/versions/v1/app.py b/marble_api/versions/v1/app.py deleted file mode 100644 index 6948f77..0000000 --- a/marble_api/versions/v1/app.py +++ /dev/null @@ -1,9 +0,0 @@ -from fastapi import APIRouter - -from marble_api.versions.v1.data_request.routes import admin_router as data_request_admin_router -from marble_api.versions.v1.data_request.routes import user_router as data_request_user_router - -router = APIRouter(prefix="/v1") - -router.include_router(data_request_user_router) -router.include_router(data_request_admin_router) diff --git a/marble_api/versions/v1/data_request/routes.py b/marble_api/versions/v1/data_request/routes.py index 335e6a5..12466a1 100644 --- a/marble_api/versions/v1/data_request/routes.py +++ b/marble_api/versions/v1/data_request/routes.py @@ -25,20 +25,14 @@ async def _handle_serialization_error() -> AsyncGenerator[None]: raise HTTPException(status_code=422, detail=str(e)) from e -user_router = APIRouter(prefix="/users/{user}/data-requests", tags=["User"]) -admin_router = APIRouter( - prefix="/admin/data-requests", tags=["Admin"], dependencies=[Depends(_handle_serialization_error)] -) +user_router = APIRouter(prefix="/data-requests") +admin_router = APIRouter(prefix="/data-requests", dependencies=[Depends(_handle_serialization_error)]) def _data_request_id(id_: str) -> ObjectId: return object_id(id_, HTTPException(status_code=404, detail=f"data publish request with id={id_} not found")) -def _is_router_scope(request: Request, router: APIRouter) -> bool: - return request.scope.get("route").path.startswith(f"{router.prefix}/") - - @user_router.post("/") @admin_router.post("/") async def post_data_request_user(user: str, data_request: DataRequest) -> DataRequestPublic: @@ -51,18 +45,21 @@ async def post_data_request_user(user: str, data_request: DataRequest) -> DataRe return new_data_request -@user_router.patch("/{request_id}") +def _check_user_change(data_request: DataRequestUpdate, user: str | None = None) -> None: + """Users cannot change the data request so that it belongs to a different user.""" + updated_fields = data_request.model_dump(exclude_unset=True, by_alias=True) + if updated_fields.get("user") and user != updated_fields.get("user"): + raise HTTPException(status_code=403, detail="Forbidden") + + +@user_router.patch("/{request_id}", dependencies=[Depends(_check_user_change)]) @admin_router.patch("/{request_id}") async def patch_data_request( - request_id: str, data_request: DataRequestUpdate, request: Request, user: str | None = None + request_id: str, data_request: DataRequestUpdate, user: str | None = None ) -> DataRequestPublic: """Update fields of data request and return the updated data request.""" updated_fields = data_request.model_dump(exclude_unset=True, by_alias=True) - updated_user = updated_fields.get("user") - if updated_user and _is_router_scope(request, user_router) and user != updated_user: - # Users cannot change the data request so that it belongs to a different user - raise HTTPException(status_code=403, detail="Forbidden") - if user: + if user is not None: data_request.user = user selector = {"_id": _data_request_id(request_id)} # updated timestamps are handled automatically @@ -82,12 +79,10 @@ async def patch_data_request( @user_router.get("/{request_id}", response_model_by_alias=False) @admin_router.get("/{request_id}", response_model_by_alias=False) -async def get_data_request( - request_id: str, request: Request, stac: bool = False, user: str | None = None -) -> DataRequestPublic: +async def get_data_request(request_id: str, stac: bool = False, user: str | None = None) -> DataRequestPublic: """Get a data request with the given request_id.""" selector = {"_id": _data_request_id(request_id)} - if _is_router_scope(request, user_router): + if user is not None: selector["user"] = user if (result := await client.db["data-request"].find_one(selector)) is not None: if stac: @@ -105,7 +100,7 @@ async def get_data_request( async def delete_data_request(request_id: str, request: Request, user: str | None = None) -> Response: """Delete a data request with the given request_id.""" selector = {"_id": _data_request_id(request_id)} - if _is_router_scope(request, user_router): + if user is not None: selector["user"] = user result = await client.db["data-request"].delete_one(selector) @@ -139,7 +134,7 @@ async def get_data_requests( selector = {} - if _is_router_scope(request, user_router): + if user is not None: selector["user"] = user data_requests, links = await paginated_query( diff --git a/pyproject.toml b/pyproject.toml index 7e79e15..3818fb3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,13 +22,15 @@ dependencies = [ "pymongo~=4.14", "geojson-pydantic~=2.0", "stac-pydantic~=3.4", - "pydantic[email]~=2.11" + "pydantic[email]~=2.11", + "pydantic-settings~=2.14", + "httpx~=0.28" ] [project.optional-dependencies] dev = ["ruff~=0.13", "pre-commit~=4.3", "fastapi[standard]"] prod = ["uvicorn~=0.34"] -test = ["pytest~=8.4", "faker~=37.8", "pystac[validation]~=1.14", "httpx~=0.28"] +test = ["pytest~=8.4", "faker~=37.8", "pystac[validation]~=1.14", "respx~=0.23"] [tool.ruff] line-length = 120 diff --git a/test/conftest.py b/test/conftest.py index 9ce3eca..dee4ae8 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -1,6 +1,14 @@ +import os + +# set this to false by default so that tests don't require magpie authentication +# unless explicitly set during the tests +os.environ["MARBLE_API_MAGPIE_AUTH_ENABLED"] = "false" + import pytest from faker_providers import DataRequestProvider, GeoJsonProvider +from marble_api._config import config + @pytest.fixture(scope="session") def anyio_backend(): @@ -10,3 +18,13 @@ def anyio_backend(): @pytest.fixture(scope="session") def faker_providers(): return {"DataRequestProvider": DataRequestProvider, "GeoJsonProvider": GeoJsonProvider} + + +@pytest.fixture +def test_config(): + prev = config.model_dump() + try: + yield config + finally: + for k, v in prev.items(): + setattr(config, k, v) diff --git a/test/integration/conftest.py b/test/integration/conftest.py index 875f1f6..4a316c8 100644 --- a/test/integration/conftest.py +++ b/test/integration/conftest.py @@ -36,3 +36,16 @@ async def refresh_database(request): async def async_client(): async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: yield client + + +@pytest.fixture +async def enable_magpie_auth(test_config): + test_config.magpie_auth_enabled = True + test_config.magpie_admin_group = "admin_test" + test_config.magpie_url = "http://localhost/magpie" + yield + + +@pytest.fixture +async def auth_mock(enable_magpie_auth, respx_mock, test_config): + yield respx_mock.get(f"{test_config.magpie_url}/session") diff --git a/test/integration/versions/v1/data_request/test_routes.py b/test/integration/versions/v1/data_request/test_routes.py index 3693374..b400556 100644 --- a/test/integration/versions/v1/data_request/test_routes.py +++ b/test/integration/versions/v1/data_request/test_routes.py @@ -1,9 +1,11 @@ import datetime import inspect import json +from typing import Literal from urllib.parse import parse_qs, urlparse import bson +import httpx import pytest from stac_pydantic import Item @@ -21,28 +23,88 @@ def compare_no_timestamps(dict1, dict2, message=""): }, message -class _TestUser: +class _TestAuthEnabled: + request_method: str + request_path_type: Literal["member", "collection"] + auth_type: Literal["user", "admin"] + + @pytest.fixture + async def make_request(self, async_client, member_route, collection_route): + route = member_route if self.request_path_type == "member" else collection_route + return getattr(async_client, self.request_method.lower())(route) + + async def test_authenticate_success(self, auth_mock, data_requests, make_request, test_config): + if self.auth_type == "user": + auth_mock.mock( + return_value=httpx.Response( + 200, json={"authenticated": True, "user": {"user_name": data_requests[0]["user"]}} + ) + ) + else: + auth_mock.mock( + return_value=httpx.Response( + 200, json={"authenticated": True, "user": {"group_names": [test_config.magpie_admin_group]}} + ) + ) + response = await make_request + # note that 422 is ok here because we're just checking that the authentication worked, not that the + # request was well formed otherwise + assert response.status_code < 300 or response.status_code == 422 + + async def test_authenticate_failure_unauthorized(self, auth_mock, make_request): + auth_mock.mock(return_value=httpx.Response(200, json={"authenticated": False})) + response = await make_request + assert response.status_code == 403 + + async def test_authenticate_failure_upstream(self, auth_mock, make_request): + auth_mock.mock(return_value=httpx.Response(500, json={})) + response = await make_request + assert response.status_code == 403 + + +class _TestUser(_TestAuthEnabled): + auth_type: str = "user" + @pytest.fixture def member_route(self, data_requests): - return f"/v1/users/{data_requests[0]['user']}/data-requests/{data_requests[0]['_id']}" + return ( + f"/v1/users/{data_requests[0]['user']}/data-requests/{data_requests[0].get('_id', 'should not get here')}" + ) @pytest.fixture def collection_route(self, data_requests): return f"/v1/users/{data_requests[0]['user']}/data-requests/" + async def test_authenticate_failure_different_user(self, auth_mock, data_requests, make_request): + auth_mock.mock( + return_value=httpx.Response( + 200, json={"authenticated": True, "user": {"user_name": data_requests[0]["user"] + "suffix"}} + ) + ) + response = await make_request + assert response.status_code == 403 + + +class _TestAdmin(_TestAuthEnabled): + auth_type: str = "admin" -class _TestAdmin: @pytest.fixture def member_route(self, data_requests): - return f"/v1/admin/data-requests/{data_requests[0]['_id']}" + return f"/v1/admin/data-requests/{data_requests[0].get('_id', 'should not get here')}" @pytest.fixture def collection_route(self): return "/v1/admin/data-requests/" + async def test_authenticate_failure_not_admin(self, auth_mock, make_request): + auth_mock.mock(return_value=httpx.Response(200, json={"authenticated": True, "user": {"group_names": []}})) + response = await make_request + assert response.status_code == 403 + class _TestGet: n_data_requests = 2 + request_method = "get" @pytest.fixture(scope="class", autouse=True) @classmethod @@ -66,7 +128,9 @@ async def data_requests(cls): yield await client.db.get_collection("data-request").find({}).to_list() -class _TestGetOne(_TestGet): +class _TestGetOne(_TestGet, _TestAuthEnabled): + request_path_type = "member" + async def test_get(self, async_client, data_requests, member_route): resp = await async_client.get(member_route) assert resp.status_code == 200 @@ -97,6 +161,7 @@ class TestGetOneAdmin(_TestGetOne, _TestAdmin): ... class _TestGetMany(_TestGet): + request_path_type = "collection" default_link_limit = inspect.signature(get_data_requests).parameters["limit"].default n_data_requests = default_link_limit * 2 + 2 n_data_requests_return_count: int @@ -255,6 +320,9 @@ async def test_get_all(self, async_client, collection_route): class _TestPost: + request_method = "post" + request_path_type = "collection" + @pytest.fixture def data_requests(self): return [{"user": "user1"}] @@ -293,7 +361,9 @@ def collection_route(self, data_requests): class _TestUpdate: - @pytest.fixture + request_path_type = "member" + + @pytest.fixture(autouse=True) async def loaded_data(self, fake): model = json.loads(fake.data_request().model_dump_json()) resp = await client.db.get_collection("data-request").insert_one(model) @@ -307,6 +377,8 @@ async def data_requests(self, loaded_data): class _TestPatch(_TestUpdate): + request_method = "patch" + async def test_valid(self, loaded_data, async_client, fake, member_route): title = fake.sentence() update = {"title": title} @@ -409,6 +481,8 @@ async def test_update_user(self, loaded_data, async_client, member_route): class _TestDelete(_TestUpdate): + request_method = "delete" + async def test_exists(self, loaded_data, async_client, member_route): response = await async_client.delete(member_route) assert response.status_code == 204