From 765f05bee282fdfdae371893789d0f45b5e72271 Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Tue, 15 Oct 2024 22:52:17 -0500 Subject: [PATCH 01/11] Adding non-default basehref support for PKCE auth flow Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 19 ++++++++++++++++--- ui/src/app/login/components/pkce-verify.tsx | 3 ++- ui/src/app/login/components/utils.ts | 11 ++++++++--- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 7b73bc50ac70c..fbbabb0f7cb06 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -1,4 +1,4 @@ -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'; @@ -11,7 +11,7 @@ import settings from './settings'; import {Layout, ThemeWrapper} from './shared/components/layout/layout'; import {Page} from './shared/components/page/page'; import {VersionPanel} from './shared/components/version-info/version-info-panel'; -import {AuthSettingsCtx, Provider} from './shared/context'; +import {AuthSettingsCtx, Context, Provider} from './shared/context'; import {services} from './shared/services'; import requests from './shared/services/requests'; import {hashCode} from './shared/utils'; @@ -19,6 +19,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(); @@ -101,7 +102,19 @@ requests.onError.subscribe(async err => { // 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)}`; + const authSettings = await services.authService.settings(); + const ctx = React.useContext(Context); + + if (authSettings?.oidcConfig?.enablePKCEAuthentication) { + pkceLogin(authSettings.oidcConfig, getPKCERedirectURI().toString()).catch(err => { + ctx.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)}`); } diff --git a/ui/src/app/login/components/pkce-verify.tsx b/ui/src/app/login/components/pkce-verify.tsx index ee3f420fe07ea..9c069e7eda3b5 100644 --- a/ui/src/app/login/components/pkce-verify.tsx +++ b/ui/src/app/login/components/pkce-verify.tsx @@ -2,6 +2,7 @@ import React, {useEffect, useState} from 'react'; import {RouteComponentProps} from 'react-router'; import {services} from '../../shared/services'; import {PKCECodeVerifier, PKCEState, PKCELoginError, getPKCERedirectURI, pkceCallback} from './utils'; +import requests from '../../shared/services/requests'; import './pkce-verify.scss'; @@ -32,7 +33,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 94372797f4912..475173cf4bdcd 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)); @@ -31,7 +32,7 @@ export const PKCEState = { export const getPKCERedirectURI = () => { const currentOrigin = new URL(window.location.origin); - currentOrigin.pathname = '/pkce/verify'; + currentOrigin.pathname = requests.toAbsURL('/pkce/verify'); return currentOrigin; }; @@ -153,7 +154,11 @@ 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'); + window.location.replace(requests.toAbsURL('/applications')); }; From 46b66c0e00d23e68e3f30cc66c2f42c8eaf3120b Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Tue, 15 Oct 2024 23:02:37 -0500 Subject: [PATCH 02/11] Adding ; for linting Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index fbbabb0f7cb06..43ef43c28b9de 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -110,7 +110,7 @@ requests.onError.subscribe(async err => { ctx.notifications.show({ type: NotificationType.Error, content: err?.message || JSON.stringify(err) - }) + }); }); } else { window.location.href = `${basehref}/auth/login?return_url=${encodeURIComponent(location.href)}`; From 4fcb087a17d5bfb7040ada558f801ef1c44e7212 Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Wed, 16 Oct 2024 10:04:14 -0500 Subject: [PATCH 03/11] removing hook function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 43ef43c28b9de..53c5bb9b51458 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -103,11 +103,10 @@ requests.onError.subscribe(async err => { const basehref = document.querySelector('head > base').getAttribute('href').replace(/\/$/, ''); if (isSSO) { const authSettings = await services.authService.settings(); - const ctx = React.useContext(Context); if (authSettings?.oidcConfig?.enablePKCEAuthentication) { pkceLogin(authSettings.oidcConfig, getPKCERedirectURI().toString()).catch(err => { - ctx.notifications.show({ + Context.notifications.show({ type: NotificationType.Error, content: err?.message || JSON.stringify(err) }); From 391f5770638ff172e9a079658762f89a13968485 Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Wed, 16 Oct 2024 21:24:09 -0500 Subject: [PATCH 04/11] 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> --- ui/src/app/app.tsx | 80 ++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 34 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 53c5bb9b51458..ed20f18bc1de8 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -11,7 +11,7 @@ import settings from './settings'; import {Layout, ThemeWrapper} from './shared/components/layout/layout'; import {Page} from './shared/components/page/page'; import {VersionPanel} from './shared/components/version-info/version-info-panel'; -import {AuthSettingsCtx, Context, Provider} from './shared/context'; +import {AuthSettingsCtx, Provider} from './shared/context'; import {services} from './shared/services'; import requests from './shared/services/requests'; import {hashCode} from './shared/utils'; @@ -87,39 +87,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) { - const authSettings = await services.authService.settings(); - - if (authSettings?.oidcConfig?.enablePKCEAuthentication) { - pkceLogin(authSettings.oidcConfig, getPKCERedirectURI().toString()).catch(err => { - Context.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)}`); - } - } -}); - export class App extends React.Component< {}, {popupProps: PopupProps; showVersionPanel: boolean; error: Error; navItems: NavItem[]; routes: Routes; extensionsLoaded: boolean; authSettings: AuthSettings} @@ -152,6 +119,7 @@ export class App extends React.Component< public async componentDidMount() { this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})); + this.subscribeUnauthorized(); const authSettings = await services.authService.settings(); const {trackingID, anonymizeUsers} = authSettings.googleAnalytics || {trackingID: '', anonymizeUsers: true}; const {loggedIn, username} = await services.users.get(); @@ -179,6 +147,11 @@ export class App extends React.Component< this.setState({...this.state, navItems: this.navItems, routes: this.routes, extensionsLoaded: false, authSettings}); } + public componentWillUnmount() { + this.popupManager.popupProps.unsubscribe(); + this.unsubscribeUnauthorized(); + } + public render() { if (this.state.error != null) { const stack = this.state.error.stack; @@ -254,6 +227,45 @@ export class App extends React.Component< return {history, apis: {popup: this.popupManager, notifications: this.notificationsManager, navigation: this.navigationManager}}; } + private async subscribeUnauthorized() { + 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 unsubscribeUnauthorized() { + requests.onError.unsubscribe(); + } + private onAddSystemLevelExtension(extension: SystemLevelExtension) { const extendedNavItems = this.navItems; const extendedRoutes = this.routes; From 752ce6e9b07b6601aa72fd88bcbdfac3da85f21e Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Thu, 17 Oct 2024 07:56:11 -0500 Subject: [PATCH 05/11] Store the subsrition handle to close in unmount Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index ed20f18bc1de8..71c4257a548d3 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -21,6 +21,7 @@ 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'; +import {Subscription} from 'rxjs'; services.viewPreferences.init(); const bases = document.getElementsByTagName('base'); @@ -105,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); @@ -148,8 +151,8 @@ export class App extends React.Component< } public componentWillUnmount() { - this.popupManager.popupProps.unsubscribe(); - this.unsubscribeUnauthorized(); + this.popupPropsSubscription.unsubscribe(); + this.unauthorizedSubscription.unsubscribe(); } public render() { @@ -262,10 +265,6 @@ export class App extends React.Component< }); } - private unsubscribeUnauthorized() { - requests.onError.unsubscribe(); - } - private onAddSystemLevelExtension(extension: SystemLevelExtension) { const extendedNavItems = this.navItems; const extendedRoutes = this.routes; From b62ab8fc28df8d68798ee98a08c8846fc8020e7d Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Thu, 17 Oct 2024 07:57:11 -0500 Subject: [PATCH 06/11] reorder imports Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 71c4257a548d3..1e7aadd9c3adf 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -4,6 +4,7 @@ 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'; @@ -21,7 +22,6 @@ 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'; -import {Subscription} from 'rxjs'; services.viewPreferences.init(); const bases = document.getElementsByTagName('base'); From 4673121a78e6330c3941f42efcd81b5f44d59dbf Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Thu, 17 Oct 2024 07:59:30 -0500 Subject: [PATCH 07/11] Actually saving the subscriptions now Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 1e7aadd9c3adf..00b8716ac5748 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -121,8 +121,8 @@ export class App extends React.Component< } public async componentDidMount() { - this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})); - this.subscribeUnauthorized(); + this.popupPropsSubscription = this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})); + this.unauthorizedSubscription = this.subscribeUnauthorized(); const authSettings = await services.authService.settings(); const {trackingID, anonymizeUsers} = authSettings.googleAnalytics || {trackingID: '', anonymizeUsers: true}; const {loggedIn, username} = await services.users.get(); From 905fb28261e3b75e83b9b20d10c30ef8be16b43c Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Thu, 17 Oct 2024 08:01:49 -0500 Subject: [PATCH 08/11] returning the 401 subscription from helper function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 00b8716ac5748..88017d69c8219 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -231,7 +231,7 @@ export class App extends React.Component< } private async subscribeUnauthorized() { - requests.onError.subscribe(async err => { + return requests.onError.subscribe(async err => { if (err.status === 401) { if (history.location.pathname.startsWith('/login')) { return; From 4070e08fa903c8062f5674e5da4c328c0f95b383 Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Thu, 17 Oct 2024 09:11:30 -0500 Subject: [PATCH 09/11] Handle the promise of a subscription Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 88017d69c8219..14fd16b07d540 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -117,12 +117,18 @@ 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.popupPropsSubscription = this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})); - this.unauthorizedSubscription = this.subscribeUnauthorized(); + this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})).then((subscription) => { + this.popupPropsSubscription = subscription; + }); + 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(); @@ -151,8 +157,12 @@ export class App extends React.Component< } public componentWillUnmount() { - this.popupPropsSubscription.unsubscribe(); - this.unauthorizedSubscription.unsubscribe(); + if (this.popupPropsSubscription) { + this.popupPropsSubscription.unsubscribe(); + } + if (this.unauthorizedSubscription) { + this.unauthorizedSubscription.unsubscribe(); + } } public render() { From ad31a3acb912e0901cfd711a5d75209aba803380 Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Thu, 17 Oct 2024 09:19:22 -0500 Subject: [PATCH 10/11] Removing then from non async subscribe Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index 14fd16b07d540..ad676594d413a 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -123,9 +123,7 @@ export class App extends React.Component< } public async componentDidMount() { - this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})).then((subscription) => { - this.popupPropsSubscription = subscription; - }); + this.popupPropsSubscription = this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})); this.subscribeUnauthorized().then((subscription) => { this.unauthorizedSubscription = subscription; }); From d2a6e1f528325f3145fbee517daa63ffabd24591 Mon Sep 17 00:00:00 2001 From: austin5219 <3936059+austin5219@users.noreply.github.com> Date: Thu, 17 Oct 2024 09:27:22 -0500 Subject: [PATCH 11/11] Linter fixes Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- ui/src/app/app.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/src/app/app.tsx b/ui/src/app/app.tsx index ad676594d413a..2d2e192590fa2 100644 --- a/ui/src/app/app.tsx +++ b/ui/src/app/app.tsx @@ -124,7 +124,7 @@ export class App extends React.Component< public async componentDidMount() { this.popupPropsSubscription = this.popupManager.popupProps.subscribe(popupProps => this.setState({popupProps})); - this.subscribeUnauthorized().then((subscription) => { + this.subscribeUnauthorized().then(subscription => { this.unauthorizedSubscription = subscription; }); const authSettings = await services.authService.settings(); @@ -244,7 +244,7 @@ export class App extends React.Component< 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')) { @@ -255,7 +255,7 @@ export class App extends React.Component< 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({