feat!: Migrate to `@supabase/ssr`

felixgabler commented 6 months ago

Started using @supabase/ssr to create clients, simplifying cookie handling.

https://supabase.com/docs/guides/auth/server-side/creating-a-client

Types of changes

[x] Bug fix (a non-breaking change which fixes an issue)
[ ] New feature (a non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Description

Resolves: #301

I replaced the plain usage of createClient (in serverSupabaseClient, supabase.server.ts, and supabase.client.ts) with the more advanced createServerClient and createBrowserClient from @supabase/ssr. In the process, I removed some of the manual cookie handling since it should be taken care of by Supabase's auth.storage setting. This is based on my limited understanding though, so would appreciate a confirmation.

This was because we were encountering quite a few issues of cookies getting lost somewhere. Now everything seems to work fine again.

I was not entirely sure what to do about the clientOptions since they should be managed by @supabase/ssr now. Also, the cookieName is now also quite irrelevant and I did not find a good way to include it in the new setup for the other cookies. One idea would be to prefix them all again through the cookies option functions?

Checklist:

[x] My change requires a change to the documentation.
[x] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes (if not applicable, please state why): No testing structure available

netlify[bot] commented 6 months ago

Deploy request for n3-supabase pending review.

Visit the deploys page to approve it

Name	Link
Latest commit	830161c4699e020b6eec4da46f3b0c5f6c6172af

felixgabler commented 6 months ago

There is another issue that comes up now:

WARN Using the user object as returned from supabase.auth.getSession() or from some supabase.auth.onAuthStateChange() events could be insecure! This value comes directly from the storage medium (usually cookies on the server) and may not be authentic. Use supabase.auth.getUser() instead which authenticates the data by contacting the Supabase Auth server.

This is actually something that should be changed in the library regardless of whether this PR is merged. getSession is not as secure as getUser, so one should assess where it makes sense to switch to the additional network request.

EDIT: After upgrading supabase-js (see https://github.com/supabase/auth-js/issues/873#issuecomment-2078077562), they don't seem to come up anymore. However, I still think it would be important to think about whether the current approach is insecure at times.

larbish commented 6 months ago

Hello @felixgabler, I' don't have a lot of time for this module since I've created it for a side project that do not need evolution no more and I'm so happy when people help me to maintain it and make it evolve, thanks for that!

Concerning the PR, I faced two blocking points:

Server side session/user was not up to date on first load. Since you removed this line, the session was not set on server side on first load.
I am still facing the insecure warn about getSession

After reading the Supabase docs concerning the getSession method and taking into account your integration of supabase/user (the removal of the legacy custom server side synchronisation), I told my self that it could be the time for a refactor. So I decided to transform useSupabaseUser and useSupabaseSession into async composable. It ensures data are always up to date when called on purpose. I've updated the useSupabaseUser to be consistent with Supabase docs and avoid insecure call of getSession on server side.

Those changes include breaking changes but it make sense to release a major version once this PR is merged to be sure users make some complete test to upgrade the module.

Tell me what you think of this and if it's working well on your project.

Also I want to get rid of the getSession warning before merging this, you talked about upgrading supabase-js but I still have it.

felixgabler commented 6 months ago

Those changes include breaking changes but it make sense to release a major version once this PR is merged to be sure users make some complete test to upgrade the module.

That all sounds very reasonable and I welcome the changes you made to keep things more secure! Very cool.

Tell me what you think of this and if it's working well on your project.

We deployed my branch and have not faced issues so far. It actually fixed some bugs (we set cookies on the top-level domain which the subdomains inherit and I suspect something broke around this but works now with @supabase/ssr).

Also I want to get rid of the getSession warning before merging this, you talked about upgrading supabase-js but I still have it.

That is weird.. I don't get them anymore after the upgrade. Perhaps you need to delete the node_modules and .nuxt?

felixgabler commented 6 months ago

If useSupabaseUser is now async as well, will this lead to even more warnings like the following because there might be sequential promises in a component setup? Stumbling upon this and cannot find clear guidelines of how to deal with it in Nuxt.

[nuxt] [useLazyAsyncData] Component is already mounted, please use $fetch instead

A simple example would be fetching the user profile with useLazyAsyncData after getting the user ID from useSupabaseUser. How would you go about this?

larbish commented 6 months ago

I pushed two new fix:

I was facing this issue after releasing and using the module on another project: https://github.com/supabase/auth-helpers/issues/725 so i fixed it by optimising this lib
Use getUser instead of getSession for serverSupabaseUser

larbish commented 6 months ago

I tried to play with useLazyAsyncData and the async resolution of the user and I'm not facing any issue/warning. Could you provide an example in the playground of the module?

felixgabler commented 6 months ago

I created a branch with a repro of the warning: https://github.com/felixgabler/supabase-nuxt/tree/warning-repro

The warning is shown when navigating between /login and /unprotected using the buttons.

However, I might have figured out what the problem is. Does "useAsyncData is a composable meant to be called directly in the Nuxt context" (Nuxt Docs) mean that these composables can't be reused in other composables? That would be somewhat annoying but would at least explain the warning.. If this is indeed true, do you know what the "right" way would be to share this user state?

larbish commented 6 months ago

Indeed I don't think that calling useAsyncData inside a composable makes sense. I don't exactly understand what you are trying to achieve so maybe I need more info to propose something but one solution could be to create a state using the useState in your composable. Then you can update this state from the page where it makes sense (using useAsyncData). Each time you need to access this state from another page/component, you can call this composable and access the state.

larbish commented 6 months ago

Let's closely follow this issue. Once it's fixed, we can merge this PR . Wdyt?

felixgabler commented 6 months ago

Indeed I don't think that calling useAsyncData inside a composable makes sense. I don't exactly understand what you are trying to achieve so maybe I need more info to propose something but one solution could be to create a state using the useState in your composable. Then you can update this state from the page where it makes sense (using useAsyncData). Each time you need to access this state from another page/component, you can call this composable and access the state.

We put useAsyncData in a composable because we have many pages/components in our app that work on the same underlying data. The problem is that there is not really one component that is always there which could become the owner of the useAsyncData, where we could update a shared state. This is where the reusability of composables really comes in handy. Does that make sense?

felixgabler commented 6 months ago

Let's closely follow this issue. Once it's fixed, we can merge this PR . Wdyt?

Sounds good to me!

larbish commented 6 months ago

Maybe you can try with this experimental feature: https://nuxt.com/docs/guide/going-further/experimental-features#asynccontext

Or what about calling the useAsyncData inside a root page or even in a plugin on app load?

felixgabler commented 6 months ago

Maybe you can try with this experimental feature: https://nuxt.com/docs/guide/going-further/experimental-features#asynccontext

Unfortunately, this does not remove the warnings. The good thing right now is that at least it works in SPA mode. It is only that the warnings are annoying but they can be lived with.

Or what about calling the useAsyncData inside a root page or even in a plugin on app load?

The issue here is that we also don't want to load all data if it won't be used. So calling it inside a root page or plugin is also not optimal. The only way I can think of making the useAsyncData reusable would be to extract and share its arguments, i.e., the fetching function and options. But this is also an ugly solution in my opinion.

Especially given that the convention is that all composables (i.e., anything with use... as oppose to define...) can again be composed in Vue, I think this could be an area of improvement either in docs or making it possible to compose useAsyncData.

larbish commented 6 months ago

Still facing the warning even after upgrading to the latest supabase-js release.

To discuss with Supabase team here: https://github.com/supabase/auth-js/pull/895

felixgabler commented 6 months ago

Still facing the warning even after upgrading to the latest supabase-js release.

To discuss with Supabase team here: supabase/auth-js#895

I wonder if it fell off the plate. Perhaps best to create a new issue?

felixgabler commented 6 months ago

Maybe you can try with this experimental feature: https://nuxt.com/docs/guide/going-further/experimental-features#asynccontext

Unfortunately, this does not remove the warnings. The good thing right now is that at least it works in SPA mode. It is only that the warnings are annoying but they can be lived with.

Or what about calling the useAsyncData inside a root page or even in a plugin on app load?

The issue here is that we also don't want to load all data if it won't be used. So calling it inside a root page or plugin is also not optimal. The only way I can think of making the useAsyncData reusable would be to extract and share its arguments, i.e., the fetching function and options. But this is also an ugly solution in my opinion.

Especially given that the convention is that all composables (i.e., anything with use... as oppose to define...) can again be composed in Vue, I think this could be an area of improvement either in docs or making it possible to compose useAsyncData.

@Atinux could I perhaps also get your thoughts on this? I was quite surprised to learn that useAsyncData is not composable

larbish commented 6 months ago

Still facing the warning even after upgrading to the latest supabase-js release. To discuss with Supabase team here: supabase/auth-js#895

I wonder if it fell off the plate. Perhaps best to create a new issue?

Indeed I've create a new one: https://github.com/supabase/auth-js/issues/912

felixgabler commented 4 months ago

Opening another thread, I have also added a branch to my fork that adds ofetch-like retrying to Supabase calls. We got quite a lot of Sentry reports ("Load failed" and other flaky errors) that got reduced quite a bit by this. However, I was not able to add ofetch directly but had to use a separate function for some reason. What do you think of this approach? Can it be made cleaner by using ofetch directly somehow?

XStarlink commented 4 months ago

Vous pouvez peut-être essayer cette fonctionnalité expérimentale : https://nuxt.com/docs/guide/going-further/experimental-features#asynccontext

Malheureusement, cela ne supprime pas les avertissements. La bonne nouvelle pour le moment est qu'au moins cela fonctionne en mode SPA. C'est juste que les avertissements sont ennuyeux mais on peut les supporter.

Ou que diriez-vous d'appeler l' useAsyncDataintérieur d'une page racine ou même d'un plugin lors du chargement de l'application ?

Le problème ici est que nous ne voulons plus charger toutes les données si elles ne sont pas utilisées. Par conséquent, l'appel à l'intérieur d'une page racine ou d'un plugin n'est pas plus optimal. La seule façon dont je peux penser à rendre le useAsyncDataréutilisable serait d'extraire et de partager ses arguments, c'est de dire la fonction de récupération et les options. Mais c'est aussi une solution miracle à mon avis. Surtout étant donné que la convention est que tous les composables (c'est-à-dire tout ce qui use...est opposé à define...) peuvent à nouveau être composés dans Vue, je pense que cela pourrait être un domaine d'amélioration soit dans la documentation, soit en permettant de composer useAsyncData.

@AtinuxPourrais-je également avoir votre avis sur ce sujet ? J'ai été assez surpris d'apprendre que ce useAsyncDatan'est pas composable

Hi @felixgabler, I read your comment and since I've already encountered this problem, I thought I'd reply. I came across this video by Alexandre Lichter core Nuxt team member, which I recommend you watch. It explains that useFetch() and useAsyncData() should only be used at the root of <script setup>, and that $fetch() should be used in all other cases (inside functions, stores, composables).

I don't know exactly what you want to do in your case, but to share the data and be able to use it just where it's needed I use Pinia stores, with actions that contain API calls using $fetch(), the return of these API calls is stored in the store state (For large data collections, as explained here, you can use shallowRef() to avoid making all object keys reactive), then in the pages where I need the data I call these actions in a useAsyncData(() => { storeName.getSomeData() }) at the root of the page's script setup, and use the data through the store where I need it (storeName.stateData).

I find that stores are more efficient than composables for managing data coming from API requests, because they allow access to their state anywhere in the Nuxt app easily.

Anyway, sorry for the off topic, I wanted to thank you both for the work you've done to move to @supabase/srr.

I've been having a big problem for some time in my Nuxt app, my users' Supabase session is lost after a while, and in my /server routes when I want to do a supabase.auth.getUser() to be able to use the user currently connected I just get null, so I respond to the front with an Unauthorized but as a result my users have the impression that my app is bugging a lot when it's just a problem of a session that gets lost at some point.. (I opened an issue here https://github.com/nuxt-modules/supabase/issues/381)

When I did some research, I saw that it happened to other people, but I didn't see any solutions. I saw yesterday that this PR was open, I really hope that would be the solution to theses problems.

Thanks again for your efforts

larbish commented 4 months ago

@felixgabler, I will merge this PR. Users have been raising concerns about outdated dependencies and other issues within the module. My intention was to wait for this merge, as managing updates across two separate branches is hard but I can't wait no more.

The warning mentioned is non-disruptive and does not affect functionality, so merging this now should help resolve some of the existing issues, including those you've reported @XStarlink. Additionally, if users continue to encounter the warning, they can upvote the issue and it might encourage the supabase maintainers to address it.

Concerning your ofetch reflexion, let's create another PR for this :)

Once again, thanks for your help on this PR 💚

nuxt-modules / supabase