fix: Alpha should not be able to edit datasets that they don't own (a…

…pache#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
hve-labs · May 11, 2022 · eb9a71c · eb9a71c
1 parent 70b1c26
commit eb9a71c
Show file tree

Hide file tree

Showing 10 changed files with 127 additions and 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
@@ -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
@@ -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
@@ -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/...set-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.jsx b/...set-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/...set-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx b/...set-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
@@ -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 = (
       <Menu onClick={this.handleMenuItemClick}>
         {this.props.isEditable && (
-          <Menu.Item key={EDIT_DATASET} data-test="edit-dataset">
-            {t('Edit dataset')}
+          <Menu.Item
+            key={EDIT_DATASET}
+            data-test="edit-dataset"
+            disabled={!allowEdit}
+          >
+            {!allowEdit ? (
+              <Tooltip
+                title={t(
+                  'You must be a dataset owner in order to edit. Please reach out to a dataset owner to request modifications or edit access.',
+                )}
+              >
+                {editText}
+              </Tooltip>
+            ) : (
+              editText
+            )}
           </Menu.Item>
         )}
         <Menu.Item key={CHANGE_DATASET}>{t('Change dataset')}</Menu.Item>

diff --git 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
@@ -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<DatasetListProps> = ({
       },
       {
         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<DatasetListProps> = ({
               {canEdit && (
                 <Tooltip
                   id="edit-action-tooltip"
-                  title={t('Edit')}
-                  placement="bottom"
+                  title={
+                    allowEdit
+                      ? t('Edit')
+                      : t(
+                          'You must be a dataset owner in order to edit. Please reach out to a dataset owner to request modifications or edit access.',
+                        )
+                  }
+                  placement="bottomRight"
                 >
                   <span
                     role="button"
                     tabIndex={0}
-                    className="action-button"
-                    onClick={handleEdit}
+                    className={allowEdit ? 'action-button' : 'disabled'}
+                    onClick={allowEdit ? handleEdit : undefined}
                   >
                     <Icons.EditAlt />
                   </span>

diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py
@@ -27,7 +27,7 @@
 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 @@ def save(self) -> FlaskResponse:
 
         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
@@ -278,6 +278,7 @@ def test_save(self):
 
         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 @@ def save_datasource_from_dict(self, datasource_post):
     @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 @@ def test_change_database(self):
 
     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 @@ def test_save_duplicate_key(self):
 
     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}/"