From 9855f7d4a90bfe0e36f8ce4b20ac889949cb3bd5 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Mon, 14 Mar 2022 13:33:45 -0700 Subject: [PATCH 01/10] wip --- .../src/connection/SupersetClientClass.ts | 19 +-- .../superset-ui-core/src/connection/types.ts | 2 + .../connection/SupersetClientClass.test.ts | 112 +++++++++++++----- 3 files changed, 95 insertions(+), 38 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts index fa75148afe624..8ba39a66c0bd9 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts @@ -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; @@ -58,6 +64,8 @@ export default class SupersetClientClass { timeout: ClientTimeout; + handleUnauthorized: () => void; + constructor({ baseUrl = DEFAULT_BASE_URL, host, @@ -70,6 +78,7 @@ export default class SupersetClientClass { csrfToken = undefined, guestToken = undefined, guestTokenHeaderName = 'X-GuestToken', + unauthorizedHandler = defaultUnauthorizedHandler, }: ClientConfig = {}) { const url = new URL( host || protocol @@ -100,6 +109,7 @@ export default class SupersetClientClass { if (guestToken) { this.headers[guestTokenHeaderName] = guestToken; } + this.handleUnauthorized = unauthorizedHandler; } async init(force = false): CsrfPromise { @@ -151,6 +161,7 @@ export default class SupersetClientClass { headers, timeout, fetchRetryOptions, + ignoreUnauthorized, ...rest }: RequestConfig & { parseMethod?: T }) { await this.ensureAuth(); @@ -164,7 +175,7 @@ export default class SupersetClientClass { fetchRetryOptions: fetchRetryOptions ?? this.fetchRetryOptions, }).catch(res => { if (res?.status === 401) { - this.redirectUnauthorized(); + this.handleUnauthorized(); } return Promise.reject(res); }); @@ -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 - }`; - } } diff --git a/superset-frontend/packages/superset-ui-core/src/connection/types.ts b/superset-frontend/packages/superset-ui-core/src/connection/types.ts index 0ea6a957a8739..004fb59e8c314 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/types.ts @@ -89,6 +89,7 @@ export interface RequestBase { signal?: Signal; stringify?: Stringify; timeout?: ClientTimeout; + ignoreUnauthorized?: boolean; } export interface CallApi extends RequestBase { @@ -136,6 +137,7 @@ export interface ClientConfig { headers?: Headers; mode?: Mode; timeout?: ClientTimeout; + unauthorizedHandler?: () => void; } export interface SupersetClientInterface diff --git a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts index ae6ac138d5a67..e4738a3da0027 100644 --- a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts @@ -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¶m=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, + + beforeEach(() => { + originalLocation = window.location; + // @ts-ignore + delete window.location; + // @ts-ignore + window.location = { + pathname: mockRequestPath, + search: mockRequestSearch, + href: mockRequestPath + mockRequestSearch, + }; + 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(mockRequestPath + mockRequestSearch); + 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', + ignoreUnauthorized: true, + }); + } catch (err) { + error = err; + } finally { + // unchanged href, no redirect + expect(window.location.href).toBe(mockRequestPath + mockRequestSearch); + expect(error.status).toBe(401); + expect(unauthorizedHandler).toHaveBeenCalledTimes(1); + } + }); }); }); From 3a25ee72668cfcd38ebe213d725d4f93dfb8dbbb Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Mon, 14 Mar 2022 16:21:12 -0700 Subject: [PATCH 02/10] feat: make 401 responses configurable in SupersetClient --- .../src/connection/SupersetClientClass.ts | 2 +- .../test/connection/SupersetClientClass.test.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts index 8ba39a66c0bd9..bf0c02a81d789 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts @@ -174,7 +174,7 @@ export default class SupersetClientClass { timeout: timeout ?? this.timeout, fetchRetryOptions: fetchRetryOptions ?? this.fetchRetryOptions, }).catch(res => { - if (res?.status === 401) { + if (res?.status === 401 && !ignoreUnauthorized) { this.handleUnauthorized(); } return Promise.reject(res); diff --git a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts index e4738a3da0027..abe3edf2284f4 100644 --- a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts @@ -505,6 +505,7 @@ describe('SupersetClientClass', () => { const mockRequestUrl = 'https://host/get/url'; const mockRequestPath = '/get/url'; const mockRequestSearch = '?param=1¶m=2'; + const mockHref = 'http://localhost' + mockRequestPath + mockRequestSearch; beforeEach(() => { originalLocation = window.location; @@ -514,7 +515,7 @@ describe('SupersetClientClass', () => { window.location = { pathname: mockRequestPath, search: mockRequestSearch, - href: mockRequestPath + mockRequestSearch, + href: mockHref, }; authSpy = jest .spyOn(SupersetClientClass.prototype, 'ensureAuth') @@ -561,7 +562,7 @@ describe('SupersetClientClass', () => { error = err; } finally { // unchanged href, no redirect - expect(window.location.href).toBe(mockRequestPath + mockRequestSearch); + expect(window.location.href).toBe(mockHref); expect(error.status).toBe(401); } }); @@ -575,13 +576,12 @@ describe('SupersetClientClass', () => { await client.request({ url: mockRequestUrl, method: 'GET', - ignoreUnauthorized: true, }); } catch (err) { error = err; } finally { // unchanged href, no redirect - expect(window.location.href).toBe(mockRequestPath + mockRequestSearch); + expect(window.location.href).toBe(mockHref); expect(error.status).toBe(401); expect(unauthorizedHandler).toHaveBeenCalledTimes(1); } From 605e281ff9e00c932da8b3992e1b4db697d2f621 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Mon, 14 Mar 2022 16:45:55 -0700 Subject: [PATCH 03/10] sort --- .../packages/superset-ui-core/src/connection/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/packages/superset-ui-core/src/connection/types.ts b/superset-frontend/packages/superset-ui-core/src/connection/types.ts index 004fb59e8c314..0ab382917e170 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/types.ts @@ -81,6 +81,7 @@ export interface RequestBase { fetchRetryOptions?: FetchRetryOptions; headers?: Headers; host?: Host; + ignoreUnauthorized?: boolean; mode?: Mode; method?: Method; jsonPayload?: Payload; @@ -89,7 +90,6 @@ export interface RequestBase { signal?: Signal; stringify?: Stringify; timeout?: ClientTimeout; - ignoreUnauthorized?: boolean; } export interface CallApi extends RequestBase { From 983f0fee2c054e87e75edde4adf70d761e85aca8 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Fri, 18 Mar 2022 13:25:01 -0700 Subject: [PATCH 04/10] guest unauthorized handler --- .../components/MessageToasts/withToasts.tsx | 3 +- superset-frontend/src/embedded/index.tsx | 33 ++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/components/MessageToasts/withToasts.tsx b/superset-frontend/src/components/MessageToasts/withToasts.tsx index 2d0486a65e347..f7dc7c72176f8 100644 --- a/superset-frontend/src/components/MessageToasts/withToasts.tsx +++ b/superset-frontend/src/components/MessageToasts/withToasts.tsx @@ -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, diff --git a/superset-frontend/src/embedded/index.tsx b/superset-frontend/src/embedded/index.tsx index c3e8f6a34f7db..5e541939ea8a4 100644 --- a/superset-frontend/src/embedded/index.tsx +++ b/superset-frontend/src/embedded/index.tsx @@ -19,12 +19,15 @@ 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'; const debugMode = process.env.WEBPACK_MODE === 'development'; @@ -75,11 +78,39 @@ 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 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, }); } From 3fa5be72a4f9453c97190929e5e488668de55ea7 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Fri, 18 Mar 2022 18:33:25 -0700 Subject: [PATCH 05/10] add toast container to embedded app --- superset-frontend/src/embedded/index.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/superset-frontend/src/embedded/index.tsx b/superset-frontend/src/embedded/index.tsx index 5e541939ea8a4..e3cabb4aab17c 100644 --- a/superset-frontend/src/embedded/index.tsx +++ b/superset-frontend/src/embedded/index.tsx @@ -28,6 +28,7 @@ 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'; @@ -52,6 +53,7 @@ const EmbeddedApp = () => ( + From 6384896d71b9691d35cc50d6bf1315b05f950120 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Fri, 18 Mar 2022 18:53:28 -0700 Subject: [PATCH 06/10] add option for toast presenter to go at the top --- ...{ToastContainer.jsx => ToastContainer.tsx} | 8 +++-- .../MessageToasts/ToastPresenter.tsx | 29 ++++++++++++------- superset-frontend/src/embedded/index.tsx | 2 +- 3 files changed, 24 insertions(+), 15 deletions(-) rename superset-frontend/src/components/MessageToasts/{ToastContainer.jsx => ToastContainer.tsx} (85%) diff --git a/superset-frontend/src/components/MessageToasts/ToastContainer.jsx b/superset-frontend/src/components/MessageToasts/ToastContainer.tsx similarity index 85% rename from superset-frontend/src/components/MessageToasts/ToastContainer.jsx rename to superset-frontend/src/components/MessageToasts/ToastContainer.tsx index d61920de579ee..1929d1d34533d 100644 --- a/superset-frontend/src/components/MessageToasts/ToastContainer.jsx +++ b/superset-frontend/src/components/MessageToasts/ToastContainer.tsx @@ -17,12 +17,14 @@ * under the License. */ import { bindActionCreators } from 'redux'; -import { connect } from 'react-redux'; +import { connect, ConnectedProps } from 'react-redux'; 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; diff --git a/superset-frontend/src/components/MessageToasts/ToastPresenter.tsx b/superset-frontend/src/components/MessageToasts/ToastPresenter.tsx index 8fcd4c44a90e3..7d778041d19d7 100644 --- a/superset-frontend/src/components/MessageToasts/ToastPresenter.tsx +++ b/superset-frontend/src/components/MessageToasts/ToastPresenter.tsx @@ -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` max-width: 600px; position: fixed; - bottom: 0px; + ${({ position }) => (position === 'bottom' ? 'bottom' : 'top')}: 0px; right: 0px; margin-right: 50px; margin-bottom: 50px; @@ -69,22 +73,25 @@ const StyledToastPresenter = styled.div` } `; -type ToastPresenterProps = { +type ToastPresenterProps = VisualProps & { toasts: Array; - removeToast: () => void; + removeToast: () => any; }; export default function ToastPresenter({ toasts, removeToast, + position = 'top', }: ToastPresenterProps) { return ( - toasts.length > 0 && ( - - {toasts.map(toast => ( - - ))} - - ) + <> + {toasts.length > 0 && ( + + {toasts.map(toast => ( + + ))} + + )} + ); } diff --git a/superset-frontend/src/embedded/index.tsx b/superset-frontend/src/embedded/index.tsx index e3cabb4aab17c..b390ec27a9f1c 100644 --- a/superset-frontend/src/embedded/index.tsx +++ b/superset-frontend/src/embedded/index.tsx @@ -53,7 +53,7 @@ const EmbeddedApp = () => ( - + From 419f54b8eda7332be07fa7bff9ebc5b4a86438f2 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Fri, 18 Mar 2022 19:12:59 -0700 Subject: [PATCH 07/10] remove confusing comms logging --- superset-frontend/src/embedded/index.tsx | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/superset-frontend/src/embedded/index.tsx b/superset-frontend/src/embedded/index.tsx index b390ec27a9f1c..64e517610e968 100644 --- a/superset-frontend/src/embedded/index.tsx +++ b/superset-frontend/src/embedded/index.tsx @@ -117,14 +117,6 @@ function setupGuestClient(guestToken: string) { } function validateMessageEvent(event: MessageEvent) { - if ( - 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'); // } @@ -138,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; } From e76094a7c3771eb13eb0c85003fdb0d19b56e464 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Fri, 18 Mar 2022 19:29:14 -0700 Subject: [PATCH 08/10] lint --- .../test/connection/SupersetClientClass.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts index abe3edf2284f4..921061b22f297 100644 --- a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts @@ -505,7 +505,7 @@ describe('SupersetClientClass', () => { const mockRequestUrl = 'https://host/get/url'; const mockRequestPath = '/get/url'; const mockRequestSearch = '?param=1¶m=2'; - const mockHref = 'http://localhost' + mockRequestPath + mockRequestSearch; + const mockHref = `http://localhost${mockRequestPath + mockRequestSearch}`; beforeEach(() => { originalLocation = window.location; From 11a866955f8947f17567c59fe1799c794c85ab53 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com> Date: Fri, 18 Mar 2022 19:30:23 -0700 Subject: [PATCH 09/10] Update superset-frontend/src/embedded/index.tsx --- superset-frontend/src/embedded/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/embedded/index.tsx b/superset-frontend/src/embedded/index.tsx index 64e517610e968..3cd89a88be225 100644 --- a/superset-frontend/src/embedded/index.tsx +++ b/superset-frontend/src/embedded/index.tsx @@ -98,7 +98,7 @@ function guestUnauthorizedHandler() { '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 closed + duration: -1, // stay open until manually closed noDuplicate: true, }, ), From 49a4d1a0800490b930c411ee57c62d933ef69d54 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Fri, 18 Mar 2022 20:28:44 -0700 Subject: [PATCH 10/10] type correction --- .../src/components/MessageToasts/ToastContainer.tsx | 2 +- .../src/components/MessageToasts/ToastPresenter.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/components/MessageToasts/ToastContainer.tsx b/superset-frontend/src/components/MessageToasts/ToastContainer.tsx index 1929d1d34533d..0157ff8a4d988 100644 --- a/superset-frontend/src/components/MessageToasts/ToastContainer.tsx +++ b/superset-frontend/src/components/MessageToasts/ToastContainer.tsx @@ -17,7 +17,7 @@ * under the License. */ import { bindActionCreators } from 'redux'; -import { connect, ConnectedProps } from 'react-redux'; +import { connect } from 'react-redux'; import ToastPresenter from './ToastPresenter'; import { removeToast } from './actions'; diff --git a/superset-frontend/src/components/MessageToasts/ToastPresenter.tsx b/superset-frontend/src/components/MessageToasts/ToastPresenter.tsx index 7d778041d19d7..0be676ad788e5 100644 --- a/superset-frontend/src/components/MessageToasts/ToastPresenter.tsx +++ b/superset-frontend/src/components/MessageToasts/ToastPresenter.tsx @@ -73,7 +73,7 @@ const StyledToastPresenter = styled.div` } `; -type ToastPresenterProps = VisualProps & { +type ToastPresenterProps = Partial & { toasts: Array; removeToast: () => any; };