Refactored search immunisations flow

Author: dlzhry2nhs

backend/src/authorisation/authoriser.py

@@ -39,6 +39,8 @@ def authorise(
         requested_operation: ApiOperationCode,
         vaccination_types: set[str]
     ) -> bool:
+        """Checks that the supplier system is permitted to carry out the requested operation on the given vaccination
+        type(s)"""
         supplier_permissions = self._get_supplier_permissions(supplier_system)
 
         logger.info(
@@ -49,3 +51,20 @@ def authorise(
             requested_operation in supplier_permissions.get(vaccination_type.lower(), [])
             for vaccination_type in vaccination_types
         )
+
+    def filter_permitted_vacc_types(
+        self,
+        supplier_system: str,
+        requested_operation: ApiOperationCode,
+        vaccination_types: set[str]
+    ) -> set[str]:
+        """Returns the set of vaccine types that a given supplier can interact with for a given operation type.
+        This is a more permissive form of authorisation e.g. used in search as it will filter out any requested vacc
+        types that they cannot interact with without throwing an error"""
+        supplier_permissions = self._get_supplier_permissions(supplier_system)
+
+        return set([
+            vaccine_type
+            for vaccine_type in vaccination_types
+            if requested_operation in supplier_permissions.get(vaccine_type.lower(), [])
+        ])

backend/src/fhir_controller.py

@@ -410,25 +410,16 @@ def search_immunizations(self, aws_event: APIGatewayProxyEventV1) -> dict:
             return self.create_response(403, unauthorized.to_operation_outcome())
         # Check vaxx type permissions on the existing record - start
         try:
-            expanded_permissions = _expand_permissions(imms_vax_type_perms)
-            vax_type_perm = [
-                vaccine_type
-                for vaccine_type in search_params.immunization_targets
-                if ApiOperationCode.SEARCH in expanded_permissions.get(vaccine_type.lower(), [])
-            ]
-            if not vax_type_perm:
-                raise UnauthorizedVaxError
+            result, request_contained_unauthorised_vaccs = self.fhir_service.search_immunizations(
+                search_params.patient_identifier,
+                search_params.immunization_targets,
+                create_query_string(search_params),
+                supplier_system,
+                search_params.date_from,
+                search_params.date_to,
+            )
         except UnauthorizedVaxError as unauthorized:
             return self.create_response(403, unauthorized.to_operation_outcome())
-        # Check vaxx type permissions on the existing record - end
-
-        result = self.fhir_service.search_immunizations(
-            search_params.patient_identifier,
-            vax_type_perm,
-            create_query_string(search_params),
-            search_params.date_from,
-            search_params.date_to,
-        )
 
         if "diagnostics" in result:
             exp_error = create_operation_outcome(
@@ -450,7 +441,7 @@ def search_immunizations(self, aws_event: APIGatewayProxyEventV1) -> dict:
                 1 for entry in result_json_dict["entry"] if entry.get("search", {}).get("mode") == "match"
             )
             result_json_dict["total"] = total_count
-            if sorted(search_params.immunization_targets) != sorted(vax_type_perm):
+            if request_contained_unauthorised_vaccs:
                 exp_error = create_operation_outcome(
                     resource_id=str(uuid.uuid4()),
                     severity=Severity.warning,
@@ -498,7 +489,7 @@ def _validate_identifier_system(self, _id: str, _elements: str) -> Optional[dict
                     'e.g. "http://xyz.org/vaccs|2345-gh3s-r53h7-12ny"'
                 ),
             )
-        
+
         if not _elements:
             return None
 
@@ -570,7 +561,7 @@ def fetch_identifier_system_and_element(self, event: dict):
                 query_string_has_immunization_identifier,
                 query_string_has_element,
             )
-        
+
         # Post Search Identifier by body form
         if body and not query_params:
             decoded_body = base64.b64decode(body).decode("utf-8")

backend/src/fhir_repository.py

@@ -365,7 +365,7 @@ def delete_immunization(self, imms_id: str, supplier_system: str) -> dict:
                     response=error.response,
                 )
 
-    def find_immunizations(self, patient_identifier: str, vaccine_types: list):
+    def find_immunizations(self, patient_identifier: str, vaccine_types: set):
         """it should find all of the specified patient's Immunization events for all of the specified vaccine_types"""
         condition = Key("PatientPK").eq(_make_patient_pk(patient_identifier))
         is_not_deleted = Attr("DeletedAt").not_exists() | Attr("DeletedAt").eq("reinstated")

backend/src/fhir_service.py

@@ -71,7 +71,7 @@ def get_immunization_by_identifier(
 
         if not imms_resp:
             return form_json(imms_resp, None, None, base_url)
-        
+
         if not self.authoriser.authorise(supplier_name, ApiOperationCode.SEARCH, {vaccination_type}):
             raise UnauthorizedVaxError()
 
@@ -312,22 +312,32 @@ def search_immunizations(
         nhs_number: str,
         vaccine_types: list[str],
         params: str,
+        supplier_system: str,
         date_from: datetime.date = parameter_parser.date_from_default,
         date_to: datetime.date = parameter_parser.date_to_default,
-    ) -> FhirBundle:
+    ) -> tuple[FhirBundle, bool]:
         """
         Finds all instances of Immunization(s) for a specified patient which are for the specified vaccine type(s).
-        Bundles the resources with the relevant patient resource and returns the bundle.
+        Bundles the resources with the relevant patient resource and returns the bundle along with a boolean to state
+        whether the supplier requested vaccine types they were not authorised for.
         """
         # TODO: is disease type a mandatory field? (I assumed it is)
         #  i.e. Should we provide a search option for getting Patient's entire imms history?
         if not nhs_number_mod11_check(nhs_number):
             return create_diagnostics()
 
+        permitted_vacc_types = self.authoriser.filter_permitted_vacc_types(
+            supplier_system, ApiOperationCode.SEARCH, set(vaccine_types)
+        )
+
+        # Only raise error if supplier's request had no permitted vaccinations
+        if not permitted_vacc_types:
+            raise UnauthorizedVaxError()
+
         # Obtain all resources which are for the requested nhs number and vaccine type(s) and within the date range
         resources = [
             r
-            for r in self.immunization_repo.find_immunizations(nhs_number, vaccine_types)
+            for r in self.immunization_repo.find_immunizations(nhs_number, permitted_vacc_types)
             if self.is_valid_date_from(r, date_from) and self.is_valid_date_to(r, date_to)
         ]
 
@@ -366,8 +376,9 @@ def search_immunizations(
         # Create the bundle
         fhir_bundle = FhirBundle(resourceType="Bundle", type="searchset", entry=entries)
         fhir_bundle.link = [BundleLink(relation="self", url=self.create_url_for_bundle_link(params, vaccine_types))]
+        supplier_requested_unauthorised_vaccs = len(vaccine_types) != len(permitted_vacc_types)
 
-        return fhir_bundle
+        return fhir_bundle, supplier_requested_unauthorised_vaccs
 
     @timed
     def _validate_patient(self, imms: dict) -> dict:

backend/tests/test_fhir_controller.py

@@ -1654,7 +1654,7 @@ def test_get_search_immunizations(self, mock_get_supplier_permissions):
         """it should search based on patient_identifier and immunization_target"""
         mock_get_supplier_permissions.return_value = ["COVID19.S"]
         search_result = Bundle.construct()
-        self.service.search_immunizations.return_value = search_result
+        self.service.search_immunizations.return_value = search_result, False
 
         vaccine_type = "COVID19"
         params = f"{self.immunization_target_key}={vaccine_type}&" + urllib.parse.urlencode(
@@ -1677,18 +1677,18 @@ def test_get_search_immunizations(self, mock_get_supplier_permissions):
         # Then
         mock_get_supplier_permissions.assert_called_once_with("test")
         self.service.search_immunizations.assert_called_once_with(
-            self.nhs_number_valid_value, [vaccine_type], params, ANY, ANY
+            self.nhs_number_valid_value, [vaccine_type], params, "test", ANY, ANY
         )
         self.assertEqual(response["statusCode"], 200)
         body = json.loads(response["body"])
         self.assertEqual(body["resourceType"], "Bundle")
 
     @patch("fhir_controller.get_supplier_permissions")
     def test_get_search_immunizations_vax_permission_check(self, mock_get_supplier_permissions):
-        """it should search based on patient_identifier and immunization_target"""
-        mock_get_supplier_permissions.return_value = []
+        """it should return a 403 error if the service raises an unauthorized error"""
         search_result = Bundle.construct()
-        self.service.search_immunizations.return_value = search_result
+        mock_get_supplier_permissions.return_value = []
+        self.service.search_immunizations.side_effect = UnauthorizedVaxError()
 
         vaccine_type = "COVID19"
         lambda_event = {
@@ -1704,14 +1704,16 @@ def test_get_search_immunizations_vax_permission_check(self, mock_get_supplier_p
 
         # Then
         self.assertEqual(response["statusCode"], 403)
+        body = json.loads(response["body"])
+        self.assertEqual(body["resourceType"], "OperationOutcome")
 
     @patch("fhir_controller.get_supplier_permissions")
     def test_get_search_immunizations_for_unauthorized_vaccine_type_search(self, mock_get_supplier_permissions):
         """it should return 200 and contains warning operation outcome as the user is not having authorization for one of the vaccine type"""
         search_result = load_json_data("sample_immunization_response _for _not_done_event.json")
         mock_get_supplier_permissions.return_value = ["covid19.S"]
         bundle = Bundle.parse_obj(search_result)
-        self.service.search_immunizations.return_value = bundle
+        self.service.search_immunizations.return_value = bundle, True
 
         vaccine_type = ["COVID19", "FLU"]
         vaccine_type = ",".join(vaccine_type)
@@ -1737,11 +1739,11 @@ def test_get_search_immunizations_for_unauthorized_vaccine_type_search(self, moc
 
     @patch("fhir_controller.get_supplier_permissions")
     def test_get_search_immunizations_for_unauthorized_vaccine_type_search_400(self,mock_get_supplier_permissions):
-        """it should return 400 as the the request is having invalid vaccine type"""
+        """it should return 400 as the request has an invalid vaccine type"""
         search_result = load_json_data("sample_immunization_response _for _not_done_event.json")
         mock_get_supplier_permissions.return_value = ["covid19.S"]
         bundle = Bundle.parse_obj(search_result)
-        self.service.search_immunizations.return_value = bundle
+        self.service.search_immunizations.return_value = bundle, False
 
         vaccine_type = "FLUE"
 
@@ -1759,60 +1761,12 @@ def test_get_search_immunizations_for_unauthorized_vaccine_type_search_400(self,
         body = json.loads(response["body"])
         self.assertEqual(body["resourceType"], "OperationOutcome")
 
-    @patch("fhir_controller.get_supplier_permissions")
-    def test_get_search_immunizations_for_unauthorized_vaccine_type_search_403(self, mock_get_supplier_permissions):
-        """it should return 403 as the user doesnt have vaccinetype permission"""
-        search_result = load_json_data("sample_immunization_response _for _not_done_event.json")
-        bundle = Bundle.parse_obj(search_result)
-        mock_get_supplier_permissions.return_value = []
-        self.service.search_immunizations.return_value = bundle
-
-        vaccine_type = "COVID19,FLU"
-        lambda_event = {
-            "headers": {"Content-Type": "application/x-www-form-urlencoded", "SupplierSystem": "test",},
-            "multiValueQueryStringParameters": {
-                self.immunization_target_key: [vaccine_type],
-                self.patient_identifier_key: [self.patient_identifier_valid_value],
-            },
-        }
-
-        # When
-        response = self.controller.search_immunizations(lambda_event)
-        mock_get_supplier_permissions.assert_called_once_with("test")
-        self.assertEqual(response["statusCode"], 403)
-        body = json.loads(response["body"])
-        self.assertEqual(body["resourceType"], "OperationOutcome")
-
-    @patch("fhir_controller.get_supplier_permissions")
-    def test_get_search_immunizations_unauthorized(self, mock_get_supplier_permissions):
-        """it should search based on patient_identifier and immunization_target"""
-        search_result = Bundle.construct()
-        mock_get_supplier_permissions.return_value = []
-        self.service.search_immunizations.return_value = search_result
-
-        vaccine_type = "COVID19"
-        params = f"{self.immunization_target_key}={vaccine_type}&" + urllib.parse.urlencode(
-            [(f"{self.patient_identifier_key}", f"{self.patient_identifier_valid_value}")]
-        )
-        lambda_event = {
-            "headers": {"Content-Type": "application/x-www-form-urlencoded", "SupplierSystem": "test"},
-            "multiValueQueryStringParameters": {
-                self.immunization_target_key: [vaccine_type],
-                self.patient_identifier_key: [self.patient_identifier_valid_value],
-            },
-        }
-
-        # When
-        response = self.controller.search_immunizations(lambda_event)
-        mock_get_supplier_permissions.assert_called_once_with("test")
-        self.assertEqual(response["statusCode"], 403)
-
     @patch("fhir_controller.get_supplier_permissions")
     def test_post_search_immunizations(self,mock_get_supplier_permissions):
         """it should search based on patient_identifier and immunization_target"""
         mock_get_supplier_permissions.return_value = ["covid19.s"]
         search_result = Bundle.construct()
-        self.service.search_immunizations.return_value = search_result
+        self.service.search_immunizations.return_value = search_result, False
 
         vaccine_type = "COVID19"
         params = f"{self.immunization_target_key}={vaccine_type}&" + urllib.parse.urlencode(
@@ -1837,7 +1791,7 @@ def test_post_search_immunizations(self,mock_get_supplier_permissions):
         response = self.controller.search_immunizations(lambda_event)
         # Then
         self.service.search_immunizations.assert_called_once_with(
-            self.nhs_number_valid_value, [vaccine_type], params, ANY, ANY
+            self.nhs_number_valid_value, [vaccine_type], params, "Test", ANY, ANY
         )
         self.assertEqual(response["statusCode"], 200)
         mock_get_supplier_permissions.assert_called_once_with("Test")
@@ -1849,7 +1803,7 @@ def test_post_search_immunizations_for_unauthorized_vaccine_type_search(self,moc
         """it should return 200 and contains warning operation outcome as the user is not having authorization for one of the vaccine type"""
         search_result = load_json_data("sample_immunization_response _for _not_done_event.json")
         bundle = Bundle.parse_obj(search_result)
-        self.service.search_immunizations.return_value = bundle
+        self.service.search_immunizations.return_value = bundle, True
         mock_get_supplier_permissions.return_value = ["covid19.s"]
 
         vaccine_type = "COVID19", "FLU"
@@ -1881,10 +1835,10 @@ def test_post_search_immunizations_for_unauthorized_vaccine_type_search(self,moc
         self.assertTrue(operation_outcome_present, "OperationOutcome resource is not present in the response")
 
     def test_post_search_immunizations_for_unauthorized_vaccine_type_search_400(self):
-        """it should return 400 as the the request is having invalid vaccine type"""
+        """it should return 400 as the request is having invalid vaccine type"""
         search_result = load_json_data("sample_immunization_response _for _not_done_event.json")
         bundle = Bundle.parse_obj(search_result)
-        self.service.search_immunizations.return_value = bundle
+        self.service.search_immunizations.return_value = bundle, False
 
         vaccine_type = "FLUE"
 
@@ -1915,7 +1869,7 @@ def test_post_search_immunizations_for_unauthorized_vaccine_type_search_403(self
         search_result = load_json_data("sample_immunization_response _for _not_done_event.json")
         bundle = Bundle.parse_obj(search_result)
         mock_get_supplier_permissions.return_value = []
-        self.service.search_immunizations.return_value = bundle
+        self.service.search_immunizations.side_effect = UnauthorizedVaxError()
 
         vaccine_type = ["COVID19", "FLU"]
         vaccine_type = ",".join(vaccine_type)
@@ -1981,11 +1935,11 @@ def test_search_immunizations_returns_400_on_ParameterException_from_parameter_p
 
     @patch("fhir_controller.get_supplier_permissions")
     def test_search_immunizations_returns_400_on_passing_superseded_nhs_number(self, mock_get_supplier_permissions):
-        "This method should return 400 as input paramter has superseded nhs number."
+        """This method should return 400 as input parameter has superseded nhs number."""
         search_result = {
             "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value does not exists"
         }
-        self.service.search_immunizations.return_value = search_result
+        self.service.search_immunizations.return_value = search_result, False
         mock_get_supplier_permissions.return_value = ["covid19.s"]
 
         vaccine_type = "COVID19"
@@ -2010,11 +1964,11 @@ def test_search_immunizations_returns_400_on_passing_superseded_nhs_number(self,
 
     @patch("fhir_controller.get_supplier_permissions")
     def test_search_immunizations_returns_200_remove_vaccine_not_done(self, mock_get_supplier_permissions):
-        "This method should return 200 but remove the data which has status as not done."
+        """This method should return 200 but remove the data which has status as not done."""
         search_result = load_json_data("sample_immunization_response _for _not_done_event.json")
         bundle = Bundle.parse_obj(search_result)
         mock_get_supplier_permissions.return_value = ["COVID19.CRUDS"]
-        self.service.search_immunizations.return_value = bundle
+        self.service.search_immunizations.return_value = bundle, False
         vaccine_type = "COVID19"
         lambda_event = {
             "headers": {
@@ -2038,7 +1992,7 @@ def test_search_immunizations_returns_200_remove_vaccine_not_done(self, mock_get
     @patch("fhir_controller.get_supplier_permissions")
     def test_self_link_excludes_extraneous_params(self, mock_get_supplier_permissions):
         search_result = Bundle.construct()
-        self.service.search_immunizations.return_value = search_result
+        self.service.search_immunizations.return_value = search_result, False
         vaccine_type = "COVID19"
         mock_get_supplier_permissions.return_value = ["covid19.CUDS"]
         params = f"{self.immunization_target_key}={vaccine_type}&" + urllib.parse.urlencode(
@@ -2063,5 +2017,5 @@ def test_self_link_excludes_extraneous_params(self, mock_get_supplier_permission
         self.controller.search_immunizations(lambda_event)
 
         self.service.search_immunizations.assert_called_once_with(
-            self.nhs_number_valid_value, [vaccine_type], params, ANY, ANY
+            self.nhs_number_valid_value, [vaccine_type], params, "Test", ANY, ANY
         )