Skip to content

Commit

Permalink
fix(pkce): 20111 PKCE auth flow does not return user to previous path…
Browse files Browse the repository at this point in the history 
… 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>
  • Loading branch information
@austin5219
austin5219 committed Nov 5, 2024
1 parent 347f221 commit 61e7ad1
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 28 deletions.
79 changes: 55 additions & 24 deletions ui/src/app/app.tsx
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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();
Expand Down Expand Up @@ -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}
Expand All @@ -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);
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion ui/src/app/login/components/pkce-verify.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,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>
);
Expand Down
24 changes: 21 additions & 3 deletions ui/src/app/login/components/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand All @@ -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;
};
Expand Down Expand Up @@ -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');
}
Expand Down Expand Up @@ -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'));
}
};

0 comments on commit 61e7ad1

Please sign in to comment.