sidebase / nuxt-auth

Authentication built for Nuxt 3! Easily add authentication via OAuth providers, credentials or Email Magic URLs!
https://auth.sidebase.io
MIT License
1.22k stars 156 forks source link

Setting getSession endpoint to false results in unauthenticated status even if the user successfully logged in #781

Closed HELWATANY closed 1 month ago

HELWATANY commented 1 month ago

Environment

------------------------------
- Operating System: Windows_NT
- Node Version:     v20.14.0
- Nuxt Version:     3.11.2
- nuxt-auth:        0.8.0-rc.1
- CLI Version:      3.11.1
- Nitro Version:    2.9.6
- Package Manager:  pnpm@9.4.0
- Builder:          -
- User Config:      devServer, modules, build, auth, routeRules
- Runtime Modules:  ../src/module.ts --> _(@sidebase/nuxt-auth@0.8.0-rc.1)_
- Build Modules:    -
------------------------------

Reproduction

Reproduction Steps:

Expected Result:

Actual Result:

Nuxt Config Used To Reproduce The Issue

export default defineNuxtConfig({
  modules: ['../src/module.ts'],
  build: {
    transpile: ['jsonwebtoken']
  },
  auth: {
    provider: {
      type: 'local',
      endpoints: {
        getSession: false
      },
      pages: {
        login: '/'
      },
      token: {
        signInResponseTokenPointer: '/token/accessToken'
      },
      session: {
        dataType: { id: 'string', email: 'string', name: 'string', role: "'admin' | 'guest' | 'account'", subscriptions: "{ id: number, status: 'ACTIVE' | 'INACTIVE' }[]" },
        dataResponsePointer: '/'
      }
    },
    sessionRefresh: {
      // Whether to refresh the session every time the browser window is refocused.
      enableOnWindowFocus: true,
      // Whether to refresh the session every `X` milliseconds. Set this to `false` to turn it off. The session will only be refreshed if a session already exists.
      enablePeriodically: 5000
    },
    globalAppMiddleware: {
      isEnabled: true
    }
  },
  routeRules: {
    '/with-caching': {
      swr: 86400000,
      auth: {
        disableServerSideAuth: true
      }
    }
  }
})

Describe the bug

Hello Everyone, I was trying the new version 0.8.0-rc.1 to plan for the upgrade. While testing the playground-local I noticed that when setting the endpoints.getSession to false the user status is still unauthenticated even after successful login. I able to retrieve the token using const { token } = useAuth() but the const { status } = useAuth() still returns unauthenticated

Additional context

In case the session endpoint is set to false the auth status should be set to authenticated after successful login, and the developer should handle forcing logout of the current user in the way he/she sees fit (when getting status code 401 when calling his API endpoint for example)

Logs

No response

phoenix-ru commented 1 month ago

At first I thought it was a bug introduced in #768, but now I think this is a conceptual misunderstanding.

The sole purpose of getSession is to make sure that whatever session data user has is valid, for instance JWT has correct signature and not expired, user exists in the system, etc.

By disabling getSession you are basically putting nuxt-auth into a state where it has no knowledge of the session validity - in this sense merging #768 was a mistake (cc @zoey-kaiser it may be worth reverting for better stability).

At the current moment nuxt-auth relies on session data internally to determine the status: https://github.com/sidebase/nuxt-auth/blob/cb3db19563f8cb3af6dd63097e91305697c3ce8a/src/runtime/composables/commonAuthState.ts#L23-L31

And it cannot rely on any kind of tokens/cookies/etc. because:

  1. these are implementation details which differ between providers;
  2. as mentioned before, token is just a string and there is no reliable way to verify it on client.

Assuming a user is authenticated after doing successful sign in without having any session is not really possible. Due to security reasons I would not approach sessionless authentication without thorough considerations.

Armillus commented 1 month ago

Hi @phoenix-ru,

I just wanted to clarify a few things, since it seems there are a few shortcuts in your reasoning, in my opinion.

On one hand, the "conceptual misunderstanding" that you are referring to has been introduced by the documentation of the library, which explicitly mentions that it can be disabled (second example) from a few versions now.

Hence, in this regard, merging the PR was probably not a mistake, because it was a bugfix for a documented feature. It would have probably been better to merge it retroactively in an older version and to revert its effect in the last version, since it looks like a breaking change.

On the other hand, although the security concerns you are raising are completely understandable, they just represent one point of view about the question. Other libraries have different approaches, since authjs (which is used by this library) does not make the getSession() call in their equivalent of the signIn function, and nuxt/auth handles this with an autoFetch boolean. This is handy, because user data might be located in the response from the login endpoint, or developers might just want to control where and how to store user data.

I'm not saying that your approach is bad or wrong, but just be aware that you're restraining developer capabilities for the sake of security with such a choice. If this is an opinionated choice or if the design of the library cannot fit this way of thinking for now, then no problem, but the documentation must be fixed accordingly, as it is currently misleading (https://github.com/sidebase/nuxt-auth/issues/761 is an example).

Anyways, I'll be glad to help in this regard if I can :)

phoenix-ru commented 1 month ago

@Armillus Don't get me wrong, I am just setting priorities straight - and releasing a stable 0.8.0 is higher priority than doing a potentially unsecure refactor which I am certain would postpone the release by two weeks.

by the documentation of the library, which explicitly mentions that it can be disabled

I agree with you, that is why we have #783 which is a general overhaul of the documentation which is somewhat outdated.

you're restraining developer capabilities for the sake of security with such a choice

This was the point of reverting, as nuxt-auth is valuing security in the first place


Aside from this, I'd be glad if you could outline a proposal which we can then discuss and implement. Having parity with authjs would add good value to supporting local/refresh providers, in my opinion.

Please also note that our company internally makes heavy use of authjs provider, including for cases which are covered by the other two providers. This in turn means that there are less chances for us to extensively test different features of community-lead effort.

HELWATANY commented 1 month ago

At first I thought it was a bug introduced in #768, but now I think this is a conceptual misunderstanding.

The sole purpose of getSession is to make sure that whatever session data user has is valid, for instance JWT has correct signature and not expired, user exists in the system, etc.

By disabling getSession you are basically putting nuxt-auth into a state where it has no knowledge of the session validity - in this sense merging #768 was a mistake (cc @zoey-kaiser it may be worth reverting for better stability).

At the current moment nuxt-auth relies on session data internally to determine the status:

https://github.com/sidebase/nuxt-auth/blob/cb3db19563f8cb3af6dd63097e91305697c3ce8a/src/runtime/composables/commonAuthState.ts#L23-L31

And it cannot rely on any kind of tokens/cookies/etc. because:

  1. these are implementation details which differ between providers;
  2. as mentioned before, token is just a string and there is no reliable way to verify it on client.

Assuming a user is authenticated after doing successful sign in without having any session is not really possible. Due to security reasons I would not approach sessionless authentication without thorough considerations.

Hello @phoenix-ru , I get your point now as you explain it, I agree with you on this point, after receiving a token there should be some validation of the token's authenticity.

I only wrote this since I found that getSession can be disabled in the documentation as I and @Armillus mentioned.

sorry for the inconvenience caused by my issue.

zoey-kaiser commented 1 month ago

Hi! We have made it not possible to disable the getSession endpoint for this reason and have explicitly mentioned in the new docs that this endpoint must be set: https://auth.sidebase.io/guide/local/quick-start#api-endpoints

I hope this resolves the confusion that the incomplete docs created.