atinux / nuxt-auth-utils

Add Authentication to Nuxt applications with secured & sealed cookies sessions.
MIT License
962 stars 90 forks source link

User property is required for fetch hook to run #175

Closed katlyn closed 1 month ago

katlyn commented 2 months ago

Currently, user is required to be defined on the session object before the fetch hook will be run. This logic was added several months ago in #130. While this isn't a breaking change according to types, it does remove any ability to populate session data unless a user is signed in which was previously possible.

For my use case i would like the user property to be populated dynamically by a session hook based on other information in the session, but the changes in #130 prevent this.

While the easy solution would be to add a new hook that always runs regardless of the value of the user key, I think care needs to be taken to prevent convoluted names. The fetch hook name implies that it is run every time the session is fetched, and has no hint that it will only be run if a user is authenticated. Perhaps a better approach would be to change the fetch hook to have the signature of (session: UserSession, event: H3Event) => void | Promise<void>, calling the hook with every session fetch. Additionally, a secondary hook authenticatedFetch: (session: UserSessionRequired, event: H3Event) => void | Promise<void> (open to naming suggestions) could be added that would be called when the session is fetched with an authenticated user, mirroring the current functionality of the fetch hook.

Changing the hooks in this way would technically be a breaking change, however as already mentioned the current functionality implemented in #130 was already partially breaking for some use cases. I'm happy to open a PR to implement my suggested changes, but would like some feedback on them (particularly on the naming for the new hook that may be created).

atinux commented 2 months ago

Hey @katlyn

Actually the logic was already there as it was using requireUserSession(event) which is throwing if no user is defined.

Would you be happy to explain me in details your usage or not using the .user?

katlyn commented 2 months ago

Session data can contain much more than just user data - I want to be able to run session hooks to validate and populate data in a session regardless of if the user is logged in, as the session exists independently of a user being logged in. This is enforced and suggested by allowing typed to be extended for the UserSession interface.

As for my use case, users authenticating to my application must sign into two individual IDPs. As the user is not authenticated when only signed in with one, I don't want to populate user with information from either of the two IDPs as that will make loggedIn return true. Ideally, what I would like to do is have a session hook that checks the user's session to see if they have authenticated with both of the IDPs, and populates the user key with the correct information if so. It would look something like this, as a very basic representation.

interface UserSession {
  github?: GithubUser,
  google?: GoogleUser
}

defineNitroPlugin(() => {
  sessionHooks.hook("fetch", async (session: UserSession, event) => {
    if (session.google && session.discord) {
       // Dynamically populate the session with the correct user based on their authenticated IDPs
      const user = getUser(session.google, session.github);
      await updateSession(event, { user });
    }
  });
});

Alternatively, another use case that the hooks can be used for is validating the session, as suggested by a comment in this example. Only validating the session when a user is present seems a bit counter intuitive, as the session can be used to store much more than just user data. While this functionality isn't directly related to authentication, this library still provides it and suggests it as a usecase, and as far as I'm aware h3 doesn't not provide its own hooks to be able to do similar functionality itself.

atinux commented 2 months ago

I see and your use case makes sense. I am fine updating the hook so it is called every-time as a breaking change.

I might just add a condition to not call it is the session is empty (we always have a session in the end)