Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ jobs:
# SSO Configuration (using existing GitHub Secrets)
SSO_CLIENT_ID: ${{ secrets.SSO_CLIENT_ID }}
SSO_CLIENT_SECRET: ${{ secrets.SSO_CLIENT_SECRET }}
SSO_IDENTITY_METADATA: ${{ secrets.SSO_ISSUER_URL }}
SSO_ISSUER_URL: ${{ secrets.SSO_ISSUER_URL }}
SSO_SYSTEM_ADMIN_GROUP_ID: ${{ secrets.SSO_SG_SYSTEM_ADMIN }}
SSO_INTERNAL_ADMIN_CTSC_GROUP_ID: ${{ secrets.SSO_SG_ADMIN_CTSC }}
SSO_INTERNAL_ADMIN_LOCAL_GROUP_ID: ${{ secrets.SSO_SG_ADMIN_LOCAL }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/nightly.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ jobs:
# SSO Configuration (using existing GitHub Secrets)
SSO_CLIENT_ID: ${{ secrets.SSO_CLIENT_ID }}
SSO_CLIENT_SECRET: ${{ secrets.SSO_CLIENT_SECRET }}
SSO_IDENTITY_METADATA: ${{ secrets.SSO_ISSUER_URL }}
SSO_ISSUER_URL: ${{ secrets.SSO_ISSUER_URL }}
SSO_SYSTEM_ADMIN_GROUP_ID: ${{ secrets.SSO_SG_SYSTEM_ADMIN }}
SSO_INTERNAL_ADMIN_CTSC_GROUP_ID: ${{ secrets.SSO_SG_ADMIN_CTSC }}
SSO_INTERNAL_ADMIN_LOCAL_GROUP_ID: ${{ secrets.SSO_SG_ADMIN_LOCAL }}
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ For local development, configuration is split between Azure Key Vault (for sensi
**Secrets loaded from Azure Key Vault** (requires `az login`):
- `SSO_CLIENT_ID` - Azure AD application client ID
- `SSO_CLIENT_SECRET` - Azure AD application client secret
- `SSO_IDENTITY_METADATA` - OpenID Connect metadata endpoint
- `SSO_ISSUER_URL` - Azure AD issuer URL
- `SSO_SYSTEM_ADMIN_GROUP_ID` - System admin Azure AD group ID
- `SSO_INTERNAL_ADMIN_CTSC_GROUP_ID` - CTSC admin Azure AD group ID
- `SSO_INTERNAL_ADMIN_LOCAL_GROUP_ID` - Local admin Azure AD group ID
Expand Down Expand Up @@ -332,7 +332,7 @@ SSO_ALLOW_HTTP_REDIRECT=true
|------------------------|---------------------|-------------|
| `sso-client-id` | `SSO_CLIENT_ID` | Azure AD application client ID |
| `sso-client-secret` | `SSO_CLIENT_SECRET` | Azure AD application client secret |
| `sso-config-endpoint` | `SSO_IDENTITY_METADATA` | OpenID Connect metadata endpoint |
| `sso-issuer-url` | `SSO_ISSUER_URL` | Azure AD issuer URL |
| `sso-sg-system-admin` | `SSO_SYSTEM_ADMIN_GROUP_ID` | System admin Azure AD group ID |
| `sso-sg-admin-ctsc` | `SSO_INTERNAL_ADMIN_CTSC_GROUP_ID` | CTSC admin Azure AD group ID |
| `sso-sg-admin-local` | `SSO_INTERNAL_ADMIN_LOCAL_GROUP_ID` | Local admin Azure AD group ID |
Expand Down
2 changes: 1 addition & 1 deletion apps/web/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ CFT_IDAM_URL=https://idam-web-public.aat.platform.hmcts.net
# Secrets loaded from Key Vault (pip-ss-kv):
# - SSO_CLIENT_ID
# - SSO_CLIENT_SECRET
# - SSO_IDENTITY_METADATA
# - SSO_ISSUER_URL
# - SSO_SYSTEM_ADMIN_GROUP_ID
# - SSO_INTERNAL_ADMIN_CTSC_GROUP_ID
# - SSO_INTERNAL_ADMIN_LOCAL_GROUP_ID
Expand Down
2 changes: 1 addition & 1 deletion apps/web/config/custom-environment-variables.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
},
"SSO_CLIENT_ID": "SSO_CLIENT_ID",
"SSO_CLIENT_SECRET": "SSO_CLIENT_SECRET",
"SSO_IDENTITY_METADATA": "SSO_IDENTITY_METADATA",
"SSO_ISSUER_URL": "SSO_ISSUER_URL",
"SSO_ALLOW_HTTP_REDIRECT": "SSO_ALLOW_HTTP_REDIRECT",
"SSO_SYSTEM_ADMIN_GROUP_ID": "SSO_SYSTEM_ADMIN_GROUP_ID",
"SSO_INTERNAL_ADMIN_CTSC_GROUP_ID": "SSO_INTERNAL_ADMIN_CTSC_GROUP_ID",
Expand Down
2 changes: 1 addition & 1 deletion apps/web/helm/values.dev.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ nodejs:
- name: sso-client-secret-dev
alias: SSO_CLIENT_SECRET
- name: sso-issuer-url
alias: SSO_IDENTITY_METADATA
alias: SSO_ISSUER_URL
- name: sso-sg-system-admin
alias: SSO_SYSTEM_ADMIN_GROUP_ID
- name: sso-sg-admin-ctsc
Expand Down
2 changes: 1 addition & 1 deletion apps/web/helm/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ nodejs:
- name: sso-client-secret
alias: SSO_CLIENT_SECRET
- name: sso-issuer-url
alias: SSO_IDENTITY_METADATA
alias: SSO_ISSUER_URL
- name: sso-sg-system-admin
alias: SSO_SYSTEM_ADMIN_GROUP_ID
- name: sso-sg-admin-ctsc
Expand Down
2 changes: 1 addition & 1 deletion apps/web/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export async function createApp(): Promise<Express> {
app.locals.redisClient = redisClient;

// Initialize Passport for Azure AD authentication
configurePassport(app);
await configurePassport(app);

const modulePaths = [
__dirname,
Expand Down
208 changes: 208 additions & 0 deletions docs/tickets/347/plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
# Plan: Replace passport-azure-ad with openid-client (#347)

## Overview

Replace the deprecated `passport-azure-ad` library with `openid-client` for the SSO (Azure AD) authentication flow. The B2C and CFT IDAM flows are unaffected: B2C already uses manual OAuth (added in #229), and CFT IDAM has its own native token exchange. Only the internal staff SSO flow (using `OIDCStrategy` from `passport-azure-ad`) needs to change.

The approach is to write a custom Passport strategy that wraps `openid-client`. This is a drop-in replacement: the rest of the codebase continues to call `passport.authenticate("sso-oidc", ...)`, no other files need to change.

---

## Technical Approach

### Custom Passport Strategy wrapping openid-client

The `passport-azure-ad` `OIDCStrategy` currently handles:
1. Building the authorization redirect URL and redirecting the browser
2. On callback, exchanging the authorization code for tokens
3. Calling the verify callback with the access token and profile claims

The replacement custom strategy does the same three things using `openid-client` primitives, registered under the strategy name `"sso-oidc"`. References to `"azuread-openidconnect"` in `login/index.ts` and `sso-callback/index.ts` are updated to `"sso-oidc"`.

`openid-client` is already a transitive dependency via other HMCTS packages. It will be added as a direct dependency of `@hmcts/auth`.

### openid-client API used

- `Issuer.discover(issuerUrl)` — fetches the OIDC provider metadata (replaces `identityMetadata` passed to `OIDCStrategy`)
- `new issuer.BaseClient({ client_id, client_secret, redirect_uris, response_types })` — creates the client
- `client.authorizationUrl({ scope, state, nonce, response_type })` — builds the authorization redirect
- `client.callbackParams(req)` — extracts code/state/error params from the callback request
- `client.callback(redirectUri, params, { state, nonce })` — exchanges the code for a `TokenSet`
- `tokenSet.claims()` — returns the ID token claims (oid, upn, email, name, etc.)
- `tokenSet.access_token` — the access token passed to the Graph API

### State and nonce handling

`passport-azure-ad` managed state and nonce internally. The custom strategy must manage them explicitly:

- On the authorization redirect, generate a cryptographically secure `state` and `nonce` using `crypto.randomBytes`
- Store both in the session under a namespaced key (e.g., `req.session.ssoOidc = { state, nonce }`)
- On callback, read `state` and `nonce` from session, pass them to `client.callback()` for validation
- Clear the session values after use

This pattern mirrors what `b2c-login/index.ts` already does for the B2C flow.

### Session serialization

No change required. The verify callback still calls `done(null, user)` with a `UserProfile` object. `passport.serializeUser` / `passport.deserializeUser` store the whole object in the session, as they do today. Token refresh is out of scope (session-only approach confirmed).

---

## Implementation Details

### 1. `libs/auth/package.json`

- Remove `"passport-azure-ad": "4.3.5"` from `dependencies`
- Add `"openid-client": "<pinned version>"` to `dependencies`

The version to pin should be the latest stable `openid-client` v5 (e.g. `5.7.0`). v6+ is ESM-only and requires different import syntax — verify what the broader monorepo uses before pinning.

### 2. `libs/auth/src/config/sso-config.ts`

The `appendOpenIdConfigPath` function and the `identityMetadata` field exist solely because `passport-azure-ad` requires the full `/.well-known/openid-configuration` URL. `openid-client`'s `Issuer.discover()` accepts the bare issuer URL and fetches the discovery document itself.

Changes:
- Rename the `SSO_IDENTITY_METADATA` env var read to `SSO_ISSUER_URL` (matches the env var name from Key Vault / GitHub secrets as stated in the ticket)
- Remove `appendOpenIdConfigPath` — it is no longer needed
- Rename the field in the `SsoConfig` interface from `identityMetadata` to `issuerUrl`
- Update `isSsoConfigured()` to check `issuerUrl` instead of `identityMetadata`

The `responseType` and `responseMode` fields on `SsoConfig` are `passport-azure-ad`-specific options that will not be passed to `openid-client` directly. They can be removed from the interface, or retained as documentation; removing them is cleaner.

### 3. `libs/auth/src/config/passport-config.ts`

This file is replaced almost entirely. It keeps the same public API: `configurePassport(app: Express): void`.

The new implementation:

```
import { Issuer } from "openid-client";
import passport from "passport";
import { Strategy as PassportStrategy } from "passport";
```

`configurePassport` becomes `async` (it needs to call `Issuer.discover()`). The call site in `apps/web/src/app.ts` must be updated to `await configurePassport(app)`.

Structure of the new strategy (inner class or inline, not exported):

```
class SsoOidcStrategy extends PassportStrategy {
name = "sso-oidc";

authenticate(req: Request): void {
if (req.path matches the callback path) {
// callback branch: exchange code, call verify callback
} else {
// redirect branch: generate state+nonce, store in session, redirect
}
}
}
```

In practice, `passport-azure-ad`'s `OIDCStrategy` uses a single strategy instance for both the initiation redirect and the callback by inspecting whether query/body parameters from the IdP are present. The replacement strategy does the same: check for `req.query.code` or `req.query.error` to distinguish callback from initiation.

The verify callback signature simplifies from the `passport-azure-ad` 6-argument form `(_iss, _sub, profile, accessToken, _refreshToken, done)` to a cleaner function:

```typescript
async function verifyOidcCallback(claims: Record<string, unknown>, accessToken: string, done: VerifyCallback): Promise<void>
```

`claims` come from `tokenSet.claims()`. The fields used (`oid`, `upn`, `email`, `name`) are standard OIDC claims present in Azure AD ID tokens. The Graph API call (`fetchUserProfile`) still receives the `access_token` for group membership lookup, so that path is unchanged.

Because `Issuer.discover()` is async and must happen before the strategy is usable, the OIDC client is created once during `configurePassport` and captured in the strategy's closure.

### 4. `libs/auth/src/pages/login/index.ts`

One-line change:

```typescript
// Before
passport.authenticate("azuread-openidconnect", { ... })

// After
passport.authenticate("sso-oidc", { ... })
```

### 5. `libs/auth/src/pages/sso-callback/index.ts`

One-line change (same as login):

```typescript
// Before
passport.authenticate("azuread-openidconnect", { ... })

// After
passport.authenticate("sso-oidc", { ... })
```

The rest of `sso-callback/index.ts` — session regeneration, role check, `createOrUpdateUser`, redirect logic — is unchanged.

### 6. `apps/web/src/app.ts`

`configurePassport` becomes async, so the call must be awaited. Check the call site and add `await`.

---

## Error Handling and Edge Cases

### Missing SSO configuration

Existing guard in `configurePassport`: if `issuerUrl`, `clientId`, or `clientSecret` are missing, passport is initialized without a strategy (same behaviour as today). The `isSsoConfigured()` function gates the login and callback handlers before they attempt `passport.authenticate`, so unauthenticated environments degrade gracefully.

### `Issuer.discover()` failure

If the issuer URL is wrong or the IdP is unreachable at startup, `Issuer.discover()` will throw. This should propagate and crash the process at startup rather than silently swallowing the error, since a service that cannot reach its IdP is not usable. Wrap the call in a try/catch that logs the error clearly and re-throws.

### Token exchange failure

`client.callback()` throws an `OPError` or `RPError` for invalid codes, expired codes, state mismatches, or nonce mismatches. The strategy should call `this.fail(error)` for expected OIDC errors, and `this.error(error)` for unexpected errors. This causes passport to follow `failureRedirect` (set to `/login` in `sso-callback/index.ts`).

### Graph API failure

The `fetchUserProfile` call in the verify callback can fail if the access token lacks the required Graph API scopes or the Graph API is unavailable. The existing error path `done(error, false)` is retained — passport will redirect to `failureRedirect`.

### Role determination returning `undefined`

`determineSsoUserRole` already returns `undefined` when no matching group is found. The `sso-callback/index.ts` handler already redirects to `/sso-rejected` when `req.user.role` is falsy. No change needed.

### State/nonce mismatch

`openid-client` validates state and nonce internally when they are passed to `client.callback()`. A mismatch throws `RPError: state mismatch` which the strategy handles via `this.fail(error)`.

---

## Acceptance Criteria Mapping

| Criterion | How satisfied |
|---|---|
| SSO logins work end-to-end | Custom `sso-oidc` strategy uses `openid-client` to perform the full OIDC code flow with Azure AD |
| CFT IDAM login continues to work | CFT IDAM uses its own token exchange (`cft-idam/token-client.ts`) and is completely unaffected |
| Azure B2C login works | B2C uses its own manual OAuth flow (`b2c-callback/index.ts`) and is completely unaffected |
| Crime IDAM login works | Crime IDAM follows the same manual token exchange pattern as CFT IDAM and is unaffected |
| `passport-azure-ad` removed | Removed from `package.json` dependencies |
| `SSO_ISSUER_URL` env var used | `sso-config.ts` reads `SSO_ISSUER_URL`; the old `SSO_IDENTITY_METADATA` is no longer read |

---

## Files Changed Summary

| File | Nature of change |
|---|---|
| `libs/auth/package.json` | Remove `passport-azure-ad`, add `openid-client` |
| `libs/auth/src/config/sso-config.ts` | Replace `identityMetadata`/`SSO_IDENTITY_METADATA` with `issuerUrl`/`SSO_ISSUER_URL`; remove `appendOpenIdConfigPath` |
| `libs/auth/src/config/passport-config.ts` | Rewrite: replace `OIDCStrategy` with custom strategy using `openid-client` |
| `libs/auth/src/pages/login/index.ts` | Change strategy name string from `"azuread-openidconnect"` to `"sso-oidc"` |
| `libs/auth/src/pages/sso-callback/index.ts` | Change strategy name string from `"azuread-openidconnect"` to `"sso-oidc"` |
| `libs/auth/src/config/passport-config.test.ts` | Rewrite: mock `openid-client` instead of `passport-azure-ad` |
| `libs/auth/src/config/sso-config.test.ts` | Update env var name from `SSO_IDENTITY_METADATA` to `SSO_ISSUER_URL`; remove `appendOpenIdConfigPath` assertion |
| `libs/auth/src/pages/login/index.test.ts` | Update strategy name assertion from `"azuread-openidconnect"` to `"sso-oidc"` |
| `apps/web/src/app.ts` | Add `await` to `configurePassport(app)` call |

---

## Assumptions

- `openid-client` v5 is used (CommonJS-compatible, available as `import { Issuer } from "openid-client"`). If the monorepo has already moved to v6+ ESM-only, import syntax needs adjustment.
- `SSO_ISSUER_URL` contains a bare issuer URL such as `https://login.microsoftonline.com/<tenant-id>/v2.0` — no trailing `/` or `.well-known` suffix. `Issuer.discover()` appends the discovery path itself.
- Existing `@types/passport` dev dependency covers the base `Strategy` class without additional type packages.
- The `configurePassport` call in `apps/web/src/app.ts` is in an async context where `await` can be added without structural changes.
22 changes: 22 additions & 0 deletions docs/tickets/347/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Implementation Tasks: Replace passport-azure-ad with openid-client (#347)

## Implementation Tasks

- [ ] Confirm `openid-client` version in use across the monorepo (v5 vs v6+) and identify the exact version to pin
- [ ] In `libs/auth/package.json`: remove `passport-azure-ad` from `dependencies` and add `openid-client` at the pinned version
- [ ] In `libs/auth/src/config/sso-config.ts`: rename env var read from `SSO_IDENTITY_METADATA` to `SSO_ISSUER_URL`, rename the interface field from `identityMetadata` to `issuerUrl`, and remove the `appendOpenIdConfigPath` function
- [ ] In `libs/auth/src/config/sso-config.ts`: update `isSsoConfigured()` to check `issuerUrl` instead of `identityMetadata`
- [ ] In `libs/auth/src/config/passport-config.ts`: make `configurePassport` async and replace the `OIDCStrategy` instantiation with a custom Passport strategy that uses `Issuer.discover()` and `openid-client`'s `client.authorizationUrl()` / `client.callback()` for the OIDC code flow
- [ ] In the custom strategy: generate `state` and `nonce` on the redirect leg, store them in `req.session.ssoOidc`, and read them back on the callback leg for validation
- [ ] In the custom strategy: simplify the verify callback to receive `(claims, accessToken, done)` and keep the existing `fetchUserProfile` and `determineSsoUserRole` calls unchanged
- [ ] In `libs/auth/src/pages/login/index.ts`: change strategy name from `"azuread-openidconnect"` to `"sso-oidc"`
- [ ] In `libs/auth/src/pages/sso-callback/index.ts`: change strategy name from `"azuread-openidconnect"` to `"sso-oidc"`
- [ ] In `apps/web/src/app.ts`: add `await` to the `configurePassport(app)` call
- [ ] In `libs/auth/src/config/sso-config.test.ts`: replace `SSO_IDENTITY_METADATA` with `SSO_ISSUER_URL` throughout, update the expected `issuerUrl` assertion to remove the `/.well-known/openid-configuration` suffix, and remove the `appendOpenIdConfigPath` test case
- [ ] In `libs/auth/src/config/passport-config.test.ts`: replace the `passport-azure-ad` / `OIDCStrategy` mock with an `openid-client` mock (`Issuer.discover`, `BaseClient`), update assertions to use the `"sso-oidc"` strategy name, and update the verify callback extraction to match the new signature
- [ ] In `libs/auth/src/pages/login/index.test.ts`: update the `passport.authenticate` call assertion from `"azuread-openidconnect"` to `"sso-oidc"`
- [ ] Run `yarn test` in `libs/auth` and fix any remaining test failures
- [ ] Run `yarn lint:fix` in `libs/auth` and resolve any linting issues
- [ ] Verify the full build compiles without TypeScript errors: `yarn build` in `libs/auth`
- [ ] Smoke test locally: confirm SSO login redirects to the Azure AD login page, completes the callback, and lands on the correct dashboard for each role (SYSTEM_ADMIN, INTERNAL_ADMIN_CTSC, INTERNAL_ADMIN_LOCAL)
- [ ] Confirm CFT IDAM and B2C login flows are unaffected by running their respective journeys
Loading
Loading