FLAGSAPI-1046 return 400 if we cannot retrieve job role code (#648)

stuartmcneill1-nhs · gps035 · web-flow · commit 25a518a6aeca · 2025-06-17T17:38:52.000+01:00
* Change AcsService getUserRoleCodeAndId() to throw a BadRequestException if we are unable to retrieve the users job role code * Changed GetScrIdUAT to return 400 if unable to retrieve user role code - corrected path problem when building attachment resource url - corrected documentation to advise using combined auth-authentication * fixed stubFailedSdsService test function * FLAGSAPI-1046 reverted formatting changes * FLAGSAPI-1046 reverted most of the the auto formatting changes to simplify PR * Revert one more whitespace change * FLAGSAPI-1046 corrected test method name * FLAGSAPI-1046 reverted gitignore file * FLAGSAPI-1046 reverted change to ATTACHMENT_URL in GetScrService.java * FLAGSAPI-1046 simplified code by pre building an error string * FLAGSAPI-1046 fixed pipeline build python version error by specifying python 3.9 in run-tests yaml files * Fix editorconfig * Approved --------- Co-authored-by: Gareth Somerville <gareth.somerville2@nhs.net>
diff --git a/.editorconfig b/.editorconfig
@@ -11,7 +11,7 @@ end_of_line = lf
 [Makefile]
 indent_style = tab
 
-[*.{xml,js,json,yaml}]
+[*.{xml,js,json,yaml,yml}]
 indent_size = 2
 
 [*.postman_collection.json]
diff --git a/azure/templates/run-tests-int.yml b/azure/templates/run-tests-int.yml
@@ -5,12 +5,16 @@ parameters:
     default: false
 
 steps:
+  - task: UsePythonVersion@0
+    inputs:
+      versionSpec: "3.9"
+
   - task: s3-cache-action@1
     inputs:
-      key: 'poetry | $(SERVICE_NAME) | $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/poetry.lock'
-      location: '$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/.venv'
+      key: "poetry | $(SERVICE_NAME) | $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/poetry.lock"
+      location: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/.venv"
       debug: true
-      alias: 'Pytest'
+      alias: "Pytest"
     displayName: cache pytest dependencies
 
   - bash: |
@@ -20,44 +24,44 @@ steps:
     displayName: Setup pytests
   # Smoketests
   - ${{ if parameters.smoke_tests }}:
-    - template: "azure/components/aws-assume-role.yml@common"
-      parameters:
-        role: "auto-ops"
-        profile: "apm_ptl"
-    - template: "azure/components/get-aws-secrets-and-ssm-params.yml@common"
-      parameters:
-        secret_file_ids:
-          - ptl/app-credentials/jwt_testing/non-prod/JWT_TESTING_PRIVATE_KEY
-          - ptl/app-credentials/jwt_testing/non-prod/ID_TOKEN_NHS_LOGIN_PRIVATE_KEY
-        config_ids:
-          - /ptl/azure-devops/summary-care-record/int/CLIENT_ID_INT
-        secret_ids:
-          - ptl/azure-devops/summary-care-record/int/CLIENT_SECRET_INT
-    - bash: |
-        wait
-        sleep 350
-        export RELEASE_RELEASEID=$(Build.BuildId)
-        export SOURCE_COMMIT_ID=$(Build.SourceVersion)
-        export APIGEE_ENVIRONMENT="$(ENVIRONMENT)"
-        export SERVICE_BASE_PATH="$(SERVICE_BASE_PATH)"
-        export STATUS_ENDPOINT_API_KEY="$(status-endpoint-api-key)"
-        export APIGEE_PRODUCT="$(FULLY_QUALIFIED_SERVICE_NAME)"
-        export OAUTH_PROXY="oauth2"
-        export OAUTH_BASE_URI="https://$(ENVIRONMENT).api.service.nhs.uk"
-        export APIGEE_API_TOKEN="$(secret.AccessToken)"
-        export REDIRECT_URI="https://example.org/callback"
-        export CLIENT_ID="$(CLIENT_ID_INT)"
-        export CLIENT_SECRET="$(CLIENT_SECRET_INT)"
-        export JWT_PRIVATE_KEY_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(JWT_TESTING_PRIVATE_KEY)"
-        export ID_TOKEN_NHS_LOGIN_PRIVATE_KEY_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(ID_TOKEN_NHS_LOGIN_PRIVATE_KEY)"
-        export JWT_PRIVATE_KEY_APP_RESTRICTED_ABSOLUTE_PATH=""
-        export AUTHENTICATE_URL=""
-        make prod-smoketest
-      workingDirectory: $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)
-      displayName: Run smoketests
-    - task: PublishTestResults@2
-      displayName: 'Publish smoketest results'
-      condition: always()
-      inputs:
-        testResultsFiles: '$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/smoketest-report.xml'
-        failTaskOnFailedTests: true
+      - template: "azure/components/aws-assume-role.yml@common"
+        parameters:
+          role: "auto-ops"
+          profile: "apm_ptl"
+      - template: "azure/components/get-aws-secrets-and-ssm-params.yml@common"
+        parameters:
+          secret_file_ids:
+            - ptl/app-credentials/jwt_testing/non-prod/JWT_TESTING_PRIVATE_KEY
+            - ptl/app-credentials/jwt_testing/non-prod/ID_TOKEN_NHS_LOGIN_PRIVATE_KEY
+          config_ids:
+            - /ptl/azure-devops/summary-care-record/int/CLIENT_ID_INT
+          secret_ids:
+            - ptl/azure-devops/summary-care-record/int/CLIENT_SECRET_INT
+      - bash: |
+          wait
+          sleep 350
+          export RELEASE_RELEASEID=$(Build.BuildId)
+          export SOURCE_COMMIT_ID=$(Build.SourceVersion)
+          export APIGEE_ENVIRONMENT="$(ENVIRONMENT)"
+          export SERVICE_BASE_PATH="$(SERVICE_BASE_PATH)"
+          export STATUS_ENDPOINT_API_KEY="$(status-endpoint-api-key)"
+          export APIGEE_PRODUCT="$(FULLY_QUALIFIED_SERVICE_NAME)"
+          export OAUTH_PROXY="oauth2"
+          export OAUTH_BASE_URI="https://$(ENVIRONMENT).api.service.nhs.uk"
+          export APIGEE_API_TOKEN="$(secret.AccessToken)"
+          export REDIRECT_URI="https://example.org/callback"
+          export CLIENT_ID="$(CLIENT_ID_INT)"
+          export CLIENT_SECRET="$(CLIENT_SECRET_INT)"
+          export JWT_PRIVATE_KEY_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(JWT_TESTING_PRIVATE_KEY)"
+          export ID_TOKEN_NHS_LOGIN_PRIVATE_KEY_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(ID_TOKEN_NHS_LOGIN_PRIVATE_KEY)"
+          export JWT_PRIVATE_KEY_APP_RESTRICTED_ABSOLUTE_PATH=""
+          export AUTHENTICATE_URL=""
+          make prod-smoketest
+        workingDirectory: $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)
+        displayName: Run smoketests
+      - task: PublishTestResults@2
+        displayName: "Publish smoketest results"
+        condition: always()
+        inputs:
+          testResultsFiles: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/smoketest-report.xml"
+          failTaskOnFailedTests: true
diff --git a/azure/templates/run-tests-prod.yml b/azure/templates/run-tests-prod.yml
@@ -1,10 +1,14 @@
 steps:
+  - task: UsePythonVersion@0
+    inputs:
+      versionSpec: "3.9"
+
   - task: s3-cache-action@1
     inputs:
-      key: 'poetry | $(SERVICE_NAME) | $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/poetry.lock'
-      location: '$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/.venv'
+      key: "poetry | $(SERVICE_NAME) | $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/poetry.lock"
+      location: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/.venv"
       debug: true
-      alias: 'Pytest'
+      alias: "Pytest"
     displayName: cache pytest dependencies
 
   - bash: |
@@ -41,8 +45,8 @@ steps:
     workingDirectory: $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)
     displayName: Run smoketests
   - task: PublishTestResults@2
-    displayName: 'Publish smoketest results'
+    displayName: "Publish smoketest results"
     condition: always()
     inputs:
-      testResultsFiles: '$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/smoketest-report.xml'
+      testResultsFiles: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/smoketest-report.xml"
       failTaskOnFailedTests: true
diff --git a/azure/templates/run-tests.yml b/azure/templates/run-tests.yml
@@ -5,12 +5,16 @@ parameters:
     default: false
 
 steps:
+  - task: UsePythonVersion@0
+    inputs:
+      versionSpec: "3.9"
+
   - task: s3-cache-action@1
     inputs:
-      key: 'poetry | $(SERVICE_NAME) | $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/poetry.lock'
-      location: '$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/.venv'
+      key: "poetry | $(SERVICE_NAME) | $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/poetry.lock"
+      location: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/.venv"
       debug: true
-      alias: 'Pytest'
+      alias: "Pytest"
     displayName: cache pytest dependencies
 
   - bash: |
@@ -20,44 +24,44 @@ steps:
     displayName: Setup pytests
   # Smoketests.
   - ${{ if parameters.smoke_tests }}:
-    - template: "azure/components/aws-assume-role.yml@common"
-      parameters:
-        role: "auto-ops"
-        profile: "apm_ptl"
-    - template: "azure/components/get-aws-secrets-and-ssm-params.yml@common"
-      parameters:
-        secret_file_ids:
-          - ptl/app-credentials/jwt_testing/non-prod/JWT_TESTING_PRIVATE_KEY
-          - ptl/app-credentials/jwt_testing/non-prod/ID_TOKEN_NHS_LOGIN_PRIVATE_KEY
-          - ptl/azure-devops/summary-care-record/JWT_TESTING_APP_RESTRICTED_PRIVATE_KEY
-        config_ids:
-          - /ptl/azure-devops/env-internal-dev/test-app/internal-testing-internal-dev/CLIENT_ID
-          - /ptl/azure-devops/env-internal-dev/test-app/internal-testing-internal-dev/CLIENT_SECRET
-    - bash: |
-        wait
-        sleep 350
-        export RELEASE_RELEASEID=$(Build.BuildId)
-        export SOURCE_COMMIT_ID=$(Build.SourceVersion)
-        export APIGEE_ENVIRONMENT="$(ENVIRONMENT)"
-        export SERVICE_BASE_PATH="$(SERVICE_BASE_PATH)"
-        export STATUS_ENDPOINT_API_KEY="$(status-endpoint-api-key)"
-        export APIGEE_PRODUCT="$(FULLY_QUALIFIED_SERVICE_NAME)"
-        export OAUTH_PROXY="oauth2-mock"
-        export OAUTH_BASE_URI="https://$(ENVIRONMENT).api.service.nhs.uk"
-        export APIGEE_API_TOKEN="$(secret.AccessToken)"
-        export REDIRECT_URI="https://example.org/callback"
-        export CLIENT_ID="$(CLIENT_ID)"
-        export CLIENT_SECRET="$(CLIENT_SECRET)"
-        export JWT_PRIVATE_KEY_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(JWT_TESTING_PRIVATE_KEY)"
-        export JWT_PRIVATE_KEY_APP_RESTRICTED_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(JWT_TESTING_APP_RESTRICTED_PRIVATE_KEY)"
-        export ID_TOKEN_NHS_LOGIN_PRIVATE_KEY_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(ID_TOKEN_NHS_LOGIN_PRIVATE_KEY)"
-        export AUTHENTICATE_URL="https://nhsd-apim-testing-internal-dev.herokuapp.com/"
-        make smoketest
-      workingDirectory: $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)
-      displayName: Run smoketests
-    - task: PublishTestResults@2
-      displayName: 'Publish smoketest results'
-      condition: always()
-      inputs:
-        testResultsFiles: '$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/smoketest-report.xml'
-        failTaskOnFailedTests: true
+      - template: "azure/components/aws-assume-role.yml@common"
+        parameters:
+          role: "auto-ops"
+          profile: "apm_ptl"
+      - template: "azure/components/get-aws-secrets-and-ssm-params.yml@common"
+        parameters:
+          secret_file_ids:
+            - ptl/app-credentials/jwt_testing/non-prod/JWT_TESTING_PRIVATE_KEY
+            - ptl/app-credentials/jwt_testing/non-prod/ID_TOKEN_NHS_LOGIN_PRIVATE_KEY
+            - ptl/azure-devops/summary-care-record/JWT_TESTING_APP_RESTRICTED_PRIVATE_KEY
+          config_ids:
+            - /ptl/azure-devops/env-internal-dev/test-app/internal-testing-internal-dev/CLIENT_ID
+            - /ptl/azure-devops/env-internal-dev/test-app/internal-testing-internal-dev/CLIENT_SECRET
+      - bash: |
+          wait
+          sleep 350
+          export RELEASE_RELEASEID=$(Build.BuildId)
+          export SOURCE_COMMIT_ID=$(Build.SourceVersion)
+          export APIGEE_ENVIRONMENT="$(ENVIRONMENT)"
+          export SERVICE_BASE_PATH="$(SERVICE_BASE_PATH)"
+          export STATUS_ENDPOINT_API_KEY="$(status-endpoint-api-key)"
+          export APIGEE_PRODUCT="$(FULLY_QUALIFIED_SERVICE_NAME)"
+          export OAUTH_PROXY="oauth2-mock"
+          export OAUTH_BASE_URI="https://$(ENVIRONMENT).api.service.nhs.uk"
+          export APIGEE_API_TOKEN="$(secret.AccessToken)"
+          export REDIRECT_URI="https://example.org/callback"
+          export CLIENT_ID="$(CLIENT_ID)"
+          export CLIENT_SECRET="$(CLIENT_SECRET)"
+          export JWT_PRIVATE_KEY_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(JWT_TESTING_PRIVATE_KEY)"
+          export JWT_PRIVATE_KEY_APP_RESTRICTED_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(JWT_TESTING_APP_RESTRICTED_PRIVATE_KEY)"
+          export ID_TOKEN_NHS_LOGIN_PRIVATE_KEY_ABSOLUTE_PATH="$(Pipeline.Workspace)/secrets/$(ID_TOKEN_NHS_LOGIN_PRIVATE_KEY)"
+          export AUTHENTICATE_URL="https://nhsd-apim-testing-internal-dev.herokuapp.com/"
+          make smoketest
+        workingDirectory: $(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)
+        displayName: Run smoketests
+      - task: PublishTestResults@2
+        displayName: "Publish smoketest results"
+        condition: always()
+        inputs:
+          testResultsFiles: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/smoketest-report.xml"
+          failTaskOnFailedTests: true
diff --git a/docker/service/build.gradle b/docker/service/build.gradle
@@ -50,10 +50,16 @@ dependencies {
     testImplementation "io.rest-assured:json-path:4.4.0"
     testImplementation "io.rest-assured:xml-path:4.4.0"
     testImplementation "com.github.tomakehurst:wiremock-jre8-standalone:2.31.0"
+    testImplementation(platform('org.junit:junit-bom:5.13.0'))
+    testImplementation('org.junit.jupiter:junit-jupiter')
+    testRuntimeOnly('org.junit.platform:junit-platform-launcher')
 }
 
 test {
     useJUnitPlatform()
+	testLogging {
+		events "passed", "skipped", "failed"
+	}    
 }
 
 sourceSets {
diff --git a/docker/service/src/integration-test/java/uk/nhs/adaptors/scr/uat/SetAcsUAT.java b/docker/service/src/integration-test/java/uk/nhs/adaptors/scr/uat/SetAcsUAT.java
@@ -121,6 +121,20 @@ public void testSetAcsPermissionBadRequest(TestData testData) throws Exception {
             .andExpect(content().json(testData.getFhirResponse()));
     }
 
+    @ParameterizedTest(name = "[{index}] - {0}")
+    @ArgumentsSource(SetAcsBadRequest.class)
+    public void testSetAcsPermissionNoRoleCodeBadRequest(TestData testData) throws Exception {
+        // FLAGSAPI-1046 should return Bad Request if no role code is returned from SDS
+        // or Identity Service
+        stubFailedIdentityService();
+        stubFailedSdsService();
+        stubSpineAcsEndpoint(acsErrorResponse);
+
+        performRequest(testData.getFhirRequest())
+                .andExpect(status().isBadRequest())
+                .andExpect(content().json(testData.getFhirResponse()));
+    }
+
     private ResultActions performRequest(String request) throws Exception {
         return mockMvc.perform(post(ACS_ENDPOINT)
             .contentType(APPLICATION_FHIR_JSON)
@@ -154,6 +168,17 @@ private void stubSdsService(Resource response) throws IOException {
                     .withBody(readString(response.getFile().toPath(), UTF_8))));
     }
 
+    private void stubFailedSdsService() {
+        wireMockServer.stubFor(
+            WireMock.get(WireMock.urlPathEqualTo(PRACTITIONER_ROLE_ENDPOINT))
+                .withQueryParam(USER_ID_QUERY_PARAM,
+                    containing(NHSD_SESSION_URID))
+                .willReturn(aResponse()
+                    .withStatus(OK.value())
+                    .withHeader(CONTENT_TYPE, APPLICATION_JSON_VALUE)
+                    .withBody("")));
+    }
+
     private void stubIdentityService(Resource response) throws IOException {
         wireMockServer.stubFor(
             WireMock.get(USER_INFO_ENDPOINT)
diff --git a/docker/service/src/main/java/uk/nhs/adaptors/scr/services/AcsService.java b/docker/service/src/main/java/uk/nhs/adaptors/scr/services/AcsService.java
@@ -90,12 +90,21 @@ private Pair<String, String> getUserRoleCodeAndId(String authorisation, String n
 
         }
 
+        String errorMessage = String.format("Unable to determine Job Role Code for "
+                + "the given RoleID via the SDS Service: %s", nhsdSessionUrid);
+
+        // if we haven't retrieved a role code from openID we try the SDS service
         try {
-            return Pair.of(sdsService.getUserRoleCode(nhsdSessionUrid), nhsdIdentity);
+            var roleCode = sdsService.getUserRoleCode(nhsdSessionUrid);
+            if (roleCode != null && !roleCode.isEmpty()) {
+                return Pair.of(roleCode, nhsdIdentity);
+            }
         } catch (BadRequestException | URISyntaxException e) {
-            throw new BadRequestException(String.format("Unable to determine SDS Job Role Code for "
-                + "the given RoleID: %s", nhsdSessionUrid));
+            LOGGER.info(errorMessage);
+            throw new BadRequestException(errorMessage);
         }
+        LOGGER.info(errorMessage);
+        throw new BadRequestException(errorMessage);
     }
 
     private String prepareAcsRequest(ParametersParameterComponent parameter, RequestData requestData, String sdsJobRoleCode,
diff --git a/specification/summary-care-record.yaml b/specification/summary-care-record.yaml
@@ -88,10 +88,8 @@ info:
     - a health or care staff providing direct care to patients
     - strongly authenticated, using either an [NHS smartcard or a modern alternative](https://digital.nhs.uk/developer/guides-and-documentation/security-and-authorisation/nhs-smartcards-for-developers) available via [NHS Care Identity Service 2 (NHS CIS2)](https://digital.nhs.uk/services/nhs-identity)
 
-    The API uses OAuth 2.0 to authorise the calling system. It supports the following security patterns:
-
+    The API uses OAuth 2.0 to authorise the calling system. It only supports CIS2 combined authentication and authorisation (see link below). Do not use separate authentication and authorisation:
     - [user-restricted RESTful API - using NHS CIS2 - combined authentication and authorisation](https://digital.nhs.uk/developer/guides-and-documentation/security-and-authorisation/user-restricted-restful-apis-nhs-cis2-combined-authentication-and-authorisation)
-    - [user-restricted RESTful API - using NHS CIS2 - separate authentication and authorisation](https://digital.nhs.uk/developer/guides-and-documentation/security-and-authorisation/user-restricted-restful-apis-nhs-cis2-separate-authentication-and-authorisation)
 
     For more details, see [user-restricted APIs](https://digital.nhs.uk/developer/guides-and-documentation/security-and-authorisation#user-restricted-apis).
 
