diff --git a/CHANGELOG.md b/CHANGELOG.md index b560472..5f55df7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **The Pelican federation routes required no credentials at all.** Every route in `api/routes/pelican_routes.py` — `GET /pelican/federations`, `/browse`, `/info`, `/download` and `POST /pelican/import-metadata` — was mounted with no authentication and no authorization: the module never imported `Depends`, the router carried no `dependencies`, and the only middlewares on the application are correlation IDs and CORS, so nothing gated them anywhere along the path. Anyone able to reach the port could enumerate a namespace, stream files out of the configured federation, and attach resources to an existing package. The `PELICAN_ENABLED` flag hid the problem rather than solving it: it decides whether the router is mounted, so the routes were only closed on deployments that had Pelican switched off entirely. The router now carries a read-tier dependency covering every route it holds, and `/import-metadata`, which writes to the catalog, additionally takes the write-tier dependency used by the rest of the registration routes. The gate is declared on the router rather than repeated on each route so that a Pelican route added later inherits it instead of shipping open. + +### Backwards compatibility +- Callers of `/pelican/*` must now send a bearer token, and the authenticated user needs a viewer, writer or admin role on the endpoint — writer or admin for `/import-metadata`. An anonymous request that used to succeed now returns 401, and an authenticated user without a role tier gets 403. No web UI code calls these routes, so the admin console is unaffected; scripts and notebooks that reached them without a token need updating. Deployments running with `PELICAN_ENABLED` unset are unaffected, since the routes were never mounted there. + ## [0.34.21] - 2026-08-26 ### Removed diff --git a/api/routes/pelican_routes.py b/api/routes/pelican_routes.py index aea2e06..58bee13 100644 --- a/api/routes/pelican_routes.py +++ b/api/routes/pelican_routes.py @@ -5,7 +5,7 @@ These endpoints allow browsing and downloading from external Pelican federations. """ -from fastapi import APIRouter, HTTPException, Query, Response +from fastapi import APIRouter, Depends, HTTPException, Query, Response from fastapi.responses import StreamingResponse from pydantic import BaseModel from typing import Optional, Dict, Any @@ -16,12 +16,24 @@ ) from api.services.pelican_services.download_file import download_file, stream_file from api.services.pelican_services.import_metadata import import_file_as_resource +from api.services.auth_services import ( + get_user_for_read_operation, + get_user_for_write_operation, +) import logging import os logger = logging.getLogger(__name__) -router = APIRouter(prefix="/pelican", tags=["Pelican Federation"]) +# The read gate is declared on the router rather than on each route: these +# endpoints shipped completely unauthenticated (issue #261), and a +# router-level dependency means a route added later cannot silently miss it. +# Routes that write take the stricter write dependency on top of this one. +router = APIRouter( + prefix="/pelican", + tags=["Pelican Federation"], + dependencies=[Depends(get_user_for_read_operation)], +) # Pydantic models @@ -234,7 +246,10 @@ async def download( @router.post("/import-metadata") -async def import_metadata(request: ImportMetadataRequest): +async def import_metadata( + request: ImportMetadataRequest, + _user=Depends(get_user_for_write_operation), +): """ Import a Pelican file as a resource in the local catalog. diff --git a/tests/test_pelican_routes.py b/tests/test_pelican_routes.py index 046b6ac..dadac6b 100644 --- a/tests/test_pelican_routes.py +++ b/tests/test_pelican_routes.py @@ -152,3 +152,134 @@ async def test_import_metadata_invalid_url(self): await import_metadata(request) assert exc_info.value.status_code == 400 + + +class TestPelicanRoutesAuthorization: + """ + The Pelican routes shipped without any authentication (issue #261), so + these tests pin the gate down: no anonymous access, no access without a + role, and the write route demands more than the read routes. + """ + + @staticmethod + def _client(): + """Mount the Pelican router on a bare app, independent of + ``PELICAN_ENABLED`` and of the rest of the application.""" + from fastapi import FastAPI + from fastapi.testclient import TestClient + from api.routes.pelican_routes import router + + app = FastAPI() + app.include_router(router) + return app, TestClient(app) + + @staticmethod + def _as(roles): + """Build a ``get_current_user`` override for a user with ``roles``.""" + return lambda: { + "roles": roles, + "groups": [], + "sub": "test_user", + "username": "Test User", + } + + def test_read_route_rejects_anonymous_caller(self): + """A request with no Authorization header never reaches the route.""" + _app, client = self._client() + + response = client.get("/pelican/federations") + + assert response.status_code == 401 + + @patch("api.routes.pelican_routes.browse_namespace") + def test_browse_rejects_anonymous_caller(self, mock_browse): + """Browsing is refused before the federation is contacted.""" + _app, client = self._client() + + response = client.get("/pelican/browse", params={"path": "/public"}) + + assert response.status_code == 401 + mock_browse.assert_not_called() + + @patch("api.routes.pelican_routes.download_file") + def test_download_rejects_anonymous_caller(self, mock_download): + """Downloading is refused before any byte leaves the federation.""" + _app, client = self._client() + + response = client.get("/pelican/download", params={"path": "/public/f.txt"}) + + assert response.status_code == 401 + mock_download.assert_not_called() + + def test_read_route_rejects_user_without_role(self): + """An authenticated user with no role tier still gets 403.""" + from api.services.auth_services import get_current_user + + app, client = self._client() + app.dependency_overrides[get_current_user] = self._as([]) + try: + response = client.get("/pelican/federations") + finally: + app.dependency_overrides.clear() + + assert response.status_code == 403 + + def test_read_route_allows_viewer(self): + """A viewer may read: the gate is authorization, not a blanket block.""" + from api.services.auth_services import get_current_user + + app, client = self._client() + app.dependency_overrides[get_current_user] = self._as(["ndp_viewer"]) + try: + response = client.get("/pelican/federations") + finally: + app.dependency_overrides.clear() + + assert response.status_code == 200 + assert response.json()["success"] is True + + @patch("api.routes.pelican_routes.import_file_as_resource") + def test_import_metadata_rejects_viewer(self, mock_import): + """The write route is stricter than the read gate it sits behind.""" + from api.services.auth_services import get_current_user + + app, client = self._client() + app.dependency_overrides[get_current_user] = self._as(["ndp_viewer"]) + try: + response = client.post( + "/pelican/import-metadata", + json={ + "pelican_url": "pelican://osg-htc.org/public/f.txt", + "package_id": "pkg-1", + }, + ) + finally: + app.dependency_overrides.clear() + + assert response.status_code == 403 + mock_import.assert_not_called() + + @patch("api.routes.pelican_routes.get_pelican_repo") + @patch("api.routes.pelican_routes.import_file_as_resource") + def test_import_metadata_allows_writer(self, mock_import, mock_get_repo): + """A writer reaches the route body.""" + from api.services.auth_services import get_current_user + + mock_get_repo.return_value = MagicMock() + mock_import.return_value = {"success": True, "id": "res-1"} + + app, client = self._client() + app.dependency_overrides[get_current_user] = self._as(["ndp_editor"]) + try: + response = client.post( + "/pelican/import-metadata", + json={ + "pelican_url": "pelican://osg-htc.org/public/f.txt", + "package_id": "pkg-1", + }, + ) + finally: + app.dependency_overrides.clear() + + assert response.status_code == 200 + mock_import.assert_called_once()