-
Notifications
You must be signed in to change notification settings - Fork 7
fix(PN-15723): Handle user validation failure after token-exchange #1687
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
base: develop
Are you sure you want to change the base?
Conversation
AndreaCimini90
left a comment
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.
Remember to write the test case for this scenario.
Make the changes also for pg and pa
| @@ -0,0 +1,46 @@ | |||
| import * as React from 'react'; | |||
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.
Rename the file and function with FeedbackPage.
Remember to write test
| @@ -0,0 +1,26 @@ | |||
| /* eslint-disable functional/immutable-data */ | |||
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.
If userDataMatcher is used only in one place, we can avoid to create a new file and move the content into the file where we are using it
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.
userDataMatcher is used both in auth/reducers.ts and in auth/actions.ts
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.
You are right! Can you move this file under utility directory and rename it with user.utility.ts?
| title={getLocalizedOrDefaultLabel( | ||
| 'common', | ||
| 'user-validation-failed.title', | ||
| 'Validazione utente fallita' |
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.
remove the third parameter from the getLocalizedOrDefaultLabel
| } | ||
|
|
||
| export function adaptedTokenExchangeError(originalError: any) { | ||
| // validazione `user` TokenExchangeRequest fallisce |
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.
Write this in english
| isUnauthorizedUser: true, | ||
| response: { | ||
| ...originalError.response, | ||
| status: 499, |
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.
499 stands for?
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.
It's a non-standard client-defined HTTP status code, so it's available for custom application-level error handling.
| element={<AppNotAccessible onAssistanceClick={handleAssistanceClick} />} | ||
| element={<AppNotAccessible onAssistanceClick={handleGoToLandingSite} />} | ||
| /> | ||
| <Route |
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.
Maybe we can avoid to define another route and use the not_accessible one. Also in this case the app is not accessible. To distinguish between the two cases, you can pass a parameter (the error code can be enough) and do the logic inside the component.
In this way the UserValidationFailed component will be useless.
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.
I've refactored the code so that we now handle the two cases with two separate routes as before, both pointing to the same AppNotAccessible component (deleting UserValidationFailed) using a prop to distinguish them.
If you really meant using a single route for both cases I can update it and switch to a query-string parameter to discriminate between them. Just let me know which approach you prefer.
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.
I think that the single route is the best approach
| state.loading = false; | ||
| } | ||
| state.user = action.payload; | ||
| state.loading = false; |
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.
I should restore the sessionStorage.setItem('user', JSON.stringify(user)); code removing it from the sessionGuard
Short description
This PR introduces error handling for the case when the user object returned from the token-exchange API fails schema validation.
Instead of silently redirect the user to the login page, we now display a dedicated page that informs the user about the validation failure. A new reusable ThankYouPage component has also been introduced following the design guidelines linked into the Jira issue.
How to test
This way you should be redirected to the "User validation failed" page