[HOTE-1082] feat: create-session-login-lambda#415
Conversation
Co-authored-by: Copilot <copilot@github.com>
UI Coverage Report
|
Lambdas Coverage Report
|
|
There was a problem hiding this comment.
Pull request overview
Adds a new preview-only session-login-lambda that performs NHS Login callback handling, persists a session record to Aurora via SessionDbClient, and issues isolated preview cookies; also adds supporting local infra and Postman tooling.
Changes:
- Introduces
lambdas/src/session-login-lambda/*(handler, DI init, service layer, schemas, cookie helpers) with Jest unit coverage. - Extends
NhsLoginServiceto surface verified ID tokenissandaudclaims for downstream session persistence. - Adds local infra wiring (
session-preview/loginroute), plus Postman collection/environment and a rootpackage.jsonNewman script to exercise the flow.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/config/gitleaks.toml | Ignores UI .pnpm-store to reduce false-positive secret scans. |
| postman/session_login_preview_wiremock.postman_collection.json | Adds a collection to exercise the preview session login flow against LocalStack + WireMock. |
| postman/session_login_preview_wiremock.local.postman_environment.json | Provides a local Postman environment for the collection. |
| postman/README.md | Documents how to run the Postman collection manually and via CLI. |
| package.json | Adds a Newman-based script to run the Postman collection with Terraform-derived API Gateway values. |
| local-environment/infra/main.tf | Wires a new session_login_lambda module and exposes it under session-preview/login. |
| lambdas/src/session-login-lambda/session-login-service.ts | Implements session creation + persistence + token signing after successful NHS Login. |
| lambdas/src/session-login-lambda/session-login-service.test.ts | Unit tests for the session login service behavior and failure modes. |
| lambdas/src/session-login-lambda/schemas.ts | Zod schema for the callback request body (code). |
| lambdas/src/session-login-lambda/init.ts | DI wiring for HTTP/JWKS/NHS services, DB client, and session token service. |
| lambdas/src/session-login-lambda/init.test.ts | Unit tests for DI wiring, singleton protection, and rejection retry behavior. |
| lambdas/src/session-login-lambda/index.ts | Lambda handler that validates input, calls service, sets preview cookies, and echoes correlation ID. |
| lambdas/src/session-login-lambda/index.test.ts | Handler tests for validation, cookie output, and status code mapping. |
| lambdas/src/session-login-lambda/cookies.ts | Helpers to build isolated preview cookie strings and paths. |
| lambdas/src/lib/login/nhs-login-service.ts | Adds idTokenIssuer and idTokenAudience to the callback result. |
| lambdas/src/lib/login/nhs-login-service.test.ts | Updates tests to assert the newly-returned issuer/audience fields. |
| .gitignore | Ignores .pnpm-store at repo root. |
| throw new Error( | ||
| "AUTH_COOKIE_PRIVATE_KEYS_SECRET_NAME must be either a non-JSON private key string or a JSON object containing a non-empty 'key' entry.", | ||
| ); | ||
| } catch (error) { | ||
| if (!(error instanceof SyntaxError)) { | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| if (secretValue.trim().length === 0) { | ||
| throw new Error("AUTH_COOKIE_PRIVATE_KEYS_SECRET_NAME secret value must not be empty."); | ||
| } | ||
|
|
||
| return secretValue; |
There was a problem hiding this comment.
parseAuthCookiePrivateKey error messages refer to AUTH_COOKIE_PRIVATE_KEYS_SECRET_NAME as if it were the secret value, which is confusing when debugging (the env var is a secret name, not the secret contents).
Reword these errors to distinguish between the secret name env var and the retrieved secret value (e.g. "AUTH_COOKIE_PRIVATE_KEYS_SECRET_NAME secret value must...").
| "const body = pm.response.json();", | ||
| "pm.test(\"User info payload is returned\", function () {", | ||
| " pm.expect(body.sub).to.eql(\"test-sub-123\");", | ||
| " pm.expect(body.nhs_number).to.eql(\"9912003071\");", | ||
| " pm.expect(body.gp_registration_details.gp_ods_code).to.eql(\"Y12345\");", | ||
| "});", |
There was a problem hiding this comment.
This Postman collection uses a 10-digit nhs_number value in assertions; even in test tooling, we should avoid using data that could be mistaken for real patient identifiers.
Replace it with a clearly synthetic/non-production marker value and/or add an explicit note that all identifiers are fictitious test data.
| if (typeof audience !== "string") { | ||
| return undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
normalizeAudience rejects the JWT aud claim when it is an array, but per JWT/OIDC the aud claim is allowed to be string | string[], so this can cause valid NHS Login tokens to fail with ID_TOKEN_AUD_INVALID and block login.
Accept string[] audiences (e.g., if length===1 use the only entry; if multiple, select the expected clientId if present, otherwise fail with a clear error).
| if (typeof audience !== "string") { | |
| return undefined; | |
| } | |
| if (typeof audience === "string") { | |
| return this.normalizeAudienceValue(audience); | |
| } | |
| if (!Array.isArray(audience)) { | |
| return undefined; | |
| } | |
| const normalizedAudiences = audience | |
| .map((value) => this.normalizeAudienceValue(value)) | |
| .filter((value): value is string => value !== undefined); | |
| if (normalizedAudiences.length === 0) { | |
| return undefined; | |
| } | |
| const uniqueAudiences = [...new Set(normalizedAudiences)]; | |
| if (uniqueAudiences.length === 1) { | |
| return uniqueAudiences[0]; | |
| } | |
| throw new Error("ID token contains multiple audiences and cannot be normalized"); | |
| } | |
| private normalizeAudienceValue(audience: string): string | undefined { |
| export function buildPreviewAccessCookie(token: string, sameSite: string, secure: boolean): string { | ||
| return `${PREVIEW_SESSION_ACCESS_COOKIE_NAME}=${token}; HttpOnly; Path=${PREVIEW_SESSION_ACCESS_COOKIE_PATH}; SameSite=${sameSite};${secureAttribute(secure)}`; | ||
| } | ||
|
|
||
| export function buildPreviewRefreshCookie( | ||
| token: string, | ||
| sameSite: string, | ||
| secure: boolean, | ||
| ): string { | ||
| return `${PREVIEW_SESSION_REFRESH_COOKIE_NAME}=${token}; HttpOnly; Path=${PREVIEW_SESSION_REFRESH_COOKIE_PATH}; SameSite=${sameSite};${secureAttribute(secure)}`; | ||
| } |
There was a problem hiding this comment.
sameSite is interpolated directly into Set-Cookie without validation, so an unexpected env value can produce invalid cookies or (in worst cases) response-splitting if it contains control characters.
Validate AUTH_COOKIE_SAME_SITE to an allowed set (e.g. Strict/Lax/None) and enforce Secure when SameSite=None before building cookies.
| })); | ||
|
|
||
| jest.mock("../lib/login/nhs-token-verifier", () => ({ | ||
| NhsTokenVerifier: jest.fn().mockImplementation(() => ({ verifyIdToken: jest.fn() })), |
There was a problem hiding this comment.
The init unit test mocks NhsTokenVerifier with a verifyIdToken method, but the real verifier interface used by NhsLoginService is verifyToken; this mismatch can hide wiring issues and makes the mock misleading.
Update the mock to expose verifyToken (or return a shape matching INhsTokenVerifier).
| NhsTokenVerifier: jest.fn().mockImplementation(() => ({ verifyIdToken: jest.fn() })), | |
| NhsTokenVerifier: jest.fn().mockImplementation(() => ({ verifyToken: jest.fn() })), |
Description
Context
The login Lambda currently exchanges the NHS Login authorisation code for tokens and sets JWT cookies, but it does not persist a session record to Aurora. This ticket reworks the (or creates a new) login Lambda so that, at the point of successful NHS Login authentication, a new session row is written to the session table with all required fields. Downstream tickets (token refresh, reuse detection, logout) all depend on this record existing.
A new Goose migration is not required — migration 000008_create_session_table.sql already defines the schema. We should validate this schema and implement against it.
We should rewrite or create a new login lambda to handle this.
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.