-
Notifications
You must be signed in to change notification settings - Fork 388
fix: [UIE-9661] - oAuth callback improvements #13105
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
base: develop
Are you sure you want to change the base?
Changes from all commits
34ff460
04da29c
ea0ed7b
001ed10
17198ad
f12c95d
39c72af
3e275c5
c116337
42205c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,14 @@ | ||
| import * as Sentry from '@sentry/react'; | ||
| import { useNavigate } from '@tanstack/react-router'; | ||
| import { useSearch } from '@tanstack/react-router'; | ||
| import React from 'react'; | ||
|
|
||
| import { SplashScreen } from 'src/components/SplashScreen'; | ||
|
|
||
| import { clearStorageAndRedirectToLogout, handleOAuthCallback } from './oauth'; | ||
|
|
||
| import type { LinkProps } from '@tanstack/react-router'; | ||
|
|
||
| /** | ||
| * Login will redirect back to Cloud Manager with a URL like: | ||
| * https://cloud.linode.com/oauth/callback?returnTo=%2F&state=066a6ad9-b19a-43bb-b99a-ef0b5d4fc58d&code=42ddf75dfa2cacbad897 | ||
|
|
@@ -14,24 +17,57 @@ import { clearStorageAndRedirectToLogout, handleOAuthCallback } from './oauth'; | |
| */ | ||
| export const OAuthCallback = () => { | ||
| const navigate = useNavigate(); | ||
| const authenticate = async () => { | ||
| try { | ||
| const { returnTo } = await handleOAuthCallback({ | ||
| params: location.search, | ||
| }); | ||
|
|
||
| navigate({ to: returnTo }); | ||
| } catch (error) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(error); | ||
| Sentry.captureException(error); | ||
| clearStorageAndRedirectToLogout(); | ||
| } | ||
| }; | ||
| const search = useSearch({ | ||
| from: '/oauth/callback', | ||
| }); | ||
|
|
||
| const hasStartedAuth = React.useRef(false); | ||
| const isAuthenticating = React.useRef(false); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using references to avoid triggering the callback more than needed and end up with stale values |
||
|
|
||
| React.useEffect(() => { | ||
| // Prevent running if already started or currently running | ||
| if (hasStartedAuth.current || isAuthenticating.current) { | ||
| return; | ||
| } | ||
|
|
||
| hasStartedAuth.current = true; | ||
| isAuthenticating.current = true; | ||
|
|
||
| const authenticate = async () => { | ||
| try { | ||
| const { returnTo } = await handleOAuthCallback({ | ||
| params: search, | ||
| }); | ||
|
|
||
| // None of these paths are valid return destinations | ||
| const invalidReturnToPaths: LinkProps['to'][] = [ | ||
| '/logout', | ||
| '/admin/callback', | ||
| '/oauth/callback', | ||
| '/cancel', | ||
| ]; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the fix to not redirect to one of these routes in order to never be stuck in a logout loop |
||
|
|
||
| const isInvalidReturnTo = | ||
| !returnTo || invalidReturnToPaths.some((path) => returnTo === path); | ||
|
|
||
| if (isInvalidReturnTo) { | ||
| navigate({ to: '/' }); | ||
| return; | ||
| } | ||
|
|
||
| navigate({ to: returnTo }); | ||
| } catch (error) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(error); | ||
| Sentry.captureException(error); | ||
| clearStorageAndRedirectToLogout(); | ||
| } finally { | ||
| isAuthenticating.current = false; | ||
| } | ||
| }; | ||
|
|
||
| authenticate(); | ||
| }, []); | ||
| }, [navigate, search]); | ||
|
|
||
| return <SplashScreen />; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,10 +187,13 @@ export async function redirectToLogin() { | |
| * @returns Some information about the new session because authentication was successfull | ||
| */ | ||
| export async function handleOAuthCallback(options: AuthCallbackOptions) { | ||
| const paramsObject = | ||
| typeof options.params === 'string' | ||
| ? getQueryParamsFromQueryString(options.params) | ||
| : options.params; | ||
|
|
||
| const { data: params, error: parseParamsError } = await tryCatch( | ||
| OAuthCallbackParamsSchema.validate( | ||
| getQueryParamsFromQueryString(options.params) | ||
| ) | ||
| OAuthCallbackParamsSchema.validate(paramsObject) | ||
| ); | ||
|
|
||
| if (parseParamsError) { | ||
|
|
@@ -296,7 +299,9 @@ export async function handleLoginAsCustomerCallback( | |
| ) { | ||
| const { data: params, error } = await tryCatch( | ||
| LoginAsCustomerCallbackParamsSchema.validate( | ||
| getQueryParamsFromQueryString(options.params) | ||
| typeof options.params === 'string' | ||
| ? getQueryParamsFromQueryString(options.params) | ||
| : options.params | ||
| ) | ||
| ); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just added some typeguard here to satisfy the new callback strict route types |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,12 +50,12 @@ export const checkIAMEnabled = async ( | |
| flags: FlagSet, | ||
| profile: Profile | undefined | ||
| ): Promise<boolean> => { | ||
| if (!flags?.iam?.enabled) { | ||
| if (!flags?.iam?.enabled || !profile) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not directly related to this PR. just a follow up to #13037 |
||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| if (profile?.username) { | ||
| if (profile.username) { | ||
| // For restricted users ONLY, get permissions | ||
| const permissions = await queryClient.ensureQueryData( | ||
| queryOptions(iamQueries.user(profile.username)._ctx.accountPermissions) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one potential issue:
here we were using
windows.locationwhich means it would get evaluated (and return a value) before router initiation.