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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Apr 14, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db9f124
fix(nav): infinite redirect and upload dataset nav permissions
ktmud Apr 13, 2022
751dfd8
fix frontend tests
ktmud Apr 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
View file
Open in desktop
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')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Only redirect when we are not on the login page so we never see infinite redirect caused by the API client ever again.

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,
Copy link
Member Author

Choose a reason for hiding this comment

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

Be more explicit what is the default. I kind of feel this should probably default to true but that's another topic. Async API requests should never implicitly redirect users as it may often cause surprises. There should be better error handling on the API user side instead.

cc @geido @kgabryje could you give more context on why #17597 made redirects on 401 the default?

Copy link
Member

@nytai nytai Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I strongly agree here, this behavior is quite surprising from an api client. Though I guess this approach involves the least amount of code changes.

From a UX perspective I'd suggest presenting the user with the option to re-auth. There could be unsaved work that would be lost via a the redirect.

Copy link
Member Author

Choose a reason for hiding this comment

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

@geido @kgabryje @suddjian do you mind following up here to change the default to true and set it to false where needed? I'm not familiar where we wanted the redirect behavior to be the default.

Copy link
Member

@etr2460 etr2460 Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think we always want to redirect to login on a 401. 401 means that there's no auth token at all. 403 should be used if the user is forbidden, right?

Copy link
Member Author

@ktmud ktmud Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@etr2460 This option was actually added by @suddjian because he had a case where redirects aren't desired: #19144 (i.e. when Superset in embedded in another app)

Copy link
Member

Choose a reason for hiding this comment

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

ahh i see. so we're fine with rendering some stuff even when a user is logged out. I guess that's fine then, carry on!

Copy link
Member

Choose a reason for hiding this comment

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

We will have a conversation about having the default to true and we'll follow up. Thank you!

...rest
}: RequestConfig & { parseMethod?: T }) {
await this.ensureAuth();
Expand Down
27 changes: 23 additions & 4 deletions superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts
View file
Open in desktop
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
View file
Open in desktop
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,
};
};
4 changes: 2 additions & 2 deletions superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/welcome/ChartTable.test.tsx
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions superset-frontend/src/views/CRUD/welcome/DashboardTable.test.tsx
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions superset-frontend/src/views/CRUD/welcome/SavedQueries.test.tsx
View file
Open in desktop
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
View file
Open in desktop
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 && (
Copy link
Member Author

Choose a reason for hiding this comment

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

Bycatch: only render the DatabaseModal when users can create new database connection.

<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
View file
Open in desktop
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
Copy link
Member Author

Choose a reason for hiding this comment

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

Bycatch: fix a React warning about nested <li />s.

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