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

feat(auth-middleware): Return a Tuple with Route pattern configuration when creating dbAuth middleware #10642

Merged
merged 4 commits into from
May 21, 2024

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented May 17, 2024

  • This PR renames createDbAuthMiddleware -> initDbAuthMiddleware
  • Returns a tuple of [dbAuthMw, '*'] from the init function to make it harder to accidentally misconfigure the auth middleware

createDbAuthMw -> initDbAuthMw
@dac09 dac09 requested a review from dthyresson May 17, 2024 12:10
@dac09 dac09 added the release:feature This PR introduces a new feature label May 17, 2024
@dac09 dac09 self-assigned this May 17, 2024
@dac09 dac09 added this to the SSR milestone May 17, 2024
@dac09
Copy link
Collaborator Author

dac09 commented May 17, 2024

@dthyresson As discussed last night!

We should think this through though.... what this effectively does is make it harder to misconfigure, but also makes it harder to intentionally configure the dbAuthMiddleware on certain paths. We're OK with this right? I can't think of a case where you wouldn't want auth mw running though.

Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

  1. What was thinking of init vs create? Is that the naming pattern we like ... ie, use this in other custom middleware?
  2. We should think this through though.... what this effectively does is make it harder to misconfigure, but also makes it harder to intentionally configure the dbAuthMiddleware on certain paths. We're OK with this right? I can't think of a case where you wouldn't want auth mw running though.

Could there be a base AuthMiddlware that dbAuth, Supabase, Clerk etc all use that define the global *. That way it could document and enforce the fact that "all auth middleware" is global. And prevent specific path invocation?

I cannot see why auth would not be on all routes -- unless maybe you have possible have two+ auth providers and maybe dbAuth governs /admin.* and rest if app is *. But that may not be a scenario we support. Unless api functions use a different auth provider than the app's. api key on all function and use login on app facing routes.,

@dac09
Copy link
Collaborator Author

dac09 commented May 21, 2024

What was thinking of init vs create? Is that the naming pattern we like ... ie, use this in other custom middleware?

Init because we're returning a tuple instead of the actual middleware. I was thinking "create" implies you return the object you create, where as init can be a little vague.

@dac09
Copy link
Collaborator Author

dac09 commented May 21, 2024

Could there be a base AuthMiddlware that dbAuth, Supabase, Clerk etc all use that define the global *. That way it could document and enforce the fact that "all auth middleware" is global. And prevent specific path invocation?

I've just done this with types now. Can totally export the type to be general.

I cannot see why auth would not be on all routes -- unless maybe you have possible have two+ auth providers and maybe dbAuth governs /admin.* and rest if app is *. But that may not be a scenario we support. Unless api functions use a different auth provider than the app's. api key on all function and use login on app facing routes.

I think the only case is if we want to disable the auth middleware for certain routes. Same as you I currently can't think of a reason you want to do this.

dac09 added 2 commits May 21, 2024 16:32
…tuples-dbauth-mw

* 'main' of github.com:redwoodjs/redwood:
  fix(deps): update dependency isbot to v5 (redwoodjs#10340)
  chore(deps): update dependency vscode-languageserver-protocol to v3.17.5 (redwoodjs#10351)
  chore(deps): update node.js to >=14.17 <=20.13 (redwoodjs#10368)
  fix(deps): update dependency graphql-sse to v2.5.3 (redwoodjs#10364)
  chore(deps): update dependency @supabase/supabase-js to v2.43.2 (redwoodjs#10365)
  chore(deps): update babel monorepo to v7.24.5 (redwoodjs#10614)
  fix(vite): Rename `serverAuthContext` to `serverAuthState` (redwoodjs#10653)
  feat(middleware): Add .shortCircuit to MiddlewareResponse (redwoodjs#10586)
  chore(ssr): Make entry.client.tsx better match standard entry.client (redwoodjs#10652)
  patch(crwa): Fix small annoyances (formatting, spell-check) (redwoodjs#10651)
  chore(ci): Update rsc readme (redwoodjs#10650)
  chore(serverAuth): Rename serverAuthContext to serverAuthState where relevant (redwoodjs#10643)
  chore(deps): Remove unused deps from prerender (redwoodjs#10649)
  Add missing rwjs/auth deps (redwoodjs#10648)
  feat(rsc-auth): Implement serverStore to hold and pass req info to RSC (redwoodjs#10585)
  chore(deps): Remove unused octokit package (redwoodjs#10645)
  chore(deps): bump @fastify/reply-from from 9.4.0 to 9.8.0 (redwoodjs#10644)
@dthyresson dthyresson self-requested a review May 21, 2024 12:30
Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

LGTM ... will merge

@dthyresson dthyresson merged commit b9e509d into redwoodjs:main May 21, 2024
46 checks passed
@dac09 dac09 deleted the feat/return-tuples-dbauth-mw branch May 21, 2024 12:46
dac09 added a commit to dac09/redwood that referenced this pull request May 21, 2024
…-role-authmw

* 'main' of github.com:redwoodjs/redwood:
  feat(auth-middleware): Return a Tuple with Route pattern configuration when creating dbAuth middleware (redwoodjs#10642)
dthyresson pushed a commit that referenced this pull request May 28, 2024
…ental misconfiguration (#10687)

Supabase equivalent of this:
#10642

Instead of returning middleware we now return `[middleware, '*']`. This
reduces the chance of the user accidentally not configuring the
middleware for certain routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants