From 61e7ad123e59ae2d1b42b5153eb3913fb5b85c52 Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Wed, 30 Oct 2024 05:37:18 -0500 Subject: [PATCH] fix(pkce): 20111 PKCE auth flow does not return user to previous path like dex auth flow (#20202) * Adding non-default basehref support for PKCE auth flow Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Adding ; for linting Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * removing hook function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Moving unauthorized error handling to class component to access context for error handling within 401 error Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Store the subsrition handle to close in unmount Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * reorder imports Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Actually saving the subscriptions now Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * returning the 401 subscription from helper function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Handle the promise of a subscription Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Removing then from non async subscribe Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Linter fixes Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Adding path caching to sessionStorage on pkceLogin and redirect step to cached path if available in pkceCallback to mirror Dex functionality Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --------- Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 79 ++++++++++++++------- ui/src/app/login/components/pkce-verify.tsx | 2 +- ui/src/app/login/components/utils.ts | 24 ++++++- 3 files changed, 77 insertions(+), 28 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 7a9bfb21635bb..1e8bd3b383c34 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -1,9 +1,10 @@ -import {DataLoader, NavigationManager, Notifications, NotificationsManager, PageContext, Popup, PopupManager, PopupProps} from 'argo-ui'; +import {DataLoader, NavigationManager, NotificationType, Notifications, NotificationsManager, PageContext, Popup, PopupManager, PopupProps} from 'argo-ui'; import {createBrowserHistory} from 'history'; import * as PropTypes from 'prop-types'; import * as React from 'react'; import {Helmet} from 'react-helmet'; import {Redirect, Route, RouteComponentProps, Router, Switch} from 'react-router'; +import {Subscription} from 'rxjs'; import applications from './applications'; import help from './help'; import login from './login'; @@ -19,6 +20,7 @@ import {Banner} from './ui-banner/ui-banner'; import userInfo from './user-info'; import {AuthSettings} from './shared/models'; import {PKCEVerification} from './login/components/pkce-verify'; +import {getPKCERedirectURI, pkceLogin} from './login/components/utils'; import {SystemLevelExtension} from './shared/services/extensions-service'; services.viewPreferences.init(); @@ -86,28 +88,6 @@ async function isExpiredSSO() { return false; } -requests.onError.subscribe(async err => { - if (err.status === 401) { - if (history.location.pathname.startsWith('/login')) { - return; - } - - const isSSO = await isExpiredSSO(); - // location might change after async method call, so we need to check again. - if (history.location.pathname.startsWith('/login')) { - return; - } - // Query for basehref and remove trailing /. - // If basehref is the default `/` it will become an empty string. - const basehref = document.querySelector('head > base').getAttribute('href').replace(/\/$/, ''); - if (isSSO) { - window.location.href = `${basehref}/auth/login?return_url=${encodeURIComponent(location.href)}`; - } else { - history.push(`/login?return_url=${encodeURIComponent(location.href)}`); - } - } -}); - export class App extends React.Component< {}, {popupProps: PopupProps; showVersionPanel: boolean; error: Error; navItems: NavItem[]; routes: Routes; extensionsLoaded: boolean; authSettings: AuthSettings} @@ -126,6 +106,8 @@ export class App extends React.Component< private navigationManager: NavigationManager; private navItems: NavItem[]; private routes: Routes; + private popupPropsSubscription: Subscription; + private unauthorizedSubscription: Subscription; constructor(props: {}) { super(props); @@ -135,11 +117,16 @@ export class App extends React.Component< this.navigationManager = new NavigationManager(history); this.navItems = navItems; this.routes = routes; + this.popupPropsSubscription = null; + this.unauthorizedSubscription = null; services.extensions.addEventListener('systemLevel', this.onAddSystemLevelExtension.bind(this)); } public async componentDidMount() { - this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})); + this.popupPropsSubscription = this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})); + this.subscribeUnauthorized().then(subscription => { + this.unauthorizedSubscription = subscription; + }); const authSettings = await services.authService.settings(); const {trackingID, anonymizeUsers} = authSettings.googleAnalytics || {trackingID: '', anonymizeUsers: true}; const {loggedIn, username} = await services.users.get(); @@ -167,6 +154,15 @@ export class App extends React.Component< this.setState({...this.state, navItems: this.navItems, routes: this.routes, extensionsLoaded: false, authSettings}); } + public componentWillUnmount() { + if (this.popupPropsSubscription) { + this.popupPropsSubscription.unsubscribe(); + } + if (this.unauthorizedSubscription) { + this.unauthorizedSubscription.unsubscribe(); + } + } + public render() { if (this.state.error != null) { const stack = this.state.error.stack; @@ -242,6 +238,41 @@ export class App extends React.Component< return {history, apis: {popup: this.popupManager, notifications: this.notificationsManager, navigation: this.navigationManager}}; } + private async subscribeUnauthorized() { + return requests.onError.subscribe(async err => { + if (err.status === 401) { + if (history.location.pathname.startsWith('/login')) { + return; + } + + const isSSO = await isExpiredSSO(); + // location might change after async method call, so we need to check again. + if (history.location.pathname.startsWith('/login')) { + return; + } + // Query for basehref and remove trailing /. + // If basehref is the default `/` it will become an empty string. + const basehref = document.querySelector('head > base').getAttribute('href').replace(/\/$/, ''); + if (isSSO) { + const authSettings = await services.authService.settings(); + + if (authSettings?.oidcConfig?.enablePKCEAuthentication) { + pkceLogin(authSettings.oidcConfig, getPKCERedirectURI().toString()).catch(err => { + this.getChildContext().apis.notifications.show({ + type: NotificationType.Error, + content: err?.message || JSON.stringify(err) + }); + }); + } else { + window.location.href = `${basehref}/auth/login?return_url=${encodeURIComponent(location.href)}`; + } + } else { + history.push(`/login?return_url=${encodeURIComponent(location.href)}`); + } + } + }); + } + private onAddSystemLevelExtension(extension: SystemLevelExtension) { const extendedNavItems = this.navItems; const extendedRoutes = this.routes; diff --git a/ui/src/app/login/components/pkce-verify.tsx b/ui/src/app/login/components/pkce-verify.tsx index f8207e8a5d81f..6e37d97e8d8df 100644 --- a/ui/src/app/login/components/pkce-verify.tsx +++ b/ui/src/app/login/components/pkce-verify.tsx @@ -31,7 +31,7 @@ export const PKCEVerification = (props: RouteComponentProps) => {

Error occurred:

{error?.message || JSON.stringify(error)}

- Try to Login again + Try to Login again
); diff --git a/ui/src/app/login/components/utils.ts b/ui/src/app/login/components/utils.ts index 6c715077cc9cc..555f146c83c93 100644 --- a/ui/src/app/login/components/utils.ts +++ b/ui/src/app/login/components/utils.ts @@ -13,6 +13,7 @@ import { validateAuthResponse } from 'oauth4webapi'; import {AuthSettings} from '../../shared/models'; +import requests from '../../shared/services/requests'; export const discoverAuthServer = (issuerURL: URL): Promise => discoveryRequest(issuerURL).then(res => processDiscoveryResponse(issuerURL, res)); @@ -25,7 +26,7 @@ export const PKCECodeVerifier = { export const getPKCERedirectURI = () => { const currentOrigin = new URL(window.location.origin); - currentOrigin.pathname = '/pkce/verify'; + currentOrigin.pathname = requests.toAbsURL('/pkce/verify'); return currentOrigin; }; @@ -70,6 +71,12 @@ const validateAndGetOIDCForPKCE = async (oidcConfig: AuthSettings['oidcConfig']) export const pkceLogin = async (oidcConfig: AuthSettings['oidcConfig'], redirectURI: string) => { const {authorizationServer} = await validateAndGetOIDCForPKCE(oidcConfig); + // This sets the return path for the user after the pkce auth flow. + // This is ignored if the return path would be the login page as it would just loop. + if (!location.pathname.startsWith(requests.toAbsURL('/login'))) { + sessionStorage.setItem('return_url', location.pathname + location.search); + } + if (!authorizationServer.authorization_endpoint) { throw new PKCELoginError('No Authorization Server endpoint found'); } @@ -145,7 +152,18 @@ export const pkceCallback = async (queryParams: string, oidcConfig: AuthSettings throw new PKCELoginError('No token in response'); } - document.cookie = `argocd.token=${result.id_token}; path=/`; + // This regex removes any leading or trailing '/' characters and the result is appended to a '/'. + // This is because when base href if not just '/' toAbsURL() will append a trailing '/'. + // Just removing a trailing '/' from the string would break when base href is not specified, defaulted to '/'. + // This pattern is used to handle both cases. + document.cookie = `argocd.token=${result.id_token}; path=/${requests.toAbsURL('').replace(/^\/|\/$/g, '')}`; - window.location.replace('/applications'); + const returnURL = sessionStorage.getItem('return_url'); + + if (returnURL) { + sessionStorage.removeItem('return_url'); + window.location.replace(returnURL); + } else { + window.location.replace(requests.toAbsURL('/applications')); + } };