OJ-3227 - Improvements to common code

Author: barnabycollins

infrastructure/template.yaml

@@ -839,6 +839,7 @@ Resources:
           ISSUER: !Sub "{{resolve:ssm:/${CommonStackName}/verifiable-credential/issuer}}"
           EVENT_BUS_NAME: !Ref CheckHmrcEventBus
           EVENT_BUS_SOURCE: !FindInMap [EnvironmentConfiguration, !Ref Environment, DOMAINNAME]
+          LOG_FULL_ERRORS: !If [IsProdEnvironment, "false", "true"]
       Policies:
         - DynamoDBReadPolicy:
             TableName: !Sub "{{resolve:ssm:/${CommonStackName}/SessionTableName}}"
@@ -1417,6 +1418,7 @@ Resources:
           AUDIT_EVENT_BUS: !Ref CheckHmrcEventBus
           AUDIT_SOURCE: !FindInMap [EnvironmentConfiguration, !Ref Environment, DOMAINNAME]
           AUDIT_ISSUER: !Sub "{{resolve:ssm:/${CommonStackName}/verifiable-credential/issuer}}"
+          LOG_FULL_ERRORS: !If [IsProdEnvironment, "false", "true"]
 
   NinoCheckFunctionLogGroup:
     Type: AWS::Logs::LogGroup

lambdas/abandon/src/services/abandon-dynamo-service.ts

@@ -1,29 +1,25 @@
 import { getRecordBySessionId } from "../../../common/src/database/get-record-by-session-id";
 import { SessionItem } from "../../../common/src/database/types/session-item";
 import { DynamoDBClient, UpdateItemCommand } from "@aws-sdk/client-dynamodb";
-import { RecordExpiredError, RecordNotFoundError } from "../../../common/src/database/exceptions/errors";
+import { RecordNotFoundError } from "../../../common/src/database/exceptions/errors";
 import { CriError } from "../../../common/src/errors/cri-error";
 import { logger } from "../../../common/src/util/logger";
 
 const dynamoClient = new DynamoDBClient();
 
 export async function retrieveSessionRecord(sessionTableName: string, sessionId: string) {
   try {
-    const [sessionItem] = await getRecordBySessionId<SessionItem>(
+    const sessionItem = await getRecordBySessionId<SessionItem>(
+      dynamoClient,
       sessionTableName,
       sessionId,
-      logger,
-      {},
-      dynamoClient
+      "expiryDate"
     );
     return sessionItem;
   } catch (error: unknown) {
     if (error instanceof RecordNotFoundError) {
       throw new CriError(400, "Session not found");
     }
-    if (error instanceof RecordExpiredError) {
-      throw new CriError(400, "Session expired");
-    }
     throw error;
   }
 }

lambdas/abandon/tests/abandon-handler.test.ts

@@ -59,8 +59,13 @@ describe("abandon-handler", () => {
     expect(ddbMock).toHaveReceivedCommandWith(QueryCommand, {
       ExpressionAttributeValues: {
         ":value": { S: "session-123" },
+        ":expiry": { N: expect.stringMatching(/\d+/) },
       },
       KeyConditionExpression: "sessionId = :value",
+      FilterExpression: "#expiry > :expiry",
+      ExpressionAttributeNames: {
+        "#expiry": "expiryDate",
+      },
       TableName: "session-table",
     });
     expect(ddbMock).toHaveReceivedCommandWith(UpdateItemCommand, {

lambdas/abandon/tests/services/abandon-dynamo-service.test.ts

@@ -31,9 +31,14 @@ describe("abandon-dynamo-service", () => {
       expect(sessionItem.clientSessionId).toBe("gov-123");
       expect(ddbMock).toHaveReceivedCommandWith(QueryCommand, {
         ExpressionAttributeValues: {
+          ":expiry": { N: expect.stringMatching(/\d+/) },
           ":value": { S: "session-123" },
         },
         KeyConditionExpression: "sessionId = :value",
+        FilterExpression: "#expiry > :expiry",
+        ExpressionAttributeNames: {
+          "#expiry": "expiryDate",
+        },
         TableName: "session-table",
       });
     });
@@ -55,28 +60,6 @@ describe("abandon-dynamo-service", () => {
       }
     });
 
-    it("should throw when Session expired", async () => {
-      expect.assertions(3);
-
-      ddbMock.on(QueryCommand).resolves({
-        Items: [
-          {
-            sessionId: { S: "session-123" },
-            expiryDate: { N: "1" },
-          },
-        ],
-        Count: 1,
-      });
-
-      try {
-        await retrieveSessionRecord("session-table", "session-123");
-      } catch (err) {
-        expect(err).toBeInstanceOf(CriError);
-        expect((err as CriError).message).toBe("Session expired");
-        expect((err as CriError).status).toBe(400);
-      }
-    });
-
     it("should throw an exception when it errors retrievings a record", async () => {
       ddbMock.on(QueryCommand).rejects();
 

lambdas/common/src/config/base-function-config.ts

@@ -0,0 +1,38 @@
+import { AuditConfig, TableNames } from "../../../issue-credential/src/types/input";
+
+const envVarNames = {
+  sessionTable: "SESSION_TABLE",
+  personIdentityTable: "PERSON_IDENTITY_TABLE",
+  attemptTable: "ATTEMPT_TABLE",
+  ninoUserTable: "NINO_USER_TABLE",
+  auditEventBus: "AUDIT_EVENT_BUS",
+  auditSource: "AUDIT_SOURCE",
+  auditIssuer: "AUDIT_ISSUER",
+};
+
+export class BaseFunctionConfig {
+  public readonly tableNames: TableNames;
+  public readonly audit: AuditConfig;
+
+  public static checkEnvEntry(name: string) {
+    if (!process.env[name]) {
+      throw new Error(`Missing required environment variable at init: ${name}`);
+    }
+  }
+
+  constructor() {
+    Object.values(envVarNames).forEach(BaseFunctionConfig.checkEnvEntry);
+
+    this.tableNames = {
+      sessionTable: process.env[envVarNames.sessionTable] as string,
+      personIdentityTable: process.env[envVarNames.personIdentityTable] as string,
+      attemptTable: process.env[envVarNames.attemptTable] as string,
+      ninoUserTable: process.env[envVarNames.ninoUserTable] as string,
+    };
+    this.audit = {
+      eventBus: process.env[envVarNames.auditEventBus] as string,
+      source: process.env[envVarNames.auditSource] as string,
+      issuer: process.env[envVarNames.auditIssuer] as string,
+    };
+  }
+}

lambdas/common/src/database/count-attempts.ts

@@ -0,0 +1,36 @@
+import { DynamoDBClient, QueryCommand, QueryCommandInput } from "@aws-sdk/client-dynamodb";
+
+export async function countAttempts(
+  attemptTable: string,
+  dynamoClient: DynamoDBClient,
+  sessionId: string,
+  status?: "PASS" | "FAIL"
+): Promise<number> {
+  const statusQueryString = status ? [`attempt = :status`] : "";
+  const statusAttribute: QueryCommandInput["ExpressionAttributeValues"] = status ? { ":status": { S: status } } : {};
+
+  // Need to use ExpressionAttributeNames for ttl as it is a reserved word
+  // https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/ReservedWords.html
+  // https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Expressions.ExpressionAttributeNames.html
+
+  const command = new QueryCommand({
+    TableName: attemptTable,
+    Select: "COUNT",
+    KeyConditionExpression: "sessionId = :value",
+    FilterExpression: ["#ttl > :ttl", ...statusQueryString].join(" and "),
+    ExpressionAttributeNames: { "#ttl": "ttl" },
+    ExpressionAttributeValues: {
+      ":value": {
+        S: sessionId,
+      },
+      ":ttl": {
+        N: Math.floor(Date.now() / 1000).toString(),
+      },
+      ...statusAttribute,
+    },
+  });
+
+  const result = await dynamoClient.send(command);
+
+  return result.Count ?? 0;
+}

lambdas/common/src/database/exceptions/errors.ts

@@ -1,24 +1,3 @@
-export class RecordExpiredError extends Error {
-  public readonly name = "RecordExpiredError";
-
-  constructor(
-    public readonly tableName: string,
-    public readonly sessionId: string,
-    public readonly expiryDates: number[]
-  ) {
-    super();
-  }
-
-  public get message() {
-    const expiries = this.expiryDates
-      // multiply by 1000 as expiryDate is in Unix seconds but Date expects milliseconds
-      .map((r) => new Date(r * 1000).toISOString())
-      .join(", ");
-
-    return `Found only expired records on the ${this.tableName} table: ${expiries}.`;
-  }
-}
-
 export class RecordNotFoundError extends Error {
   public readonly name = "RecordNotFoundError";
 

lambdas/common/src/database/get-record-by-session-id.ts

@@ -1,74 +1,62 @@
 import { DynamoDBClient, QueryCommand } from "@aws-sdk/client-dynamodb";
 import { unmarshall } from "@aws-sdk/util-dynamodb";
 import { withRetry } from "../util/retry";
-import { isRecordExpired, RecordWithExpiry } from "./util/is-record-expired";
-import { RecordExpiredError, RecordNotFoundError, TooManyRecordsError } from "./exceptions/errors";
-import { Logger } from "@aws-lambda-powertools/logger";
+import { RecordNotFoundError, TooManyRecordsError } from "./exceptions/errors";
+import { UnixSecondsTimestamp } from "../types/brands";
+import { logger } from "../util/logger";
 
-export type SessionIdRecord = { sessionId: string } & RecordWithExpiry;
+export type SessionIdRecord = { sessionId: string; expiryDate?: UnixSecondsTimestamp };
 
 /**
  * Retrieves a record from DynamoDB, given a table name and session ID.
- * Handles retries and expiry validation, and returns an array of valid entries.
- * The array will usually have length 1, but it's possible that multiple rows will be returned.
+ * Handles retries and expiry validation, and returns a single valid entry.
  *
  * Use a type parameter to set the type of the entity that will be returned.
  */
 export async function getRecordBySessionId<
   /** The type that will be returned by the function. Must include sessionId key. */
   ReturnType extends SessionIdRecord,
->(
-  /** The name of the table in DynamoDB. Probably looks like "some-table-some-stack". */
-  tableName: string,
-  /** The session ID to search for. */
-  sessionId: string,
-  logger: Logger,
-  opts?: { allowNoEntries?: boolean; allowMultipleEntries?: boolean },
-  dynamoClient = new DynamoDBClient()
-) {
-  const allowNoEntries = opts?.allowNoEntries ?? false;
-  const allowMultipleEntries = opts?.allowMultipleEntries ?? false;
-
+>(dynamoClient: DynamoDBClient, tableName: string, sessionId: string, expiryColumn: keyof ReturnType) {
   async function queryRecord() {
+    // Use ExpressionAttributeNames as it's possible the expiry column name is a reserved word.
+    // Notably, 'ttl' is a reserved word.
+    // https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/ReservedWords.html
+    // https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Expressions.ExpressionAttributeNames.html
+
     const command = new QueryCommand({
       TableName: tableName,
-      KeyConditionExpression: "sessionId = :value",
+      KeyConditionExpression: `sessionId = :value`,
+      FilterExpression: `#expiry > :expiry`,
+      ExpressionAttributeNames: {
+        "#expiry": String(expiryColumn),
+      },
       ExpressionAttributeValues: {
         ":value": {
           S: sessionId,
         },
+        ":expiry": {
+          N: Math.floor(Date.now() / 1000).toString(),
+        },
       },
     });
 
     const result = await dynamoClient.send(command);
 
-    if (!allowNoEntries && (result.Count === 0 || !result.Items)) {
+    if (result.Count === 0 || !result.Items) {
       throw new RecordNotFoundError(tableName, sessionId);
     }
 
-    return result.Items ?? [];
+    return result.Items;
   }
 
   const queryResult = await withRetry(queryRecord, logger, {
     maxRetries: 3,
     baseDelay: 300,
   });
 
-  const retrievedRecords = queryResult.map((v) => unmarshall(v)) as ReturnType[];
-
-  const validRecords = retrievedRecords.filter((v) => !isRecordExpired(v));
-
-  if (retrievedRecords.length > 0 && validRecords.length === 0) {
-    throw new RecordExpiredError(
-      tableName,
-      sessionId,
-      retrievedRecords.map((v) => v.expiryDate ?? v.ttl ?? -1)
-    );
-  }
-
-  if (!allowMultipleEntries && validRecords.length > 1) {
-    throw new TooManyRecordsError(tableName, sessionId, validRecords.length);
+  if (queryResult.length > 1) {
+    throw new TooManyRecordsError(tableName, sessionId, queryResult.length);
   }
 
-  return validRecords;
+  return unmarshall(queryResult[0]) as ReturnType;
 }

lambdas/common/src/database/retrieve-person-identity.ts

@@ -0,0 +1,33 @@
+import { getRecordBySessionId } from "./get-record-by-session-id";
+import { PersonIdentityItem } from "./types/person-identity";
+import { DynamoDBClient } from "@aws-sdk/client-dynamodb";
+import { logger } from "../util/logger";
+import { RecordNotFoundError } from "./exceptions/errors";
+import { CriError } from "../errors/cri-error";
+import { safeStringifyError } from "../util/stringify-error";
+
+export async function retrievePersonIdentity(
+  personIdentityTableName: string,
+  dynamoClient: DynamoDBClient,
+  sessionId: string
+): Promise<PersonIdentityItem> {
+  try {
+    const personIdentity = await getRecordBySessionId<PersonIdentityItem>(
+      dynamoClient,
+      personIdentityTableName,
+      sessionId,
+      "expiryDate"
+    );
+
+    return personIdentity;
+  } catch (error) {
+    if (error instanceof RecordNotFoundError) {
+      logger.info(`No valid person identity record found.`);
+      throw new CriError(500, `No person identity entry found for the given session ID.`);
+    }
+
+    logger.error(`Caught unexpected person identity retrieval error: ${safeStringifyError(error)}`);
+
+    throw new CriError(500, "Unexpected error getting person identity");
+  }
+}