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

Prevent clear auth data when getSession error #763

Open
2 of 5 tasks
nvthuong1996 opened this issue Jun 5, 2024 · 9 comments
Open
2 of 5 tasks

Prevent clear auth data when getSession error #763

nvthuong1996 opened this issue Jun 5, 2024 · 9 comments
Labels
enhancement An improvement that needs to be added rfc Request for Comments: Needs discussion

Comments

@nvthuong1996
Copy link

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.

How would you implement this?

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

Additional information

  • Would you be willing to help implement this feature?

Provider

  • AuthJS
  • Local
  • Refresh
  • New Provider
@nvthuong1996 nvthuong1996 added enhancement An improvement that needs to be added pending An issue waiting for triage labels Jun 5, 2024
@mbellamyy
Copy link

mbellamyy commented Jun 5, 2024

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

@zoey-kaiser zoey-kaiser added rfc Request for Comments: Needs discussion and removed pending An issue waiting for triage labels Jun 27, 2024
@zoey-kaiser
Copy link
Member

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!

@nvthuong1996
Copy link
Author

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

@zoey-kaiser
Copy link
Member

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).

@Ulrich-Mbouna
Copy link

Some updates for this error ?

@bitfactory-frank-spee
Copy link
Contributor

It would be nice if the getSession method throws an error instead of just clearing the session data and returning null:

// Clear all data: Request failed so we must not be authenticated

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 : '/');
    }

@sadeghi-aa
Copy link

@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
Copy link

sadeghi-aa commented Feb 10, 2025

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.

catch (err) {
if (!data.value && err instanceof Error) {
console.error(`Session: unable to extract session, ${err.message}`)
}
// Clear all data: Request failed so we must not be authenticated
data.value = null
rawToken.value = null
}

@sadeghi-aa
Copy link

sadeghi-aa commented Feb 10, 2025

I created this PR (#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement that needs to be added rfc Request for Comments: Needs discussion
Projects
None yet
Development

No branches or pull requests

6 participants