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

feat: Add sign in/up info in auth URL generated by Passport #320

Merged
merged 11 commits into from
Sep 28, 2018

Conversation

impactmass
Copy link
Contributor

@impactmass impactmass commented Sep 20, 2018

Part of reactioncommerce/reaction/issues/4640
Type: feature

Issue

When a Starterkit user clicks "Register", we redirect to the Reaction Hydra login page, but it shows the "Sign In" view rather than "Create Account" view.

Solution

Pass extra info to Hydra about the specific auth flow the user is going through: Sign up OR Sign in.

Changes

  • Extend Passport configuration to allow passing custom field denoting "signin" or "signup" flow
  • Separate /auth route into two distinct /signin and signup routes to allow requirement above (I couldn't find a way around it with single route)
  • Changes needed on the Identity provider to use above update is done in feat: Show correct form state during create account oauth flow reaction#4651.
  • Update logout function to delete session in Hydra
  • fix: Show logout dropdown onclick Viewer when logged in
  • Also moved splitnames helper into AuthStore, making it available for reuse beyond Header comp

Breaking changes

N/A

Testing

NB: This is to be tested along with reactioncommerce/reaction/pull/4651

  1. From Starterkit, click "Sign In"
  2. Confirm that you see a "Sign In" form on the Meteor Login page
  3. From Starterkit, click "Create Account"
  4. Confirm that you see a "Create Account" form on the Meteor Login page

@impactmass impactmass changed the title WIP - feat: Add signin/up info in auth URL generated by passport WIP - feat: Add sign in/up info in auth URL generated by passport Sep 20, 2018
@impactmass impactmass changed the title WIP - feat: Add sign in/up info in auth URL generated by passport WIP - feat: Add sign in/up info in auth URL generated by Passport Sep 20, 2018
@impactmass impactmass force-pushed the feat-4640-impactmass-idp-form-state branch from 5078185 to d9579d3 Compare September 20, 2018 23:29
@impactmass impactmass changed the title WIP - feat: Add sign in/up info in auth URL generated by Passport feat: Add sign in/up info in auth URL generated by Passport Sep 20, 2018
@impactmass impactmass changed the title feat: Add sign in/up info in auth URL generated by Passport WIP feat: Add sign in/up info in auth URL generated by Passport Sep 21, 2018
@impactmass impactmass force-pushed the feat-4640-impactmass-idp-form-state branch 2 times, most recently from 9385756 to e2dd45c Compare September 24, 2018 13:24
@impactmass impactmass changed the title WIP feat: Add sign in/up info in auth URL generated by Passport feat: Add sign in/up info in auth URL generated by Passport Sep 24, 2018
@impactmass impactmass force-pushed the feat-4640-impactmass-idp-form-state branch from e2dd45c to ee705eb Compare September 24, 2018 14:11
@mikemurray
Copy link
Member

@impactmass It seems that once I've created an account and signed in once. Even if I visit http://localhost:4000/logout to sign out, If I sign in again, I get auto signed in with the last account I signed in with.

  1. From the starter kit, create a new account
  2. Create a new account
  3. On a signed-in starter kit, navigate to http://localhost:4000/logout
  4. Sign in / create account again
  5. It should auto-sign you into your previous account

@aldeed aldeed changed the base branch from master to develop September 24, 2018 20:53
@impactmass
Copy link
Contributor Author

@mikemurray thanks, good call 👍 . I'm checking on it.

This is directly related to having the Hydra sessions working. So when logging out from the Starter kit, there needs to be a call to invalidate the session in Hydra as well. I'll check if it's a 'small' fix that can be added to this PR (or if it needs to be broken out into another).

The reason for Hydra tracking session is here:
https://www.ory.sh/docs/guides/master/hydra/3-overview/1-oauth2#user-login

The endpoint handler at /login must not remember previous sessions. This task is solved by ORY Hydra. If the REST API call tells you to show the login ui, you must show it. If the REST API tells you to not show the login ui, you must not show it. Again, do not implement any type of session here.

@impactmass
Copy link
Contributor Author

Ready for another review

@mikemurray
Copy link
Member

@seun As discussed eariler and just adding it here for reference... When I try to sign out, the app never goes to the logout route, and if I refresh the window, I don’t see the account dropdown at all.

reaction_and_reaction_ docker-compose docker-compose_logs-f_ 279x68_and_slack-_reaction

What I do see is the following warning in the console:

Warning: Did not expect server HTML to contain a <div> in <button>.

@impactmass
Copy link
Contributor Author

@mikemurray I've been looking into this after we discussed, and getting different variations of that error:
Did not expect server HTML to contain a <div> in <button>, Expected server HTML to contain a matching <svg> in <span>. It seems to be an SSR issue that I have yet to figure out.

@nnnnat nnnnat assigned nnnnat and unassigned nnnnat Sep 27, 2018
@nnnnat nnnnat self-requested a review September 27, 2018 15:30
@impactmass impactmass force-pushed the feat-4640-impactmass-idp-form-state branch from 71e2e0c to 6bedc76 Compare September 27, 2018 19:08
@impactmass
Copy link
Contributor Author

This commit fixed the issue with the Authstore & re-rendering after logout:
fix: Update AuthStore to correctly unset account when logged out

@nnnnat
Copy link
Contributor

nnnnat commented Sep 27, 2018

@impactmass I created a merge conflict by approving another PR I'll resolve.

@nnnnat nnnnat removed their request for review September 27, 2018 20:57
Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

When I sign up for a new account I get the following error when I'm redirected back to the starter kit.

Seems to be an issue with splitNames, keeping in mind that a new account has no name, so it will be null.

Cannot read property 'split' of null
TypeError: Cannot read property 'split' of null
    at AuthStore.splitNames (src/lib/stores/AuthStore.js:76:0)
    at AuthStore.setAccount (src/lib/stores/AuthStore.js:89:0)
    at executeAction (/usr/local/src/node_modules/mobx/lib/mobx.js:195:19)
    at AuthStore.res (/usr/local/src/node_modules/mobx/lib/mobx.js:187:16)
    at /usr/local/src/reaction-app/src/.next/server/bundles/pages/_app.js:2901:23
    at Query.render (/usr/local/src/node_modules/react-apollo/react-apollo.umd.js:422:20)
    at processChild (/usr/local/src/node_modules/react-dom/cjs/react-dom-server.node.development.js:2207:18)
    at resolve (/usr/local/src/node_modules/react-dom/cjs/react-dom-server.node.development.js:2064:5)
    at ReactDOMServerRenderer.render (/usr/local/src/node_modules/react-dom/cjs/react-dom-server.node.development.js:2383:22)
    at ReactDOMServerRenderer.read (/usr/local/src/node_modules/react-dom/cjs/react-dom-server.node.development.js:2357:19)
    at renderToString (/usr/local/src/node_modules/react-dom/cjs/react-dom-server.node.development.js:2729:25)
    at Object.renderPage (/usr/local/src/node_modules/next/dist/server/render.js:275:26)
    at Function._class.getInitialProps (src/pages/_document.js:22:0)
    at _callee$ (/usr/local/src/node_modules/next/dist/lib/utils.js:111:30)
    at tryCatch (/usr/local/src/node_modules/regenerator-runtime/runtime.js:62:40)
    at Generator.invoke [as _invoke] (/usr/local/src/node_modules/regenerator-runtime/runtime.js:296:22)
    at Generator.prototype.(anonymous function) [as next] (/usr/local/src/node_modules/regenerator-runtime/runtime.js:114:21)
    at step (/usr/local/src/node_modules/next/node_modules/@babel/runtime/helpers/asyncToGenerator.js:12:30)
    at _next (/usr/local/src/node_modules/next/node_modules/@babel/runtime/helpers/asyncToGenerator.js:27:9)
    at /usr/local/src/node_modules/next/node_modules/@babel/runtime/helpers/asyncToGenerator.js:34:7
    at new Promise (<anonymous>)
    at new F (/usr/local/src/node_modules/core-js/library/modules/_export.js:36:28)

// See https://github.com/reactioncommerce/reaction/issues/4646
splitNames(account) {
const { name } = account;
const [firstName, lastName] = name.split(" ");
Copy link
Contributor

Choose a reason for hiding this comment

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

@mike probably from this?
Maybe something like const [firstName, lastName] = name && name.split(" ");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Will push a fix

Copy link
Contributor

@nnnnat nnnnat left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@nnnnat nnnnat merged commit faba3c3 into develop Sep 28, 2018
@nnnnat nnnnat deleted the feat-4640-impactmass-idp-form-state branch September 28, 2018 00:45
This was referenced Jan 15, 2019
@spencern spencern mentioned this pull request Jan 25, 2019
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