Skip to content

Require signin for Meadow#164

Merged
mbklein merged 1 commit intodeploy/stagingfrom
meadow-auth
Sep 11, 2019
Merged

Require signin for Meadow#164
mbklein merged 1 commit intodeploy/stagingfrom
meadow-auth

Conversation

@kdid
Copy link
Contributor

@kdid kdid commented Sep 10, 2019

  • Using the app or API requires login with NU SSO:
    • Adds "Accounts" context in Meadow for user and groups related functionality
    • Adds AuthController - SSO callback finds or creates user in database and stores some user information in session
    • SetCurrentUser plug intercepts all API requests and checks for user session. Puts the current user into the Absinthe execution context
    • Adds Absinthe Middleware Authenticate which checks for the current user in the context and if found resolves the query.
    • Adds a GetCurrentUser query that returns the currently logged in user
    • Adds RequiresSignIn and CurrentUser components to the front end to guard the app

**Authorization was out of scope for this unit of work. For this PR anyone with a NetId can log in and use Meadow.

@kdid kdid requested review from adamjarling and mbklein September 11, 2019 13:21
@kdid kdid assigned bmquinn and kdid and unassigned bmquinn Sep 11, 2019
@kdid kdid requested a review from bmquinn September 11, 2019 13:21
@kdid
Copy link
Contributor Author

kdid commented Sep 11, 2019

@adamjarling - I added skip to one JS test as I wasn't sure how best to address.

Copy link
Contributor

@adamjarling adamjarling left a comment

Choose a reason for hiding this comment

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

All in all I think this is really good (from the React side) and works as expected. I'd like to merge this in, then take a crack at building off this branch with the following strategy for protected routes, and globally exposing a currentUser object w/o Redux.

<Auth /> component wraps the app, similar to <RequireSignIn />, and uses/sets/reads a React Context auth/currentUser variable which can be accessed throughout the app.

};

export default CurrentUser;
export { GET_CURRENT_USER_QUERY };
Copy link
Contributor

Choose a reason for hiding this comment

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

What about creating a /assets/js/components/Auth directory, where we can organize Auth related components (CurrentUser, RequireSignIn) and queries? Then we could create .../Auth/auth.query.js or something and avoid exporting from individual components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I think that's a good idea. Some of that stuff didn't seem to fit anywhere so I just threw it somewhere. Let me see if BMQ and MBK have comments and I'll change at same time.

import ContentWrapper from "../components/UI/ContentWrapper";

const redirectToLogin = event => {
event.preventDefault;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might not need this if not a link or form submit action.

Copy link
Contributor

@mbklein mbklein left a comment

Choose a reason for hiding this comment

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

Looks great except for one small comment.

@mbklein mbklein merged commit 9233dca into deploy/staging Sep 11, 2019
@mbklein mbklein deleted the meadow-auth branch September 11, 2019 19:22
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