Skip to content

Conversation

@junaidiqbalmoj
Copy link
Contributor

@junaidiqbalmoj junaidiqbalmoj commented Nov 20, 2025

Jira link

https://tools.hmcts.net/jira/browse/VIBE-143

Change description

Add user table in database and populate it when user is logged in.

Summary by CodeRabbit

  • New Features

    • User accounts now persist to the database with email, names, provenance and role.
    • Last sign-in dates are automatically tracked; users are created or updated on SSO and CFT sign-in.
  • Improvements

    • Enhanced error tracking and logging for database operations during authentication flows; dedicated exception reporting added.
    • Improved token parsing to extract first and last names and more reliable id/email mapping.
  • Tests

    • Added comprehensive unit tests covering repository, callbacks, and monitoring.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 20, 2025

Walkthrough

This PR adds a Postgres-backed Prisma User model and migration, implements a Prisma-backed user repository with create/read/update/upsert flows, integrates repository calls into SSO and CFT authentication callbacks with error tracking, adds an Application Insights trackException helper, updates token claim mapping, and includes tests and documentation.

Changes

Cohort / File(s) Summary
Database schema & migration
apps/postgres/prisma/schema.prisma, apps/postgres/prisma/migrations/20251121153203_create_user/migration.sql
New Prisma User model (UUID PK, email, optional names, provenance, provenance id unique, role, created/last signed-in timestamps) and SQL migration creating the user table with PK and unique index.
Account repository (implementation & types)
libs/account/src/repository/model.ts, libs/account/src/repository/query.ts, libs/account/src/repository/query.test.ts, libs/account/package.json
New User and UpdateUserInput interfaces; repository functions createUser, findUserByProvenanceId, findUserByEmail, updateUser, createOrUpdateUser implemented with Prisma; comprehensive unit tests; package export subpath added for ./repository/query.
Authentication callbacks (SSO & CFT)
libs/auth/src/pages/sso-callback/index.ts, libs/auth/src/pages/sso-callback/index.test.ts, libs/auth/src/pages/cft-callback/index.ts, libs/auth/src/pages/cft-callback/index.test.ts
Call createOrUpdateUser on successful sign-in (SSO provenance "SSO", CFT provenance "CFT_IDAM" with role VERIFIED); wrap DB call in try/catch, call trackException on failure and redirect with db_error; tests updated/mocked accordingly.
CFT IDAM token parsing
libs/auth/src/cft-idam/token-client.ts, libs/auth/src/cft-idam/token-client.test.ts
CftIdamUserInfo extended with firstName and surname; extractUserInfoFromToken now prefers uid for id, maps given_name/family_name to name fields, adjusts email/displayName source logic; tests updated for claims.
Monitoring utility
libs/cloud-native-platform/src/monitoring/track-exception.ts, libs/cloud-native-platform/src/monitoring/track-exception.test.ts, libs/cloud-native-platform/src/index.ts
New trackException(error, properties?) function that logs and forwards exceptions to Application Insights defaultClient when available; re-exported from package root; tests cover initialized/uninitialized AI and edge cases.
Config / paths
tsconfig.json
Added path mapping "@hmcts/account/repository/query""libs/account/src/repository/query" to support internal imports.
Documentation
docs/tickets/VIBE-143/ticket.md, docs/tickets/VIBE-143/tasks.md, docs/tickets/VIBE-143/review.md
Added ticket spec, task checklist (marked complete), and review notes documenting schema, acceptance criteria, validation gaps, and next steps.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant User
    participant Browser
    participant SSO_Callback as sso-callback
    participant CFT_Callback as cft-callback
    participant TokenClient as token-client
    participant Repo as createOrUpdateUser
    participant Prisma
    participant Track as trackException
    participant AppInsights as AppInsights

    User->>Browser: Initiate login
    Browser->>SSO_Callback: Redirect (SSO) / Browser->>CFT_Callback: Redirect (CFT)

    alt SSO flow
        SSO_Callback->>Repo: createOrUpdateUser({email, provenance: "SSO", provenanceId, role})
    else CFT flow
        CFT_Callback->>TokenClient: extract claims (uid, given_name, family_name, sub)
        TokenClient-->>CFT_Callback: {id: uid, email, firstName, surname}
        CFT_Callback->>Repo: createOrUpdateUser({email, firstName, surname, provenance: "CFT_IDAM", provenanceId: uid, role: "VERIFIED"})
    end

    Repo->>Prisma: findUnique by provenanceId
    alt user exists
        Prisma-->>Repo: user
        Repo->>Prisma: update(role?, lastSignedInDate?)
        Prisma-->>Repo: updated user
    else user missing
        Prisma-->>Repo: null
        Repo->>Prisma: create(user fields, lastSignedInDate)
        Prisma-->>Repo: created user
    end

    alt success
        Repo-->>SSO_Callback: user
        Repo-->>CFT_Callback: user
        SSO_Callback->>Browser: continue login
        CFT_Callback->>Browser: continue login
    else error
        Repo--xSSO_Callback: throw
        SSO_Callback->>Track: trackException(error, { area: "auth", userEmail, userId })
        Track->>AppInsights: trackException(...)
        SSO_Callback->>Browser: redirect /login?error=db_error
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~40 minutes

  • Pay extra attention to:
    • Database/migration correctness (types, maps, indexes; missing composite unique noted).
    • createOrUpdateUser branching logic and timestamp handling.
    • Callback error-handling paths and redirect construction.
    • Token claim mapping (uid vs sub) and name extraction edge cases.
    • Tests that mock Prisma and Application Insights for correctness.

Suggested reviewers

  • KianKwa
  • ChrisS1512

Poem

🐰 A table of users hops into sight,
Provenance stamped — SSO and CFT alight.
Callbacks now whisper, "Upsert and save,"
Exceptions tracked so no errors misbehave.
Hooray — users created, updated, and brave!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a User table to the database as the primary objective of this PR.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch vibe-143-user-table

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
libs/auth/src/user-repository/index.ts (1)

3-10: Consider adding runtime validation for provenance and role values.

While TypeScript unions provide compile-time type safety, runtime validation would guard against invalid values from external sources (e.g., deserialized data, API calls). This could prevent invalid data from being persisted to the database.

Example validation:

const VALID_PROVENANCES = ["SSO", "CFT_IDAM", "CRIME_IDAM", "B2C_IDAM"] as const;
const VALID_ROLES = ["VERIFIED", "LOCAL_ADMIN", "CTSC_ADMIN", "SYSTEM_ADMIN"] as const;

function validateInput(input: CreateUserInput): void {
  if (!VALID_PROVENANCES.includes(input.userProvenance)) {
    throw new Error(`Invalid provenance: ${input.userProvenance}`);
  }
  if (!VALID_ROLES.includes(input.role)) {
    throw new Error(`Invalid role: ${input.role}`);
  }
}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5db183 and 6a5ecc5.

📒 Files selected for processing (8)
  • apps/postgres/prisma/schema.prisma (1 hunks)
  • docs/tickets/VIBE-143/review.md (1 hunks)
  • docs/tickets/VIBE-143/tasks.md (1 hunks)
  • docs/tickets/VIBE-143/ticket.md (1 hunks)
  • libs/auth/src/pages/cft-callback/index.ts (2 hunks)
  • libs/auth/src/pages/sso-callback/index.ts (2 hunks)
  • libs/auth/src/user-repository/index.test.ts (1 hunks)
  • libs/auth/src/user-repository/index.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
libs/auth/src/pages/cft-callback/index.ts (1)
libs/auth/src/user-repository/index.ts (1)
  • createOrUpdateUser (53-66)
libs/auth/src/pages/sso-callback/index.ts (1)
libs/auth/src/user-repository/index.ts (1)
  • createOrUpdateUser (53-66)
libs/auth/src/user-repository/index.test.ts (1)
libs/auth/src/user-repository/index.ts (6)
  • CreateUserInput (3-10)
  • createUser (17-29)
  • findUserByProvenanceId (31-35)
  • findUserByEmail (37-41)
  • updateUser (43-51)
  • createOrUpdateUser (53-66)
🪛 LanguageTool
docs/tickets/VIBE-143/ticket.md

[style] ~38-~38: To form a complete sentence, be sure to include a subject.
Context: ...IMESTAMP | No | Date/Time (to seconds). Can be blank when user is first created | ...

(MISSING_IT_THERE)

docs/tickets/VIBE-143/review.md

[uncategorized] ~60-~60: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... role: ${role}`); } } ``` --- ## ⚠️ HIGH PRIORITY Issues ### 1. Graceful Failure May Hid...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: SonarQube Analysis
  • GitHub Check: E2E Tests
🔇 Additional comments (2)
libs/auth/src/user-repository/index.test.ts (1)

1-320: Excellent test coverage!

The test suite is comprehensive and well-structured, covering all repository methods with appropriate edge cases. The mocking strategy is correct, and the assertions validate both call parameters and return values.

libs/auth/src/user-repository/index.ts (1)

53-66: LGTM! Upsert pattern correctly implemented.

The createOrUpdateUser function correctly implements the upsert pattern as specified in the ticket requirements. It properly handles both new user creation and existing user updates, including role changes and timestamp updates.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 20, 2025

🎭 Playwright E2E Test Results

265 tests   265 ✅  17m 42s ⏱️
 16 suites    0 💤
  1 files      0 ❌

Results for commit e25dcc2.

♻️ This comment has been updated with latest results.

Replace console.error with trackException in SSO and CFT callback handlers to ensure failures during user creation/update are properly recorded in Application Insights. Add contextual properties (area, userEmail, userId) to aid debugging.

Changes:
- Create trackException helper function in cloud-native-platform
- Update sso-callback to track exceptions with SSO context
- Update cft-callback to track exceptions with CFT context
- Export trackException from cloud-native-platform index

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
libs/auth/src/pages/cft-callback/index.ts (1)

1-7: Harden error handling and re‑consider displayName splitting

  1. Normalise caught errors for telemetry (as in SSO callback)
    To avoid odd telemetry if a non‑Error is thrown, consider the same pattern as suggested for the SSO callback:
-    // Create or update user record in database
-    try {
+    // Create or update user record in database
+    try {
       const nameParts = userInfo.displayName.split(" ");
       const firstName = nameParts[0] || undefined;
       const surname = nameParts.slice(1).join(" ") || undefined;

       await createOrUpdateUser({
         email: userInfo.email,
         firstName,
         surname,
         userProvenance: "CFT_IDAM",
         userProvenanceId: userInfo.id,
         role: "VERIFIED"
       });
-    } catch (error) {
-      trackException(error as Error, {
+    } catch (error) {
+      const err = error instanceof Error ? error : new Error(String(error));
+      trackException(err, {
         area: "CFT callback",
         userEmail: userInfo.email,
         userId: userInfo.id
       });
       // Continue with authentication even if database write fails
     }
  1. Name parsing remains simplistic
    Splitting displayName on spaces into firstName and surname will mis-handle single names, multiple middle names, and many non‑Western formats. If the upstream CFT IDAM token exposes structured name fields, it would be more robust to prefer those and only fall back to this heuristic, or at least document the limitation where this parsing is defined.

Also applies to: 45-66

🧹 Nitpick comments (1)
libs/auth/src/pages/sso-callback/index.ts (1)

2-6: Normalize caught errors before calling trackException

The persistence try/catch is in the right place and using trackException meets the earlier monitoring requirement. To make this more robust against non-Error throws (e.g. strings), consider normalizing the caught value before passing it on:

-    // Create or update user record in database
-    try {
+    // Create or update user record in database
+    try {
       await createOrUpdateUser({
         email: req.user.email,
         userProvenance: "SSO",
         userProvenanceId: req.user.id,
         role: req.user.role as "VERIFIED" | "LOCAL_ADMIN" | "CTSC_ADMIN" | "SYSTEM_ADMIN"
       });
-    } catch (error) {
-      trackException(error as Error, {
+    } catch (error) {
+      const err = error instanceof Error ? error : new Error(String(error));
+      trackException(err, {
         area: "SSO callback",
         userEmail: req.user?.email,
         userId: req.user?.id
       });
       // Continue with authentication even if database write fails
     }

This keeps the current behaviour while ensuring telemetry always receives a proper Error instance.

Also applies to: 36-51

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a5ecc5 and 8103d26.

📒 Files selected for processing (4)
  • libs/auth/src/pages/cft-callback/index.ts (2 hunks)
  • libs/auth/src/pages/sso-callback/index.ts (2 hunks)
  • libs/cloud-native-platform/src/index.ts (1 hunks)
  • libs/cloud-native-platform/src/monitoring/track-exception.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/auth/src/pages/cft-callback/index.ts (2)
libs/auth/src/user-repository/index.ts (1)
  • createOrUpdateUser (53-66)
libs/cloud-native-platform/src/monitoring/track-exception.ts (1)
  • trackException (11-22)
libs/auth/src/pages/sso-callback/index.ts (2)
libs/auth/src/user-repository/index.ts (1)
  • createOrUpdateUser (53-66)
libs/cloud-native-platform/src/monitoring/track-exception.ts (1)
  • trackException (11-22)
🔇 Additional comments (2)
libs/cloud-native-platform/src/index.ts (1)

7-7: Expose trackException at package root looks good

Re-exporting ./monitoring/track-exception.js keeps the public surface consistent with other monitoring utilities and simplifies imports for callers.

libs/cloud-native-platform/src/monitoring/track-exception.ts (1)

1-22: Centralised exception tracking helper looks sound

The helper cleanly wraps exception tracking for routes that don’t have direct access to a monitoring service, with a defensive check around the default client and a console fallback. This should make it easy to add consistent telemetry from handlers like the SSO/CFT callbacks without risking runtime errors if monitoring isn’t initialised.

Please just confirm this logging pattern (both console.error and telemetry) aligns with your expected log volume and PII policy for the properties you pass in.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
libs/auth/src/cft-idam/token-client.ts (1)

11-18: Guard against missing id/email/displayName claims when mapping token -> user.

The new mapping to uid / sub / given_name / family_name is a good improvement over parsing a single display name. One concern: if the token ever omits some of these claims, we can end up with:

  • id: claims.uid being undefined
  • email: claims.sub || claims.email || "" becoming an empty string
  • displayName: claims.name || claims.given_name being undefined

Those values then flow into UserProfile and the user repository (e.g. as userProvenanceId and email), which are modeled as required strings and are likely constrained in the DB.

Consider adding explicit validation/fallbacks here (e.g. throw if uid or email are missing, or fall back to a safer identifier), and/or add tests that cover missing uid / sub / name scenarios so failures are deterministic instead of silently producing invalid data.

Also applies to: 77-84

🧹 Nitpick comments (3)
libs/auth/src/pages/cft-callback/index.test.ts (1)

53-60: Mocks correctly updated for new name fields; consider adding a focused upsert assertion.

Including firstName and surname in the mocked extractUserInfoFromToken responses keeps these tests aligned with the expanded CftIdamUserInfo shape and the new persistence behaviour. If you want stronger coverage of the new DB upsert, consider (in this or a separate test) mocking ../../user-repository/index.js and asserting that createOrUpdateUser is called with these fields when a non‑rejected user signs in.

Also applies to: 90-97, 132-139, 158-165, 184-191, 212-219, 248-255

libs/auth/src/pages/cft-callback/index.ts (1)

1-1: CFT callback user upsert and exception tracking look solid.

The new createOrUpdateUser call with CFT_IDAM provenance and the trackException‑backed catch provide the right balance: DB issues are observable without blocking authentication. This cleanly wires uid/email/firstName/surname into persistence while keeping the session flow unchanged.

As a minor enhancement, you could add provenance and role into the trackException properties for richer diagnostics, but the current implementation is already sufficient.

Also applies to: 7-7, 45-62

libs/auth/src/cft-idam/token-client.test.ts (1)

80-86: Tests correctly cover uid/sub and new name fields; a couple of extra fallbacks would complete coverage.

The updated tests now assert that:

  • uid is used for id and sub for email
  • given_name / family_name map to firstName / surname
  • Roles behaviour (array, empty, missing) is preserved

This matches the production mapping and gives good confidence. If you want to fully lock down the new logic, consider adding small tests for:

  • Missing name but present given_name (displayName fallback)
  • Missing sub but present email (email fallback)

Not strictly necessary, but would make the behaviour fully specified.

Also applies to: 99-106, 111-116, 129-136, 141-147, 163-168, 182-202

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8103d26 and 674a69d.

📒 Files selected for processing (8)
  • apps/postgres/prisma/migrations/20251121153203_create_user/migration.sql (1 hunks)
  • apps/postgres/prisma/schema.prisma (1 hunks)
  • libs/auth/src/cft-idam/token-client.test.ts (6 hunks)
  • libs/auth/src/cft-idam/token-client.ts (2 hunks)
  • libs/auth/src/pages/cft-callback/index.test.ts (7 hunks)
  • libs/auth/src/pages/cft-callback/index.ts (2 hunks)
  • libs/auth/src/user-repository/index.test.ts (1 hunks)
  • libs/auth/src/user-repository/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • libs/auth/src/user-repository/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/postgres/prisma/schema.prisma
  • libs/auth/src/user-repository/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/auth/src/pages/cft-callback/index.ts (2)
libs/auth/src/user-repository/index.ts (1)
  • createOrUpdateUser (53-66)
libs/cloud-native-platform/src/monitoring/track-exception.ts (1)
  • trackException (11-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: SonarQube Analysis
  • GitHub Check: E2E Tests
🔇 Additional comments (1)
apps/postgres/prisma/migrations/20251121153203_create_user/migration.sql (1)

1-17: Migration structure is sound.

The table design properly supports the provenance-based upsert pattern with:

  • UUID primary key for consistent, distributed user identification
  • Unique constraint on user_provenance_id to enforce single identity per auth provider
  • Appropriate nullable fields (first_name, surname, last_signed_in_date)
  • Sensible column sizing and timestamp defaults

Assuming email is always populated, this migration aligns well with the repository layer expectations.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
libs/cloud-native-platform/src/monitoring/track-exception.test.ts (1)

108-118: Consider using a more maintainable mock restoration pattern.

The manual mutation and restoration of defaultClient works but is fragile. If the mock structure evolves, the restoration logic in lines 115-117 requires manual updates. Consider extracting the mock creation into a helper function or using vi.spyOn to temporarily override behavior.

For example:

const createMockClient = () => ({
  trackException: vi.fn()
});

// In the outer beforeEach, you could reset properly
// Or use vi.doMock with dynamic factory

This would make the tests more maintainable and reduce the risk of test pollution.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 674a69d and 641f2b1.

📒 Files selected for processing (1)
  • libs/cloud-native-platform/src/monitoring/track-exception.test.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: E2E Tests
🔇 Additional comments (6)
libs/cloud-native-platform/src/monitoring/track-exception.test.ts (6)

1-3: LGTM!

The imports are clean and appropriate for the test setup. The ESM-style import with .js extension is correct for TypeScript projects using ES modules.


5-13: LGTM!

The mock setup correctly isolates the applicationinsights module for testing. The minimal mock structure with just trackException is appropriate for these focused unit tests.


18-25: LGTM!

The test setup and teardown properly manages mock state and console spy lifecycle. Clearing mocks in beforeEach and restoring the spy in afterEach ensures test isolation.


27-105: LGTM!

The test coverage for initialized Application Insights is thorough, checking basic exception tracking, properties handling, and edge cases like special characters and multiline messages. The assertions correctly verify both console logging and telemetry calls.


120-141: LGTM!

The tests properly verify the fallback behavior when Application Insights is not initialized, ensuring errors and properties are still logged to the console with appropriate messaging.


144-212: LGTM!

The edge case tests are well-designed, covering empty messages, nested objects, and null/undefined values. The use of expect.any(String) for the timestamp field (line 182) is a good practice for testing dynamic values.

@@ -0,0 +1,66 @@
import { prisma } from "@hmcts/postgres";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought in one of your previous tickets, you create a lib called account? Can it be moved there. In account create a folder called repository and add the repository method in a class called queries.ts. Please refer to publication/location and follow the same style

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, move to account lib and rename it as well.

@@ -0,0 +1,66 @@
import { prisma } from "@hmcts/postgres";

export interface CreateUserInput {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to account/repository/model.ts? Also rename the interface from CreateUserInput to just User.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

userEmail: req.user?.email,
userId: req.user?.id
});
// Continue with authentication even if database write fails
Copy link
Contributor

@KianKwa KianKwa Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this behaviour is what we want. In current CaTH, we redirect to error page if the user failed to be created. Might need a chat to see what do we want for AI CaTH

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redirecting to error page now in case of any error.

@sonarqubecloud
Copy link

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
libs/account/package.json (1)

5-13: Use workspace:* or bounded semver for @hmcts/postgres peerDependency

@hmcts/postgres is an internal workspace package (marked "private": true), yet @hmcts/account references it as "*" in peerDependencies. This is inconsistent: all other packages in the monorepo reference @hmcts/postgres as a regular dependency with "workspace:*". Either switch to the workspace pattern or use a bounded semver range (e.g., "^1.0.0") if peerDependency is intentional. Using "*" allows any external version and breaks the monorepo's internal dependency management.

🧹 Nitpick comments (6)
libs/auth/src/pages/sso-callback/index.test.ts (1)

13-16: Leverage the createOrUpdateUser mock in assertions

You mock createOrUpdateUser but never assert on it; consider adding a happy‑path test that expects it to be called with the SSO user details, and another where it rejects to assert the /login?error=db_error redirect and telemetry behaviour. This will protect the new persistence flow from regressions.

libs/auth/src/pages/cft-callback/index.test.ts (1)

11-13: Add coverage for persistence interaction and db_error handling

You’ve updated the token payloads to include firstName/surname and mocked @hmcts/account/repository/query, but the tests still only assert redirects and session behaviour. Consider in the successful authentication test:

  • Asserting that createOrUpdateUser is called with email, firstName, surname, provenance CFT_IDAM, and the correct userProvenanceId/role.
  • Adding a test where createOrUpdateUser rejects so you can assert the /sign-in?error=db_error&lng=... redirect and that telemetry is invoked.

This will better exercise the new user‑persistence logic added to the CFT callback.

Also applies to: 56-63, 93-100, 135-142, 161-168, 187-194, 215-222, 251-258

libs/auth/src/pages/sso-callback/index.ts (1)

2-3: Persistence + telemetry wiring looks good; add tests and consider centralising role type

The SSO callback now upserts the user via createOrUpdateUser and uses trackException with useful context before redirecting to /login?error=db_error, which is a solid pattern. Two follow‑ups to consider:

  • Add a unit test where createOrUpdateUser rejects so you can assert both the trackException invocation and the ?error=db_error redirect.
  • Avoid repeating the role string‑literal union here by reusing a shared role type (e.g. from the account model or a central enum) instead of as "VERIFIED" | "LOCAL_ADMIN" | "CTSC_ADMIN" | "SYSTEM_ADMIN", to keep things in sync if roles change.

Also double‑check with product/UX that failing back to /login?error=db_error matches the intended behaviour compared to the legacy CaTH flow.

Also applies to: 36-51

libs/auth/src/pages/cft-callback/index.ts (1)

1-2: CFT persistence + telemetry integration is solid; add db_error test coverage

The CFT callback now upserts the user (including firstName/surname) and uses trackException with contextual properties before redirecting to /sign-in?error=db_error&lng=${lng} on failure, which aligns well with the new repository and monitoring utilities.

To tighten this up:

  • Add a test where createOrUpdateUser rejects, asserting the db_error redirect and that telemetry is called with the expected area, userEmail, and userId.
  • (Optional) You can reuse the top‑level lng constant in the outer catch instead of recomputing it.

Also applies to: 45-62

libs/account/src/repository/model.ts (1)

1-13: Interfaces match repository usage; consider centralising shared enums

User and UpdateUserInput cleanly capture the fields used by the repository and callbacks, with sensible unions for userProvenance and role. To avoid drift as more parts of the system depend on these values, consider centralising the role/provenance unions (e.g. shared enum or exported type) and reusing them in auth flows and elsewhere rather than duplicating string literals.

libs/account/src/repository/query.ts (1)

1-53: Consider Prisma upsert to consolidate createOrUpdateUser logic and eliminate race condition

The CRUD functions are correct and well-structured. The createOrUpdateUser function implements the intended "update lastSignedInDate or create" behavior, but currently uses a findUserByProvenanceId + conditional create/update sequence that creates a small race window: two concurrent logins for a new user could both see "no record" and attempt create, relying on the unique constraint and caller error handling.

Prisma 6.19.0 (in use here) supports upsert natively. Since userProvenanceId is marked @unique in the schema, you could replace the explicit find-then-create/update pattern with a single prisma.user.upsert call keyed on userProvenanceId. This would:

  • Eliminate the race window entirely (atomic at the database level when criteria are met)
  • Reduce two DB round-trips to one

Example pattern:

return await prisma.user.upsert({
  where: { userProvenanceId: input.userProvenanceId },
  update: { role: input.role, lastSignedInDate: new Date() },
  create: { /* all fields from input */ }
});
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a0f232 and 785ffb7.

📒 Files selected for processing (9)
  • libs/account/package.json (2 hunks)
  • libs/account/src/repository/model.ts (1 hunks)
  • libs/account/src/repository/query.test.ts (1 hunks)
  • libs/account/src/repository/query.ts (1 hunks)
  • libs/auth/src/pages/cft-callback/index.test.ts (8 hunks)
  • libs/auth/src/pages/cft-callback/index.ts (2 hunks)
  • libs/auth/src/pages/sso-callback/index.test.ts (1 hunks)
  • libs/auth/src/pages/sso-callback/index.ts (2 hunks)
  • tsconfig.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • tsconfig.json
🧰 Additional context used
🧬 Code graph analysis (4)
libs/account/src/repository/query.test.ts (2)
libs/account/src/repository/model.ts (1)
  • User (1-8)
libs/account/src/repository/query.ts (5)
  • createUser (4-16)
  • findUserByProvenanceId (18-22)
  • findUserByEmail (24-28)
  • updateUser (30-38)
  • createOrUpdateUser (40-53)
libs/auth/src/pages/cft-callback/index.ts (2)
libs/account/src/repository/query.ts (1)
  • createOrUpdateUser (40-53)
libs/cloud-native-platform/src/monitoring/track-exception.ts (1)
  • trackException (11-22)
libs/account/src/repository/query.ts (1)
libs/account/src/repository/model.ts (2)
  • User (1-8)
  • UpdateUserInput (10-13)
libs/auth/src/pages/sso-callback/index.ts (2)
libs/account/src/repository/query.ts (1)
  • createOrUpdateUser (40-53)
libs/cloud-native-platform/src/monitoring/track-exception.ts (1)
  • trackException (11-22)
🔇 Additional comments (1)
libs/account/src/repository/query.test.ts (1)

1-323: Comprehensive and precise coverage for the user repository

These tests thoroughly exercise all repository functions, including optional fields and the different createOrUpdateUser branches, and verify the exact Prisma call shapes. The mocking pattern for @hmcts/postgres and subsequent use of prisma looks appropriate for this setup.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
libs/account/package.json (1)

10-13: Add types declaration to the subpath export for better TypeScript support.

The new ./repository/query export is missing a types field, which is a best practice for TypeScript modules. Without it, TypeScript consumers may not properly resolve types when importing from this subpath.

Apply this diff to add the types field:

     "./repository/query": {
+      "types": "./dist/repository/query.d.ts",
       "production": "./dist/repository/query.js",
       "default": "./src/repository/query.ts"
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 785ffb7 and e25dcc2.

📒 Files selected for processing (1)
  • libs/account/package.json (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: E2E Tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants