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

auth?method=getToken happening 5x per page request (using dbAuth) #4739

Closed
jacebenson opened this issue Mar 13, 2022 · 13 comments · Fixed by #5816
Closed

auth?method=getToken happening 5x per page request (using dbAuth) #4739

jacebenson opened this issue Mar 13, 2022 · 13 comments · Fixed by #5816
Assignees

Comments

@jacebenson
Copy link
Contributor

I wanted to log this as I was having trouble finding anything about it.

When I go to a page with two cells I'm getting 5x auth calls.

Is this normal?

image

@thedavidprice
Copy link
Contributor

thedavidprice commented Mar 16, 2022

Using dbAuth?

cc @cannikin

@cannikin
Copy link
Member

You'll get one for every isAuthenticated or currentUser call on your page: it makes a trip to the server to make sure you're still who you said you are. Before dbAuth every auth solution just used LocalStorage, so a call to isAuthenticated was make 5 calls to get your current user info locally in the browser and was effectively instantaneous. But we can't do that with the cookie since it's encrypted and the decryption keys only live on the server.

I've theorized it would be nice to somehow only make one of these requests per "page" but I don't know how we'd go about doing that...the way the auth code is currently written when you use the useAuth() hook it's scoped locally to the component you call it in, so 5 different components need to make 5 separate calls to getToken, they can't share the result as far as I know.

You might be able to get around this today by doing a single useAuth() in a Context and the wrap your app in that in the router, passing isAuthenticated and currentUser to all children. Although I'm not a Context expert so don't quote me on that!

@jacebenson
Copy link
Contributor Author

I've been trying to figure out useContext, this is a great excuse for that.
Also I did once try to manage this with useState but that ended poorly. Okay. Let me see what I can do to make a context for useAuth()

@jacebenson
Copy link
Contributor Author

And yes, this is using dbAuth.

@cannikin
Copy link
Member

cannikin commented Mar 16, 2022

The thing is there's already <AuthProvider> surrounding everything, so you'd think it could go in there somehow...you'll see the context provided at the end here: https://github.com/redwoodjs/redwood/blob/main/packages/auth/src/AuthProvider.tsx#L321-L339 including a bunch of state.

But when calls are made to getToken(), that calls getToken() in the actual auth client, which for dbAuth says to make the call to the auth function (which is where all the server calls are coming from): https://github.com/redwoodjs/redwood/blob/main/packages/auth/src/authClients/dbAuth.ts#L39-L51

It might this line right here that calls getToken() so often (there's even a comment saying it's going to do it): https://github.com/redwoodjs/redwood/blob/main/packages/auth/src/AuthProvider.tsx#L148-L150

@doowb
Copy link

doowb commented Mar 17, 2022

You'll get one for every isAuthenticated or currentUser call on your page

I believe the call to getToken is made for every graphql request and it's independent of isAuthenticated and currentUser.

When I first looked at the code you linked above, I also thought it was for each currentUser because getToken is called in getCurrentUser, but getCurrentUser is only called in the reauthenticate method (unless a developer calls getCurrentUser manually).

I think the getToken calls from the dbAuth client could be reduced by implementing tokens to be more like some of the other auth providers, e.g. using an access_token and refresh_token that the dbAuth client could keep in localstorage and only update when necessary.

If a token becomes "stale", due to not refreshing properly, the graphql api will still attempt to use the stale token and authentication will fail as if they're logged out. But since getToken is async and is called for each graphql call, dbAuth still has the opportunity to issue new tokens based on the cookie still existing.

I hope all of this was useful... :) I was considering creating a custom auth provider and these are some of the things I was considering. I'd much rather use what's already there, if possible.

@cannikin
Copy link
Member

Hmm I'll have to do some more tracing at some point...so you think you only see multiple getToken calls if you have multiple GraphQL queries on the page?

I created dbAuth specifically to avoid the access/refresh tokens that are so prevalent in the JWT world! Ever since my first OAuth implementation, probably 15 years ago, I've wanted to overthrow the access token regime. 😄 Not to mention the security concerns of putting stuff in LocalStorage. And it seems like other folks are coming around: Supabase is re-doing their third party auth to use cookies instead of LocalStorage! Here's the first Google result for "localstorage security": https://dev.to/rdegges/please-stop-using-local-storage-1i04

So I don't think we'll be updating dbAuth to use tokens any time soon, but I feel like there's a solution that will save us these multiple authentication requests for a single page, I just haven't had time to look into it!

@doowb
Copy link

doowb commented Mar 18, 2022

so you think you only see multiple getToken calls if you have multiple GraphQL queries on the page?

Yes, at least with the default RedwoodApolloProvider, because it calls getToken before each request.

So, if there are multiple cells in a page, there are multiple requests being made.

I created dbAuth specifically to avoid the access/refresh tokens that are so prevalent in the JWT world!

I see now, and it does make sense. I almost didn't mention localStorage, but my initial thoughts where, if someone compromised the system to get the token from localStorage, they could also call the auth endpoint to get the token. It would still require more knowledge of the system, so it wouldn't be as easy I guess. Also, I thought the main auth code was storing something in localStorage, but when I looked again, it was the session cookie that I had seen.

Supabase is re-doing their third party auth to use cookies instead of LocalStorage!

Supabase (and gotrue/netlify) is the other auth provider that I was specifically thinking of. This is probably because I still have a gotrue token in my localhost localStorage from when I used netlify auth as I was going through the redwood tutorial.

After all of that :) ... I agree that it's correct to use secure, http-only cookies for the session token, but I think there could be something done to keep the token in memory. Even if it's for a short amount of time. Currently, the only way the session token will be invalid, without the user logging out, is if the user record is deleted.

I just had another thought, but I haven't looked into what would be involved yet. When the main graphql handler calls getAuthenticationContext, this delegates to the auth provider specified in the auth-provider header. If that method still calls the provider even when the authorization header is not provided, then dbAuth would still work by using the session cookie at that point. This seems more in line with how I remember web app authentication, but it doesn't seem as popular now that the web and api sides are decoupled.

@standuprey
Copy link
Contributor

I can clearly see that happening on https://superstore-redwood-stripe.netlify.app/ (from https://github.com/redwoodjs/example-store-stripe) and we also see calls to the cell being 2x and the first call being canceled by the second one, something not clean is going on

@nowlena
Copy link

nowlena commented Jun 13, 2022

This has been a deterrent for me as well. While it's probably completely fine I don't want to use this auth solution since it's making so many extraneous calls.

Maybe I can try to dig into this and see if there's a way to:

  1. return an accessToken from the login route as data (not a cookie)
  • when a user logs in they receive their user data but also their access token as part of the payload
  • the access token is kept only in state so that the react app can make auth'd requests with this token
  • you can store the token by any means... react context or apollo's reactive variables
  1. return a refreshToken from the login route as an http only secure cookie (not as data)
  • the refresh token should be signed with a different secret than the access token and stored has an http only secure cookie
  1. add a refreshToken endpoint
  • when users reload the page we will no longer have their access token in state, so we'll hit this endpoint and validate / check the stored refreshToken cookie
  • if there is no cookie or it is invalid, the user is not logged in and just return a 200
  • if the cookie exists and is valid, return a new accessToken to be kept in state only as described in step 1. We can either additionally return current user data here or make another call for it once the new accessToken is received and in state

@orta
Copy link
Contributor

orta commented Jun 13, 2022

This has been a deterrent for me as well. While it's probably completely fine I don't want to use this auth solution since it's making so many extraneous calls.

Yeah, I also wasn't too happy at those extra API calls, you can see a pretty comprehensive alternative which does what you're talking about here: https://github.com/orta/redwood-jwt-2phase-auth

@cannikin
Copy link
Member

cannikin commented Jun 13, 2022

If you're looking at creating major structural changes please open an RFC issue so we can discuss! 🙂

You're welcome to create a new kind of auth client (cookieTokenAuth?) but I want dbAuth itself to stay as is—simple, single cookie authentication. I'd rather look into a way to batch these multiple getToken calls so that there's only one per "page" but that will probably require re-working how a lot of the core auth logic works (and that code is currently shared by all clients).

I purposefully built dbAuth to not use access/refresh tokens or JWTs like our other auth clients (you can find all kinds of issues with JWTs and usage of LocalStorage on the internet) and get back to good ol' cookie storage, which has worked for hundreds of years (slight exaggeration). I know you weren't suggesting using JWTs, but just a little background on where I was coming from!

@jacebenson
Copy link
Contributor Author

I just upgraded to canary to test this out. Works great for me thank you @cannikin !

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 a pull request may close this issue.

8 participants