From e0759b9db3eb1c6438096bef7c130d7d7c48bf5a Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Thu, 29 Dec 2022 10:31:02 +0000 Subject: [PATCH 01/12] fix(sqllab): remove link to sqllab if missing perms --- superset-frontend/src/SqlLab/App.jsx | 6 ++++ .../dashboard/util/permissionUtils.test.ts | 30 ++++++++++++++++++- .../src/dashboard/util/permissionUtils.ts | 13 ++++++++ .../controls/DatasourceControl/index.jsx | 13 ++++++-- 4 files changed, 58 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/SqlLab/App.jsx b/superset-frontend/src/SqlLab/App.jsx index 812202eec20f..22e63cad1f75 100644 --- a/superset-frontend/src/SqlLab/App.jsx +++ b/superset-frontend/src/SqlLab/App.jsx @@ -30,6 +30,7 @@ import { FeatureFlag, } from 'src/featureFlags'; import setupExtensions from 'src/setup/setupExtensions'; +import { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; import getInitialState from './reducers/getInitialState'; import rootReducer from './reducers/index'; import { initEnhancer } from '../reduxUtils'; @@ -54,6 +55,11 @@ const bootstrapData = JSON.parse(appContainer.getAttribute('data-bootstrap')); initFeatureFlags(bootstrapData.common.feature_flags); const initialState = getInitialState(bootstrapData); + +if (!canUserAccessSqlLab(bootstrapData.user)) { + window.location.href = '/'; +} + const sqlLabPersistStateConfig = { paths: ['sqlLab'], config: { diff --git a/superset-frontend/src/dashboard/util/permissionUtils.test.ts b/superset-frontend/src/dashboard/util/permissionUtils.test.ts index a1aa8c1ca425..d22767e7b359 100644 --- a/superset-frontend/src/dashboard/util/permissionUtils.test.ts +++ b/superset-frontend/src/dashboard/util/permissionUtils.test.ts @@ -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', @@ -60,6 +64,14 @@ const owner: Owner = { username: ownerUser.username, }; +const sqlLabUser: UserWithPermissionsAndRoles = { + ...ownerUser, + roles: { + ...ownerUser.roles, + sql_lab: [], + }, +}; + const undefinedUser: UndefinedUser = {}; describe('canUserEditDashboard', () => { @@ -116,3 +128,19 @@ test('isUserAdmin returns false for undefined user', () => { 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 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); +}); diff --git a/superset-frontend/src/dashboard/util/permissionUtils.ts b/superset-frontend/src/dashboard/util/permissionUtils.ts index 4980480cee0b..c39925b42269 100644 --- a/superset-frontend/src/dashboard/util/permissionUtils.ts +++ b/superset-frontend/src/dashboard/util/permissionUtils.ts @@ -27,6 +27,7 @@ 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, @@ -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, + )) + ); +} diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx index 2dfa78363f87..b6adbd9ceec4 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx @@ -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'; @@ -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 = ( @@ -303,7 +308,7 @@ class DatasourceControl extends React.PureComponent { )} {t('Swap dataset')} - {datasource && ( + {datasource && canAccessSqlLab && ( {t('View in SQL Lab')} )} @@ -333,7 +338,9 @@ class DatasourceControl extends React.PureComponent { responsive /> - {t('View in SQL Lab')} + {canAccessSqlLab && ( + {t('View in SQL Lab')} + )} {t('Save as dataset')} ); From 6bd8ca75b0eb4f5d79c31fdc3b1a8bf347be6e1e Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 2 Jan 2023 09:00:57 +0000 Subject: [PATCH 02/12] add redirects --- superset-frontend/src/SqlLab/App.jsx | 31 ++++++++++--------- .../dashboard/util/permissionUtils.test.ts | 8 +++++ .../src/dashboard/util/permissionUtils.ts | 4 +-- .../src/views/CRUD/data/query/QueryList.tsx | 9 ++++++ .../CRUD/data/savedquery/SavedQueryList.tsx | 9 ++++++ 5 files changed, 45 insertions(+), 16 deletions(-) diff --git a/superset-frontend/src/SqlLab/App.jsx b/superset-frontend/src/SqlLab/App.jsx index 22e63cad1f75..7a2052aefdf5 100644 --- a/superset-frontend/src/SqlLab/App.jsx +++ b/superset-frontend/src/SqlLab/App.jsx @@ -56,10 +56,6 @@ initFeatureFlags(bootstrapData.common.feature_flags); const initialState = getInitialState(bootstrapData); -if (!canUserAccessSqlLab(bootstrapData.user)) { - window.location.href = '/'; -} - const sqlLabPersistStateConfig = { paths: ['sqlLab'], config: { @@ -142,15 +138,22 @@ if (sqlLabMenu) { } } -const Application = () => ( - - - - - - - - -); +const Application = () => { + if (!canUserAccessSqlLab(bootstrapData.user)) { + window.location.href = '/'; + return <>; + } + + return ( + + + + + + + + + ); +}; export default hot(Application); diff --git a/superset-frontend/src/dashboard/util/permissionUtils.test.ts b/superset-frontend/src/dashboard/util/permissionUtils.test.ts index d22767e7b359..d19b048769ec 100644 --- a/superset-frontend/src/dashboard/util/permissionUtils.test.ts +++ b/superset-frontend/src/dashboard/util/permissionUtils.test.ts @@ -121,6 +121,10 @@ 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); }); @@ -133,6 +137,10 @@ 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); }); diff --git a/superset-frontend/src/dashboard/util/permissionUtils.ts b/superset-frontend/src/dashboard/util/permissionUtils.ts index c39925b42269..3ea63976bfda 100644 --- a/superset-frontend/src/dashboard/util/permissionUtils.ts +++ b/superset-frontend/src/dashboard/util/permissionUtils.ts @@ -30,7 +30,7 @@ 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( @@ -53,7 +53,7 @@ export const canUserEditDashboard = ( findPermission('can_write', 'Dashboard', user.roles); export function canUserAccessSqlLab( - user: UserWithPermissionsAndRoles | UndefinedUser, + user?: UserWithPermissionsAndRoles | UndefinedUser, ) { return ( isUserAdmin(user) || diff --git a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx index dbe8e259dacc..ac16bb4593e6 100644 --- a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx @@ -49,6 +49,8 @@ import { DATETIME_WITH_TIME_ZONE, TIME_WITH_MS } from 'src/constants'; import { QueryObject, QueryObjectColumns } from 'src/views/CRUD/types'; import Icons from 'src/components/Icons'; +import getBootstrapData from 'src/utils/getBootstrapData'; +import { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; import QueryPreviewModal from './QueryPreviewModal'; const PAGE_SIZE = 25; @@ -86,6 +88,8 @@ const StyledPopoverItem = styled.div` color: ${({ theme }) => theme.colors.grayscale.dark2}; `; +const bootstrapData = getBootstrapData(); + function QueryList({ addDangerToast }: QueryListProps) { const { state: { loading, resourceCount: queryCount, resourceCollection: queries }, @@ -417,6 +421,11 @@ function QueryList({ addDangerToast }: QueryListProps) { [addDangerToast], ); + if (!canUserAccessSqlLab(bootstrapData.user)) { + window.location.href = '/'; + return <>; + } + return ( <> diff --git a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx index 7edd063cb82a..34ae89959b6b 100644 --- a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx @@ -50,6 +50,8 @@ 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 { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; +import getBootstrapData from 'src/utils/getBootstrapData'; import SavedQueryPreviewModal from './SavedQueryPreviewModal'; const PAGE_SIZE = 25; @@ -87,6 +89,8 @@ const StyledPopoverItem = styled.div` color: ${({ theme }) => theme.colors.grayscale.dark2}; `; +const bootstrapData = getBootstrapData(); + function SavedQueryList({ addDangerToast, addSuccessToast, @@ -472,6 +476,11 @@ function SavedQueryList({ [addDangerToast], ); + if (!canUserAccessSqlLab(bootstrapData.user)) { + window.location.href = '/'; + return <>; + } + return ( <> From 0aba55f24760085b83208b180ea5558eda65c4c7 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 2 Jan 2023 12:52:24 +0000 Subject: [PATCH 03/12] add test --- .../DatasourceControl.test.tsx | 51 ++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx index 231300711e27..27b7bc56844c 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx @@ -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', @@ -64,6 +64,7 @@ const createProps = () => ({ }, onChange: jest.fn(), onDatasourceSave: jest.fn(), + ...overrides, }); async function openAndSaveChanges(datasource: any) { @@ -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(); + + 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(); + + 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( From d9ac91c64fd234b682333335f9f7646070857b07 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 2 Jan 2023 13:01:31 +0000 Subject: [PATCH 04/12] replace bootstrap user with props user --- .../src/views/CRUD/data/query/QueryList.tsx | 9 ++++----- .../src/views/CRUD/data/savedquery/SavedQueryList.tsx | 11 ++++------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx index ac16bb4593e6..612030f4cc8c 100644 --- a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx @@ -49,8 +49,8 @@ import { DATETIME_WITH_TIME_ZONE, TIME_WITH_MS } from 'src/constants'; import { QueryObject, QueryObjectColumns } from 'src/views/CRUD/types'; import Icons from 'src/components/Icons'; -import getBootstrapData from 'src/utils/getBootstrapData'; import { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; +import { BootstrapUser } from 'src/types/bootstrapTypes'; import QueryPreviewModal from './QueryPreviewModal'; const PAGE_SIZE = 25; @@ -73,6 +73,7 @@ const StyledSyntaxHighlighter = styled(SyntaxHighlighter)` interface QueryListProps { addDangerToast: (msg: string, config?: any) => any; addSuccessToast: (msg: string, config?: any) => any; + user: BootstrapUser; } const StyledTableLabel = styled.div` @@ -88,9 +89,7 @@ const StyledPopoverItem = styled.div` color: ${({ theme }) => theme.colors.grayscale.dark2}; `; -const bootstrapData = getBootstrapData(); - -function QueryList({ addDangerToast }: QueryListProps) { +function QueryList({ addDangerToast, user }: QueryListProps) { const { state: { loading, resourceCount: queryCount, resourceCollection: queries }, fetchData, @@ -421,7 +420,7 @@ function QueryList({ addDangerToast }: QueryListProps) { [addDangerToast], ); - if (!canUserAccessSqlLab(bootstrapData.user)) { + if (!canUserAccessSqlLab(user)) { window.location.href = '/'; return <>; } diff --git a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx index 34ae89959b6b..be6a5749c85f 100644 --- a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx @@ -51,7 +51,7 @@ import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import ImportModelsModal from 'src/components/ImportModal/index'; import Icons from 'src/components/Icons'; import { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; -import getBootstrapData from 'src/utils/getBootstrapData'; +import { BootstrapUser } from 'src/types/bootstrapTypes'; import SavedQueryPreviewModal from './SavedQueryPreviewModal'; const PAGE_SIZE = 25; @@ -71,9 +71,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` @@ -89,11 +87,10 @@ const StyledPopoverItem = styled.div` color: ${({ theme }) => theme.colors.grayscale.dark2}; `; -const bootstrapData = getBootstrapData(); - function SavedQueryList({ addDangerToast, addSuccessToast, + user, }: SavedQueryListProps) { const { state: { @@ -476,7 +473,7 @@ function SavedQueryList({ [addDangerToast], ); - if (!canUserAccessSqlLab(bootstrapData.user)) { + if (!canUserAccessSqlLab(user)) { window.location.href = '/'; return <>; } From 03c73a27f6c941e7674e8ad77dea8425ce7a7836 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Tue, 3 Jan 2023 07:47:24 +0000 Subject: [PATCH 05/12] fix tests --- .../src/views/CRUD/data/query/QueryList.test.tsx | 10 +++++++++- .../CRUD/data/savedquery/SavedQueryList.test.jsx | 14 ++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx b/superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx index be28d7e2dfa8..9143719690cc 100644 --- a/superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx +++ b/superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx @@ -89,7 +89,15 @@ fetchMock.get('glob:*/api/v1/query/disting/status*', { }); describe('QueryList', () => { - const mockedProps = {}; + const mockedProps = { + user: { + username: 'user', + permissions: [], + roles: { + Admin: [], + }, + }, + }; const wrapper = mount( diff --git a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.test.jsx b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.test.jsx index 3acf44faadef..89f0d9927728 100644 --- a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.test.jsx +++ b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.test.jsx @@ -134,10 +134,20 @@ fetchMock.get(queriesDistinctEndpoint, { // Mock utils module jest.mock('src/views/CRUD/utils'); +const mockedProps = { + user: { + username: 'user', + permissions: [], + roles: { + Admin: [], + }, + }, +}; + describe('SavedQueryList', () => { const wrapper = mount( - + , ); @@ -246,7 +256,7 @@ describe('RTL', () => { const mounted = act(async () => { render( - + , { useRedux: true }, ); From aaf3a71227357f45589fa5fab925105107417d4a Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Tue, 3 Jan 2023 10:54:09 +0000 Subject: [PATCH 06/12] move redirect to backend --- superset-frontend/src/SqlLab/App.jsx | 29 +++++++------------ .../views/CRUD/data/query/QueryList.test.tsx | 10 +------ .../src/views/CRUD/data/query/QueryList.tsx | 6 ---- .../data/savedquery/SavedQueryList.test.jsx | 14 ++------- .../CRUD/data/savedquery/SavedQueryList.tsx | 6 ---- superset/views/core.py | 14 +++++++++ tests/integration_tests/sqllab_tests.py | 19 ++++++++++++ 7 files changed, 46 insertions(+), 52 deletions(-) diff --git a/superset-frontend/src/SqlLab/App.jsx b/superset-frontend/src/SqlLab/App.jsx index 7a2052aefdf5..812202eec20f 100644 --- a/superset-frontend/src/SqlLab/App.jsx +++ b/superset-frontend/src/SqlLab/App.jsx @@ -30,7 +30,6 @@ import { FeatureFlag, } from 'src/featureFlags'; import setupExtensions from 'src/setup/setupExtensions'; -import { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; import getInitialState from './reducers/getInitialState'; import rootReducer from './reducers/index'; import { initEnhancer } from '../reduxUtils'; @@ -55,7 +54,6 @@ const bootstrapData = JSON.parse(appContainer.getAttribute('data-bootstrap')); initFeatureFlags(bootstrapData.common.feature_flags); const initialState = getInitialState(bootstrapData); - const sqlLabPersistStateConfig = { paths: ['sqlLab'], config: { @@ -138,22 +136,15 @@ if (sqlLabMenu) { } } -const Application = () => { - if (!canUserAccessSqlLab(bootstrapData.user)) { - window.location.href = '/'; - return <>; - } - - return ( - - - - - - - - - ); -}; +const Application = () => ( + + + + + + + + +); export default hot(Application); diff --git a/superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx b/superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx index 9143719690cc..be28d7e2dfa8 100644 --- a/superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx +++ b/superset-frontend/src/views/CRUD/data/query/QueryList.test.tsx @@ -89,15 +89,7 @@ fetchMock.get('glob:*/api/v1/query/disting/status*', { }); describe('QueryList', () => { - const mockedProps = { - user: { - username: 'user', - permissions: [], - roles: { - Admin: [], - }, - }, - }; + const mockedProps = {}; const wrapper = mount( diff --git a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx index 612030f4cc8c..5f69ec80599d 100644 --- a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx @@ -49,7 +49,6 @@ import { DATETIME_WITH_TIME_ZONE, TIME_WITH_MS } from 'src/constants'; import { QueryObject, QueryObjectColumns } from 'src/views/CRUD/types'; import Icons from 'src/components/Icons'; -import { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; import { BootstrapUser } from 'src/types/bootstrapTypes'; import QueryPreviewModal from './QueryPreviewModal'; @@ -420,11 +419,6 @@ function QueryList({ addDangerToast, user }: QueryListProps) { [addDangerToast], ); - if (!canUserAccessSqlLab(user)) { - window.location.href = '/'; - return <>; - } - return ( <> diff --git a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.test.jsx b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.test.jsx index 89f0d9927728..3acf44faadef 100644 --- a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.test.jsx +++ b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.test.jsx @@ -134,20 +134,10 @@ fetchMock.get(queriesDistinctEndpoint, { // Mock utils module jest.mock('src/views/CRUD/utils'); -const mockedProps = { - user: { - username: 'user', - permissions: [], - roles: { - Admin: [], - }, - }, -}; - describe('SavedQueryList', () => { const wrapper = mount( - + , ); @@ -256,7 +246,7 @@ describe('RTL', () => { const mounted = act(async () => { render( - + , { useRedux: true }, ); diff --git a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx index be6a5749c85f..0e6e3b59170e 100644 --- a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx @@ -50,7 +50,6 @@ 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 { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; import { BootstrapUser } from 'src/types/bootstrapTypes'; import SavedQueryPreviewModal from './SavedQueryPreviewModal'; @@ -473,11 +472,6 @@ function SavedQueryList({ [addDangerToast], ); - if (!canUserAccessSqlLab(user)) { - window.location.href = '/'; - return <>; - } - return ( <> diff --git a/superset/views/core.py b/superset/views/core.py index 534f8f667d70..1fe42fada5eb 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2777,6 +2777,13 @@ def _get_sqllab_tabs(user_id: Optional[int]) -> Dict[str, Any]: @expose("/sqllab/", methods=["GET", "POST"]) def sqllab(self) -> FlaskResponse: """SQL Editor""" + if not ( + security_manager.is_admin() + or "sql_lab" in (role.name for role in security_manager.get_user_roles()) + ): + flash(__("You do not have access to SQL Lab"), "danger") + return redirect("/") + payload = { "defaultDbId": config["SQLLAB_DEFAULT_DBID"], "common": common_bootstrap_payload(g.user), @@ -2804,6 +2811,13 @@ def sqllab(self) -> FlaskResponse: @expose("/sqllab/history/", methods=["GET"]) @event_logger.log_this def sqllab_history(self) -> FlaskResponse: + if not ( + security_manager.is_admin() + or "sql_lab" in (role.name for role in security_manager.get_user_roles()) + ): + flash(__("You do not have access to SQL Lab"), "danger") + return redirect("/") + return super().render_app_template() @api diff --git a/tests/integration_tests/sqllab_tests.py b/tests/integration_tests/sqllab_tests.py index b1b0480d5663..ee568d0e0f04 100644 --- a/tests/integration_tests/sqllab_tests.py +++ b/tests/integration_tests/sqllab_tests.py @@ -257,6 +257,25 @@ def test_sql_json_has_access(self): db.session.commit() self.assertLess(0, len(data["data"])) + def test_sqllab_has_access(self): + self.create_user_with_roles("sqluser", ["Gamma", "sql_lab"]) + + self.login("sqluser") + for endpoint in ("/superset/sqllab/", "/superset/sqllab/history/"): + resp = self.client.get(endpoint) + self.assertEqual(200, resp.status_code) + + user = self.get_user("sqluser") + db.session.delete(user) + db.session.commit() + + def test_sqllab_no_access(self): + self.login("gamma") + for endpoint in ("/superset/sqllab/", "/superset/sqllab/history/"): + resp = self.client.get(endpoint) + # Redirects to the main page + self.assertEqual(302, resp.status_code) + def test_sql_json_schema_access(self): examples_db = get_example_database() db_backend = examples_db.backend From c12ae9ddbb158647a515e29f37ac834e2c81614d Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Tue, 3 Jan 2023 12:07:06 +0000 Subject: [PATCH 07/12] remove redundant changes --- superset-frontend/src/views/CRUD/data/query/QueryList.tsx | 4 +--- .../src/views/CRUD/data/savedquery/SavedQueryList.tsx | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx index 5f69ec80599d..dbe8e259dacc 100644 --- a/superset-frontend/src/views/CRUD/data/query/QueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/query/QueryList.tsx @@ -49,7 +49,6 @@ import { DATETIME_WITH_TIME_ZONE, TIME_WITH_MS } from 'src/constants'; import { QueryObject, QueryObjectColumns } from 'src/views/CRUD/types'; import Icons from 'src/components/Icons'; -import { BootstrapUser } from 'src/types/bootstrapTypes'; import QueryPreviewModal from './QueryPreviewModal'; const PAGE_SIZE = 25; @@ -72,7 +71,6 @@ const StyledSyntaxHighlighter = styled(SyntaxHighlighter)` interface QueryListProps { addDangerToast: (msg: string, config?: any) => any; addSuccessToast: (msg: string, config?: any) => any; - user: BootstrapUser; } const StyledTableLabel = styled.div` @@ -88,7 +86,7 @@ const StyledPopoverItem = styled.div` color: ${({ theme }) => theme.colors.grayscale.dark2}; `; -function QueryList({ addDangerToast, user }: QueryListProps) { +function QueryList({ addDangerToast }: QueryListProps) { const { state: { loading, resourceCount: queryCount, resourceCollection: queries }, fetchData, diff --git a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx index 0e6e3b59170e..ca2f26e61af0 100644 --- a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx @@ -89,7 +89,6 @@ const StyledPopoverItem = styled.div` function SavedQueryList({ addDangerToast, addSuccessToast, - user, }: SavedQueryListProps) { const { state: { From fd999626cae8b8aa0a390aebabdb12ad525ec8f4 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Tue, 3 Jan 2023 18:27:38 +0000 Subject: [PATCH 08/12] move access check to security manager --- superset/errors.py | 1 + superset/security/manager.py | 13 +++++++++++++ superset/views/core.py | 18 ++++++++---------- tests/integration_tests/sqllab_tests.py | 9 +++++---- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/superset/errors.py b/superset/errors.py index 8bc84a7425c4..e775c0d8aff8 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -65,6 +65,7 @@ class SupersetErrorType(str, Enum): QUERY_SECURITY_ACCESS_ERROR = "QUERY_SECURITY_ACCESS_ERROR" MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR" USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR" + SQL_LAB_SECURITY_ACCESS_ERROR = "SQL_LAB_SECURITY_ACCESS_ERROR" # Other errors BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR" diff --git a/superset/security/manager.py b/superset/security/manager.py index ea6040ced02a..947dacd1abad 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1964,6 +1964,19 @@ def raise_for_user_activity_access(user_id: int) -> None: ) ) + def raise_for_sql_lab_access(self) -> None: + if not ( + self.is_admin() + or "sql_lab" in (role.name for role in self.get_user_roles()) + ): + raise SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.SQL_LAB_SECURITY_ACCESS_ERROR, + message="You do not have access to SQL Lab", + level=ErrorLevel.ERROR, + ) + ) + def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: """ Raise an exception if the user cannot access the dashboard. diff --git a/superset/views/core.py b/superset/views/core.py index 1fe42fada5eb..a58dc882e9c1 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2777,11 +2777,10 @@ def _get_sqllab_tabs(user_id: Optional[int]) -> Dict[str, Any]: @expose("/sqllab/", methods=["GET", "POST"]) def sqllab(self) -> FlaskResponse: """SQL Editor""" - if not ( - security_manager.is_admin() - or "sql_lab" in (role.name for role in security_manager.get_user_roles()) - ): - flash(__("You do not have access to SQL Lab"), "danger") + try: + security_manager.raise_for_sql_lab_access() + except SupersetSecurityException as ex: + flash(ex.error.message, "danger") return redirect("/") payload = { @@ -2811,11 +2810,10 @@ def sqllab(self) -> FlaskResponse: @expose("/sqllab/history/", methods=["GET"]) @event_logger.log_this def sqllab_history(self) -> FlaskResponse: - if not ( - security_manager.is_admin() - or "sql_lab" in (role.name for role in security_manager.get_user_roles()) - ): - flash(__("You do not have access to SQL Lab"), "danger") + try: + security_manager.raise_for_sql_lab_access() + except SupersetSecurityException as ex: + flash(ex.error.message, "danger") return redirect("/") return super().render_app_template() diff --git a/tests/integration_tests/sqllab_tests.py b/tests/integration_tests/sqllab_tests.py index ee568d0e0f04..5549a2fe9c12 100644 --- a/tests/integration_tests/sqllab_tests.py +++ b/tests/integration_tests/sqllab_tests.py @@ -260,10 +260,11 @@ def test_sql_json_has_access(self): def test_sqllab_has_access(self): self.create_user_with_roles("sqluser", ["Gamma", "sql_lab"]) - self.login("sqluser") - for endpoint in ("/superset/sqllab/", "/superset/sqllab/history/"): - resp = self.client.get(endpoint) - self.assertEqual(200, resp.status_code) + for username in ("admin", "sqluser"): + self.login(username) + for endpoint in ("/superset/sqllab/", "/superset/sqllab/history/"): + resp = self.client.get(endpoint) + self.assertEqual(200, resp.status_code) user = self.get_user("sqluser") db.session.delete(user) From b91afd616d90d5bf0a60ceb60487d789077a659c Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 4 Jan 2023 14:59:03 +0200 Subject: [PATCH 09/12] improve sql lab role and conditionally show saved queries on welcome --- .../views/CRUD/welcome/ActivityTable.test.tsx | 4 +- .../src/views/CRUD/welcome/ActivityTable.tsx | 8 +-- .../src/views/CRUD/welcome/Welcome.tsx | 58 +++++++++++-------- superset/errors.py | 1 - superset/security/manager.py | 47 +++++++++------ superset/views/core.py | 12 ---- 6 files changed, 66 insertions(+), 64 deletions(-) diff --git a/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx b/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx index 53f637e4e6ea..4d1835496fa8 100644 --- a/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx +++ b/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx @@ -82,7 +82,7 @@ describe('ActivityTable', () => { activityData: mockData, setActiveChild: jest.fn(), user: { userId: '1' }, - loadedCount: 3, + doneFetching: true, }; let wrapper: ReactWrapper; @@ -127,7 +127,7 @@ describe('ActivityTable', () => { activityData: {}, setActiveChild: jest.fn(), user: { userId: '1' }, - loadedCount: 3, + doneFetching: true, }; const wrapper = mount( diff --git a/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx b/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx index 677f9e51bfa3..e886618d4235 100644 --- a/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx +++ b/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx @@ -74,7 +74,7 @@ interface ActivityProps { activeChild: string; setActiveChild: (arg0: string) => void; activityData: ActivityData; - loadedCount: number; + doneFetching: boolean; } const Styles = styled.div` @@ -128,7 +128,7 @@ export default function ActivityTable({ setActiveChild, activityData, user, - loadedCount, + doneFetching, }: ActivityProps) { const [editedObjs, setEditedObjs] = useState>(); const [loadingState, setLoadingState] = useState(false); @@ -200,9 +200,7 @@ export default function ActivityTable({ ); }); - const doneFetching = loadedCount < 3; - - if ((loadingState && !editedObjs) || doneFetching) { + if ((loadingState && !editedObjs) || !doneFetching) { return ; } return ( diff --git a/superset-frontend/src/views/CRUD/welcome/Welcome.tsx b/superset-frontend/src/views/CRUD/welcome/Welcome.tsx index 711098ff348c..583968a9dba0 100644 --- a/superset-frontend/src/views/CRUD/welcome/Welcome.tsx +++ b/superset-frontend/src/views/CRUD/welcome/Welcome.tsx @@ -47,6 +47,7 @@ import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; import { AntdSwitch } from 'src/components'; import getBootstrapData from 'src/utils/getBootstrapData'; import { TableTab } from 'src/views/CRUD/types'; +import { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils'; import { WelcomePageLastTab } from './types'; import ActivityTable from './ActivityTable'; import ChartTable from './ChartTable'; @@ -161,6 +162,7 @@ export const LoadingCards = ({ cover }: LoadingProps) => ( ); function Welcome({ user, addDangerToast }: WelcomeProps) { + const canAccessSqlLab = canUserAccessSqlLab(user); const userid = user.userId; const id = userid!.toString(); // confident that user is not a guest user const recent = `/superset/recent_activity/${user.userId}/?limit=6`; @@ -179,6 +181,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { null, ); const [loadedCount, setLoadedCount] = useState(0); + const doneFetching = loadedCount === (canAccessSqlLab ? 3 : 2); const collapseState = getItem(LocalStorageKeys.homepage_collapse_state, []); const [activeState, setActiveState] = useState>(collapseState); @@ -282,18 +285,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { setLoadedCount(loadedCount => loadedCount + 1); addDangerToast(t('There was an issue fetching your chart: %s', err)); }); - getUserOwnedObjects(id, 'saved_query', ownSavedQueryFilters) - .then(r => { - setQueryData(r); - setLoadedCount(loadedCount => loadedCount + 1); - }) - .catch((err: unknown) => { - setQueryData([]); - setLoadedCount(loadedCount => loadedCount + 1); - addDangerToast( - t('There was an issues fetching your saved queries: %s', err), - ); - }); + if (canAccessSqlLab) { + getUserOwnedObjects(id, 'saved_query', ownSavedQueryFilters) + .then(r => { + setQueryData(r); + setLoadedCount(loadedCount => loadedCount + 1); + }) + .catch((err: unknown) => { + setQueryData([]); + setLoadedCount(loadedCount => loadedCount + 1); + addDangerToast( + t('There was an issue fetching your saved queries: %s', err), + ); + }); + } }, [otherTabFilters]); const handleToggle = () => { @@ -323,6 +328,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { const isRecentActivityLoading = !activityData?.[TableTab.Other] && !activityData?.[TableTab.Viewed]; + return ( {WelcomeMessageExtension && } @@ -356,7 +362,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { activeChild={activeChild} setActiveChild={setActiveChild} activityData={activityData} - loadedCount={loadedCount} + doneFetching={doneFetching} /> ) : ( @@ -390,18 +396,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { /> )} - - {!queryData ? ( - - ) : ( - - )} - + {canAccessSqlLab && ( + + {!queryData ? ( + + ) : ( + + )} + + )} )} diff --git a/superset/errors.py b/superset/errors.py index e775c0d8aff8..8bc84a7425c4 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -65,7 +65,6 @@ class SupersetErrorType(str, Enum): QUERY_SECURITY_ACCESS_ERROR = "QUERY_SECURITY_ACCESS_ERROR" MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR" USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR" - SQL_LAB_SECURITY_ACCESS_ERROR = "SQL_LAB_SECURITY_ACCESS_ERROR" # Other errors BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR" diff --git a/superset/security/manager.py b/superset/security/manager.py index 947dacd1abad..ff0309d396bb 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -189,7 +189,6 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods } ADMIN_ONLY_PERMISSIONS = { - "can_sql_json", # TODO: move can_sql_json to sql_lab role "can_override_role_permissions", "can_sync_druid_source", "can_override_role_permissions", @@ -223,11 +222,11 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods ACCESSIBLE_PERMS = {"can_userinfo", "resetmypassword"} - SQLLAB_PERMISSION_VIEWS = { - ("can_csv", "Superset"), + SQLLAB_ONLY_PERMISSIONS = { + ("can_my_queries", "SqlLab"), ("can_read", "SavedQuery"), - ("can_read", "Database"), ("can_sql_json", "Superset"), + ("can_sqllab_history", "Superset"), ("can_sqllab_viz", "Superset"), ("can_sqllab_table_viz", "Superset"), ("can_sqllab", "Superset"), @@ -237,6 +236,12 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods ("menu_access", "Query Search"), } + SQLLAB_EXTRA_PERMISSION_VIEWS = { + ("can_csv", "Superset"), + ("can_read", "Superset"), + ("can_read", "Database"), + } + data_access_permissions = ( "database_access", "schema_access", @@ -908,7 +913,9 @@ def _is_alpha_pvm(self, pvm: PermissionView) -> bool: """ return not ( - self._is_user_defined_permission(pvm) or self._is_admin_only(pvm) + self._is_user_defined_permission(pvm) + or self._is_admin_only(pvm) + or self._is_sql_lab_only(pvm) ) or self._is_accessible_to_all(pvm) def _is_gamma_pvm(self, pvm: PermissionView) -> bool: @@ -924,8 +931,19 @@ def _is_gamma_pvm(self, pvm: PermissionView) -> bool: self._is_user_defined_permission(pvm) or self._is_admin_only(pvm) or self._is_alpha_only(pvm) + or self._is_sql_lab_only(pvm) ) or self._is_accessible_to_all(pvm) + def _is_sql_lab_only(self, pvm: PermissionView) -> bool: + """ + Return True if the FAB permission/view is only SQL Lab related, False + otherwise. + + :param pvm: The FAB permission/view + :returns: Whether the FAB object is SQL Lab related + """ + return (pvm.permission.name, pvm.view_menu.name) in self.SQLLAB_ONLY_PERMISSIONS + def _is_sql_lab_pvm(self, pvm: PermissionView) -> bool: """ Return True if the FAB permission/view is SQL Lab related, False @@ -934,7 +952,11 @@ def _is_sql_lab_pvm(self, pvm: PermissionView) -> bool: :param pvm: The FAB permission/view :returns: Whether the FAB object is SQL Lab related """ - return (pvm.permission.name, pvm.view_menu.name) in self.SQLLAB_PERMISSION_VIEWS + return ( + self._is_sql_lab_only(pvm) + or (pvm.permission.name, pvm.view_menu.name) + in self.SQLLAB_EXTRA_PERMISSION_VIEWS + ) def _is_granter_pvm( # pylint: disable=no-self-use self, pvm: PermissionView @@ -1964,19 +1986,6 @@ def raise_for_user_activity_access(user_id: int) -> None: ) ) - def raise_for_sql_lab_access(self) -> None: - if not ( - self.is_admin() - or "sql_lab" in (role.name for role in self.get_user_roles()) - ): - raise SupersetSecurityException( - SupersetError( - error_type=SupersetErrorType.SQL_LAB_SECURITY_ACCESS_ERROR, - message="You do not have access to SQL Lab", - level=ErrorLevel.ERROR, - ) - ) - def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: """ Raise an exception if the user cannot access the dashboard. diff --git a/superset/views/core.py b/superset/views/core.py index a58dc882e9c1..534f8f667d70 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2777,12 +2777,6 @@ def _get_sqllab_tabs(user_id: Optional[int]) -> Dict[str, Any]: @expose("/sqllab/", methods=["GET", "POST"]) def sqllab(self) -> FlaskResponse: """SQL Editor""" - try: - security_manager.raise_for_sql_lab_access() - except SupersetSecurityException as ex: - flash(ex.error.message, "danger") - return redirect("/") - payload = { "defaultDbId": config["SQLLAB_DEFAULT_DBID"], "common": common_bootstrap_payload(g.user), @@ -2810,12 +2804,6 @@ def sqllab(self) -> FlaskResponse: @expose("/sqllab/history/", methods=["GET"]) @event_logger.log_this def sqllab_history(self) -> FlaskResponse: - try: - security_manager.raise_for_sql_lab_access() - except SupersetSecurityException as ex: - flash(ex.error.message, "danger") - return redirect("/") - return super().render_app_template() @api From 4859955fd1380f201c7cca6e99a9c68b71455e71 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Thu, 5 Jan 2023 12:59:38 +0200 Subject: [PATCH 10/12] fix test --- .../src/views/CRUD/welcome/Welcome.test.tsx | 55 ++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/views/CRUD/welcome/Welcome.test.tsx b/superset-frontend/src/views/CRUD/welcome/Welcome.test.tsx index ce688f08fffd..5c693ab261ae 100644 --- a/superset-frontend/src/views/CRUD/welcome/Welcome.test.tsx +++ b/superset-frontend/src/views/CRUD/welcome/Welcome.test.tsx @@ -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 () => { @@ -122,6 +127,10 @@ describe('Welcome', () => { }); }); + afterAll(() => { + fetchMock.resetHistory(); + }); + it('renders', () => { expect(wrapper).toExist(); }); @@ -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( + + + , + ); + }); + }); + + 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( From 318529e4c53608e382f3ebb6d4023d08915a27f6 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Thu, 5 Jan 2023 17:20:16 +0200 Subject: [PATCH 11/12] fix python test --- .../integration_tests/queries/saved_queries/api_tests.py | 8 ++++---- tests/integration_tests/sqllab_tests.py | 8 ++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/integration_tests/queries/saved_queries/api_tests.py b/tests/integration_tests/queries/saved_queries/api_tests.py index 4a615a816f57..91843b0085dd 100644 --- a/tests/integration_tests/queries/saved_queries/api_tests.py +++ b/tests/integration_tests/queries/saved_queries/api_tests.py @@ -98,7 +98,7 @@ def create_saved_queries(self): self.insert_default_saved_query( label=f"label{SAVED_QUERIES_FIXTURE_COUNT}", schema=f"schema{SAVED_QUERIES_FIXTURE_COUNT}", - username="gamma", + username="gamma_sqllab", ) ) @@ -157,12 +157,12 @@ def test_get_list_saved_query_gamma(self): """ Saved Query API: Test get list saved query """ - gamma = self.get_user("gamma") + user = self.get_user("gamma_sqllab") saved_queries = ( - db.session.query(SavedQuery).filter(SavedQuery.created_by == gamma).all() + db.session.query(SavedQuery).filter(SavedQuery.created_by == user).all() ) - self.login(username="gamma") + self.login(username=user.username) uri = f"api/v1/saved_query/" rv = self.get_assert_metric(uri, "get_list") assert rv.status_code == 200 diff --git a/tests/integration_tests/sqllab_tests.py b/tests/integration_tests/sqllab_tests.py index 5549a2fe9c12..a33a541a6324 100644 --- a/tests/integration_tests/sqllab_tests.py +++ b/tests/integration_tests/sqllab_tests.py @@ -258,17 +258,13 @@ def test_sql_json_has_access(self): self.assertLess(0, len(data["data"])) def test_sqllab_has_access(self): - self.create_user_with_roles("sqluser", ["Gamma", "sql_lab"]) - - for username in ("admin", "sqluser"): + for username in ("admin", "gamma_sqllab"): self.login(username) for endpoint in ("/superset/sqllab/", "/superset/sqllab/history/"): resp = self.client.get(endpoint) self.assertEqual(200, resp.status_code) - user = self.get_user("sqluser") - db.session.delete(user) - db.session.commit() + self.logout() def test_sqllab_no_access(self): self.login("gamma") From e317744d3ec36cc4e4a27c8a623ac74664bce5ba Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Fri, 6 Jan 2023 09:26:58 +0200 Subject: [PATCH 12/12] refactor welcome components --- .../views/CRUD/welcome/ActivityTable.test.tsx | 4 +- .../src/views/CRUD/welcome/ActivityTable.tsx | 27 ++++--- .../src/views/CRUD/welcome/Welcome.tsx | 71 ++++++++++--------- 3 files changed, 51 insertions(+), 51 deletions(-) diff --git a/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx b/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx index 4d1835496fa8..66d521f362b0 100644 --- a/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx +++ b/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx @@ -82,7 +82,7 @@ describe('ActivityTable', () => { activityData: mockData, setActiveChild: jest.fn(), user: { userId: '1' }, - doneFetching: true, + isFetchingActivityData: false, }; let wrapper: ReactWrapper; @@ -127,7 +127,7 @@ describe('ActivityTable', () => { activityData: {}, setActiveChild: jest.fn(), user: { userId: '1' }, - doneFetching: true, + isFetchingActivityData: false, }; const wrapper = mount( diff --git a/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx b/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx index e886618d4235..d6dc3858a943 100644 --- a/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx +++ b/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx @@ -74,7 +74,7 @@ interface ActivityProps { activeChild: string; setActiveChild: (arg0: string) => void; activityData: ActivityData; - doneFetching: boolean; + isFetchingActivityData: boolean; } const Styles = styled.div` @@ -128,22 +128,21 @@ export default function ActivityTable({ setActiveChild, activityData, user, - doneFetching, + isFetchingActivityData, }: ActivityProps) { - const [editedObjs, setEditedObjs] = useState>(); - const [loadingState, setLoadingState] = useState(false); + const [editedCards, setEditedCards] = useState(); + 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]); @@ -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); @@ -200,16 +199,14 @@ export default function ActivityTable({ ); }); - if ((loadingState && !editedObjs) || !doneFetching) { + if ((isFetchingEditedCards && !editedCards) || isFetchingActivityData) { return ; } return ( {activityData[activeChild]?.length > 0 || - (activeChild === TableTab.Edited && - editedObjs && - editedObjs.length > 0) ? ( + (activeChild === TableTab.Edited && editedCards?.length) ? ( {renderActivity()} diff --git a/superset-frontend/src/views/CRUD/welcome/Welcome.tsx b/superset-frontend/src/views/CRUD/welcome/Welcome.tsx index 583968a9dba0..1617c8b6fde8 100644 --- a/superset-frontend/src/views/CRUD/welcome/Welcome.tsx +++ b/superset-frontend/src/views/CRUD/welcome/Welcome.tsx @@ -180,8 +180,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { const [dashboardData, setDashboardData] = useState | null>( null, ); - const [loadedCount, setLoadedCount] = useState(0); - const doneFetching = loadedCount === (canAccessSqlLab ? 3 : 2); + const [isFetchingActivityData, setIsFetchingActivityData] = useState(true); const collapseState = getItem(LocalStorageKeys.homepage_collapse_state, []); const [activeState, setActiveState] = useState>(collapseState); @@ -263,42 +262,46 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { value: `${id}`, }, ]; - getUserOwnedObjects(id, 'dashboard') - .then(r => { - setDashboardData(r); - setLoadedCount(loadedCount => loadedCount + 1); - }) - .catch((err: unknown) => { - setDashboardData([]); - setLoadedCount(loadedCount => loadedCount + 1); - addDangerToast( - t('There was an issue fetching your dashboards: %s', err), - ); - }); - getUserOwnedObjects(id, 'chart') - .then(r => { - setChartData(r); - setLoadedCount(loadedCount => loadedCount + 1); - }) - .catch((err: unknown) => { - setChartData([]); - setLoadedCount(loadedCount => loadedCount + 1); - addDangerToast(t('There was an issue fetching your chart: %s', err)); - }); - if (canAccessSqlLab) { - getUserOwnedObjects(id, 'saved_query', ownSavedQueryFilters) + Promise.all([ + getUserOwnedObjects(id, 'dashboard') .then(r => { - setQueryData(r); - setLoadedCount(loadedCount => loadedCount + 1); + setDashboardData(r); + return Promise.resolve(); }) .catch((err: unknown) => { - setQueryData([]); - setLoadedCount(loadedCount => loadedCount + 1); + setDashboardData([]); addDangerToast( - t('There was an issue fetching your saved queries: %s', err), + t('There was an issue fetching your dashboards: %s', err), ); - }); - } + return Promise.resolve(); + }), + getUserOwnedObjects(id, 'chart') + .then(r => { + setChartData(r); + return Promise.resolve(); + }) + .catch((err: unknown) => { + setChartData([]); + addDangerToast(t('There was an issue fetching your chart: %s', err)); + return Promise.resolve(); + }), + canAccessSqlLab + ? getUserOwnedObjects(id, 'saved_query', ownSavedQueryFilters) + .then(r => { + setQueryData(r); + return Promise.resolve(); + }) + .catch((err: unknown) => { + setQueryData([]); + addDangerToast( + t('There was an issue fetching your saved queries: %s', err), + ); + return Promise.resolve(); + }) + : Promise.resolve(), + ]).then(() => { + setIsFetchingActivityData(false); + }); }, [otherTabFilters]); const handleToggle = () => { @@ -362,7 +365,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { activeChild={activeChild} setActiveChild={setActiveChild} activityData={activityData} - doneFetching={doneFetching} + isFetchingActivityData={isFetchingActivityData} /> ) : (