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

User property is required for fetch hook to run #175

Closed
katlyn opened this issue Sep 14, 2024 · 3 comments · Fixed by #188
Closed

User property is required for fetch hook to run #175

katlyn opened this issue Sep 14, 2024 · 3 comments · Fixed by #188
Labels
question Further information is requested

Comments

@katlyn
Copy link

katlyn commented Sep 14, 2024

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

atinux commented Sep 14, 2024

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?

@atinux atinux added the question Further information is requested label Sep 14, 2024
@katlyn
Copy link
Author

katlyn commented Sep 14, 2024

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.

Copy link
Owner

atinux commented Sep 16, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants