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.

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

fix(sqllab): remove link to sqllab if missing perms #22566

Merged
merged 12 commits into from
Jan 9, 2023
38 changes: 37 additions & 1 deletion superset-frontend/src/dashboard/util/permissionUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ import {
} from 'src/types/bootstrapTypes';
import { Dashboard } from 'src/types/Dashboard';
import Owner from 'src/types/Owner';
import { canUserEditDashboard, isUserAdmin } from './permissionUtils';
import {
canUserAccessSqlLab,
canUserEditDashboard,
isUserAdmin,
} from './permissionUtils';
Comment on lines +25 to +29
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: these should probably be moved to a general util outside the dashboard section, but refactoring it seems outside the scope of this PR


const ownerUser: UserWithPermissionsAndRoles = {
createdOn: '2021-05-12T16:56:22.116839',
Expand Down Expand Up @@ -60,6 +64,14 @@ const owner: Owner = {
username: ownerUser.username,
};

const sqlLabUser: UserWithPermissionsAndRoles = {
...ownerUser,
roles: {
...ownerUser.roles,
sql_lab: [],
},
};

const undefinedUser: UndefinedUser = {};

describe('canUserEditDashboard', () => {
Expand Down Expand Up @@ -109,10 +121,34 @@ test('isUserAdmin returns true for admin user', () => {
expect(isUserAdmin(adminUser)).toEqual(true);
});

test('isUserAdmin returns false for undefined', () => {
expect(isUserAdmin(undefined)).toEqual(false);
});

test('isUserAdmin returns false for undefined user', () => {
expect(isUserAdmin(undefinedUser)).toEqual(false);
});

test('isUserAdmin returns false for non-admin user', () => {
expect(isUserAdmin(ownerUser)).toEqual(false);
});

test('canUserAccessSqlLab returns true for admin user', () => {
expect(canUserAccessSqlLab(adminUser)).toEqual(true);
});

test('canUserAccessSqlLab returns false for undefined', () => {
expect(canUserAccessSqlLab(undefined)).toEqual(false);
});

test('canUserAccessSqlLab returns false for undefined user', () => {
expect(canUserAccessSqlLab(undefinedUser)).toEqual(false);
});

test('canUserAccessSqlLab returns false for non-sqllab role', () => {
expect(canUserAccessSqlLab(ownerUser)).toEqual(false);
});

test('canUserAccessSqlLab returns true for sqllab role', () => {
expect(canUserAccessSqlLab(sqlLabUser)).toEqual(true);
});
15 changes: 14 additions & 1 deletion superset-frontend/src/dashboard/util/permissionUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ import { findPermission } from 'src/utils/findPermission';
// this should really be a config value,
// but is hardcoded in backend logic already, so...
const ADMIN_ROLE_NAME = 'admin';
const SQL_LAB_ROLE = 'sql_lab';

export const isUserAdmin = (
user: UserWithPermissionsAndRoles | UndefinedUser,
user?: UserWithPermissionsAndRoles | UndefinedUser,
) =>
isUserWithPermissionsAndRoles(user) &&
Object.keys(user.roles || {}).some(
Expand All @@ -50,3 +51,15 @@ export const canUserEditDashboard = (
isUserWithPermissionsAndRoles(user) &&
(isUserAdmin(user) || isUserDashboardOwner(dashboard, user)) &&
findPermission('can_write', 'Dashboard', user.roles);

export function canUserAccessSqlLab(
user?: UserWithPermissionsAndRoles | UndefinedUser,
) {
return (
isUserAdmin(user) ||
(isUserWithPermissionsAndRoles(user) &&
Object.keys(user.roles || {}).some(
role => role.toLowerCase() === SQL_LAB_ROLE,
))
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@
import React from 'react';
import { render, screen, act, waitFor } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import { SupersetClient, DatasourceType } from '@superset-ui/core';
import { DatasourceType, JsonObject, SupersetClient } from '@superset-ui/core';
import fetchMock from 'fetch-mock';
import DatasourceControl from '.';

const SupersetClientGet = jest.spyOn(SupersetClient, 'get');

const createProps = () => ({
const createProps = (overrides: JsonObject = {}) => ({
hovered: false,
type: 'DatasourceControl',
label: 'Datasource',
Expand Down Expand Up @@ -64,6 +64,7 @@ const createProps = () => ({
},
onChange: jest.fn(),
onDatasourceSave: jest.fn(),
...overrides,
});

async function openAndSaveChanges(datasource: any) {
Expand Down Expand Up @@ -104,6 +105,52 @@ test('Should open a menu', async () => {
expect(screen.getByText('View in SQL Lab')).toBeInTheDocument();
});

test('Should not show SQL Lab for non sql_lab role', async () => {
const props = createProps({
user: {
createdOn: '2021-04-27T18:12:38.952304',
email: 'gamma',
firstName: 'gamma',
isActive: true,
lastName: 'gamma',
permissions: {},
roles: { Gamma: [] },
userId: 2,
username: 'gamma',
},
});
render(<DatasourceControl {...props} />);

userEvent.click(screen.getByTestId('datasource-menu-trigger'));

expect(await screen.findByText('Edit dataset')).toBeInTheDocument();
expect(screen.getByText('Swap dataset')).toBeInTheDocument();
expect(screen.queryByText('View in SQL Lab')).not.toBeInTheDocument();
});

test('Should show SQL Lab for sql_lab role', async () => {
const props = createProps({
user: {
createdOn: '2021-04-27T18:12:38.952304',
email: 'sql',
firstName: 'sql',
isActive: true,
lastName: 'sql',
permissions: {},
roles: { Gamma: [], sql_lab: [] },
userId: 2,
username: 'sql',
},
});
render(<DatasourceControl {...props} />);

userEvent.click(screen.getByTestId('datasource-menu-trigger'));

expect(await screen.findByText('Edit dataset')).toBeInTheDocument();
expect(screen.getByText('Swap dataset')).toBeInTheDocument();
expect(screen.getByText('View in SQL Lab')).toBeInTheDocument();
});

test('Click on Swap dataset option', async () => {
const props = createProps();
SupersetClientGet.mockImplementation(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ import ErrorAlert from 'src/components/ErrorMessage/ErrorAlert';
import WarningIconWithTooltip from 'src/components/WarningIconWithTooltip';
import { URL_PARAMS } from 'src/constants';
import { getDatasourceAsSaveableDataset } from 'src/utils/datasourceUtils';
import { isUserAdmin } from 'src/dashboard/util/permissionUtils';
import {
canUserAccessSqlLab,
isUserAdmin,
} from 'src/dashboard/util/permissionUtils';
import ModalTrigger from 'src/components/ModalTrigger';
import ViewQueryModalFooter from 'src/explore/components/controls/ViewQueryModalFooter';
import ViewQuery from 'src/explore/components/controls/ViewQuery';
Expand Down Expand Up @@ -279,6 +282,8 @@ class DatasourceControl extends React.PureComponent {
datasource.owners?.map(o => o.id || o.value).includes(user.userId) ||
isUserAdmin(user);

const canAccessSqlLab = canUserAccessSqlLab(user);

const editText = t('Edit dataset');

const defaultDatasourceMenu = (
Expand All @@ -303,7 +308,7 @@ class DatasourceControl extends React.PureComponent {
</Menu.Item>
)}
<Menu.Item key={CHANGE_DATASET}>{t('Swap dataset')}</Menu.Item>
{datasource && (
{datasource && canAccessSqlLab && (
<Menu.Item key={VIEW_IN_SQL_LAB}>{t('View in SQL Lab')}</Menu.Item>
)}
</Menu>
Expand Down Expand Up @@ -333,7 +338,9 @@ class DatasourceControl extends React.PureComponent {
responsive
/>
</Menu.Item>
<Menu.Item key={VIEW_IN_SQL_LAB}>{t('View in SQL Lab')}</Menu.Item>
{canAccessSqlLab && (
<Menu.Item key={VIEW_IN_SQL_LAB}>{t('View in SQL Lab')}</Menu.Item>
)}
<Menu.Item key={SAVE_AS_DATASET}>{t('Save as dataset')}</Menu.Item>
</Menu>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import copyTextToClipboard from 'src/utils/copy';
import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
import ImportModelsModal from 'src/components/ImportModal/index';
import Icons from 'src/components/Icons';
import { BootstrapUser } from 'src/types/bootstrapTypes';
import SavedQueryPreviewModal from './SavedQueryPreviewModal';

const PAGE_SIZE = 25;
Expand All @@ -69,9 +70,7 @@ const CONFIRM_OVERWRITE_MESSAGE = t(
interface SavedQueryListProps {
addDangerToast: (msg: string) => void;
addSuccessToast: (msg: string) => void;
user: {
userId: string | number;
};
user: BootstrapUser;
}

const StyledTableLabel = styled.div`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ describe('ActivityTable', () => {
activityData: mockData,
setActiveChild: jest.fn(),
user: { userId: '1' },
loadedCount: 3,
isFetchingActivityData: false,
};

let wrapper: ReactWrapper;
Expand Down Expand Up @@ -127,7 +127,7 @@ describe('ActivityTable', () => {
activityData: {},
setActiveChild: jest.fn(),
user: { userId: '1' },
loadedCount: 3,
isFetchingActivityData: false,
};
const wrapper = mount(
<Provider store={store}>
Expand Down
29 changes: 12 additions & 17 deletions superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ interface ActivityProps {
activeChild: string;
setActiveChild: (arg0: string) => void;
activityData: ActivityData;
loadedCount: number;
isFetchingActivityData: boolean;
}

const Styles = styled.div`
Expand Down Expand Up @@ -128,22 +128,21 @@ export default function ActivityTable({
setActiveChild,
activityData,
user,
loadedCount,
isFetchingActivityData,
}: ActivityProps) {
const [editedObjs, setEditedObjs] = useState<Array<ActivityData>>();
const [loadingState, setLoadingState] = useState(false);
const [editedCards, setEditedCards] = useState<ActivityData[]>();
const [isFetchingEditedCards, setIsFetchingEditedCards] = useState(false);

const getEditedCards = () => {
setLoadingState(true);
setIsFetchingEditedCards(true);
getEditedObjects(user.userId).then(r => {
setEditedObjs([...r.editedChart, ...r.editedDash]);
setLoadingState(false);
setEditedCards([...r.editedChart, ...r.editedDash]);
setIsFetchingEditedCards(false);
});
};

useEffect(() => {
if (activeChild === TableTab.Edited) {
setLoadingState(true);
getEditedCards();
}
}, [activeChild]);
Expand Down Expand Up @@ -178,9 +177,9 @@ export default function ActivityTable({
});
}
const renderActivity = () =>
(activeChild !== TableTab.Edited
? activityData[activeChild]
: editedObjs
(activeChild === TableTab.Edited
? editedCards
: activityData[activeChild]
).map((entity: ActivityObject) => {
const url = getEntityUrl(entity);
const lastActionOn = getEntityLastActionOn(entity);
Expand All @@ -200,18 +199,14 @@ export default function ActivityTable({
);
});

const doneFetching = loadedCount < 3;

if ((loadingState && !editedObjs) || doneFetching) {
if ((isFetchingEditedCards && !editedCards) || isFetchingActivityData) {
return <LoadingCards />;
}
return (
<Styles>
<SubMenu activeChild={activeChild} tabs={tabs} />
{activityData[activeChild]?.length > 0 ||
(activeChild === TableTab.Edited &&
editedObjs &&
editedObjs.length > 0) ? (
(activeChild === TableTab.Edited && editedCards?.length) ? (
<CardContainer className="recentCards">
{renderActivity()}
</CardContainer>
Expand Down
55 changes: 54 additions & 1 deletion superset-frontend/src/views/CRUD/welcome/Welcome.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,15 @@ const mockedProps = {
userId: 5,
email: 'alpha@alpha.com',
isActive: true,
isAnonymous: false,
permissions: {},
roles: {
sql_lab: [],
},
},
};

describe('Welcome', () => {
describe('Welcome with sql role', () => {
let wrapper: ReactWrapper;

beforeAll(async () => {
Expand All @@ -122,6 +127,10 @@ describe('Welcome', () => {
});
});

afterAll(() => {
fetchMock.resetHistory();
});

it('renders', () => {
expect(wrapper).toExist();
});
Expand All @@ -142,6 +151,50 @@ describe('Welcome', () => {
});
});

describe('Welcome without sql role', () => {
let wrapper: ReactWrapper;

beforeAll(async () => {
await act(async () => {
const props = {
...mockedProps,
user: {
...mockedProps.user,
roles: {},
},
};
wrapper = mount(
<Provider store={store}>
<Welcome {...props} />
</Provider>,
);
});
});

afterAll(() => {
fetchMock.resetHistory();
});

it('renders', () => {
expect(wrapper).toExist();
});

it('renders all panels on the page on page load', () => {
expect(wrapper.find('CollapsePanel')).toHaveLength(6);
});

it('calls api methods in parallel on page load', () => {
const chartCall = fetchMock.calls(/chart\/\?q/);
const savedQueryCall = fetchMock.calls(/saved_query\/\?q/);
const recentCall = fetchMock.calls(/superset\/recent_activity\/*/);
const dashboardCall = fetchMock.calls(/dashboard\/\?q/);
expect(chartCall).toHaveLength(2);
expect(recentCall).toHaveLength(1);
expect(savedQueryCall).toHaveLength(0);
expect(dashboardCall).toHaveLength(2);
});
});

async function mountAndWait(props = mockedProps) {
const wrapper = mount(
<Provider store={store}>
Expand Down
Loading