From deb4ef8244a1568c5fd83e00b47f582cf59b2b4a Mon Sep 17 00:00:00 2001 From: Kamen Bachvarov Date: Fri, 19 Dec 2025 16:45:20 +0000 Subject: [PATCH 1/4] add file type check for review docs --- lambdas/enums/lambda_error.py | 9 ++--- .../services/post_document_review_service.py | 26 ++++++++++++++ .../test_post_document_review_service.py | 36 +++++++++++++++++++ ...st_post_fhir_document_reference_service.py | 2 +- 4 files changed, 68 insertions(+), 5 deletions(-) diff --git a/lambdas/enums/lambda_error.py b/lambdas/enums/lambda_error.py index 192fd4d303..5d6d73c95d 100644 --- a/lambdas/enums/lambda_error.py +++ b/lambdas/enums/lambda_error.py @@ -517,6 +517,10 @@ def create_error_body(self, params: Optional[dict] = None, **kwargs) -> str: "err_code": "DRV_4005", "message": "The NHS number provided is invalid", } + DocumentReviewUnsupportedFileType = { + "err_code": "DRV_4006", + "message": "The file type provided is not supported", + } """ Errors for get ods report lambda @@ -690,10 +694,7 @@ def create_error_body(self, params: Optional[dict] = None, **kwargs) -> str: "message": "Invalid request", } - DocumentReviewUploadForbidden = { - "err_code": "UDR_4031", - "message": "Forbidden" - } + DocumentReviewUploadForbidden = {"err_code": "UDR_4031", "message": "Forbidden"} DocumentReviewPresignedFailure = { "err_code": "UDR_5003", diff --git a/lambdas/services/post_document_review_service.py b/lambdas/services/post_document_review_service.py index f37a3b2305..a6c895a284 100644 --- a/lambdas/services/post_document_review_service.py +++ b/lambdas/services/post_document_review_service.py @@ -6,6 +6,7 @@ from enums.document_review_status import DocumentReviewReason, DocumentReviewStatus from enums.lambda_error import LambdaError from enums.patient_ods_inactive_status import PatientOdsInactiveStatus +from enums.upload_forbidden_file_extensions import is_file_type_allowed from models.document_review import ( DocumentReviewFileDetails, DocumentReviewUploadEvent, @@ -14,8 +15,10 @@ from pydantic import ValidationError from services.base.s3_service import S3Service from services.document_upload_review_service import DocumentUploadReviewService +from utils import upload_file_configs from utils.audit_logging_setup import LoggingService from utils.exceptions import ( + ConfigNotFoundException, DocumentReviewException, OdsErrorException, PatientNotFoundException, @@ -51,6 +54,8 @@ def process_event(self, event: DocumentReviewUploadEvent) -> dict: 403, LambdaError.DocumentReviewUploadForbidden ) + self.validate_document_file_type(event) + document_review_reference = self.create_review_reference_from_event( event=event, author=author, patient_details=patient_details ) @@ -84,6 +89,27 @@ def process_event(self, event: DocumentReviewUploadEvent) -> dict: except ClientError: raise DocumentReviewLambdaException(500, LambdaError.DocumentReviewDB) + def validate_document_file_type(self, event: DocumentReviewUploadEvent) -> None: + snomed_code = event.snomed_code.code + accepted_file_types = self.get_accepted_file_types( + snomed_code, upload_file_configs + ) + + if not is_file_type_allowed(event.documents[0], accepted_file_types): + logger.info("File type is not supported.") + raise DocumentReviewLambdaException( + 400, LambdaError.DocumentReviewUnsupportedFileType + ) + + def get_accepted_file_types(self, snomed_code, upload_file_configs): + try: + return upload_file_configs.get_config_by_snomed_code( + snomed_code + ).accepted_file_types + except ConfigNotFoundException: + logger.error(f"Unable to find config for snomed code: {snomed_code}") + raise DocumentReviewLambdaException(400, LambdaError.DocRefInvalidType) + def create_response( self, document_review_reference: DocumentUploadReviewReference ) -> dict: diff --git a/lambdas/tests/unit/services/test_post_document_review_service.py b/lambdas/tests/unit/services/test_post_document_review_service.py index e9b9728852..0593667cf4 100644 --- a/lambdas/tests/unit/services/test_post_document_review_service.py +++ b/lambdas/tests/unit/services/test_post_document_review_service.py @@ -294,3 +294,39 @@ def test_create_response(mock_service): ) ) assert actual == expected + + +@freeze_time("2024-01-01 12:00:00") +def test_validate_file_type_with_invalid_file_type(mock_extract_ods, mock_service): + mock_extract_ods.return_value = TEST_CURRENT_GP_ODS + mock_service.pds_service.fetch_patient_details.return_value = ( + EXPECTED_PARSED_PATIENT_BASE_CASE + ) + invalid_event = deepcopy(VALID_EVENT) + invalid_event.documents = ["testFile.exe"] + + with pytest.raises(DocumentReviewLambdaException) as e: + mock_service.process_event(invalid_event) + + assert e.value.status_code == 400 + assert e.value.err_code == "DRV_4006" + + mock_service.review_document_service.create_dynamo_entry.assert_not_called() + + +@freeze_time("2024-01-01 12:00:00") +def test_validate_file_type_with_invalid_snomed_code(mock_extract_ods, mock_service): + mock_extract_ods.return_value = TEST_CURRENT_GP_ODS + mock_service.pds_service.fetch_patient_details.return_value = ( + EXPECTED_PARSED_PATIENT_BASE_CASE + ) + invalid_event = deepcopy(VALID_EVENT) + invalid_event.snomed_code.code = "INVALID_SNOMED_CODE" + + with pytest.raises(DocumentReviewLambdaException) as e: + mock_service.process_event(invalid_event) + + assert e.value.status_code == 400 + assert e.value.err_code == "DR_4007" + + mock_service.review_document_service.create_dynamo_entry.assert_not_called() diff --git a/lambdas/tests/unit/services/test_post_fhir_document_reference_service.py b/lambdas/tests/unit/services/test_post_fhir_document_reference_service.py index c6f763b8bf..b389bd433a 100644 --- a/lambdas/tests/unit/services/test_post_fhir_document_reference_service.py +++ b/lambdas/tests/unit/services/test_post_fhir_document_reference_service.py @@ -549,7 +549,7 @@ def test_extract_author_from_fhir( ], ) def test_extract_author_from_fhir_raises_error( - mock_post_fhir_doc_ref_service, mocker, fhir_author + mock_fhir_doc_ref_base_service, mock_post_fhir_doc_ref_service, mocker, fhir_author ): """Test _extract_author_from_fhir method with malformed json returns Validation errors.""" fhir_doc = mocker.MagicMock(spec=FhirDocumentReference) From 36942c8ceae766ba4093e9a6b6b5d6376027a084 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Mon, 22 Dec 2025 17:35:49 +0000 Subject: [PATCH 2/4] [PRMP-1082] move validation into model --- .../handlers/post_document_review_handler.py | 4 +- lambdas/models/document_review.py | 20 +++++++++- .../services/post_document_review_service.py | 25 ------------ .../test_post_document_review_handler.py | 14 +++++++ .../test_post_document_review_service.py | 38 +------------------ 5 files changed, 35 insertions(+), 66 deletions(-) diff --git a/lambdas/handlers/post_document_review_handler.py b/lambdas/handlers/post_document_review_handler.py index 238e8c34e7..23055c80b8 100644 --- a/lambdas/handlers/post_document_review_handler.py +++ b/lambdas/handlers/post_document_review_handler.py @@ -10,7 +10,7 @@ from utils.decorators.ensure_env_var import ensure_environment_variables from utils.decorators.handle_lambda_exceptions import handle_lambda_exceptions from utils.decorators.set_audit_arg import set_request_context_for_logging -from utils.exceptions import InvalidNhsNumberException +from utils.exceptions import InvalidNhsNumberException, DocumentReviewException from utils.lambda_exceptions import DocumentReviewLambdaException from utils.lambda_response import ApiGatewayResponse @@ -55,7 +55,7 @@ def validate_event_body(body): event_body = DocumentReviewUploadEvent.model_validate_json(body) return event_body - except (ValidationError, InvalidNhsNumberException) as e: + except (ValidationError, InvalidNhsNumberException, DocumentReviewException) as e: logger.error(e) raise DocumentReviewLambdaException( 400, LambdaError.DocumentReviewUploadInvalidRequest diff --git a/lambdas/models/document_review.py b/lambdas/models/document_review.py index 41a366c0e4..e4e85e3728 100644 --- a/lambdas/models/document_review.py +++ b/lambdas/models/document_review.py @@ -1,12 +1,15 @@ import uuid from datetime import datetime, timezone +from typing import Self from enums.document_review_status import DocumentReviewStatus from enums.metadata_field_names import DocumentReferenceMetadataFields +from enums.upload_forbidden_file_extensions import is_file_type_allowed from enums.snomed_codes import SnomedCodes -from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator, ValidationError from pydantic.alias_generators import to_camel, to_pascal -from utils.exceptions import InvalidNhsNumberException +from utils.exceptions import InvalidNhsNumberException, ConfigNotFoundException, DocumentReviewException +from utils import upload_file_configs from utils.utilities import validate_nhs_number @@ -155,3 +158,16 @@ def check_snomed_code(cls, value) -> SnomedCodes | None: def verify_nhs_number(cls, value) -> str | None: if validate_nhs_number(value): return value + + @model_validator(mode="after") + def validate_file_extension(self) -> Self: + try: + accepted_file_types = upload_file_configs.get_config_by_snomed_code(self.snomed_code.code).accepted_file_types + + for file in self.documents: + if not is_file_type_allowed(file, accepted_file_types): + raise DocumentReviewException("Invalid file extension.") + return self + except ConfigNotFoundException: + raise DocumentReviewException("Unable to find file configuration.") + diff --git a/lambdas/services/post_document_review_service.py b/lambdas/services/post_document_review_service.py index a6c895a284..bdc92dae3f 100644 --- a/lambdas/services/post_document_review_service.py +++ b/lambdas/services/post_document_review_service.py @@ -6,7 +6,6 @@ from enums.document_review_status import DocumentReviewReason, DocumentReviewStatus from enums.lambda_error import LambdaError from enums.patient_ods_inactive_status import PatientOdsInactiveStatus -from enums.upload_forbidden_file_extensions import is_file_type_allowed from models.document_review import ( DocumentReviewFileDetails, DocumentReviewUploadEvent, @@ -15,10 +14,8 @@ from pydantic import ValidationError from services.base.s3_service import S3Service from services.document_upload_review_service import DocumentUploadReviewService -from utils import upload_file_configs from utils.audit_logging_setup import LoggingService from utils.exceptions import ( - ConfigNotFoundException, DocumentReviewException, OdsErrorException, PatientNotFoundException, @@ -54,8 +51,6 @@ def process_event(self, event: DocumentReviewUploadEvent) -> dict: 403, LambdaError.DocumentReviewUploadForbidden ) - self.validate_document_file_type(event) - document_review_reference = self.create_review_reference_from_event( event=event, author=author, patient_details=patient_details ) @@ -89,26 +84,6 @@ def process_event(self, event: DocumentReviewUploadEvent) -> dict: except ClientError: raise DocumentReviewLambdaException(500, LambdaError.DocumentReviewDB) - def validate_document_file_type(self, event: DocumentReviewUploadEvent) -> None: - snomed_code = event.snomed_code.code - accepted_file_types = self.get_accepted_file_types( - snomed_code, upload_file_configs - ) - - if not is_file_type_allowed(event.documents[0], accepted_file_types): - logger.info("File type is not supported.") - raise DocumentReviewLambdaException( - 400, LambdaError.DocumentReviewUnsupportedFileType - ) - - def get_accepted_file_types(self, snomed_code, upload_file_configs): - try: - return upload_file_configs.get_config_by_snomed_code( - snomed_code - ).accepted_file_types - except ConfigNotFoundException: - logger.error(f"Unable to find config for snomed code: {snomed_code}") - raise DocumentReviewLambdaException(400, LambdaError.DocRefInvalidType) def create_response( self, document_review_reference: DocumentUploadReviewReference diff --git a/lambdas/tests/unit/handlers/test_post_document_review_handler.py b/lambdas/tests/unit/handlers/test_post_document_review_handler.py index 7b51c77c2f..9eecbc0673 100644 --- a/lambdas/tests/unit/handlers/test_post_document_review_handler.py +++ b/lambdas/tests/unit/handlers/test_post_document_review_handler.py @@ -38,6 +38,12 @@ "documents": [], } +INVALID_EVENT_INVALID_FILE_EXTENSION = { + "nhsNumber": TEST_NHS_NUMBER, + "snomedCode": SnomedCodes.LLOYD_GEORGE.value.code, + "documents": ["testFile.job"], +} + TEST_PRESIGNED_URL_1 = "https://s3.amazonaws.com/presigned1?signature=abc123" @@ -222,6 +228,14 @@ def test_validate_event_body_throws_error_unsupported_snomed_code(invalid_event) assert e.value.err_code == "UDR_4003" +def test_validate_event_body_throws_error_unsupported_file_type(invalid_event): + invalid_event["body"] = INVALID_EVENT_UNSUPPORTED_SNOMED_CODE + with pytest.raises(DocumentReviewLambdaException) as e: + validate_event_body(invalid_event["body"]) + assert e.value.status_code == 400 + assert e.value.err_code == "UDR_4003" + + def test_lambda_handler_calls_service_with_validated_event( mock_service, context, mock_upload_document_iteration_3_enabled, valid_event ): diff --git a/lambdas/tests/unit/services/test_post_document_review_service.py b/lambdas/tests/unit/services/test_post_document_review_service.py index 0593667cf4..8576b4a764 100644 --- a/lambdas/tests/unit/services/test_post_document_review_service.py +++ b/lambdas/tests/unit/services/test_post_document_review_service.py @@ -293,40 +293,4 @@ def test_create_response(mock_service): nhs_number=TEST_NHS_NUMBER, ) ) - assert actual == expected - - -@freeze_time("2024-01-01 12:00:00") -def test_validate_file_type_with_invalid_file_type(mock_extract_ods, mock_service): - mock_extract_ods.return_value = TEST_CURRENT_GP_ODS - mock_service.pds_service.fetch_patient_details.return_value = ( - EXPECTED_PARSED_PATIENT_BASE_CASE - ) - invalid_event = deepcopy(VALID_EVENT) - invalid_event.documents = ["testFile.exe"] - - with pytest.raises(DocumentReviewLambdaException) as e: - mock_service.process_event(invalid_event) - - assert e.value.status_code == 400 - assert e.value.err_code == "DRV_4006" - - mock_service.review_document_service.create_dynamo_entry.assert_not_called() - - -@freeze_time("2024-01-01 12:00:00") -def test_validate_file_type_with_invalid_snomed_code(mock_extract_ods, mock_service): - mock_extract_ods.return_value = TEST_CURRENT_GP_ODS - mock_service.pds_service.fetch_patient_details.return_value = ( - EXPECTED_PARSED_PATIENT_BASE_CASE - ) - invalid_event = deepcopy(VALID_EVENT) - invalid_event.snomed_code.code = "INVALID_SNOMED_CODE" - - with pytest.raises(DocumentReviewLambdaException) as e: - mock_service.process_event(invalid_event) - - assert e.value.status_code == 400 - assert e.value.err_code == "DR_4007" - - mock_service.review_document_service.create_dynamo_entry.assert_not_called() + assert actual == expected \ No newline at end of file From e76bbd04aff66b0aa294b27a0959786ff3d559c2 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 23 Dec 2025 12:48:35 +0000 Subject: [PATCH 3/4] [PRMP-1082] return different error body based on validation failure --- lambdas/handlers/post_document_review_handler.py | 9 +++++++-- lambdas/models/document_review.py | 6 +++--- .../unit/handlers/test_post_document_review_handler.py | 6 +++--- lambdas/utils/exceptions.py | 4 ++++ 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/lambdas/handlers/post_document_review_handler.py b/lambdas/handlers/post_document_review_handler.py index 23055c80b8..80d20b13a3 100644 --- a/lambdas/handlers/post_document_review_handler.py +++ b/lambdas/handlers/post_document_review_handler.py @@ -10,7 +10,7 @@ from utils.decorators.ensure_env_var import ensure_environment_variables from utils.decorators.handle_lambda_exceptions import handle_lambda_exceptions from utils.decorators.set_audit_arg import set_request_context_for_logging -from utils.exceptions import InvalidNhsNumberException, DocumentReviewException +from utils.exceptions import InvalidNhsNumberException, DocumentReviewException, InvalidFileTypeException from utils.lambda_exceptions import DocumentReviewLambdaException from utils.lambda_response import ApiGatewayResponse @@ -55,8 +55,13 @@ def validate_event_body(body): event_body = DocumentReviewUploadEvent.model_validate_json(body) return event_body - except (ValidationError, InvalidNhsNumberException, DocumentReviewException) as e: + except (ValidationError, InvalidNhsNumberException) as e: logger.error(e) raise DocumentReviewLambdaException( 400, LambdaError.DocumentReviewUploadInvalidRequest ) + except InvalidFileTypeException as e: + logger.error(e) + raise DocumentReviewLambdaException( + 400, LambdaError.DocumentReviewUnsupportedFileType + ) diff --git a/lambdas/models/document_review.py b/lambdas/models/document_review.py index e4e85e3728..4d4e0fef99 100644 --- a/lambdas/models/document_review.py +++ b/lambdas/models/document_review.py @@ -8,7 +8,7 @@ from enums.snomed_codes import SnomedCodes from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator, ValidationError from pydantic.alias_generators import to_camel, to_pascal -from utils.exceptions import InvalidNhsNumberException, ConfigNotFoundException, DocumentReviewException +from utils.exceptions import InvalidNhsNumberException, ConfigNotFoundException, InvalidFileTypeException from utils import upload_file_configs from utils.utilities import validate_nhs_number @@ -166,8 +166,8 @@ def validate_file_extension(self) -> Self: for file in self.documents: if not is_file_type_allowed(file, accepted_file_types): - raise DocumentReviewException("Invalid file extension.") + raise InvalidFileTypeException("Invalid file extension.") return self except ConfigNotFoundException: - raise DocumentReviewException("Unable to find file configuration.") + raise InvalidFileTypeException("Unable to find file configuration.") diff --git a/lambdas/tests/unit/handlers/test_post_document_review_handler.py b/lambdas/tests/unit/handlers/test_post_document_review_handler.py index 9eecbc0673..715bbae49e 100644 --- a/lambdas/tests/unit/handlers/test_post_document_review_handler.py +++ b/lambdas/tests/unit/handlers/test_post_document_review_handler.py @@ -221,7 +221,7 @@ def test_validate_event_body_valid_event_returns_document_review_upload_event_mo def test_validate_event_body_throws_error_unsupported_snomed_code(invalid_event): - invalid_event["body"] = INVALID_EVENT_UNSUPPORTED_SNOMED_CODE + invalid_event["body"] = json.dumps(INVALID_EVENT_UNSUPPORTED_SNOMED_CODE) with pytest.raises(DocumentReviewLambdaException) as e: validate_event_body(invalid_event["body"]) assert e.value.status_code == 400 @@ -229,11 +229,11 @@ def test_validate_event_body_throws_error_unsupported_snomed_code(invalid_event) def test_validate_event_body_throws_error_unsupported_file_type(invalid_event): - invalid_event["body"] = INVALID_EVENT_UNSUPPORTED_SNOMED_CODE + invalid_event["body"] = json.dumps(INVALID_EVENT_INVALID_FILE_EXTENSION) with pytest.raises(DocumentReviewLambdaException) as e: validate_event_body(invalid_event["body"]) assert e.value.status_code == 400 - assert e.value.err_code == "UDR_4003" + assert e.value.err_code == "DRV_4006" def test_lambda_handler_calls_service_with_validated_event( diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index 07a4a14d33..21b1a99449 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -218,3 +218,7 @@ class ReviewProcessCreateRecordException(Exception): class CorruptedFileException(Exception): pass + + +class InvalidFileTypeException(Exception): + pass From d4f855cb16c6f5a3d436898314f63e1a80a35b66 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 24 Dec 2025 12:01:34 +0000 Subject: [PATCH 4/4] [PRMP-1082] remove unused import --- lambdas/handlers/post_document_review_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lambdas/handlers/post_document_review_handler.py b/lambdas/handlers/post_document_review_handler.py index 80d20b13a3..eef9add024 100644 --- a/lambdas/handlers/post_document_review_handler.py +++ b/lambdas/handlers/post_document_review_handler.py @@ -10,7 +10,7 @@ from utils.decorators.ensure_env_var import ensure_environment_variables from utils.decorators.handle_lambda_exceptions import handle_lambda_exceptions from utils.decorators.set_audit_arg import set_request_context_for_logging -from utils.exceptions import InvalidNhsNumberException, DocumentReviewException, InvalidFileTypeException +from utils.exceptions import InvalidNhsNumberException, InvalidFileTypeException from utils.lambda_exceptions import DocumentReviewLambdaException from utils.lambda_response import ApiGatewayResponse