feat(davinci-client): send FIDO errors to DaVinci - #728
Conversation
🦋 Changeset detectedLatest commit: 60d9743 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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 60d9743
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ 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 @@
## SDKS-5100-metadata-collector #728 +/- ##
================================================================
+ Coverage 23.26% 23.73% +0.47%
================================================================
Files 161 162 +1
Lines 25667 25718 +51
Branches 1626 1661 +35
================================================================
+ Hits 5971 6104 +133
+ Misses 19696 19614 -82
🚀 New features to boost your workflow:
|
|
Deployed cbc6c62 to https://ForgeRock.github.io/ping-javascript-sdk/pr-728/cbc6c624f4639f45580dbe657cceeec5aac73016 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/davinci-client - 56.9 KB (+2.5 KB, +4.5%) 🆕 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 |
8693a1d to
a2cae19
Compare
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
It seems this PR is based on the Metadata PR. If I look at just the FIDO related commit, it looks good. But, this also contains the other unrelated changes for Metadata, so I'm hesitate to approve as this may be a mistake.
|
Edit: I just saw in the description that wiring up the flow is in TODO. I'll do another review when that's done. |
|
Sorry about that everyone, I am about to push up the part where we send the payload to DaVinci. Looks like my rebase got messed up because metadata shouldn't be in here. Let me fix and let you know when it's ready again. |
79ba07f to
acd6de3
Compare
![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.
acd6de3 to
5165254
Compare
44abe1c to
fc0caf2
Compare
5165254 to
60d9743
Compare
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We resolved three TypeScript compilation errors introduced by the metadata collector feature. The duplicate MetadataError definition (in both collector.types.ts and the new metadata.types.ts) caused a TS2308 ambiguous re-export in src/types.ts, which we fixed by having metadata.types.ts re-export from collector.types.ts and adding an explicit named export in src/types.ts to resolve the conflict. We also corrected the MetadataError import source in client.store.ts and added GenericError to DaVinciRequestValueTypes so FIDO error values can flow through the request transform in davinci.utils.ts.
Tip
✅ We verified this fix by re-running @forgerock/davinci-client:typecheck, @forgerock/davinci-client:build.
Suggested Fix changes
diff --git a/packages/davinci-client/src/lib/client.store.ts b/packages/davinci-client/src/lib/client.store.ts
index 5e16d42..7691d09 100644
--- a/packages/davinci-client/src/lib/client.store.ts
+++ b/packages/davinci-client/src/lib/client.store.ts
@@ -52,8 +52,8 @@ import type {
Validator,
Poller,
CollectorValueTypes,
- MetadataError,
} from './client.types.js';
+import type { MetadataError } from './collector.types.js';
import { returnValidator } from './collector.utils.js';
import { returnPasswordPolicyValidator } from './password-policy.rules.js';
import type { ContinueNode, StartNode } from './node.types.js';
diff --git a/packages/davinci-client/src/lib/davinci.types.ts b/packages/davinci-client/src/lib/davinci.types.ts
index 7fd8bfd..8c6460d 100644
--- a/packages/davinci-client/src/lib/davinci.types.ts
+++ b/packages/davinci-client/src/lib/davinci.types.ts
@@ -13,6 +13,7 @@ import type {
FetchBaseQueryMeta,
MutationResultSelectorResult,
} from '@reduxjs/toolkit/query';
+import type { GenericError } from '@forgerock/sdk-types';
import {
DeviceValue,
PhoneNumberInputValue,
@@ -46,7 +47,8 @@ export type DaVinciRequestValueTypes =
| PhoneNumberInputValue
| FidoRegistrationInputValue
| FidoAuthenticationInputValue
- | MetadataError;
+ | MetadataError
+ | GenericError;
/**
* Base Response for DaVinci API
diff --git a/packages/davinci-client/src/lib/utils/metadata.types.ts b/packages/davinci-client/src/lib/utils/metadata.types.ts
index fb2e3d1..fbe1957 100644
--- a/packages/davinci-client/src/lib/utils/metadata.types.ts
+++ b/packages/davinci-client/src/lib/utils/metadata.types.ts
@@ -5,7 +5,4 @@
* of the MIT license. See the LICENSE file for details.
*/
-export interface MetadataError {
- code: string;
- message: string;
-}
+export type { MetadataError } from '../collector.types.js';
diff --git a/packages/davinci-client/src/types.ts b/packages/davinci-client/src/types.ts
index 45427dd..90ead57 100644
--- a/packages/davinci-client/src/types.ts
+++ b/packages/davinci-client/src/types.ts
@@ -21,6 +21,8 @@ export * from './lib/client.types.js';
export * from './lib/node.types.js';
// All collector types (single-value, multi-value, object-value, auto-collectors, etc.)
+// MetadataError is explicitly re-exported to resolve ambiguity with ./lib/utils/utils.types.js
+export type { MetadataError } from './lib/collector.types.js';
export * from './lib/collector.types.js';
// Fido types
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
Or Apply changes locally with:
npx nx-cloud apply-locally aEP6-3Qjh
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
|
Closing in favor of #730 |
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4480
Description
Most of this was taken from #503.
TODO: wire up flow for FIDO errors and write e2e tests
Note: This also fixes a false positive from password validation test and corrects some wiring in the registration node in the flow.