nuxt-auth icon indicating copy to clipboard operation
nuxt-auth copied to clipboard

Prevent clear auth data when getSession error

Open nvthuong1996 opened this issue 1 year ago • 13 comments

Describe the feature

If getSession throws an exception, the backend is most likely at fault: If you clear auth data, all users using the website will have to log in again.

https://github.com/sidebase/nuxt-auth/blob/faa039b72da2c64d9c214f0502dea053c7a4ff84/src/runtime/composables/local/useAuth.ts#L99

How would you implement this?

Only clear the session when the http error code returned from the backend is 401

Additional information

  • [X] Would you be willing to help implement this feature?

Provider

  • [ ] AuthJS
  • [X] Local
  • [ ] Refresh
  • [ ] New Provider

nvthuong1996 avatar Jun 05 '24 13:06 nvthuong1996

I'm having the same problem with refresh method.

For example, when the user disconnects their Wifi and periodic refresh is turned on, with the first failed request, the user is logged out because the token data is removed from the state.

The tokens should only be removed in three cases imo:

  • the user explicitly wants them removed (by signing out)
  • the tokens are expired... A refresh token that is still valid should not be nullified.
  • a request to refresh (and get a valid access token) fails on SSR

mbellamyy avatar Jun 05 '24 19:06 mbellamyy

Hi @mbellamyy and @nvthuong1996 👋

I added the rfc label to this issue. I would pull in @phoenix-ru, and we can revisit the current behavior and discuss potential modifications to it!

zoey-kaiser avatar Jun 27 '24 08:06 zoey-kaiser

hello @phoenix-ru @zoey-kaiser ! has any update for this issue ?

nvthuong1996 avatar Jul 10 '24 04:07 nvthuong1996

Hi @nvthuong1996 👋

We are currently focusing on finishing up the documentation rewrites, which is why we are currently less actively pushing forward discussions on new issues. Once we have the docs deployed we will revisit this issue (should be pretty soon).

zoey-kaiser avatar Jul 11 '24 16:07 zoey-kaiser

Some updates for this error ?

Ulrich-Mbouna avatar Aug 08 '24 22:08 Ulrich-Mbouna

It would be nice if the getSession method throws an error instead of just clearing the session data and returning null: https://github.com/sidebase/nuxt-auth/blob/52d4b9a5b48431eb9d522865410d209c6d66a613/src/runtime/composables/local/useAuth.ts#L147

I made a workaround using the callGetSession option set to false of the signIn method for the local provider, but I am not sure if you can do the same with refresh calls. And I still need to do a manual getSession like call which is for us this:

public async loginWith(...): Promise<void> {
        const { getSession, signIn } = useAuth();

        await signIn(
            {...},
            {
                // Don't redirect and don't call get session, because login must catch OTP/2FA errors from getSession.
                callGetSession: false,
                // The callback URL is only set to prevent the sidebase signIn method from doing extra work.
                // Improvement issue opened: https://github.com/sidebase/nuxt-auth/issues/978
                callbackUrl: '/',
                redirect: false,
            },
        );

        // NOTE: This extra fetch is needed because of how getSession works. It does not return error response data.
        // See https://github.com/sidebase/nuxt-auth/blob/main/src/runtime/composables/local/useAuth.ts#L142
        // This is a custom API call to check if we get an error. If it returns an error it is caught upstream.
        await useNuxtApp().$backendFetch('/user/me');

        // Second is a call to really get the user session so the Sidebase Auth module is filled. Redirect after.
        await getSession();
        const { redirectedFrom } = useRoute();
        navigateTo(redirectedFrom ? redirectedFrom.fullPath : '/');
    }

bitfactory-frank-spee avatar Jan 09 '25 08:01 bitfactory-frank-spee

@bitfactory-frank-spee

I have a similar issue (described in detail in #998). The workaround you mentioned doesn't appear to fix the issue I've described, does it? Do you know any solution for my issue?

sadeghi-aa avatar Feb 10 '25 14:02 sadeghi-aa

As a suggestion: It would be nice if we could pass a custom errorHandler function to override the catch block of the getSession function. This way the developer could decide when and how to logout the user; it could be a 401 error or any other custom condition. And if not passed, the default behavior takes precedence.

https://github.com/sidebase/nuxt-auth/blob/956b0fa1b5aed3fb94a47563a9e47dc811940a6a/src/runtime/composables/local/useAuth.ts#L143-L151

sadeghi-aa avatar Feb 10 '25 16:02 sadeghi-aa

I created this PR (https://github.com/sidebase/nuxt-auth/pull/999). I'm generally new to contributing to open-source projects, so forgive me if I've made any mistakes in the PR. I deliberately left the PR as minimal as possible to get feedback and of course, to see if the feature is actually a valid feature in your opinion.

Update: Closed the PR for now because it doesn't work. Will reopen it when it works.

sadeghi-aa avatar Feb 10 '25 16:02 sadeghi-aa

It looks like this issue is going stale for some time now. I think @sadeghi-aa has a good point on how to solve it with a hook without breaking existing functionality. I don't know if this is still relevant for the latest nuxt-auth version. Shame the #999 PR is not finished. It looks like a good idea, maybe not do a return in the catch though. Just call the hook.

bitfactory-frank-spee avatar Nov 07 '25 10:11 bitfactory-frank-spee

@bitfactory-frank-spee

I have a similar issue (described in detail in #998). The workaround you mentioned doesn't appear to fix the issue I've described, does it? Do you know any solution for my issue?

Issue https://github.com/sidebase/nuxt-auth/issues/998 is indeed the same as this one, so looks duplicate to me. But I see in that issue that it is actually called a as duplicate of https://github.com/sidebase/nuxt-auth/issues/964 which links to https://github.com/sidebase/nuxt-auth/pull/1062. @phoenix-ru it would be a good idea to consolidate all these issues into 1 main one and close the others IMO.

@sadeghi-aa I don't know why my workaround would not work anymore or for you, we are still using it. Although I am not sure which version of nuxt-auth we are currently using with it in our projects. You do need to check if callGetSession is available in your used version.

bitfactory-frank-spee avatar Nov 07 '25 10:11 bitfactory-frank-spee

Shame the https://github.com/sidebase/nuxt-auth/pull/999 PR is not finished

Actually you'll get a way for customizing anything in the local provider (which will be a new provider) when #1062 lands 🙂 That PR is not so far from being ready for testing, after which it'll land as an experimental option.

You'll be able to have a way better flexibility with it, so please don't hesitate to look into the PR and give early feedback. You can also read the docs (either in .md or if you checkout the PR and run pnpm docs:dev)

would be a good idea to consolidate all these issues into 1 main one and close the others IMO.

Agreed, will do the next time I am working on NuxtAuth

phoenix-ru avatar Nov 11 '25 08:11 phoenix-ru

Shame the #999 PR is not finished

Actually you'll get a way for customizing anything in the local provider (which will be a new provider) when #1062 lands 🙂 That PR is not so far from being ready for testing, after which it'll land as an experimental option.

You'll be able to have a way better flexibility with it, so please don't hesitate to look into the PR and give early feedback. You can also read the docs (either in .md or if you checkout the PR and run pnpm docs:dev)

That sounds really nice, excellent work 👍 👍 I will look into that.

would be a good idea to consolidate all these issues into 1 main one and close the others IMO.

Agreed, will do the next time I am working on NuxtAuth

👍 🙌

bitfactory-frank-spee avatar Nov 11 '25 08:11 bitfactory-frank-spee