Skip to content
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

feat: SupersetClient config to override 401 behavior #19144

Merged
merged 11 commits into from
Mar 21, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ import {
} from './types';
import { DEFAULT_FETCH_RETRY_OPTIONS, DEFAULT_BASE_URL } from './constants';

const defaultUnauthorizedHandler = () => {
window.location.href = `/login?next=${
window.location.pathname + window.location.search
}`;
};

export default class SupersetClientClass {
credentials: Credentials;

Expand All @@ -58,6 +64,8 @@ export default class SupersetClientClass {

timeout: ClientTimeout;

handleUnauthorized: () => void;

constructor({
baseUrl = DEFAULT_BASE_URL,
host,
Expand All @@ -70,6 +78,7 @@ export default class SupersetClientClass {
csrfToken = undefined,
guestToken = undefined,
guestTokenHeaderName = 'X-GuestToken',
unauthorizedHandler = defaultUnauthorizedHandler,
}: ClientConfig = {}) {
const url = new URL(
host || protocol
Expand Down Expand Up @@ -100,6 +109,7 @@ export default class SupersetClientClass {
if (guestToken) {
this.headers[guestTokenHeaderName] = guestToken;
}
this.handleUnauthorized = unauthorizedHandler;
}

async init(force = false): CsrfPromise {
Expand Down Expand Up @@ -151,6 +161,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
ignoreUnauthorized,
...rest
}: RequestConfig & { parseMethod?: T }) {
await this.ensureAuth();
Expand All @@ -163,8 +174,8 @@ export default class SupersetClientClass {
timeout: timeout ?? this.timeout,
fetchRetryOptions: fetchRetryOptions ?? this.fetchRetryOptions,
}).catch(res => {
if (res?.status === 401) {
this.redirectUnauthorized();
if (res?.status === 401 && !ignoreUnauthorized) {
this.handleUnauthorized();
}
return Promise.reject(res);
});
Expand Down Expand Up @@ -230,10 +241,4 @@ export default class SupersetClientClass {
endpoint[0] === '/' ? endpoint.slice(1) : endpoint
}`;
}

redirectUnauthorized() {
window.location.href = `/login?next=${
window.location.pathname + window.location.search
}`;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export interface RequestBase {
fetchRetryOptions?: FetchRetryOptions;
headers?: Headers;
host?: Host;
ignoreUnauthorized?: boolean;
mode?: Mode;
method?: Method;
jsonPayload?: Payload;
Expand Down Expand Up @@ -136,6 +137,7 @@ export interface ClientConfig {
headers?: Headers;
mode?: Mode;
timeout?: ClientTimeout;
unauthorizedHandler?: () => void;
}

export interface SupersetClientInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -499,42 +499,92 @@ describe('SupersetClientClass', () => {
});
});

it('should redirect Unauthorized', async () => {
describe('when unauthorized', () => {
let originalLocation: any;
let authSpy: jest.SpyInstance;
const mockRequestUrl = 'https://host/get/url';
const mockRequestPath = '/get/url';
const mockRequestSearch = '?param=1&param=2';
const { location } = window;
// @ts-ignore
delete window.location;
// @ts-ignore
window.location = {
pathname: mockRequestPath,
search: mockRequestSearch,
};
const authSpy = jest
.spyOn(SupersetClientClass.prototype, 'ensureAuth')
.mockImplementation();
const rejectValue = { status: 401 };
fetchMock.get(mockRequestUrl, () => Promise.reject(rejectValue), {
overwriteRoutes: true,
const mockHref = `http://localhost${mockRequestPath + mockRequestSearch}`;

beforeEach(() => {
originalLocation = window.location;
// @ts-ignore
delete window.location;
// @ts-ignore
window.location = {
pathname: mockRequestPath,
search: mockRequestSearch,
href: mockHref,
};
authSpy = jest
.spyOn(SupersetClientClass.prototype, 'ensureAuth')
.mockImplementation();
const rejectValue = { status: 401 };
fetchMock.get(mockRequestUrl, () => Promise.reject(rejectValue), {
overwriteRoutes: true,
});
});

const client = new SupersetClientClass({});

let error;
try {
await client.request({ url: mockRequestUrl, method: 'GET' });
} catch (err) {
error = err;
} finally {
const redirectURL = window.location.href;
expect(redirectURL).toBe(
`/login?next=${mockRequestPath + mockRequestSearch}`,
);
expect(error.status).toBe(401);
}
afterEach(() => {
authSpy.mockReset();
window.location = originalLocation;
});

it('should redirect', async () => {
const client = new SupersetClientClass({});

authSpy.mockReset();
window.location = location;
let error;
try {
await client.request({ url: mockRequestUrl, method: 'GET' });
} catch (err) {
error = err;
} finally {
const redirectURL = window.location.href;
expect(redirectURL).toBe(
`/login?next=${mockRequestPath + mockRequestSearch}`,
);
expect(error.status).toBe(401);
}
});

it('does nothing if instructed to ignoreUnauthorized', async () => {
const client = new SupersetClientClass({});

let error;
try {
await client.request({
url: mockRequestUrl,
method: 'GET',
ignoreUnauthorized: true,
});
} catch (err) {
error = err;
} finally {
// unchanged href, no redirect
expect(window.location.href).toBe(mockHref);
expect(error.status).toBe(401);
}
});

it('accepts an unauthorizedHandler to override redirect behavior', async () => {
const unauthorizedHandler = jest.fn();
const client = new SupersetClientClass({ unauthorizedHandler });

let error;
try {
await client.request({
url: mockRequestUrl,
method: 'GET',
});
} catch (err) {
error = err;
} finally {
// unchanged href, no redirect
expect(window.location.href).toBe(mockHref);
expect(error.status).toBe(401);
expect(unauthorizedHandler).toHaveBeenCalledTimes(1);
}
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ import ToastPresenter from './ToastPresenter';

import { removeToast } from './actions';

export default connect(
({ messageToasts: toasts }) => ({ toasts }),
const ToastContainer = connect(
({ messageToasts: toasts }: any) => ({ toasts }),
dispatch => bindActionCreators({ removeToast }, dispatch),
)(ToastPresenter);

export default ToastContainer;
29 changes: 18 additions & 11 deletions superset-frontend/src/components/MessageToasts/ToastPresenter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@ import { styled } from '@superset-ui/core';
import { ToastMeta } from 'src/components/MessageToasts/types';
import Toast from './Toast';

const StyledToastPresenter = styled.div`
export interface VisualProps {
position: 'bottom' | 'top';
}

const StyledToastPresenter = styled.div<VisualProps>`
max-width: 600px;
position: fixed;
bottom: 0px;
${({ position }) => (position === 'bottom' ? 'bottom' : 'top')}: 0px;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default toasts are at the bottom, but the bottom of an embedded iframe could be offscreen, so for embedded it looks better to have toasts at the top.

right: 0px;
margin-right: 50px;
margin-bottom: 50px;
Expand Down Expand Up @@ -69,22 +73,25 @@ const StyledToastPresenter = styled.div`
}
`;

type ToastPresenterProps = {
type ToastPresenterProps = Partial<VisualProps> & {
toasts: Array<ToastMeta>;
removeToast: () => void;
removeToast: () => any;
};

export default function ToastPresenter({
toasts,
removeToast,
position = 'top',
}: ToastPresenterProps) {
return (
toasts.length > 0 && (
<StyledToastPresenter id="toast-presenter">
{toasts.map(toast => (
<Toast key={toast.id} toast={toast} onCloseToast={removeToast} />
))}
</StyledToastPresenter>
)
<>
{toasts.length > 0 && (
<StyledToastPresenter id="toast-presenter" position={position}>
{toasts.map(toast => (
<Toast key={toast.id} toast={toast} onCloseToast={removeToast} />
))}
</StyledToastPresenter>
)}
</>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ export interface ToastProps {
addWarningToast: typeof addWarningToast;
}

const toasters = {
/** just action creators, these do not dispatch */
export const toasters = {
addInfoToast,
addSuccessToast,
addWarningToast,
Expand Down
45 changes: 35 additions & 10 deletions superset-frontend/src/embedded/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,16 @@
import React, { lazy, Suspense } from 'react';
import ReactDOM from 'react-dom';
import { BrowserRouter as Router, Route } from 'react-router-dom';
import { t } from '@superset-ui/core';
import { Switchboard } from '@superset-ui/switchboard';
import { bootstrapData } from 'src/preamble';
import setupClient from 'src/setup/setupClient';
import { RootContextProviders } from 'src/views/RootContextProviders';
import { store } from 'src/views/store';
import ErrorBoundary from 'src/components/ErrorBoundary';
import Loading from 'src/components/Loading';
import { addDangerToast } from 'src/components/MessageToasts/actions';
import ToastContainer from 'src/components/MessageToasts/ToastContainer';

const debugMode = process.env.WEBPACK_MODE === 'development';

Expand All @@ -49,6 +53,7 @@ const EmbeddedApp = () => (
<ErrorBoundary>
<LazyDashboardPage />
</ErrorBoundary>
<ToastContainer position="top" />
</RootContextProviders>
</Suspense>
</Route>
Expand All @@ -75,23 +80,43 @@ if (!window.parent) {
// );
// }

let displayedUnauthorizedToast = false;

/**
* If there is a problem with the guest token, we will start getting
* 401 errors from the api and SupersetClient will call this function.
*/
function guestUnauthorizedHandler() {
if (displayedUnauthorizedToast) return; // no need to display this message every time we get another 401
displayedUnauthorizedToast = true;
// If a guest user were sent to a login screen on 401, they would have no valid login to use.
// For embedded it makes more sense to just display a message
// and let them continue accessing the page, to whatever extent they can.
store.dispatch(
addDangerToast(
t(
'This session has encountered an interruption, and some controls may not work as intended. If you are the developer of this app, please check that the guest token is being generated correctly.',
),
{
duration: -1, // stay open until manually closed
noDuplicate: true,
},
),
);
}

/**
* Configures SupersetClient with the correct settings for the embedded dashboard page.
*/
function setupGuestClient(guestToken: string) {
// need to reconfigure SupersetClient to use the guest token
setupClient({
guestToken,
guestTokenHeaderName: bootstrapData.config?.GUEST_TOKEN_HEADER_NAME,
unauthorizedHandler: guestUnauthorizedHandler,
});
}

function validateMessageEvent(event: MessageEvent) {
if (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stuff isn't required, and I think it will be more confusing long-term than not having it here.

event.data?.type === 'webpackClose' ||
event.data?.source === '@devtools-page'
) {
// sometimes devtools use the messaging api and we want to ignore those
throw new Error("Sir, this is a Wendy's");
}

// if (!ALLOW_ORIGINS.includes(event.origin)) {
// throw new Error('Message origin is not in the allowed list');
// }
Expand All @@ -105,7 +130,7 @@ window.addEventListener('message', function embeddedPageInitializer(event) {
try {
validateMessageEvent(event);
} catch (err) {
log('ignoring message', err, event);
log('ignoring message unrelated to embedded comms', err, event);
return;
}

Expand Down