Skip to content

Commit

Permalink
fix(sqllab): remove link to sqllab if missing perms (#22566)
Browse files Browse the repository at this point in the history
  • Loading branch information
villebro authored Jan 9, 2023
1 parent 30dab3a commit 5b2ca97
Show file tree
Hide file tree
Showing 12 changed files with 287 additions and 88 deletions.
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';

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

0 comments on commit 5b2ca97

Please sign in to comment.