Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Alpha should not be able to edit datasets that they don't own #19854

Merged
merged 16 commits into from
Apr 29, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -196,11 +197,22 @@ class DatasourceControl extends React.PureComponent {
}

const isSqlSupported = datasource.type === 'table';
const appContainer = document.getElementById('app');
const { user } = JSON.parse(
appContainer?.getAttribute('data-bootstrap') || '{}',
);
const allowEdit =
datasource.owners.map(o => o.id).includes(user.userId) ||
isUserAdmin(user);

const datasourceMenu = (
<Menu onClick={this.handleMenuItemClick}>
{this.props.isEditable && (
<Menu.Item key={EDIT_DATASET} data-test="edit-dataset">
<Menu.Item
key={EDIT_DATASET}
data-test="edit-dataset"
disabled={!allowEdit}
>
{t('Edit dataset')}
</Menu.Item>
)}
Expand Down
40 changes: 36 additions & 4 deletions superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 = {
Expand Down Expand Up @@ -344,6 +365,11 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
},
{
Cell: ({ row: { original } }: any) => {
// Verify owner or isAdmin
const allowEdit =
original.owners.map((o: any) => o.id).includes(user.userId) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

add Owners to for loop

isUserAdmin(user);

const handleEdit = () => openDatasetEditModal(original);
const handleDelete = () => openDatasetDeleteModal(original);
const handleExport = () => handleBulkDatasetExport([original]);
Expand Down Expand Up @@ -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>
Expand Down
34 changes: 10 additions & 24 deletions superset/views/datasource/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
23 changes: 23 additions & 0 deletions tests/integration_tests/datasource_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -299,11 +299,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
Expand All @@ -316,6 +319,26 @@ def test_change_database(self):

self.delete_fake_db()

@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_edit_alpha_not_owner(self):
self.login(username="alpha")
alpha_user = self.get_user("alpha")

tbl = self.get_table(name="birth_names")
tbl_id = tbl.id
datasource_post = get_datasource_post()
datasource_post["id"] = tbl_id
datasource_post["owners"] = [alpha_user.id]

new_db = self.create_fake_db()
datasource_post["database"]["id"] = new_db.id

data = dict(data=json.dumps(datasource_post))
resp = self.get_json_resp("/datasource/save/", data, raise_on_error=False)
self.assertIn("Changing this dataset is forbidden", resp["error"])

self.delete_fake_db()

def test_save_duplicate_key(self):
self.login(username="admin")
tbl_id = self.get_table(name="birth_names").id
Expand Down