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

Use OAuth flows for login, reg, logout, change password #171

Merged
merged 7 commits into from
Jan 14, 2020

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Jan 6, 2020

Resolves #81
Impact: breaking
Type: feature

Changes

  • Login, logout, registration, and password change now go through Hydra, using OAuth2 Authorize Code Flow + PKCE with a public Hydra client
  • The Hydra client is created or updated automatically when the server starts
  • All GraphQL requests now set the Authorization header with the Hydra access token instead of using the custom meteor-login-token header.
  • A custom Meteor Accounts login handler synchronizes Meteor DDP login with Hydra OAuth login.
  • Removed lots of components that were related to the old Meteor auth system

If the above is confusing to you, essentially nothing about the user experience has changed except that you'll notice that you're over on the Reaction Identity URL whenever you are logging in, registering, or changing your password.

Breaking changes

Reaction Admin UI no longer works if you don't also have Hydra and Reaction Identity services configured and running on the Docker network. This involves some new environment variables that are required (but are set correctly automatically for local development if you run bin/setup).

Testing

Be sure to run bin/setup after pulling, and make sure you have the latest commit on the reaction-hydra 3.0.0 branch and the latest commit on the reaction-identity trunk branch both running.

Specifically, make sure your reaction-hydra .env file has the two new variables set:

SERVE_PUBLIC_CORS_ALLOWED_ORIGINS=http://localhost:4080
SERVE_PUBLIC_CORS_ENABLED=true

Then verify all registration, login, and logout flows you can think of. Test the "Change Password" link on the profile page (click Profile in user menu).

If you log in as a non-admin user, you should be immediately logged out. If you're not logged in when you visit the Reaction Admin URL, it should kick you out to the login page.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@mikemurray
Copy link
Member

@aldeed Started testing and I'm having some success and some issues:

  • With an existing admin user and shop, I can sign in, and sign out
  • With an existing customer account, signing causes an infinite redirect loop
  • With a fresh database, creating the first user causes an infinite redirect loop

@aldeed
Copy link
Contributor Author

aldeed commented Jan 7, 2020

@mikemurray For the first user, use this Reaction Identity branch: reactioncommerce/reaction#6002 (comment) I had forgotten to PR the change I made there.

Not sure about existing customer account issue. When you sign in it should bring back to admin, and then admin should kick you back to the login form and it should stop there. I'll retest.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed
Copy link
Contributor Author

aldeed commented Jan 7, 2020

@mikemurray This should all be working now. You will need to test with this API branch and this Identity branch

@janus-reith
Copy link
Contributor

janus-reith commented Jan 8, 2020

Technically this works great so far, normal users don't get dashboard access, login as owner works.
GraphQL calls and the Meteor Collection subscriptions both still work.

What is just weird is the redirect url I get: http://localhost:4080/?state=
Ideally when not logged in and e.g. navigating to /orders i would expect to land there after signin.

Something else that may be improved: Each time i close the Tab and enter again while still authenticated, the page does a redirect to hydra, therefore increasing the time until the admin is usable.
I wonder if this is necessary, as the storefront doesn't need this - I guess the difference might be that the storefront validates on its own server while this is a client-only?

@janus-reith
Copy link
Contributor

janus-reith commented Jan 8, 2020

Also it seems like the storefront hydra client and admin hydra client registrations conflict.
Only happens on a deployed test with subdomains, but not locally.
Works by settings individual OAUTH2_CLIENT_ID envs.

@aldeed
Copy link
Contributor Author

aldeed commented Jan 8, 2020

@janus-reith The @axa-fr/react-oidc-context NPM package, which wraps https://github.com/IdentityModel/oidc-client-js, handles everything, as Hydra folks recommend, so there isn't much that can be done beyond what is configurable in those packages.

The ?state= URL should appear only briefly if everything is working correctly, and then you should be back on whatever page you originally landed on logged out. That, too, is entirely a @axa-fr/react-oidc-context package thing, but it was working for me. It's a two-step redirect. That's the PKCE part I believe.

You're correct that this is different from storefront in that it's being done as an SPA. Although there is currently a Meteor backend, we expect that this project will likely evolve into SPA after Meteor is removed. There is no need for SSR as there is on the storefront side.

That said, I think @axa-fr/react-oidc-context docs said that the default is to use session storage for the user, so maybe by customizing it to use local storage, it would remember you for longer without a full redirect flow.

For a good summary of the different OAuth types, see https://www.ory.sh/oauth2-for-mobile-app-spa-browser/. What we are doing here is described in the "What To Do Instead" section.

I'm not sure what you mean about the client ID conflicts. The OAUTH2_CLIENT_ID default for this project is "reaction-admin" and for storefront the default is "example-storefront".

@janus-reith
Copy link
Contributor

@aldeed Thanks, I read exactly this article some months ago when trying to get a better grasp of how oauth2 works.
Regarding the client ID, you're right , I just noted the default value is set in config.js. When testing, I had both containers share some env and where this was overwritten.

Also, I somehow can't reproduce the cases where I was redirected to only "?state=", it now works for both initial signups and those redirects happening after visiting again - Will keep an eye on that and report back if it happens again.

But one minor issue I can still reproduce:

  1. Make sure both backend and client are logged out.
  2. Open backend, it will redirect to auth, leave it open.
  3. On the Storefront, start the login flow, log in.
  4. Back on the Backend Login Tab, login aswell and you are redirected to an empty page:
    http://BACKENDURL/authentication/callback?error=request_forbidden&error_debug=The+CSRF+value+from+the+token+does+not+match+the+CSRF+value+from+the+data+store&error_description=The+request+is+not+allowed&error_hint=You+are+not+allowed+to+perform+this+action.&state=3562a07d9bb74f148e2e87a204592eaa
  5. Open the backend again, and you are logged in there aswell.

While probably not that common, these cases occur.
We probably don't need to alter behavior of hydra here, but maybe on /authentication/callback we could check for ?error=request_forbidden and redirect to the start page, which would then automatically trigger the reauthentication.

@aldeed
Copy link
Contributor Author

aldeed commented Jan 8, 2020

@janus-reith For that issue, are you logging in as the same user in both? Is it an admin or customer user for each?

@janus-reith
Copy link
Contributor

@aldeed Same admin user.

@janus-reith
Copy link
Contributor

janus-reith commented Jan 9, 2020

Here is a quick example that fixes this:
imports/client/ui/startup.js:

function NotAuthenticated() {
  return (
    <Fragment>
      <Redirect to="/" />
      <Typography>Not Authenticated, redirecting...</Typography>
      <Typography>Click <Link to="/">here</Link> if you are not automatically redirected.</Typography>
    </Fragment>
  )
}
const oidcProps = {
  [...]
  notAuthenticated: () => <NotAuthenticated />,
  [...]
};

I used Redirect and Link from react-router-dom
Not sure if the Text is actually needed, in my test cases it redirected before it displays.
Auto-Redirects should work as there was previous user input, but probably doesn't hurt to catch edge cases with the manual link.

With this in place, I can have Login Flows open both for backend and frontend, and when completing the backend flow after the frontend flow the backend will redirect again and log me in

The same needs to be done for the frontend on the http://localhost:4000/callback route so express redirects us, to fix cases where we start the login flow in the storefront first, finish a separate one for the backend and then try to complete the one for the frontend.
In the storefront, src/serverAuth.js I tried to add failureRedirect as option to the uses of passport.authenticate, but somehow this was ignored. I tried to grab the error from passport and redirect on Error, somehow that also didn't work - I did not investigate further yet.

@aldeed
Copy link
Contributor Author

aldeed commented Jan 9, 2020

@janus-reith Thanks. I'm wondering if this might be a Hydra bug that you are working around. It does seem like an easy workaround that doesn't hurt (at least on the admin side), but I don't fully understand why a user's login flow for one client would interrupt their login flow for another client. All challenges and tokens should be per-client.

@aldeed
Copy link
Contributor Author

aldeed commented Jan 9, 2020

@janus-reith Since this last issue is an edge case that needs more investigation and thought, and the rest of this works, I moved your comments to a new issue here: #178 Post any further discussion there.

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.

@aldeed Issues I'm seeing now

Admin

  • After creating the first user, I'm redirected to the admin, but to the main area. I don't see the create shop form. If I refresh, then the form shows up.
  • Signing in as an admin doesn't show the default page: This SS is what I see for both cases.
    image
  • Signing-out doesn't work if I attempt to sign out from the Accounts page. The URL for that page is /accounts so that might be the problem. If the refresh wheel on the same page, I'm redirected back to the blank homepage, and If I sign out there, then it works. Even though the address bar shows http://localhost:4080/accounts
  • Reset password works, doesn't redirect on success. Not sure if it's supposed to, but this could be a ticket in the reaction-identity project if it should.

The first 2 issues are probably the router/react component not getting the route change notification. Maybe there's a replace instead of a push, or missing withRouter decorators / useRouter hooks.

The last one is probably due to the fact what we had a lot of pathname.startWith("/accounts") to handle special cases. Maybe renaming the page to /manage-accounts would fix it.

Customer

  • If I register as a customer all seems to work, but I get a flash of the admin and then signed out. While no information is shown, it's would be a cleaner flow if that didn't happen. This isn't terribly critical for this release and could be another ticket.

Otherwise, things seem to works while working around these minor flow hiccups.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed
Copy link
Contributor Author

aldeed commented Jan 11, 2020

@mikemurray I added a workaround for the routing issue we discussed and simplified useAuth. I also found a way to reset Apollo entirely whenever the access token changes.

ef8d836

The workaround is a bit icky, but it works every time for me, in every browser. Everything else I tried didn't work.

One result of my changes is that I'm no longer kicking "customer" users back to the login form. If you log in as a customer user, you'll see some of the UI, but no data and lots of errors. That seems fine to me for now, but in a future PR I want to implement some kind of canSeeAdminUI boolean that we can check to hide it and display an access denied message.

I don't see any issues with logout on /accounts route, so if you're still seeing that let me know. Otherwise hopefully all issues you found are now fixed.

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.

👍

@mikemurray mikemurray merged commit 6a7348c into trunk Jan 14, 2020
@mikemurray mikemurray deleted the feat-aldeed-hydra-auth branch January 14, 2020 20:38
@kieckhafer kieckhafer mentioned this pull request Feb 6, 2020
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.

Remove Hydra/oauth routes
4 participants