Chore: [AEA-0000] - Refactor notify lambda, and rearrange the secrets (#2141)

## Summary

- Routine Change

### Details

*MUST BE MERGED ONLY AFTER THIS PR:
NHSDigital/electronic-prescription-service-account-resources#1474

Notify's callbacks do NOT sign their messages with the digital
onboarding service name, as I'd thought. Rather, they sign with the ID
of the app. i.e., they need `bd492cde-b67e-487b-97fd-44b7414c8e95`, not
`EPS PSU notify callback int` for
[this](https://onboarding.prod.api.platform.nhs.uk/MyApplications/ApplicationDetails?appId=bd492cde-b67e-487b-97fd-44b7414c8e95)
app. However, we still need the name for the CIS2 token exchange stage,
so that can't be just fixed in the config.

Luckily, I have a spare secret (I must have known this at some point,
and forgotten during the implementation), so I'm going to use that. I
will redefine the secrets to be like this:
- `secrets-PSU-Notify-Application-Name` - The application name, e.g.
`EPS PSU notify callback int`
- `secrets-PSU-Notify-Application-ID` - The Application ID, e.g.
`bd492cde-b67e-487b-97fd-44b7414c8e95`
- `secrets-PSU-Notify-API-Key` (no change) - Holds the API key of the
DoS app
- `secrets-PSU-Notify-PrivateKey` (no change) - Holds the private
signing key

While I'm here, I'm also going to do a bit of refactoring on the lambda
code. Nothing should meaningfully change there, though.

Author: wildjames

SAMtemplates/functions/main.yaml

@@ -474,7 +474,7 @@ Resources:
         Variables:
           LOG_LEVEL: !Ref LogLevel
           TABLE_NAME: !Ref PrescriptionNotificationStatesTableName
-          APP_NAME_SECRET: secrets-PSU-Notify-Application-Name
+          APP_ID_SECRET: secrets-PSU-Notify-Application-ID
           API_KEY_SECRET:  secrets-PSU-Notify-API-Key
     Metadata:
       BuildMethod: esbuild

packages/nhsNotifyLambda/src/nhsNotifyLambda.ts

@@ -14,13 +14,13 @@ import {
   removeSQSMessages,
   reportQueueStatus,
   drainQueue,
-  makeBatchNotifyRequest,
+  handleNotifyRequests,
   NotifyDataItemMessage
 } from "./utils"
 
 const logger = new Logger({serviceName: "nhsNotify"})
 
-const MAX_QUEUE_RUNTIME = 14*60*1000 // 14 minutes, to avoid Lambda timeout issues (timeout is 15 minutes)
+const MAX_QUEUE_RUNTIME = 14 * 60 * 1000 // 14 minutes, to avoid Lambda timeout issues (timeout is 15 minutes)
 
 /**
  * Process a single batch of SQS messages: filter, notify, persist, and clean up.
@@ -52,7 +52,7 @@ async function processBatch(
   // Send notifications
   let processed: Array<NotifyDataItemMessage> = []
   try {
-    processed = await makeBatchNotifyRequest(logger, routingId, toProcess)
+    processed = await handleNotifyRequests(logger, routingId, toProcess)
   } catch (err) {
     logger.error("Notification request failed, will retry", {error: err, toProcess})
   }

packages/nhsNotifyLambda/src/utils/index.ts

@@ -1,7 +1,7 @@
 import {NotifyDataItemMessage} from "./types"
 import {checkCooldownForUpdate, addPrescriptionMessagesToNotificationStateStore} from "./dynamo"
 import {removeSQSMessages, drainQueue, reportQueueStatus} from "./sqs"
-import {makeBatchNotifyRequest} from "./notify"
+import {handleNotifyRequests, makeRealNotifyRequest} from "./notify"
 
 export {
   NotifyDataItemMessage,
@@ -10,5 +10,6 @@ export {
   removeSQSMessages,
   drainQueue,
   reportQueueStatus,
-  makeBatchNotifyRequest
+  handleNotifyRequests,
+  makeRealNotifyRequest
 }

packages/nhsNotifyLambda/src/utils/notify.ts

@@ -34,32 +34,57 @@ function estimateSize(obj: unknown) {
 }
 
 /**
- * Returns the original data, updated with the status returned by NHS notify.
- * Does not return data for messages that failed to send.
- *
- * @param logger AWS logging object
- * @param routingPlanId The Notify routing plan ID with which to process the data
- * @param data The details for the notification
+ * Create and configure an axios instance for Notify
+ * @param logger
+ * @param notifyBaseUrl - The base URL for the Notify API. DOES NOT include /comms, or anything after that.
+ * @param bearerToken - The CIS2 OAuth bearer token to use for authentication
+ * @returns
  */
-export async function makeBatchNotifyRequest(
+function setupAxios(logger: Logger, notifyBaseUrl: string, bearerToken: string): ReturnType<typeof axios.create> {
+
+  const axiosInstance = axios.create({
+    baseURL: notifyBaseUrl + "/comms",
+    headers: {
+      Accept: "*/*",
+      "Content-Type": "application/vnd.api+json",
+      Authorization: `Bearer ${bearerToken}`
+    }
+  })
+
+  // Retry configuration for rate limiting
+  const onAxiosRetry = (retryCount: number, error: unknown) => {
+    logger.warn(`Call to notify failed - retrying. Retry count ${retryCount}`, {error})
+  }
+
+  // Axios-retry respects the `Retry-After` header OOTB
+  axiosRetry(axiosInstance, {
+    retries: 5,
+    onRetry: onAxiosRetry
+  })
+
+  return axiosInstance
+}
+
+/**
+ * Handles making requests to NHS Notify for a batch of messages.
+ * Decides whether to make real requests or fake ones based on config.
+ * @param logger
+ * @param routingPlanId - The Notify routing plan ID with which to process the data
+ * @param data - PSU SQS messages to process
+ * @returns
+ */
+export async function handleNotifyRequests(
   logger: Logger,
   routingPlanId: string,
   data: Array<NotifyDataItemMessage>
 ): Promise<Array<NotifyDataItemMessage>> {
-  const {makeRealNotifyRequests, notifyApiBaseUrlRaw} = await loadConfig()
-
-  if (!notifyApiBaseUrlRaw) throw new Error("NOTIFY_API_BASE_URL is not defined in the environment variables!")
-
-  // Just to be safe, trim any whitespace. Also, secrets may be bytes, so make sure it's a string
-  const BASE_URL = notifyApiBaseUrlRaw.trim()
 
   // Early break for empty data
   if (data.length === 0) {
     return []
   }
 
-  // Shared between all messages in this batch
-  const messageBatchReference = v4()
+  const configPromise = loadConfig()
 
   // Map the NotifyDataItems into the structure needed for notify
   const messages: Array<MessageBatchItem> = data.flatMap(item => {
@@ -73,10 +98,80 @@ export async function makeBatchNotifyRequest(
       messageReference: item.messageReference,
       recipient: {nhsNumber: item.PSUDataItem.PatientNHSNumber},
       originator: {odsCode: item.PSUDataItem.PharmacyODSCode},
-      personalisation: {}
+      personalisation: {},
+      PSURequestId: item.PSUDataItem.RequestID
     }]
   })
 
+  // Check if we should make real requests
+  const {makeRealNotifyRequestsFlag, notifyApiBaseUrlRaw} = await configPromise
+  if (!makeRealNotifyRequestsFlag || !notifyApiBaseUrlRaw) return await makeFakeNotifyRequest(logger, data, messages)
+
+  if (!notifyApiBaseUrlRaw) throw new Error("NOTIFY_API_BASE_URL is not defined in the environment variables!")
+  // Just to be safe, trim any whitespace. Also, secrets may be bytes, so make sure it's a string
+  const notifyBaseUrl = notifyApiBaseUrlRaw.trim()
+
+  return await makeRealNotifyRequest(logger, routingPlanId, notifyBaseUrl, data, messages)
+}
+
+/**
+ * Simulates making requests to NHS Notify for a batch of messages.
+ * Waits a short time, then returns "successful" responses for all messages.
+ */
+async function makeFakeNotifyRequest(
+  logger: Logger,
+  data: Array<NotifyDataItemMessage>,
+  messages: Array<MessageBatchItem>
+): Promise<Array<NotifyDataItemMessage>> {
+
+  logger.info("Not doing real Notify requests. Simply waiting for some time and returning success on all messages")
+  await new Promise(f => setTimeout(f, DUMMY_NOTIFY_DELAY_MS))
+
+  const messageStatus = "silent running"
+  const messageBatchReference = v4()
+
+  logger.info("Requested notifications OK!", {
+    messageBatchReference,
+    messageReferences: messages.map(e => ({
+      nhsNumber: e.recipient.nhsNumber,
+      messageReference: e.messageReference,
+      psuRequestId: data.find((el) => el.messageReference === e.messageReference)?.PSUDataItem.RequestID
+    })),
+    deliveryStatus: messageStatus // TODO: change splunk report query to messageStatus
+  })
+
+  // Map each input item to a "successful" NotifyDataItemMessage
+  return data.map(item => {
+    return {
+      ...item,
+      messageBatchReference,
+      messageStatus,
+      notifyMessageId: v4() // Create a dummy UUID
+    }
+  })
+}
+
+/**
+ * Makes real requests to NHS Notify for a batch of messages.
+ * Handles splitting large batches into smaller ones as needed.
+ *
+ * @param logger - AWS logging object
+ * @param routingPlanId - The Notify routing plan ID with which to process the data
+ * @param data - The details for the notification
+ */
+export async function makeRealNotifyRequest(
+  logger: Logger,
+  routingPlanId: string,
+  notifyBaseUrl: string,
+  data: Array<NotifyDataItemMessage>,
+  messages: Array<MessageBatchItem>,
+  bearerToken?: string,
+  axiosInstance?: ReturnType<typeof axios.create>
+): Promise<Array<NotifyDataItemMessage>> {
+
+  // Shared between all messages in this batch
+  const messageBatchReference = v4()
+
   const body: CreateMessageBatchRequest = {
     data: {
       type: "MessageBatch" as const,
@@ -88,80 +183,35 @@ export async function makeBatchNotifyRequest(
     }
   }
 
+  // Lazily get the bearer token and axios instance, so we only do it once even if we recurse
+  if (!bearerToken) bearerToken = await tokenExchange(logger, notifyBaseUrl)
+  if (!axiosInstance) axiosInstance = setupAxios(logger, notifyBaseUrl, bearerToken)
+
   // Recursive split if too large
-  if (data.length >= NOTIFY_REQUEST_MAX_ITEMS || estimateSize(body) > NOTIFY_REQUEST_MAX_BYTES) {
+  if (messages.length >= NOTIFY_REQUEST_MAX_ITEMS || estimateSize(body) > NOTIFY_REQUEST_MAX_BYTES) {
     logger.info("Received a large payload - splitting in half and trying again",
-      {messageCount: data.length, estimatedSize: estimateSize(body)}
+      {messageCount: messages.length, estimatedSize: estimateSize(body)}
     )
-    const mid = Math.floor(data.length / 2)
-    const firstHalf = data.slice(0, mid)
-    const secondHalf = data.slice(mid)
+    const mid = Math.floor(messages.length / 2)
+    const firstHalf = messages.slice(0, mid)
+    const secondHalf = messages.slice(mid)
+
     // send both halves in parallel
     const [res1, res2] = await Promise.all([
-      makeBatchNotifyRequest(logger, routingPlanId, firstHalf),
-      makeBatchNotifyRequest(logger, routingPlanId, secondHalf)
+      makeRealNotifyRequest(logger, routingPlanId, notifyBaseUrl, data, firstHalf, bearerToken, axiosInstance),
+      makeRealNotifyRequest(logger, routingPlanId, notifyBaseUrl, data, secondHalf, bearerToken, axiosInstance)
     ])
     return [...res1, ...res2]
   }
 
-  if (!makeRealNotifyRequests) {
-    logger.info("Not doing real Notify requests. Simply waiting for some time and returning success on all messages")
-    await new Promise(f => setTimeout(f, DUMMY_NOTIFY_DELAY_MS))
-
-    const messageStatus = "silent running"
-
-    logger.info("Requested notifications OK!", {
-      messageBatchReference,
-      messageReferences: messages.map(e => ({
-        nhsNumber: e.recipient.nhsNumber,
-        messageReference: e.messageReference,
-        psuRequestId: data.find((el) => el.messageReference === e.messageReference)?.PSUDataItem.RequestID
-      })),
-      deliveryStatus: messageStatus // TODO: change splunk report query to messageStatus
-    })
-
-    // Map each input item to a "successful" NotifyDataItemMessage
-    return data.map(item => {
-      return {
-        ...item,
-        messageBatchReference,
-        messageStatus,
-        notifyMessageId: v4() // Create a dummy UUID
-      }
-    })
-  }
-
-  // This is actually going to hit notify, so get the bearer token
-  const bearerToken = await tokenExchange(logger, BASE_URL)
-
-  logger.info("Making a request for notifications to NHS notify", {count: data.length, routingPlanId})
-
-  // Create an axios instance configured for Notify
-  const axiosInstance = axios.create({
-    baseURL: BASE_URL + "/comms",
-    headers: {
-      Accept: "*/*",
-      "Content-Type": "application/vnd.api+json",
-      Authorization: `Bearer ${bearerToken}`
-    }
-  })
-
-  // Retry configuration for rate limiting
-  const onAxiosRetry = (retryCount: number, error: unknown) => {
-    logger.warn(`Call to notify failed - retrying. Retry count ${retryCount}`, {error})
-  }
-
-  // Axios-retry respects the `Retry-After` header
-  axiosRetry(axiosInstance, {
-    retries: 5,
-    onRetry: onAxiosRetry
-  })
+  logger.info("Making a request for notifications to NHS notify", {count: messages.length, routingPlanId})
 
   try {
     const resp = await axiosInstance.post<CreateMessageBatchResponse>("/v1/message-batches", body)
 
+    // From here is just logging stuff for reporting, and mapping the response back to the input data
+
     if (resp.status === 201) {
-      const returnedMessages = resp.data.data.attributes.messages
       logger.info("Requested notifications OK!", {
         messageBatchReference,
         messageReferences: messages.map(e => ({
@@ -172,20 +222,24 @@ export async function makeBatchNotifyRequest(
         deliveryStatus: "requested" // TODO: change splunk report query to messageStatus
       })
 
-      // Map each input item to a NotifyDataItemMessage, marking success and attaching the notify ID
-      return data.map(item => {
-        const match = returnedMessages.find(
-          m => m.messageReference === item.messageReference
-        )
-
-        // SUCCESS
-        return {
-          ...item,
-          messageBatchReference,
-          messageStatus: match ? "requested" : "notify request failed",
-          notifyMessageId: match?.id
-        }
-      })
+      // Map each input item to a NotifyDataItemMessage, marking success and attaching the notify ID.
+      // Only return items that belong to *this* batch (so we handle recursive splits correctly).
+      const batchRefs = new Set(messages.map(m => m.messageReference))
+      const returnedByRef = new Map(
+        resp.data.data.attributes.messages.map(m => [m.messageReference, m])
+      )
+
+      return data
+        .filter(item => batchRefs.has(item.messageReference))
+        .map(item => {
+          const match = returnedByRef.get(item.messageReference)
+          return {
+            ...item,
+            messageBatchReference,
+            messageStatus: match ? "requested" : "notify request failed",
+            notifyMessageId: match?.id
+          }
+        })
 
     } else {
       logger.error("Notify batch request failed", {

packages/nhsNotifyLambda/src/utils/ssm.ts

@@ -3,12 +3,12 @@ import {SSMProvider} from "@aws-lambda-powertools/parameters/ssm"
 const ssm = new SSMProvider()
 
 export interface NotifyConfig {
-  makeRealNotifyRequests: boolean
+  makeRealNotifyRequestsFlag: boolean
   notifyApiBaseUrlRaw: string
 }
 
 export async function loadConfig(): Promise<{
-  makeRealNotifyRequests: boolean,
+  makeRealNotifyRequestsFlag: boolean,
   notifyApiBaseUrlRaw: string
 }> {
   const paramNames = {
@@ -24,7 +24,7 @@ export async function loadConfig(): Promise<{
     .trim()
 
   return {
-    makeRealNotifyRequests: realNotifyParam === "true",
+    makeRealNotifyRequestsFlag: realNotifyParam === "true",
     notifyApiBaseUrlRaw: all[process.env.NOTIFY_API_BASE_URL_PARAM!] as string
   }
 }

packages/nhsNotifyLambda/tests/testNhsNotifyLambda.test.ts

@@ -33,7 +33,7 @@ jest.unstable_mockModule(
     addPrescriptionMessagesToNotificationStateStore: mockAddPrescriptionMessagesToNotificationStateStore,
     removeSQSMessages: mockRemoveSQSMessages,
     checkCooldownForUpdate: mockCheckCooldownForUpdate,
-    makeBatchNotifyRequest: mockMakeBatchNotifyRequest
+    handleNotifyRequests: mockMakeBatchNotifyRequest
   })
 )
 
@@ -205,7 +205,7 @@ describe("Unit test for NHS Notify lambda handler", () => {
 
     // returns true if the request ID is "fresh"
     mockCheckCooldownForUpdate.mockImplementation((logger, update) => {
-      const u = update as { RequestID: string }
+      const u = update as {RequestID: string}
       return Promise.resolve(u.RequestID === "fresh")
     })
 
@@ -261,7 +261,7 @@ describe("Unit test for NHS Notify lambda handler", () => {
 
     // returns true if the request ID is "fresh"
     mockCheckCooldownForUpdate.mockImplementation((logger, update) => {
-      const u = update as { RequestID: string }
+      const u = update as {RequestID: string}
       return Promise.resolve(u.RequestID === "fresh")
     })