From 32a9265cc0cb850910e55b6f49a73169fc7ed377 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Wed, 13 Apr 2022 23:29:56 -0700 Subject: [PATCH] fix(nav): infinite redirect and upload dataset nav permissions (#19708) --- .../src/connection/SupersetClientClass.ts | 10 ++- .../connection/SupersetClientClass.test.ts | 27 +++++- superset-frontend/src/views/CRUD/utils.tsx | 22 +++-- .../views/CRUD/welcome/ActivityTable.test.tsx | 4 +- .../views/CRUD/welcome/ChartTable.test.tsx | 2 +- .../CRUD/welcome/DashboardTable.test.tsx | 4 +- .../views/CRUD/welcome/SavedQueries.test.tsx | 4 +- .../src/views/components/MenuRight.tsx | 86 +++++++++++-------- .../src/views/components/SubMenu.tsx | 8 +- 9 files changed, 104 insertions(+), 63 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 bf0c02a81d789..5e046bce7e929 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts @@ -34,9 +34,11 @@ import { import { DEFAULT_FETCH_RETRY_OPTIONS, DEFAULT_BASE_URL } from './constants'; const defaultUnauthorizedHandler = () => { - window.location.href = `/login?next=${ - window.location.pathname + window.location.search - }`; + if (!window.location.pathname.startsWith('/login')) { + window.location.href = `/login?next=${ + window.location.pathname + window.location.search + }`; + } }; export default class SupersetClientClass { @@ -161,7 +163,7 @@ export default class SupersetClientClass { headers, timeout, fetchRetryOptions, - ignoreUnauthorized, + ignoreUnauthorized = false, ...rest }: RequestConfig & { parseMethod?: T }) { await this.ensureAuth(); 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 921061b22f297..a17cbceb223d7 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,8 @@ 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 mockRequestRelativeUrl = mockRequestPath + mockRequestSearch; + const mockHref = `http://localhost${mockRequestRelativeUrl}`; beforeEach(() => { originalLocation = window.location; @@ -541,13 +542,31 @@ describe('SupersetClientClass', () => { error = err; } finally { const redirectURL = window.location.href; - expect(redirectURL).toBe( - `/login?next=${mockRequestPath + mockRequestSearch}`, - ); + expect(redirectURL).toBe(`/login?next=${mockRequestRelativeUrl}`); expect(error.status).toBe(401); } }); + it('should not redirect again if already on login page', async () => { + const client = new SupersetClientClass({}); + + // @ts-expect-error + window.location = { + href: '/login?next=something', + pathname: '/login', + search: '?next=something', + }; + + let error; + try { + await client.request({ url: mockRequestUrl, method: 'GET' }); + } catch (err) { + error = err; + } finally { + expect(window.location.href).toBe('/login?next=something'); + expect(error.status).toBe(401); + } + }); it('does nothing if instructed to ignoreUnauthorized', async () => { const client = new SupersetClientClass({}); diff --git a/superset-frontend/src/views/CRUD/utils.tsx b/superset-frontend/src/views/CRUD/utils.tsx index 1a7b3ca1fa721..31f3d4c9edeb1 100644 --- a/superset-frontend/src/views/CRUD/utils.tsx +++ b/superset-frontend/src/views/CRUD/utils.tsx @@ -427,14 +427,20 @@ export const uploadUserPerms = ( colExt: Array, excelExt: Array, allowedExt: Array, -) => ({ - canUploadCSV: +) => { + const canUploadCSV = findPermission('can_this_form_get', 'CsvToDatabaseView', roles) && - checkUploadExtensions(csvExt, allowedExt), - canUploadColumnar: + checkUploadExtensions(csvExt, allowedExt); + const canUploadColumnar = checkUploadExtensions(colExt, allowedExt) && - findPermission('can_this_form_get', 'ColumnarToDatabaseView', roles), - canUploadExcel: + findPermission('can_this_form_get', 'ColumnarToDatabaseView', roles); + const canUploadExcel = checkUploadExtensions(excelExt, allowedExt) && - findPermission('can_this_form_get', 'ExcelToDatabaseView', roles), -}); + findPermission('can_this_form_get', 'ExcelToDatabaseView', roles); + return { + canUploadCSV, + canUploadColumnar, + canUploadExcel, + canUploadData: canUploadCSV || canUploadColumnar || canUploadExcel, + }; +}; diff --git a/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx b/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx index 2fd7feb977ed5..71067b817a3e2 100644 --- a/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx +++ b/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx @@ -100,14 +100,14 @@ describe('ActivityTable', () => { expect(wrapper.find(ActivityTable)).toExist(); }); it('renders tabs with three buttons', () => { - expect(wrapper.find('li.no-router')).toHaveLength(3); + expect(wrapper.find('[role="tab"]')).toHaveLength(3); }); it('renders ActivityCards', async () => { expect(wrapper.find('ListViewCard')).toExist(); }); it('calls the getEdited batch call when edited tab is clicked', async () => { act(() => { - const handler = wrapper.find('li.no-router a').at(1).prop('onClick'); + const handler = wrapper.find('[role="tab"] a').at(1).prop('onClick'); if (handler) { handler({} as any); } diff --git a/superset-frontend/src/views/CRUD/welcome/ChartTable.test.tsx b/superset-frontend/src/views/CRUD/welcome/ChartTable.test.tsx index c61cb1b33e1b3..cfa9230328c08 100644 --- a/superset-frontend/src/views/CRUD/welcome/ChartTable.test.tsx +++ b/superset-frontend/src/views/CRUD/welcome/ChartTable.test.tsx @@ -79,7 +79,7 @@ describe('ChartTable', () => { it('fetches chart favorites and renders chart cards', async () => { act(() => { - const handler = wrapper.find('li.no-router a').at(0).prop('onClick'); + const handler = wrapper.find('[role="tab"] a').at(0).prop('onClick'); if (handler) { handler({} as any); } diff --git a/superset-frontend/src/views/CRUD/welcome/DashboardTable.test.tsx b/superset-frontend/src/views/CRUD/welcome/DashboardTable.test.tsx index 26fd3a13d32d7..79f88e3128834 100644 --- a/superset-frontend/src/views/CRUD/welcome/DashboardTable.test.tsx +++ b/superset-frontend/src/views/CRUD/welcome/DashboardTable.test.tsx @@ -71,10 +71,10 @@ describe('DashboardTable', () => { it('render a submenu with clickable tabs and buttons', async () => { expect(wrapper.find('SubMenu')).toExist(); - expect(wrapper.find('li.no-router')).toHaveLength(2); + expect(wrapper.find('[role="tab"]')).toHaveLength(2); expect(wrapper.find('Button')).toHaveLength(6); act(() => { - const handler = wrapper.find('li.no-router a').at(0).prop('onClick'); + const handler = wrapper.find('[role="tab"] a').at(0).prop('onClick'); if (handler) { handler({} as any); } diff --git a/superset-frontend/src/views/CRUD/welcome/SavedQueries.test.tsx b/superset-frontend/src/views/CRUD/welcome/SavedQueries.test.tsx index dd883a1aa1ab4..f656a9e8e175f 100644 --- a/superset-frontend/src/views/CRUD/welcome/SavedQueries.test.tsx +++ b/superset-frontend/src/views/CRUD/welcome/SavedQueries.test.tsx @@ -81,7 +81,7 @@ describe('SavedQueries', () => { const clickTab = (idx: number) => { act(() => { - const handler = wrapper.find('li.no-router a').at(idx).prop('onClick'); + const handler = wrapper.find('[role="tab"] a').at(idx).prop('onClick'); if (handler) { handler({} as any); } @@ -105,7 +105,7 @@ describe('SavedQueries', () => { it('renders a submenu with clickable tables and buttons', async () => { expect(wrapper.find(SubMenu)).toExist(); - expect(wrapper.find('li.no-router')).toHaveLength(1); + expect(wrapper.find('[role="tab"]')).toHaveLength(1); expect(wrapper.find('button')).toHaveLength(2); clickTab(0); await waitForComponentToPaint(wrapper); diff --git a/superset-frontend/src/views/components/MenuRight.tsx b/superset-frontend/src/views/components/MenuRight.tsx index 9d657aa9b7df2..1c46f6bcf079d 100644 --- a/superset-frontend/src/views/components/MenuRight.tsx +++ b/superset-frontend/src/views/components/MenuRight.tsx @@ -105,15 +105,15 @@ const RightMenu = ({ const canChart = findPermission('can_write', 'Chart', roles); const canDatabase = findPermission('can_write', 'Database', roles); - const { canUploadCSV, canUploadColumnar, canUploadExcel } = uploadUserPerms( - roles, - CSV_EXTENSIONS, - COLUMNAR_EXTENSIONS, - EXCEL_EXTENSIONS, - ALLOWED_EXTENSIONS, - ); + const { canUploadData, canUploadCSV, canUploadColumnar, canUploadExcel } = + uploadUserPerms( + roles, + CSV_EXTENSIONS, + COLUMNAR_EXTENSIONS, + EXCEL_EXTENSIONS, + ALLOWED_EXTENSIONS, + ); - const canUpload = canUploadCSV || canUploadColumnar || canUploadExcel; const showActionDropdown = canSql || canChart || canDashboard; const [allowUploads, setAllowUploads] = useState(false); const isAdmin = isUserAdmin(user); @@ -137,19 +137,19 @@ const RightMenu = ({ label: t('Upload CSV to database'), name: 'Upload a CSV', url: '/csvtodatabaseview/form', - perm: CSV_EXTENSIONS && showUploads, + perm: canUploadCSV && showUploads, }, { label: t('Upload columnar file to database'), name: 'Upload a Columnar file', url: '/columnartodatabaseview/form', - perm: COLUMNAR_EXTENSIONS && showUploads, + perm: canUploadColumnar && showUploads, }, { label: t('Upload Excel file to database'), name: 'Upload Excel', url: '/exceltodatabaseview/form', - perm: EXCEL_EXTENSIONS && showUploads, + perm: canUploadExcel && showUploads, }, ], }, @@ -176,7 +176,7 @@ const RightMenu = ({ }, ]; - const hasFileUploadEnabled = () => { + const checkAllowUploads = () => { const payload = { filters: [ { col: 'allow_file_upload', opr: 'upload_is_enabled', value: true }, @@ -189,7 +189,11 @@ const RightMenu = ({ }); }; - useEffect(() => hasFileUploadEnabled(), []); + useEffect(() => { + if (canUploadData) { + checkAllowUploads(); + } + }, [canUploadData]); const menuIconAndLabel = (menu: MenuObjectProps) => ( <> @@ -234,19 +238,21 @@ const RightMenu = ({ }; const onMenuOpen = (openKeys: string[]) => { - if (openKeys.length) { - return hasFileUploadEnabled(); + if (openKeys.length && canUploadData) { + return checkAllowUploads(); } return null; }; return ( - + {canDatabase && ( + + )} } > {dropdownItems.map(menu => { + const canShowChild = menu.childs?.some( + item => typeof item === 'object' && !!item.perm, + ); if (menu.childs) { - return canDatabase || canUpload ? ( - - {menu.childs.map((item, idx) => - typeof item !== 'string' && item.name && item.perm ? ( - - {idx === 2 && } - {buildMenuItem(item)} - - ) : null, - )} - - ) : null; + if (canShowChild) { + return ( + + {menu.childs.map((item, idx) => + typeof item !== 'string' && item.name && item.perm ? ( + + {idx === 2 && } + {buildMenuItem(item)} + + ) : null, + )} + + ); + } + if (!menu.url) { + return null; + } } return ( findPermission( diff --git a/superset-frontend/src/views/components/SubMenu.tsx b/superset-frontend/src/views/components/SubMenu.tsx index 9dc5b41cdcfb2..2ae897196b78b 100644 --- a/superset-frontend/src/views/components/SubMenu.tsx +++ b/superset-frontend/src/views/components/SubMenu.tsx @@ -240,7 +240,7 @@ const SubMenuComponent: React.FunctionComponent = props => { if ((props.usesRouter || hasHistory) && !!tab.usesRouter) { return ( -
  • = props => {
    {tab.label}
    -
    • +     ); } return ( -
  • = props => { {tab.label} -
    • +     ); })}