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

Authentication component that tracks user authentication status #45

Merged
merged 21 commits into from
Jul 20, 2020

Conversation

keiffer01
Copy link
Collaborator

@keiffer01 keiffer01 commented Jul 10, 2020

What is a quick description of the change?

Adds components that allows for tracking of the authentication status of the current user, allowing for certain pages to be marked as private.

Is this fixing an issue?

Partially addressed Issue #25.

Are there more details that are relevant?

An Auth directory was created in the components directory, and the following components were made:

  • An AuthProvider component that makes use of React Context to set up the firebase auth listener and allow the current firebase user to be globally accessed. This component was wrapped around the App component's Router.
  • A PrivateRoute component. Acts as a consumer of AuthProvider to determine the status of the current user. If the user is authenticated, this component functions identically to a Route component. If the user is not authenticated, they are redirected to the SignIn page. This component can now be used for any pages where the user needs to be authenticated. See the App/index.js file for usage.

Check lists (check x in [ ] of list items)

  • Test written/updating
  • Tests passing
  • Coding style (indentation, etc)

Any additional comments?

The history module was added to allow tracking of the URL during testing.

A demonstration GIF is given below, where the view-trips and view-activities pages are now marked as PrivateRoutes:
Untitled_ Jul 9, 2020 5_49 PM

@keiffer01 keiffer01 requested review from anan-ya-y and zghera July 10, 2020 16:51
@keiffer01 keiffer01 self-assigned this Jul 10, 2020
Base automatically changed from login-react to master July 13, 2020 16:01
@keiffer01 keiffer01 changed the title Authentication component that tracks user component. Authentication component that tracks user authentication status Jul 13, 2020
@zghera zghera mentioned this pull request Jul 14, 2020
3 tasks
@keiffer01 keiffer01 marked this pull request as ready for review July 14, 2020 17:12
@keiffer01 keiffer01 requested review from anan-ya-y and zghera July 17, 2020 15:23
return (
<Route
{...rest}
render={routeProps =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, ...rest are all the props passed into this component besides RouteComponent so justpath. So then where does routeProps come from? Also why is it render instead of component? Should it have the same props as one like this:

<Route path={ROUTES.SIGN_IN} component={SignInPage} />

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Essentially I just want PrivateRoute to act exactly as a Route component, with the added authentication check. So yeah you have the right idea that ...rest are just all the other props passed into PrivateRoute. I then used a render prop to specify that the component to be rendered is either the RouteComponent, if the user is logged in, or a Redirect component if the user is not logged in. The routeProps are the props that are usually passed into the RouteComponent, such as URL params, etc.

If you think it would be beneficial to comment my code on some of these I can definitely do that!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a brief comment mentioning this would be good. +1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, will do that before merging.

Copy link
Collaborator

@HiramSilvey HiramSilvey left a comment

Choose a reason for hiding this comment

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

Approved with request for a minor comment addition.

return (
<Route
{...rest}
render={routeProps =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a brief comment mentioning this would be good. +1

@keiffer01 keiffer01 merged commit 8b72656 into master Jul 20, 2020
@keiffer01 keiffer01 deleted the authenticate-on-all-pages-react branch July 20, 2020 22:24
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.

4 participants