nuxt-modules / supabase

Supabase module for Nuxt.
https://supabase.nuxtjs.org
MIT License
706 stars 129 forks source link

Improve module supabaseClient/GoTrueClient handling, session management and switch to PKCE flow #209

Closed Aietes closed 1 year ago

Aietes commented 1 year ago

First of all, this module is great, thank you for all the hard work on it!

A warning message when using the nuxt supabase module: Multiple GoTrueClient instances detected in the same browser context. like mention in #203 led us to look into the root cause of the issue, and led to a number of insights. I’d like to contribute to this module, to make it better!

I’ve implemented my own plugin to connect to Supabase, and have achieved a clean flow without errors. In this process I came across a couple of things that I believe could greatly improve this module.

  1. Most importantly, the supabaseClient/GoTrueClient should only be created once. I’m achieving this by creating the supabaseClient in the respective server and client plugin and use provide() to make it available in the app, and then never recreate it. In the module it’s recreated in the composables, which leads to issues and should be avoided.

  2. I’m using the PKCE flow, that is now available on all Supabase projects. Since it’s more secure, it would be great if the module would adopt a PKCE flow by default. This should be fairly straightforward on the client, and setting cookies on the server for SSR is already implemented in the module anyway.

  3. Persisting the session can be handled by the supabase-js library, making the supabase nuxt model leaner and ensure no conflicts in how the session is handled occur. This also applies to how the user is retrieved, this should happen from the session provided by supabase-js.

I’d like to contribute and help with the changes above. I’ve already implemented them in a separate plugin, I’m now looking for the best way to engage with you to help. Since some of the changes are more substantial in how things work, it’s not simply a number of small pull requests. @larbish Are you interested in discussing some of the changes and let me help with them?

larbish commented 1 year ago

Hey @Aietes, It seems the warning you are experiencing have been introduced by a new version of supabase or nuxt. I need to dig into that. However, I'll be really happy to discuss it with you in order to fix it and implement the PKCE flow. Let's find a moment to discuss that !

riemervdzee commented 1 year ago

Maybe it's some help, but some of the warnings disappeared when I downgraded @supabase/gotrue-js to 2.42.0. It was previously on 2.44.1 as sub-dependency.

The downgrade also fixed my issue similar to https://github.com/nuxt-modules/supabase/issues/208

Aietes commented 1 year ago

I have done some work to implement the changes above. We are currently testing this out in one of our projects using Supabase. If you want to take a look or test you can find a fork here, and a test package under @aietes/supabase:

pnpm add -D @aietes/supabase

There are some breaking changes in there since things are handled quite differently. Basically, the module handles the entire auth flow automatically, no additional watches, events or forwards are needed. The default settings assume route '/login' initiating the login, redirecting to '/confirm' on successful authentication. On the confirm page you would redirect or apply any routing logic you want in your app. By default, a middleware is added to check for a valid session, which redirects to login if a valid session cannot be found or refreshed.

For the changes that make sense, I'll work with @larbish to integrate them into the official module.

CptJJ commented 1 year ago

Thank you @Aietes for this, I was looking to give your changes a try and notices you removed serversupabaseuser? did you find that it was having/causing issues? How do you plan to authenticate users on the server side if at all?

Edit:

Seems like you can just call:

  const { data: { user: supabaseUser }, error } = await client.auth.getUser(event.context._token)
CptJJ commented 1 year ago

I was in desperate need to fix a lot of the issues that have been magically turning up with my authentication flow over the last week and I was able to get everything working flawlessly by forking off a lot of the work done by @Aietes, so thank you a ton for this.

If anyone as interested you can use my changes found here: https://www.npmjs.com/package/@cptjj/supabase

It removes the automatic navigation of Aietes fork, and reintroduces the serverSupabaseUser, just make sure to set redirect: false in your public config under supabase: {}. Authentication flow is not using PKCE because i user username and password w/ no email verification on my app and its not support. There is no more need for the useSupabaseAuthClient so just use use useSupabaseClient().auth for signin signup etc.

This is how i handle navigation post sign in and have had no issues so far, everything is SSR:

       try {
            loading.value = true
            const { error } = await auth.signInWithPassword({
                email: state.value.email,
                password: state.value.password,
            })
            if (error) throw error
            await useSignIn() // all your signin or signup logic 
            await navigateTo('/quotes')
        } catch (e) {
            alert.value = e.message
        } finally {
            loading.value = false
        }

Im new to both webdev and opensource so would love to assist with merging this stuff will just need some guidance!

Hope this helps someone!

Aietes commented 1 year ago

@CptJJ I'm glad it helped! As you pointed out, getting the user on the server is easy like you referenced above. However, it would probably be good to add the serverSupabaseUser back in, I didn't really leave it out on purpose. I'll also take a look at adding the Implicit authorization flow as an option, in case the PKCE flow is not applicable. However, the PKCE flow should be preferred for security reasons, if possible.

Automatic navigation and route handling can be disabled by setting redirect:false in the plugin options, in that case, the middleware for automatically checking auth is not added, and checking authentication and routing needs to be handled manually. All this requires updated documentation for the module, so far I only implemented things as a proof-of-concept, to then work with @larbish to bring it into the official module for the Nuxt community.

Aietes commented 1 year ago

I just added clientOptions for the module to set options for the SupabaseClient. This allows setting the flowType in auth to implicit like this in nuxt.config.ts :

export default defineNuxtConfig({
  supabase: {
    clientOptions: {
      auth: {
        flowType: 'implicit',
      },
    },
  },
})

Of course you can also set any other option for the SupabaseClient. I also made some adjustments to the typings, and removed some legacy code from the module. For testing, I released version 0.4.2 of the npm package @aietes/supabase. The fork can still be found here.

CptJJ commented 1 year ago

@Aietes I was having issues with redirect false so i implemented that logic directly into middleware in my fork FYI. Also added my own version of serverSupabaseUser that correctly loads the users based off the auth session that your setting in the serverclient.

The other thing i dont fully understand is that useSupabaseUser is no longer a ref and just a promise? Im guessing this is no longer needed because the signin functionality correctly sets the user so this value should never be changing besides calling signin and signout? Regardless things have been super predictable over the last 24 hours in my production app so +1 for bringing these changes to the nuxt module

Aietes commented 1 year ago

The useSuperbaseUser is a promise because the underlying method to get the user is async. While the user might be correctly set initially, we can only be sure of a valid session by checking with getSession(). This is not only to determine the authenticated state but will also take care of refreshing the session when it expires. I guess we could wrap it into a ref, like it was before if required.