More idiomatic supabase auth #139
Conversation
…es/auth/server-side/sveltekit Differences: - only run the auth layout under /account and /login, want fast unauthed marketing pages with no JS - Be even more explicit in maing getSession safe by not calling it on the server at all
Deploying saasstarter with
|
| Latest commit: |
3d0cf79
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d2a2a919.saasstarter.pages.dev |
| Branch Preview URL: | https://ssr-redux.saasstarter.pages.dev |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve modifications to session management and user authentication across multiple files in the application. Key alterations include the addition of a session property in global declarations, renaming and redefining middleware functions for improved request handling, and streamlining session retrieval processes in various routes. These updates enhance the application's structure and control flow regarding user sessions and access management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Middleware
participant Supabase
participant App
User->>Middleware: Request
Middleware->>Supabase: Initialize Client
Middleware->>Middleware: AuthGuard Check
Middleware->>App: Proceed with Request
App->>User: Response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
src/hooks.server.ts (1)
Line range hint
13-87: Approve the changes to thesupabasefunction with a suggestion for improvement.The initialization of two Supabase clients—one for general use and one with a service role—is a good practice that separates concerns effectively. The custom cookie handling setup is also well-thought-out, ensuring compatibility with SvelteKit's API.
However, consider documenting the use of
suppressGetSessionWarningmore thoroughly, as it is not part of the official API and could lead to future maintenance issues.Add documentation for the use of
suppressGetSessionWarning:+ // Note: suppressGetSessionWarning is used here to suppress a specific warning + // related to getSession. This is not part of the official Supabase API and should + // be used with caution as it may be subject to change. if ("suppressGetSessionWarning" in event.locals.supabase.auth) { // @ts-expect-error - suppressGetSessionWarning is not part of the official API event.locals.supabase.auth.suppressGetSessionWarning = true } else { console.warn( "SupabaseAuthClient#suppressGetSessionWarning was removed. See https://github.com/supabase/auth-js/issues/888.", ) }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/app.d.ts (1 hunks)
- src/hooks.server.ts (2 hunks)
- src/routes/(admin)/account/+layout.server.ts (1 hunks)
- src/routes/(admin)/account/+layout.ts (2 hunks)
- src/routes/(marketing)/login/+layout.server.ts (1 hunks)
Additional comments not posted (5)
src/routes/(admin)/account/+layout.server.ts (1)
4-10: Simplification of session handling noted. Verify upstream validation.The changes in the
loadfunction simplify the session handling by directly accessing thesessionobject and returning it with cookies. This assumes that session validation is handled upstream, possibly by middleware.Ensure that the session validation logic is robust and securely implemented before this point in the code flow to maintain security and functionality.
Verification successful
Session validation is handled upstream.
The
safeGetSessionfunction insrc/hooks.server.tsvalidates the session before it is used in theloadfunction. This confirms that the simplification in session handling is appropriate and secure.
src/hooks.server.ts:safeGetSessionvalidates the JWT before returning the session.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that session validation is handled upstream. # Test: Search for middleware or hooks that validate the session before this function is called. rg --type typescript -A 5 $'session validation'Length of output: 83
Script:
#!/bin/bash # Description: Verify that session validation is handled upstream. # Test: Search for middleware or hooks that validate the session before this function is called. rg --type ts -A 5 $'session validation'Length of output: 40
Script:
#!/bin/bash # Description: Search for session-related middleware or hooks that might handle validation. # Test: Look for common session handling patterns or functions in TypeScript files. rg --type ts -A 5 'session' | rg 'middleware|hook'Length of output: 2320
src/routes/(marketing)/login/+layout.server.ts (1)
10-10: Optimization and simplification of session handling approved.The repositioning of the session retrieval to just before the redirect logic optimizes the function by avoiding unnecessary session fetches. The direct return of the session variable simplifies the code and enhances clarity.
Verify that this change does not impact other functionalities that depend on the session data being manipulated or checked in a specific way.
src/app.d.ts (1)
16-16: Addition of session property approved.The addition of the
sessionproperty to the global declarations enhances session management and aligns with the PR's objectives to improve authentication handling.src/routes/(admin)/account/+layout.ts (1)
63-67: Approve user profile retrieval changes.Fetching the user profile directly from the database using the user's ID is a robust approach that ensures data integrity and freshness. This change is well-implemented and improves the accuracy of the profile data retrieved.
src/hooks.server.ts (1)
88-102: Approve the changes to theauthGuardfunction.The implementation of the
authGuardfunction is crucial for maintaining secure access to specific routes based on user authentication status. The logic for redirecting users based on their authentication status is well-implemented and enhances the application's security.
| // on server populated on server by LayoutData, using authGuard hook | ||
| let session = data.session | ||
| if (isBrowser()) { | ||
| // Only call getSession in browser where it's safe. | ||
| const getSessionResponse = await supabase.auth.getSession() | ||
| session = getSessionResponse.data.session | ||
| } | ||
| if (!session) { | ||
| redirect(303, "/login") | ||
| } |
There was a problem hiding this comment.
Approve session management changes with a suggestion for improvement.
The conditional session management logic is a significant improvement, enhancing security by ensuring that session retrieval only occurs in the browser context. This approach aligns well with best practices.
However, consider adding error handling for the getSession call to manage potential failures gracefully.
Add error handling around the getSession call:
- const getSessionResponse = await supabase.auth.getSession()
+ let getSessionResponse;
+ try {
+ getSessionResponse = await supabase.auth.getSession()
+ } catch (error) {
+ console.error('Failed to retrieve session:', error);
+ redirect(303, '/login');
+ return;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // on server populated on server by LayoutData, using authGuard hook | |
| let session = data.session | |
| if (isBrowser()) { | |
| // Only call getSession in browser where it's safe. | |
| const getSessionResponse = await supabase.auth.getSession() | |
| session = getSessionResponse.data.session | |
| } | |
| if (!session) { | |
| redirect(303, "/login") | |
| } | |
| // on server populated on server by LayoutData, using authGuard hook | |
| let session = data.session | |
| if (isBrowser()) { | |
| // Only call getSession in browser where it's safe. | |
| let getSessionResponse; | |
| try { | |
| getSessionResponse = await supabase.auth.getSession() | |
| } catch (error) { | |
| console.error('Failed to retrieve session:', error); | |
| redirect(303, '/login'); | |
| return; | |
| } | |
| session = getSessionResponse.data.session | |
| } | |
| if (!session) { | |
| redirect(303, "/login") | |
| } |
Follow official guide: https://supabase.com/docs/guides/auth/server-side/sveltekit
Differences from guide:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor