IS-11347 Unify completed-with-success and completed-with-error handling#196
Conversation
Merges both terminal OAuth response steps under a single `handleCompletedStep` with one config knob: `autoRedirectOnAuthenticationComplete`. Default `true` (auto-redirect both); accepts `false`, `'ONLY_ON_SUCCESS'`, `'ONLY_ON_ERROR'` for finer control. Replaces `redirectOnAuthenticationCompletedWithSuccess`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
luisgoncalves
left a comment
luisgoncalves
left a comment
There was a problem hiding this comment.
Tested locally and LGTM 👍
Added a couple of notes I think are relevant.
| * For further information, please contact Curity AB. | ||
| */ | ||
|
|
||
| export enum AUTO_REDIRECT_ON_AUTHENTICATION_COMPLETE { |
There was a problem hiding this comment.
This feels so YAGNI. I don't think we should be adding more things this late in the game, namely when there isn't any specific requirement.
Even the config boolean is debatable, because on regular full-page flows the server simply redirects to the client app - there's no way to control that. But the boolean was already there, so I guess it's fine.
Note that OAuth clear defines the meaning of authorization responses. If there is an error authorization response, the protocol contains error codes et al, so the client application is expected to somehow inform the user. Having a setting to control this in the library/LWA, means that the user could have an additional interaction here, then something else in the client app. It's really hard to control this in a cross-cutting place like this library/app in a way that works well for all cases. So, this can actually be error prone, leading to worst UX.
Anyway, I suggest not adding this new thing for now.
There was a problem hiding this comment.
The boolean has a concrete use case — when the LWA is embedded inside a third-party app (no separate OAuth client to redirect back to), the consumer needs to suppress the auto-redirect and consume the completed step as data. It also applies to the completed with error step, but you are right, there is no need for handling them differently. The enum and related logic are removed now ✅
| }); | ||
|
|
||
| describe('Authentication / Registration Step', () => { | ||
| describe('WebAuthn Auto-Start', () => { |
There was a problem hiding this comment.
The diff is a bit confusing, but looks like some webauthn tests may have been lost here? Or are they defined somewhere else now?
There was a problem hiding this comment.
Pull request overview
Unifies HAAPI “completed” step handling by routing both completed-with-success and completed-with-error through a single handleCompletedStep and introducing the new autoRedirectOnAuthenticationComplete config to control redirect vs. exposing nextStepData.
Changes:
- Added
handleCompletedStepand updated the stepper to use it for both completed step types. - Introduced
autoRedirectOnAuthenticationComplete(boolean orAUTO_REDIRECT_ON_AUTHENTICATION_COMPLETE) and removed the previous success-only redirect config. - Updated fixtures and tests to cover redirect/render behavior for both completed-with-success and completed-with-error.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/login-web-app/src/shared/util/api-responses.ts | Adds a completed-with-error fixture that includes an authorization-response link for redirect testing. |
| src/login-web-app/src/haapi-stepper/feature/stepper/typings.ts | Adds AUTO_REDIRECT_ON_AUTHENTICATION_COMPLETE enum values for config. |
| src/login-web-app/src/haapi-stepper/feature/stepper/step-handlers/completed-with-success-step.ts | Removes old success-only completed-step handler. |
| src/login-web-app/src/haapi-stepper/feature/stepper/step-handlers/completed-step.ts | Adds unified completed-step handler with redirect logic based on new config. |
| src/login-web-app/src/haapi-stepper/feature/stepper/HaapiStepper.tsx | Switches to unified handler and replaces old config knob with autoRedirectOnAuthenticationComplete. |
| src/login-web-app/src/haapi-stepper/feature/stepper/HaapiStepper.spec.tsx | Updates tests to cover redirect/render behavior for both completed success and error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { formatNextStepData } from './data-formatters/format-next-step-data'; | ||
| import { handleCompletedWithSuccessStep } from './step-handlers/completed-with-success-step'; | ||
| import { handleCompletedStep } from './step-handlers/completed-step'; | ||
| import { AUTO_REDIRECT_ON_AUTHENTICATION_COMPLETE } from './typings'; |
| describe.each([ | ||
| { | ||
| label: 'success', | ||
| stepType: HAAPI_STEPS.COMPLETED_WITH_SUCCESS, | ||
| stepFixture: completedWithSuccessStep, | ||
| }, | ||
| { | ||
| label: 'error', | ||
| stepType: HAAPI_PROBLEM_STEPS.COMPLETED_WITH_ERROR, | ||
| stepFixture: completedWithErrorStep, | ||
| }, | ||
| ] as const)('Completed With $label Step', ({ label, stepType, stepFixture }) => { |
…lean. Per code review: the ONLY_ON_SUCCESS / ONLY_ON_ERROR asymmetric gates were speculative — no concrete consumer needs per-side control. Reduce the public config surface to plain boolean, defer the enum until requested. - Delete typings.ts (only held the enum). - handleCompletedStep simplifies to a single early-return on !autoRedirect. - Drop the two ONLY_ON_* test cases per completed-step iteration in the spec (12 → 8 cases for the autoRedirectOnAuthenticationComplete block).
Jira: https://curity.atlassian.net/browse/IS-11347
Summary
Unifies
completed-with-successandcompleted-with-errorHAAPI step handling under a singlehandleCompletedStepwith one config knob —autoRedirectOnAuthenticationComplete. Previously the LWA auto-redirected on success but exposed errors asnextStepData, even though both server-side step shapes carry anauthorization-responselink and the Velocity-rendered UI auto-redirects in both cases.Config
autoRedirectOnAuthenticationComplete: boolean | AUTO_REDIRECT_ON_AUTHENTICATION_COMPLETE(defaulttrue).true→ redirect on both success and errorfalse→ expose the completed step asnextStepDatafor both'ONLY_ON_SUCCESS'→ redirect on success, expose on error'ONLY_ON_ERROR'→ redirect on error, expose on successBreaking rename of
redirectOnAuthenticationCompletedWithSuccess— safe because the LWA is still pre-1.0 and has no external consumers.Test plan
completed-with-error(e.g. unknownclient_id) auto-redirects under default configautoRedirectOnAuthenticationComplete: 'ONLY_ON_SUCCESS'causes the error step to surface asnextStepData