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

fix(clerk): add alternative decoder #8642

Merged
merged 18 commits into from
Jun 22, 2023
Merged

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Jun 16, 2023

Working fix for #8641. This is a work in progress; I haven't tested it yet, I just want to get the conversation started. @asher-eastham, @o0charlie0o, would love your input here. @asher-eastham I think what I'm proposing here is along the lines of what I suggested to you, but curious to know how much your implementation ended up differing?

I know the existing authDecoder needs to go, but removing that would be breaking. What if we mark it as deprecated, add a new authDecoder (right now named jwtAuthDecoder, but open to suggestions), configure the yarn rw setup auth clerk command to use that decoder instead, and in the release notes, I'll recommend everyone use Clerk use the new decoder?

  • docs on how to customize the JWT

jtoar and others added 2 commits June 16, 2023 11:35
Co-authored-by: Charlie Ray <charlie@dsrecordings.com>
Co-authored-by: Charlie Ray <charlie@dsrecordings.com>
@jtoar jtoar marked this pull request as ready for review June 16, 2023 21:12
@jtoar
Copy link
Contributor Author

jtoar commented Jun 16, 2023

It seems I have some work to do with the template getCurrentUser function. It passes the decoded token to parseJWT which doesn't seem to do the right thing anymore. Let me double check what it does with the existing decoded token, but with my changes, I'm getting back an object like this: This is what parseJWT returns in stable, so this behavior is expected.

{ appMetadata: {}, roles: [] }

It still feels like the template getCurrentUser needs to be updated. @o0charlie0o and @asher-eastham do you use parseJWT at all in your getCurrentUser function? I'm going to ask the team what the intention behind it was, it doesn't seem that necessary to me for Clerk.

@o0charlie0o
Copy link
Contributor

@jtoar We do not use parseJWT at all.

@jtoar jtoar merged commit f63678f into main Jun 22, 2023
@jtoar jtoar deleted the ds-clerk-auth/add-alternative-decoder branch June 22, 2023 23:31
jtoar added a commit that referenced this pull request Jun 23, 2023
* fix(clerk) add alternative decoder

* Update packages/auth-providers/clerk/api/src/decoder.ts

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>

* Update packages/auth-providers/clerk/api/src/decoder.ts

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>

* add `id` for backwards compatibility

* fix prettier error

* extend type for jwt payload

* simplify decoder

* duplicate test for new decoder

* fix set up notes

* remove `parseJWT`; only return `id` in the template

* lint fix

* update docs

* fix typos, add link

* rename to clerkAuthDecoder

---------

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>
@jtoar jtoar modified the milestones: next-release-patch, v5.3.2 Jun 23, 2023
jtoar added a commit that referenced this pull request Jun 23, 2023
* fix(clerk) add alternative decoder

* Update packages/auth-providers/clerk/api/src/decoder.ts

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>

* Update packages/auth-providers/clerk/api/src/decoder.ts

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>

* add `id` for backwards compatibility

* fix prettier error

* extend type for jwt payload

* simplify decoder

* duplicate test for new decoder

* fix set up notes

* remove `parseJWT`; only return `id` in the template

* lint fix

* update docs

* fix typos, add link

* rename to clerkAuthDecoder

---------

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>
jtoar added a commit that referenced this pull request Jun 23, 2023
* fix(clerk) add alternative decoder

* Update packages/auth-providers/clerk/api/src/decoder.ts

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>

* Update packages/auth-providers/clerk/api/src/decoder.ts

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>

* add `id` for backwards compatibility

* fix prettier error

* extend type for jwt payload

* simplify decoder

* duplicate test for new decoder

* fix set up notes

* remove `parseJWT`; only return `id` in the template

* lint fix

* update docs

* fix typos, add link

* rename to clerkAuthDecoder

---------

Co-authored-by: Charlie Ray <charlie@dsrecordings.com>
dac09 added a commit to dac09/redwood that referenced this pull request Jun 23, 2023
…ace-svg-as-components

* 'main' of github.com:redwoodjs/redwood: (25 commits)
  fix(deps): update dependency @whatwg-node/fetch to v0.9.7 (redwoodjs#8702)
  fix(deps): update dependency @heroicons/react to v2.0.18 (redwoodjs#8701)
  fix(deps): update dependency @headlessui/react to v1.7.15 (redwoodjs#8700)
  fix(deps): update dependency webpack to v5.88.0 (redwoodjs#8697)
  fix(deps): update dependency @graphiql/toolkit to v0.8.4 (redwoodjs#8698)
  fix(deps): update dependency react-error-boundary to v4.0.10 (redwoodjs#8693)
  Rename cache file (redwoodjs#8699)
  fix(clerk): add alternative decoder (redwoodjs#8642)
  fix(deps): update dependency @vitejs/plugin-react to v4.0.1 (redwoodjs#8692)
  chore(deps): update dependency @simplewebauthn/server to v7.3.1 (redwoodjs#8690)
  chore(rwfw): Add force optimise to vite.config when running project:sync (redwoodjs#8688)
  fix(deps): update storybook monorepo to v7.0.23 (redwoodjs#8696)
  fix(deps): update dependency react-toastify to v9.1.3 (redwoodjs#8694)
  fix(deps): update prisma monorepo to v4.16.1 (redwoodjs#8695)
  Mark broken gql prerender test as slow (redwoodjs#8687)
  fix(deps): update dependency @graphiql/plugin-explorer to v0.1.20 (redwoodjs#8691)
  fix(deps): update typescript-eslint monorepo to v5.60.0 (redwoodjs#8660)
  fix(deps): update dependency @fastify/http-proxy to v9.2.1 (redwoodjs#8680)
  chore(deps): update dependency vite to v4.3.9 (redwoodjs#8682)
  fix(deps): update prisma monorepo to v4.16.0 (redwoodjs#8684)
  ...
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants