Conversation
Add technical implementation plan and task list for completing the media user creation journey with Azure AD B2C integration. Plan includes: - Azure AD B2C user creation/update via Graph API - Conditional email notifications (new vs existing users) - Error handling strategy for Azure AD and email failures - Environment variables for GOV.UK Notify templates - 8 clarifying questions that need resolution Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
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 Azure AD B2C provisioning to the media application approval flow: controller obtains a Graph API token, service checks/creates Azure AD users and local accounts, updates application status, and the controller sends environment-driven GOV.UK Notify emails for new or existing users with i18n-aware error handling and CI/mock flags. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as Approve Controller
participant Service as Media App Service
participant GraphAPI as Graph API Client
participant Database as Local Database
participant Notify as GOV.UK Notify
Client->>Controller: POST /approve (application id)
Controller->>GraphAPI: getGraphApiAccessToken()
GraphAPI-->>Controller: accessToken
Controller->>Service: approveApplication(id, accessToken)
Service->>GraphAPI: findUserByEmail(accessToken, email)
GraphAPI-->>Service: userId or null
alt user does not exist
Service->>GraphAPI: createMediaUser(accessToken, userData)
GraphAPI-->>Service: { azureAdUserId }
Service->>Database: createUser(local, provenance=AZURE_B2C, role=VERIFIED)
Database-->>Service: user created
Service-->>Controller: { isNewUser: true }
else user exists
Service->>GraphAPI: updateMediaUser(accessToken, userId, userData)
Service->>Database: updateUser(local with names)
Service-->>Controller: { isNewUser: false }
end
Service->>Database: updateApplicationStatus -> APPROVED
Database-->>Service: updated
Controller->>Notify: sendMediaNewAccountEmail(...) or sendMediaExistingUserEmail(...)
Notify-->>Controller: sent / error (logged, non-blocking)
Controller-->>Client: redirect to approved page
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 |
🎭 Playwright E2E Test Results257 tests 257 ✅ 26m 22s ⏱️ Results for commit 946e551. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/account/src/repository/query.ts (1)
4-14:⚠️ Potential issue | 🟠 MajorVerify
lastSignedInDatedatabase default before this lands.
lastSignedInDate: new Date()was removed from thecreateUserpayload. The database column has no default value and is nullable, so all newly created users will have aNULLlastSignedInDate. SincecreateUseris a shared function, any downstream logic expecting this field to be populated on creation (e.g., "last seen" displays, reporting queries) will silently break.e2e-tests/tests/media-application-management.spec.ts (2)
41-63:⚠️ Potential issue | 🟠 Major
afterAllleaks localuserrecords created during the approval journey.The approval flow (
approveApplication) now callscreateLocalMediaUser, which inserts a record into theusertable. TheafterAllonly deletesmediaApplicationrecords. Every test run leaves an orphaneduserrow (and potentially an Azure AD B2C account) forapprovalEmailand the inlinetestAppemail.🔧 Suggested cleanup addition
test.afterAll(async () => { const { prisma } = await import("@hmcts/postgres"); + // Clean up local user records created by the approval flow + if (approvalEmail) { + await prisma.user.deleteMany({ where: { email: approvalEmail } }).catch(() => {}); + } + if (approvalApplicationId) { await prisma.mediaApplication .delete({ where: { id: approvalApplicationId } }) .catch(() => {}); } // ... rest of cleanup });
260-323:⚠️ Potential issue | 🟡 MinorSeparate
AccessibilityandWelsh Language Supportdescribes violate the E2E test structure guideline.These checks should be folded inline into the existing journey tests rather than living in separate
test.describeblocks. As per coding guidelines: "E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests."
🧹 Nitpick comments (7)
docs/tickets/346/review.md (1)
1-6: Review artefact committed to the source tree.This document serves as a peer-review note rather than production documentation. Committing it to
docs/tickets/means it will become stale immediately and adds noise to the repository history. PR comments or a linked GitHub issue are more appropriate homes for this content.libs/auth/src/graph-api/client.ts (1)
26-30:Client.initduplicated three times — extract a helper.
fetchUserProfile,checkUserExists, andcreateMediaUserall repeat the sameClient.initblock.♻️ Suggested refactor
+function createGraphClient(accessToken: string) { + return Client.init({ + authProvider: (done) => done(null, accessToken) + }); +} export async function fetchUserProfile(accessToken: string): Promise<UserProfile> { - const client = Client.init({ - authProvider: (done) => { - done(null, accessToken); - } - }); + const client = createGraphClient(accessToken); // ...Also applies to: 108-112, 145-149
apps/web/helm/values.dev.yaml (1)
48-51: Confirm least-privilege intent for Azure B2C credentials.The secrets
auto-pip-stg-pip-account-management-stg-id/auto-pip-stg-pip-account-management-stg-pwdappear to belong to thepip-account-managementservice. Verify that this application registration has only the Graph API permissions needed (User.ReadWrite.Allscoped to the B2C tenant) and that re-using cross-service credentials aligns with your security posture.libs/auth/src/graph-api/client.test.ts (2)
257-263: OnlyAZURE_B2C_TENANT_IDis deleted — missing credential combinations aren't tested.The implementation likely guards against all three missing env vars (
TENANT_ID,CLIENT_ID,CLIENT_SECRET). A single deletion only exercises one code path. Consider adding sibling cases that deleteCLIENT_IDandCLIENT_SECRETindividually.
473-491: No test for a Graph API 409 (user-already-exists) conflict response.Given the TOCTOU window between
checkUserExistsandcreateMediaUser, a concurrent approval could trigger a409 Conflict. The current error test only covers a generic"Forbidden"message. A dedicated 409 scenario would confirm the error propagates correctly and surfaces a meaningful message to the caller.libs/admin-pages/src/media-application/service.ts (1)
72-84: Remove the export fromsplitNameand test it indirectly through the functions that use it.The function is only exported to enable direct unit testing in
service.test.ts. Since it's used internally withinservice.ts(lines 22 and 87), it can be tested indirectly throughapproveApplicationandcreateLocalMediaUserrather than imported for standalone unit tests.libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (1)
153-276: Thorough POST handler test coverage — nice work on the branching paths.New user, existing user, email failure, Azure AD failure, and token failure are all well-covered.
One gap: there's no Welsh (
?lng=cy) test for any POST error scenario. The GET suite has one (line 111), but the POST suite doesn't verify that error messages render in Welsh. As per coding guidelines: "Every page must support both English and Welsh by providing en and cy content objects to the renderer, and templates should test with ?lng=cy query parameter."
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/web/.env.exampleapps/web/helm/values.dev.yamlapps/web/helm/values.yamldocs/tickets/346/plan.mddocs/tickets/346/review.mddocs/tickets/346/tasks.mddocs/tickets/346/ticket.mde2e-tests/tests/media-application-management.spec.tslibs/account/src/repository/query.test.tslibs/account/src/repository/query.tslibs/admin-pages/src/media-application/service.test.tslibs/admin-pages/src/media-application/service.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.tslibs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/admin-pages/src/pages/media-applications/[id]/reject.test.tslibs/admin-pages/src/pages/media-applications/[id]/reject.tslibs/auth/src/graph-api/client.test.tslibs/auth/src/graph-api/client.tslibs/auth/src/index.tslibs/notification/src/govuk-notify-service.test.tslibs/notification/src/govuk-notify-service.tslibs/notification/src/index.ts
| REDIS_HOST: 'cath-{{ .Values.global.environment }}.redis.cache.windows.net' | ||
| BASE_URL: 'https://{{ .Values.nodejs.ingressHost }}' | ||
| SSO_ALLOW_HTTP_REDIRECT: 'false' | ||
| MEDIA_FORGOT_PASSWORD_LINK: 'https://sign-in.pip-frontend.staging.platform.hmcts.net/pip-frontend.staging.platform.hmcts.net/oauth2/v2.0/authorize?p=B2C_1A_PASSWORD_RESET&client_id=cae650ba-431b-4fc8-be14-22d476ebd31b&nonce=defaultNonce&redirect_uri=https://pip-frontend.staging.platform.hmcts.net/password-change-confirmation&scope=openid&response_type=code&prompt=login&response_mode=form_post' |
There was a problem hiding this comment.
MEDIA_FORGOT_PASSWORD_LINK is hardcoded to a staging endpoint in a file used for production deployments.
The URL contains staging.platform.hmcts.net multiple times. Since values.yaml is described as being used by Flux for both non-prod and prod deployments, production users approving media applications will receive emails with a link pointing at the staging B2C password-reset endpoint.
This should either use a Helm template expression (e.g., {{ .Values.global.environment }}) or be overridden per-environment in separate values.<env>.yaml files.
| export async function sendMediaNewAccountEmail(data: { | ||
| email: string; | ||
| full_name: string; | ||
| forgot_password_link: string; | ||
| }): Promise<void> | ||
|
|
||
| /** | ||
| * Send existing user confirmation email to media user | ||
| * Template ID: cc1b744d-6aa1-4410-9f53-216f8bd3298f | ||
| */ | ||
| export async function sendMediaExistingUserEmail(data: { | ||
| email: string; | ||
| forgot_password_link: string; | ||
| subscription_page_link: string; | ||
| start_page_link: string; | ||
| }): Promise<void> | ||
| ``` | ||
|
|
||
| **Implementation Notes:** | ||
| - Follow existing pattern from `sendMediaApprovalEmail` and `sendMediaRejectionEmail` | ||
| - Read template IDs from environment variables | ||
| - **Personalisation field names use underscores** (e.g., `full_name`, `forgot_password_link`, `subscription_page_link`, `start_page_link`) to match GOV.UK Notify template configuration | ||
| - Use descriptive reference strings (e.g., `media-new-account-${Date.now()}`) | ||
|
|
||
| ### Service Layer Changes | ||
|
|
||
| **Location:** `libs/admin-pages/src/media-application/service.ts` | ||
|
|
||
| Modify `approveApplication` function: | ||
|
|
||
| ```typescript | ||
| export async function approveApplication( | ||
| id: string, | ||
| accessToken: string // NEW: Access token from client credentials flow | ||
| ): Promise<{ isNewUser: boolean }> { | ||
| const application = await getApplicationById(id); | ||
|
|
||
| if (!application) { | ||
| throw new Error("Application not found"); | ||
| } | ||
|
|
||
| if (application.status !== APPLICATION_STATUS.PENDING) { | ||
| throw new Error("Application has already been reviewed"); | ||
| } | ||
|
|
||
| // NEW: Check if user exists in Azure AD B2C | ||
| const userExists = await checkUserExists(accessToken, application.email); | ||
|
|
||
| // NEW: Create user in Azure AD B2C only if they don't already exist | ||
| if (!userExists) { | ||
| await createMediaUser(accessToken, { | ||
| email: application.email, | ||
| displayName: application.name, | ||
| givenName: application.givenName, // Assumes these fields exist on application | ||
| surname: application.surname | ||
| }); | ||
| } | ||
| // NOTE: Existing users are NOT updated in Azure AD | ||
|
|
||
| // NEW: Create local user record | ||
| await createLocalMediaUser({ | ||
| email: application.email, | ||
| displayName: application.name, | ||
| userProvenance: "AZURE_B2C", | ||
| role: "VERIFIED" | ||
| }); | ||
|
|
||
| // EXISTING: Update application status | ||
| await updateApplicationStatus(id, APPLICATION_STATUS.APPROVED); | ||
|
|
||
| // EXISTING: Delete proof of ID file | ||
| if (application.proofOfIdPath) { | ||
| await deleteProofOfIdFile(application.proofOfIdPath); | ||
| } | ||
|
|
||
| return { isNewUser: !userExists }; | ||
| } |
There was a problem hiding this comment.
Plan is stale — multiple discrepancies with the actual implementation.
A few notable mismatches that could mislead future readers:
| Plan says | Implementation does |
|---|---|
sendMediaExistingUserEmail takes forgot_password_link, subscription_page_link, start_page_link |
Takes email, fullName, signInPageLink only |
application.givenName / application.surname (line 176-177) |
Uses splitName(application.name) |
createLocalMediaUser called unconditionally (line 183) |
Called only inside if (!userExists) |
userProvenance: "AZURE_B2C" (line 186) |
Actual value is "B2C_IDAM" (per tests) |
createOrUpdateMediaUser (line 429) |
Function is createMediaUser |
Consider updating the plan to reflect the implemented design, or adding a note at the top that the plan is superseded by the code.
🧰 Tools
🪛 LanguageTool
[style] ~144-~144: Would you like to use the Oxford spelling “Personalization”? The spelling ‘Personalisation’ is also correct.
Context: ...late IDs from environment variables - Personalisation field names use underscores (e.g., `f...
(OXFORD_SPELLING_Z_NOT_S)
| ## CRITICAL Issues | ||
|
|
||
| ### 1. OData Injection in `checkUserExists` | ||
|
|
||
| **File:** `/workspaces/cath-service/libs/auth/src/graph-api/client.ts`, line 115 | ||
|
|
||
| ```typescript | ||
| const result = await client.api("/users").filter(`mail eq '${email}'`).select("id").get(); | ||
| ``` | ||
|
|
||
| The email address is interpolated directly into an OData filter expression without sanitisation. A malicious or malformed email containing single quotes (e.g., `O'Brien@example.com` or a crafted value like `' or 1 eq 1 or mail eq '`) could break the query or return unintended results from the Graph API. This is the OData equivalent of SQL injection. | ||
|
|
||
| **Impact:** An attacker or even a legitimate user with an apostrophe in their email could cause the user existence check to return incorrect results, potentially skipping Azure AD user creation or erroneously treating a new user as existing. | ||
|
|
||
| **Solution:** Escape single quotes in the email by replacing `'` with `''` (OData escaping convention): | ||
|
|
||
| ```typescript | ||
| const sanitisedEmail = email.replace(/'/g, "''"); | ||
| const result = await client.api("/users").filter(`mail eq '${sanitisedEmail}'`).select("id").get(); | ||
| ``` |
There was a problem hiding this comment.
CRITICAL #1 is already fixed — the Next Steps checklist is stale.
The OData sanitisation described here has already been applied in libs/auth/src/graph-api/client.ts (line 115: const sanitisedEmail = email.replace(/'/g, "''")). The "Next Steps" item at line 175 still lists it as an open TODO, which is misleading for anyone triaging this document.
Update the checklist to mark it done, or remove the entry.
🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Would you like to use the Oxford spelling “sanitization”? The spelling ‘sanitisation’ is also correct.
Context: ...into an OData filter expression without sanitisation. A malicious or malformed email contain...
(OXFORD_SPELLING_Z_NOT_S)
| - [x] Add `sendMediaNewAccountEmail` function to `libs/notification/src/govuk-notify-service.ts` (personalisation: `full_name`, `forgot_password_link`) | ||
| - [x] Add `sendMediaExistingUserEmail` function to `libs/notification/src/govuk-notify-service.ts` (personalisation: `forgot_password_link`, `subscription_page_link`, `start_page_link`) | ||
| - [x] Add unit tests for `sendMediaNewAccountEmail` in `libs/notification/src/govuk-notify-service.test.ts` | ||
| - [x] Add unit tests for `sendMediaExistingUserEmail` in `libs/notification/src/govuk-notify-service.test.ts` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "govuk-notify-service" --exec cat {}Repository: hmcts/cath-service
Length of output: 13268
Both sendMediaNewAccountEmail and sendMediaExistingUserEmail have incorrect personalisation fields documented.
Line 26 lists forgot_password_link, but the implementation uses "forgot password process link" instead.
Line 27 lists forgot_password_link, subscription_page_link, and start_page_link, but the implementation only sends "Full name" and "sign in page link". Update the documentation to match the actual implementation.
🧰 Tools
🪛 LanguageTool
[style] ~26-~26: Would you like to use the Oxford spelling “personalization”? The spelling ‘personalisation’ is also correct.
Context: ...ification/src/govuk-notify-service.ts(personalisation:full_name, forgot_password_link`) -...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~27-~27: Would you like to use the Oxford spelling “personalization”? The spelling ‘personalisation’ is also correct.
Context: ...ification/src/govuk-notify-service.ts(personalisation:forgot_password_link, subscription_...
(OXFORD_SPELLING_Z_NOT_S)
| const accessToken = await getGraphApiAccessToken(); | ||
| const { isNewUser } = await approveApplication(id, accessToken); | ||
|
|
||
| const emailData = { | ||
| email: application.email, | ||
| fullName: application.name, | ||
| signInPageLink: process.env.MEDIA_FORGOT_PASSWORD_LINK || "" | ||
| }; | ||
|
|
||
| // Send approval email notification | ||
| try { | ||
| await sendMediaApprovalEmail({ | ||
| name: application.name, | ||
| email: application.email, | ||
| employer: application.employer | ||
| }); | ||
| if (isNewUser) { | ||
| await sendMediaNewAccountEmail(emailData); | ||
| } else { | ||
| await sendMediaExistingUserEmail(emailData); | ||
| } | ||
| } catch (error) { | ||
| console.error("❌ Failed to send approval email:", error); | ||
| // Don't fail the approval if email fails | ||
| const { status, message } = extractNotifyError(error); | ||
| console.error(`Failed to send confirmation email: ${status} ${message}`); | ||
| } | ||
|
|
||
| res.redirect(`/media-applications/${id}/approved`); | ||
| } catch (_error) { | ||
| } catch (error) { | ||
| console.error("Failed to approve media application:", error); | ||
| const application = await getApplicationById(id).catch(() => null); | ||
|
|
||
| res.render("media-applications/[id]/approve", { | ||
| pageTitle: lang.pageTitle, | ||
| error: lang.errorMessages.loadFailed, | ||
| application: null, | ||
| error: lang.errorMessages.azureAdFailed, |
There was a problem hiding this comment.
Catch-all error handler shows misleading Azure AD message for unrelated failures.
The outer catch on line 105 renders azureAdFailed for every thrown error, but the try block also covers getApplicationById (line 54) and approveApplication — which can throw "Application has already been reviewed" (a status check) or a DB error, neither of which is an Azure AD issue. An admin who double-clicks approve, or hits a brief DB blip, sees "Unable to create user account in Azure AD".
Consider differentiating error types, e.g. by checking the error message or introducing typed errors:
Proposed approach
} catch (error) {
console.error("Failed to approve media application:", error);
const application = await getApplicationById(id).catch(() => null);
+ const errorMessage =
+ error instanceof Error && error.message === "Application has already been reviewed"
+ ? lang.errorMessages.alreadyReviewed
+ : lang.errorMessages.azureAdFailed;
res.render("media-applications/[id]/approve", {
pageTitle: lang.pageTitle,
- error: lang.errorMessages.azureAdFailed,
+ error: errorMessage,
subheading: lang.subheading,| const response = await fetch(tokenUrl, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/x-www-form-urlencoded" }, | ||
| body: body.toString() | ||
| }); |
There was a problem hiding this comment.
No timeout on fetch — risks hanging the approval request indefinitely.
The token endpoint call has no AbortController signal. If the Azure token endpoint is slow or unreachable, the entire approval handler will stall with no circuit-break.
🛠️ Proposed fix
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10_000);
const response = await fetch(tokenUrl, {
method: "POST",
headers: { "Content-Type": "application/x-www-form-urlencoded" },
- body: body.toString()
+ body: body.toString(),
+ signal: controller.signal
});
+ clearTimeout(timeoutId);📝 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.
| const response = await fetch(tokenUrl, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/x-www-form-urlencoded" }, | |
| body: body.toString() | |
| }); | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 10_000); | |
| const response = await fetch(tokenUrl, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/x-www-form-urlencoded" }, | |
| body: body.toString(), | |
| signal: controller.signal | |
| }); | |
| clearTimeout(timeoutId); |
| identities: [ | ||
| { | ||
| signInType: "emailAddress", | ||
| issuer: b2cDomain, | ||
| issuerAssignedId: userData.email | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libs/auth/src/graph-api/client.ts | head -200Repository: hmcts/cath-service
Length of output: 6796
🏁 Script executed:
grep -n "b2cDomain" libs/auth/src/graph-api/client.tsRepository: hmcts/cath-service
Length of output: 240
🏁 Script executed:
rg "AZURE_B2C_DOMAIN|b2c-ad-url" --type ts --type jsRepository: hmcts/cath-service
Length of output: 808
🏁 Script executed:
grep -rn "identities" libs/auth/src/graph-api/ --type tsRepository: hmcts/cath-service
Length of output: 179
🌐 Web query:
Microsoft Graph API identities issuer B2C requirements .onmicrosoft.com custom domain
💡 Result:
For Microsoft Graph operations on Azure AD B2C local accounts (identities with signInType like emailAddress / userName):
identities[].issuermust be your B2C tenant’s default domain:yourtenant.onmicrosoft.com(the initial domain). Custom domains are not used as theissuervalue for local accounts in theidentitiescollection. [1]- Custom domains are for B2C endpoints and token issuer (
iss), not for Graphidentities[].issuer. When you use a custom domain for sign-in, the tokenissclaim changes accordingly, but that’s separate from the Graph user object’sidentitiesissuer. [2] - Uniqueness rule:
issuer+issuerAssignedIdmust be unique in the tenant. For local accounts,issuerAssignedIdis typically the email/username used to sign in. [1] - Querying/filtering note: when filtering users by
identitiesfor local sign-in identifiers, Graph may effectively ignoreissuer(it matters more for federated identities). [3]
So: even if you configure and use a custom domain (e.g., login.contoso.com) for B2C sign-in, in Microsoft Graph you typically still set identities[].issuer to yourtenant.onmicrosoft.com for local accounts. [1][2]
References: [1] [2] [3]
🌐 Web query:
Azure AD B2C Graph API local account issuer format documentation
💡 Result:
For Azure AD B2C local accounts in the Microsoft Graph API, the user.identities[] object uses this issuer format:
issuer: your B2C tenant’s default domain name, e.g.contoso.onmicrosoft.com(this applies whensignInTypeis notfederated).- Example (local account):
signInType:emailAddressoruserNameissuer:contoso.onmicrosoft.comissuerAssignedId: the email/username value.
This is documented in “User profile attributes in Azure Active Directory B2C” under the Identities attribute description and example JSON. [1]
If you meant the token iss claim (not Graph user identity issuer), B2C can emit issuers in either https://<domain>/<tenant>/v2.0/ or https://<domain>/tfp/<tenant>/<policy>/v2.0/ formats (configured per user flow). [2]
Sources: [1] “User profile attributes in Azure Active Directory B2C” (Microsoft Learn) (learn.microsoft.com), [2] “Azure AD B2C issuer claim support” (microsoft-identity-web wiki) (github.com)
Citations:
- 1: https://learn.microsoft.com/en-us/azure/active-directory-b2c/user-profile-attributes?utm_source=openai
- 2: https://github.com/AzureAD/microsoft-identity-web/wiki/Azure-AD-B2C-issuer-claim-support?utm_source=openai
identities.issuer must be the tenant's .onmicrosoft.com domain — a custom domain will be rejected by Graph API.
For local accounts (where signInType is emailAddress), Microsoft Graph requires issuer to be the B2C tenant's default domain, for example contoso.onmicrosoft.com. Custom domains are only used for B2C sign-in endpoints and token issuers, not for the identities.issuer field. If AZURE_B2C_DOMAIN holds a custom domain or URL, the /users POST request will fail with a 400 Bad Request.
Add validation to ensure AZURE_B2C_DOMAIN is in the correct *.onmicrosoft.com format, or extract the domain name if the variable may contain additional context.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/admin-pages/src/pages/media-applications/[id]/approve.ts (1)
110-126:⚠️ Potential issue | 🟠 MajorCatch‑all Azure AD error message is misleading for non‑Azure failures.
This was raised previously and still applies; the error message should distinguish Azure AD failures from other exceptions (e.g. already‑reviewed or DB errors).
🧹 Nitpick comments (1)
apps/web/.env.example (1)
38-48: Consider reordering env keys to satisfy dotenv-linter.If dotenv-linter runs in CI, the current order will trigger UnorderedKey warnings. Reordering the keys will keep the file clean and avoid noisy lint failures.
♻️ Proposed reordering
-GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_REJECTION=template-uuid-here -GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_NEW_ACCOUNT=template-uuid-here -GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_EXISTING_USER=template-uuid-here +GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_EXISTING_USER=template-uuid-here +GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_NEW_ACCOUNT=template-uuid-here +GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_REJECTION=template-uuid-here ... -MEDIA_FORGOT_PASSWORD_LINK= -MEDIA_SUBSCRIPTION_PAGE_LINK= -MEDIA_START_PAGE_LINK= +MEDIA_FORGOT_PASSWORD_LINK= +MEDIA_START_PAGE_LINK= +MEDIA_SUBSCRIPTION_PAGE_LINK=
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/.env.examplelibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.ts
There was a problem hiding this comment.
Actionable comments posted: 2
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/graph-api/client.test.ts (1)
1-15:⚠️ Potential issue | 🟡 MinorRestore the global fetch stub after the suite.
Leaving
fetchstubbed can leak into other test files running in the same worker. Move the stub intobeforeAll/afterAll(or explicitly unstub inafterAll).Suggested fix
-import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; @@ -const mockFetch = vi.fn(); -vi.stubGlobal("fetch", mockFetch); +const mockFetch = vi.fn(); +beforeAll(() => { + vi.stubGlobal("fetch", mockFetch); +}); +afterAll(() => { + vi.unstubAllGlobals(); +});
♻️ Duplicate comments (1)
libs/auth/src/graph-api/client.ts (1)
118-139:⚠️ Potential issue | 🟠 MajorUse
identitiesfiltering for B2C local accounts (notB2C local accounts commonly store the sign-in email under
identities[].issuerAssignedId, so filtering bySuggested fix
- const result = await client.api("/users").filter(`mail eq '${sanitisedEmail}'`).select("id").get(); + const result = await client + .api("/users") + .filter(`identities/any(i:i/issuerAssignedId eq '${sanitisedEmail}' and i/signInType eq 'emailAddress')`) + .select("id") + .get();Microsoft Graph API Azure AD B2C filter identities issuerAssignedId signInType emailAddress
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/e2e.yml.github/workflows/nightly.ymllibs/auth/src/graph-api/client.test.tslibs/auth/src/graph-api/client.ts
| describe("getGraphApiAccessToken", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| process.env.AZURE_B2C_TENANT_ID = "test-tenant-id"; | ||
| process.env.AZURE_B2C_CLIENT_ID = "test-client-id"; | ||
| process.env.AZURE_B2C_CLIENT_SECRET = "test-client-secret"; | ||
| }); |
There was a problem hiding this comment.
Restore AZURE_B2C env vars after each suite.
These values persist across test files and can cause order-dependent failures. Save originals and restore in afterEach/afterAll (apply the same pattern in the createMediaUser suite).
Suggested fix (pattern)
describe("getGraphApiAccessToken", () => {
+ const originalEnv = {
+ AZURE_B2C_TENANT_ID: process.env.AZURE_B2C_TENANT_ID,
+ AZURE_B2C_CLIENT_ID: process.env.AZURE_B2C_CLIENT_ID,
+ AZURE_B2C_CLIENT_SECRET: process.env.AZURE_B2C_CLIENT_SECRET
+ };
beforeEach(() => {
vi.clearAllMocks();
process.env.AZURE_B2C_TENANT_ID = "test-tenant-id";
process.env.AZURE_B2C_CLIENT_ID = "test-client-id";
process.env.AZURE_B2C_CLIENT_SECRET = "test-client-secret";
});
+ afterEach(() => {
+ for (const [key, value] of Object.entries(originalEnv)) {
+ if (value === undefined) {
+ delete process.env[key];
+ } else {
+ process.env[key] = value;
+ }
+ }
+ });Also applies to: 391-396
Moves env var check before approveApplication() call to prevent approving in the DB when the email link is not configured. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (1)
55-76:⚠️ Potential issue | 🟡 MinorRestore
MEDIA_FORGOT_PASSWORD_LINKafter each test to avoid cross-test leakage.
process.envis mutated in this suite but never restored to its original state. That can make other suites order-dependent.Suggested patch
-import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const ORIGINAL_MEDIA_FORGOT_PASSWORD_LINK = process.env.MEDIA_FORGOT_PASSWORD_LINK; ... beforeEach(() => { vi.clearAllMocks(); ... mockGetGraphApiAccessToken.mockResolvedValue("mock-access-token"); process.env.MEDIA_FORGOT_PASSWORD_LINK = "https://example.com/reset"; }); + + afterEach(() => { + if (ORIGINAL_MEDIA_FORGOT_PASSWORD_LINK === undefined) { + delete process.env.MEDIA_FORGOT_PASSWORD_LINK; + } else { + process.env.MEDIA_FORGOT_PASSWORD_LINK = ORIGINAL_MEDIA_FORGOT_PASSWORD_LINK; + } + });Also applies to: 230-234
🧹 Nitpick comments (2)
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (2)
4-5: Align module-level constant names with project convention.Please rename these constants to SCREAMING_SNAKE_CASE (
mockGetGraphApiAccessToken,mockSendMediaNewAccountEmail,mockSendMediaExistingUserEmail,mockApplication).As per coding guidelines, "Constants should use SCREAMING_SNAKE_CASE (e.g.,
MAX_FILE_SIZE,DEFAULT_TIMEOUT)."Also applies to: 12-13, 43-53
231-241: Remove non-essential AAA comments in the test body.
// Arrange,// Act, and// Asserthere restate obvious structure and add noise.Suggested patch
- // Arrange delete process.env.MEDIA_FORGOT_PASSWORD_LINK; vi.mocked(getApplicationById).mockResolvedValue(mockApplication); mockRequest.body = { confirm: "yes" }; - - // Act const handler = POST[1]; await handler(mockRequest as Request, mockResponse as Response, vi.fn()); - - // Assert expect(approveApplication).not.toHaveBeenCalled();As per coding guidelines, "Do not add comments unless meaningful - explain why, not what."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a551793-1046-4b8e-98de-888e8e4ebabc
📒 Files selected for processing (2)
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/admin-pages/src/pages/media-applications/[id]/approve.ts
- Rename checkUserExists to findUserByEmail, returning user ID or null - Add updateMediaUser to PATCH existing user's name in Azure AD - Update local DB user record (firstName, surname) for existing users - Add subscription/start page links and forgot_password_process_link to existing user email personalisation - Validate MEDIA_FORGOT_PASSWORD_LINK before approving application Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/admin-pages/src/pages/media-applications/[id]/approve.ts (1)
112-130:⚠️ Potential issue | 🟠 MajorCatch-all error handler still shows misleading Azure AD message for unrelated failures.
The outer
catchrendersazureAdFailedfor all errors, butgetApplicationById(line 54 in the POST flow re-fetch scenario) could throw DB errors, andapproveApplicationcan throw"Application has already been reviewed". Neither is an Azure AD issue.Consider differentiating error types by checking the error message or introducing typed errors, as suggested previously.
,
🧹 Nitpick comments (1)
libs/account/src/repository/query.ts (1)
40-42: Falsy values won't be written to database.The conditional spread pattern
...(input.firstName && ...)will skip the field if the value is an empty string"",0, or other falsy values. If clearing a name is a valid use case, this logic should use explicitundefinedcheck instead.- ...(input.firstName && { firstName: input.firstName }), - ...(input.surname && { surname: input.surname }) + ...(input.firstName !== undefined && { firstName: input.firstName }), + ...(input.surname !== undefined && { surname: input.surname })If empty strings should be rejected, the current implementation is acceptable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2213380-c876-4ad4-a295-bfdfd1d4edda
📒 Files selected for processing (11)
libs/account/src/repository/model.tslibs/account/src/repository/query.tslibs/admin-pages/src/media-application/service.test.tslibs/admin-pages/src/media-application/service.tslibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/auth/src/graph-api/client.test.tslibs/auth/src/graph-api/client.tslibs/auth/src/index.tslibs/notification/src/govuk-notify-service.test.tslibs/notification/src/govuk-notify-service.ts
| async function updateLocalMediaUser(azureAdUserId: string, firstName: string, surname: string): Promise<void> { | ||
| try { | ||
| await updateUser(azureAdUserId, { firstName, surname }); | ||
| } catch (error) { | ||
| console.error("Failed to update local media user:", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent error swallowing may hide sync failures.
updateLocalMediaUser catches and logs errors but doesn't propagate them. If the local DB update fails, the Azure AD user will be updated but the local record won't reflect the changes. Consider whether this should throw or at least return a success indicator.
| subscriptionPageLink: process.env.MEDIA_SUBSCRIPTION_PAGE_LINK ?? "", | ||
| startPageLink: process.env.MEDIA_START_PAGE_LINK ?? "" |
There was a problem hiding this comment.
Empty link fallbacks may result in blank links for existing users.
When MEDIA_SUBSCRIPTION_PAGE_LINK or MEDIA_START_PAGE_LINK environment variables are unset, empty strings are passed to sendMediaExistingUserEmail. The template includes these as personalisation fields, potentially rendering blank links.
Consider validating these for the existing user path or logging a warning when they're missing.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
3 participants
Jira link
#346
Change description
B2C Account Creation on Approval
Summary by CodeRabbit