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

dbAuth: No more process.env.RWJS_API_DBAUTH_URL #7032

Merged
merged 7 commits into from
Dec 9, 2022

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Dec 6, 2022

Using process.env is troublesome for Vite.

Also, overriding default values by passing them as config to the auth client brings dbAuth more inline with the other auth providers.

This PR also adds tests for webAuthn, that we were lacking. And fixes a bug where the cookie matching was too lax.

@Tobbe Tobbe marked this pull request as draft December 6, 2022 16:02
@Tobbe Tobbe added the release:fix This PR is a fix label Dec 6, 2022
@jtoar
Copy link
Contributor

jtoar commented Dec 7, 2022

Could you expand on Vite not liking env vars? env vars seem very necessary for auth to me, and nearly all the other auth providers use them during initialization

@dac09
Copy link
Collaborator

dac09 commented Dec 8, 2022

Could you expand on Vite not liking env vars? env vars seem very necessary for auth to me, and nearly all the other auth providers use them during initialization

We should avoid using process.env in the framework code (not in the user's project) - because Vite prefers using import.meta.env. I can make it work with process.env, but we'll be compounding problems later - I can't exactly remember why, but I made a note in my experiments to avoid using process.env where possible! 🤷

All the auth providers, on the web side instantiate a client with envars in the user's codebase, then pass it to the framework - process.env is never used in the provider or framework code (on the web side). But in dbAuth we directly configure it from values in the Redwood.toml - this is inconsistent, and also the cause of incompatibility between webpack and vite.

@dac09
Copy link
Collaborator

dac09 commented Dec 8, 2022

@Tobbe changes look good to me... but just wondering where we pass in the CORS config for dbAuth? It might make sense that its configured the same way. https://redwoodjs.com/docs/cors#graphql-xhr-credentials

@dac09
Copy link
Collaborator

dac09 commented Dec 8, 2022

We should avoid using process.env in the framework code (not in the user's project) - because Vite prefers using import.meta.env. I can make it work with process.env, but we'll be compounding problems later - I can't exactly remember why, but I made a note in my experiments to avoid using process.env where possible! 🤷

Found a note on why:

// @mark instead of using process.env, we use RWJS_GLOBALS
// This is because it seems to interfere with the node polyfills in esbuildOptions, for SSR

@Tobbe
Copy link
Member Author

Tobbe commented Dec 8, 2022

Thanks @dac09. Dom and I talked yesterday and I explained the Vite env vars stuff. But good to also have it "on paper".

And about CORS, it's still configured the same way as it was before. You pass a config object to dbAuth when you set it up. I reused that same config object for the custom api url. Just added another option to it. So nothing has (or need to) change regarding CORS

@Tobbe Tobbe marked this pull request as ready for review December 8, 2022 19:30
@dac09 dac09 enabled auto-merge (squash) December 9, 2022 05:58
@dac09
Copy link
Collaborator

dac09 commented Dec 9, 2022

Thanks @Tobbe! We should think about moving the CORs config too perhaps (not in this PR obviously) - because to me with the new auth "thing" we have in in src/web/auth it feels like the most obvious place to configure your client!

@dac09 dac09 merged commit b480d88 into redwoodjs:main Dec 9, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Dec 9, 2022
@redwoodjs-bot
Copy link

redwoodjs-bot bot commented Dec 9, 2022

🔔 @jtoar, @Tobbe—I couldn't cherry pick this one. If you want it in the next release, you'll have to cherry pick it manually.

@Tobbe Tobbe deleted the tobbe-api-dbauth-url branch December 9, 2022 15:53
@jtoar jtoar modified the milestones: next-release, v4.0.0 Dec 10, 2022
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
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants