Skip to content

Commit

Permalink
fix(nav): infinite redirect and upload dataset nav permissions (apach…
Browse files Browse the repository at this point in the history
…e#19708)

(cherry picked from commit 32a9265)
  • Loading branch information
ktmud authored and sadpandajoe committed Apr 14, 2022
1 parent 228e3b9 commit b477ee3
Show file tree
Hide file tree
Showing 9 changed files with 104 additions and 63 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
ignoreUnauthorized,
ignoreUnauthorized = false,
...rest
}: RequestConfig & { parseMethod?: T }) {
await this.ensureAuth();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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({});

Expand Down
22 changes: 14 additions & 8 deletions superset-frontend/src/views/CRUD/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
Expand Down
86 changes: 50 additions & 36 deletions superset-frontend/src/views/components/MenuRight.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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,
},
],
},
Expand All @@ -176,7 +176,7 @@ const RightMenu = ({
},
];

const hasFileUploadEnabled = () => {
const checkAllowUploads = () => {
const payload = {
filters: [
{ col: 'allow_file_upload', opr: 'upload_is_enabled', value: true },
Expand All @@ -189,7 +189,11 @@ const RightMenu = ({
});
};

useEffect(() => hasFileUploadEnabled(), []);
useEffect(() => {
if (canUploadData) {
checkAllowUploads();
}
}, [canUploadData]);

const menuIconAndLabel = (menu: MenuObjectProps) => (
<>
Expand Down Expand Up @@ -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"
Expand All @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions superset-frontend/src/views/components/SubMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})}
Expand All @@ -264,7 +264,7 @@ const SubMenuComponent: React.FunctionComponent<SubMenuProps> = props => {
<a href={tab.url} onClick={tab.onClick}>
{tab.label}
</a>
</li>
</div>
</Menu.Item>
);
})}
Expand Down

0 comments on commit b477ee3

Please sign in to comment.