From 33aace3b0d2d76ebfd83863fd22bbf32e63c050d Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Tue, 17 Mar 2026 13:18:57 +0000 Subject: [PATCH 1/2] NRL-1995 Access control for allow_supersede_with_delete_failure --- .../create_document_reference.py | 5 +- .../tests/test_create_document_reference.py | 141 ++++++++++++++++- .../tests/test_upsert_document_reference.py | 143 +++++++++++++++++- .../upsert_document_reference.py | 5 +- layer/nrlf/core/constants.py | 1 + layer/nrlf/core/dynamodb/repository.py | 8 + 6 files changed, 294 insertions(+), 9 deletions(-) diff --git a/api/producer/createDocumentReference/create_document_reference.py b/api/producer/createDocumentReference/create_document_reference.py index 4e0deba1c..1fe2aeadf 100644 --- a/api/producer/createDocumentReference/create_document_reference.py +++ b/api/producer/createDocumentReference/create_document_reference.py @@ -254,7 +254,10 @@ def handler( return error_response can_ignore_delete_fail = ( - PERMISSION_SUPERSEDE_IGNORE_DELETE_FAIL in metadata.nrl_permissions + AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value + in metadata.nrl_permissions_policy.access_controls + if metadata.nrl_permissions_policy + else PERMISSION_SUPERSEDE_IGNORE_DELETE_FAIL in metadata.nrl_permissions ) if ids_to_delete := _get_document_ids_to_supersede( diff --git a/api/producer/createDocumentReference/tests/test_create_document_reference.py b/api/producer/createDocumentReference/tests/test_create_document_reference.py index 079069f97..b92cd4922 100644 --- a/api/producer/createDocumentReference/tests/test_create_document_reference.py +++ b/api/producer/createDocumentReference/tests/test_create_document_reference.py @@ -1435,7 +1435,7 @@ def test_create_document_reference_supersede_deletes_old_pointers_replace( @mock_aws @mock_repository @freeze_uuid("00000000-0000-0000-0000-000000000001") -def test_create_document_reference_supersede_succeeds_with_toggle( +def test_supersede_non_existent_pointer_succeeds_with_v1_ignore_delete_fail( repository: DocumentPointerRepository, ): doc_ref = load_document_reference("Y05868-736253002-Valid") @@ -1493,7 +1493,7 @@ def test_create_document_reference_supersede_succeeds_with_toggle( @mock_aws @mock_repository -def test_create_document_reference_supersede_fails_without_toggle( +def test_supersede_non_existent_pointer_fails_without_v1_ignore_delete_fail( repository: DocumentPointerRepository, ): doc_ref = load_document_reference("Y05868-736253002-Valid") @@ -1544,6 +1544,143 @@ def test_create_document_reference_supersede_fails_without_toggle( } +@mock_aws +@mock_repository +@freeze_uuid("00000000-0000-0000-0000-000000000001") +@patch("nrlf.core.decorators.get_pointer_permissions_v2") +def test_supersede_non_existent_pointer_succeeds_with_v2_access_control( + get_pointer_permissions_mock, repository: DocumentPointerRepository +): + doc_ref = load_document_reference("Y05868-736253002-Valid") + + # Add reference to a non-existing pointer + doc_ref.relatesTo = [ + DocumentReferenceRelatesTo( + code="replaces", + target=Reference(identifier=Identifier(value="Y05868-99999-99999-000000")), + ) + ] + + v2_headers = create_headers( + additional_headers={ + V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868", + V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678", + } + ) + v2_headers.pop(CLIENT_RP_DETAILS) + + get_pointer_permissions_mock.return_value = { + "access_controls": [AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value], + "types": ["http://snomed.info/sct|736253002"], + } + + event = create_test_api_gateway_event( + headers=v2_headers, + body=doc_ref.model_dump_json(exclude_none=True), + ) + + result = handler(event, create_mock_context()) + body = result.pop("body") + + assert result == { + "statusCode": "201", + "headers": { + "Location": "/DocumentReference/Y05868-00000000-0000-0000-0000-000000000001", + **default_response_headers(), + }, + "isBase64Encoded": False, + } + + parsed_body = json.loads(body) + + assert parsed_body == { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "information", + "code": "informational", + "details": { + "coding": [ + { + "code": "RESOURCE_SUPERSEDED", + "display": "Resource created and resource(s) deleted", + "system": "https://fhir.nhs.uk/ValueSet/NRL-ResponseCode", + } + ] + }, + "diagnostics": "The document has been superseded by a new version", + } + ], + } + + +@mock_aws +@mock_repository +@patch("nrlf.core.decorators.get_pointer_permissions_v2") +def test_supersede_fails_without_v2_access_control( + get_pointer_permissions_mock, repository: DocumentPointerRepository +): + doc_ref = load_document_reference("Y05868-736253002-Valid") + + # Add reference to a non-existing pointer + doc_ref.relatesTo = [ + DocumentReferenceRelatesTo( + code="replaces", + target=Reference(identifier=Identifier(value="Y05868-99999-99999-000000")), + ) + ] + + v2_headers = create_headers( + additional_headers={ + V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868", + V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678", + } + ) + v2_headers.pop(CLIENT_RP_DETAILS) + + get_pointer_permissions_mock.return_value = { + "access_controls": [], + "types": ["http://snomed.info/sct|736253002"], + } + + event = create_test_api_gateway_event( + headers=v2_headers, + body=doc_ref.model_dump_json(exclude_none=True), + ) + + result = handler(event, create_mock_context()) + body = result.pop("body") + + assert result == { + "statusCode": "422", + "headers": default_response_headers(), + "isBase64Encoded": False, + } + + parsed_body = json.loads(body) + + assert parsed_body == { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "error", + "code": "business-rule", + "details": { + "coding": [ + { + "code": "UNPROCESSABLE_ENTITY", + "display": "Unprocessable Entity", + "system": "https://fhir.nhs.uk/CodeSystem/Spine-ErrorOrWarningCode", + } + ] + }, + "diagnostics": "The relatesTo target document does not exist", + "expression": ["relatesTo[0].target.identifier.value"], + } + ], + } + + @mock_aws @mock_repository @freeze_uuid("00000000-0000-0000-0000-000000000001") diff --git a/api/producer/upsertDocumentReference/tests/test_upsert_document_reference.py b/api/producer/upsertDocumentReference/tests/test_upsert_document_reference.py index 10f971db4..f359a5f4b 100644 --- a/api/producer/upsertDocumentReference/tests/test_upsert_document_reference.py +++ b/api/producer/upsertDocumentReference/tests/test_upsert_document_reference.py @@ -1411,7 +1411,7 @@ def test_upsert_document_reference_supersede_deletes_old_pointers_replace( @mock_aws @mock_repository -def test_upsert_document_reference_supersede_succeeds_with_toggle( +def test_supersede_non_existent_pointer_succeeds_with_v1_ignore_delete_fail( repository: DocumentPointerRepository, ): doc_ref = load_document_reference("Y05868-736253002-Valid") @@ -1463,13 +1463,10 @@ def test_upsert_document_reference_supersede_succeeds_with_toggle( ], } - non_existent_pointer = repository.get_by_id("Y05868-99999-99999-000000") - assert non_existent_pointer is None - @mock_aws @mock_repository -def test_upsert_document_reference_supersede_fails_without_toggle( +def test_supersede_non_existent_pointer_fails_without_v1_ignore_delete_fail( repository: DocumentPointerRepository, ): doc_ref = load_document_reference("Y05868-736253002-Valid") @@ -1520,6 +1517,142 @@ def test_upsert_document_reference_supersede_fails_without_toggle( } +@mock_aws +@mock_repository +@patch("nrlf.core.decorators.get_pointer_permissions_v2") +def test_supersede_non_existent_pointer_succeeds_with_v2_access_control( + get_pointer_permissions_mock, repository: DocumentPointerRepository +): + doc_ref = load_document_reference("Y05868-736253002-Valid") + + # Add reference to a non-existing pointer + doc_ref.relatesTo = [ + DocumentReferenceRelatesTo( + code="replaces", + target=Reference(identifier=Identifier(value="Y05868-99999-99999-000000")), + ) + ] + + v2_headers = create_headers( + additional_headers={ + V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868", + V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678", + } + ) + v2_headers.pop(CLIENT_RP_DETAILS) + + get_pointer_permissions_mock.return_value = { + "access_controls": [AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value], + "types": ["http://snomed.info/sct|736253002"], + } + + event = create_test_api_gateway_event( + headers=v2_headers, + body=doc_ref.model_dump_json(exclude_none=True), + ) + + result = handler(event, create_mock_context()) + body = result.pop("body") + + assert result == { + "statusCode": "201", + "headers": { + "Location": "/DocumentReference/Y05868-99999-99999-999999", + **default_response_headers(), + }, + "isBase64Encoded": False, + } + + parsed_body = json.loads(body) + + assert parsed_body == { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "information", + "code": "informational", + "details": { + "coding": [ + { + "code": "RESOURCE_SUPERSEDED", + "display": "Resource created and resource(s) deleted", + "system": "https://fhir.nhs.uk/ValueSet/NRL-ResponseCode", + } + ] + }, + "diagnostics": "The document has been superseded by a new version", + } + ], + } + + +@mock_aws +@mock_repository +@patch("nrlf.core.decorators.get_pointer_permissions_v2") +def test_supersede_fails_without_v2_access_control( + get_pointer_permissions_mock, repository: DocumentPointerRepository +): + doc_ref = load_document_reference("Y05868-736253002-Valid") + + # Add reference to a non-existing pointer + doc_ref.relatesTo = [ + DocumentReferenceRelatesTo( + code="replaces", + target=Reference(identifier=Identifier(value="Y05868-99999-99999-000000")), + ) + ] + + v2_headers = create_headers( + additional_headers={ + V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868", + V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678", + } + ) + v2_headers.pop(CLIENT_RP_DETAILS) + + get_pointer_permissions_mock.return_value = { + "access_controls": [], + "types": ["http://snomed.info/sct|736253002"], + } + + event = create_test_api_gateway_event( + headers=v2_headers, + body=doc_ref.model_dump_json(exclude_none=True), + ) + + result = handler(event, create_mock_context()) + body = result.pop("body") + + assert result == { + "statusCode": "422", + "headers": default_response_headers(), + "isBase64Encoded": False, + } + + parsed_body = json.loads(body) + + assert parsed_body == { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "error", + "code": "business-rule", + "details": { + "coding": [ + { + "code": "UNPROCESSABLE_ENTITY", + "display": "Unprocessable Entity", + "system": "https://fhir.nhs.uk/CodeSystem/Spine-ErrorOrWarningCode", + } + ] + }, + "diagnostics": "The relatesTo target document does not exist", + "expression": ["relatesTo[0].target.identifier.value"], + } + ], + } + + @mock_aws @mock_repository def test_upsert_document_reference_create_relatesto_not_replaces( diff --git a/api/producer/upsertDocumentReference/upsert_document_reference.py b/api/producer/upsertDocumentReference/upsert_document_reference.py index c8b5a122c..dd9e87c6c 100644 --- a/api/producer/upsertDocumentReference/upsert_document_reference.py +++ b/api/producer/upsertDocumentReference/upsert_document_reference.py @@ -258,7 +258,10 @@ def handler( return error_response can_ignore_delete_fail = ( - PERMISSION_SUPERSEDE_IGNORE_DELETE_FAIL in metadata.nrl_permissions + AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value + in metadata.nrl_permissions_policy.access_controls + if metadata.nrl_permissions_policy + else PERMISSION_SUPERSEDE_IGNORE_DELETE_FAIL in metadata.nrl_permissions ) if ids_to_delete := _get_document_ids_to_supersede( diff --git a/layer/nrlf/core/constants.py b/layer/nrlf/core/constants.py index 9ef41b69c..2fa26e088 100644 --- a/layer/nrlf/core/constants.py +++ b/layer/nrlf/core/constants.py @@ -58,6 +58,7 @@ class AccessControls(Enum): ALLOW_PRODUCE_FOR_ANY_AUTHOR = "allow_produce_for_any_author" ALLOW_PRODUCE_FOR_ANY_CUSTODIAN = "allow_produce_for_any_custodian" ALLOW_OVERRIDE_CREATION_DATETIME = "allow_override_creation_datetime" + ALLOW_SUPERSEDE_WITH_DELETE_FAILURE = "allow_supersede_with_delete_failure" @staticmethod def list(): diff --git a/layer/nrlf/core/dynamodb/repository.py b/layer/nrlf/core/dynamodb/repository.py index 26a36deef..f984edbbe 100644 --- a/layer/nrlf/core/dynamodb/repository.py +++ b/layer/nrlf/core/dynamodb/repository.py @@ -352,6 +352,14 @@ def delete_by_id(self, id_: str, can_ignore_delete_fail: bool = False): stacklevel=5, error=str(exc), ) + # Is this what it should be doing, do we want to change the behaviour here? + else: + raise OperationOutcomeError( + status_code="500", + severity="error", + code="exception", + details=SpineErrorConcept.from_code("INTERNAL_SERVER_ERROR"), + ) from exc def _query(self, **kwargs) -> Iterator[DocumentPointer]: """ From 8a0937fc79cc7ddf98687cb5a8ef64fd040bdb5d Mon Sep 17 00:00:00 2001 From: Sandy Forrester Date: Tue, 17 Mar 2026 17:27:11 +0000 Subject: [PATCH 2/2] NRL-1995 Add feature tests --- layer/nrlf/core/dynamodb/repository.py | 14 ++-- scripts/get_s3_permissions.py | 1 + .../v2-permissions-access-controls.feature | 67 +++++++++++++++++++ 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/layer/nrlf/core/dynamodb/repository.py b/layer/nrlf/core/dynamodb/repository.py index f984edbbe..0700b9752 100644 --- a/layer/nrlf/core/dynamodb/repository.py +++ b/layer/nrlf/core/dynamodb/repository.py @@ -353,13 +353,13 @@ def delete_by_id(self, id_: str, can_ignore_delete_fail: bool = False): error=str(exc), ) # Is this what it should be doing, do we want to change the behaviour here? - else: - raise OperationOutcomeError( - status_code="500", - severity="error", - code="exception", - details=SpineErrorConcept.from_code("INTERNAL_SERVER_ERROR"), - ) from exc + # else: + # raise OperationOutcomeError( + # status_code="500", + # severity="error", + # code="exception", + # details=SpineErrorConcept.from_code("INTERNAL_SERVER_ERROR"), + # ) from exc def _query(self, **kwargs) -> Iterator[DocumentPointer]: """ diff --git a/scripts/get_s3_permissions.py b/scripts/get_s3_permissions.py index dbf2f2538..66f39c287 100644 --- a/scripts/get_s3_permissions.py +++ b/scripts/get_s3_permissions.py @@ -103,6 +103,7 @@ def add_feature_test_files(local_path): [ AccessControls.ALLOW_ALL_TYPES.value, AccessControls.ALLOW_OVERRIDE_CREATION_DATETIME.value, + AccessControls.ALLOW_SUPERSEDE_WITH_DELETE_FAILURE.value, ], ), ], diff --git a/tests/features/producer/v2-permissions-access-controls.feature b/tests/features/producer/v2-permissions-access-controls.feature index 716bc0966..dad872e43 100644 --- a/tests/features/producer/v2-permissions-access-controls.feature +++ b/tests/features/producer/v2-permissions-access-controls.feature @@ -91,3 +91,70 @@ Feature: Producer v2 access_control permissions - Success and Failure Scenarios | url | https://example.org/my-doc.pdf | | practiceSetting | 788002001 | And the date of the resource in the Location header is not '2024-06-01T12:00:00Z' + + Scenario: Successfully supersede a DocumentReference with ALLOW_SUPERSEDE_WITH_DELETE_FAILURE - createDocumentReference + Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API + When producer v2 '4LLTYP35P' creates a DocumentReference with values: + | property | value | + | subject | 9278693472 | + | status | current | + | type | 736253002 | + | category | 734163000 | + | custodian | 4LLTYP35P | + | author | 4LLTYP35P | + | url | https://example.org/newdoc.pdf | + | supercedes | 4LLTYP35P-000-ThisRefDoesNotExistSupersedeTest | + Then the response status code is 201 + And the response is an OperationOutcome with 1 issue + And the OperationOutcome contains the issue: + """ + { + "severity": "information", + "code": "informational", + "details": { + "coding": [ + { + "system": "https://fhir.nhs.uk/ValueSet/NRL-ResponseCode", + "code": "RESOURCE_SUPERSEDED", + "display": "Resource created and resource(s) deleted" + } + ] + }, + "diagnostics": "The document has been superseded by a new version" + } + """ + + Scenario: Supersede a DocumentReference fails without ALLOW_SUPERSEDE_WITH_DELETE_FAILURE - createDocumentReference + Given the application 'DataShare' (ID 'z00z-y11y-x22x') is registered to access the API + When producer v2 'RX898' creates a DocumentReference with values: + | property | value | + | subject | 9278693472 | + | status | current | + | type | 736373009 | + | category | 734163000 | + | custodian | RX898 | + | author | RX898 | + | url | https://example.org/newdoc.pdf | + | supercedes | RX898-000-ThisRefDoesNotExistSupersedeTest | + Then the response status code is 422 + And the response is an OperationOutcome with 1 issue + And the OperationOutcome contains the issue: + """ + { + "severity": "error", + "code": "business-rule", + "details": { + "coding": [ + { + "system": "https://fhir.nhs.uk/CodeSystem/Spine-ErrorOrWarningCode", + "code": "UNPROCESSABLE_ENTITY", + "display": "Unprocessable Entity" + } + ] + }, + "diagnostics": "The relatesTo target document does not exist", + "expression": [ + "relatesTo[0].target.identifier.value" + ] + } + """