feat(davinci-client): send FIDO errors to DaVinci (SDKS-4480) - #730
feat(davinci-client): send FIDO errors to DaVinci (SDKS-4480)#730ancheetah wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: da1c018 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughFIDO registration and authentication now convert WebAuthn exceptions into structured errors, propagate them through DaVinci collectors and action requests, and expose updated public types. Unit and end-to-end tests cover error mapping, reducer handling, request construction, and canceled WebAuthn prompts. ChangesFIDO error flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant FidoClient
participant WebAuthn
participant DaVinci
Browser->>FidoClient: Start registration or authentication
FidoClient->>WebAuthn: Request credential
WebAuthn-->>FidoClient: Credential or DOMException
FidoClient->>DaVinci: Submit formatted value or fido_error action
DaVinci-->>Browser: Updated flow response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 da1c018
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
@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: |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (23.73%) 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 #730 +/- ##
==========================================
+ Coverage 23.26% 23.73% +0.46%
==========================================
Files 161 162 +1
Lines 25661 25714 +53
Branches 1626 1661 +35
==========================================
+ Hits 5970 6103 +133
+ Misses 19691 19611 -80
🚀 New features to boost your workflow:
|
|
Deployed 6de8880 to https://ForgeRock.github.io/ping-javascript-sdk/pr-730/6de88803e5d841888adfb38e7bbcdebd7aa4000f branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/davinci-client - 56.9 KB (+1.7 KB, +3.2%) 🆕 New Packages🆕 @forgerock/journey-client - 92.6 KB (new) ➖ No Changes➖ @forgerock/sdk-types - 9.1 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
641b051 to
da1c018
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/davinci-client/src/lib/davinci.api.ts (1)
172-198: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrioritize pending FIDO errors over metadata collectors.
hasMetadataCollectoris evaluated beforefidoErrorCollector, so a node with both collectors sendsstate.node.client?.actioninstead of the FIDO error code, andtransformActionRequestexcludes FIDO collectors fromformData. This can discard FIDO error information; move the FIDO error branch first.Also add coverage for the FIDO-only error path and the combined MetadataCollector + FIDO error case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/davinci-client/src/lib/davinci.api.ts` around lines 172 - 198, Update the request-body selection in the no-body branch so the fidoErrorCollector condition is evaluated before hasMetadataCollector, ensuring pending FIDO errors use their error code even when both collectors exist; retain transformSubmitRequest as the fallback. Add coverage for both FIDO-only errors and the combined MetadataCollector plus FIDO error scenario.
🧹 Nitpick comments (3)
packages/davinci-client/src/lib/fido/fido.test.ts (2)
55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated
navigator.credentialsmock setup into a helper.The
Object.defineProperty(navigator, 'credentials', {...})block is duplicated across ~12 test cases.♻️ Suggested helper
function mockCredentials(methods: Partial<CredentialsContainer>) { Object.defineProperty(navigator, 'credentials', { value: methods, writable: true, configurable: true, }); }Also applies to: 77-83, 96-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/davinci-client/src/lib/fido/fido.test.ts` around lines 55 - 63, Extract the duplicated navigator.credentials Object.defineProperty setup into a shared mockCredentials helper in fido.test.ts, accepting the required partial CredentialsContainer methods and preserving writable/configurable behavior. Replace each repeated setup across the affected tests with this helper, including the cases around the existing mockCreate definitions.
53-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the "no options provided" branches.
No test calls
register()/authenticate()with falsy options, leaving the early-return branches infido.ts(lines ~47-53 and ~104-110) untested — this lines up with the PR's own Codecov note of 12 missing-coverage lines infido.ts.✅ Suggested additional tests
it('should return GenericError when register is called without options', async () => { const fidoClient = fido(silentConfig); const result = await fidoClient.register(undefined as unknown as FidoRegistrationOptions); expect(isGenericError(result)).toBe(true); }); it('should return GenericError when authenticate is called without options', async () => { const fidoClient = fido(silentConfig); const result = await fidoClient.authenticate(undefined as unknown as FidoAuthenticationOptions); expect(isGenericError(result)).toBe(true); });Also applies to: 242-429
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/davinci-client/src/lib/fido/fido.test.ts` around lines 53 - 240, Add tests covering the falsy-options early-return branches in the fido client: call register and authenticate with undefined options, then assert each result is a GenericError. Place these cases alongside the existing register and authenticate tests, using the existing fidoClient setup and type casts as needed.packages/davinci-client/src/lib/davinci.utils.test.ts (1)
215-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new FIDO exclusion branch in
transformActionRequest.This suite still only exercises
transformActionRequestwith an emptycollectorsarray, so the new filter that excludesFidoRegistrationCollector/FidoAuthenticationCollectorfromformData(davinci.utils.ts lines 99-103) is untested — this lines up with the coverage gap Codecov flagged fordavinci.utils.ts.it('should exclude FIDO collectors from formData', () => { const node: ContinueNode = { /* ... */ client: { action: 'SIGNON', collectors: [ { category: 'ObjectValueAutoCollector', type: 'FidoRegistrationCollector', input: { key: 'fido2-registration', value: {} }, // ... }, ], status: 'continue', }, // ... }; const result = transformActionRequest(node, 'TEST_ACTION', logger({ level: 'none' })); expect(result.parameters.data.formData).toEqual({}); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/davinci-client/src/lib/davinci.utils.test.ts` around lines 215 - 253, Add a test case alongside the existing transformActionRequest coverage that supplies FidoRegistrationCollector and FidoAuthenticationCollector entries in node.client.collectors, then verifies transformActionRequest returns an empty parameters.data.formData. Reuse the existing ContinueNode setup and logger, and cover both FIDO collector types so the exclusion branch is fully exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/thirty-badgers-lick.md:
- Line 5: Correct the typo in the changeset description by replacing
“DOMExeceptions” with “DOMExceptions”; leave the rest of the text unchanged.
In `@e2e/davinci-suites/src/password-policy.test.ts`:
- Line 75: Update the cleanup failure throw in the test-user deletion flow to
preserve native Error details instead of relying on JSON.stringify(err). Use the
error’s message and/or stack, with a safe fallback for non-Error values, while
retaining the email context and [cleanup] prefix.
In `@packages/davinci-client/src/lib/node.reducer.test.ts`:
- Around line 1270-1348: Rename the FIDO error test title at
packages/davinci-client/src/lib/node.reducer.test.ts:1270-1348 to state that the
GenericError is stored in collector.input.value, not collector.error. Apply the
same title correction to the authentication test at
packages/davinci-client/src/lib/node.reducer.test.ts:2178-2249; no assertion or
reducer changes are needed.
---
Outside diff comments:
In `@packages/davinci-client/src/lib/davinci.api.ts`:
- Around line 172-198: Update the request-body selection in the no-body branch
so the fidoErrorCollector condition is evaluated before hasMetadataCollector,
ensuring pending FIDO errors use their error code even when both collectors
exist; retain transformSubmitRequest as the fallback. Add coverage for both
FIDO-only errors and the combined MetadataCollector plus FIDO error scenario.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/davinci.utils.test.ts`:
- Around line 215-253: Add a test case alongside the existing
transformActionRequest coverage that supplies FidoRegistrationCollector and
FidoAuthenticationCollector entries in node.client.collectors, then verifies
transformActionRequest returns an empty parameters.data.formData. Reuse the
existing ContinueNode setup and logger, and cover both FIDO collector types so
the exclusion branch is fully exercised.
In `@packages/davinci-client/src/lib/fido/fido.test.ts`:
- Around line 55-63: Extract the duplicated navigator.credentials
Object.defineProperty setup into a shared mockCredentials helper in
fido.test.ts, accepting the required partial CredentialsContainer methods and
preserving writable/configurable behavior. Replace each repeated setup across
the affected tests with this helper, including the cases around the existing
mockCreate definitions.
- Around line 53-240: Add tests covering the falsy-options early-return branches
in the fido client: call register and authenticate with undefined options, then
assert each result is a GenericError. Place these cases alongside the existing
register and authenticate tests, using the existing fidoClient setup and type
casts as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d515f1aa-e45e-4e0a-b2df-73a168ce8cf2
📒 Files selected for processing (21)
.changeset/thirty-badgers-lick.mde2e/davinci-app/components/fido.tse2e/davinci-app/server-configs.tse2e/davinci-suites/src/fido.test.tse2e/davinci-suites/src/password-policy.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/davinci.api.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/davinci.utils.test.tspackages/davinci-client/src/lib/davinci.utils.tspackages/davinci-client/src/lib/fido/fido.test.tspackages/davinci-client/src/lib/fido/fido.tspackages/davinci-client/src/lib/fido/fido.types.test.tspackages/davinci-client/src/lib/fido/fido.types.tspackages/davinci-client/src/lib/fido/fido.utils.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/types.ts
| '@forgerock/davinci-client': minor | ||
| --- | ||
|
|
||
| Catch FIDO/WebAuthn DOMExeceptions and send them to DaVinci |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo: "DOMExeceptions" → "DOMExceptions".
✏️ Proposed fix
-Catch FIDO/WebAuthn DOMExeceptions and send them to DaVinci
+Catch FIDO/WebAuthn DOMExceptions and send them to DaVinci📝 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.
| Catch FIDO/WebAuthn DOMExeceptions and send them to DaVinci | |
| Catch FIDO/WebAuthn DOMExceptions and send them to DaVinci |
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Ensure spelling is correct
Context: ...client': minor --- Catch FIDO/WebAuthn DOMExeceptions and send them to DaVinci
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/thirty-badgers-lick.md at line 5, Correct the typo in the
changeset description by replacing “DOMExeceptions” with “DOMExceptions”; leave
the rest of the text unchanged.
Source: Linters/SAST tools
| await deleteTestUser(page, email); | ||
| } catch (err) { | ||
| console.error(`[cleanup] Failed to delete test user ${email}:`, err); | ||
| throw new Error(`[cleanup] Failed to delete test user ${email}: ${JSON.stringify(err)}`); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
JSON.stringify(err) loses the error detail for native Error objects.
Error instances don't enumerate message/stack, so JSON.stringify(err) typically serializes to {}, defeating the purpose of surfacing cleanup-failure details in the thrown error.
🐛 Proposed fix
- throw new Error(`[cleanup] Failed to delete test user ${email}: ${JSON.stringify(err)}`);
+ throw new Error(
+ `[cleanup] Failed to delete test user ${email}: ${err instanceof Error ? err.message : String(err)}`,
+ );📝 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.
| throw new Error(`[cleanup] Failed to delete test user ${email}: ${JSON.stringify(err)}`); | |
| throw new Error( | |
| `[cleanup] Failed to delete test user ${email}: ${err instanceof Error ? err.message : String(err)}`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/davinci-suites/src/password-policy.test.ts` at line 75, Update the
cleanup failure throw in the test-user deletion flow to preserve native Error
details instead of relying on JSON.stringify(err). Use the error’s message
and/or stack, with a safe fallback for non-Error values, while retaining the
email context and [cleanup] prefix.
| it('should store a GenericError on collector.error when a FIDO error is passed as value', () => { | ||
| const fidoError: GenericError = { | ||
| code: 'NotAllowedError', | ||
| error: 'registration_error', | ||
| message: 'FIDO registration failed: NotAllowedError', | ||
| type: 'fido_error', | ||
| }; | ||
| const publicKeyCredentialCreationOptions: FidoRegistrationOptions = { | ||
| rp: { name: 'Example RP', id: 'example.com' }, | ||
| user: { id: [1], displayName: 'Test User', name: 'testuser' }, | ||
| challenge: [1, 2, 3, 4], | ||
| pubKeyCredParams: [{ type: 'public-key', alg: -7 }], | ||
| timeout: 60000, | ||
| authenticatorSelection: { | ||
| residentKey: 'required', | ||
| requireResidentKey: true, | ||
| userVerification: 'required', | ||
| }, | ||
| attestation: 'none', | ||
| extensions: { credProps: true, hmacCreateSecret: true }, | ||
| }; | ||
| const action = { | ||
| type: 'node/update', | ||
| payload: { | ||
| id: 'fido2-registration-0', | ||
| value: fidoError, | ||
| }, | ||
| }; | ||
| const state: FidoRegistrationCollector[] = [ | ||
| { | ||
| category: 'ObjectValueAutoCollector', | ||
| error: null, | ||
| type: 'FidoRegistrationCollector', | ||
| id: 'fido2-registration-0', | ||
| name: 'fido2-registration', | ||
| input: { | ||
| key: 'fido2-registration', | ||
| value: {}, | ||
| type: 'FIDO2', | ||
| validation: null, | ||
| }, | ||
| output: { | ||
| key: 'fido2-registration', | ||
| type: 'FIDO2', | ||
| config: { | ||
| publicKeyCredentialCreationOptions, | ||
| action: 'REGISTER', | ||
| trigger: 'BUTTON', | ||
| }, | ||
| }, | ||
| }, | ||
| ]; | ||
|
|
||
| expect(nodeCollectorReducer(state, action)).toStrictEqual([ | ||
| { | ||
| category: 'ObjectValueAutoCollector', | ||
| error: null, | ||
| type: 'FidoRegistrationCollector', | ||
| id: 'fido2-registration-0', | ||
| name: 'fido2-registration', | ||
| input: { | ||
| key: 'fido2-registration', | ||
| value: fidoError, | ||
| type: 'FIDO2', | ||
| validation: null, | ||
| }, | ||
| output: { | ||
| key: 'fido2-registration', | ||
| type: 'FIDO2', | ||
| config: { | ||
| publicKeyCredentialCreationOptions, | ||
| action: 'REGISTER', | ||
| trigger: 'BUTTON', | ||
| }, | ||
| }, | ||
| }, | ||
| ]); | ||
| }); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both new FIDO-error test titles say "collector.error" but assert on input.value. In each test, error: null is unchanged before/after; the GenericError is actually stored in collector.input.value (per node.reducer.ts), not the top-level collector.error field. Rename both titles to reflect the actual behavior.
packages/davinci-client/src/lib/node.reducer.test.ts#L1270-L1348: rename'should store a GenericError on collector.error when a FIDO error is passed as value'(registration) to describe storing oninput.valueinstead.packages/davinci-client/src/lib/node.reducer.test.ts#L2178-L2249: rename the identical title (authentication) the same way.
📍 Affects 1 file
packages/davinci-client/src/lib/node.reducer.test.ts#L1270-L1348(this comment)packages/davinci-client/src/lib/node.reducer.test.ts#L2178-L2249
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/davinci-client/src/lib/node.reducer.test.ts` around lines 1270 -
1348, Rename the FIDO error test title at
packages/davinci-client/src/lib/node.reducer.test.ts:1270-1348 to state that the
GenericError is stored in collector.input.value, not collector.error. Apply the
same title correction to the authentication test at
packages/davinci-client/src/lib/node.reducer.test.ts:2178-2249; no assertion or
reducer changes are needed.
| */ | ||
| export function fido(): FidoClient { | ||
| export function fido(config?: FidoClientConfig): FidoClient { | ||
| const log = loggerFn({ level: config?.logger?.level ?? 'error', custom: config?.logger?.custom }); |
There was a problem hiding this comment.
Why create a new logger instance here? Shouldn't we use the existing logger instance from the original config generation?
| 'ObjectValueAutoCollector', | ||
| 'FidoRegistrationCollector', | ||
| FidoRegistrationInputValue, | ||
| FidoRegistrationInputValue | GenericError, |
There was a problem hiding this comment.
I'm guessing the above stems from this?
| }; | ||
| } | ||
|
|
||
| export interface FidoClient { |
There was a problem hiding this comment.
Wondering about the naming of FidoClient. This kind of goes against our original naming of a client / server model with naming things client.
Is this a Client or just Fido?
| data: { | ||
| actionKey: action || node.client?.action || '', | ||
| ...(Object.keys(formData ?? {}).length && { formData: formData }), | ||
| formData: formData ?? {}, |
There was a problem hiding this comment.
This feels like it's breaking other functionality? we're changing formData to not be in an object?
There was a problem hiding this comment.
I think the earlier case excluded formData key from the object if formData was empty, now we are sending {} if formData does not exist. I believe it is to cover for the Fido case where formData is empty always.
And we already send {} for formData for submit and we now are doing the same for action: https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/davinci-client/src/lib/davinci.utils.ts#L74
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I think this looks good. I just have a comment on the GenericError bit, but it's not worth blocking the PR over it.
| ? PhoneNumberExtensionInputValue | ||
| : T extends { type: 'FidoRegistrationCollector' } | ||
| ? FidoRegistrationInputValue | ||
| ? FidoRegistrationInputValue | GenericError |
There was a problem hiding this comment.
Would it be possible to have FidoRegistrationInputValue be inclusive of GenericError? In other words:
type FidoRegistrationInputValue = FidoRegistrationInputSuccessValue | GenericError;
That way, we don't have to pass around GenericError everywhere. If not, no big deal, but I figured it's worth asking.
| 'ObjectValueAutoCollector', | ||
| 'FidoRegistrationCollector', | ||
| FidoRegistrationInputValue, | ||
| FidoRegistrationInputValue | GenericError, |
There was a problem hiding this comment.
Yeah, these GenericErrors are sticking out like sore thumbs :)
| @@ -333,10 +341,12 @@ export const nodeCollectorReducer = createReducer(initialCollectorValues, (build | |||
| if (typeof action.payload.value !== 'object') { | |||
| throw new Error('Value argument must be an object'); | |||
| } | |||
| if (!('assertionValue' in action.payload.value)) { | |||
| const isFidoError = | |||
| 'type' in action.payload.value && action.payload.value.type === 'fido_error'; | |||
| if (!isFidoError && !('assertionValue' in action.payload.value)) { | |||
| throw new Error('Value argument must contain an assertionValue property'); | |||
| } | |||
| collector.input.value = action.payload.value; | |||
| collector.input.value = action.payload.value as FidoAuthenticationInputValue | GenericError; | |||
There was a problem hiding this comment.
We can simplify the code here like below
if (
collector.type === 'FidoRegistrationCollector' ||
collector.type === 'FidoAuthenticationCollector'
) {
if (typeof action.payload.id !== 'string') {
throw new Error('Index argument must be a string');
}
if (typeof action.payload.value !== 'object') {
throw new Error('Value argument must be an object');
}
const requiredProp =
collector.type === 'FidoRegistrationCollector' ? 'attestationValue' : 'assertionValue';
const isFidoError =
'type' in action.payload.value && action.payload.value.type === 'fido_error';
if (!isFidoError && !(requiredProp in action.payload.value)) {
throw new Error(`Value argument must contain an ${requiredProp} property`);
}
collector.input.value = action.payload.value as
| FidoRegistrationInputValue
| FidoAuthenticationInputValue
| GenericError;
5 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4480
Description
Summary by CodeRabbit
New Features
Bug Fixes
Tests