redwoodjs / redwood

The App Framework for Startups
https://redwoodjs.com
MIT License
17.32k stars 993 forks source link

[Bug?]: isAuthenticated=true but currentUser=undefined when dev server restarts #10403

Closed taivo closed 6 months ago

taivo commented 7 months ago

What's not working?

When we kill and restart the dev server, it automatically triggers a browser reload. If there is already a logged in user, this first reload ends up with a situation where useAuth() props isAuthenticated and currentUser are of sync (loading=false, isAuthenticated=true but currentUser=undefined).

I'm using supabase auth so it's possible this is an issue with that auth adapter.

I ran into this issue while playing with a hook that redirects users to a registration page if they have not claimed a username currentUser?.username. As soon as the app restarts, this hook redirects an already-registered user to the registration page due to the bug above.

My current workaround is to rely on currentUser instead of isAuthenticated for authentication status. When this workaround is in place, the app displays the expected behavior via the graphQL error The RedwoodJS API server is not available or is currently reloading. Please refresh.

My guess is this is a minor cache issue (isAuthenticated initialized from some client-side cache whereas currentUser is fetched from the server). The workaround is simple enough. Just posting in case it correlates with any other strange issues that others may be experiencing.

How do we reproduce the bug?

No response

What's your environment? (If it applies)

No response

Are you interested in working on this?

ahaywood commented 7 months ago

Hey @taivo thanks for reporting the issue ... I'm going to phone a friend and tag one of our core team members who specializes in Supabase + Auth ... stay tuned.

dthyresson commented 7 months ago

Hi @taivo and thanks for noting this.

Some background first --

For

When this workaround is in place, the app displays the expected behavior via the graphQL error The RedwoodJS API server is not available or is currently reloading. Please refresh.

This is a dev server only case -- this won't happen when deployed or running in prod.

What happens here is that when reloading, both the web and api side reload and the web is faster (sometimes, more than sometimes) than api side (since it assembles the fgraphql schema and such). So, the api just isn't ready yet. But - again - this is dev server only.

Maybe when that happens, the auth state needs to be cleared? maybe the reAuth is picking something up from session/localStorage but then the getCurrentUser api call fails and one is set and the other is null. Hm. That could be. @dac09 what do you think about this scenario?

Places to look

Also - the places in the framework auth code you could look at are:

  1. In the way each auth provider "reauthenticates" https://github.com/redwoodjs/redwood/blob/f99e3b3937e281d33daea0aa084ce8368d0cd241/packages/auth/src/AuthProvider/useReauthenticate.ts#L64
  2. And how Supabase uses that to restore the session https://github.com/redwoodjs/redwood/blob/f99e3b3937e281d33daea0aa084ce8368d0cd241/packages/auth-providers/supabase/web/src/supabase.ts#L202
  3. as well as how UserMetadata gates that action https://github.com/redwoodjs/redwood/blob/f99e3b3937e281d33daea0aa084ce8368d0cd241/packages/auth/src/AuthProvider/useReauthenticate.ts#L41

Ideas

User Metadata

Since userMetadata is important, perhaps you could try instead of currentUser?.username you could try const { userMetadata } = useAuth() and then checking userMetadata.<somevalue> to determine the redirect?

See: userMetadata

Try in serve/prod?

Instead of running yarn rw dev try yarn rw build and yarn rw serve.

Then try your refresh case as I don't think the api not ready error will happened (fingers crossed). That could be a way to confirm the hypothesis that when api serve isn't available then "auth things get out of sync"?

Hope this helps and if you can confirm that behavior, then we'll bee to handle that case better (not sure how just yet).,

dthyresson commented 7 months ago

@dac09 What do you think about line https://github.com/redwoodjs/redwood/blob/f99e3b3937e281d33daea0aa084ce8368d0cd241/packages/auth/src/AuthProvider/useReauthenticate.ts#L58 and if ...

try {
      const userMetadata = await authImplementation.getUserMetadata()

      if (!userMetadata) {
        let loading = false

        if (authImplementation.clientHasLoaded) {
          loading = !authImplementation.clientHasLoaded()
        }

        setAuthProviderState({
          ...notAuthenticatedState,
          loading,
          client: authImplementation.client,
        })
      } else {
        await getToken()

        const currentUser = skipFetchCurrentUser ? null : await getCurrentUser()

        setAuthProviderState((oldState) => ({
          ...oldState,
          userMetadata,
          currentUser,
          isAuthenticated: true,
          loading: false,
          client: authImplementation.client,
        }))
      }
    } catch (e: any) {
      setAuthProviderState({
        ...notAuthenticatedState,
        hasError: true,
        error: e as Error,
      })
    }

So if getToken or getCurrentUser excepts, should be caught and not authenticated.

But, I wonder if currentUser comes back null and now we have a case where userMetadata exists, but currentUser is null. Ie - we're out of sync?

If currentUser is null, should we not just

setAuthProviderState({
        ...notAuthenticatedState,
      })

but with no error? Perhaps?

dac09 commented 7 months ago

Thanks @taivo for the breakdown and explanation.

@dthyresson yeah seems sensible to me! I would've expected the currentUser function to throw though.

taivo commented 7 months ago

Thank you guys, for the fast turn around time. @dthyresson I did some digging into the scenario you provided and you're exactly right!!

First off, confirmed that this situation only happens during the first few seconds when the server restarts and is not yet ready. Perhaps this is applicable to production as well as dev.

To elaborate, in this code segment

else {
        await getToken()

        const currentUser = skipFetchCurrentUser ? null : await getCurrentUser()

        setAuthProviderState((oldState) => ({
          ...oldState,
          userMetadata,
          currentUser,
          isAuthenticated: true,
          loading: false,
          client: authImplementation.client,
        }))
      }

await getCurrentUser() did not throw but rather returned undefined. Since we hardcoded isAuthenticated to true, it lead to the out of sync situation. Could we fix that by setting isAuthenticated: !!currentUser instead? Lmk, I'll send a PR.

In addition to the consistency issue, we may need to communicate this warm up period to the user somehow, or put some retry logic into getCurrentUser when it gets empty data in the server response. That said, I'm not sure if this is an issue because after setting isAuthenticated: !!currentUser, my app just works. My guess is there are other mechanism that calls getCurrentUser at a later time and set things right. Just bringing up the potential issue here in case it rings some bell.

dac09 commented 6 months ago

I think this may be fixed after my PR here: https://github.com/redwoodjs/redwood/pull/10420

@taivo if you get a chance to take it for a spin on canary (yarn rw upgrade -t canary) and close the issue if it's resolved, would be grateful!

taivo commented 6 months ago

@dac09 I ran into an error with the canary upgrade (-t latest works just fine)

git:(main) ✗ yarn rw upgrade -t canary
✔ Checking latest version
✔ Updating your Redwood version
  ✔ Updating /.../package.json
  ✔ Updating /.../package.json
  ✔ Updating /.../web/package.json
✔ Updating other packages in your package.json(s)
  ✔ Updating /.../package.json
  ✔ Updating /.../api/package.json
  ✔ Updating /.../web/package.json
✖ Could not finish installation. Please run `yarn install` and then `yarn dedupe`, before
  continuing
◼ Refreshing the Prisma client
◼ De-duplicating dependencies
◼ One more thing..
-------------------------------------------------------------------------------------------------
Error: Could not finish installation. Please run `yarn install` and then `yarn dedupe`, before continuing
    at yarnInstall (/.../node_modules/@redwoodjs/cli/dist/commands/upgrade.js:212:11)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async _Task.run (/.../node_modules/listr2/dist/index.cjs:2049:11)

Running yarn install according to the error message also fails

git:(main) ✗ yarn install
➤ YN0000: · Yarn 4.1.1
➤ YN0000: ┌ Resolution step
➤ YN0085: │ + @redwoodjs/api-server@npm:8.0.0-canary.488, and 85 more.
➤ YN0085: │ - @redwoodjs/api-server@npm:7.4.2, @redwoodjs/api@npm:7.4.2, and 250 more.
➤ YN0000: └ Completed in 2s 125ms
➤ YN0000: ┌ Post-resolution validation
➤ YN0001: │ TypeError: Cannot read properties of null (reading 'set')
    at /.../.yarn/releases/yarn-4.1.1.cjs:140:118084
    at Array.map (<anonymous>)
    at sM (/.../.yarn/releases/yarn-4.1.1.cjs:140:118072)
    at /.../.yarn/releases/yarn-4.1.1.cjs:207:6478
    at Array.map (<anonymous>)
    at QAt (/.../.yarn/releases/yarn-4.1.1.cjs:207:6078)
    at /.../.yarn/releases/yarn-4.1.1.cjs:213:2771
    at Nt.startSectionPromise (/.../.yarn/releases/yarn-4.1.1.cjs:176:2834)
    at Nt.startTimerPromise (/.../.yarn/releases/yarn-4.1.1.cjs:176:3734)
    at Pt.install (/.../.yarn/releases/yarn-4.1.1.cjs:213:2697)
➤ YN0000: └ Completed
➤ YN0000: · Failed with errors in 2s 136ms
dac09 commented 6 months ago

Thanks @taivo - yeah sometimes canary can be a little bit flakey. If you have a chance to try again with the newer published version (same command) that'll be great.

If not I'll validate this as we upgrade supabase to support ssr as well.

taivo commented 6 months ago

hey @dac09 , I just tried it. Same issue at the canary installation step.

If it helps, I'm using a MacBook with Sonoma 14.4.1, Node v20.11.1, yarn 4.1.1.

I can try this again for you next week to see if there are any changes.

taivo commented 6 months ago

Hi @dac09 , I was able to install canary 8.0.0-canary.543. However, the auth issue above is still there.

After logging in, if I restart the dev server, my app in the browser redirects me to its signin page. Manually navigating away from that signing page clearly shows that I'm still logged in.


Update:

I've just tested again with latest rw 7.4.3. It looks like the auth issue is gone. As for my comment regarding 8.0.0-canary.543 above, my guess is the fix hasn't been ported to it. It most likely will be ported via your release process so I'm going to close this ticket.

Thank you for addressing the bug I reported.

dac09 commented 6 months ago

Thank you for reporting and the help in validation @taivo ✌️✌️✌️

richcatt commented 5 months ago

@dac09 I'm having the same issue and tried updating (to 7.6.2) but this doesn't seem to have helped - isAuthenticated returns true after a server restart.

Is there anything else I should be doing here?

dac09 commented 5 months ago

Hi @richcatt - the change here is currently only released on the canary branch. We don't see this as a critical issue (please do let us know if you feel otherwise) - and will roll out with SSR/RSC changes in the next major.

In theory should be possible to backport the fix too though, just not high on our priority list at the moment.

richcatt commented 5 months ago

@dac09 thanks for getting back to me.

I was just going off the above comment where taivo mentioned it was fixed in 7.4.3. No problem, I agree it's not a critical issue so happy if it's in the next major.

Tobbe commented 5 months ago

Just to set the expectations right here, the next major most likely will not include the new SSR code or any of the RSC code.

We don't want to hold up the next major until RSC is done. So we will continue releasing majors as needed to keep up-to-date with major updates to libraries we use, and to be able to release other breaking changes in the framework in the meantime.