Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the NHS Login flow by moving previously hardcoded login configuration into environment variables/Terraform, improving the login lambda’s dependency wiring around Secrets Manager, and adding UI test coverage for the OAuth callback handling.
Changes:
- Parameterised NHS Login client configuration (client ID/scope) and propagated it into UI + local Terraform + env generation script.
- Updated login lambda init to fetch a dedicated auth-cookie signing secret (with JSON support for multi-key secrets).
- Added a new UI unit test for
CallbackPageand tightened some login-related typing/logging.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/settings.ts | Exposes new UI-facing env vars for NHS Login client ID and scope. |
| ui/src/routes/LoginPage.tsx | Uses env-driven client_id and scope when redirecting to NHS Login authorize URL. |
| ui/src/tests/routes/CallbackPage.test.tsx | Adds unit tests for callback handling, navigation, and error paths. |
| scripts/terraform/post-apply-env-update.sh | Extends UI env generation with NHS Login client config (but changes output filename). |
| local-environment/infra/variables.tf | Adds local Terraform variables for login/session lambda auth settings and secret names. |
| local-environment/infra/main.tf | Wires new Terraform variables into login/session lambda environment variables. |
| lambdas/src/login-lambda/login-service.ts | Minor refactor/cleanup (imports/comments). |
| lambdas/src/login-lambda/init.ts | Fetches auth-cookie private key secret from Secrets Manager and parses JSON key sets. |
| lambdas/src/login-lambda/init.test.ts | Updates init tests for the new secret and JSON secret parsing behavior. |
| lambdas/src/login-lambda/index.ts | Refactors request body parsing and switches 400 responses to createJsonResponse. |
| lambdas/src/login-lambda/index.test.ts | Updates tests to match the new 400 response body shape. |
| lambdas/src/lib/login/test-user-mapping.ts | Formatting/comment updates around test-user enrichment behavior. |
| lambdas/src/lib/login/nhs-login-client.ts | Tightens token response typing and updates enrichment comments. |
| lambdas/src/lib/login/nhs-login-client.test.ts | Adjusts test base URI expectations for sandpit hostname. |
| lambdas/src/lib/http/login-http-client.ts | Improves typing of error handling/logging for axios-backed HTTP client. |
5501cc5 to
f1b626e
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the NHS Login flow across the UI, local Terraform, and the login-lambda to remove hardcoded values, improve configuration via env/terraform, and tighten request/secret handling as the service matures towards Beta.
Changes:
- UI: moves NHS Login
client_idandscopeintoNEXT_PUBLIC_*settings and updates the login redirect builder (plus adds CallbackPage tests). - Local infra/scripts: adds Terraform variables/outputs and post-apply env generation for the new NHS Login settings and cookie key secret names.
- Lambdas: adds Zod-based request validation in
login-lambda, retrieves auth-cookie signing keys from Secrets Manager (including JSON/key-rotation parsing), and improves typing/error handling in login HTTP client code.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/settings.ts | Exposes NEXT_PUBLIC_NHS_LOGIN_CLIENT_ID and NEXT_PUBLIC_NHS_LOGIN_SCOPE to the SPA. |
| ui/src/routes/LoginPage.tsx | Builds authorize redirect URL from settings; adds missing-config handling. |
| ui/src/tests/routes/LoginPage.test.tsx | Updates mocks to support new settings and React Router navigation. |
| ui/src/tests/routes/CallbackPage.test.tsx | Adds unit tests for CallbackPage auth flow and error paths. |
| scripts/terraform/post-apply-env-update.sh | Writes new NEXT_PUBLIC_NHS_LOGIN_CLIENT_ID/SCOPE into ui/.env.local. |
| local-environment/infra/variables.tf | Introduces local variables for login client/scope/redirect and auth cookie key material config. |
| local-environment/infra/outputs.tf | Exposes local NHS Login client/scope for scripts to consume. |
| local-environment/infra/main.tf | Wires new env vars into login/session lambda modules. |
| lambdas/src/login-lambda/login-service.ts | Removes/reshuffles legacy comments/imports (no behaviour change). |
| lambdas/src/login-lambda/init.ts | Reads AUTH_COOKIE_PRIVATE_KEYS_SECRET_NAME and parses signing keys from Secrets Manager. |
| lambdas/src/login-lambda/init.test.ts | Updates init wiring tests; adds coverage for JSON-formatted key secrets. |
| lambdas/src/login-lambda/index.ts | Adds Zod validation + JSON parse guard; standardises JSON responses via createJsonResponse. |
| lambdas/src/login-lambda/index.test.ts | Adds invalid-shape request test and updates error-body assertions. |
| lambdas/src/lib/login/test-user-mapping.ts | Formatting/comment updates and minor signature formatting. |
| lambdas/src/lib/login/nhs-login-client.ts | Tightens token response typing and updates sandpit enrichment comments. |
| lambdas/src/lib/login/nhs-login-client.test.ts | Updates expected base URI for sandpit domain. |
| lambdas/src/lib/http/login-http-client.ts | Improves typing and error-shape handling; migrates console.log → console.error. |
92327ba to
7e32a18
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the NHS Login flow across UI, Lambda, and local Terraform so the login/session lambdas use the shared fetch-based HTTP client, with configuration moved out of hardcoded values and into environment/Terraform outputs.
Changes:
- UI: parameterises NHS Login authorize redirect using
NEXT_PUBLIC_NHS_LOGIN_CLIENT_IDandNEXT_PUBLIC_NHS_LOGIN_SCOPE, adds a Service Error fallback when config is missing, and adds correlation IDs to the/logincall. - Lambdas: replaces the legacy axios-based login HTTP client with
FetchHttpClient, adds request-body validation + correlation ID echoing inlogin-lambda, and updates wiring/tests accordingly. - Local env: adds Terraform variables/outputs and post-apply scripting to populate the new UI env vars and provide separated cookie-signing secret names.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/settings.ts | Exposes new public UI settings for NHS Login client ID and scope. |
| ui/src/routes/LoginPage.tsx | Builds NHS Login authorize URL from settings; redirects to a service error route when misconfigured. |
| ui/src/routes/CallbackPage.tsx | Adds X-Correlation-ID header to the /login POST request. |
| ui/src/tests/routes/LoginPage.test.tsx | Updates mocks and adds coverage for missing-config navigation behaviour. |
| ui/src/tests/routes/CallbackPage.test.tsx | Adds unit coverage for callback behaviours (missing code/CSRF/state, success, API failure) including correlation header. |
| scripts/terraform/post-apply-env-update.sh | Writes the new NEXT_PUBLIC_NHS_LOGIN_CLIENT_ID and NEXT_PUBLIC_NHS_LOGIN_SCOPE entries into ui/.env.local. |
| local-environment/infra/variables.tf | Adds local Terraform variables for NHS Login and auth-cookie configuration. |
| local-environment/infra/outputs.tf | Exposes Terraform outputs for client ID and scope consumed by post-apply scripting. |
| local-environment/infra/main.tf | Wires new local variables into login/session lambda environment variables. |
| lambdas/src/session-lambda/init.ts | Switches session lambda wiring to use FetchHttpClient. |
| lambdas/src/session-lambda/init.test.ts | Updates mocks to match the new shared HTTP client import. |
| lambdas/src/login-lambda/login-service.ts | Minor cleanup/re-ordering while keeping the service interface stable. |
| lambdas/src/login-lambda/login-lambda.test.ts | Removes the old handler test in favour of the new index.test.ts. |
| lambdas/src/login-lambda/init.ts | Adds cookie private-key secret retrieval/parsing and swaps to FetchHttpClient. |
| lambdas/src/login-lambda/init.test.ts | Updates dependency wiring expectations + adds coverage for JSON secret parsing. |
| lambdas/src/login-lambda/index.ts | Adds Zod validation, correlation ID resolution/echoing, and safer error responses; switches to createJsonResponse. |
| lambdas/src/login-lambda/index.test.ts | New unit tests for validation, correlation header echoing, success cookies, and failure responses. |
| lambdas/src/lib/login/test-user-mapping.ts | Formatting/comment updates for the sandpit workaround mapping. |
| lambdas/src/lib/login/nhs-login-client.ts | Migrates token/userinfo calls onto the shared HttpClient interface (get/post). |
| lambdas/src/lib/login/nhs-login-client.test.ts | Updates mocks and expectations for the shared HTTP client methods. |
| lambdas/src/lib/http/login-http-client.ts | Removes the legacy axios-based login HTTP client implementation. |
| lambdas/src/lib/http/http-client.ts | Improves FetchHttpClient body handling (preserves URLSearchParams). |
| lambdas/src/lib/http/http-client.test.ts | Adds test coverage ensuring URLSearchParams bodies are sent correctly. |
| lambdas/src/get-results-lambda/init.test.ts | Adjusts typing around the FetchHttpClient mock. |
7e32a18 to
929b403
Compare
929b403 to
1a4a595
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the NHS Login flow across the UI and backend by moving hardcoded NHS Login configuration to environment/Terraform, updating the UI redirect/callback handling, and migrating lambdas from the bespoke axios-based client to the shared FetchHttpClient.
Changes:
- UI: parameterise NHS Login authorize URL construction using
NEXT_PUBLIC_*config and add missing-config handling; add correlation IDs to/loginrequests. - Backend: refactor login/session lambdas to use the shared
FetchHttpClient, add stronger request validation inlogin-lambda, and update tests accordingly. - Local env: add Terraform variables/outputs and post-apply script updates to populate new NHS Login env vars.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/settings.ts | Exposes new NEXT_PUBLIC_NHS_LOGIN_CLIENT_ID and NEXT_PUBLIC_NHS_LOGIN_SCOPE settings. |
| ui/src/routes/LoginPage.tsx | Builds authorize redirect URL from settings; handles missing config by navigating to service error. |
| ui/src/routes/CallbackPage.tsx | Adds X-Correlation-ID header to the login POST request. |
| ui/src/tests/routes/LoginPage.test.tsx | Updates tests for settings + navigation behaviour when config is missing. |
| ui/src/tests/routes/CallbackPage.test.tsx | Adds unit coverage for callback flow behaviour and correlation ID header on login call. |
| scripts/terraform/post-apply-env-update.sh | Writes the new NEXT_PUBLIC_NHS_LOGIN_* values into ui/.env.local. |
| local-environment/infra/variables.tf | Adds local Terraform variables for NHS Login + auth-cookie configuration. |
| local-environment/infra/outputs.tf | Exposes client ID and scope outputs for local env wiring. |
| local-environment/infra/main.tf | Wires new env vars into login_lambda/session_lambda modules. |
| lambdas/src/session-lambda/init.ts | Switches session lambda wiring to FetchHttpClient. |
| lambdas/src/session-lambda/init.test.ts | Updates mocks for the new HTTP client import. |
| lambdas/src/login-lambda/login-service.ts | Minor refactor/cleanup and import ordering. |
| lambdas/src/login-lambda/login-lambda.test.ts | Removes old handler test (superseded by new index.test.ts). |
| lambdas/src/login-lambda/init.ts | Moves auth-cookie signing keys to dedicated secret env var and parses secret values. |
| lambdas/src/login-lambda/init.test.ts | Updates init wiring tests, including JSON secret parsing. |
| lambdas/src/login-lambda/index.ts | Adds Zod validation, correlation ID response headers, and safer error handling. |
| lambdas/src/login-lambda/index.test.ts | New handler test suite for validation, correlation ID propagation, and error responses. |
| lambdas/src/lib/login/test-user-mapping.ts | Formatting/comment updates and minor signature formatting. |
| lambdas/src/lib/login/nhs-login-client.ts | Migrates to shared HttpClient interface (get/post). |
| lambdas/src/lib/login/nhs-login-client.test.ts | Updates mocks and expectations for the new HTTP client interface. |
| lambdas/src/lib/http/login-http-client.ts | Deletes legacy axios-based login HTTP client. |
| lambdas/src/lib/http/http-client.ts | Enhances shared fetch client (URLSearchParams handling + optional undici dispatcher). |
| lambdas/src/lib/http/http-client.test.ts | Adds test coverage for URLSearchParams body serialisation. |
| lambdas/src/get-results-lambda/init.test.ts | Adjusts typing/casting for FetchHttpClient mock. |
| lambdas/package.json | Adds undici dependency (for Agent dispatcher support). |
| lambdas/package-lock.json | Locks undici dependency resolution. |
Files not reviewed (1)
- lambdas/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
lambdas/src/lib/http/http-client.ts:58
FetchHttpClient’srejectUnauthorized: falseoption is only applied to POST/POSTRAW requests; GET requests ignore the dispatcher, so the option won’t work consistently. Pass the same dispatcher option intofetch()insideget()as well.
constructor(options?: { rejectUnauthorized?: boolean }) {
if (options?.rejectUnauthorized === false) {
this.dispatcher = new Agent({ connect: { rejectUnauthorized: false } });
}
}
async get<T>(url: string, headers?: Record<string, string>): Promise<T> {
const response = await fetch(url, {
method: "GET",
headers: {
Accept: "application/json",
...headers,
},
});
1a4a595 to
73abaa3
Compare
+ updated or removed comments to reflect changes + moved hardcoded values to environment variables and terraform + created seperate env variable for auth cookie secret + removed unused code and variables
73abaa3 to
ecb1078
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the NHS Login flow across the UI and backend by moving previously hardcoded login parameters into environment/Terraform configuration, updating the login/session lambdas to use the shared FetchHttpClient, and tightening request validation/error handling.
Changes:
- UI: read NHS Login
client_idandscopefromNEXT_PUBLIC_*env vars; add correlation IDs to/logincalls. - Backend: replace the legacy axios-based login HTTP client with the shared
FetchHttpClient; add Zod validation + consistent JSON responses in the login lambda. - Local infra/scripts: plumb new Terraform variables/outputs into
.env.localgeneration and lambda env configuration.
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/settings.ts | Exposes new public settings for NHS Login client_id + scope. |
| ui/src/routes/LoginPage.tsx | Builds authorize URL using env-configured values; adds missing-config navigation. |
| ui/src/routes/CallbackPage.tsx | Adds X-Correlation-ID header to /login request. |
| ui/src/tests/routes/LoginPage.test.tsx | Updates mocks to support new settings + missing-config behavior. |
| ui/src/tests/routes/CallbackPage.test.tsx | Adds unit coverage for callback flow including correlation header. |
| ui/package-lock.json | Lockfile updates from dependency resolution. |
| scripts/terraform/post-apply-env-update.sh | Writes new NEXT_PUBLIC_NHS_LOGIN_CLIENT_ID/SCOPE into .env.local. |
| local-environment/infra/variables.tf | Introduces local Terraform variables for NHS Login + cookie/auth settings. |
| local-environment/infra/outputs.tf | Adds Terraform outputs for client id + scope. |
| local-environment/infra/main.tf | Uses Terraform vars for login/session lambda env vars (removes some hardcoding). |
| lambdas/src/session-lambda/init.ts | Switches to shared FetchHttpClient. |
| lambdas/src/session-lambda/init.test.ts | Updates mocks for new HTTP client import. |
| lambdas/src/login-lambda/login-service.ts | Minor cleanup/reordering of imports/comments. |
| lambdas/src/login-lambda/login-lambda.test.ts | Removes old test file (replaced by new index.test.ts). |
| lambdas/src/login-lambda/init.ts | Reads new env var for auth-cookie signing keys secret; parses key material; uses shared HTTP client. |
| lambdas/src/login-lambda/init.test.ts | Extends init wiring tests to cover new secret + JSON key parsing. |
| lambdas/src/login-lambda/index.ts | Adds request validation, correlation ID handling, consistent JSON responses, safer error handling/logging. |
| lambdas/src/login-lambda/index.test.ts | Adds handler unit tests for 400/200/500 paths and correlation header behavior. |
| lambdas/src/lib/login/test-user-mapping.ts | Formatting + comment tweaks. |
| lambdas/src/lib/login/nhs-login-client.ts | Migrates to shared HttpClient interface (get/post) and keeps test-user enrichment. |
| lambdas/src/lib/login/nhs-login-client.test.ts | Updates mocks to match new HttpClient shape. |
| lambdas/src/lib/http/login-http-client.ts | Removes legacy axios-based client. |
| lambdas/src/lib/http/http-client.ts | Enhances body serialization (URLSearchParams support) and optional TLS behavior via undici Agent. |
| lambdas/src/lib/http/http-client.test.ts | Adds test coverage for URLSearchParams POST bodies. |
| lambdas/src/get-results-lambda/init.test.ts | Adjusts typing cast for mocked FetchHttpClient. |
| lambdas/package.json | Removes axios; adds undici. |
| lambdas/package-lock.json | Lockfile updates reflecting axios removal + undici addition. |
Files not reviewed (2)
- lambdas/package-lock.json: Language not supported
- ui/package-lock.json: Language not supported
Comments suppressed due to low confidence (2)
lambdas/src/lib/http/http-client.ts:58
FetchHttpClientsupportsrejectUnauthorized: falsevia anundicidispatcher, but the dispatcher is only applied to POST requests; apply the same dispatcher option inget()(and any other methods) so GET requests behave consistently when TLS verification is intentionally disabled (e.g. local/dev endpoints).
constructor(options?: { rejectUnauthorized?: boolean }) {
if (options?.rejectUnauthorized === false) {
this.dispatcher = new Agent({ connect: { rejectUnauthorized: false } });
}
}
async get<T>(url: string, headers?: Record<string, string>): Promise<T> {
const response = await fetch(url, {
method: "GET",
headers: {
Accept: "application/json",
...headers,
},
});
ui/src/tests/routes/LoginPage.test.tsx:75
- The test still stubs
Math.random()to stabilise a numeric nonce, but the implementation now usescrypto.randomUUID(); remove the unused spy (or switch to mockingcrypto.randomUUIDif the nonce value needs to be asserted) to keep the test intent aligned with the code.
let randomSpy: jest.SpyInstance<number, []>;
beforeEach(() => {
// Stabilise the nonce so the useEffect is deterministic under JSDOM.
randomSpy = jest.spyOn(Math, "random").mockReturnValue(0.1234);
mockedGetAuthorizeLoginHintFragment.mockReturnValue("&login_hint=stored-hint");
| const response = await fetch(`${backendUrl}/login`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "X-Correlation-ID": globalThis.crypto.randomUUID(), | ||
| }, | ||
| body: JSON.stringify({ code }), | ||
| credentials: "include", | ||
| }); |
| import { RoutePath } from "@/lib/models/route-paths"; | ||
| import * as settings from "@/settings"; | ||
|
|
||
| export default function RedirectPage() { |
| mockInit.mockImplementation(async () => ({ | ||
| authTokenService: { | ||
| generateAuthAccessToken: jest.fn().mockReturnValue("signed-access"), | ||
| generateAuthRefreshToken: jest.fn().mockReturnValue("signed-refresh"), | ||
| }, | ||
| loginService: { | ||
| performLogin: jest.fn().mockImplementation(async () => loginOutput), | ||
| }, | ||
| })); |
| @@ -41,48 +39,70 @@ const envVars: LoginEnvVariables = { | |||
| }; | |||
| enable_cors = true | ||
| cors_allow_origin = "http://localhost:3000" | ||
| cors_allow_methods = ["POST", "OPTIONS"] | ||
| cors_allow_headers = ["Content-Type", "Authorization"] | ||
| cors_allow_credentials = true | ||
|
|
||
| environment_variables = { | ||
| NODE_OPTIONS = "--enable-source-maps", | ||
| ALLOW_ORIGIN = "http://localhost:3000", | ||
| NHS_LOGIN_BASE_ENDPOINT_URL = local.resolved_nhs_login_base_url, | ||
| NHS_LOGIN_CLIENT_ID = "hometest", | ||
| NHS_LOGIN_REDIRECT_URL = "http://localhost:3000/callback", | ||
| NHS_LOGIN_PRIVATE_KEY_SECRET_NAME = "nhs-login-private-key", | ||
| AUTH_SESSION_MAX_DURATION_MINUTES = "60", | ||
| AUTH_ACCESS_TOKEN_EXPIRY_DURATION_MINUTES = "60", | ||
| AUTH_REFRESH_TOKEN_EXPIRY_DURATION_MINUTES = "60", | ||
| AUTH_COOKIE_SAME_SITE = "Lax" | ||
| AUTH_COOKIE_SECURE = "false" | ||
| NHS_LOGIN_CLIENT_ID = var.local_nhs_login_client_id, | ||
| NHS_LOGIN_REDIRECT_URL = var.local_nhs_login_redirect_url, | ||
| NHS_LOGIN_PRIVATE_KEY_SECRET_NAME = var.local_nhs_login_private_key_secret_name, | ||
| AUTH_COOKIE_PRIVATE_KEYS_SECRET_NAME = var.local_auth_cookie_private_keys_secret_name |
| function parseAuthCookiePrivateKeys(secretValue: string): Record<string, string> { | ||
| try { | ||
| const parsedSecret = JSON.parse(secretValue) as Record<string, unknown>; | ||
| const privateKeys = Object.entries(parsedSecret).reduce<Record<string, string>>( | ||
| (accumulator, [key, value]) => { | ||
| if (typeof value === "string" && value.length > 0) { | ||
| accumulator[key] = value; | ||
| } | ||
|
|
||
| return accumulator; | ||
| }, | ||
| {}, | ||
| ); | ||
|
|
||
| if (typeof privateKeys.key === "string" && privateKeys.key.length > 0) { | ||
| return privateKeys; | ||
| } | ||
| 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 { | ||
| // Non-JSON values are treated as a single key. | ||
| } | ||
|
|
||
| return { | ||
| key: secretValue, | ||
| }; |
| @@ -28,8 +83,6 @@ export const lambdaHandler = async ( | |||
| }; | |||
| } | |||
|



Description
Refactor Login Lambda
Context
Progress the maturation of the Login Lambda and ready move to Beta
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.