feat(journey-app): delete webauthn devices using device client#549
feat(journey-app): delete webauthn devices using device client#549vatsalparikh wants to merge 2 commits intomainfrom
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
vatsalparikh
requested review from
SteinGabriel,
ancheetah,
cerebrl and
ryanbas21
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed 9165436 to https://ForgeRock.github.io/ping-javascript-sdk/pr-549/9165436a4d36b3c80d12caf00083a3c6ce96d47f branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/device-client - 9.7 KB (+0.5 KB, +5.8%) 🆕 New Packages🆕 @forgerock/journey-client - 87.8 KB (new) ➖ No Changes➖ @forgerock/sdk-logger - 1.6 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (14.77%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #549 +/- ##
==========================================
- Coverage 18.84% 14.77% -4.08%
==========================================
Files 142 153 +11
Lines 27770 26262 -1508
Branches 990 1056 +66
==========================================
- Hits 5232 3879 -1353
+ Misses 22538 22383 -155 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/journey-app/components/webauthn.ts`:
- Around line 21-34: The catch block in handleWebAuthn currently swallows errors
from WebAuthn.authenticate() and WebAuthn.register(), so change handleWebAuthn
(inside webauthnComponent) to return an explicit success/failure signal (e.g., a
boolean or thrown error) instead of always resolving: on success return true, on
any failure either rethrow the caught error or return false (do not just
console.error), and update the caller in main.ts to await handleWebAuthn() and
only call submitForm() when the signal indicates success; reference the
handleWebAuthn function, WebAuthn.authenticate / WebAuthn.register calls,
webAuthnStepType/WebAuthnStepType, webauthnComponent(), and submitForm() when
making these changes.
In `@e2e/journey-app/main.ts`:
- Around line 154-160: The code currently logs the session token and injects it
via journeyEl.innerHTML, creating an injection sink and duplicate id on the
<pre> tag; instead stop logging the session, build the success view using DOM
APIs: create the h2 (completeHeader), span (sessionLabel), pre (sessionToken)
and button (logoutButton) nodes, set the session string into the pre via
textContent (not innerHTML), append them to journeyEl, and ensure only one
id="sessionToken" exists and the logout button gets its event listener as
before.
In `@e2e/journey-app/services/delete-webauthn-devices.ts`:
- Around line 20-35: WEBAUTHN_DEVICES_KEY is global and causes cross-user/realm
leakage; change getStoredDevices() and all store/delete flows (including
deleteDevicesInSession() and deleteAllDevices()) to derive the localStorage key
per-user+realm by calling getStoredDevicesKey(realm, userId) after
getWebAuthnDevicesForCurrentUser() returns its realm and userId, then read/write
that namespaced key instead of the shared WEBAUTHN_DEVICES_KEY so each
user+realm has an isolated baseline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96d920ce-e219-48ef-b591-df282a4d2e73
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
e2e/journey-app/components/webauthn-devices.tse2e/journey-app/components/webauthn.tse2e/journey-app/main.tse2e/journey-app/package.jsone2e/journey-app/server-configs.tse2e/journey-app/services/delete-webauthn-devices.tse2e/journey-app/style.csse2e/journey-app/tsconfig.app.jsone2e/journey-suites/src/recovery-codes.test.tse2e/journey-suites/src/webauthn-devices.test.tspackages/device-client/package.json
💤 Files with no reviewable changes (1)
- e2e/journey-suites/src/recovery-codes.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/journey-app/tsconfig.app.json
- e2e/journey-app/style.css
- e2e/journey-app/components/webauthn-devices.ts
- packages/device-client/package.json
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/journey-app/main.ts (1)
99-103:⚠️ Potential issue | 🟡 MinorError message rendered via
innerHTMLis also an injection risk.Similar to the session token, the error message is interpolated into HTML without sanitization. If the server returns a crafted error message, it could execute scripts.
🛡️ Proposed fix using textContent
if (errorEl) { - errorEl.innerHTML = ` - <pre id="errorMessage">${error}</pre> - `; + const errorPre = document.createElement('pre'); + errorPre.id = 'errorMessage'; + errorPre.textContent = error ?? ''; + errorEl.replaceChildren(errorPre); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/main.ts` around lines 99 - 103, The code assigns unsanitized error text into innerHTML on the errorEl element (see errorEl and errorMessage), creating an XSS risk; change this to create or select a <pre> element and set its textContent (or use createTextNode) instead of using innerHTML so the error string is rendered as plain text; update the block that currently assigns errorEl.innerHTML to instead populate a safe element via textContent and preserve any intended formatting without injecting HTML.
🧹 Nitpick comments (2)
e2e/journey-suites/src/webauthn-devices.test.ts (2)
96-114: Consider wrapping CDP cleanup in try-catch to ensure cleanup completes.If
cdp.send('WebAuthn.removeVirtualAuthenticator', ...)throws,cdp.send('WebAuthn.disable')won't execute. While rare, a failure during cleanup could leave the virtual authenticator attached for subsequent tests.🧹 Proposed defensive cleanup
test.afterEach(async ({ page }) => { await page.unroute('**/*'); try { await deleteDevicesInSession(page); } catch (error) { console.error('Delete failed:', error); } if (!cdp) { return; } - if (authenticatorId) { - await cdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); + try { + if (authenticatorId) { + await cdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); + } + } catch (error) { + console.error('Failed to remove virtual authenticator:', error); } - await cdp.send('WebAuthn.disable'); + try { + await cdp.send('WebAuthn.disable'); + } catch (error) { + console.error('Failed to disable WebAuthn:', error); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-suites/src/webauthn-devices.test.ts` around lines 96 - 114, Wrap the CDP cleanup calls in test.afterEach so removal errors don't prevent disabling: when cdp and authenticatorId are present, call cdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }) inside a try-catch (log the error) and always call cdp.send('WebAuthn.disable') in a finally or subsequent safe block so disable runs even if removeVirtualAuthenticator throws; reference the existing test.afterEach, authenticatorId, and cdp.send(...) calls to locate where to add the try/catch/finally.
20-36: Add TypeScript type annotations for helper function parameters.The
pageparameter lacks type annotations inlogin,logout,getDevicesBeforeSession,deleteDevicesInSession, andcompleteAuthenticationJourney. Adding thePagetype improves IDE support and catches type errors.✨ Proposed fix
+import type { Page, CDPSession } from '@playwright/test'; + - async function login(page, journey = 'Login'): Promise<void> { + async function login(page: Page, journey = 'Login'): Promise<void> {Apply similarly to
logout,getDevicesBeforeSession,deleteDevicesInSession,completeAuthenticationJourney, andcompleteRegistrationJourney.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-suites/src/webauthn-devices.test.ts` around lines 20 - 36, Add explicit TypeScript Page parameter types to all helper functions that accept a Playwright page: update function signatures for login, logout, getDevicesBeforeSession, deleteDevicesInSession, completeAuthenticationJourney, and completeRegistrationJourney to type the first parameter as Page (importing Page from '@playwright/test' or 'playwright' as used in the test suite) so IDE/typechecker can validate usages; ensure the import for Page is added or consolidated at the top of the file if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/journey-app/main.ts`:
- Around line 126-134: The WebAuthn branch currently calls submitForm()
unconditionally after webauthnComponent(...), which can submit despite internal
WebAuthn failures; change the flow in the WebAuthn.getWebAuthnStepType /
isWebAuthn branch to await a success result from webauthnComponent(journeyEl,
step, 0) (or have it return a boolean or throw on failure), then only call
submitForm() when that result indicates success, and otherwise abort/return and
surface or handle the error (e.g., show an error UI or log) so submitForm() is
not invoked on WebAuthn failure.
In `@e2e/journey-app/services/delete-webauthn-devices.ts`:
- Around line 28-31: Wrap the JSON.parse call for retrievedDevices in a
try/catch to guard against malformed JSON, so parse errors are caught and a
clear, controlled error is thrown (or handled) before the Array.isArray check;
specifically, update the logic around the parsedDevices assignment in
delete-webauthn-devices.ts to try parsing retrievedDevices, catch any exception
(referencing retrievedDevices and parsedDevices), and rethrow or return a
descriptive error like "Invalid JSON in localStorage for WebAuthn devices"
including the caught error message, then proceed to the existing Array.isArray
validation.
- Around line 43-47: getRealmFromWellknown currently returns only the final
realm segment which breaks nested realm URLs; update it to return the full
nested realm path (e.g. "root/realms/alpha") instead of just "alpha" so API
calls like `/json/realms/${realm}/users/...` produce correct URLs. Replace the
regex that captures a single segment with one that captures the entire path
between the first `/realms/` and `/.well-known` (or simply call and return the
utility getRealmUrlPath() from packages/sdk-utilities), and ensure any consumers
of getRealmFromWellknown keep using the returned string directly for
constructing `/json/realms/${realm}/...` endpoints.
---
Outside diff comments:
In `@e2e/journey-app/main.ts`:
- Around line 99-103: The code assigns unsanitized error text into innerHTML on
the errorEl element (see errorEl and errorMessage), creating an XSS risk; change
this to create or select a <pre> element and set its textContent (or use
createTextNode) instead of using innerHTML so the error string is rendered as
plain text; update the block that currently assigns errorEl.innerHTML to instead
populate a safe element via textContent and preserve any intended formatting
without injecting HTML.
---
Nitpick comments:
In `@e2e/journey-suites/src/webauthn-devices.test.ts`:
- Around line 96-114: Wrap the CDP cleanup calls in test.afterEach so removal
errors don't prevent disabling: when cdp and authenticatorId are present, call
cdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }) inside a
try-catch (log the error) and always call cdp.send('WebAuthn.disable') in a
finally or subsequent safe block so disable runs even if
removeVirtualAuthenticator throws; reference the existing test.afterEach,
authenticatorId, and cdp.send(...) calls to locate where to add the
try/catch/finally.
- Around line 20-36: Add explicit TypeScript Page parameter types to all helper
functions that accept a Playwright page: update function signatures for login,
logout, getDevicesBeforeSession, deleteDevicesInSession,
completeAuthenticationJourney, and completeRegistrationJourney to type the first
parameter as Page (importing Page from '@playwright/test' or 'playwright' as
used in the test suite) so IDE/typechecker can validate usages; ensure the
import for Page is added or consolidated at the top of the file if missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75d15fa2-4e74-46c3-97e5-e1e1b622ca9c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
e2e/journey-app/components/webauthn-devices.tse2e/journey-app/components/webauthn.tse2e/journey-app/main.tse2e/journey-app/package.jsone2e/journey-app/server-configs.tse2e/journey-app/services/delete-webauthn-devices.tse2e/journey-app/style.csse2e/journey-app/tsconfig.app.jsone2e/journey-suites/src/recovery-codes.test.tse2e/journey-suites/src/webauthn-devices.test.tspackages/device-client/package.json
💤 Files with no reviewable changes (1)
- e2e/journey-suites/src/recovery-codes.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/journey-app/server-configs.ts
- packages/device-client/package.json
- e2e/journey-app/package.json
- e2e/journey-app/components/webauthn-devices.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/journey-app/services/delete-webauthn-devices.ts (1)
28-42:⚠️ Potential issue | 🟡 MinorWrap
JSON.parsein try/catch to handle corrupted localStorage.If localStorage contains malformed JSON,
JSON.parsethrows before the array validation. This could crash the device management flow.🛡️ Proposed fix
function getStoredDevices(): Set<string> | null { const retrievedDevices = window.localStorage.getItem(WEBAUTHN_DEVICES_KEY); if (!retrievedDevices) { return null; } - const parsedDevices = JSON.parse(retrievedDevices) as unknown; - if (!Array.isArray(parsedDevices)) { - throw new Error('Invalid data in localStorage'); + let parsedDevices: unknown; + try { + parsedDevices = JSON.parse(retrievedDevices); + } catch { + window.localStorage.removeItem(WEBAUTHN_DEVICES_KEY); + return null; } + if (!Array.isArray(parsedDevices)) { + window.localStorage.removeItem(WEBAUTHN_DEVICES_KEY); + return null; + } return new Set(,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/services/delete-webauthn-devices.ts` around lines 28 - 42, getStoredDevices currently calls JSON.parse directly and can throw on malformed localStorage; wrap the JSON.parse(...) call in a try/catch inside getStoredDevices to handle corrupted JSON gracefully (catch the error, optionally remove the bad key via window.localStorage.removeItem(WEBAUTHN_DEVICES_KEY) or just return null) and then proceed with the existing Array.isArray(parsedDevices) validation; keep throwing only for non-array shapes but do not let JSON.parse exceptions bubble up and crash the flow.
🧹 Nitpick comments (3)
e2e/journey-app/services/delete-webauthn-devices.ts (1)
180-217: Config mutation has side effects.Line 186 mutates the passed
configobject (config.redirectUri = getRedirectUri()). Sinceconfigcomes from the sharedserverConfigsobject inserver-configs.ts, this mutation persists across calls.For the current E2E test usage this is likely fine, but consider cloning the config or passing redirectUri separately to avoid surprising callers.
♻️ Suggested approach
async function getWebAuthnDevicesForCurrentUser(config: OidcConfig): Promise<{ ... }> { - config.redirectUri = getRedirectUri(); - const oidcClient = await oidc({ config }); + const oidcConfig = { ...config, redirectUri: getRedirectUri() }; + const oidcClient = await oidc({ config: oidcConfig });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/services/delete-webauthn-devices.ts` around lines 180 - 217, The function getWebAuthnDevicesForCurrentUser currently mutates the passed config by assigning config.redirectUri = getRedirectUri(), which causes side effects on the shared serverConfigs; instead, avoid mutating the input by creating a shallow copy or new config object (e.g., const localConfig = { ...config, redirectUri: getRedirectUri() }) and use that localConfig when calling oidc({ config }) and any subsequent calls, or pass redirectUri as a separate argument into oidc/getOidcTokens; update all references in getWebAuthnDevicesForCurrentUser (oidc, getOidcTokens, etc.) to use the non-mutated localConfig.e2e/journey-suites/src/webauthn-devices.test.ts (1)
20-36: Add explicitPagetype annotations for helper functions.The helper functions use implicit
anyfor thepageparameter. Adding explicit types improves maintainability and IDE support.♻️ Suggested fix
+import type { Page, CDPSession } from '@playwright/test'; -import type { CDPSession } from '@playwright/test'; - async function login(page, journey = 'Login'): Promise<void> { + async function login(page: Page, journey = 'Login'): Promise<void> { - async function logout(page): Promise<void> { + async function logout(page: Page): Promise<void> {Apply similarly to
getDevicesBeforeSession,deleteDevicesInSession,completeAuthenticationJourney, andcompleteRegistrationJourney.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-suites/src/webauthn-devices.test.ts` around lines 20 - 36, The helper functions currently accept an untyped page parameter (implicit any); add an explicit Playwright Page type to each function signature (e.g., change login(page) to login(page: Page)) and import the Page type from '@playwright/test' at the top of the file; apply the same change to logout, getDevicesBeforeSession, deleteDevicesInSession, completeAuthenticationJourney, and completeRegistrationJourney so IDEs and the typechecker have proper types for page.e2e/journey-app/server-configs.ts (1)
15-34: EmptyredirectUrirelies on runtime mutation.The
redirectUri: ''placeholders satisfy theOidcConfigtype but would cause OIDC failures if passed directly to token endpoints. The current code works becausedelete-webauthn-devices.ts(line 186) mutatesconfig.redirectUri = getRedirectUri()before use.Consider either:
- Setting a valid default (e.g.,
window.location.origin) here, or- Documenting that callers must set
redirectUribefore OIDC operationsThis is acceptable for a test app, but the implicit contract is fragile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/server-configs.ts` around lines 15 - 34, serverConfigs currently sets redirectUri to an empty string on each OidcConfig which relies on a runtime mutation (config.redirectUri = getRedirectUri()) and is fragile; update the serverConfigs object so each entry sets a sensible default redirectUri (e.g., window.location.origin or the existing getRedirectUri() helper) instead of '' OR add a clear comment/docstring on serverConfigs and OidcConfig indicating callers must set redirectUri before any OIDC calls; focus your change on the serverConfigs constant and reference symbols: serverConfigs, OidcConfig, redirectUri, and getRedirectUri.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/journey-app/services/delete-webauthn-devices.ts`:
- Around line 63-67: The getRealmPathFromWellknown function currently only
captures the last realm segment; update it to extract the entire path between
the first "/realms/" and "/.well-known/" so nested realms are preserved (e.g.,
"root/realms/alpha"). Replace the existing regex with one that uses a non-greedy
capture from the first "/realms/" to "/.well-known/" (or compute startIndex =
pathname.indexOf('/realms/') and endIndex = pathname.indexOf('/.well-known/')
and substring between them) and return that capture (match[1]) or 'root' if not
found; ensure you update getRealmPathFromWellknown to return the full nested
realm path used by getWebAuthnDevices, updateWebAuthnDeviceName, and
deleteWebAuthnDeviceName.
---
Duplicate comments:
In `@e2e/journey-app/services/delete-webauthn-devices.ts`:
- Around line 28-42: getStoredDevices currently calls JSON.parse directly and
can throw on malformed localStorage; wrap the JSON.parse(...) call in a
try/catch inside getStoredDevices to handle corrupted JSON gracefully (catch the
error, optionally remove the bad key via
window.localStorage.removeItem(WEBAUTHN_DEVICES_KEY) or just return null) and
then proceed with the existing Array.isArray(parsedDevices) validation; keep
throwing only for non-array shapes but do not let JSON.parse exceptions bubble
up and crash the flow.
---
Nitpick comments:
In `@e2e/journey-app/server-configs.ts`:
- Around line 15-34: serverConfigs currently sets redirectUri to an empty string
on each OidcConfig which relies on a runtime mutation (config.redirectUri =
getRedirectUri()) and is fragile; update the serverConfigs object so each entry
sets a sensible default redirectUri (e.g., window.location.origin or the
existing getRedirectUri() helper) instead of '' OR add a clear comment/docstring
on serverConfigs and OidcConfig indicating callers must set redirectUri before
any OIDC calls; focus your change on the serverConfigs constant and reference
symbols: serverConfigs, OidcConfig, redirectUri, and getRedirectUri.
In `@e2e/journey-app/services/delete-webauthn-devices.ts`:
- Around line 180-217: The function getWebAuthnDevicesForCurrentUser currently
mutates the passed config by assigning config.redirectUri = getRedirectUri(),
which causes side effects on the shared serverConfigs; instead, avoid mutating
the input by creating a shallow copy or new config object (e.g., const
localConfig = { ...config, redirectUri: getRedirectUri() }) and use that
localConfig when calling oidc({ config }) and any subsequent calls, or pass
redirectUri as a separate argument into oidc/getOidcTokens; update all
references in getWebAuthnDevicesForCurrentUser (oidc, getOidcTokens, etc.) to
use the non-mutated localConfig.
In `@e2e/journey-suites/src/webauthn-devices.test.ts`:
- Around line 20-36: The helper functions currently accept an untyped page
parameter (implicit any); add an explicit Playwright Page type to each function
signature (e.g., change login(page) to login(page: Page)) and import the Page
type from '@playwright/test' at the top of the file; apply the same change to
logout, getDevicesBeforeSession, deleteDevicesInSession,
completeAuthenticationJourney, and completeRegistrationJourney so IDEs and the
typechecker have proper types for page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7148b14d-1f6c-49ea-bf54-13553a777275
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
e2e/journey-app/components/webauthn-devices.tse2e/journey-app/components/webauthn.tse2e/journey-app/main.tse2e/journey-app/package.jsone2e/journey-app/server-configs.tse2e/journey-app/services/delete-webauthn-devices.tse2e/journey-app/style.csse2e/journey-app/tsconfig.app.jsone2e/journey-suites/src/recovery-codes.test.tse2e/journey-suites/src/webauthn-devices.test.tspackages/device-client/package.json
💤 Files with no reviewable changes (1)
- e2e/journey-suites/src/recovery-codes.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/journey-app/components/webauthn-devices.ts
- e2e/journey-app/tsconfig.app.json
- e2e/journey-app/package.json
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We updated the four failing e2e tests to reflect the PR's intentional removal of the Complete heading from the journey app's completion screen. The getByText('Complete') assertion is replaced with getByRole('button', { name: 'Logout' }), which is the stable element present in the new completion screen. In no-session.test.ts, the console-log-based session token assertion is replaced with a DOM check on #sessionToken, since the PR now renders the token directly in the element rather than logging it.
Tip
✅ We verified this fix by re-running @forgerock/journey-suites:e2e-ci--src/registration.test.ts.
Suggested Fix changes
diff --git a/e2e/journey-suites/src/login.test.ts b/e2e/journey-suites/src/login.test.ts
index 796381ea0..403dc6a77 100644
--- a/e2e/journey-suites/src/login.test.ts
+++ b/e2e/journey-suites/src/login.test.ts
@@ -26,7 +26,7 @@ test('Test happy paths on test page', async ({ page }) => {
await page.getByLabel('Password').fill(password);
await clickButton('Submit', '/authenticate');
- await expect(page.getByText('Complete')).toBeVisible();
+ await expect(page.getByRole('button', { name: 'Logout' })).toBeVisible();
// Perform logout
await clickButton('Logout', '/authenticate');
diff --git a/e2e/journey-suites/src/no-session.test.ts b/e2e/journey-suites/src/no-session.test.ts
index 679e6e9cb..b17c1241a 100644
--- a/e2e/journey-suites/src/no-session.test.ts
+++ b/e2e/journey-suites/src/no-session.test.ts
@@ -28,8 +28,8 @@ test('Test happy paths on test page', async ({ page }) => {
await page.getByLabel('Password').fill(password);
await clickButton('Submit', '/authenticate');
- await expect(page.getByText('Complete')).toBeVisible();
+ await expect(page.getByRole('button', { name: 'Logout' })).toBeVisible();
// Test assertions
- await expect(messageArray.includes('Session Token: none')).toBe(true);
+ await expect(page.locator('#sessionToken')).toHaveText('none');
});
diff --git a/e2e/journey-suites/src/qr-code.test.ts b/e2e/journey-suites/src/qr-code.test.ts
index b18ebd2d4..54bd9b2e3 100644
--- a/e2e/journey-suites/src/qr-code.test.ts
+++ b/e2e/journey-suites/src/qr-code.test.ts
@@ -41,7 +41,7 @@ test('Test QR Code journey flow using QRCode module', async ({ page }) => {
await clickButton('Submit', '/authenticate');
- await expect(page.getByText('Complete')).toBeVisible();
+ await expect(page.getByRole('button', { name: 'Logout' })).toBeVisible();
await clickButton('Logout', '/sessions');
diff --git a/e2e/journey-suites/src/registration.test.ts b/e2e/journey-suites/src/registration.test.ts
index 6bf6725ad..c6ffeff6a 100644
--- a/e2e/journey-suites/src/registration.test.ts
+++ b/e2e/journey-suites/src/registration.test.ts
@@ -58,7 +58,7 @@ test('Test happy paths on test page', async ({ page }) => {
await clickButton('Submit', '/authenticate');
- await expect(page.getByText('Complete')).toBeVisible();
+ await expect(page.getByRole('button', { name: 'Logout' })).toBeVisible();
// Perform logout
await clickButton('Logout', '/authenticate');
Or Apply changes locally with:
npx nx-cloud apply-locally wLil-kCVU
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
♻️ Duplicate comments (3)
e2e/journey-app/services/delete-webauthn-devices.ts (3)
20-42:⚠️ Potential issue | 🟠 MajorNamespace the stored device baseline by user and realm.
WEBAUTHN_DEVICES_KEYis shared across all logins in the browser. A snapshot captured for one user/tenant is then reused bydeleteDevicesInSession(), anddeleteAllDevices()clears that same shared key. This makes the "preserve pre-existing devices" behavior unsafe when the app switches users orclientIds in the same browser.Consider deriving a namespaced key after obtaining the user/realm:
const WEBAUTHN_DEVICES_KEY_PREFIX = 'journey-app:webauthn-device-uuids'; function getStoredDevicesKey(realm: string, userId: string): string { return `${WEBAUTHN_DEVICES_KEY_PREFIX}:${realm}:${userId}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/services/delete-webauthn-devices.ts` around lines 20 - 42, The stored devices key is global and must be namespaced per user/realm; change WEBAUTHN_DEVICES_KEY to a prefix constant (WEBAUTHN_DEVICES_KEY_PREFIX) and add a helper getStoredDevicesKey(realm: string, userId: string) that returns `${WEBAUTHN_DEVICES_KEY_PREFIX}:${realm}:${userId}`; update getStoredDevices to accept realm and userId and use getStoredDevicesKey(...) when calling localStorage.getItem, and update all callers (deleteDevicesInSession, deleteAllDevices, and any others) to pass realm and userId so each user's devices are isolated in localStorage.
63-67:⚠️ Potential issue | 🔴 CriticalThe regex captures only the final realm segment, breaking nested realm support.
For nested realm URLs like
https://example.com/am/oauth2/realms/root/realms/alpha/.well-known/openid-configuration, the regex returns"alpha"instead of"root/realms/alpha". The device-client WebAuthn endpoints construct/json/realms/${realm}/users/..., producing incorrect URLs for nested realms.Extract the full path between the first
/realms/and/.well-known/to properly support nested realm hierarchies.🔧 Suggested fix
function getRealmPathFromWellknown(wellknown: string): string { const pathname = new URL(wellknown).pathname; - const match = pathname.match(/\/realms\/([^/]+)\/\.well-known\/openid-configuration\/?$/); - return match?.[1] ?? 'root'; + const realmsIndex = pathname.indexOf('/realms/'); + const wellknownIndex = pathname.indexOf('/.well-known/'); + if (realmsIndex === -1 || wellknownIndex === -1 || realmsIndex >= wellknownIndex) { + return 'root'; + } + // Extract everything between first /realms/ and /.well-known/, e.g., "root/realms/alpha" + return pathname.substring(realmsIndex + '/realms/'.length, wellknownIndex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/services/delete-webauthn-devices.ts` around lines 63 - 67, The getRealmPathFromWellknown function currently only captures the final realm segment; update its extraction to grab the entire path between the first "/realms/" and "/.well-known/openid-configuration" so nested realms are preserved (e.g., "root/realms/alpha"). Locate getRealmPathFromWellknown and replace the current regex-based extraction with one that captures everything after the first "/realms/" up to "/.well-known/" (for example using a regex like /\/realms\/(.+?)\/\.well-known\/openid-configuration\/?$/ or by finding the first "/realms/" index and slicing to the start of "/.well-known/"), and keep the existing fallback to 'root' when no match is found.
34-37:⚠️ Potential issue | 🟡 Minor
JSON.parsecan throw on malformed localStorage data.If the stored value is corrupted or not valid JSON,
JSON.parsewill throw an unhandled exception before the array validation check.🛡️ Proposed fix to handle parse errors
const retrievedDevices = window.localStorage.getItem(WEBAUTHN_DEVICES_KEY); if (!retrievedDevices) { return null; } - const parsedDevices = JSON.parse(retrievedDevices) as unknown; - if (!Array.isArray(parsedDevices)) { - throw new Error('Invalid data in localStorage'); + let parsedDevices: unknown; + try { + parsedDevices = JSON.parse(retrievedDevices); + } catch { + window.localStorage.removeItem(WEBAUTHN_DEVICES_KEY); + return null; } + if (!Array.isArray(parsedDevices)) { + window.localStorage.removeItem(WEBAUTHN_DEVICES_KEY); + return null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/services/delete-webauthn-devices.ts` around lines 34 - 37, The code calls JSON.parse(retrievedDevices) which can throw for malformed JSON; wrap the parse in a try/catch around the parsedDevices creation (using the retrievedDevices identifier) and on parse error either throw a new Error with a clear message including the original error/invalid value or handle it safely (e.g. treat as empty array) before the Array.isArray check; ensure you still validate parsedDevices with Array.isArray after a successful parse so the existing error path ('Invalid data in localStorage') remains reachable.
🧹 Nitpick comments (2)
e2e/journey-app/services/delete-webauthn-devices.ts (1)
186-186: Mutating the passedconfigobject may cause side effects.Line 186 directly mutates
config.redirectUri, which could affect callers if the same config instance is reused. Consider creating a shallow copy instead.🔧 Suggested fix
async function getWebAuthnDevicesForCurrentUser(config: OidcConfig): Promise<{ userId: string; realm: string; webAuthnClient: DeviceClient; devices: WebAuthnDevice[]; }> { - config.redirectUri = getRedirectUri(); - const oidcClient = await oidc({ config }); + const oidcConfig = { ...config, redirectUri: getRedirectUri() }; + const oidcClient = await oidc({ config: oidcConfig });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/services/delete-webauthn-devices.ts` at line 186, The code mutates the incoming config object by assigning config.redirectUri = getRedirectUri(), which can cause caller-side effects; instead create a shallow copy and set redirectUri on that copy (e.g., use object spread or Object.assign) and use the new copy for subsequent operations so the original config remains unchanged; refer to the local variable config and the function getRedirectUri() when making this change.e2e/journey-suites/src/webauthn-devices.test.ts (1)
20-36: Add type annotations to function parameters for type safety.The helper functions
login,logout, and others use untypedpageparameters. Adding explicit types improves maintainability and catches errors at compile time.🔧 Suggested improvement
+import type { Page } from '@playwright/test'; + - async function login(page, journey = 'Login'): Promise<void> { + async function login(page: Page, journey = 'Login'): Promise<void> { const { clickButton, navigate } = asyncEvents(page); // ... } - async function logout(page): Promise<void> { + async function logout(page: Page): Promise<void> { const { clickButton } = asyncEvents(page); // ... }Apply similarly to
getDevicesBeforeSession,deleteDevicesInSession,completeAuthenticationJourney, andcompleteRegistrationJourney.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-suites/src/webauthn-devices.test.ts` around lines 20 - 36, The functions login and logout (and helpers getDevicesBeforeSession, deleteDevicesInSession, completeAuthenticationJourney, completeRegistrationJourney) accept an untyped page parameter—add explicit Playwright types to improve type safety: import Page from '@playwright/test' (or use the Project's Page type) and annotate each function signature with page: Page (keeping existing Promise<void> return types), and update any usages/parameters accordingly so the compiler can validate page methods like getByLabel/getByRole and asyncEvents(page).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/journey-app/services/delete-webauthn-devices.ts`:
- Around line 20-42: The stored devices key is global and must be namespaced per
user/realm; change WEBAUTHN_DEVICES_KEY to a prefix constant
(WEBAUTHN_DEVICES_KEY_PREFIX) and add a helper getStoredDevicesKey(realm:
string, userId: string) that returns
`${WEBAUTHN_DEVICES_KEY_PREFIX}:${realm}:${userId}`; update getStoredDevices to
accept realm and userId and use getStoredDevicesKey(...) when calling
localStorage.getItem, and update all callers (deleteDevicesInSession,
deleteAllDevices, and any others) to pass realm and userId so each user's
devices are isolated in localStorage.
- Around line 63-67: The getRealmPathFromWellknown function currently only
captures the final realm segment; update its extraction to grab the entire path
between the first "/realms/" and "/.well-known/openid-configuration" so nested
realms are preserved (e.g., "root/realms/alpha"). Locate
getRealmPathFromWellknown and replace the current regex-based extraction with
one that captures everything after the first "/realms/" up to "/.well-known/"
(for example using a regex like
/\/realms\/(.+?)\/\.well-known\/openid-configuration\/?$/ or by finding the
first "/realms/" index and slicing to the start of "/.well-known/"), and keep
the existing fallback to 'root' when no match is found.
- Around line 34-37: The code calls JSON.parse(retrievedDevices) which can throw
for malformed JSON; wrap the parse in a try/catch around the parsedDevices
creation (using the retrievedDevices identifier) and on parse error either throw
a new Error with a clear message including the original error/invalid value or
handle it safely (e.g. treat as empty array) before the Array.isArray check;
ensure you still validate parsedDevices with Array.isArray after a successful
parse so the existing error path ('Invalid data in localStorage') remains
reachable.
---
Nitpick comments:
In `@e2e/journey-app/services/delete-webauthn-devices.ts`:
- Line 186: The code mutates the incoming config object by assigning
config.redirectUri = getRedirectUri(), which can cause caller-side effects;
instead create a shallow copy and set redirectUri on that copy (e.g., use object
spread or Object.assign) and use the new copy for subsequent operations so the
original config remains unchanged; refer to the local variable config and the
function getRedirectUri() when making this change.
In `@e2e/journey-suites/src/webauthn-devices.test.ts`:
- Around line 20-36: The functions login and logout (and helpers
getDevicesBeforeSession, deleteDevicesInSession, completeAuthenticationJourney,
completeRegistrationJourney) accept an untyped page parameter—add explicit
Playwright types to improve type safety: import Page from '@playwright/test' (or
use the Project's Page type) and annotate each function signature with page:
Page (keeping existing Promise<void> return types), and update any
usages/parameters accordingly so the compiler can validate page methods like
getByLabel/getByRole and asyncEvents(page).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed16181c-5bc2-4c1a-8ef2-e973288bfd6e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
e2e/journey-app/components/webauthn-devices.tse2e/journey-app/components/webauthn.tse2e/journey-app/main.tse2e/journey-app/package.jsone2e/journey-app/server-configs.tse2e/journey-app/services/delete-webauthn-devices.tse2e/journey-app/style.csse2e/journey-app/tsconfig.app.jsone2e/journey-suites/src/recovery-codes.test.tse2e/journey-suites/src/webauthn-devices.test.tspackages/device-client/package.json
💤 Files with no reviewable changes (1)
- e2e/journey-suites/src/recovery-codes.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/journey-app/style.css
- e2e/journey-app/components/webauthn-devices.ts
vatsalparikh
force-pushed
the
sdks-4504
branch
2 times, most recently
from
b624f06 to
d25db7c
Compare
There was a problem hiding this comment.
Some comments, I think it maybe helpful to do a live review also to walk through this.
I'd also like to talk through creating a pattern for this. We want to create something that everyone can see and implement.
If we have to test webauth, we have a clear pattern for how to register, authenticate, delete the test data from the tenant encapsulated into our flow.
What do you feel the pattern is at this point?
| }); | ||
|
|
||
| test.afterEach(async ({ page }) => { | ||
| await page.unroute('**/*'); |
There was a problem hiding this comment.
Why do we need this call?
| * @param config OIDC configuration used to retrieve the current user's devices. | ||
| * @returns A human-readable status message for UI display. | ||
| */ | ||
| export async function storeDevicesBeforeSession(config: OidcConfig): Promise<string> { |
There was a problem hiding this comment.
Why do you feel we should use local storage? My concern is that we are using the app instance to store test data, which isn't harmful but i'm not sure we need it.
Technically, don't we have the uuid's that the test creates, could we send it via a query parameter and then call the removal from the app with the parameter?
I don't think we will ever really need to remove more than one device, so technically my thought process is
- test go to the login page
- login with username password
- register virtual authenticaor
- virtual authenticator is encapsulated in the test at this point
- we complete the journey
- go to page with button, append a query param called something like deviceId
- we click the button, which the function grabs the device id from the url.
- we verify the device deletion worked via the device-client
- we remove the virtual authenticator once successful in the test file.
Do you think the local storage solution is better? I'm open to feedback, my concern is local storage really only survives the test I think, so it feels like some added complexity since each browser window is unique in playwright.
| * @returns A human-readable status message for UI display. | ||
| * @throws When the delete endpoint returns an error shape. | ||
| */ | ||
| export async function deleteAllDevices(config: OidcConfig): Promise<string> { |
There was a problem hiding this comment.
What scenario are you imagining needing to delete multiple devices?
| await login(page, 'TEST_WebAuthn-Registration'); | ||
| } | ||
|
|
||
| test('should register multiple devices, authenticate and delete devices', async ({ page }) => { |
There was a problem hiding this comment.
Similar question to one on another file, what scenario would this test be validating? Do we need to delete multiple devices? I think we just care that if we create a device in the browser and save it to the AM tenant, that we can and do remove it.
| await logout(page); | ||
| }); | ||
|
|
||
| test('should delete all registered devices', async ({ page }) => { |
There was a problem hiding this comment.
similar to above I think.
| let cdp: CDPSession | undefined; | ||
| let authenticatorId: string | undefined; | ||
|
|
||
| async function login(page: Page, journey = 'Login'): Promise<void> { |
There was a problem hiding this comment.
Not a blocker, but maybe we can consider using test.step to indicate the different steps in this test, rather than hoisting functions?
Perhaps a personal taste, but I don't mind repeativeness / verboseness in tests where we have to do a lot of actions. that way each test is very self contained and articulate what it's doing in the encapsulated block.
This kind of does the same thing, but we lose some of the expressiveness when running the tests.
I'd ask you to maybe look into test.step and then see if you prefer it or not, and let me know which way you feel strongest about. This isn't a blocker or a requirement to the code, I just want to gauge opinion on a different approach.
3 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4504
Description
This PR adds functionality to journey-app to be able to delete devices during a CI run. A playwright test is added to demo how devices can be retrieved and deleted. The functionality supports the ability to preserve devices not part of the current CI run.
For better context:
getanddeletemethod calls to device-client expect a uuid of the user, and so the uuid is first retrieved using the OIDC flow from oidc-client and then device-client methods can be called.To summarize, I created three buttons, get, delete for session, and delete all. I am calling these buttons from the playwright test. I am also using local storage for storing devices registered before the current session. Because get devices needs to be called before webauthn flow so we can capture webauthn registrations, I am doing a normal login flow to retrieve existing registered devices, then doing the webauthn flow to register, authenticate, then delete devices from the current session.
Summary by CodeRabbit
New Features
Tests
Style