Feature/347 Replace passport-azure-ad with openid-client - #662
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace passport-azure-ad with openid-client, make configurePassport async to run OIDC discovery, rename SSO config to issuerUrl/SSO_ISSUER_URL, register "sso-oidc" strategy, update routes/tests/docs/CI/helm, and adjust a few ancillary configs and logs. ChangesSSO Library Migration & OIDC Strategy Implementation
Sequence DiagramsequenceDiagram
participant App as apps/web
participant ConfigurePassport as configurePassport
participant Issuer as Issuer.discover
participant Passport as passport
App->>ConfigurePassport: await configurePassport(app)
ConfigurePassport->>Issuer: Issuer.discover(issuerUrl)
Issuer-->>ConfigurePassport: OIDC metadata
ConfigurePassport->>Passport: passport.use("sso-oidc", new Strategy(...))
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
docs/tickets/347/plan.md (2)
27-27: ⚡ Quick winVerify the client class name.
The openid-client v5 API uses
new issuer.Client({...})rather thannew issuer.BaseClient({...}).BaseClientis an internal base class not typically instantiated directly.📝 Suggested correction
-- `new issuer.BaseClient({ client_id, client_secret, redirect_uris, response_types })` — creates the client +- `new issuer.Client({ client_id, client_secret, redirect_uris, response_types })` — creates the client
78-100: 💤 Low valueAdd language specifiers to fenced code blocks.
The code blocks at lines 78 and 88 are missing language identifiers, which would improve syntax highlighting and markdown quality.
📝 Suggested fix
-``` +```typescript import { Issuer } from "openid-client";libs/auth/src/config/passport-config.ts (3)
10-34: 💤 Low valueModule ordering: exported function should precede helpers.
The coding guidelines require exported functions to appear before other functions in usage order.
verifyOidcCallback(a non-exported helper) is currently declared above the exportedconfigurePassport. Consider movingverifyOidcCallbackbelowconfigurePassport.As per coding guidelines: "Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom."
37-67: ⚡ Quick winDuplicated passport bootstrap in two early-return branches.
The
disableSsobranch and the missing-config branch register the sameinitialize/session/serializeUser/deserializeUsersetup verbatim. Extracting a small helper (e.g.registerNoOpPassport(app)) would remove the duplication and keep both fallback paths in lockstep if future tweaks are needed.♻️ Suggested extraction
+function registerNoOpPassport(app: Express): void { + app.use(passport.initialize()); + app.use(passport.session()); + passport.serializeUser((user, done) => done(null, user)); + passport.deserializeUser((user: Express.User, done) => done(null, user)); +} + export async function configurePassport(app: Express): Promise<void> { const disableSso = process.env.NODE_ENV === "development" && !process.env.ENABLE_SSO; - - if (disableSso) { - app.use(passport.initialize()); - app.use(passport.session()); - passport.serializeUser((user, done) => { done(null, user); }); - passport.deserializeUser((user: Express.User, done) => { done(null, user); }); - return; - } + if (disableSso) { + registerNoOpPassport(app); + return; + } const ssoConfig = getSsoConfig(); if (!ssoConfig.issuerUrl || !ssoConfig.clientId || !ssoConfig.clientSecret) { - app.use(passport.initialize()); - app.use(passport.session()); - passport.serializeUser((user, done) => { done(null, user); }); - passport.deserializeUser((user: Express.User, done) => { done(null, user); }); + registerNoOpPassport(app); return; }
69-84: ⚡ Quick winConfirm
openid-client/passportv6.8.4Strategy+discoveryusage (and improve malformed issuer URL error clarity)
Strategyoptions:callbackURLandscopeare supportedopenid-client/passportStrategyOptionsfields, and the repo’s unit test already asserts those exact option names/values.discovery(server, clientId, clientSecret): the 3rd positional argument as a string is supported as a shorthand forclient_secret(so your positional form is fine).- Consider wrapping
new URL(ssoConfig.issuerUrl)in a try/catch (libs/auth/src/config/passport-config.ts:69) to surface a clear “invalid SSO_ISSUER_URL” configuration error instead of a rawTypeError.libs/auth/src/pages/logout/index.ts (1)
5-8: 💤 Low valueTenant regex is restricted to lowercase hex GUIDs.
/\/([a-f0-9-]+)\/v2\.0(?:\/|$)/will fail to extract the tenant when the issuer URL uses uppercase hex (Azure occasionally emits these) or a domain-style tenant likecontoso.onmicrosoft.com. The v2.0 issuer from discovery is typically the lowercase GUID, so this is unlikely to bite in practice, but adding theiflag and broadening the character class would make this resilient to either form.♻️ Suggested regex
- const match = issuerUrl.match(/\/([a-f0-9-]+)\/v2\.0(?:\/|$)/); + const match = issuerUrl.match(/\/([^/]+)\/v2\.0(?:\/|$)/i);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13877257-5159-4315-9b39-ac1617d83257
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
.github/workflows/e2e.yml.github/workflows/nightly.ymlREADME.mdapps/web/.env.exampleapps/web/config/custom-environment-variables.jsonapps/web/helm/values.dev.yamlapps/web/helm/values.yamlapps/web/src/app.tsdocs/tickets/347/plan.mddocs/tickets/347/tasks.mddocs/tickets/347/ticket.mdlibs/auth/package.jsonlibs/auth/src/config/passport-config.test.tslibs/auth/src/config/passport-config.tslibs/auth/src/config/sso-config.test.tslibs/auth/src/config/sso-config.tslibs/auth/src/pages/login/index.test.tslibs/auth/src/pages/login/index.tslibs/auth/src/pages/logout/index.test.tslibs/auth/src/pages/logout/index.tslibs/auth/src/pages/sso-callback/index.tspackage.json
🎭 Playwright E2E Test Results84 tests 51 ✅ 6m 8s ⏱️ Results for commit 1445e93. ♻️ This comment has been updated with latest results. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81a45d9d-eea9-4468-9225-93034b51ea3a
📒 Files selected for processing (5)
libs/auth/src/config/crime-idam-config.test.tslibs/auth/src/config/crime-idam-config.tslibs/auth/src/config/sso-config.test.tslibs/auth/src/pages/cft-callback/index.tslibs/auth/src/pages/sso-callback/index.test.ts
💤 Files with no reviewable changes (1)
- libs/auth/src/pages/cft-callback/index.ts
| vi.mocked(isSsoConfigured).mockReturnValue(true); | ||
|
|
||
| const mockMiddleware = vi.fn(); | ||
| vi.mocked(passport.authenticate).mockReturnValue(mockMiddleware as any); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining `as any` usages in this test file.
rg -nP '\bas any\b' libs/auth/src/pages/sso-callback/index.test.tsRepository: hmcts/cath-service
Length of output: 581
🏁 Script executed:
#!/bin/bash
# Inspect the relevant parts around the flagged `as any` usages.
sed -n '35,90p' libs/auth/src/pages/sso-callback/index.test.ts
echo "----"
sed -n '120,260p' libs/auth/src/pages/sso-callback/index.test.ts
echo "----"
sed -n '330,390p' libs/auth/src/pages/sso-callback/index.test.tsRepository: hmcts/cath-service
Length of output: 7060
Remove the as any cast in the passport.authenticate middleware mock
Line 55 weakens type-safety; type mockMiddleware directly so the as any cast is unnecessary.
Suggested change
- const mockMiddleware = vi.fn();
- vi.mocked(passport.authenticate).mockReturnValue(mockMiddleware as any);
+ const mockMiddleware: (req: Request, res: Response, next: () => void) => void = vi.fn();
+ vi.mocked(passport.authenticate).mockReturnValue(mockMiddleware);Also, the file contains several other as any / cb: any usages in session-save and user update mocks; they should be cleaned up using typed mocks to match strict-mode expectations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.mocked(passport.authenticate).mockReturnValue(mockMiddleware as any); | |
| const mockMiddleware: (req: Request, res: Response, next: () => void) => void = vi.fn(); | |
| vi.mocked(passport.authenticate).mockReturnValue(mockMiddleware); |
…lace-passport-azure-ad # Conflicts: # package.json # yarn.lock
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…re-ad' into feature/347-replace-passport-azure-ad
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
3 participants
Jira link
#347
Change description
Replace passport-azure-ad with openid-client
Checklist
Summary by CodeRabbit
Chores
Documentation
Bug Fixes