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

Netlify client getToken fix when GoTrue client refreshes JWT #4539

Merged
merged 3 commits into from Feb 20, 2022

Conversation

dthyresson
Copy link
Contributor

@dthyresson dthyresson commented Feb 20, 2022

This PR fixes an issue in the Netlify authentication client where refreshing and getting a token can cause an error such as:

Error: null is not an object (evaluating 'u.default.gotrue.currentUser().jwt')

Because of a null default value set as part of the isAuthentication fix to set default values: https://github.com/redwoodjs/redwood/pull/4320/files

This PR catches any error from the Netlify client sdk and handles it.

Note that it isn't client that's null and raising the error as checking from it still causes the error.

      if (client) {
        // The client refresh function only actually refreshes token
        // when it's been expired. Don't panic
        await client.refresh()
        const user = await client.currentUser()
        return user?.token?.access_token || null
      } else {
        return null
      }

Edit: Also added some check for the GoTrue client, since it is the same underneath.

@dthyresson dthyresson added the release:fix This PR is a fix label Feb 20, 2022
@dthyresson dthyresson changed the title Netlify client getToken fix get GoTrue client refreshes JWT Netlify client getToken fix when GoTrue client refreshes JWT Feb 20, 2022
@Tobbe
Copy link
Member

Tobbe commented Feb 20, 2022

Because of a null default value set as part of the isAuthentication fix to set default values: #4320 (files)

I also thought that was the problem, but it wasn't. It's that we now always (unconditionally) call getToken() in apollo/index.tsx. So still the same PR, just a different piece of code.

I believe the netlify-identity-widget is doing the wrong thing here when it's throwing the null exception. I wrote a PR for it. netlify/netlify-identity-widget#516

try {
const user = await client.currentUser()
return user?.jwt() || null
} catch {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're swallowing error messages here. Is there any way we can be more helpful to the developer? I don't like just printing things to the console the the end-developer can't do anything about. But I also don't like swallowing errors that the developer might want to know about, and might want to track in something like Sentry in production.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to alternatives — I didn’t check what happens if an error is raised but wasn’t that the issue? An error here broke cell data fetching?

Copy link
Member

@Tobbe Tobbe Feb 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left my comment open ended because I couldn't think of a good alternative. Was hoping you could come up with something 🙃

Let's merge this so our users can have working auth again. Then revisit when the netlify people have answered on my PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😀

const user = await client.currentUser()
return user?.token?.access_token || null
} catch {
return null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

@Tobbe Tobbe merged commit 12d17bd into redwoodjs:main Feb 20, 2022
@jtoar jtoar added this to the next-release milestone Feb 20, 2022
dac09 added a commit to dac09/redwood that referenced this pull request Feb 21, 2022
…test

* 'main' of github.com:redwoodjs/redwood: (23 commits)
  Netlify client getToken fix when GoTrue client refreshes JWT (redwoodjs#4539)
  Update dependency @supabase/supabase-js to v1.30.4 (redwoodjs#4536)
  Envelop: Don't use useImmediateIntrospection as it causes auth bug (redwoodjs#4538)
  Update dependency react-hook-form to v7.27.1 (redwoodjs#4521)
  try increasing timeout for flaky test (redwoodjs#4526)
  Update dependency stacktracey to v2.1.8 (redwoodjs#4519)
  Update dependency msw to v0.38.1 (redwoodjs#4525)
  Update dependency eslint-config-prettier to v8.4.0 (redwoodjs#4522)
  Provide a Revised Runtime Error Page (redwoodjs#4167)
  update yarn.lock
  v0.46.0
  Update dependency esbuild to v0.14.23 (redwoodjs#4518)
  Fix Storybook build args (redwoodjs#4455)
  Update dependency react-helmet-async to v1.2.3 (redwoodjs#4502)
  Bump url-parse in /__fixtures__/example-todo-main-with-errors (redwoodjs#4511)
  Bump url-parse from 1.5.1 to 1.5.7 in /__fixtures__/example-todo-main (redwoodjs#4512)
  Update dependency fastify to v3.27.2 (redwoodjs#4516)
  Uncomment role checks (redwoodjs#4476)
  Update dependency zx to v5.1.0 (redwoodjs#4505)
  Update dependency firebase to v9.6.7 (redwoodjs#4514)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

None yet

4 participants