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

Add session management example for SvelteKit #6184

Merged
merged 9 commits into from
Jan 4, 2023

Conversation

DoodlesEpic
Copy link
Contributor

Added a code sample for managing the session for auth.js through the $page store from Svelte. The sample demonstrates how to retrieve the session data in the root +page.server.ts file and make it globally accessible through the $page store, simplifying state management in the application. The previous examples already used the data available in this store but did not show how to set it.

Closes #6167

☕️ Reasoning

This is just a small documentation improvement to the SvelteKit documentation which previously lacked an example to show to store the data in the $page store to be available to all routes.

🧢 Checklist

  • Documentation
  • Tests (not needed in this case)
  • Ready to be merged

🎫 Affected issues

Fixes: #6167

@vercel
Copy link

vercel bot commented Dec 25, 2022

@DoodlesEpic is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Dec 25, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
auth-docs ❌ Failed (Inspect) Jan 4, 2023 at 2:21PM (UTC)

@xmlking
Copy link

xmlking commented Dec 25, 2022

it might be a good idea to also document using await parent() in +page.server.js as suggested here:

https://www.youtube.com/watch?v=UbhhJWV3bmI
sveltejs/kit#6315

@DoodlesEpic
Copy link
Contributor Author

DoodlesEpic commented Dec 26, 2022

Honestly this seems much more a shortcoming of SvelteKit than something in auth.js problem space (authentication).

However, I agree we should document this (non obvious) behaviour. event.alwaysRun() seems at a first glance much better than await parent() as if you forget to use await parent() in any of the leafs that is going to create subtle and hard to find bugs. However await parent() also solves the problems of race conditions between the root layout server file and the leaf server page file. Maybe the ideal solution is to do both:

  • Make sure the +layout.server.ts file always runs the validation using event.alwaysRun as suggested by Rich Harris in that linked thread.
  • Also document the need to use await parent() in each of the leaf nodes to guarantee the data in the page store is actually up to date when checking for authorization.

A bit of a tangent here, but hopefully they will add something like +auth.server.ts or something simillar, these are WAY too many footguns for something so essential as authorization.

Typing this from my cell, later at my desk will take a look at this and see if I make some changes to this documentation page.

Of course before the event.alwaysRun function gets implemented we could suggest a trick such as the one here in that thread: sveltejs/kit#6315 (comment)

@DoodlesEpic
Copy link
Contributor Author

Ok, so I've added a section exclusive to handling authorization with the best practices I saw in that thread there (didn't see the linked video yet). I feel quit uneasy about this since, as noted in that thread, it is quite easy to forget about this caveat and create a very serious authorization problem. However, I feel this is a SvelteKit problem, not a auth.js one. Right now I think the best that can be done is documenting the behavior and waiting for changes upstream to remove this foot gun.

@DoodlesEpic
Copy link
Contributor Author

Just took a look at the linked video, the approach there is to just await the parent but it doesn't grab the session from there, instead it relies on the layout having been run (since you just awaited it), that's an interesting approach but I'm not a fan as forgetting to add that has disastrous consequences and will happen very often since this is a bug that happens silently.

Handling this manually in each pages in my opinion is safer as the programmer is forced to await the parent to get the session data and check if it should be redirected every time they want to fetch data in the server.

But honestly even this way it still feels hacky. The same creator has a guide on how to do this same kind of authorization using handle hooks Here if anyone is interested. Again, I don't feel like this is a responsibility of this library as its meant to only handle authentication, but maybe we should also add that authorization method to the docs so that developers can decide what's the better way for them to handle it based on their needs.

@DoodlesEpic
Copy link
Contributor Author

DoodlesEpic commented Dec 26, 2022

Now that I think about it this could be VASTLY simplified if the SvelteKitAuth function we use in hooks.server.ts already handled storing the session information in the locals in some predefined place such as 'session'. It feels kinda invasive modifying this variable by default but I guess as long as it's documented how it works it could be awesome. This would remove the need for that root layout file setting this manually and would prevent all of the consequent problems in authorization in this case.

It could also provide a way to provide rules to protect any routes the developer may wish. The disadvantage is this stops being an authentication library and suddenly also becomes an authorization one which could complicate things a little and increase attack surface.

Another option to make it easier is in the documentation instead of setting the value of the hook directly, instead just call the SvelteKitAuth function with providers, and THEN return that object, this way it's very obvious to the developer they can just modify and read that object and do whatever they want with it, such as handling authorization based on pathNames and such.

This could be made even simpler by using https://kit.svelte.dev/docs/modules#sveltejs-kit-hooks-sequence, the developer can create its own authorization middleware and separate that code from the auth.js stuff.

@xmlking
Copy link

xmlking commented Dec 26, 2022

In case it is useful , I solved some issues with Auth + pre-rendering, I.e bypass auth during build ,
And using guard hook to allow /block to certain pages
https://github.com/xmlking/svelte-starter-kit

@xmlking
Copy link

xmlking commented Dec 26, 2022

Now that I think about it this could be VASTLY simplified if the SvelteKitAuth function we use in hooks.server.ts already handled storing the session information in the locals in some predefined place such as 'session'. It feels kinda invasive modifying this variable by default but I guess as long as it's documented how it works it could be awesome. This would remove the need for that root layout file setting this manually and would prevent all of the consequent problems in authorization in this case.

It could also provide a way to provide rules to protect any routes the developer may wish. The disadvantage is this stops being an authentication library and suddenly also becomes an authorization one which could complicate things a little and increase attack surface.

Another option to make it easier is in the documentation instead of setting the value of the hook directly, instead just call the SvelteKitAuth function with providers, and THEN return that object, this way it's very obvious to the developer they can just modify and read that object and do whatever they want with it, such as handling authorization based on pathNames and such.

This could be made even simpler by using https://kit.svelte.dev/docs/modules#sveltejs-kit-hooks-sequence, the developer can create its own authorization middleware and separate that code from the auth.js stuff.

Agree . Having session object on locals then as getSession function.

@DoodlesEpic
Copy link
Contributor Author

DoodlesEpic commented Dec 27, 2022

I'm personally feeling good about it now. Haven't checked for grammatical or writing errors but I feel the explanation is much more complete now and correctly explains the different strategies to handle authorization in a SvelteKit application. The hooks based approach overall felt much cleaner so I provided a complete example anyone could use as a base for the handle.hooks.ts file.

The only thing that's also possible is to completely remove that part about using the layout file to set the session information in the $page store as that is very error prone, and instead instruct to do so through the handle hook. I'll wait for feedback to make any major changes. The way we are doing it now if someone just mindlessly copies it there will be no problem, but if they write this later by directly accessing the $page store they will have written that subtle bug talked about in that other thread.

@ThangHuuVu
Copy link
Member

Thanks for the PR! The only thing I'd add is the formatting of code variable components in the docs. For example:
+page.server.ts, LayoutServerLoad, etc... Could you help update those? 🙌

DoodlesEpic and others added 6 commits December 29, 2022 19:33
Added a code sample for managing the session through the $page store.
The sample demonstrates how to retrieve the session data in the root
+page.server.ts file and make it globally accessible through the $page
store, simplifying state management in the application. The previous
examples already used the data available in this store but did not show
how to set it.
This authorization section was added to make sure a few caveats with
SvelteKit were well documented to anyone using the library.

The problem is documented here: sveltejs/kit#6315

Essentially, propagation of data between leafs is not guaranteed when
using the +layout.server.ts file as its load function is not guaranteed
to rerun every page change. The current approach to solve this is to do
authorization in each +page.server.ts file and additionally make sure to
grab the session data by awaiting the parent instead of directly
accessing the $page store, to make sure the information there is
current.
PageLoad type should actually be PageServerLoad. Not setting this does
not actually generate any problems other than TypeScript complaining
that this type is not actually exported.
Another way to handle authorization is through a path-based method. This
added part of the documentation uses the handle hook to protect certain
routes based on their path. The previous method which is per-component
is still present.
Using event.locals.getSession() exposed by SvelteKitAuth instead of
relying in the root layout file making that available in the $page
store.
Finalize the explanation for the URI-based approach and also clarify
interactions with the component-based approach.
Format the variables like this: `var` so that it appears clearly as code
when reading the documentation.
@DoodlesEpic DoodlesEpic force-pushed the docs/sveltekit-session branch from 514be99 to 01df7da Compare December 29, 2022 22:33
@DoodlesEpic
Copy link
Contributor Author

DoodlesEpic commented Dec 29, 2022

I think I got them all @ThangHuuVu :D
I also rebased the branch with the main branch to keep it updated. So it's good to merge I guess.

Copy link
Member

@ThangHuuVu ThangHuuVu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🫶

@ThangHuuVu ThangHuuVu merged commit 7c96351 into nextauthjs:main Jan 4, 2023
@DoodlesEpic DoodlesEpic deleted the docs/sveltekit-session branch January 4, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SvelteKit Auth documentation incomplete
3 participants