diff --git a/manage_breast_screening/config/.env.tpl b/manage_breast_screening/config/.env.tpl index c56afb35b..ad37c4e15 100644 --- a/manage_breast_screening/config/.env.tpl +++ b/manage_breast_screening/config/.env.tpl @@ -39,6 +39,8 @@ API_AUTH_TOKEN=changeme # Django Ninja API API_ENABLED=true API_DOCS_ENABLED=true +BYPASS_API_AUTHORISATION=false +BYPASS_API_AUTHENTICATION=false # Automatic loading of PACS images from gateway GATEWAY_IMAGES_ENABLED=False diff --git a/manage_breast_screening/config/settings/base.py b/manage_breast_screening/config/settings/base.py index 3d1defd76..41dbb8eca 100644 --- a/manage_breast_screening/config/settings/base.py +++ b/manage_breast_screening/config/settings/base.py @@ -383,4 +383,5 @@ def list_env(key): } } -BYPASS_API_TOKEN_AUTH = boolean_env("BYPASS_API_TOKEN_AUTH", default=False) +BYPASS_API_AUTHENTICATION = boolean_env("BYPASS_API_AUTHENTICATION", default=False) +BYPASS_API_AUTHORISATION = boolean_env("BYPASS_API_AUTHORISATION", default=False) diff --git a/manage_breast_screening/core/api.py b/manage_breast_screening/core/api.py index b2038d396..1941236d1 100644 --- a/manage_breast_screening/core/api.py +++ b/manage_breast_screening/core/api.py @@ -8,15 +8,19 @@ from manage_breast_screening.dicom.api import router as dicom_router -from .api_schema import StatusResponse +from .api_schema import ErrorResponse, StatusResponse def check_availability(): def decorator(func): @wraps(func) def wrapper(request, *args, **kwargs): - if not os.getenv("API_ENABLED", "true").lower() == "true": - return 403, {"status": "API is not available"} + if os.getenv("API_ENABLED", "true").lower() != "true": + return 403, { + "title": "Forbidden", + "status": 403, + "detail": "API is not available", + } return func(request, *args, **kwargs) return wrapper @@ -58,8 +62,6 @@ def authenticate(self, request, token): dicom_router.add_decorator(check_availability()) -@api.get( - "/status", response={200: StatusResponse, 403: StatusResponse}, tags=["Status"] -) +@api.get("/status", response={200: StatusResponse, 403: ErrorResponse}, tags=["Status"]) def status(request): return 200, {"status": "API is available"} diff --git a/manage_breast_screening/core/tests/test_api.py b/manage_breast_screening/core/tests/test_api.py index e76c2def2..813af406a 100644 --- a/manage_breast_screening/core/tests/test_api.py +++ b/manage_breast_screening/core/tests/test_api.py @@ -37,7 +37,10 @@ def test_status_endpoint_api_disabled(monkeypatch): response = client.get("/status", headers={"Authorization": "Bearer testtoken"}) assert response.status_code == 403 - assert response.json() == {"status": "API is not available"} + json = response.json() + assert json["title"] == "Forbidden" + assert json["status"] == 403 + assert json["detail"] == "API is not available" def test_status_wrong_auth(monkeypatch): diff --git a/manage_breast_screening/dicom/api.py b/manage_breast_screening/dicom/api.py index bbf663e42..904dc1620 100644 --- a/manage_breast_screening/dicom/api.py +++ b/manage_breast_screening/dicom/api.py @@ -11,6 +11,7 @@ from manage_breast_screening.gateway.models import GatewayAction, GatewayActionStatus from .authentication import Authentication +from .authorisation import Authorisation from .dicom_recorder import DicomRecorder router = Router(auth=Authentication()) @@ -36,7 +37,7 @@ class FailurePayload(ninja.Schema): response={ 201: SuccessResponse, 400: ErrorResponse, - 403: StatusResponse, + 403: ErrorResponse, 500: ErrorResponse, }, ) @@ -58,6 +59,22 @@ def upload(request, source_message_id: str, file: File[UploadedFile]): "detail": "A DICOM file must be uploaded in the 'file' form field.", } + oid = request.auth.get("oid") + + if oid is None: + return 403, { + "title": "Forbidden", + "status": 403, + "detail": "Authentication failed: OID claim is missing.", + } + + if Authorisation.authorise(source_message_id, oid) is False: + return 403, { + "title": "Forbidden", + "status": 403, + "detail": "You do not have permission to upload for this message ID.", + } + try: study, series, image = DicomRecorder.get_or_create_records( source_message_id, file diff --git a/manage_breast_screening/dicom/authentication.py b/manage_breast_screening/dicom/authentication.py index c23f94447..11232573d 100644 --- a/manage_breast_screening/dicom/authentication.py +++ b/manage_breast_screening/dicom/authentication.py @@ -13,15 +13,16 @@ class Authentication(HttpBearer): - def authenticate(self, _, token) -> dict | None: + def authenticate(self, request, token) -> dict | None: """ Authenticates the incoming request by validating the JWT token. """ if self.bypass_authentication: logger.warning("Authentication bypass is enabled.") - return {"sub": "bypass_user"} + return {"oid": "bypass_object_id", "sub": "bypass_user"} - return self._decode(token) + request.auth = self._decode(token) + return request.auth def _decode(self, token: str) -> dict | None: """ @@ -93,4 +94,4 @@ def issuers(self) -> list: @property def bypass_authentication(self) -> bool: - return getattr(settings, "BYPASS_API_TOKEN_AUTH", False) + return getattr(settings, "BYPASS_API_AUTHENTICATION", False) diff --git a/manage_breast_screening/dicom/authorisation.py b/manage_breast_screening/dicom/authorisation.py new file mode 100644 index 000000000..2ef284b4c --- /dev/null +++ b/manage_breast_screening/dicom/authorisation.py @@ -0,0 +1,34 @@ +import logging +from datetime import date + +from django.conf import settings + +from manage_breast_screening.gateway.models import GatewayAction, GatewayActionStatus + +logger = logging.getLogger(__name__) + + +class Authorisation: + @staticmethod + def authorise(source_message_id: str, oid: str) -> bool: + """ + Check for the existence of a GatewayAction with the given source_message_id and oid, created today. + This provides a link between the source_message_id we send to the gateway in the appointment workflow + and the oid associated with the system assigned managed identity of the gateway stored in the Gateway model. + """ + if __class__.bypass_authorisation(): + return True + + return GatewayAction.objects.filter( + id=source_message_id, + gateway__oid=oid, + created_at__date=date.today(), + status__in=[ + GatewayActionStatus.SENT, + GatewayActionStatus.CONFIRMED, + ], + ).exists() + + @staticmethod + def bypass_authorisation() -> bool: + return getattr(settings, "BYPASS_API_AUTHORISATION", False) diff --git a/manage_breast_screening/dicom/tests/test_api.py b/manage_breast_screening/dicom/tests/test_api.py index 8a7589f09..fc2fbf91c 100644 --- a/manage_breast_screening/dicom/tests/test_api.py +++ b/manage_breast_screening/dicom/tests/test_api.py @@ -10,13 +10,17 @@ from manage_breast_screening.core.api import api from manage_breast_screening.gateway.models import GatewayActionStatus -from manage_breast_screening.gateway.tests.factories import GatewayActionFactory +from manage_breast_screening.gateway.tests.factories import ( + GatewayActionFactory, + GatewayFactory, +) from manage_breast_screening.participants.models.appointment import ( AppointmentStatusNames, ) from manage_breast_screening.participants.tests.factories import AppointmentFactory from ..authentication import Authentication +from ..authorisation import Authorisation from ..dicom_recorder import DicomRecorder from ..models import Study @@ -50,13 +54,40 @@ def appointment_stub(): @pytest.fixture -def mock_authentication(): - with patch.object(Authentication, "authenticate", return_value={"sub": "testuser"}): +def source_message_id(): + return "00000000-0000-0000-0000-000000000009" + + +@pytest.fixture +def gateway_oid(): + return "00000000-0000-0000-0000-000000000001" + + +@pytest.fixture +def gateway_action(source_message_id, gateway_oid): + return GatewayActionFactory( + id=source_message_id, + status=GatewayActionStatus.SENT, + gateway=GatewayFactory(oid=gateway_oid), + ) + + +@pytest.fixture +def mock_authentication(gateway_oid): + with patch.object( + Authentication, "_decode", return_value={"oid": gateway_oid, "sub": "testuser"} + ): + yield + + +@pytest.fixture +def mock_authorisation(gateway_oid): + with patch.object(Authorisation, "authorise", return_value=True): yield @pytest.mark.django_db -def test_upload_success(dataset, dicom_file, mock_authentication, appointment_stub): +def test_upload_success(dataset, dicom_file, mock_authentication, gateway_action): appointment = AppointmentFactory(current_status=AppointmentStatusNames.IN_PROGRESS) with patch( @@ -64,11 +95,12 @@ def test_upload_success(dataset, dicom_file, mock_authentication, appointment_st return_value=appointment, ): response = client.put( - f"/dicom/{appointment.pk}", + f"/dicom/{gateway_action.id}", FILES={"file": dicom_file}, headers={"Authorization": "Bearer testtoken"}, ) + print(response.json()) assert response.status_code == 201 json = response.json() study = Study.objects.last() @@ -76,7 +108,7 @@ def test_upload_success(dataset, dicom_file, mock_authentication, appointment_st assert json["series_instance_uid"] == dataset.SeriesInstanceUID assert json["sop_instance_uid"] == dataset.SOPInstanceUID assert json["instance_id"] == str(study.images().first().id) - assert study.source_message_id == str(appointment.pk) + assert study.source_message_id == str(gateway_action.id) def test_upload_no_file(mock_authentication): @@ -89,7 +121,9 @@ def test_upload_no_file(mock_authentication): assert response.status_code == 422 -def test_upload_invalid_file(mock_authentication, appointment_stub): +def test_upload_invalid_file( + mock_authentication, mock_authorisation, appointment_stub, source_message_id +): invalid_file = SimpleUploadedFile( "invalid.dcm", b"not a dicom file", content_type="application/dicom" ) @@ -99,7 +133,7 @@ def test_upload_invalid_file(mock_authentication, appointment_stub): return_value=appointment_stub, ): response = client.put( - "/dicom/abc123", + f"/dicom/{source_message_id}", FILES={"file": invalid_file}, headers={"Authorization": "Bearer testtoken"}, ) @@ -125,7 +159,9 @@ def test_upload_file_thats_too_large(mock_authentication): assert response.json()["detail"] == "The file cannot be larger than 100MB" -def test_upload_missing_uids(dataset, mock_authentication, appointment_stub): +def test_upload_missing_uids( + dataset, mock_authentication, mock_authorisation, appointment_stub +): del dataset.StudyInstanceUID del dataset.SeriesInstanceUID del dataset.SOPInstanceUID @@ -157,7 +193,7 @@ def test_upload_missing_uids(dataset, mock_authentication, appointment_stub): def test_upload_appointment_not_in_progress( - dicom_file, mock_authentication, appointment_stub + dicom_file, mock_authentication, mock_authorisation, appointment_stub ): appointment_stub.is_in_progress.return_value = False @@ -185,7 +221,7 @@ def test_upload_when_api_disabled(dicom_file, mock_authentication, monkeypatch): ) assert response.status_code == 403 - assert response.json()["status"] == "API is not available" + assert response.json()["detail"] == "API is not available" def test_upload_no_auth(dicom_file): @@ -213,22 +249,23 @@ def test_upload_invalid_auth(dicom_file): } -def test_upload_bypass_token_validation(dicom_file): - with patch.object(Authentication, "bypass_authentication", return_value=True): - with patch.object( - DicomRecorder, - "get_or_create_records", - return_value=( - MagicMock(study_instance_uid=generate_uid()), - MagicMock(series_instance_uid=generate_uid()), - MagicMock(sop_instance_uid=generate_uid(), id=1), - ), - ): - response = client.put( - "/dicom/abc123", - FILES={"file": dicom_file}, - headers={"Authorization": "Bearer anytoken"}, - ) +@patch.object(Authentication, "bypass_authentication", return_value=True) +@patch.object(Authorisation, "bypass_authorisation", return_value=True) +def test_upload_bypass_auth(_y, _x, dicom_file): + with patch.object( + DicomRecorder, + "get_or_create_records", + return_value=( + MagicMock(study_instance_uid=generate_uid()), + MagicMock(series_instance_uid=generate_uid()), + MagicMock(sop_instance_uid=generate_uid(), id=1), + ), + ): + response = client.put( + "/dicom/abc123", + FILES={"file": dicom_file}, + headers={"Authorization": "Bearer anytoken"}, + ) assert response.status_code == 201 diff --git a/manage_breast_screening/dicom/tests/test_authentication.py b/manage_breast_screening/dicom/tests/test_authentication.py index d8e6ee770..438bdec7c 100644 --- a/manage_breast_screening/dicom/tests/test_authentication.py +++ b/manage_breast_screening/dicom/tests/test_authentication.py @@ -94,9 +94,19 @@ def test_with_valid_token(self, _, mock_logger, mock_jwks_signing_key): } mock_logger.exception.assert_not_called() + def test_request_auth_object_is_set(self, mock_logger, mock_jwks_signing_key): + with patch( + f"{Authentication.__module__}.jwt.decode", + return_value={"oid": "test_oid", "sub": "test_user"}, + ): + authenticator = Authentication() + request = Mock(headers={"Authorization": "Bearer abc123"}) + assert authenticator(request) == {"oid": "test_oid", "sub": "test_user"} + assert request.auth == {"oid": "test_oid", "sub": "test_user"} + def test_authentication_bypass_enabled(self, mock_logger, mock_jwks_signing_key): - with patch.object(settings, "BYPASS_API_TOKEN_AUTH", return_value=True): + with patch.object(settings, "BYPASS_API_AUTHENTICATION", return_value=True): authenticator = Authentication() assert authenticator( Mock(headers={"Authorization": "Bearer anytoken"}) - ) == {"sub": "bypass_user"} + ) == {"oid": "bypass_object_id", "sub": "bypass_user"} diff --git a/manage_breast_screening/dicom/tests/test_authorisation.py b/manage_breast_screening/dicom/tests/test_authorisation.py new file mode 100644 index 000000000..855d9bf6a --- /dev/null +++ b/manage_breast_screening/dicom/tests/test_authorisation.py @@ -0,0 +1,91 @@ +import uuid +from datetime import date, timedelta +from unittest.mock import patch + +import pytest +from django.conf import settings + +from manage_breast_screening.gateway.models import GatewayActionStatus +from manage_breast_screening.gateway.tests.factories import ( + GatewayActionFactory, + GatewayFactory, +) + +from ..authorisation import Authorisation + + +@pytest.mark.django_db +class TestAuthorisation: + def test_authorisation(self): + source_message_id = str(uuid.uuid4()) + oid = str(uuid.uuid4()) + GatewayActionFactory( + id=source_message_id, + gateway=GatewayFactory(oid=oid), + created_at=date.today(), + status=GatewayActionStatus.SENT, + ) + + assert Authorisation.authorise(source_message_id, oid) is True + + def test_authorisation_no_action(self): + source_message_id = str(uuid.uuid4()) + oid = str(uuid.uuid4()) + + assert Authorisation.authorise(source_message_id, oid) is False + + def test_authorisation_wrong_status(self): + source_message_id = str(uuid.uuid4()) + oid = str(uuid.uuid4()) + GatewayActionFactory( + id=source_message_id, + gateway=GatewayFactory(oid=oid), + created_at=date.today(), + status=GatewayActionStatus.PENDING, + ) + + assert Authorisation.authorise(source_message_id, oid) is False + + def test_authorisation_old_action(self): + source_message_id = str(uuid.uuid4()) + oid = str(uuid.uuid4()) + action = GatewayActionFactory( + id=source_message_id, + gateway=GatewayFactory(oid=oid), + status=GatewayActionStatus.SENT, + ) + action.created_at = date.today() - timedelta(days=1) + action.save() + + assert Authorisation.authorise(source_message_id, oid) is False + + def test_authorisation_wrong_oid(self): + source_message_id = str(uuid.uuid4()) + oid = str(uuid.uuid4()) + GatewayActionFactory( + id=source_message_id, + gateway=GatewayFactory(oid=str(uuid.uuid4())), + created_at=date.today(), + status=GatewayActionStatus.SENT, + ) + + assert Authorisation.authorise(source_message_id, oid) is False + + def test_authorisation_no_gateway(self): + source_message_id = str(uuid.uuid4()) + oid = str(uuid.uuid4()) + GatewayActionFactory( + id=source_message_id, + gateway=None, + created_at=date.today(), + status=GatewayActionStatus.SENT, + ) + + assert Authorisation.authorise(source_message_id, oid) is False + + def test_bypass_authorisation(self, monkeypatch): + source_message_id = str(uuid.uuid4()) + oid = str(uuid.uuid4()) + + with patch.object(settings, "BYPASS_API_AUTHORISATION", return_value=True): + assert Authorisation.authorise(source_message_id, oid) is True diff --git a/manage_breast_screening/gateway/tests/factories.py b/manage_breast_screening/gateway/tests/factories.py index 951254586..abbc4cc82 100644 --- a/manage_breast_screening/gateway/tests/factories.py +++ b/manage_breast_screening/gateway/tests/factories.py @@ -4,6 +4,16 @@ from .. import models +class GatewayFactory(DjangoModelFactory): + class Meta: + model = models.Gateway + + name = Sequence(lambda n: f"Gateway {n}") + description = Sequence(lambda n: f"Description for Gateway {n}") + oid = Sequence(lambda n: f"00000000-0000-0000-0000-{n:012d}") + resource_name = Sequence(lambda n: f"gateway-resource-{n}") + + class GatewayActionFactory(DjangoModelFactory): class Meta: model = models.GatewayAction @@ -16,6 +26,7 @@ class Meta: status = models.GatewayActionStatus.PENDING accession_number = Sequence(lambda n: f"ACC20240601{n:04d}") sent_at = None + gateway = SubFactory(GatewayFactory) class RelayFactory(DjangoModelFactory): @@ -29,3 +40,4 @@ class Meta: setting = SubFactory( "manage_breast_screening.clinics.tests.factories.SettingFactory" ) + gateway = SubFactory(GatewayFactory) diff --git a/manage_breast_screening/gateway/tests/test_worklist_item_service.py b/manage_breast_screening/gateway/tests/test_worklist_item_service.py index abb2da31d..2550ae576 100644 --- a/manage_breast_screening/gateway/tests/test_worklist_item_service.py +++ b/manage_breast_screening/gateway/tests/test_worklist_item_service.py @@ -22,7 +22,7 @@ class TestWorklistItemService: def test_create_returns_gateway_action(self, _): appointment = AppointmentFactory() - RelayFactory(setting=appointment.clinic_slot.clinic.setting) + relay = RelayFactory(setting=appointment.clinic_slot.clinic.setting) action = WorklistItemService.create(appointment) @@ -31,6 +31,7 @@ def test_create_returns_gateway_action(self, _): assert action.status == GatewayActionStatus.PENDING assert action.type == GatewayActionType.WORKLIST_CREATE assert action.appointment == appointment + assert action.gateway == relay.gateway @time_machine.travel("2025-06-15 10:30:00", tick=False) def test_accession_number_format(self, _): diff --git a/manage_breast_screening/gateway/worklist_item_service.py b/manage_breast_screening/gateway/worklist_item_service.py index a967ee8ed..dbede9f1d 100644 --- a/manage_breast_screening/gateway/worklist_item_service.py +++ b/manage_breast_screening/gateway/worklist_item_service.py @@ -139,6 +139,7 @@ def _create_action(self) -> GatewayAction | None: accession_number=accession_number, payload=payload, status=GatewayActionStatus.PENDING, + gateway=relay.gateway, ) except IntegrityError as e: raise GatewayActionAlreadyExistsError(