Skip to content
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

Add dependency to redirect unauthenticated users to login and add webpages to display error responses. #26

Merged
merged 11 commits into from Jul 24, 2022

Conversation

kuries
Copy link
Member

@kuries kuries commented Jun 23, 2022

Parent: #25

  1. Redirect unauthorized users to /login endpoint.
  2. Add extra web pages to handle HTTP 404 errors and responses from external URL invites.
  • I added a dependency in the backend which acts as middleware and checks if the request has a valid session ID. If it doesn't, then it returns a "401 unauthorized request" as a response.

  • To handle this on the client-side I created an Axios interceptor which checks for a 401 error in all the incoming responses. If present, it'll redirect the user to the login page.

Demo pages for 404 and External URL response messages:

error-404

error-404.png

image

External Url invite message.

@kuries kuries changed the title Add additional webpages to handle 404 Error and external url invite response. Additional webpages to handle 404 Error and external url invite response. Jun 23, 2022
@kuries kuries changed the title Additional webpages to handle 404 Error and external url invite response. Support for protected routes and Add additional webpages to handle 404 Error and external url invite response. Jun 23, 2022
@kuries kuries changed the title Support for protected routes and Add additional webpages to handle 404 Error and external url invite response. Support for protected routes and add additional webpages to handle 404 Error and external url invite response. Jun 23, 2022
@kuries kuries marked this pull request as ready for review June 23, 2022 19:29
@kuries kuries requested review from Cadair and Half-Shot June 23, 2022 19:29
@kuries kuries force-pushed the pages-without-navbar branch 2 times, most recently from 0a92a5b to b7720a9 Compare June 25, 2022 19:33
@kuries kuries changed the title Support for protected routes and add additional webpages to handle 404 Error and external url invite response. Add dependency to handle unauthenticated users and add webpages to handle 404 Error and external url invite responses. Jun 25, 2022
@kuries kuries changed the title Add dependency to handle unauthenticated users and add webpages to handle 404 Error and external url invite responses. Add dependency to redirect unauthenticated users to login and add webpages to display error responses. Jun 25, 2022
@kuries
Copy link
Member Author

kuries commented Jun 27, 2022

To redirect users on the login page we can do it in two ways.

  1. A simple and easier way is to replace the window URL directly by changing window.location.href. This leads to a full page reload which might be against "SPA" nature.

  2. A little more complex way is to create a custom router and supply a history object to it. This way we can pass this newly created history object to the axios interceptor and directly manipulate the custom router without triggering a full page reload. But this adds some unnecessary code which might possibly break in the future (not sure).
    Reference: https://stackoverflow.com/a/70012117

@Half-Shot @Cadair
Can you suggest which might be a good option in the long run?

Comment on lines 11 to 13
if (axios.isAxiosError(er)) {
if (er.response) {
if (er.response.status == 401) {
history.replace("/login");
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saves you a bit of nesting. You can use foo?.bar to say if foo is undefined, don't try to access bar. Very handy for these kinds of logic.

Suggested change
if (axios.isAxiosError(er)) {
if (er.response) {
if (er.response.status == 401) {
history.replace("/login");
}
}
}
if (axios.isAxiosError(er) && er.response?.status == 401) {
history.replace("/login");
}

Copy link
Member Author

@kuries kuries Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks.

return response;
},
function (er) {
if (axios.isAxiosError(er)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if this is a good thing to explicitly log, so we know why we've been navigated away?

</div>
</div>
<div>
<img src="https://i.ibb.co/ck1SGFJ/Group.png" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spot another cheeky remote asset :p

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :D

if(loginToken == null){
throw new Error("Invalid login token.");
}
fetchData();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always await these.

Suggested change
fetchData();
await fetchData();

@kuries kuries force-pushed the feature_external-url branch 2 times, most recently from f1cad3b to 992b84b Compare June 27, 2022 12:58
…nticate_user` dependency.

Unauthenticated users are redirected to login page (full reload of SPA occurs). Sessions return a null value when session_id is not present in redis.
Update client-side API calls and backend tests to handle url changes.
These interceptors get triggered on every response and if the status code is 401, the user is redirected to login.
All axios imports are replaced with a custom axios instance.
Copy link

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kuries kuries changed the base branch from feature_external-url to main July 15, 2022 18:18
@Cadair Cadair merged commit 4f3dac7 into main Jul 24, 2022
@kuries kuries deleted the pages-without-navbar branch September 7, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants