test: add-virtual-authenticator-tests #467
Conversation
|
WalkthroughAdds WebAuthn support across the e2e journey app: new Zed tasks, a WebAuthn UI component, main render flow integration, TypeScript/project reference updates, new server configs, and three Playwright WebAuthn tests with virtual authenticators. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI
participant Main as journey main.ts
participant WebComp as webauthnComponent
participant WebAuthnAPI as WebAuthn (sdk)
rect rgb(235,245,255)
Main->>UI: renderForm(step)
UI->>Main: step is WebAuthn
end
Main->>WebComp: webauthnComponent(journeyEl, step)
activate WebComp
WebComp->>WebAuthnAPI: detect step type
alt Authentication
WebComp->>WebAuthnAPI: WebAuthn.authenticate(step)
WebAuthnAPI-->>WebComp: credential / result
else Registration
WebComp->>WebAuthnAPI: WebAuthn.register(step)
WebAuthnAPI-->>WebComp: credential / result
else Other
WebComp-->>Main: undefined
end
WebComp-->>Main: Promise(result)
deactivate WebComp
Main->>Main: advanceJourney(result) / render callbacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
View your CI Pipeline Execution ↗ for commit 576b1e8
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
just reordered imports...LSP did it, I can revert it out though
| }, | ||
| realmPath: '/alpha', | ||
| }, | ||
| ['TEST_WebAuthn-Registration']: { |
There was a problem hiding this comment.
We can think about how to actually do this if the objects the same every time.
There was a problem hiding this comment.
Yeah, I wasn't sure exactly how we wanted to handle different configs. I ended up not using it at all in all the tests I moved over from the legacy repo.
| @@ -0,0 +1,90 @@ | |||
| // Project tasks configuration. See https://zed.dev/docs/tasks for documentation. | |||
There was a problem hiding this comment.
these are my zed tasks, would be nice to keep cause i dont have to rercreate them every time, should be able to use in vscode pretty closely if you'd like
There was a problem hiding this comment.
I don't think we'd want to commit this. We can ignore this, but also not remove it when we do a clean. The same as we do with our .env files and such.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We identified that the forEach callback was incorrectly changed to async in the renderForm function. This change broke the basic authentication flow because async forEach callbacks don't properly await or handle asynchronous operations. We've reverted the callbacks.forEach arrow function back to its synchronous form to restore the expected behavior.
We could not verify this fix.
diff --git a/e2e/journey-app/main.ts b/e2e/journey-app/main.ts
index e497087..afb6cd8 100644
--- a/e2e/journey-app/main.ts
+++ b/e2e/journey-app/main.ts
@@ -129,7 +129,7 @@ const requestMiddleware: RequestMiddleware[] = [
}
const callbacks = step.callbacks;
- callbacks.forEach(async (callback, idx) => {
+ callbacks.forEach((callback, idx) => {
if (callback.getType() === 'NameCallback') {
const cb = callback as NameCallback;
textComponent(
Or Apply changes locally with:
npx nx-cloud apply-locally lBpJ-lcml
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 To learn more about Self Healing CI, please visit nx.dev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/journey-client/src/lib/journey.store.ts (1)
22-22: Consider usingimport typefor consistency.
NextOptions,ResumeOptions, andStartParamare only used as type annotations (lines 48, 60, 87, 153) and never as runtime values. For consistency with the other type imports on lines 21 and 23, consider usingimport type.Apply this diff:
-import { NextOptions, ResumeOptions, StartParam } from './interfaces.js'; +import type { NextOptions, ResumeOptions, StartParam } from './interfaces.js';e2e/journey-app/server-configs.ts (1)
16-45: Consider reducing configuration duplication.All five WebAuthn test configurations share identical
baseUrlandrealmPathvalues. While explicit entries provide clarity, you could reduce duplication by extracting common configuration:+const defaultConfig = { + serverConfig: { + baseUrl: 'https://openam-sdks.forgeblocks.com/am/', + }, + realmPath: '/alpha', +}; + export const serverConfigs: Record<string, JourneyClientConfig> = { - basic: { - serverConfig: { - baseUrl: 'https://openam-sdks.forgeblocks.com/am/', - }, - realmPath: '/alpha', - }, + basic: defaultConfig, - ['TEST_WebAuthn-Registration']: { - serverConfig: { - baseUrl: 'https://openam-sdks.forgeblocks.com/am/', - }, - realmPath: '/alpha', - }, + ['TEST_WebAuthn-Registration']: defaultConfig, // ... apply to all entries };e2e/journey-app/components/webauthn.ts (1)
23-23: Simplify redundant Promise.resolve.Line 23 explicitly returns
Promise.resolve(undefined), but sincehandleWebAuthnis already an async function, you can simply returnundefinedor omit the return statement for unsupported step types.Apply this diff:
- return Promise.resolve(undefined); + return undefined;Or simply:
} else { - return Promise.resolve(undefined); + // Unsupported step type }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.zed/tasks.json(1 hunks)e2e/journey-app/components/webauthn.ts(1 hunks)e2e/journey-app/main.ts(4 hunks)e2e/journey-app/package.json(1 hunks)e2e/journey-app/server-configs.ts(1 hunks)e2e/journey-app/tsconfig.app.json(1 hunks)e2e/journey-app/tsconfig.json(1 hunks)e2e/journey-suites/playwright.config.ts(1 hunks)e2e/journey-suites/src/webauthn-username-password.test.ts(1 hunks)e2e/journey-suites/src/webauthn-usernameless.test.ts(1 hunks)e2e/journey-suites/src/webauthn.test.ts(1 hunks)packages/journey-client/src/lib/journey.store.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
PR: ForgeRock/ping-javascript-sdk#430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
e2e/journey-app/main.tspackages/journey-client/src/lib/journey.store.ts
🧬 Code graph analysis (1)
e2e/journey-app/main.ts (2)
e2e/journey-app/server-configs.ts (1)
serverConfigs(9-46)e2e/journey-app/components/webauthn.ts (1)
webauthnComponent(4-31)
🪛 Biome (2.1.2)
e2e/journey-suites/src/webauthn-usernameless.test.ts
[error] 85-85: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
e2e/journey-suites/src/webauthn.test.ts
[error] 96-96: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
e2e/journey-suites/src/webauthn-username-password.test.ts
[error] 106-106: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🪛 ESLint
e2e/journey-suites/src/webauthn-usernameless.test.ts
[error] 85-85: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
e2e/journey-suites/src/webauthn.test.ts
[error] 96-96: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
e2e/journey-suites/src/webauthn-username-password.test.ts
[error] 106-106: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
⏰ 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: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (6)
packages/journey-client/src/lib/journey.store.ts (1)
8-17: LGTM! Import reorganization looks good.The import statements have been reorganized and
createStorageis now explicitly imported from@forgerock/storage, which aligns with its usage at line 42. The changes maintain proper import ordering (external packages first, then relative imports) and have no functional impact.e2e/journey-app/tsconfig.app.json (1)
21-23: LGTM!The sdk-types project reference is correctly added and aligns with the new dependency in package.json.
e2e/journey-app/tsconfig.json (1)
23-25: LGTM!The sdk-types project reference is correctly added at the root tsconfig level, complementing the reference in tsconfig.app.json.
e2e/journey-app/package.json (1)
19-20: LGTM!The sdk-types dependency is correctly added using the workspace protocol, consistent with other ForgeRock package dependencies.
.zed/tasks.json (1)
1-90: LGTM!The Zed tasks configuration provides a comprehensive set of development workflows including build, lint, test, watch, and e2e tasks. The task definitions follow consistent patterns and properly integrate with the Nx/pnpm toolchain.
e2e/journey-suites/playwright.config.ts (1)
29-29: The nx targetvite:watch-depsis properly configured and should exist.The Nx Vite plugin is correctly configured with
"watchDepsTargetName": "vite:watch-deps"in nx.json, and the@forgerock/journey-appproject matches the plugin's include pattern (e2e/**/**/*). The project has a valid vite.config.ts file, which triggers Nx to auto-generate targets with the configured names. The target is available and executable.
| async function handleWebAuthn() { | ||
| try { | ||
| if (webAuthnStepType === WebAuthnStepType.Authentication) { | ||
| console.log('trying authentication'); | ||
| await WebAuthn.authenticate(step); | ||
| console.log('trying registration'); | ||
| } else if (WebAuthnStepType.Registration === webAuthnStepType) { | ||
| await WebAuthn.register(step); | ||
| } else { | ||
| return Promise.resolve(undefined); | ||
| } | ||
| } catch (error) { | ||
| console.error('WebAuthn error:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix misplaced console.log statement.
Line 19 contains console.log('trying registration') immediately after the authentication completes, which appears to be a copy-paste error. This log statement is misleading and should either be removed or placed before the registration call.
Apply this diff to fix the logging:
async function handleWebAuthn() {
try {
if (webAuthnStepType === WebAuthnStepType.Authentication) {
console.log('trying authentication');
await WebAuthn.authenticate(step);
- console.log('trying registration');
} else if (WebAuthnStepType.Registration === webAuthnStepType) {
+ console.log('trying registration');
await WebAuthn.register(step);
} else {
return Promise.resolve(undefined);
}
} catch (error) {
console.error('WebAuthn error:', error);
}
}📝 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.
| async function handleWebAuthn() { | |
| try { | |
| if (webAuthnStepType === WebAuthnStepType.Authentication) { | |
| console.log('trying authentication'); | |
| await WebAuthn.authenticate(step); | |
| console.log('trying registration'); | |
| } else if (WebAuthnStepType.Registration === webAuthnStepType) { | |
| await WebAuthn.register(step); | |
| } else { | |
| return Promise.resolve(undefined); | |
| } | |
| } catch (error) { | |
| console.error('WebAuthn error:', error); | |
| } | |
| } | |
| async function handleWebAuthn() { | |
| try { | |
| if (webAuthnStepType === WebAuthnStepType.Authentication) { | |
| console.log('trying authentication'); | |
| await WebAuthn.authenticate(step); | |
| } else if (WebAuthnStepType.Registration === webAuthnStepType) { | |
| console.log('trying registration'); | |
| await WebAuthn.register(step); | |
| } else { | |
| return Promise.resolve(undefined); | |
| } | |
| } catch (error) { | |
| console.error('WebAuthn error:', error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In e2e/journey-app/components/webauthn.ts around lines 14 to 28, the
console.log('trying registration') immediately after the authentication branch
is misplaced and misleading; remove that log line (or if a registration-start
log is desired, move a similar console.log to just before the
WebAuthn.register(step) call in the Registration branch) so that logging
accurately reflects the action being taken.
| if (!cdp) { | ||
| return; | ||
| } | ||
|
|
||
| const activeCdp = cdp; | ||
| await test.step('Remove virtual authenticator', async () => { | ||
| if (authenticatorId) { | ||
| await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); | ||
| } | ||
| if (webauthnEnabled) { | ||
| await activeCdp.send('WebAuthn.disable'); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Do not return from the finally cleanup.
Returning inside the finally block replaces any pending exception or assertion failure with an undefined return, letting the test pass even when the setup or expectations fail (Biome/ESLint both flag this). Guard the cleanup without returning so failures still surface.
Apply this diff to keep the cleanup while avoiding the unsafe return:
} finally {
- if (!cdp) {
- return;
- }
-
- const activeCdp = cdp;
- await test.step('Remove virtual authenticator', async () => {
- if (authenticatorId) {
- await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId });
- }
- if (webauthnEnabled) {
- await activeCdp.send('WebAuthn.disable');
- }
- });
+ const activeCdp = cdp;
+ if (activeCdp) {
+ await test.step('Remove virtual authenticator', async () => {
+ if (authenticatorId) {
+ await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId });
+ }
+ if (webauthnEnabled) {
+ await activeCdp.send('WebAuthn.disable');
+ }
+ });
+ }
}📝 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.
| if (!cdp) { | |
| return; | |
| } | |
| const activeCdp = cdp; | |
| await test.step('Remove virtual authenticator', async () => { | |
| if (authenticatorId) { | |
| await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); | |
| } | |
| if (webauthnEnabled) { | |
| await activeCdp.send('WebAuthn.disable'); | |
| } | |
| }); | |
| } | |
| const activeCdp = cdp; | |
| if (activeCdp) { | |
| await test.step('Remove virtual authenticator', async () => { | |
| if (authenticatorId) { | |
| await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); | |
| } | |
| if (webauthnEnabled) { | |
| await activeCdp.send('WebAuthn.disable'); | |
| } | |
| }); | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 106-106: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🪛 ESLint
[error] 106-106: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
🤖 Prompt for AI Agents
In e2e/journey-suites/src/webauthn-username-password.test.ts around lines
105-118, remove the `return` inside the finally cleanup so a thrown error isn't
swallowed; instead, ensure the cleanup is conditional by wrapping the cleanup
steps in an `if (cdp) { ... }` block (or by checking `if (!cdp) { /* skip */ }`
without returning) and keep the rest of the finally block intact so exceptions
and assertion failures still propagate.
| if (!cdp) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Remove unsafe return from finally block.
The return statement in the finally block (line 85) is unsafe because it overwrites control flow from the try block, potentially suppressing errors. If an error occurs in the try block, this return will prevent it from being propagated.
Apply this diff to fix the issue:
} finally {
- if (!cdp) {
- return;
- }
-
- const activeCdp = cdp;
await test.step('Remove virtual authenticator', async () => {
- if (authenticatorId) {
+ if (cdp && authenticatorId) {
- await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId });
+ await cdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId });
}
- if (webauthnEnabled) {
+ if (cdp && webauthnEnabled) {
- await activeCdp.send('WebAuthn.disable');
+ await cdp.send('WebAuthn.disable');
}
});
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 85-85: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🪛 ESLint
[error] 85-85: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
🤖 Prompt for AI Agents
In e2e/journey-suites/src/webauthn-usernameless.test.ts around lines 84 to 86,
the finally block contains a bare "return;" which can suppress errors thrown in
the try block; remove the "return;" so the finally only performs cleanup (e.g.,
close/cleanup cdp) and does not alter control flow, or if early exit behavior is
required handle it before entering the try block or set a flag and act on it
after the try/finally so exceptions are not swallowed.
| if (!cdp) { | ||
| return; | ||
| } | ||
|
|
||
| const activeCdp = cdp; | ||
| await test.step('Remove virtual authenticator', async () => { | ||
| if (authenticatorId) { | ||
| await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); | ||
| } | ||
| if (webauthnEnabled) { | ||
| await activeCdp.send('WebAuthn.disable'); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Eliminate the return inside finally.
The early return in finally overrides any exception/expectation failure from the try block, so the test can falsely succeed when setup or assertions fail (also flagged by Biome/ESLint). Guard the cleanup without returning to preserve failure propagation.
Apply this diff to keep the teardown but avoid suppressing errors:
} finally {
- if (!cdp) {
- return;
- }
-
- const activeCdp = cdp;
- await test.step('Remove virtual authenticator', async () => {
- if (authenticatorId) {
- await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId });
- }
- if (webauthnEnabled) {
- await activeCdp.send('WebAuthn.disable');
- }
- });
+ const activeCdp = cdp;
+ if (activeCdp) {
+ await test.step('Remove virtual authenticator', async () => {
+ if (authenticatorId) {
+ await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId });
+ }
+ if (webauthnEnabled) {
+ await activeCdp.send('WebAuthn.disable');
+ }
+ });
+ }
}📝 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.
| if (!cdp) { | |
| return; | |
| } | |
| const activeCdp = cdp; | |
| await test.step('Remove virtual authenticator', async () => { | |
| if (authenticatorId) { | |
| await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); | |
| } | |
| if (webauthnEnabled) { | |
| await activeCdp.send('WebAuthn.disable'); | |
| } | |
| }); | |
| } | |
| const activeCdp = cdp; | |
| if (activeCdp) { | |
| await test.step('Remove virtual authenticator', async () => { | |
| if (authenticatorId) { | |
| await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); | |
| } | |
| if (webauthnEnabled) { | |
| await activeCdp.send('WebAuthn.disable'); | |
| } | |
| }); | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 96-96: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🪛 ESLint
[error] 96-96: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
🤖 Prompt for AI Agents
In e2e/journey-suites/src/webauthn.test.ts around lines 95 to 108, remove the
early `return` inside the `finally` so it doesn't swallow errors; instead, keep
the teardown but guard it with a conditional that only runs the cleanup when
`cdp` exists (e.g., capture `const activeCdp = cdp` and wrap the
`test.step('Remove virtual authenticator', ...)` block in `if (activeCdp) { ...
}`), leaving all awaits inside so failures propagate.
ryanbas21
force-pushed
the
virtual-authenticators
branch
from
e624f97 to
576b1e8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (18.52%) 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 #467 +/- ##
=======================================
Coverage 18.52% 18.52%
=======================================
Files 138 138
Lines 27402 27402
Branches 963 963
=======================================
Hits 5076 5076
Misses 22326 22326
🚀 New features to boost your workflow:
|
@forgerock/davinci-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 4ccc985 to https://ForgeRock.github.io/ping-javascript-sdk/pr-467/4ccc9857e85a4e6aaea9f3a7c98b0ba3334288f5 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.0 KB, -100.0%) 📊 Minor Changes📉 @forgerock/journey-client - 82.0 KB (-0.0 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
e2e/journey-suites/src/webauthn-username-password.test.ts (1)
103-117: Let test failures surface by avoidingreturninfinally.Returning from the
finallyblock swallows any exception raised in the registration/authentication steps, so the test can pass while logic fails (Biome/ESLint highlight this). Guard the cleanup with anif (cdp)check but keep thefinallyflow fall-through.
Suggested diff (mirrors the other WebAuthn test):- } finally { - if (!cdp) { - return; - } - - const activeCdp = cdp; - await test.step('Remove virtual authenticator', async () => { - if (authenticatorId) { - await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); - } - if (webauthnEnabled) { - await activeCdp.send('WebAuthn.disable'); - } - }); - } + } finally { + if (!cdp) { + return; + } + + await test.step('Remove virtual authenticator', async () => { + if (authenticatorId) { + await cdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); + } + if (webauthnEnabled) { + await cdp.send('WebAuthn.disable'); + } + }); + }e2e/journey-suites/src/webauthn-usernameless.test.ts (1)
83-96: Remove thereturnfrom thefinallycleanup.The
returnin thefinallyblock prevents any errors thrown in thetrysteps from propagating—tests will pass even when WebAuthn setup/assertions fail (Biome/ESLint already flag this). Instead, guard the cleanup with anif (cdp)check and perform the teardown without returning from thefinally.
Apply this diff:- } finally { - if (!cdp) { - return; - } - - const activeCdp = cdp; - await test.step('Remove virtual authenticator', async () => { - if (authenticatorId) { - await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); - } - if (webauthnEnabled) { - await activeCdp.send('WebAuthn.disable'); - } - }); - } + } finally { + if (!cdp) { + return; + } + + await test.step('Remove virtual authenticator', async () => { + if (authenticatorId) { + await cdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); + } + if (webauthnEnabled) { + await cdp.send('WebAuthn.disable'); + } + }); + }e2e/journey-suites/src/webauthn.test.ts (1)
95-97: Eliminate the early return in the finally block.The
returnstatement at line 96 will suppress any errors or assertion failures from thetryblock, causing the test to falsely pass when it should fail. This was previously flagged in review and by static analysis tools (Biome, ESLint).Apply this diff to preserve error propagation while safely guarding cleanup:
} finally { - if (!cdp) { - return; - } - const activeCdp = cdp; - await test.step('Remove virtual authenticator', async () => { - if (authenticatorId) { - await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); - } - if (webauthnEnabled) { - await activeCdp.send('WebAuthn.disable'); - } - }); + if (activeCdp) { + await test.step('Remove virtual authenticator', async () => { + if (authenticatorId) { + await activeCdp.send('WebAuthn.removeVirtualAuthenticator', { authenticatorId }); + } + if (webauthnEnabled) { + await activeCdp.send('WebAuthn.disable'); + } + }); + } }
🧹 Nitpick comments (2)
e2e/journey-app/server-configs.ts (1)
10-45: Consider extracting common configuration to reduce duplication.All six entries share identical
baseUrlandrealmPathvalues. If these change, you'll need to update six places. Consider extracting the common configuration:+const commonConfig = { + serverConfig: { + baseUrl: 'https://openam-sdks.forgeblocks.com/am/', + }, + realmPath: '/alpha', +}; + export const serverConfigs: Record<string, JourneyClientConfig> = { - UsernamePassword: { - serverConfig: { - baseUrl: 'https://openam-sdks.forgeblocks.com/am/', - }, - realmPath: '/alpha', - }, + UsernamePassword: commonConfig, - ['TEST_WebAuthn-Registration']: { - serverConfig: { - baseUrl: 'https://openam-sdks.forgeblocks.com/am/', - }, - realmPath: '/alpha', - }, + ['TEST_WebAuthn-Registration']: commonConfig, // ... repeat for remaining entries };Alternatively, use a factory helper or
Object.fromEntriesto map all journey keys to the shared config in one pass.e2e/journey-suites/src/webauthn.test.ts (1)
12-12: Unused variable:recordedCredentialIdsis never consumed.The
recordedCredentialIdsvariable is populated at line 61 but never referenced in subsequent steps. If you intended to verify these credentials are reused during authentication, add that assertion; otherwise, remove the unused variable or convert to a simple existence check.Also applies to: 56-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.zed/tasks.json(1 hunks)e2e/journey-app/components/webauthn.ts(1 hunks)e2e/journey-app/main.ts(4 hunks)e2e/journey-app/package.json(1 hunks)e2e/journey-app/server-configs.ts(1 hunks)e2e/journey-app/tsconfig.app.json(1 hunks)e2e/journey-app/tsconfig.json(1 hunks)e2e/journey-suites/playwright.config.ts(1 hunks)e2e/journey-suites/src/webauthn-username-password.test.ts(1 hunks)e2e/journey-suites/src/webauthn-usernameless.test.ts(1 hunks)e2e/journey-suites/src/webauthn.test.ts(1 hunks)packages/journey-client/package.json(0 hunks)packages/journey-client/src/lib/journey.store.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/journey-client/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/journey-app/tsconfig.app.json
- e2e/journey-app/tsconfig.json
- packages/journey-client/src/lib/journey.store.ts
- e2e/journey-app/components/webauthn.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
PR: ForgeRock/ping-javascript-sdk#430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
e2e/journey-app/main.ts
🧬 Code graph analysis (1)
e2e/journey-app/main.ts (2)
e2e/journey-app/server-configs.ts (1)
serverConfigs(9-46)e2e/journey-app/components/webauthn.ts (1)
webauthnComponent(4-31)
🪛 Biome (2.1.2)
e2e/journey-suites/src/webauthn-username-password.test.ts
[error] 106-106: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
e2e/journey-suites/src/webauthn-usernameless.test.ts
[error] 85-85: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
e2e/journey-suites/src/webauthn.test.ts
[error] 96-96: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🪛 ESLint
e2e/journey-suites/src/webauthn-username-password.test.ts
[error] 106-106: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
e2e/journey-suites/src/webauthn-usernameless.test.ts
[error] 85-85: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
e2e/journey-suites/src/webauthn.test.ts
[error] 96-96: Unsafe usage of ReturnStatement.
(no-unsafe-finally)
⏰ 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: Mend Code Security Check
- GitHub Check: pr
| "label": "nx typecheck repo", | ||
| "command": "pnpm", | ||
| "args": ["nx", "affected -t typecheck"], | ||
| "cwd": "$ZED_WORKTREE_ROOT", |
There was a problem hiding this comment.
Split Nx CLI flags into separate args.
"args": ["nx", "affected -t typecheck"] puts three tokens into a single argument, so pnpm spawns Nx with affected -t typecheck as the subcommand. Nx interprets that as an unknown target and exits before running type-checks. The earlier tasks (build, lint, test) have the same pattern ("-t build", etc.) and will misparse for the same reason. Please split each token into its own array entry (e.g., ["nx", "affected", "-t", "typecheck"] or ["nx", "affected", "-t=typecheck"]) so the commands execute.
🤖 Prompt for AI Agents
.zed/tasks.json around lines 58 to 61: the args entry currently uses a single
string "nx", "affected -t typecheck" which passes multiple CLI tokens as one
argument so pnpm spawns Nx with an unknown subcommand; split the Nx CLI tokens
into separate args elements (e.g. nx, affected, -t, typecheck or nx, affected,
-t=typecheck) so each flag or subcommand is its own array entry; apply the same
fix to the other tasks that use "-t build", "-t lint", "-t test" earlier in the
file.
| if (step?.type === 'Step') { | ||
| await renderForm(); | ||
| } else if (step?.type === 'LoginSuccess') { | ||
| console.log('Basic login successful'); | ||
| renderComplete(); | ||
| } else if (step?.type === 'LoginFailure') { | ||
| renderForm(); | ||
| renderError(); | ||
| } else { | ||
| console.error('Unknown node status', step); | ||
| } |
There was a problem hiding this comment.
Handle LoginFailure without re-entering renderForm.
When journeyClient.next returns a LoginFailure, this branch calls renderForm() before renderError(). However, renderForm() throws if step.type !== 'Step', so any failure in a WebAuthn step now crashes instead of showing the error page. Remove that renderForm() call and surface the error (e.g., call renderError() directly or restart the journey) so the failure state is reachable.
🤖 Prompt for AI Agents
In e2e/journey-app/main.ts around lines 117 to 127, the LoginFailure branch
calls renderForm() before renderError(), but renderForm() throws unless
step.type === 'Step', causing crashes on WebAuthn failures; remove the
renderForm() call in the LoginFailure branch and instead call renderError() (or
trigger a journey restart) so the failure state is surfaced without invoking
renderForm() when step.type !== 'Step'.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I'm a bit confused by this PR. I think we should catch up soon, so I can better understand what's happening here.
| @@ -0,0 +1,90 @@ | |||
| // Project tasks configuration. See https://zed.dev/docs/tasks for documentation. | |||
There was a problem hiding this comment.
I don't think we'd want to commit this. We can ignore this, but also not remove it when we do a clean. The same as we do with our .env files and such.
| }, | ||
| realmPath: '/alpha', | ||
| }, | ||
| ['TEST_WebAuthn-Registration']: { |
There was a problem hiding this comment.
Yeah, I wasn't sure exactly how we wanted to handle different configs. I ended up not using it at all in all the tests I moved over from the legacy repo.
| import { asyncEvents } from './utils/async-events.js'; | ||
| import { password, username } from './utils/demo-user.js'; | ||
|
|
||
| test.use({ browserName: 'chromium' }); // ensure CDP/WebAuthn is available |
There was a problem hiding this comment.
Are Virtual Authenticators only viable on Chromium?
| let cdp: CDPSession | undefined; | ||
| let authenticatorId: string | undefined; | ||
| let webauthnEnabled = false; | ||
| let recordedCredentialIds: string[] = []; | ||
|
|
||
| await test.step('Configure virtual authenticator', async () => { | ||
| cdp = await context.newCDPSession(page); | ||
| await cdp.send('WebAuthn.enable'); | ||
| webauthnEnabled = true; | ||
|
|
||
| // A "platform" authenticator (aka internal) with UV+RK enabled is the usual default for passkeys. | ||
| const response = await cdp.send('WebAuthn.addVirtualAuthenticator', { | ||
| options: { | ||
| protocol: 'ctap2', | ||
| transport: 'internal', // platform authenticator | ||
| hasResidentKey: true, // allow discoverable credentials (passkeys) | ||
| hasUserVerification: true, // device supports UV | ||
| isUserVerified: true, // simulate successful UV (PIN/biometric) | ||
| automaticPresenceSimulation: true, // auto "touch"/presence | ||
| }, | ||
| }); | ||
| authenticatorId = response.authenticatorId; | ||
| }); |
There was a problem hiding this comment.
Might be good to have a Friday conversation about this CDP config stuff.
|
|
||
| try { | ||
| // First, register a credential we can use later | ||
| await test.step('Navigate to registration journey', async () => { |
There was a problem hiding this comment.
This seems like a new pattern (e.g. test.step) for out testing. I'd really like to stay with one pattern consistently throughout this repo.
| await test.step('Register WebAuthn credential', async () => { | ||
| // With the virtual authenticator present and presence auto-simulated, | ||
| // registration will complete without any OS prompts. | ||
| await expect(page.getByRole('button', { name: 'Logout' })).toBeVisible(); |
There was a problem hiding this comment.
I'd like to avoid testing the test app's UI logic (e.g. using the presence of the Logout button to indirectly assert success), and focus more on testing outcomes of SDK functionality.
There was a problem hiding this comment.
we do this in almost every test suite, is this something you're suggesting we no longer do?
There was a problem hiding this comment.
Yeah, I noticed this when I was refactoring the OIDC tests, and removed it from them. I just wanted to at least not continue to do it, and refactor existing tests when we can.
| await page.getByRole('button', { name: 'Submit' }).click(); | ||
| }); | ||
|
|
||
| await test.step('Register WebAuthn credential', async () => { |
There was a problem hiding this comment.
This test says 'Register WebAuthn credential', but it doesn't do anything other than expect the Logout button to be present. Am I missing something?
There was a problem hiding this comment.
Yeah I think this is from my debugging leftover. was going to clean up once I fix the tests with the node solution
| await test.step('Capture virtual authenticator credentials', async () => { | ||
| if (!cdp || !authenticatorId) { | ||
| throw new Error('Expected CDP session and authenticator to be configured'); | ||
| } | ||
| const { credentials } = await cdp.send('WebAuthn.getCredentials', { authenticatorId }); | ||
| recordedCredentialIds = (credentials || []).map((item) => item.credentialId); | ||
| expect(recordedCredentialIds.length).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
What is this block testing in relation to the SDK?
There was a problem hiding this comment.
it's testing that the credentials were actually stored. so that the sdk stored the credential itself.
There was a problem hiding this comment.
The SDK stores credentials?
There was a problem hiding this comment.
maybe I could have been more clear, but i just meant that the SDK creates the credential and it exists.
The sdk calls create.
This verifies that we are in fact creating the credentials and they are stored (which is on the device it was created)
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4462
Description
Adds virtual authenticator tests for a few webauthn flows
Summary by CodeRabbit
Tests
New Features
Chores