-
Notifications
You must be signed in to change notification settings - Fork 45
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
Handle 401 unauthorized error in the UI #3640
Handle 401 unauthorized error in the UI #3640
Conversation
Hello chengyanjin,My role is to assist you with the merge of this Status report is not available. |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/2.11/bugfix/ARTESCA-2572-handle-unauthorized-error-in-the-UI origin/development/2.11
$ git merge origin/bugfix/ARTESCA-2572-handle-unauthorized-error-in-the-UI
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/2.11/bugfix/ARTESCA-2572-handle-unauthorized-error-in-the-UI |
@scality/doc Could you please review the wording? |
a6a8e09
to
298082f
Compare
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.
shell-ui/package.json
Outdated
@@ -47,7 +47,7 @@ | |||
"webpack-dev-server": "^3.11.2" | |||
}, | |||
"dependencies": { | |||
"@scality/core-ui": "github:scality/core-ui.git#v0.22.2.1", | |||
"@scality/core-ui": "github:scality/core-ui.git#695c63c", |
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.
do we really want to keep the commit hash rather than deploying a new version ?
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.
Yes. You are right. We should tag a new version for the core-ui.
I am just waiting a bit to see if I can get a review from @scality/doc about the wording in the 401 unauthorized page.
export default function reducer( | ||
state: AuthErrorState = defaultState, | ||
action: AuthErrorAction, | ||
) { | ||
switch (action.type) { | ||
case 'AUTH_ERROR': { | ||
return { ...state, isAuthError: true }; | ||
} | ||
default: | ||
return { ...state }; | ||
} | ||
} |
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'm not familiar with the way it work but what i see from the code, there is no way to remove the isAuthError. And we check in privateRoute.js
if isAuthError is true. Are we sure that if the user relogin the isAuthError is reset ?
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.
Hello @gaudiauj ,
It's a very good concern! but thanks to the authentication workflow, the isAuthError should be reset if we re-login again.
Once the user triggers logout action, the page will be redirected to the IDP, either dex or Keycloak.
Given the page is fully reloaded, the redux store will be re-initiated as well.
0dbba18
to
898aaad
Compare
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
898aaad
to
e43c69f
Compare
History mismatchMerge commit #898aaada770194e5d835311af7a9158a6f77fe24 on the integration branch It is likely due to a rebase of the branch Please use the |
e43c69f
to
4897a2c
Compare
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.
Other than this comment and Jean's comment LGTM
ui/src/services/salt/api.js
Outdated
@@ -113,4 +128,9 @@ export async function getNodesIPsInterfaces( | |||
'ip_interfaces', | |||
], | |||
}); | |||
if (result.error) { | |||
handleUnAuthorizedError({ error: result.error }); |
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 it may need to be returned
ui/src/services/salt/api.js
Outdated
client: 'runner', | ||
fun: 'jobs.print_job', | ||
arg: [jid], | ||
}); | ||
if (result.error) { | ||
handleUnAuthorizedError({ error: result.error }); |
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.
here too
4897a2c
to
370e5c6
Compare
History mismatchMerge commit #2432f52b5feba423da595be39b60efbdb66ec818 on the integration branch It is likely due to a rebase of the branch Please use the |
3d2d869
to
846c8cf
Compare
@ChengYanJin Here's my suggestion for the French part.
@lucieleonard Feel free to suggest something better if you can 😛 @John-VanHook will review the English part 😉 |
The content of the dialog box is not wrong. You could keep it as is. But I suggest this is better: You don’t have permission to view this page using the credentials you have supplied. You can contact |
846c8cf
to
2eb5c9c
Compare
Following the changing in Errorpage from 100vh to 100%, we have to make sure the error page takes the remaining entire height
/status |
StatusStatus report is not available. |
History mismatchMerge commit #2432f52b5feba423da595be39b60efbdb66ec818 on the integration branch It is likely due to a rebase of the branch Please use the |
/reset |
Reset completeI have successfully deleted this pull request's integration branches. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
1 similar comment
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
/status |
StatusStatus report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue ARTESCA-2572. Goodbye chengyanjin. |
Component: UI
Context:
The aim of this PR is to handle correctly the 401 unauthorize error on the UI side.
Summary:
Add Error Reducer in order to dispatch authErrorAction.
We throw out AuthError when we get unauthorized status from Salt API and K8s API, and catch it at the top of reducer.
In the PrivateRoute, we display the
<ErrorPage401/>
if isAuthError is true in the redux-store.Acceptance criteria:
![image](https://user-images.githubusercontent.com/18453133/146037117-5a7a6f2e-2187-41c2-83df-d244e8365b67.png)
![image](https://user-images.githubusercontent.com/18453133/146161057-4177f73b-0a88-4cf1-b443-6332dacad233.png)
EN:
FR:
Core-ui v0.25.0 includes this 401 unauthorized error page.