LearnersGuild / idm

identity management service
MIT License
2 stars 24 forks source link

redirect after sign-up #88

Closed jeffreywescott closed 8 years ago

jeffreywescott commented 8 years ago

Fixes: #86

Because we're using an OAuth-based authentication via GitHub, up until now, the redirect and responseType parameters have been ignored when the user switches from a sign-in-based authentication flow to a sign-up-based flow.

This pull request makes sure that those parameters get properly propagated when switching from sign-in to sign-up such that:

  1. If there is a redirect query string parameter, the user will be redirected to that URL after the sign-up process is completed, and
  2. If there is a responseType=token query string parameter, the lgJWT token will be passed-back when redirecting (which is needed if redirecting to services like the chat service that cannot easily detect the lgJWT cookie).

Most of the changes are pretty straight forward to review, but you'll need an understanding of mapStateToProps, mapDispatchToProps, and mergeProps in redux to review the code in common/containers/UserForm.jsx, since those functions are used by reduxForm.

heyheyjp commented 8 years ago

@jeffreywescott: I still find parts of the sign up flow confusing. Apologies if these tensions are intended to be addressed in another issue/PR. Please point me in the right direction if so...

Issue 1 What do we mean here by "sign up"? There seem to be 2 different kinds of signing up in play:

When I see this...

screen shot 2016-06-08 at 10 17 33 pm

...as someone totally new...

SIGN-IN USING GITHUB

...followed by...

Don't have an account? Sign-up.

...gives me the impression that by clicking the sign-up link, I'll be directed to set up a github account.

Instead, I'm directed to enter an invitation code. This is confusing.

Issue 2 I wasn't redirected to the idm service after setting up a brand new github account because of a requirement that email addresses for new accounts be verified before the user is allowed to authorize an OAuth application.

screen shot 2016-06-08 at 10 28 43 pm

But it isn't totally clear to me that this is unintended behavior by the original issue description. Could use some clarification on what "signing up" means.

Other than that...had some comments above re: the flow of data through the app + redux and where it's most appropriate to interface directly with the dispatcher. Nothing urgent, but worth discussing.

Code looks pretty good otherwise!

jeffreywescott commented 8 years ago

Great feedback, @prattsj. I created another issue, #92, to track your thoughts about the flow so that we can improve things.

I'll address the action creator stuff and the name validation and then re-assign to you for re-review. I'd like to get this merged today if at all possible, so we can give @shereefb the thumbs-up on inviting learners into our chat service.

heyheyjp commented 8 years ago

👍 Cool. The action creator stuff isn't a blocking issue -- I think it might even best be deferred and tackled across all repos at once to establish a new convention. As long as this works as intended, I'm OK with merging the code!

jeffreywescott commented 8 years ago

Hey, @prattsj -- I addressed the two main issues you brought up with the code. Mind giving this a re-review when you get a chance?

heyheyjp commented 8 years ago

Everything discussed looks to have either been addressed here or w/ new issues, so LGTM! 👍