From 8b15b68979bf033979fe7014ef2730095ae85120 Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Fri, 29 Apr 2022 14:21:05 -0700 Subject: [PATCH] fix: Alpha should not be able to edit datasets that they don't own (#19854) * fix api for checking owners * fix styles for disabling * fix styles for disabling * fix lint * fix lint * add owners key * plzz * remove * update test * add tooltip * add type * fix test * fix user reference * lit * fix test * work --- .../src/shared-controls/index.tsx | 6 +++ .../DatasourcePanel/DatasourcePanel.test.tsx | 17 ++++++++ .../components/DatasourcePanel/index.tsx | 2 + .../DatasourceControl.test.jsx | 13 ++++++ .../DatasourceControl.test.tsx | 11 +++++ .../controls/DatasourceControl/index.jsx | 25 +++++++++++- .../CRUD/data/dataset/DatasetList.test.jsx | 1 + .../views/CRUD/data/dataset/DatasetList.tsx | 40 +++++++++++++++++-- superset/views/datasource/views.py | 34 +++++----------- tests/integration_tests/datasource_tests.py | 8 ++++ 10 files changed, 127 insertions(+), 30 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx index 382fcb5bb3..713fbf1c2d 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx @@ -91,6 +91,11 @@ export const PRIMARY_COLOR = { r: 0, g: 122, b: 135, a: 1 }; const ROW_LIMIT_OPTIONS = [10, 50, 100, 250, 500, 1000, 5000, 10000, 50000]; const SERIES_LIMITS = [5, 10, 25, 50, 100, 500]; +const appContainer = document.getElementById('app'); +const { user } = JSON.parse( + appContainer?.getAttribute('data-bootstrap') || '{}', +); + type Control = { savedMetrics?: Metric[] | null; default?: unknown; @@ -167,6 +172,7 @@ const datasourceControl: SharedControlConfig<'DatasourceControl'> = { mapStateToProps: ({ datasource, form_data }) => ({ datasource, form_data, + user, }), }; diff --git a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanel.test.tsx b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanel.test.tsx index 0f11f48dd8..99c596b80e 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanel.test.tsx +++ b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanel.test.tsx @@ -47,7 +47,22 @@ const datasource = { main_dttm_col: 'None', datasource_name: 'table1', description: 'desc', + owners: [{ username: 'admin', userId: 1 }], }; + +const mockUser = { + createdOn: '2021-04-27T18:12:38.952304', + email: 'admin', + firstName: 'admin', + isActive: true, + lastName: 'admin', + permissions: {}, + roles: { Admin: Array(173) }, + userId: 1, + username: 'admin', + isAnonymous: false, +}; + const props: DatasourcePanelProps = { datasource, controls: { @@ -57,6 +72,7 @@ const props: DatasourcePanelProps = { type: DatasourceControl, label: 'hello', datasource, + user: mockUser, }, }, actions: { @@ -154,6 +170,7 @@ test('should render a warning', async () => { datasource: { ...props.controls.datasource, datasource: deprecatedDatasource, + user: mockUser, }, }, }), diff --git a/superset-frontend/src/explore/components/DatasourcePanel/index.tsx b/superset-frontend/src/explore/components/DatasourcePanel/index.tsx index c38c1b59ae..8bd39aa52f 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/index.tsx +++ b/superset-frontend/src/explore/components/DatasourcePanel/index.tsx @@ -31,12 +31,14 @@ import { FAST_DEBOUNCE } from 'src/constants'; import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; import { ExploreActions } from 'src/explore/actions/exploreActions'; import Control from 'src/explore/components/Control'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; import DatasourcePanelDragOption from './DatasourcePanelDragOption'; import { DndItemType } from '../DndItemType'; import { StyledColumnOption, StyledMetricOption } from '../optionRenderers'; interface DatasourceControl extends ControlConfig { datasource?: DatasourceMeta; + user: UserWithPermissionsAndRoles; } export interface Props { diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.jsx index 73c2777663..41a40c0bc4 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.jsx @@ -41,6 +41,7 @@ const defaultProps = { id: 1, columns: [], metrics: [], + owners: [{ username: 'admin', userId: 1 }], database: { backend: 'mysql', name: 'main', @@ -51,6 +52,17 @@ const defaultProps = { setDatasource: sinon.spy(), }, onChange: sinon.spy(), + user: { + createdOn: '2021-04-27T18:12:38.952304', + email: 'admin', + firstName: 'admin', + isActive: true, + lastName: 'admin', + permissions: {}, + roles: { Admin: Array(173) }, + userId: 1, + username: 'admin', + }, }; describe('DatasourceControl', () => { @@ -107,6 +119,7 @@ describe('DatasourceControl', () => { id: 1, columns: [], metrics: [], + owners: [{ username: 'admin', userId: 1 }], database: { backend: 'druid', name: 'main', 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 f4f66ef43e..4811743489 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx @@ -48,6 +48,17 @@ const createProps = () => ({ name: 'datasource', actions: {}, isEditable: true, + user: { + createdOn: '2021-04-27T18:12:38.952304', + email: 'admin', + firstName: 'admin', + isActive: true, + lastName: 'admin', + permissions: {}, + roles: { Admin: Array(173) }, + userId: 1, + username: 'admin', + }, onChange: jest.fn(), onDatasourceSave: jest.fn(), }); diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx index b011901eb0..73aa5e4d91 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx @@ -35,6 +35,7 @@ import Button from 'src/components/Button'; import ErrorAlert from 'src/components/ErrorMessage/ErrorAlert'; import WarningIconWithTooltip from 'src/components/WarningIconWithTooltip'; import { URL_PARAMS } from 'src/constants'; +import { isUserAdmin } from 'src/dashboard/util/findPermission'; const propTypes = { actions: PropTypes.object.isRequired, @@ -196,12 +197,32 @@ class DatasourceControl extends React.PureComponent { } const isSqlSupported = datasource.type === 'table'; + const { user } = this.props; + const allowEdit = + datasource.owners.map(o => o.id).includes(user.userId) || + isUserAdmin(user); + + const editText = t('Edit dataset'); const datasourceMenu = ( {this.props.isEditable && ( - - {t('Edit dataset')} + + {!allowEdit ? ( + + {editText} + + ) : ( + editText + )} )} {t('Change dataset')} diff --git a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.test.jsx b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.test.jsx index ece974e41e..6c416ae55e 100644 --- a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.test.jsx +++ b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.test.jsx @@ -55,6 +55,7 @@ const mockdatasets = [...new Array(3)].map((_, i) => ({ id: i, schema: `schema ${i}`, table_name: `coolest table ${i}`, + owners: [{ username: 'admin', userId: 1 }], })); const mockUser = { diff --git a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx index c99a69e1c1..563a3bce1f 100644 --- a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx +++ b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx @@ -56,7 +56,9 @@ import InfoTooltip from 'src/components/InfoTooltip'; import ImportModelsModal from 'src/components/ImportModal/index'; import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import WarningIconWithTooltip from 'src/components/WarningIconWithTooltip'; +import { isUserAdmin } from 'src/dashboard/util/findPermission'; import AddDatasetModal from './AddDatasetModal'; + import { PAGE_SIZE, SORT_BY, @@ -75,6 +77,25 @@ const FlexRowContainer = styled.div` const Actions = styled.div` color: ${({ theme }) => theme.colors.grayscale.base}; + + .disabled { + svg, + i { + &:hover { + path { + fill: ${({ theme }) => theme.colors.grayscale.light1}; + } + } + } + color: ${({ theme }) => theme.colors.grayscale.light1}; + .ant-menu-item:hover { + color: ${({ theme }) => theme.colors.grayscale.light1}; + cursor: default; + } + &::after { + color: ${({ theme }) => theme.colors.grayscale.light1}; + } + } `; type Dataset = { @@ -344,6 +365,11 @@ const DatasetList: FunctionComponent = ({ }, { Cell: ({ row: { original } }: any) => { + // Verify owner or isAdmin + const allowEdit = + original.owners.map((o: Owner) => o.id).includes(user.userId) || + isUserAdmin(user); + const handleEdit = () => openDatasetEditModal(original); const handleDelete = () => openDatasetDeleteModal(original); const handleExport = () => handleBulkDatasetExport([original]); @@ -387,14 +413,20 @@ const DatasetList: FunctionComponent = ({ {canEdit && ( diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py index 791f7c500e..560c12d6f1 100644 --- a/superset/views/datasource/views.py +++ b/superset/views/datasource/views.py @@ -27,7 +27,7 @@ from marshmallow import ValidationError from sqlalchemy.exc import NoSuchTableError from sqlalchemy.orm.exc import NoResultFound -from superset import app, db, event_logger +from superset import db, event_logger from superset.commands.utils import populate_owners from superset.connectors.connector_registry import ConnectorRegistry from superset.connectors.sqla.utils import get_physical_table_metadata @@ -81,29 +81,15 @@ class Datasource(BaseSupersetView): if "owners" in datasource_dict and orm_datasource.owner_class is not None: # Check ownership - if app.config.get("OLD_API_CHECK_DATASET_OWNERSHIP"): - # mimic the behavior of the new dataset command that - # checks ownership and ensures that non-admins aren't locked out - # of the object - try: - check_ownership(orm_datasource) - except SupersetSecurityException as ex: - raise DatasetForbiddenError() from ex - user = security_manager.get_user_by_id(g.user.id) - datasource_dict["owners"] = populate_owners( - user, datasource_dict["owners"], default_to_user=False - ) - else: - # legacy behavior - datasource_dict["owners"] = ( - db.session.query(orm_datasource.owner_class) - .filter( - orm_datasource.owner_class.id.in_( - datasource_dict["owners"] or [] - ) - ) - .all() - ) + try: + check_ownership(orm_datasource) + except SupersetSecurityException as ex: + raise DatasetForbiddenError() from ex + + user = security_manager.get_user_by_id(g.user.id) + datasource_dict["owners"] = populate_owners( + user, datasource_dict["owners"], default_to_user=False + ) duplicates = [ name diff --git a/tests/integration_tests/datasource_tests.py b/tests/integration_tests/datasource_tests.py index 1428a20c48..6d46afa0a9 100644 --- a/tests/integration_tests/datasource_tests.py +++ b/tests/integration_tests/datasource_tests.py @@ -278,6 +278,7 @@ class TestDatasource(SupersetTestCase): datasource_post = get_datasource_post() datasource_post["id"] = tbl_id + datasource_post["owners"] = [1] data = dict(data=json.dumps(datasource_post)) resp = self.get_json_resp("/datasource/save/", data) for k in datasource_post: @@ -299,11 +300,14 @@ class TestDatasource(SupersetTestCase): @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_change_database(self): self.login(username="admin") + admin_user = self.get_user("admin") + tbl = self.get_table(name="birth_names") tbl_id = tbl.id db_id = tbl.database_id datasource_post = get_datasource_post() datasource_post["id"] = tbl_id + datasource_post["owners"] = [admin_user.id] new_db = self.create_fake_db() datasource_post["database"]["id"] = new_db.id @@ -318,10 +322,12 @@ class TestDatasource(SupersetTestCase): def test_save_duplicate_key(self): self.login(username="admin") + admin_user = self.get_user("admin") tbl_id = self.get_table(name="birth_names").id datasource_post = get_datasource_post() datasource_post["id"] = tbl_id + datasource_post["owners"] = [admin_user.id] datasource_post["columns"].extend( [ { @@ -346,10 +352,12 @@ class TestDatasource(SupersetTestCase): def test_get_datasource(self): self.login(username="admin") + admin_user = self.get_user("admin") tbl = self.get_table(name="birth_names") datasource_post = get_datasource_post() datasource_post["id"] = tbl.id + datasource_post["owners"] = [admin_user.id] data = dict(data=json.dumps(datasource_post)) self.get_json_resp("/datasource/save/", data) url = f"/datasource/get/{tbl.type}/{tbl.id}/"