From 6249b228105651e4d5b3dd325c3d68e79a40d69b Mon Sep 17 00:00:00 2001 From: adamwhitingnhs Date: Wed, 11 Feb 2026 14:48:56 +0000 Subject: [PATCH] [PRMP-1439] Fix doc ref GET for deceased patient --- lambdas/handlers/authoriser_handler.py | 9 +- lambdas/services/authoriser_service.py | 28 ++-- .../unit/handlers/test_authoriser_handler.py | 54 ++++-- .../unit/services/test_authoriser_service.py | 155 ++++++++++++++---- 4 files changed, 181 insertions(+), 65 deletions(-) diff --git a/lambdas/handlers/authoriser_handler.py b/lambdas/handlers/authoriser_handler.py index 9d226784e..789cd18cd 100644 --- a/lambdas/handlers/authoriser_handler.py +++ b/lambdas/handlers/authoriser_handler.py @@ -42,7 +42,8 @@ def lambda_handler(event, context): if event.get("methodArn") is None: return {"Error": "methodArn is not defined"} _, _, _, region, aws_account_id, api_gateway_arn = event.get("methodArn").split( - ":", 5 + ":", + 5, ) api_id, stage, _http_verb, _resource_name = api_gateway_arn.split("/", 3) path = "/" + _resource_name @@ -57,7 +58,11 @@ def lambda_handler(event, context): patient_id = event.get("queryStringParameters", {}).get("patientId", None) logger.info("Validating resource req: %s, http: %s" % (path, _http_verb)) is_allow_policy = authoriser_service.auth_request( - path, ssm_jwt_public_key_parameter, auth_token, patient_id + path, + _http_verb, + ssm_jwt_public_key_parameter, + auth_token, + patient_id, ) if is_allow_policy: policy.allow_method(_http_verb, path) diff --git a/lambdas/services/authoriser_service.py b/lambdas/services/authoriser_service.py index f7632ac07..2f86cdea3 100644 --- a/lambdas/services/authoriser_service.py +++ b/lambdas/services/authoriser_service.py @@ -2,6 +2,7 @@ import time from enums.repository_role import RepositoryRole +from models.auth_policy import HttpVerb from models.staging_metadata import NHS_NUMBER_PLACEHOLDER from services.manage_user_session_access import ManageUserSessionAccess from services.token_service import TokenService @@ -23,7 +24,12 @@ def __init__(self): self.manage_user_session_service = ManageUserSessionAccess() def auth_request( - self, path, ssm_jwt_public_key_parameter, auth_token, nhs_number: str = None + self, + path, + http_verb, + ssm_jwt_public_key_parameter, + auth_token, + nhs_number: str = None, ): try: decoded_token = token_service.get_public_key_and_decode_auth_token( @@ -39,7 +45,7 @@ def auth_request( user_role = decoded_token.get("repository_role") current_session = self.manage_user_session_service.find_login_session( - ndr_session_id + ndr_session_id, ) self.allowed_nhs_numbers = ( current_session.get("AllowedNHSNumbers", "").split(",") @@ -54,7 +60,12 @@ def auth_request( self.validate_login_session(float(current_session["TimeToExist"])) - resource_denied = self.deny_access_policy(path, user_role, nhs_number) + resource_denied = self.deny_access_policy( + path, + http_verb, + user_role, + nhs_number, + ) allow_policy = False @@ -67,7 +78,7 @@ def auth_request( except (KeyError, IndexError) as e: raise AuthorisationException(e) - def deny_access_policy(self, path, user_role, nhs_number: str = None): + def deny_access_policy(self, path, http_verb, user_role, nhs_number: str = None): logger.info(f"Path: {path}") patient_access_is_allowed = ( @@ -80,7 +91,6 @@ def deny_access_policy(self, path, user_role, nhs_number: str = None): patient_id_is_placeholder = nhs_number == NHS_NUMBER_PLACEHOLDER - is_user_gp_admin = user_role == RepositoryRole.GP_ADMIN.value is_user_gp_clinical = user_role == RepositoryRole.GP_CLINICAL.value is_user_pcse = user_role == RepositoryRole.PCSE.value @@ -100,12 +110,8 @@ def deny_access_policy(self, path, user_role, nhs_number: str = None): deny_resource = not patient_access_is_allowed or is_user_gp_clinical case doc_ref if re.match(doc_ref_pattern, doc_ref): - deny_resource = True - if ( - is_user_gp_admin or is_user_gp_clinical - ) and patient_access_is_allowed: - deny_resource = False - if patient_access_is_allowed and access_to_deceased_patient: + deny_resource = is_user_pcse or not patient_access_is_allowed + if http_verb != HttpVerb.GET and access_to_deceased_patient: deny_resource = True case "/LloydGeorgeStitch": diff --git a/lambdas/tests/unit/handlers/test_authoriser_handler.py b/lambdas/tests/unit/handlers/test_authoriser_handler.py index 61b2f2c69..2974223bf 100644 --- a/lambdas/tests/unit/handlers/test_authoriser_handler.py +++ b/lambdas/tests/unit/handlers/test_authoriser_handler.py @@ -11,7 +11,7 @@ "Action": "execute-api:Invoke", "Effect": "Deny", "Resource": [f"{MOCK_METHOD_ARN_PREFIX}/*/*"], - } + }, ], "Version": "2012-10-17", } @@ -24,12 +24,13 @@ def test_valid_gp_admin_token_return_allow_policy(set_env, context, mocker): "Action": "execute-api:Invoke", "Effect": "Allow", "Resource": [f"{MOCK_METHOD_ARN_PREFIX}/GET/SearchDocumentReferences"], - } + }, ], "Version": "2012-10-17", } mock_auth_service = mocker.patch( - "services.authoriser_service.AuthoriserService.auth_request", return_value=True + "services.authoriser_service.AuthoriserService.auth_request", + return_value=True, ) auth_token = "valid_gp_admin_token" test_event = { @@ -41,6 +42,7 @@ def test_valid_gp_admin_token_return_allow_policy(set_env, context, mocker): response = lambda_handler(event=test_event, context=context) mock_auth_service.assert_called_with( "/SearchDocumentReferences", + "GET", SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, auth_token, TEST_NHS_NUMBER, @@ -55,7 +57,7 @@ def test_valid_pcse_token_return_allow_policy(set_env, mocker, context): "Action": "execute-api:Invoke", "Effect": "Allow", "Resource": [f"{MOCK_METHOD_ARN_PREFIX}/GET/SearchPatient"], - } + }, ], "Version": "2012-10-17", } @@ -67,13 +69,18 @@ def test_valid_pcse_token_return_allow_policy(set_env, mocker, context): "methodArn": f"{MOCK_METHOD_ARN_PREFIX}/GET/SearchPatient", } mock_auth_service = mocker.patch( - "services.authoriser_service.AuthoriserService.auth_request", return_value=True + "services.authoriser_service.AuthoriserService.auth_request", + return_value=True, ) response = lambda_handler(test_event, context=context) mock_auth_service.assert_called_with( - "/SearchPatient", SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, auth_token, TEST_NHS_NUMBER + "/SearchPatient", + "GET", + SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, + auth_token, + TEST_NHS_NUMBER, ) assert response["policyDocument"] == expected_allow_policy @@ -85,12 +92,13 @@ def test_valid_gp_admin_token_return_deny_policy(set_env, context, mocker): "Action": "execute-api:Invoke", "Effect": "Deny", "Resource": [f"{MOCK_METHOD_ARN_PREFIX}/GET/SearchDocumentReferences"], - } + }, ], "Version": "2012-10-17", } mock_auth_service = mocker.patch( - "services.authoriser_service.AuthoriserService.auth_request", return_value=False + "services.authoriser_service.AuthoriserService.auth_request", + return_value=False, ) auth_token = "valid_gp_admin_token" test_event = { @@ -103,6 +111,7 @@ def test_valid_gp_admin_token_return_deny_policy(set_env, context, mocker): mock_auth_service.assert_called_with( "/SearchDocumentReferences", + "GET", SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, auth_token, TEST_NHS_NUMBER, @@ -117,7 +126,7 @@ def test_valid_pcse_token_return_deny_policy(set_env, mocker, context): "Action": "execute-api:Invoke", "Effect": "Deny", "Resource": [f"{MOCK_METHOD_ARN_PREFIX}/GET/SearchPatient"], - } + }, ], "Version": "2012-10-17", } @@ -129,13 +138,18 @@ def test_valid_pcse_token_return_deny_policy(set_env, mocker, context): "methodArn": f"{MOCK_METHOD_ARN_PREFIX}/GET/SearchPatient", } mock_auth_service = mocker.patch( - "services.authoriser_service.AuthoriserService.auth_request", return_value=False + "services.authoriser_service.AuthoriserService.auth_request", + return_value=False, ) response = lambda_handler(test_event, context=context) mock_auth_service.assert_called_with( - "/SearchPatient", SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, auth_token, TEST_NHS_NUMBER + "/SearchPatient", + "GET", + SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, + auth_token, + TEST_NHS_NUMBER, ) assert response["policyDocument"] == expected_deny_policy @@ -157,7 +171,11 @@ def test_return_deny_all_policy_pcse_user_when_auth_exception(set_env, mocker, c assert response["policyDocument"] == DENY_ALL_POLICY mock_auth_service.assert_called_with( - "/SearchPatient", SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, auth_token, TEST_NHS_NUMBER + "/SearchPatient", + "GET", + SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, + auth_token, + TEST_NHS_NUMBER, ) @@ -191,7 +209,7 @@ def test_deny_dr_for_deceased_patients(set_env, context, mocker, endpoint_path): "Action": "execute-api:Invoke", "Effect": "Deny", "Resource": [f"{MOCK_METHOD_ARN_PREFIX}{endpoint_path}"], - } + }, ], "Version": "2012-10-17", } @@ -205,13 +223,15 @@ def test_deny_dr_for_deceased_patients(set_env, context, mocker, endpoint_path): } mock_auth_service = mocker.patch( - "services.authoriser_service.AuthoriserService.auth_request", return_value=False + "services.authoriser_service.AuthoriserService.auth_request", + return_value=False, ) response = lambda_handler(test_event, context=context) mock_auth_service.assert_called_with( "/" + endpoint_path.split("/", 2)[2], + endpoint_path.split("/")[1], SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, auth_token, TEST_NHS_NUMBER, @@ -226,7 +246,7 @@ def test_deny_dr_for_non_gp_admins_or_clinicians(set_env, context, mocker): "Action": "execute-api:Invoke", "Effect": "Deny", "Resource": [f"{MOCK_METHOD_ARN_PREFIX}/POST/DocumentReference"], - } + }, ], "Version": "2012-10-17", } @@ -238,13 +258,15 @@ def test_deny_dr_for_non_gp_admins_or_clinicians(set_env, context, mocker): "methodArn": f"{MOCK_METHOD_ARN_PREFIX}/POST/DocumentReference", } mock_auth_service = mocker.patch( - "services.authoriser_service.AuthoriserService.auth_request", return_value=False + "services.authoriser_service.AuthoriserService.auth_request", + return_value=False, ) pcse_response = lambda_handler(test_pcse_event, context=context) mock_auth_service.assert_called_with( "/DocumentReference", + "POST", SSM_PARAM_JWT_TOKEN_PUBLIC_KEY, auth_pcse_token, TEST_NHS_NUMBER, diff --git a/lambdas/tests/unit/services/test_authoriser_service.py b/lambdas/tests/unit/services/test_authoriser_service.py index ca6266c23..9642eb5f3 100644 --- a/lambdas/tests/unit/services/test_authoriser_service.py +++ b/lambdas/tests/unit/services/test_authoriser_service.py @@ -13,7 +13,7 @@ "Action": "execute-api:Invoke", "Effect": "Deny", "Resource": [f"{MOCK_METHOD_ARN_PREFIX}/*/*"], - } + }, ], "Version": "2012-10-17", } @@ -87,13 +87,17 @@ def test_deny_access_policy_returns_true_for_gp_clinical_on_paths( expected = True mock_auth_service.allowed_nhs_numbers = ["900000001"] actual = mock_auth_service.deny_access_policy( - test_path, RepositoryRole.GP_CLINICAL.value, "900000001" + test_path, + "GET", + RepositoryRole.GP_CLINICAL.value, + "900000001", ) assert actual == expected @pytest.mark.parametrize( - "test_path", ["/DocumentManifest", "/DocumentDelete", "/DocumentStatus", "Any"] + "test_path", + ["/DocumentManifest", "/DocumentDelete", "/DocumentStatus", "Any"], ) def test_deny_access_policy_returns_true_for_nhs_number_not_in_allowed( test_path, @@ -102,7 +106,10 @@ def test_deny_access_policy_returns_true_for_nhs_number_not_in_allowed( expected = True mock_auth_service.allowed_nhs_numbers = ["900000002"] actual = mock_auth_service.deny_access_policy( - test_path, RepositoryRole.GP_ADMIN.value, "900000001" + test_path, + "GET", + RepositoryRole.GP_ADMIN.value, + "900000001", ) assert actual == expected @@ -115,7 +122,10 @@ def test_deny_access_policy_returns_false_for_nhs_number_in_allowed( expected = False mock_auth_service.allowed_nhs_numbers = ["900000002"] actual = mock_auth_service.deny_access_policy( - test_path, RepositoryRole.GP_ADMIN.value, "900000002" + test_path, + "GET", + RepositoryRole.GP_ADMIN.value, + "900000002", ) assert actual == expected @@ -157,7 +167,7 @@ def test_deny_access_policy_for_various_paths_and_roles( ): mock_auth_service.allowed_nhs_numbers.append("122222222") - actual = mock_auth_service.deny_access_policy(path, role, "122222222") + actual = mock_auth_service.deny_access_policy(path, "GET", role, "122222222") assert actual == expected @@ -171,21 +181,25 @@ def test_deny_access_policy_allows_review_for_placeholder_nhs_number( ] for role in roles: actual = mock_auth_service.deny_access_policy( - f"/DocumentReview/{TEST_UUID}/1", role, NHS_NUMBER_PLACEHOLDER + f"/DocumentReview/{TEST_UUID}/1", + "GET", + role, + NHS_NUMBER_PLACEHOLDER, ) assert not actual @pytest.mark.parametrize( - "path", + ["path", "http_verb"], [ - "/DocumentReference", - "/DocumentReference/6b6417b5-58ed-45db-8359-bd78891e67b7", + ("/DocumentReference", "POST"), + ("/DocumentReference/6b6417b5-58ed-45db-8359-bd78891e67b7", "PUT"), ], ) -def test_deny_document_reference_as_any_role_on_deceased_patient_returns_true( +def test_deny_document_reference_as_any_role_on_deceased_patient_returns_true_for_POST_and_PUT( mock_auth_service: AuthoriserService, path: str, + http_verb: str, ): expected = True @@ -197,7 +211,33 @@ def test_deny_document_reference_as_any_role_on_deceased_patient_returns_true( RepositoryRole.GP_CLINICAL.value, RepositoryRole.GP_ADMIN.value, ): - actual = mock_auth_service.deny_access_policy(path, role, "122222222") + actual = mock_auth_service.deny_access_policy( + path, + http_verb, + role, + "122222222", + ) + assert actual == expected + + +def test_deny_document_reference_as_gp_role_on_deceased_patient_returns_false_for_GET( + mock_auth_service: AuthoriserService, +): + expected = False + + mock_auth_service.allowed_nhs_numbers.append("122222222") + mock_auth_service.deceased_nhs_numbers.append("122222222") + + for role in ( + RepositoryRole.GP_CLINICAL.value, + RepositoryRole.GP_ADMIN.value, + ): + actual = mock_auth_service.deny_access_policy( + "/DocumentReference", + "GET", + role, + "122222222", + ) assert actual == expected @@ -219,7 +259,10 @@ def test_deny_access_policy_returns_true_for_invalid_nhs_number( expected = True mock_auth_service.allowed_nhs_numbers = ["900000002"] actual = mock_auth_service.deny_access_policy( - test_path, RepositoryRole.GP_ADMIN.value, nhs_number + test_path, + "GET", + RepositoryRole.GP_ADMIN.value, + nhs_number, ) assert actual == expected @@ -247,7 +290,12 @@ def test_endpoints_allow_access_regardless_of_nhs_number( RepositoryRole.GP_CLINICAL.value, RepositoryRole.GP_ADMIN.value, ): - actual = mock_auth_service.deny_access_policy(test_path, role, "122222222") + actual = mock_auth_service.deny_access_policy( + test_path, + "GET", + role, + "122222222", + ) assert actual == expected @@ -264,7 +312,10 @@ def test_deny_access_policy_returns_false_for_pcse_on_all_paths( mock_auth_service.allowed_nhs_numbers = ["122222222"] actual = mock_auth_service.deny_access_policy( - test_path, RepositoryRole.PCSE.value, "122222222" + test_path, + "GET", + RepositoryRole.PCSE.value, + "122222222", ) assert expected == actual @@ -285,7 +336,11 @@ def test_deny_access_policy_returns_true_for_pcse_on_paths( mock_auth_service: AuthoriserService, ): expected = True - actual = mock_auth_service.deny_access_policy(test_path, RepositoryRole.PCSE.value) + actual = mock_auth_service.deny_access_policy( + test_path, + "GET", + RepositoryRole.PCSE.value, + ) assert expected == actual @@ -296,6 +351,7 @@ def test_deny_access_policy_returns_true_for_unrecognised_path( actual = mock_auth_service.deny_access_policy( "/test", + "GET", RepositoryRole.PCSE.value, ) @@ -310,7 +366,10 @@ def test_validate_login_expired_session(mocker, mock_auth_service: AuthoriserSer @pytest.mark.parametrize("path_test", ["/DocumentManifest"]) def test_valid_gp_admin_token_returns_allow_policy_true( - path_test, mock_jwt_decode, mocker, mock_auth_service: AuthoriserService + path_test, + mock_jwt_decode, + mocker, + mock_auth_service: AuthoriserService, ): auth_token = "valid_gp_admin_token" mock_auth_service.manage_user_session_service.find_login_session.return_value = ( @@ -326,10 +385,16 @@ def test_valid_gp_admin_token_returns_allow_policy_true( return_value=False, ) - response = mock_auth_service.auth_request(path_test, "test public key", auth_token) + response = mock_auth_service.auth_request( + path_test, + "GET", + "test public key", + auth_token, + ) mock_jwt_decode.assert_called_with( - auth_token=auth_token, ssm_public_key_parameter="test public key" + auth_token=auth_token, + ssm_public_key_parameter="test public key", ) assert response mock_auth_service.manage_user_session_service.find_login_session.assert_called_once() @@ -339,7 +404,10 @@ def test_valid_gp_admin_token_returns_allow_policy_true( @pytest.mark.parametrize("test_path", ["/SearchPatient"]) def test_valid_pcse_token_return_allow_policy_true( - test_path, mocker, mock_jwt_decode, mock_auth_service: AuthoriserService + test_path, + mocker, + mock_jwt_decode, + mock_auth_service: AuthoriserService, ): auth_token = "valid_pcse_token" @@ -348,17 +416,23 @@ def test_valid_pcse_token_return_allow_policy_true( ) mock_validate_login_session = mocker.patch( - "services.authoriser_service.AuthoriserService.validate_login_session" + "services.authoriser_service.AuthoriserService.validate_login_session", ) mock_deny_access_policy = mocker.patch( "services.authoriser_service.AuthoriserService.deny_access_policy", return_value=False, ) - response = mock_auth_service.auth_request(test_path, "test public key", auth_token) + response = mock_auth_service.auth_request( + test_path, + "GET", + "test public key", + auth_token, + ) mock_jwt_decode.assert_called_with( - auth_token=auth_token, ssm_public_key_parameter="test public key" + auth_token=auth_token, + ssm_public_key_parameter="test public key", ) assert response mock_auth_service.manage_user_session_service.find_login_session.assert_called_once() @@ -368,7 +442,10 @@ def test_valid_pcse_token_return_allow_policy_true( @pytest.mark.parametrize("test_path", ["/SearchPatient"]) def test_return_deny_policy_when_no_session_found( - test_path, mocker, mock_jwt_decode, mock_auth_service: AuthoriserService + test_path, + mocker, + mock_jwt_decode, + mock_auth_service: AuthoriserService, ): auth_token = "valid_pcse_token" mock_auth_service.manage_user_session_service.find_login_session.side_effect = ( @@ -376,16 +453,17 @@ def test_return_deny_policy_when_no_session_found( ) mock_validate_login_session = mocker.patch( - "services.authoriser_service.AuthoriserService.validate_login_session" + "services.authoriser_service.AuthoriserService.validate_login_session", ) mock_deny_access_policy = mocker.patch( - "services.authoriser_service.AuthoriserService.deny_access_policy" + "services.authoriser_service.AuthoriserService.deny_access_policy", ) with pytest.raises(AuthorisationException): - mock_auth_service.auth_request(test_path, "test public key", auth_token) + mock_auth_service.auth_request(test_path, "GET", "test public key", auth_token) mock_jwt_decode.assert_called_with( - auth_token=auth_token, ssm_public_key_parameter="test public key" + auth_token=auth_token, + ssm_public_key_parameter="test public key", ) mock_auth_service.manage_user_session_service.find_login_session.assert_called_once() mock_validate_login_session.assert_not_called() @@ -410,13 +488,14 @@ def test_raise_exception_when_user_session_is_expired( side_effect=AuthorisationException(), ) mock_deny_access_policy = mocker.patch( - "services.authoriser_service.AuthoriserService.deny_access_policy" + "services.authoriser_service.AuthoriserService.deny_access_policy", ) with pytest.raises(AuthorisationException): - mock_auth_service.auth_request(test_path, "test public key", auth_token) + mock_auth_service.auth_request(test_path, "GET", "test public key", auth_token) mock_jwt_decode.assert_called_with( - auth_token=auth_token, ssm_public_key_parameter="test public key" + auth_token=auth_token, + ssm_public_key_parameter="test public key", ) mock_auth_service.manage_user_session_service.find_login_session.assert_called_once() mock_validate_login_session.assert_called_once() @@ -425,21 +504,25 @@ def test_raise_exception_when_user_session_is_expired( @pytest.mark.parametrize("test_path", ["/SearchPatient"]) def test_invalid_token_raise_exception( - test_path, mocker, mock_jwt_decode, mock_auth_service: AuthoriserService + test_path, + mocker, + mock_jwt_decode, + mock_auth_service: AuthoriserService, ): auth_token = "invalid_token" mock_validate_login_session = mocker.patch( - "services.authoriser_service.AuthoriserService.validate_login_session" + "services.authoriser_service.AuthoriserService.validate_login_session", ) mock_deny_access_policy = mocker.patch( - "services.authoriser_service.AuthoriserService.deny_access_policy" + "services.authoriser_service.AuthoriserService.deny_access_policy", ) with pytest.raises(AuthorisationException): - mock_auth_service.auth_request(test_path, "test public key", auth_token) + mock_auth_service.auth_request(test_path, "GET", "test public key", auth_token) mock_jwt_decode.assert_called_with( - auth_token=auth_token, ssm_public_key_parameter="test public key" + auth_token=auth_token, + ssm_public_key_parameter="test public key", ) mock_auth_service.manage_user_session_service.find_login_session.assert_not_called() mock_validate_login_session.assert_not_called()