feat(davinci-client): send FIDO errors to DaVinci (SDKS-4480) - #730
Conversation
🦋 Changeset detectedLatest commit: 5a9e974 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 |
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 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 5a9e974
💡 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.72%) 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.72% +0.46%
==========================================
Files 161 162 +1
Lines 25661 25712 +51
Branches 1626 1660 +34
==========================================
+ Hits 5970 6101 +131
+ Misses 19691 19611 -80
🚀 New features to boost your workflow:
|
|
Deployed 13e1359 to https://ForgeRock.github.io/ping-javascript-sdk/pr-730/13e1359d185c79d19b7d2e4bd0c31cd0ff4a6083 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/davinci-client - 56.7 KB (+1.5 KB, +2.8%) 🆕 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
| 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.
| */ | ||
| 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?
There was a problem hiding this comment.
Both the config and the logger in the config are optional so we need to define a default logger in case they don't pass one in.
| '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?
There was a problem hiding this comment.
I agree with you but we've already released this so we can't go back now. My fault.
https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/davinci-client/src/lib/fido/fido.ts#L23
| 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
There was a problem hiding this comment.
Vatsal is right, we only introduced sending formData in the metadata pr. We historically have not sent formData at all with action requests. So it's ok to change this here. Also, it's still an object either way.
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.
There was a problem hiding this comment.
Would this be a breaking change? We've already released FidoRegistrationInputValue, so redefining it this way would be breaking, no?
| '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;
da1c018 to
c0ae86b
Compare
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
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 has identified a possible root cause for your failed CI:
We investigated the @forgerock/oidc-suites:e2e-ci--src/login.spec.ts failure and determined it is unrelated to this PR. The error is a 30-second browser context setup timeout (browserContext.newPage) in a PingOne OIDC test that was not modified by our changes, with 8 of 9 tests passing successfully. This is a transient environment issue and a re-run should resolve it.
No code changes were suggested for this issue.
Trigger a rerun:
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
🎓 Learn more about Self-Healing CI on nx.dev
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
approved pending upcoming changes
5 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4480
Description
Summary by CodeRabbit
New Features
Bug Fixes
Tests