From 759afd8e1bb2f5bcfbf976e5bed608516414bbc6 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 16 Jan 2026 16:54:50 +0000 Subject: [PATCH 1/7] nhs number validation fix --- .../common/request_validator.py | 4 +- .../in_process/test_eligibility_endpoint.py | 50 +++++++++++++++++++ tests/unit/common/test_request_validator.py | 16 ++++-- 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/eligibility_signposting_api/common/request_validator.py b/src/eligibility_signposting_api/common/request_validator.py index e4fe7ab76..c012861a6 100644 --- a/src/eligibility_signposting_api/common/request_validator.py +++ b/src/eligibility_signposting_api/common/request_validator.py @@ -60,8 +60,8 @@ def validate_request_params() -> Callable: def decorator(func: Callable) -> Callable: @wraps(func) def wrapper(*args, **kwargs) -> ResponseReturnValue: # noqa:ANN002,ANN003 - path_nhs_number = str(kwargs.get("nhs_number")) - header_nhs_no = str(request.headers.get(NHS_NUMBER_HEADER)) + path_nhs_number = str(kwargs.get("nhs_number")) if kwargs.get("nhs_number") else None + header_nhs_no = str(request.headers.get(NHS_NUMBER_HEADER)) if request.headers.get(NHS_NUMBER_HEADER) else None if not validate_nhs_number(path_nhs_number, header_nhs_no): message = "You are not authorised to request information for the supplied NHS Number" diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 4e5cdbfb8..625ddd955 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -1,5 +1,6 @@ from http import HTTPStatus +import pytest from botocore.client import BaseClient from brunns.matchers.data import json_matching as is_json_that from brunns.matchers.werkzeug import is_werkzeug_response as is_response @@ -39,6 +40,55 @@ def test_nhs_number_given( is_response().with_status_code(HTTPStatus.OK).and_text(is_json_that(has_key("processedSuggestions"))), ) + @pytest.mark.parametrize( + "headers", + [ + {"nhs-login-nhs-number": None}, # header present but empty + {}, # header missing entirely + {"nhs-login-nhs-number": ""}, # header present but blank + ], + ) + def test_nhs_number_given_but_no_nhs_number_in_header( + self, + client: FlaskClient, + persisted_person: NHSNumber, + campaign_config: CampaignConfig, # noqa: ARG002 + secretsmanager_client: BaseClient, # noqa: ARG002 + headers: dict, + ): + # Given + # When + response = client.get(f"/patient-check/{persisted_person}", headers=headers) + + # Then + assert_that( + response, + is_response() + .with_status_code(HTTPStatus.OK) + .and_text(is_json_that(has_key("processedSuggestions"))), + ) + + def test_nhs_number_given_but_header_nhs_number_doesnt_match( + self, + client: FlaskClient, + persisted_person: NHSNumber, + campaign_config: CampaignConfig, # noqa: ARG002 + secretsmanager_client: BaseClient, # noqa: ARG002 + ): + # Given + headers = {"nhs-login-nhs-number": f"123{str(persisted_person)}"} + + # When + response = client.get(f"/patient-check/{persisted_person}", headers=headers) + + # Then + assert_that( + response, + is_response() + .with_status_code(HTTPStatus.FORBIDDEN) + .and_text(is_json_that(has_entries(resourceType="OperationOutcome"))), + ) + def test_no_nhs_number_given(self, client: FlaskClient): # Given diff --git a/tests/unit/common/test_request_validator.py b/tests/unit/common/test_request_validator.py index e04392ebd..a158b10a8 100644 --- a/tests/unit/common/test_request_validator.py +++ b/tests/unit/common/test_request_validator.py @@ -21,8 +21,9 @@ class TestValidateNHSNumber: ("path_nhs", "header_nhs", "expected_result", "expected_log_msg"), [ (None, None, False, "NHS number is not present in path"), - ("1234567890", None, True, None), (None, "1234567890", False, "NHS number is not present in path"), + ("1234567890", None, True, None), + ("1234567890", "", True, None), ("1234567890", "0987654321", False, "NHS number mismatch"), ("1234567890", "1234567890", True, None), ], @@ -40,7 +41,16 @@ def test_validate_nhs_number(self, path_nhs, header_nhs, expected_result, expect class TestValidateRequestParams: - def test_validate_request_params_success(self, app, caplog): + @pytest.mark.parametrize( + "headers", + [ + {"nhs-login-nhs-number": None}, # header present but empty + {}, # header missing entirely + {"nhs-login-nhs-number": ""}, # header present but blank + {"nhs-login-nhs-number": "1234567890"} # present and matches + ], + ) + def test_validate_request_params_success(self, headers, app, caplog): mock_api = MagicMock(return_value="success") decorator = request_validator.validate_request_params() @@ -48,7 +58,7 @@ def test_validate_request_params_success(self, app, caplog): with app.test_request_context( "/dummy?id=1234567890", - headers={"nhs-login-nhs-number": "1234567890"}, + headers=headers, method="GET", ): with caplog.at_level(logging.INFO): From 04d7814f1c70e8ababe7d2ae895b0f52f0bcf7ce Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 19 Jan 2026 11:47:13 +0000 Subject: [PATCH 2/7] nhs number validation fix --- .../common/api_error_response.py | 2 +- .../common/request_validator.py | 27 ++++++++++--------- tests/unit/common/test_request_validator.py | 26 +++++++++++------- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/eligibility_signposting_api/common/api_error_response.py b/src/eligibility_signposting_api/common/api_error_response.py index 40c1ddcdd..90d8d1909 100644 --- a/src/eligibility_signposting_api/common/api_error_response.py +++ b/src/eligibility_signposting_api/common/api_error_response.py @@ -128,7 +128,7 @@ def log_and_generate_response( fhir_display_message="An unexpected internal server error occurred.", ) -NHS_NUMBER_MISMATCH_ERROR = APIErrorResponse( +NHS_NUMBER_ERROR = APIErrorResponse( status_code=HTTPStatus.FORBIDDEN, fhir_issue_code=FHIRIssueCode.FORBIDDEN, fhir_issue_severity=FHIRIssueSeverity.ERROR, diff --git a/src/eligibility_signposting_api/common/request_validator.py b/src/eligibility_signposting_api/common/request_validator.py index c012861a6..84c6065f0 100644 --- a/src/eligibility_signposting_api/common/request_validator.py +++ b/src/eligibility_signposting_api/common/request_validator.py @@ -10,7 +10,7 @@ INVALID_CATEGORY_ERROR, INVALID_CONDITION_FORMAT_ERROR, INVALID_INCLUDE_ACTIONS_ERROR, - NHS_NUMBER_MISMATCH_ERROR, + NHS_NUMBER_ERROR, ) from eligibility_signposting_api.config.constants import NHS_NUMBER_HEADER @@ -39,17 +39,11 @@ def validate_query_params(query_params: dict[str, str]) -> tuple[bool, ResponseR return True, None -def validate_nhs_number(path_nhs: str | None, header_nhs: str | None) -> bool: - logger.info("NHS numbers from the request", extra={"header_nhs": header_nhs, "path_nhs": path_nhs}) - +def validate_nhs_number_in_header(path_nhs: str | None, header_nhs: str | None) -> bool: if not path_nhs: - logger.error("NHS number is not present in path", extra={"path_nhs": path_nhs}) + logger.error("NHS number is not present in path", extra={"header_nhs": header_nhs, "path_nhs": path_nhs}) return False - if not header_nhs: # Not a validation error - logger.info("NHS number is not present in header", extra={"header_nhs": header_nhs, "path_nhs": path_nhs}) - return True - if header_nhs != path_nhs: logger.error("NHS number mismatch", extra={"header_nhs": header_nhs, "path_nhs": path_nhs}) return False @@ -61,11 +55,20 @@ def decorator(func: Callable) -> Callable: @wraps(func) def wrapper(*args, **kwargs) -> ResponseReturnValue: # noqa:ANN002,ANN003 path_nhs_number = str(kwargs.get("nhs_number")) if kwargs.get("nhs_number") else None - header_nhs_no = str(request.headers.get(NHS_NUMBER_HEADER)) if request.headers.get(NHS_NUMBER_HEADER) else None - if not validate_nhs_number(path_nhs_number, header_nhs_no): + if not path_nhs_number: message = "You are not authorised to request information for the supplied NHS Number" - return NHS_NUMBER_MISMATCH_ERROR.log_and_generate_response(log_message=message, diagnostics=message) + return NHS_NUMBER_ERROR.log_and_generate_response(log_message=message, diagnostics=message) + + if NHS_NUMBER_HEADER in request.headers: + header_nhs_no = request.headers.get(NHS_NUMBER_HEADER) + logger.info("NHS numbers from the request", extra={"header_nhs": header_nhs_no, "path_nhs": path_nhs_number}) + if not validate_nhs_number_in_header(path_nhs_number, header_nhs_no): + message = "You are not authorised to request information for the supplied NHS Number" + return NHS_NUMBER_ERROR.log_and_generate_response(log_message=message, diagnostics=message) + else: + logger.info("NHS numbers from the request", + extra={"header_nhs": None, "path_nhs": path_nhs_number}) query_params = request.args if query_params: diff --git a/tests/unit/common/test_request_validator.py b/tests/unit/common/test_request_validator.py index a158b10a8..1741dd67f 100644 --- a/tests/unit/common/test_request_validator.py +++ b/tests/unit/common/test_request_validator.py @@ -22,15 +22,15 @@ class TestValidateNHSNumber: [ (None, None, False, "NHS number is not present in path"), (None, "1234567890", False, "NHS number is not present in path"), - ("1234567890", None, True, None), - ("1234567890", "", True, None), + ("1234567890", None, False, "NHS number mismatch"), + ("1234567890", "", False, "NHS number mismatch"), ("1234567890", "0987654321", False, "NHS number mismatch"), ("1234567890", "1234567890", True, None), ], ) - def test_validate_nhs_number(self, path_nhs, header_nhs, expected_result, expected_log_msg, caplog): + def test_validate_nhs_number_in_header(self, path_nhs, header_nhs, expected_result, expected_log_msg, caplog): with caplog.at_level(logging.ERROR): - result = request_validator.validate_nhs_number(path_nhs, header_nhs) + result = request_validator.validate_nhs_number_in_header(path_nhs, header_nhs) assert result == expected_result @@ -44,10 +44,8 @@ class TestValidateRequestParams: @pytest.mark.parametrize( "headers", [ - {"nhs-login-nhs-number": None}, # header present but empty - {}, # header missing entirely - {"nhs-login-nhs-number": ""}, # header present but blank - {"nhs-login-nhs-number": "1234567890"} # present and matches + {}, # header missing entirely - request from application restricted consumer + {"nhs-login-nhs-number": "1234567890"} # valid request from consumer ], ) def test_validate_request_params_success(self, headers, app, caplog): @@ -68,7 +66,15 @@ def test_validate_request_params_success(self, headers, app, caplog): assert any("NHS numbers from the request" in record.message for record in caplog.records) assert not any(record.levelname == "ERROR" for record in caplog.records) - def test_validate_request_params_nhs_mismatch(self, app, caplog): + @pytest.mark.parametrize( + "headers", + [ + {"nhs-login-nhs-number": None}, # not valid + {"nhs-login-nhs-number": ""}, # not valid + {"nhs-login-nhs-number": "9834567890"} # not valid, due to mismatch + ], + ) + def test_validate_request_params_nhs_mismatch(self, headers, app, caplog): mock_api = MagicMock() decorator = request_validator.validate_request_params() @@ -76,7 +82,7 @@ def test_validate_request_params_nhs_mismatch(self, app, caplog): with app.test_request_context( "/dummy?id=1234567890", - headers={"nhs-login-nhs-number": "0987654321"}, + headers=headers, method="GET", ): with caplog.at_level(logging.INFO): From 0301098feb8af192026945aa9696015619ceda72 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 19 Jan 2026 11:58:12 +0000 Subject: [PATCH 3/7] integration tests --- .../common/request_validator.py | 7 ++-- .../in_process/test_eligibility_endpoint.py | 36 +++++++++++++++---- tests/unit/common/test_request_validator.py | 6 ++-- 3 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/eligibility_signposting_api/common/request_validator.py b/src/eligibility_signposting_api/common/request_validator.py index 84c6065f0..178b739f0 100644 --- a/src/eligibility_signposting_api/common/request_validator.py +++ b/src/eligibility_signposting_api/common/request_validator.py @@ -62,13 +62,14 @@ def wrapper(*args, **kwargs) -> ResponseReturnValue: # noqa:ANN002,ANN003 if NHS_NUMBER_HEADER in request.headers: header_nhs_no = request.headers.get(NHS_NUMBER_HEADER) - logger.info("NHS numbers from the request", extra={"header_nhs": header_nhs_no, "path_nhs": path_nhs_number}) + logger.info( + "NHS numbers from the request", extra={"header_nhs": header_nhs_no, "path_nhs": path_nhs_number} + ) if not validate_nhs_number_in_header(path_nhs_number, header_nhs_no): message = "You are not authorised to request information for the supplied NHS Number" return NHS_NUMBER_ERROR.log_and_generate_response(log_message=message, diagnostics=message) else: - logger.info("NHS numbers from the request", - extra={"header_nhs": None, "path_nhs": path_nhs_number}) + logger.info("NHS numbers from the request", extra={"header_nhs": None, "path_nhs": path_nhs_number}) query_params = request.args if query_params: diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 625ddd955..6f01e3147 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -43,12 +43,10 @@ def test_nhs_number_given( @pytest.mark.parametrize( "headers", [ - {"nhs-login-nhs-number": None}, # header present but empty - {}, # header missing entirely - {"nhs-login-nhs-number": ""}, # header present but blank + {}, # header missing entirely, valid ], ) - def test_nhs_number_given_but_no_nhs_number_in_header( + def test_nhs_number_given_in_path_but_no_nhs_number_header_present( self, client: FlaskClient, persisted_person: NHSNumber, @@ -60,12 +58,36 @@ def test_nhs_number_given_but_no_nhs_number_in_header( # When response = client.get(f"/patient-check/{persisted_person}", headers=headers) + # Then + assert_that( + response, + is_response().with_status_code(HTTPStatus.OK).and_text(is_json_that(has_key("processedSuggestions"))), + ) + + @pytest.mark.parametrize( + "headers", + [ + {"nhs-login-nhs-number": None}, # header present but empty, invalid + {"nhs-login-nhs-number": ""}, # header present but blank, invalid + ], + ) + def test_nhs_number_in_path_and_header_present_but_empty_or_none( + self, + headers: dict, + client: FlaskClient, + persisted_person: NHSNumber, + campaign_config: CampaignConfig, # noqa: ARG002 + secretsmanager_client: BaseClient, # noqa: ARG002 + ): + # When + response = client.get(f"/patient-check/{persisted_person}", headers=headers) + # Then assert_that( response, is_response() - .with_status_code(HTTPStatus.OK) - .and_text(is_json_that(has_key("processedSuggestions"))), + .with_status_code(HTTPStatus.FORBIDDEN) + .and_text(is_json_that(has_entries(resourceType="OperationOutcome"))), ) def test_nhs_number_given_but_header_nhs_number_doesnt_match( @@ -76,7 +98,7 @@ def test_nhs_number_given_but_header_nhs_number_doesnt_match( secretsmanager_client: BaseClient, # noqa: ARG002 ): # Given - headers = {"nhs-login-nhs-number": f"123{str(persisted_person)}"} + headers = {"nhs-login-nhs-number": f"123{persisted_person!s}"} # When response = client.get(f"/patient-check/{persisted_person}", headers=headers) diff --git a/tests/unit/common/test_request_validator.py b/tests/unit/common/test_request_validator.py index 1741dd67f..486fea02c 100644 --- a/tests/unit/common/test_request_validator.py +++ b/tests/unit/common/test_request_validator.py @@ -45,7 +45,7 @@ class TestValidateRequestParams: "headers", [ {}, # header missing entirely - request from application restricted consumer - {"nhs-login-nhs-number": "1234567890"} # valid request from consumer + {"nhs-login-nhs-number": "1234567890"}, # valid request from consumer ], ) def test_validate_request_params_success(self, headers, app, caplog): @@ -70,8 +70,8 @@ def test_validate_request_params_success(self, headers, app, caplog): "headers", [ {"nhs-login-nhs-number": None}, # not valid - {"nhs-login-nhs-number": ""}, # not valid - {"nhs-login-nhs-number": "9834567890"} # not valid, due to mismatch + {"nhs-login-nhs-number": ""}, # not valid + {"nhs-login-nhs-number": "9834567890"}, # not valid, due to mismatch ], ) def test_validate_request_params_nhs_mismatch(self, headers, app, caplog): From 0d4f0fedfe586f1093cc6ee6e5fddf973b50305a Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 19 Jan 2026 16:04:12 +0000 Subject: [PATCH 4/7] integration tests --- .../lambda/test_app_running_as_lambda.py | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index 9c473696a..59c0ca7fa 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -321,7 +321,27 @@ def test_given_nhs_number_in_path_does_not_match_with_nhs_number_in_headers_resu ) -def test_given_nhs_number_not_present_in_headers_results_in_error_response( +def test_given_nhs_number_not_present_in_headers_results_in_valid_for_application_restricted_users( + lambda_client: BaseClient, # noqa:ARG001 + persisted_person: NHSNumber, + campaign_config: CampaignConfig, # noqa:ARG001 + api_gateway_endpoint: URL, +): + # Given + # When + invoke_url = f"{api_gateway_endpoint}/patient-check/{persisted_person}" + response = httpx.get( + invoke_url, + timeout=10, + ) + + assert_that( + response, + is_response().with_status_code(HTTPStatus.OK).and_body(is_json_that(has_key("processedSuggestions"))), + ) + + +def test_given_nhs_number_key_present_in_headers_have_no_value_results_in_error_response( lambda_client: BaseClient, # noqa:ARG001 persisted_person: NHSNumber, campaign_config: CampaignConfig, # noqa:ARG001 @@ -340,7 +360,7 @@ def test_given_nhs_number_not_present_in_headers_results_in_error_response( response, is_response() .with_status_code(HTTPStatus.FORBIDDEN) - .with_headers(has_entries({"Content-Type": "application/fhir+json"})) + .with_headers(has_entries({"Content-Type": "application/fhir+json", "nhs-login-nhs-number": "123"})) .and_body( is_json_that( has_entries( From 5dd64374a94bcd5f116482b102a066e0a3a37582 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 19 Jan 2026 16:13:43 +0000 Subject: [PATCH 5/7] integration tests --- tests/integration/lambda/test_app_running_as_lambda.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index 59c0ca7fa..f229c9c57 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -360,7 +360,7 @@ def test_given_nhs_number_key_present_in_headers_have_no_value_results_in_error_ response, is_response() .with_status_code(HTTPStatus.FORBIDDEN) - .with_headers(has_entries({"Content-Type": "application/fhir+json", "nhs-login-nhs-number": "123"})) + .with_headers(has_entries({"Content-Type": "application/fhir+json", "nhs-login-nhs-number": None})) .and_body( is_json_that( has_entries( From baf71fa06dfbc7916c09ebd5b2068add995bc971 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 19 Jan 2026 16:30:13 +0000 Subject: [PATCH 6/7] integration tests --- tests/integration/lambda/test_app_running_as_lambda.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index f229c9c57..46572689a 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -352,6 +352,7 @@ def test_given_nhs_number_key_present_in_headers_have_no_value_results_in_error_ invoke_url = f"{api_gateway_endpoint}/patient-check/{persisted_person}" response = httpx.get( invoke_url, + headers={"nhs-login-nhs-number": ""}, timeout=10, ) @@ -360,7 +361,6 @@ def test_given_nhs_number_key_present_in_headers_have_no_value_results_in_error_ response, is_response() .with_status_code(HTTPStatus.FORBIDDEN) - .with_headers(has_entries({"Content-Type": "application/fhir+json", "nhs-login-nhs-number": None})) .and_body( is_json_that( has_entries( From 7671684adfe9d77c9ebc6fffa88daa71592eeecd Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 20 Jan 2026 09:47:57 +0000 Subject: [PATCH 7/7] path validation fixed --- src/eligibility_signposting_api/common/request_validator.py | 4 ---- tests/unit/common/test_request_validator.py | 2 -- 2 files changed, 6 deletions(-) diff --git a/src/eligibility_signposting_api/common/request_validator.py b/src/eligibility_signposting_api/common/request_validator.py index 178b739f0..79e967c3c 100644 --- a/src/eligibility_signposting_api/common/request_validator.py +++ b/src/eligibility_signposting_api/common/request_validator.py @@ -40,10 +40,6 @@ def validate_query_params(query_params: dict[str, str]) -> tuple[bool, ResponseR def validate_nhs_number_in_header(path_nhs: str | None, header_nhs: str | None) -> bool: - if not path_nhs: - logger.error("NHS number is not present in path", extra={"header_nhs": header_nhs, "path_nhs": path_nhs}) - return False - if header_nhs != path_nhs: logger.error("NHS number mismatch", extra={"header_nhs": header_nhs, "path_nhs": path_nhs}) return False diff --git a/tests/unit/common/test_request_validator.py b/tests/unit/common/test_request_validator.py index 486fea02c..4c600ad57 100644 --- a/tests/unit/common/test_request_validator.py +++ b/tests/unit/common/test_request_validator.py @@ -20,8 +20,6 @@ class TestValidateNHSNumber: @pytest.mark.parametrize( ("path_nhs", "header_nhs", "expected_result", "expected_log_msg"), [ - (None, None, False, "NHS number is not present in path"), - (None, "1234567890", False, "NHS number is not present in path"), ("1234567890", None, False, "NHS number mismatch"), ("1234567890", "", False, "NHS number mismatch"), ("1234567890", "0987654321", False, "NHS number mismatch"),