OJ-3227 - Refactor database functions & querying

Author: barnabycollins

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

@@ -1,29 +1,26 @@
 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,9 @@ describe("abandon-handler", () => {
     expect(ddbMock).toHaveReceivedCommandWith(QueryCommand, {
       ExpressionAttributeValues: {
         ":value": { S: "session-123" },
+        ":expiry": { N: expect.stringMatching(/\d+/) },
       },
-      KeyConditionExpression: "sessionId = :value",
+      KeyConditionExpression: "sessionId = :value and expiryDate > :expiry",
       TableName: "session-table",
     });
     expect(ddbMock).toHaveReceivedCommandWith(UpdateItemCommand, {

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

@@ -31,9 +31,10 @@ 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",
+        KeyConditionExpression: "sessionId = :value and expiryDate > :expiry",
         TableName: "session-table",
       });
     });
@@ -55,28 +56,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();
 

…redential/src/helpers/function-config.ts …ommon/src/config/base-function-config.tslambdas/issue-credential/src/helpers/function-config.ts renamed to lambdas/common/src/config/base-function-config.ts lambdas/issue-credential/src/helpers/function-config.ts renamed to lambdas/common/src/config/base-function-config.ts

@@ -1,4 +1,4 @@
-import { AuditConfig, TableNames } from "../types/input";
+import { AuditConfig, TableNames } from "../../../issue-credential/src/types/input";
 
 const envVarNames = {
   sessionTable: "SESSION_TABLE",
@@ -10,16 +10,18 @@ const envVarNames = {
   auditIssuer: "AUDIT_ISSUER",
 };
 
-export class IssueCredentialFunctionConfig {
+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((name) => {
-      if (!process.env[name]) {
-        throw new Error(`Missing required environment variable at init: ${name}`);
-      }
-    });
+    Object.values(envVarNames).forEach(BaseFunctionConfig.checkEnvEntry);
 
     this.tableNames = {
       sessionTable: process.env[envVarNames.sessionTable] as string,

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

@@ -0,0 +1,30 @@
+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 } } : {};
+
+  const command = new QueryCommand({
+    TableName: attemptTable,
+    Select: "COUNT",
+    KeyConditionExpression: ["sessionId = :value", "ttl > :ttl", ...statusQueryString].join(" and "),
+    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,59 @@
-import { DynamoDBClient, QueryCommand } from "@aws-sdk/client-dynamodb";
+import { DynamoDBClient, QueryCommand, QueryCommandOutput } 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 { RecordNotFoundError, TooManyRecordsError } from "./exceptions/errors";
 import { Logger } from "@aws-lambda-powertools/logger";
+import { UnixSecondsTimestamp } from "../types/brands";
 
-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, logger: Logger, expiryColumn: keyof ReturnType) {
   async function queryRecord() {
     const command = new QueryCommand({
       TableName: tableName,
-      KeyConditionExpression: "sessionId = :value",
+      KeyConditionExpression: `sessionId = :value and ${String(expiryColumn)} > :expiry`,
       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)) {
-      throw new RecordNotFoundError(tableName, sessionId);
+    if (result.Count === 0 || !result.Items) {
+      throw new Error();
     }
 
-    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[];
+  let queryResult: QueryCommandOutput["Items"];
 
-  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)
-    );
+  try {
+    queryResult = await withRetry(queryRecord, logger, {
+      maxRetries: 3,
+      baseDelay: 300,
+    });
+  } catch {
+    throw new RecordNotFoundError(tableName, sessionId);
   }
 
-  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,34 @@
+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,
+      logger,
+      "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");
+  }
+}

lambdas/common/src/database/util/is-record-expired.ts

@@ -1,4 +1,4 @@
-import { IssueCredentialFunctionConfig } from "../../src/helpers/function-config";
+import { BaseFunctionConfig } from "../../src/config/base-function-config";
 
 const originalProcessEnv = JSON.parse(JSON.stringify(process.env));
 
@@ -25,7 +25,7 @@ describe("NINo Check function getConfig()", () => {
   });
 
   it("returns the right data for a certain input", () => {
-    const config = new IssueCredentialFunctionConfig();
+    const config = new BaseFunctionConfig();
 
     expect(config).toEqual({
       tableNames: {
@@ -44,6 +44,6 @@ describe("NINo Check function getConfig()", () => {
 
   it("throws an error for a missing config value", () => {
     process.env.NINO_USER_TABLE = "";
-    expect(() => new IssueCredentialFunctionConfig()).toThrow("NINO_USER_TABLE");
+    expect(() => new BaseFunctionConfig()).toThrow("NINO_USER_TABLE");
   });
 });