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.

Sign up for GitHub

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

Jump to bottom

Authentication Fixes #170

Merged
merged 5 commits into from
Oct 31, 2022
Merged

Authentication Fixes #170

merged 5 commits into from
Oct 31, 2022

Conversation

mstone121
Copy link
Contributor

@mstone121 mstone121 commented Oct 28, 2022 •
edited
Loading

Overview

This PR does mainly three things:

  • Prevents the app from rendering without an authenticated user
  • Prevents non-contributors from getting to the welcome page
  • Prevents contributors from accessing the app without selecting a utility

Closes #168
Closes #78
Closes #149
Closes #164

Demo

Screen.Recording.2022-10-28.at.3.59.24.PM.mov

Notes

The private routes are moved into an auth guard which prevents render of children until the user is authenticated.

The utility guard might need to moved into the private routes element and surround only certain elements. For example, if a user visits a submission details page, they'll still have to select a utility.

Testing Instructions

  • Go to http://localhost:4545
  • Log in
  • Navigate around
  • Refresh a page, a center spinner should be visible breifly before the app loads
  • You should not have had to log in again.
  • Log out
  • Go to a private page
  • You should be redirected to the login page

Checklist

  • fixup! commits have been squashed
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines
  • README.md updated if necessary to reflect the changes
  • CI passes after rebase

@rajadain
Copy link
Contributor

Taking a look now

@rajadain rajadain self-requested a review October 30, 2022 21:37
@rajadain rajadain self-assigned this Oct 30, 2022
rajadain
rajadain requested changes Oct 30, 2022
View reviewed changes
Copy link
Contributor

@rajadain rajadain left a comment

Choose a reason for hiding this comment

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

This seems to break the "redirect to originally requested private route after login" functionality, as demoed here:

2022-10-30.17.54.32.mp4

I should be redirected to /submissions/4, but instead I go to /welcome and then /draw/1.

@mstone121 mstone121 force-pushed the ms/prevent-render-before-login branch from d6b5164 to 8783b03 Compare October 31, 2022 18:58
@mstone121
Copy link
Contributor Author

mstone121 commented Oct 31, 2022 •
edited
Loading

This seems to break the "redirect to originally requested private route after login" functionality, as demoed here:

The code to fix this ended up being mixed with the utility selector so I assigned myself #164 and included the fixes here. I added a utility guard much like the auth guard (but that does not redirect).

We might need to think about moving this further into the route tree. If a user visits a submission details page, they'll still have to select a utility. Perhaps if they visit a submission details page, we should auto select the utility based on the submission.

@rajadain
Copy link
Contributor

Taking another look. I made #172 as a follow-up which may need the NavBar to be moved to be within the BoundaryProvider.

rajadain
rajadain approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@rajadain rajadain left a comment

Choose a reason for hiding this comment

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

+1 tested, nice work on this!

@rajadain rajadain assigned mstone121 and unassigned rajadain Oct 31, 2022
@mstone121
Copy link
Contributor Author

mstone121 commented Oct 31, 2022 •
edited
Loading

Taking another look. I made #172 as a follow-up which may need the NavBar to be moved to be within the BoundaryProvider.

Another possibility to move the utility guard into PrivateRoutes so that it encapsulates only the routes that have multiple boundary possibilities. The others will set the utility themselves.

Matt Stone added 5 commits October 31, 2022 15:49
Prevent render before login
f362492
Show welcome page to only contributors
6b2ac41 
This commit moves the private routes into their own component beneath
and AuthGuard so they can access the user in the store.
Remove unnecessary imports
c59d02e
Update utility selector
27f15fd 
This updates the utility selection process by adding a utility guard.
This guard ensures that the app can't be reached without having selected
a utility (if you are a contributor).

This should handle the case where a user visits a page by directly
entering a URL (or refreshing) and is already logged in.
Update CHANGELOG.md
c06b947
@mstone121 mstone121 force-pushed the ms/prevent-render-before-login branch from 8783b03 to c06b947 Compare October 31, 2022 20:49
@mstone121 mstone121 merged commit c6f6e4c into develop Oct 31, 2022
@mstone121 mstone121 deleted the ms/prevent-render-before-login branch October 31, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajadain rajadain rajadain approved these changes

Assignees

@mstone121 mstone121

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mstone121 @rajadain