feat: Implement Crime IDAM Integration (#357)#435
Open
alexbottenberg wants to merge 15 commits intomasterfrom
Open
feat: Implement Crime IDAM Integration (#357)#435alexbottenberg wants to merge 15 commits intomasterfrom
alexbottenberg wants to merge 15 commits intomasterfrom
Conversation
Add Crime IDAM as third OAuth2 authentication provider alongside Azure AD SSO and CFT IDAM. Features: - OAuth2 authorization code flow with Crime IDAM - New routes: /crime-login, /crime-login/return, /crime-rejected - Role-based access control with rejection page - Full Welsh language support - Comprehensive error handling for all failure scenarios - CSP configuration for Crime IDAM redirects - Environment variable configuration Implementation: - Config module for Crime IDAM settings - Token client for OAuth2 code exchange and JWT parsing - Page controllers for login, callback, and rejection flows - Updated sign-in page with Crime IDAM option - 224 unit tests with full coverage Documentation: - Updated .env.example with Crime IDAM variables - Added Crime IDAM secrets to GITHUB_SECRETS_SETUP.md - Technical plan and review in docs/tickets/357/ Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughAdds Crime IDAM as a third OAuth2 provider: new env/docs entries, config and token client, login/callback/rejected handlers with i18n and role validation, sign‑in option, helmet CSP update, auth exports and app wiring, and comprehensive tests for the new flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / Browser
participant App as Application
participant CrimeIDAM as Crime IDAM
participant DB as Database
User->>App: Click "Crime IDAM" login option
App->>App: getCrimeIdamConfig()
App-->>User: Redirect to Crime IDAM /oauth2/authorise (client_id, redirect_uri, scope, ui_locales)
User->>CrimeIDAM: Authenticate
CrimeIDAM-->>User: Redirect back with code to /crime-login/return
User->>App: GET /crime-login/return?code=...
App->>CrimeIDAM: POST /oauth2/token (code, client_id, client_secret, redirect_uri)
CrimeIDAM-->>App: Returns access_token (+ id_token)
App->>App: extractUserInfoFromToken()
App->>App: isRejectedCrimeRole(roles)?
alt rejected
App-->>User: Redirect to /crime-rejected
else accepted
App->>DB: createOrUpdateUser(provenance: CRIME_IDAM)
DB-->>App: confirm
App->>App: regenerate session, login user
App-->>User: Redirect to account-home (preserve lng)
end
🚥 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)
📝 Coding Plan
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 |
Contributor
🎭 Playwright E2E Test Results243 tests 243 ✅ 24m 37s ⏱️ Results for commit 5b22a0e. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
libs/auth/src/role-service/index.ts (1)
64-77: Keep a single rejected-role matcher.This duplicates the CFT regex and matching logic already defined above. Reusing one helper keeps Crime and CFT rejection rules from drifting apart on the next policy change.
♻️ Suggested consolidation
const REJECTED_ROLE_PATTERN = /^citizen(-.*)?$|^letter-holder$/; + +function hasRejectedRole(roles: string[]): boolean { + return roles.length > 0 && roles.some((role) => REJECTED_ROLE_PATTERN.test(role)); +} export function isRejectedCFTRole(roles: string[]): boolean { - if (!roles || roles.length === 0) { - return false; - } - - return roles.some((role) => REJECTED_ROLE_PATTERN.test(role)); + return hasRejectedRole(roles); } - -const REJECTED_CRIME_ROLE_PATTERN = /^citizen(-.*)?$|^letter-holder$/; /** * Checks if any of the provided Crime IDAM roles match the rejected role pattern * Rejected roles include: citizen, citizen-*, letter-holder * `@param` roles - Array of role strings to check * `@returns` true if any role matches the rejected pattern, false otherwise */ export function isRejectedCrimeRole(roles: string[]): boolean { - if (!roles || roles.length === 0) { - return false; - } - - return roles.some((role) => REJECTED_CRIME_ROLE_PATTERN.test(role)); + return hasRejectedRole(roles); }As per coding guidelines, "Follow YAGNI principle: Don't add speculative functionality. Take the simplest approach."
libs/auth/src/crime-idam/token-client.ts (2)
29-35: Consider adding a timeout to the fetch request.The token exchange request has no timeout, which could cause the request to hang indefinitely if Crime IDAM is unresponsive. This could degrade user experience or exhaust connection resources.
Proposed fix using AbortController
export async function exchangeCodeForToken(code: string, config: CrimeIdamConfig): Promise<TokenResponse> { const params = new URLSearchParams({ client_id: config.clientId, client_secret: config.clientSecret, grant_type: "authorization_code", redirect_uri: config.redirectUri, code }); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); + - const response = await fetch(config.tokenEndpoint, { + const response = await fetch(config.tokenEndpoint, { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, - body: params.toString() + body: params.toString(), + signal: controller.signal }); + + clearTimeout(timeoutId);
3-9: Consider exportingTokenResponseinterface.The
TokenResponseinterface is not exported butCrimeIdamUserInfois. If consumers need to type token responses (e.g., for mocking in tests), they'd benefit from having this exported.Proposed fix
-interface TokenResponse { +export interface TokenResponse { access_token: string;libs/auth/src/config/crime-idam-config.ts (1)
13-24: Consider extracting sharedgetConfigValuehelper.The
getConfigValuefunction is duplicated acrosssso-config.ts,cft-idam-config.ts, and nowcrime-idam-config.ts. Consider extracting to a shared utility module to reduce duplication.libs/auth/src/pages/crime-rejected/index.njk (1)
15-17: Sign-in link loses language selection.The link to
/sign-indoesn't preserve the user's language preference. Welsh users redirected here will lose their language selection when returning to sign in.Proposed fix
Consider passing the locale to the template and appending it:
<p class="govuk-body"> - <a href="/sign-in" class="govuk-link">{{ t.returnToSignIn }}</a> + <a href="/sign-in?lng={{ locale }}" class="govuk-link">{{ t.returnToSignIn }}</a> </p>This requires the controller to pass
localeto the template context.libs/auth/src/pages/crime-rejected/index.ts (1)
5-9: Consider passing locale to template and removing unnecessaryasync.The handler doesn't await anything, so
asyncis redundant. Additionally, the computedlocaleisn't passed to the template, which would be needed if the template wants to preserve language selection in links.Proposed changes
-export const GET = async (req: Request, res: Response) => { +export const GET = (req: Request, res: Response) => { const locale = (req.query.lng as string) || res.locals.locale || "en"; const t = locale === "cy" ? cy : en; - res.render("crime-rejected/index", { en, cy, t }); + res.render("crime-rejected/index", { en, cy, t, locale }); };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef3476df-0f75-402b-9c10-b7fb07cb1a08
📒 Files selected for processing (30)
apps/web/.env.exampleapps/web/src/app.tsdocs/GITHUB_SECRETS_SETUP.mddocs/tickets/357/plan.mddocs/tickets/357/review.mddocs/tickets/357/tasks.mddocs/tickets/357/ticket.mdlibs/auth/src/config/crime-idam-config.test.tslibs/auth/src/config/crime-idam-config.tslibs/auth/src/crime-idam/token-client.test.tslibs/auth/src/crime-idam/token-client.tslibs/auth/src/index.tslibs/auth/src/pages/crime-callback/index.test.tslibs/auth/src/pages/crime-callback/index.tslibs/auth/src/pages/crime-login/index.test.tslibs/auth/src/pages/crime-login/index.tslibs/auth/src/pages/crime-rejected/cy.tslibs/auth/src/pages/crime-rejected/en.tslibs/auth/src/pages/crime-rejected/index.njklibs/auth/src/pages/crime-rejected/index.test.tslibs/auth/src/pages/crime-rejected/index.tslibs/auth/src/role-service/index.test.tslibs/auth/src/role-service/index.tslibs/public-pages/src/pages/sign-in/cy.tslibs/public-pages/src/pages/sign-in/en.tslibs/public-pages/src/pages/sign-in/index.njklibs/public-pages/src/pages/sign-in/index.test.tslibs/public-pages/src/pages/sign-in/index.tslibs/web-core/src/middleware/helmet/helmet-middleware.test.tslibs/web-core/src/middleware/helmet/helmet-middleware.ts
- Fixed 17 failing tests in apps/web/src/app.test.ts - Added missing crimeCallbackHandler export to auth mock - All 61 web app tests now passing Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Changed email extraction to use claims.email first, then fall back to claims.sub - The sub claim in OIDC is typically a UUID/GUID per spec, not an email address - Storing sub as email would result in non-email values in the email field - Added 3 tests to verify correct email claim priority: - Prioritizes email over sub when both present - Falls back to sub when email not present - Uses empty string when neither present Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Email address was being logged as a trackException property, which would store it in Application Insights. Use only the opaque IDAM sub-claim (userId) for error correlation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Generate a random state on login, store it in session, and verify it on callback to prevent CSRF attacks where a forged callback could sign a victim into an attacker's account. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/auth/src/pages/crime-callback/index.test.ts (1)
14-14: Consider typingmockSessionmore explicitly.Using
anybypasses TypeScript's type checking. A minimal type alias orPartial<Session>intersection would catch mismatches between the mock and actual Express session shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4400384-9ae0-4fed-82de-0615073590b5
📒 Files selected for processing (2)
libs/auth/src/pages/crime-callback/index.test.tslibs/auth/src/pages/crime-callback/index.ts
…etry Strip email addresses, user IDs, display names, rejected roles, session IDs, req.user, session internals, and raw errors from console output and trackException properties. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lisions userProvenanceId has a global unique constraint and is looked up without a userProvenance filter. Prefixing with 'CRIME_IDAM:' ensures subject IDs are unique per provider, preventing a collision from silently updating the wrong user record. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/auth/src/pages/crime-callback/index.ts (1)
105-108:⚠️ Potential issue | 🟡 MinorAvoid logging raw error objects.
The caught error may contain sensitive request or token data. Log a sanitised message instead.
Proposed fix
} catch (error) { - console.error("Crime IDAM callback error:", error); + console.error("Crime IDAM callback error"); const lng = req.session.lng || "en"; return res.redirect(`/sign-in?error=auth_failed&lng=${lng}`); }libs/auth/src/pages/crime-callback/index.test.ts (1)
234-237:⚠️ Potential issue | 🟡 MinorTest asserts PII in telemetry payload.
The assertion expects
userIdintrackExceptionproperties. If the implementation is corrected to remove PII, this test will fail. Update to match the sanitised payload.Proposed fix
expect(trackException).toHaveBeenCalledWith(dbError, { - area: "Crime IDAM callback", - userId: "user-123" + area: "Crime IDAM callback" });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ee177f8-dcd1-4b71-964e-f11f38bd83e3
📒 Files selected for processing (4)
libs/auth/src/pages/crime-callback/index.test.tslibs/auth/src/pages/crime-callback/index.tslibs/auth/src/pages/crime-login/index.test.tslibs/auth/src/pages/crime-login/index.ts
Normalise roles to lowercase before regex matching so that variants like 'Citizen' or 'CITIZEN' are correctly rejected regardless of how Crime IDAM returns the role casing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix token endpoint path: Crime IDAM uses /idp/oauth2/access_token, not /oauth2/token - Fix Prisma pg.Pool being created before dotenv loads DATABASE_URL by changing createApp to a dynamic import in server.ts so env vars are set before any module initialises its database connection - Update all test fixtures to use the correct token endpoint path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers: - Valid user sign-in journey (nightly - requires CRIME_IDAM credentials) - crime-rejected page content, Welsh translation, and accessibility - Callback error handling for missing code and invalid state - Language parameter preserved through Crime IDAM redirect Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds ENABLE_CRIME_IDAM=true and required config env vars to the playwright webServer command so the /crime-login route redirects to Crime IDAM rather than returning 503, allowing the language preservation test to verify ui_locales is passed correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Common Platform radio on the sign-in page now triggers Crime IDAM authentication. The separate Crime IDAM radio option has been removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nt-home spec - Fix sign-in.spec.ts: Common Platform test was asserting redirect to "/" (home page) instead of the Crime IDAM OAuth URL — copy-paste error from the CaTH account test - Rewrite account-home.spec.ts: replace 20+ fragmented tests (many checking CSS properties) with a single @nightly journey test covering auth, content, accessibility, Welsh translation, and navigation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without scope=openid profile roles, CFT IDAM returns a JWT without the uid claim. This causes claims.uid to be undefined, which propagates as userProvenanceId: undefined to createOrUpdateUser. Prisma then attempts to INSERT NULL into the non-nullable user_provenance_id column, throwing a constraint violation and triggering the db_error redirect. Mirrors the equivalent fix already applied to Crime IDAM (b8cddba). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Crime IDAM as a third OAuth2 authentication provider alongside Azure AD SSO and CFT IDAM, enabling crime service users to authenticate using their Crime IDAM credentials.
Closes #357
Features Implemented
OAuth2 Authentication Flow
New Routes
/crime-login- Initiates Crime IDAM OAuth2 flow/crime-login/return- Handles OAuth2 callback/crime-rejected- Displays rejection page for unauthorized rolesUser Experience
Error Handling
Security
Technical Implementation
New Modules
libs/auth/src/config/crime-idam-config.ts- Configuration managementlibs/auth/src/crime-idam/token-client.ts- OAuth2 token clientlibs/auth/src/pages/crime-login/- Login initiation controllerlibs/auth/src/pages/crime-callback/- OAuth2 callback handlerlibs/auth/src/pages/crime-rejected/- Rejection page (EN + CY)Modified Components
isRejectedCrimeRole()functionTesting
Unit Tests
Test Coverage
Manual Testing
Documentation
Updated Documentation
apps/web/.env.example- Added Crime IDAM environment variablesdocs/GITHUB_SECRETS_SETUP.md- Added Crime IDAM secrets documentationdocs/tickets/357/- Technical plan, tasks, and code reviewEnvironment Variables Required
Code Quality
.jsextensionsAccessibility
Screenshots
Sign-In Page (English)
Crime IDAM option added as third authentication method
Crime Rejected Page (Welsh)
Full bilingual support for rejection flow
Deployment Notes
Prerequisites
{BASE_URL}/crime-login/returnRollout Plan
Follow-Up Tasks
Review Checklist
Review Report: Full code review available in
docs/tickets/357/review.mdTechnical Plan: Detailed implementation plan in
docs/tickets/357/plan.mdSummary by CodeRabbit
New Features
Documentation
Tests