Redirect with query param #2811
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR enhances user authentication flow by preserving context during logout/login processes, focusing on maintaining original URLs and query parameters across redirects.
- Added state parameter to
create_danswer_oauth_routerinbackend/danswer/auth/users.pyfor improved security and tracking - Modified
web/src/components/UserDropdown.tsxto redirect to login with encoded current URL as query parameter after logout - Updated
web/src/app/auth/oauth/callback/route.tsandweb/src/app/auth/oidc/callback/route.tsto handle manual redirections and preserve backend-provided redirect URLs - Implemented
nextUrlparameter inweb/src/lib/userSS.tsfor authentication URL generation functions to maintain context - Adjusted
web/src/app/search/page.tsxto handle query parameters during redirection for unauthenticated users
11 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
| # Copy headers and other attributes from 'response' to 'redirect_response' | ||
| for header_name, header_value in response.headers.items(): | ||
| redirect_response.headers[header_name] = header_value | ||
|
|
||
| if hasattr(response, "body"): | ||
| redirect_response.body = response.body | ||
| if hasattr(response, "status_code"): | ||
| redirect_response.status_code = response.status_code | ||
| if hasattr(response, "media_type"): | ||
| redirect_response.media_type = response.media_type |
There was a problem hiding this comment.
logic: Copying response attributes may expose sensitive information. Ensure only necessary data is transferred
| @@ -81,7 +82,6 @@ | |||
| router as token_rate_limit_settings_router, | |||
| ) | |||
| from danswer.setup import setup_danswer | |||
There was a problem hiding this comment.
logic: Removal of setup_multitenant_danswer might affect multi-tenant setups. Verify this change doesn't break existing functionality
| @@ -59,7 +63,7 @@ const Page = async ({ | |||
| let authUrl: string | null = null; | |||
| if (authTypeMetadata) { | |||
| try { | |||
| authUrl = await getAuthUrlSS(authTypeMetadata.authType); | |||
| authUrl = await getAuthUrlSS(authTypeMetadata.authType, nextUrl!); | |||
There was a problem hiding this comment.
style: Using non-null assertion (!). Consider adding a null check for nextUrl instead.
| alert("Failed to logout"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
style: Consider using a more user-friendly error message
| const searchParamsString = new URLSearchParams( | ||
| searchParams as unknown as Record<string, string> | ||
| ).toString(); |
There was a problem hiding this comment.
style: Consider using a more type-safe approach for converting searchParams
web/src/lib/userSS.ts
Outdated
| @@ -40,8 +44,12 @@ export const getAuthDisabledSS = async (): Promise<boolean> => { | |||
| return (await getAuthTypeMetadataSS()).authType === "disabled"; | |||
| }; | |||
|
|
|||
| const geOIDCAuthUrlSS = async (): Promise<string> => { | |||
| const res = await fetch(buildUrl("/auth/oidc/authorize")); | |||
| const geOIDCAuthUrlSS = async (nextUrl: string | null): Promise<string> => { | |||
There was a problem hiding this comment.
syntax: Typo in function name: 'geOIDCAuthUrlSS' should be 'getOIDCAuthUrlSS'
There was a problem hiding this comment.
This definitely seems wrong?
| return await getSAMLAuthUrlSS(); | ||
| } |
There was a problem hiding this comment.
logic: SAML auth URL generation doesn't use nextUrl. Consider if this is intentional or if it should be updated for consistency.
web/src/lib/userSS.ts
Outdated
| @@ -40,8 +44,12 @@ export const getAuthDisabledSS = async (): Promise<boolean> => { | |||
| return (await getAuthTypeMetadataSS()).authType === "disabled"; | |||
| }; | |||
|
|
|||
| const geOIDCAuthUrlSS = async (): Promise<string> => { | |||
| const res = await fetch(buildUrl("/auth/oidc/authorize")); | |||
| const geOIDCAuthUrlSS = async (nextUrl: string | null): Promise<string> => { | |||
There was a problem hiding this comment.
This definitely seems wrong?
Preserve user context during logout/login:
Screen.Recording.2024-10-15.at.3.17.04.PM.mov