Feature/347 replace passport azure ad with openid client#385
Feature/347 replace passport azure ad with openid client#385ChrisS1512 wants to merge 6 commits intomasterfrom
Conversation
…ith openid-client Created comprehensive technical plan for migrating from deprecated passport-azure-ad library to openid-client for SSO authentication. Plan includes: - Custom Passport strategy wrapper around openid-client (minimal risk approach) - Complete implementation details and file structure - Comprehensive testing strategy (unit, integration, E2E) - Zero-downtime deployment strategy - 55-task implementation checklist organized in 10 phases Key clarifications needed: - Azure B2C implementation status - SSO_ISSUER_URL format preference - Token refresh strategy - Deployment window availability - Feature flag requirements for rollback Files created: - docs/tickets/347/ticket.md - Issue details and requirements - docs/tickets/347/plan.md - Technical implementation plan - docs/tickets/347/tasks.md - Implementation task checklist Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR migrates SSO authentication from the deprecated Changes
Sequence DiagramsequenceDiagram
actor User
participant App as Express App
participant Passport as Passport
participant OIDC as openid-client
participant Provider as OIDC Provider
participant Graph as Graph API
User->>App: 1. Initiate login
App->>OIDC: 2. Discover OIDC metadata<br/>(issuerUrl)
OIDC->>Provider: 3. Fetch .well-known config
Provider-->>OIDC: 4. Return endpoints & keys
OIDC-->>App: 5. Return Client instance
App->>Passport: 6. Generate auth URL<br/>(with PKCE challenge)
Passport-->>User: 7. Redirect to Provider login
User->>Provider: 8. Authenticate & consent
Provider->>User: 9. Redirect with auth code
User->>App: 10. Return with code & state
App->>OIDC: 11. Exchange code for tokens<br/>(PKCE verifier)
OIDC->>Provider: 12. Token endpoint request
Provider-->>OIDC: 13. Return access & ID tokens
OIDC-->>App: 14. Validate ID token signature
App->>Graph: 15. Fetch user profile<br/>(if needed)
Graph-->>App: 16. Return user data
App->>Passport: 17. Verify callback with claims
Passport-->>App: 18. Session established
App-->>User: 19. Authenticated & redirected
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 9
🧹 Nitpick comments (10)
libs/auth/src/pages/logout/index.ts (1)
4-7: Minor: regex only matches lowercase hex in tenant ID.The regex
[a-f0-9-]will not match uppercase hex characters. Azure AD typically returns lowercase GUIDs in issuer URLs, so this is fine in practice, but if a configuration ever uses uppercase (e.g.,ABCDEF12-...), extraction will silently fail and the user will be redirected to/session-logged-outinstead of the Azure AD logout endpoint.Consider using a case-insensitive flag or
[a-fA-F0-9-]for robustness:Suggested fix
function extractTenantId(issuerUrl: string): string | null { - const match = issuerUrl.match(/\/([a-f0-9-]+)\/v2\.0/); + const match = issuerUrl.match(/\/([a-f0-9-]+)\/v2\.0/i); return match ? match[1] : null; }libs/auth/src/config/passport-config.ts (1)
12-23: Non-exported helper precedes exported function.Per coding guidelines, exported functions should come before non-exported helpers.
initializePassport(non-exported) is defined beforeconfigurePassport(exported).Suggested reorder
Move
initializePassportbelowconfigurePassport, or alternatively inline the calls since it's only used twice.As per coding guidelines: "constants outside function scope at top, exported functions next, other functions in usage order."
docs/tickets/347/plan.md (2)
130-141: Plan'sSsoConfiginterface is stale — includesresponseTypeandresponseModethat were removed in implementation.The actual
SsoConfiginlibs/auth/src/config/sso-config.tsno longer includes these fields. Consider updating the plan to reflect the final implementation, or adding a note that the plan predates implementation.
412-428: Code example uses openid-client v5 API (Issuer.discover()).The implementation uses the v6 API (
client.discovery()). This sample would mislead anyone referencing the plan for future work. The actual version in use is 6.3.3, not 6.1.3 (line 183).libs/auth/src/config/passport-config.test.ts (4)
94-114: Incomplete config validation tests only check empty-string values.The production code guards with
!ssoConfig.issuerUrl || !ssoConfig.clientId || !ssoConfig.clientSecret, which treats both""andundefinedas falsy. The tests only exercise the empty-string path. Consider adding a case withundefinedto confirm the guard covers both, especially since environment variables can genuinely be absent (not just blank).
139-145: Discovery failure test validates current (throwing) behaviour — be aware this will need updating.The review document (
review.md) flags unhandled discovery failure as a critical issue and proposes a graceful fallback. Once that fix lands, this test's assertion (rejects.toThrow) will need to change to expectconfigurePassportto succeed without registering a strategy. Worth noting with a// TODOor addressing in the same PR.
295-309: Missing access token test — verify the assertion matches the error constructor.Line 307 uses
expect.objectContaining({ message: "No access token received from identity provider" }). This works becausenew Error(msg)sets.message, but it's slightly fragile — if the production code ever changes to a custom error class, this assertion could silently pass or fail in unexpected ways. The current form is acceptable, though matching withexpect(new Error("No access token received from identity provider"))would be more explicit.
128-136: ImportSTRATEGY_NAMEconstant in the test file and use it instead of the hardcoded string.The test hardcodes
"azuread-openidconnect"while production uses theSTRATEGY_NAMEconstant. This creates a maintenance risk—if the constant is ever changed, the test will not catch the discrepancy. Using the constant in the test ensures a single source of truth.docs/tickets/347/review.md (1)
1-7: Useful review document overall, but needs a factual accuracy pass.The positive feedback and architectural observations are sound. However, several sections reference code states that don't match what's actually in the PR (see other comments). A quick reconciliation pass against the final test file and production code would make this a reliable reference for the team.
docs/tickets/347/tasks.md (1)
51-103: Phases 5–10 remain largely unchecked — ensure these are tracked before merge.Integration tests, E2E tests, environment configuration, manual testing, and deployment preparation are all incomplete. This is expected for an initial review, but merging to
masterwith these outstanding (particularly Phase 7: environment variable configuration and Phase 10: deployment runbook) carries risk.
| 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 }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the e2e.yml workflow file to understand the context
cat -n .github/workflows/e2e.yml | head -100Repository: hmcts/cath-service
Length of output: 4404
🏁 Script executed:
# Check if there are any other references to SSO secrets in the codebase
rg "SSO_ISSUER_URL|SSO_IDENTITY_METADATA" -A 2 -B 2Repository: hmcts/cath-service
Length of output: 13279
🏁 Script executed:
# Look for documentation about required secrets or environment setup
fd -type f \( -name "README*" -o -name "*.md" \) | xargs grep -l "secret\|SSO" 2>/dev/null | head -10Repository: hmcts/cath-service
Length of output: 232
Confirm SSO_ISSUER_URL GitHub Actions secret is provisioned.
The workflow references ${{ secrets.SSO_ISSUER_URL }} on line 64. The authentication configuration requires this secret and will fail during initialisation if it is missing or empty. Verify the secret exists in the repository settings and contains the correct Azure AD v2.0 issuer URL (format: https://login.microsoftonline.com/{tenant}/v2.0).
| // Initialize Passport for Azure AD authentication | ||
| configurePassport(app); | ||
| await configurePassport(app); |
There was a problem hiding this comment.
Stale comment still references "Azure AD".
The inline comment no longer reflects the implementation.
✏️ Proposed fix
- // Initialize Passport for Azure AD authentication
+ // Initialize Passport with OpenID Connect strategy
await configurePassport(app);📝 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.
| // Initialize Passport for Azure AD authentication | |
| configurePassport(app); | |
| await configurePassport(app); | |
| // Initialize Passport with OpenID Connect strategy | |
| await configurePassport(app); |
|
|
||
| // Initialize Passport for Azure AD authentication | ||
| configurePassport(app); | ||
| await configurePassport(app); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts -g '!**/node_modules/**' 'createApp' -A5 -B2Repository: hmcts/cath-service
Length of output: 12979
🏁 Script executed:
# Find the main entrypoint file in apps/web
fd -t f -e ts -e js 'index|main|server|start' apps/web/src/ | head -20Repository: hmcts/cath-service
Length of output: 279
🏁 Script executed:
# Check the structure of apps/web/src
ls -la apps/web/src/Repository: hmcts/cath-service
Length of output: 581
🏁 Script executed:
cat -n apps/web/src/server.tsRepository: hmcts/cath-service
Length of output: 2551
🏁 Script executed:
cat -n apps/web/src/app.ts | head -80Repository: hmcts/cath-service
Length of output: 4260
🏁 Script executed:
cat -n apps/api/src/server.ts | head -30Repository: hmcts/cath-service
Length of output: 1244
Wrap startServer() with error handling to gracefully handle OIDC discovery failures.
configurePassport performs async OIDC discovery at startup. If the network call fails, createApp() will reject and cause an unhandled promise rejection, crashing the process without logs. The web server's top-level await has no error handling (unlike the API server which uses try-catch). Add structured error handling around the startup sequence to catch and log failures, then exit cleanly.
| - [x] Create `libs/auth/src/openid-connect-strategy/` directory | ||
| - [x] Create `libs/auth/src/openid-connect-strategy/openid-connect-strategy.ts` with base strategy class |
There was a problem hiding this comment.
Phase 2 tasks 1–2 are misleadingly marked as complete.
These items say a custom openid-connect-strategy/ directory and base strategy class were created, but the actual implementation uses the built-in openid-client/passport Strategy — no custom strategy exists. The remaining Phase 2 items (lines 18–22) correctly note "(delegated to openid-client/passport built-in Strategy)"; these two should be updated similarly or struck through to avoid confusion.
| - [x] Create `libs/auth/src/openid-connect-strategy/openid-connect-strategy.test.ts` | ||
| - [x] Write tests for OIDC discovery success and failure |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for openid-connect-strategy files
fd "openid-connect-strategy" --type fRepository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check the content of the tasks.md file to understand the context
cat -n docs/tickets/347/tasks.md | head -50Repository: hmcts/cath-service
Length of output: 3500
Phase 4 tasks reference files that do not exist in the codebase.
Lines 36–37 reference openid-connect-strategy.test.ts and related OIDC tests, yet these files are not present in the repository. Although Phase 2 tasks also claim to have created the base strategy files, none of these files exist. These task items should not be marked complete ([x]) until the actual files are created and tests are written.
|
|
||
| CaTH AI is currently using library passport-azure-ad which will be deprecated soon. Instead of using passport-azure-ad, we need to use openid-client in our application. | ||
|
|
||
| SSO_ISSUER_URL will be used from keyvault and Github secrets which has been added already. |
There was a problem hiding this comment.
Minor text corrections.
"keyvault" → "Key Vault" (Azure product name), "Github" → "GitHub" (proper noun).
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: The official name of this software platform is spelled with a capital “H”.
Context: ...SUER_URL will be used from keyvault and Github secrets which has been added already. ...
(GITHUB)
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...L will be used from keyvault and Github secrets which has been added already. **Accept...
(AI_HYDRA_LEO_MISSING_COMMA)
| export async function configurePassport(app: Express): Promise<void> { | ||
| const disableSso = process.env.NODE_ENV === "development" && !process.env.ENABLE_SSO; | ||
|
|
||
| if (disableSso) { | ||
| initializePassport(app); | ||
| return; | ||
| } | ||
|
|
||
| const ssoConfig = getSsoConfig(); | ||
|
|
||
| // Check if SSO configuration is complete (e.g., for E2E tests or environments without SSO setup) | ||
| if (!ssoConfig.identityMetadata || !ssoConfig.clientId || !ssoConfig.clientSecret) { | ||
| // Initialize passport with minimal configuration (no OIDC strategy) | ||
| app.use(passport.initialize()); | ||
| app.use(passport.session()); | ||
|
|
||
| // Simple serialization | ||
| passport.serializeUser((user, done) => { | ||
| done(null, user); | ||
| }); | ||
|
|
||
| passport.deserializeUser((user: Express.User, done) => { | ||
| done(null, user); | ||
| }); | ||
|
|
||
| return; | ||
| if (!ssoConfig.issuerUrl || !ssoConfig.clientId || !ssoConfig.clientSecret) { | ||
| throw new Error("SSO configuration is incomplete. Required: SSO_ISSUER_URL, SSO_CLIENT_ID, SSO_CLIENT_SECRET"); | ||
| } | ||
|
|
||
| // Initialize passport | ||
| app.use(passport.initialize()); | ||
| app.use(passport.session()); | ||
| passport.use( | ||
| new OIDCStrategy( | ||
| { | ||
| identityMetadata: ssoConfig.identityMetadata, | ||
| clientID: ssoConfig.clientId, | ||
| clientSecret: ssoConfig.clientSecret, | ||
| redirectUrl: ssoConfig.redirectUri, | ||
| responseType: ssoConfig.responseType, | ||
| responseMode: ssoConfig.responseMode, | ||
| scope: ssoConfig.scope, | ||
| passReqToCallback: false, | ||
| validateIssuer: true, | ||
| clockSkew: 300 | ||
| }, | ||
| verifyOidcCallback | ||
| ) | ||
| const oidcConfig = await client.discovery(new URL(ssoConfig.issuerUrl), ssoConfig.clientId, ssoConfig.clientSecret); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "app.ts" --type f -exec grep -l "configurePassport" {} \;Repository: hmcts/cath-service
Length of output: 314
🏁 Script executed:
fd "plan.md" --type fRepository: hmcts/cath-service
Length of output: 566
🏁 Script executed:
git ls-files | grep -E "(app\.ts|plan\.md)" | head -20Repository: hmcts/cath-service
Length of output: 634
🏁 Script executed:
grep -n "configurePassport" apps/api/src/app.ts apps/web/src/app.ts 2>/dev/nullRepository: hmcts/cath-service
Length of output: 253
🏁 Script executed:
rg "OIDC discovery|discovery fails" docs/tickets --type mdRepository: hmcts/cath-service
Length of output: 1865
🏁 Script executed:
cat -n libs/auth/src/config/passport-config.ts | head -50Repository: hmcts/cath-service
Length of output: 1976
Add error handling for OIDC discovery failure on line 39.
Line 39 client.discovery() lacks a try/catch block. According to plan.md, when discovery fails, the expected behaviour is to "Log error, return 503 on /login attempts," not fail app startup. Wrap discovery in try/catch, log the error, and initialize passport without the OIDC strategy to allow graceful degradation.
| describe("isSsoConfigured", () => { | ||
| let originalEnv: NodeJS.ProcessEnv; | ||
|
|
||
| beforeEach(() => { | ||
| originalEnv = { ...process.env }; | ||
| vi.resetModules(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| process.env = originalEnv; | ||
| }); |
There was a problem hiding this comment.
isSsoConfigured beforeEach doesn't scrub SSO_ISSUER_URL.
Unlike the getSsoConfig suite, the beforeEach here omits deleting SSO_ISSUER_URL, SSO_CLIENT_ID, and SSO_CLIENT_SECRET. The test on line 146 that asserts isSsoConfigured() returns false when issuerUrl is missing is therefore fragile in any environment where SSO_ISSUER_URL is already set (e.g., a developer's local shell).
🛡️ Proposed fix
beforeEach(() => {
originalEnv = { ...process.env };
+ delete process.env.SSO_ISSUER_URL;
+ delete process.env.SSO_CLIENT_ID;
+ delete process.env.SSO_CLIENT_SECRET;
+ delete process.env.NODE_ENV;
+ delete process.env.ENABLE_SSO;
vi.resetModules();
});
🎭 Playwright E2E Test Results235 tests 235 ✅ 21m 51s ⏱️ Results for commit 19b9279. ♻️ This comment has been updated with latest results. |
|
2 participants
Jira link
#347
Change description
Replaced passport-azure-ad with openid client
Summary by CodeRabbit
Configuration Updates
SSO_IDENTITY_METADATAtoSSO_ISSUER_URL.SSO_ALLOW_HTTP_REDIRECTsetting, simplifying SSO setup requirements.Documentation
Tests