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

Epic #1

Open
8 of 15 tasks
henrikwirth opened this issue Feb 2, 2020 · 8 comments
Open
8 of 15 tasks

Epic #1

henrikwirth opened this issue Feb 2, 2020 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@henrikwirth
Copy link
Contributor

henrikwirth commented Feb 2, 2020

Intro

This epic is to track possible solutions on how to implement the JWT User Authentication workflow.

Philosophy

Checkout this post: https://blog.hasura.io/best-practices-of-using-jwt-with-graphql

I want to use that post as a base for the implementation.

Persisting JWT token in localstorage (prone to XSS) < Persisting JWT token in an HttpOnly cookie (prone to CSRF, a little bit better for XSS) < Persisting refresh token in an HttpOnly cookie (safe from CSRF, a little bit better for XSS).

Note that while this method is not resilient to serious XSS attacks, coupled with the usual XSS mitigation techniques, an HttpOnly cookie is a recommended way persisting session related information. But by persisting our session indirectly via a refresh token, we prevent a direct CSRF vulnerability we would have had with a JWT token.

Here is also a good reference: https://flaviocopes.com/graphql-auth-apollo-jwt-cookies/

Auth Flow Strategy

Register/SignUp

  1. User registers through from with following mutation:

    mutation RegisterUser($input: RegisterUserInput!) {
         registerUser(input: $input) {
             user {
                 jwtAuthToken
                 jwtRefreshToken
             }
         }
     }
    
  2. Same as Login 2. - 6.

Login

  1. The user logs in with
    mutation LoginUser($input: LoginInput!) {
        login(input: $input) {
            user {
                jwtAuthToken
                jwtRefreshToken
            }
        }
    }
    
  2. WordPress generates new jwtAuthToken (5min Expiration) and jwtRefreshToken (long lived: how long?)
  3. jwtAuthToken and jwtRefreshToken are returned back to the client as a JSON payload.
  4. The jwtAuthToken is stored in memory, the jwtRefreshToken in localStorage.
  5. A countdown to a future silent refresh is started based on jwtAuthExpiration

On Page Visit

  1. Check if jwtAuthToken is in Memory, if not check if jwtRefreshToken is stored in localStorage
  2. If jwtRefreshToken is stored, then start Silent Refresh to get new jwtAuthToken to store it in Memory

Silent Refresh

  1. Call refresh mutation like:
    mutation($input: RefreshJwtAuthTokenInput!) {
      refreshJwtAuthToken(input: $input) {
        authToken
      }
    }
    
    Note: There is no new jwtAuthExpiration and isJwtAuthSecretRevoked given and authToken is not called jwtAuthToken => Issue for JWT plugin
  2. Server checks jwtRefreshToken and if it is valid (or hasn't been revoked) jwtRefreshToken
    • Then: the server returns a new jwtAuthToken and jwtAuthExpiration to the client and also sets a new jwtRefreshToken cookie via Set-Cookie header.
  3. Use jwtAuthToken in Payload and save in Memory.
  • Before any request to the server, Expiration could be checked and a refresh could be triggered, if the jwtAuthToken is expired.

Scope

Starter

  • Setup initial Project with current Apollo v3
  • Login Form
  • SignUp Form
  • Protected Routes (Dashboard)
  • RegisterUser
  • LoginUser
  • Auth Token only in Memory
  • Make sure, everything works with WPGraphQL v0.6.x
    - There are some issues, see: JWT token could not be returned wp-graphql/wp-graphql-jwt-authentication#69
  • Silent Refresh
  • Error Handling for Invalid Tokens or User Credentials
  • Ability to force logout all sessions (invalidate all refresh tokens assigned to user)
  • Is there a way to only invalidate one session? So one refresh token assigned to a certain client, that can be revoked???
  • Set Refresh Token Cookie on Server response HttpOnly
    • This is being discussed and probably it doesn't make a difference if it is in the cookie or localStorage

NPM Module

The idea is to use all the implemented above and abstract it in a way, it is easy for users to plugin to their projects, with less boilerplate.

  • Setup initial project
  • Define API user can interact with
    • AuthProvider: A Provider, that the user should use in wrapRootElement
    • useAuthContext: Context hook, that can be used to access everything living in the Context
    • login(redirect?)
    • logout(redirect?)
    • isLoggedIn
    • PrivateRoute Component
@henrikwirth
Copy link
Contributor Author

@jasonbahl mentioned this tweet: https://twitter.com/ryanflorence/status/1060361144701833216?s=21

So one possible approach for login, is that it is just a component and not a Route itself. So for this example, if someone navigates to the Dashboard and the user is not logged in, the Login/SignUp form will be shown, instead of redirecting to login/signup routes.

I'm still not sure about it. I somehow like to have "/login" and "/signup" routes to be able to reference too. I'll leave that open for discussion.

@henrikwirth
Copy link
Contributor Author

@jasonbahl I tried the Cors plugin but didn't get it to work as expected. The login mutation they offer, breaks the login mutation, as there are 2 registered at that time. And the normal login mutation doesn't seem to send the credentials back.

I would love to see that in the JWT Plugin, so that we can set HttpOnly Cookies on Login and maybe on successful registration.

Any thoughts to that would be highly appreciated :)

@kellenmace
Copy link

Hey @henrikwirth - This looks pretty good. Some feedback:

If this repo is meant to be a community resource to show folks how to set up auth with Gatsby, Apollo Client and wp-graphql-jwt-authentication, then I think these things could be tweaked:

Trying to boot up the app without yarn installed results in errors due to "postinstall": "yarn run createTypes". This could be changed to only rely on npm and not assume that yarn is available. Or this script could be removed altogether, since it's not required for basic use of Apollo Client.

BatchHttpLink is being used in apollo.js, and I see that there is a HttpLink that is commented out. Having HttpLink as the default would probably make more sense if this is meant to be a basic community resource, though.

Using https://www.npmjs.com/package/uuid for generating UUIDs may be preferable to using my homebrew getUuid() function, just to extract that custom code out of the project and lean on an npm package to own that minor bit of complexity.

There's a lot going on in the auth.js and useAuth.js files. I think some of that code could be simplified.

When a user logs in, the JWT sent back contains the auth token, refresh token and any user data that was in the body of the query. It looks like you're storing the refresh token only as REFRESH_TOKEN in localStorage, though. I don't believe that's necessary – we can store the entire JWT in localStorage and pull all three of those pieces of data out of it (auth token, refresh token & any user data) whenever they're needed without storing them separately/individually.

Handling token refreshes in apollo/client.js using apollo-link-token-refresh and also in src/hooks/useAuth.js seems redundant. It seems like we should have a single source of truth for doing refreshes rather than doing it in two separate places in the app. I'm still thinking through the best way to do that, myself.

--
If this repo is NOT meant to be a basic example for the community to follow though and is a starter for yourself/your company to reference for future projects though, then please disregard some of my comments above :)

I'm working on a Codesandbox that is meant to be a bare bones example of how to set up Gatsby + Apollo Client + wp-graphql-jwt-authentication. I'm trying to make it as simple as possible, with token refreshes only being handled in one place I'll share it when I have it in working order. I'd love to compare our approaches and get your feedback on it. Thanks again for your work here! 👍🏼

@henrikwirth
Copy link
Contributor Author

@kellenmace
I agree, that some things should probably be simplified with more starter friendly things.
The Silent Refresh is not working for me properly with the apollo-link-token-refresh if you have any idea on how to fix it, that would be great. I certainly only want one place to do it.

Good point about npm over yarn.

For the AuthToken in localStorage I have to disagree. See the first article I linked. From my understanding that makes sense and it saver than in localStorage.

@kellenmace
Copy link

kellenmace commented Feb 2, 2020

@henrikwirth I’ll try to get the refresh flow working with that package and the newest versions of all plugins and let you know how it goes.

I missed the “Auth Token only in Memory” item on your checklist above, and the linked blog post, and didn’t realize that you were intentionally not saving the JWT auth token to localStorage. That code makes a lot of sense now :).

@henrikwirth
Copy link
Contributor Author

henrikwirth commented Feb 2, 2020

@kellenmace I just added some changes as of you suggestion:

Add uuid module. Remove BatchHttpLink and use normal HttpLink. Use npm instead of yarn.

Not sure what you mean with newest plugins, but if you mean WPGraphQL, then there is still some fixes needed. Also the RefreshToken is the same as the AuthToken at the moment. There was a fix I think Jason forgot to merge here: wp-graphql/wp-graphql-jwt-authentication#64

@henrikwirth henrikwirth added the help wanted Extra attention is needed label Feb 2, 2020
@abetoots
Copy link

@henrikwirth This looks great. Thanks for the resources as well. Can't wait to refactor some of my projects.

Based on what I read, I have a question: Does the wp-graphql-jwt-authentication plugin already implement setting the jwtRefreshToken as a HttpOnly cookie?

@henrikwirth
Copy link
Contributor Author

I didn't get the HttpOnly cookie to work yet. I'll try to figure out a solution for it though. I'll add it to the todos of this epic to keep track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants