fix(nav): infinite redirect and upload dataset nav permissions (#19708)

apache · Apr 14, 2022 · 32a9265 · 32a9265
1 parent 8865656
commit 32a9265
Show file tree

Hide file tree

Showing 9 changed files with 104 additions and 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
@@ -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
@@ -505,7 +505,8 @@ describe('SupersetClientClass', () => {
     const mockRequestUrl = 'https://host/get/url';
     const mockRequestPath = '/get/url';
     const mockRequestSearch = '?param=1&param=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
@@ -427,14 +427,20 @@ export const uploadUserPerms = (
   colExt: Array<string>,
   excelExt: Array<string>,
   allowedExt: Array<string>,
-) => ({
-  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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<boolean>(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 (
     <StyledDiv align={align}>
-      <DatabaseModal
-        onHide={handleOnHideModal}
-        show={showModal}
-        dbEngine={engine}
-      />
+      {canDatabase && (
+        <DatabaseModal
+          onHide={handleOnHideModal}
+          show={showModal}
+          dbEngine={engine}
+        />
+      )}
       <Menu
         selectable={false}
         mode="horizontal"
@@ -262,23 +268,31 @@ const RightMenu = ({
             icon={<Icons.TriangleDown />}
           >
             {dropdownItems.map(menu => {
+              const canShowChild = menu.childs?.some(
+                item => typeof item === 'object' && !!item.perm,
+              );
               if (menu.childs) {
-                return canDatabase || canUpload ? (
-                  <SubMenu
-                    key={`sub2_${menu.label}`}
-                    className="data-menu"
-                    title={menuIconAndLabel(menu)}
-                  >
-                    {menu.childs.map((item, idx) =>
-                      typeof item !== 'string' && item.name && item.perm ? (
-                        <Fragment key={item.name}>
-                          {idx === 2 && <Menu.Divider />}
-                          {buildMenuItem(item)}
-                        </Fragment>
-                      ) : null,
-                    )}
-                  </SubMenu>
-                ) : null;
+                if (canShowChild) {
+                  return (
+                    <SubMenu
+                      key={`sub2_${menu.label}`}
+                      className="data-menu"
+                      title={menuIconAndLabel(menu)}
+                    >
+                      {menu.childs.map((item, idx) =>
+                        typeof item !== 'string' && item.name && item.perm ? (
+                          <Fragment key={item.name}>
+                            {idx === 2 && <Menu.Divider />}
+                            {buildMenuItem(item)}
+                          </Fragment>
+                        ) : null,
+                      )}
+                    </SubMenu>
+                  );
+                }
+                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
@@ -240,22 +240,22 @@ const SubMenuComponent: React.FunctionComponent<SubMenuProps> = props => {
             if ((props.usesRouter || hasHistory) && !!tab.usesRouter) {
               return (
                 <Menu.Item key={tab.label}>
-                  <li
+                  <div
                     role="tab"
                     data-test={tab['data-test']}
                     className={tab.name === props.activeChild ? 'active' : ''}
                   >
                     <div>
                       <Link to={tab.url || ''}>{tab.label}</Link>
                     </div>
-                  </li>
+                  </div>
                 </Menu.Item>
               );
             }
 
             return (
               <Menu.Item key={tab.label}>
-                <li
+                <div
                   className={cx('no-router', {
                     active: tab.name === props.activeChild,
                   })}
@@ -264,7 +264,7 @@ const SubMenuComponent: React.FunctionComponent<SubMenuProps> = props => {
                   <a href={tab.url} onClick={tab.onClick}>
                     {tab.label}
                   </a>
-                </li>
+                </div>
               </Menu.Item>
             );
           })}