OJ-3201: Add retry handling to nino-check lambda

mikebeeby · mikebeeby · commit 2162e6120e9f · 2025-07-04T13:27:03.000+01:00
diff --git a/integration-tests/api-gateway/check/check-lambda-retry.test.ts b/integration-tests/api-gateway/check/check-lambda-retry.test.ts
@@ -0,0 +1,87 @@
+import { ninoCheckEndpoint, createSession, getJarAuthorization } from "../endpoints";
+import { clearAttemptsTable, clearItemsFromTables, getItemByKey, queryItemsBySessionId } from "../../resources/dynamodb-helper";
+import { AUDIENCE, NINO } from "../env-variables";
+
+jest.setTimeout(30_000);
+
+describe("check-lambda retry logic", () => {
+  const errorNino = "ER123456A";
+  let sessionId: string;
+  let sessionTableName: string;
+  let privateApi: string;
+  let issuer: string | undefined;
+
+  beforeEach(async () => {
+    privateApi = `${process.env.PRIVATE_API}`;
+    sessionTableName = `${process.env.SESSION_TABLE}`;
+    const data = await getJarAuthorization();
+    const request = await data.json();
+    const session = await createSession(privateApi, request);
+    const sessionData = await session.json();
+    sessionId = sessionData.session_id;
+  });
+
+  afterEach(async () => {
+    await clearItemsFromTables(
+      {
+        tableName: `${process.env.PERSON_IDENTITY_TABLE}`,
+        items: { sessionId: sessionId },
+      },
+      {
+        tableName: `${process.env.NINO_USERS_TABLE}`,
+        items: { sessionId: sessionId },
+      },
+      {
+        tableName: sessionTableName,
+        items: { sessionId: sessionId },
+      }
+    );
+    await clearAttemptsTable(sessionId, `${process.env.USERS_ATTEMPTS_TABLE}`);
+  });
+
+  it("should not attempt further matches after 2 attempts", async () => {
+    const firstCheck = await ninoCheckEndpoint(privateApi, { "session-id": sessionId }, errorNino);
+    const firstResBody = { ...(await firstCheck.json()) };
+    expect(firstCheck.status).toEqual(200);
+    expect(firstResBody).toStrictEqual({ requestRetry: true });
+    const firstAttemptsItems = await queryItemsBySessionId(process.env.USERS_ATTEMPTS_TABLE!, sessionId);
+    expect(firstAttemptsItems.Count).toEqual(1);
+    expect(firstAttemptsItems.Items![0].attempt).toBe("FAIL");
+    expect(firstAttemptsItems.Items![0].text).toBe("CID returned no record");
+
+    const secondCheck = await ninoCheckEndpoint(privateApi, { "session-id": sessionId }, errorNino);
+    const secondResBody = { ...(await secondCheck.json()) };
+    expect(secondCheck.status).toEqual(200);
+    expect(secondResBody).toStrictEqual({ requestRetry: false });
+    const secondAttemptsItems = await queryItemsBySessionId(process.env.USERS_ATTEMPTS_TABLE!, sessionId);
+    expect(secondAttemptsItems.Count).toEqual(2);
+    expect(secondAttemptsItems.Items![0].attempt).toBe("FAIL");
+    expect(secondAttemptsItems.Items![1].attempt).toBe("FAIL");
+
+    const thirdCheck = await ninoCheckEndpoint(privateApi, { "session-id": sessionId }, errorNino);
+    const thirdResBody = { ...(await thirdCheck.json()) };
+    expect(thirdCheck.status).toEqual(200);
+    expect(thirdResBody).toStrictEqual({ requestRetry: false });
+    const thirdAttemptsItems = await queryItemsBySessionId(process.env.USERS_ATTEMPTS_TABLE!, sessionId);
+    expect(thirdAttemptsItems.Count).toEqual(2);
+  });
+
+  it("should successfully match even after a failed attempt", async () => {
+    const firstCheck = await ninoCheckEndpoint(privateApi, { "session-id": sessionId }, errorNino);
+    const firstResBody = { ...(await firstCheck.json()) };
+    expect(firstCheck.status).toEqual(200);
+    expect(firstResBody).toStrictEqual({ requestRetry: true });
+    const firstAttemptsItems = await queryItemsBySessionId(process.env.USERS_ATTEMPTS_TABLE!, sessionId);
+    expect(firstAttemptsItems.Count).toEqual(1);
+    expect(firstAttemptsItems.Items![0].attempt).toBe("FAIL");
+
+    const secondCheck = await ninoCheckEndpoint(privateApi, { "session-id": sessionId }, NINO);
+    const secondResBody = { ...(await secondCheck.json()) };
+    expect(secondCheck.status).toEqual(200);
+    expect(secondResBody).toStrictEqual({ requestRetry: false });
+    const secondAttemptsItems = await queryItemsBySessionId(process.env.USERS_ATTEMPTS_TABLE!, sessionId);
+    expect(secondAttemptsItems.Count).toEqual(2);
+    expect(secondAttemptsItems.Items!.some(item => item.attempt === 'PASS')).toBe(true);
+    expect(secondAttemptsItems.Items!.some(item => item.attempt === 'FAIL')).toBe(true);
+  });
+});
diff --git a/integration-tests/resources/dynamodb-helper.ts b/integration-tests/resources/dynamodb-helper.ts
@@ -1,11 +1,5 @@
 import { DynamoDBClient } from "@aws-sdk/client-dynamodb";
-import {
-  DeleteCommand,
-  DynamoDBDocumentClient,
-  GetCommand,
-  PutCommand,
-  QueryCommand,
-} from "@aws-sdk/lib-dynamodb";
+import { DeleteCommand, DynamoDBDocumentClient, GetCommand, PutCommand, QueryCommand } from "@aws-sdk/lib-dynamodb";
 import { createSendCommand } from "./aws-helper";
 
 type Keys = Record<string, unknown>;
@@ -20,6 +14,15 @@ const sendCommand = createSendCommand(() =>
 export const getItemByKey = (tableName: string, key: Keys) =>
   sendCommand(GetCommand, { TableName: tableName, Key: key });
 
+export const queryItemsBySessionId = (tableName: string, sessionId: string) =>
+  sendCommand(QueryCommand, {
+    TableName: tableName,
+    KeyConditionExpression: "sessionId = :sessionId",
+    ExpressionAttributeValues: {
+      ":sessionId": sessionId ,
+    },
+  });
+
 export const populateTable = (tableName: string, items: Keys) =>
   sendCommand(PutCommand, { TableName: tableName, Item: items });
 
diff --git a/lambdas/nino-check/src/handler.ts b/lambdas/nino-check/src/handler.ts
@@ -2,7 +2,7 @@ import { APIGatewayProxyEvent, APIGatewayProxyResult, Context } from "aws-lambda
 import { initOpenTelemetry } from "../../open-telemetry/src/otel-setup";
 import { writeCompletedCheck } from "./helpers/write-completed-check";
 import { NinoCheckFunctionConfig } from "./helpers/function-config";
-import { getHmrcConfig, saveAttempt, saveTxn, handlePdvResponse } from "./helpers/nino";
+import { getHmrcConfig, saveTxn, handleResponseAndSaveAttempt } from "./helpers/nino";
 import { InputBody } from "./types/input";
 import { CriError } from "../../common/src/errors/cri-error";
 import { handleErrorResponse } from "../../common/src/errors/cri-error-response";
@@ -91,15 +91,17 @@ class NinoCheckHandler implements LambdaInterface {
 
       await sendResponseReceivedEvent(functionConfig.audit, session, pdvRes.txn, deviceInformationHeader);
 
-      const ninoMatch = handlePdvResponse(pdvRes);
-
-      if (ninoMatch) {
-        await saveAttempt(dynamoClient, functionConfig.tableNames.attemptTable, session, pdvRes);
-      }
+      const ninoMatch = await handleResponseAndSaveAttempt(
+        dynamoClient,
+        functionConfig.tableNames.attemptTable,
+        session,
+        pdvRes
+      );
 
       logger.info(`Completed NINo verification - ninoMatch=${ninoMatch}.`);
 
       if (!ninoMatch && !isFinalAttempt) {
+        captureMetric(`RetryAttemptsSentMetric`);
         logger.info(`Failed to verify. This was not the last attempt - requesting the user retries.`);
         return { statusCode: 200, body: JSON.stringify({ requestRetry: true }) };
       }
diff --git a/lambdas/nino-check/src/helpers/nino.ts b/lambdas/nino-check/src/helpers/nino.ts
@@ -1,6 +1,6 @@
 import { getParametersByName } from "@aws-lambda-powertools/parameters/ssm";
 import { ISO8601DateString } from "../../../common/src/types/brands";
-import { PdvFunctionOutput } from "../hmrc-apis/types/pdv";
+import { PdvApiErrorBody, PdvFunctionOutput } from "../hmrc-apis/types/pdv";
 import { marshall } from "@aws-sdk/util-dynamodb";
 import { CriError } from "../../../common/src/errors/cri-error";
 import { DynamoDBClient, PutItemCommand, UpdateItemCommand } from "@aws-sdk/client-dynamodb";
@@ -62,54 +62,70 @@ export async function saveTxn(dynamoClient: DynamoDBClient, sessionTableName: st
   logger.info(`Saved txn to session table: ${txnRes.$metadata.httpStatusCode}.`);
 }
 
-export async function saveAttempt(
+async function saveAttempt(
   dynamoClient: DynamoDBClient,
   attemptTableName: string,
   session: NinoSessionItem,
-  pdvRes: PdvFunctionOutput
+  matchOutcome: "PASS" | "FAIL",
+  status: number,
+  error?: string
 ) {
   const newAttempt: AttemptItem = {
     sessionId: session.sessionId,
     timestamp: new Date().toISOString() as ISO8601DateString,
-    status: pdvRes.httpStatus.toString(),
-    attempt: "PASS",
+    status: status.toString(),
+    attempt: matchOutcome,
+    text: error,
     ttl: session.expiryDate,
   };
 
   const putAttemptCmd = new PutItemCommand({
     TableName: attemptTableName,
-    Item: marshall(newAttempt),
+    Item: marshall(newAttempt, { removeUndefinedValues: true }),
   });
 
   const attemptRes = await dynamoClient.send(putAttemptCmd);
 
   logger.info(`Saved attempt: ${attemptRes.$metadata.httpStatusCode}`);
 }
 
-export function handlePdvResponse(pdvRes: PdvFunctionOutput): boolean {
-  const ninoMatch = pdvRes.httpStatus === 200;
+export async function handleResponseAndSaveAttempt(
+  dynamoClient: DynamoDBClient,
+  attemptTableName: string,
+  session: NinoSessionItem,
+  pdvRes: PdvFunctionOutput
+): Promise<boolean> {
+  const responseStatus = pdvRes.httpStatus;
 
-  if (ninoMatch) {
+  if (responseStatus === 200) {
     captureMetric(`SuccessfulFirstAttemptMetric`);
-  } else if (pdvRes.httpStatus === 424) {
+    await saveAttempt(dynamoClient, attemptTableName, session, "PASS", pdvRes.httpStatus);
+    return true;
+  }
+
+  if (pdvRes.httpStatus === 401 && "errors" in (pdvRes.parsedBody ?? {})) {
+    logger.info(`Failed PDV match received.`);
+    const errorText = (pdvRes.parsedBody as PdvApiErrorBody).errors;
+    await saveAttempt(dynamoClient, attemptTableName, session, "FAIL", pdvRes.httpStatus, errorText);
+    return false;
+  }
+
+  if (pdvRes.httpStatus === 424) {
     captureMetric(`DeceasedUserMetric`);
     logger.info(`Deceased response received`);
-  } else if (pdvRes.httpStatus === 401 && "errors" in (pdvRes.parsedBody ?? {})) {
-    captureMetric(`RetryAttemptsSentMetric`);
-    logger.info(`Failed PDV match received.`);
-  } else if (pdvRes.parsedBody && "code" in pdvRes.parsedBody && pdvRes.parsedBody?.code === "INVALID_CREDENTIALS") {
+    await saveAttempt(dynamoClient, attemptTableName, session, "FAIL", pdvRes.httpStatus, pdvRes.body);
+    return false;
+  }
+
+  if (pdvRes.parsedBody && "code" in pdvRes.parsedBody && pdvRes.parsedBody?.code === "INVALID_CREDENTIALS") {
     captureMetric(`FailedHMRCAuthMetric`);
     logger.info(
       `Failed to authenticate with HMRC API: response had a code of ${pdvRes.parsedBody.code} & http status of ${pdvRes.httpStatus}`
     );
-
     throw new CriError(500, "Failed to authenticate with HMRC API");
-  } else {
-    captureMetric(`HMRCAPIErrorMetric`);
-    logger.info(`Received an unexpected error response from the PDV API - status: ${pdvRes.httpStatus}`);
-
-    throw new CriError(500, "Unexpected error with the PDV API");
   }
 
-  return ninoMatch;
+  captureMetric(`HMRCAPIErrorMetric`);
+  logger.info(`Received an unexpected error response from the PDV API - status: ${responseStatus}`);
+  throw new CriError(500, "Unexpected error with the PDV API");
 }
diff --git a/lambdas/nino-check/tests/handler.test.ts b/lambdas/nino-check/tests/handler.test.ts
@@ -21,7 +21,7 @@ import { mockLogger } from "../../common/tests/logger";
 import { handler } from "../src/handler";
 import { retrieveSession } from "../src/helpers/retrieve-session";
 import { NinoCheckFunctionConfig } from "../src/helpers/function-config";
-import { getHmrcConfig, handlePdvResponse, saveAttempt, saveTxn } from "../src/helpers/nino";
+import { getHmrcConfig, handleResponseAndSaveAttempt, saveTxn } from "../src/helpers/nino";
 import { retrieveAttempts } from "../src/helpers/retrieve-attempts";
 import { retrievePersonIdentity } from "../src/helpers/retrieve-person-identity";
 import { sendRequestSentEvent, sendResponseReceivedEvent } from "../src/helpers/audit";
@@ -30,6 +30,7 @@ import { writeCompletedCheck } from "../src/helpers/write-completed-check";
 import { getTokenFromOtg } from "../src/hmrc-apis/otg";
 import { buildPdvInput } from "../src/helpers/build-pdv-input";
 import { captureMetric } from "../../common/src/util/metrics";
+import { CriError } from "../../common/src/errors/cri-error";
 
 const mockContext: Context = {
   awsRequestId: "",
@@ -73,7 +74,7 @@ const handlerInput: Parameters<typeof handler> = [
 (retrievePersonIdentity as unknown as jest.Mock).mockResolvedValue(mockPersonIdentity);
 (getTokenFromOtg as unknown as jest.Mock).mockResolvedValue(mockOtgToken);
 (matchUserDetailsWithPdv as unknown as jest.Mock).mockResolvedValue(mockPdvRes);
-(handlePdvResponse as unknown as jest.Mock).mockResolvedValue(true);
+(handleResponseAndSaveAttempt as unknown as jest.Mock).mockResolvedValue(true);
 
 describe("nino-check handler", () => {
   beforeEach(() => {
@@ -116,7 +117,7 @@ describe("nino-check handler", () => {
       mockPdvRes.txn,
       mockDeviceInformationHeader
     );
-    expect(saveAttempt).toHaveBeenCalledWith(
+    expect(handleResponseAndSaveAttempt).toHaveBeenCalledWith(
       mockDynamoClient,
       mockFunctionConfig.tableNames.attemptTable,
       mockSession,
@@ -181,7 +182,7 @@ describe("nino-check handler", () => {
   });
 
   it("behaves correctly if ninoMatch is false", async () => {
-    (handlePdvResponse as unknown as jest.Mock).mockReturnValueOnce(false);
+    (handleResponseAndSaveAttempt as unknown as jest.Mock).mockReturnValueOnce(false);
 
     const response = await handler(...handlerInput);
 
@@ -190,6 +191,31 @@ describe("nino-check handler", () => {
       body: JSON.stringify({ requestRetry: true }),
     });
 
+    expect(writeCompletedCheck).not.toHaveBeenCalled();
+    expect(captureMetric).toHaveBeenCalledWith("RetryAttemptsSentMetric");
+  });
+
+  it("should return 200 if nino match is false but its the final attempt", async () => {
+    (retrieveAttempts as unknown as jest.Mock).mockResolvedValueOnce(1);
+    (handleResponseAndSaveAttempt as unknown as jest.Mock).mockReturnValueOnce(false);
+
+    const response = await handler(...handlerInput);
+
+    expect(response).toStrictEqual({
+      statusCode: 200,
+      body: JSON.stringify({ requestRetry: false }),
+    });
+  });
+
+  it("should return 500 if unexpected Error recieved from PDV request", async () => {
+    (handleResponseAndSaveAttempt as unknown as jest.Mock).mockImplementationOnce(() => {
+      throw new CriError(500, "Error");
+    });
+
+    const response = await handler(...handlerInput);
+
+    expect(response).toStrictEqual(internalServerError);
+
     expect(writeCompletedCheck).not.toHaveBeenCalled();
   });
 });
diff --git a/lambdas/nino-check/tests/helpers/nino.test.ts b/lambdas/nino-check/tests/helpers/nino.test.ts
diff --git a/lambdas/nino-check/tests/mocks/mockData.ts b/lambdas/nino-check/tests/mocks/mockData.ts
