-
Notifications
You must be signed in to change notification settings - Fork 3
fix(journey-client): classify HTTP 4xx error bodies as LoginFailure #541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| '@forgerock/journey-client': patch | ||
| --- | ||
|
|
||
| Fix social login error handling in journey-client | ||
|
|
||
| - Fix `start()` and `next()` to extract AM error bodies from RTK Query's `error.data` slot, properly classifying HTTP 4xx responses (e.g. 401 from failed social login) as `JourneyLoginFailure` instead of generic `no_response_data` errors | ||
| - Fix `resume()` to return typed `GenericError` values instead of throwing raw `Error` objects, maintaining the `JourneyResult` contract |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,21 @@ import { NextOptions, StartParam, ResumeOptions } from './interfaces.js'; | |
| import type { JourneyLoginFailure } from './login-failure.utils.js'; | ||
| import type { JourneyLoginSuccess } from './login-success.utils.js'; | ||
|
|
||
| /** | ||
| * Checks whether an unknown value resembles an AM Step response. | ||
| * RTK Query's fetchBaseQuery places HTTP error response bodies in `error.data`. | ||
| * When AM returns a 4xx (e.g. 401 for failed social login), the body contains | ||
| * `code`, `reason`, and `message` — which is a valid Step that should be | ||
| * classified via `createJourneyObject()`. | ||
| */ | ||
| function isStepLike(data: unknown): data is Step { | ||
| return ( | ||
| typeof data === 'object' && | ||
| data !== null && | ||
| ('authId' in data || 'code' in data || 'callbacks' in data) | ||
| ); | ||
|
Comment on lines
+42
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restrict Line 157 and Line 178 currently convert any Step-like 💡 Proposed fix function isStepLike(data: unknown): data is Step {
return (
typeof data === 'object' &&
data !== null &&
('authId' in data || 'code' in data || 'callbacks' in data)
);
}
+
+function isClientStepError(queryError: unknown): queryError is { status: number; data: Step } {
+ return (
+ typeof queryError === 'object' &&
+ queryError !== null &&
+ 'status' in queryError &&
+ typeof (queryError as { status: unknown }).status === 'number' &&
+ (queryError as { status: number }).status >= 400 &&
+ (queryError as { status: number }).status < 500 &&
+ 'data' in queryError &&
+ isStepLike((queryError as { data: unknown }).data)
+ );
+}
@@
- if (queryError && 'data' in queryError && isStepLike(queryError.data)) {
- return createJourneyObject(queryError.data as Step);
+ if (isClientStepError(queryError)) {
+ return createJourneyObject(queryError.data);
}
@@
- if (queryError && 'data' in queryError && isStepLike(queryError.data)) {
- return createJourneyObject(queryError.data as Step);
+ if (isClientStepError(queryError)) {
+ return createJourneyObject(queryError.data);
}Also applies to: 157-159, 178-180 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| /** Result type for journey client methods. */ | ||
| type JourneyResult = | ||
| | JourneyStep | ||
|
|
@@ -135,8 +150,13 @@ export async function journey({ | |
|
|
||
| const self: JourneyClient = { | ||
| start: async (options?: StartParam) => { | ||
| const { data } = await store.dispatch(journeyApi.endpoints.start.initiate(options)); | ||
| const { data, error: queryError } = await store.dispatch( | ||
| journeyApi.endpoints.start.initiate(options), | ||
| ); | ||
| if (!data) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we can, I'd rather push this responsibility to the RTK API module that actually makes the handles the request. You can see that here in the DaVinci Client: https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/davinci-client/src/lib/davinci.api.ts#L126. This helps keep the top-level store file much leaner. |
||
| if (queryError && 'data' in queryError && isStepLike(queryError.data)) { | ||
| return createJourneyObject(queryError.data as Step); | ||
| } | ||
| const error: GenericError = { | ||
| error: 'no_response_data', | ||
| message: 'No data received from server when starting journey', | ||
|
|
@@ -151,8 +171,13 @@ export async function journey({ | |
| * Submits the current Step payload to the authentication API and retrieves the next JourneyStep in the journey. | ||
| */ | ||
| next: async (step: JourneyStep, options?: NextOptions) => { | ||
| const { data } = await store.dispatch(journeyApi.endpoints.next.initiate({ step, options })); | ||
| const { data, error: queryError } = await store.dispatch( | ||
| journeyApi.endpoints.next.initiate({ step, options }), | ||
| ); | ||
| if (!data) { | ||
| if (queryError && 'data' in queryError && isStepLike(queryError.data)) { | ||
| return createJourneyObject(queryError.data as Step); | ||
| } | ||
| const error: GenericError = { | ||
| error: 'no_response_data', | ||
| message: 'No data received from server when submitting step', | ||
|
|
@@ -210,17 +235,23 @@ export async function journey({ | |
|
|
||
| if (stored) { | ||
| if (isGenericError(stored)) { | ||
| throw new Error(`Error retrieving previous step: ${stored.message || stored.error}`); | ||
| return { | ||
| error: 'storage_error', | ||
| message: `Error retrieving previous step: ${stored.message || stored.error}`, | ||
| type: 'unknown_error', | ||
| } satisfies GenericError; | ||
| } else if (isStoredStep(stored)) { | ||
| previousStep = createJourneyObject(stored.step) as JourneyStep; | ||
| } | ||
| } | ||
| await stepStorage.remove(); | ||
|
|
||
| if (!previousStep) { | ||
| throw new Error( | ||
| 'Error: previous step information not found in storage for resume operation.', | ||
| ); | ||
| return { | ||
| error: 'missing_previous_step', | ||
| message: 'Previous step information not found in storage for resume operation.', | ||
| type: 'state_error', | ||
| } satisfies GenericError; | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the order of imports? Here's the pattern I'd like us to use: