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

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

Closed
1 task
taivo opened this issue Apr 3, 2024 · 11 comments
Closed
1 task
Assignees
Labels
bug/needs-info More information is needed for reproduction topic/auth

Comments

@taivo
Copy link
Contributor

taivo commented Apr 3, 2024

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?

  • I'm interested in working on this
@taivo taivo added the bug/needs-info More information is needed for reproduction label Apr 3, 2024
@ahaywood
Copy link
Contributor

ahaywood commented Apr 3, 2024

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
Copy link
Contributor

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"
  2. And how Supabase uses that to restore the session
    restoreAuthState: async () => {
  3. as well as how UserMetadata gates that action
    const userMetadata = await authImplementation.getUserMetadata()

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
Copy link
Contributor

dthyresson commented Apr 3, 2024

@dac09 What do you think about line

const currentUser = skipFetchCurrentUser ? null : await getCurrentUser()
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
Copy link
Collaborator

dac09 commented Apr 3, 2024

Thanks @taivo for the breakdown and explanation.

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

@taivo
Copy link
Contributor Author

taivo commented Apr 5, 2024

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
Copy link
Collaborator

dac09 commented Apr 16, 2024

I think this may be fixed after my PR here: #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
Copy link
Contributor Author

taivo commented Apr 19, 2024

@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
Copy link
Collaborator

dac09 commented Apr 23, 2024

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
Copy link
Contributor Author

taivo commented Apr 25, 2024

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
Copy link
Contributor Author

taivo commented May 7, 2024

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.

@taivo taivo closed this as completed May 7, 2024
@dac09
Copy link
Collaborator

dac09 commented May 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/needs-info More information is needed for reproduction topic/auth
Projects
None yet
Development

No branches or pull requests

5 participants