fix(pkce): 20111 PKCE auth flow does not return user to previous path…

… like dex auth flow (argoproj#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>
AmadeusITGroup · Oct 30, 2024 · 092bb73 · 092bb73
1 parent d408909
commit 092bb73
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 28 deletions.
diff --git 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
@@ -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<any>) => {
                 <div>
                     <h3>Error occurred: </h3>
                     <p>{error?.message || JSON.stringify(error)}</p>
-                    <a href='/login'>Try to Login again</a>
+                    <a href={requests.toAbsURL('/login')}>Try to Login again</a>
                 </div>
             </div>
         );

diff --git 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<AuthorizationServer> => 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;
 };
@@ -76,6 +77,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');
     }
@@ -153,7 +160,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'));
+    }
 };