From d6f9c48aa1ed3f02f1d97f8fe3447798d2414544 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Fri, 13 Aug 2021 12:42:48 +0300 Subject: [PATCH] feat(dao): admin can remove self from object owners (#15149) --- superset/charts/commands/create.py | 8 ++-- superset/charts/commands/update.py | 8 ++-- superset/commands/base.py | 41 ++++++++++++++++++- superset/commands/utils.py | 20 ++++++--- superset/dashboards/commands/create.py | 8 ++-- superset/dashboards/commands/update.py | 8 ++-- superset/datasets/commands/create.py | 7 ++-- superset/datasets/commands/update.py | 7 ++-- superset/reports/commands/create.py | 6 +-- superset/reports/commands/update.py | 6 +-- tests/integration_tests/charts/api_tests.py | 31 +++++++++++--- .../integration_tests/dashboards/api_tests.py | 39 ++++++++++++++---- 12 files changed, 138 insertions(+), 51 deletions(-) diff --git a/superset/charts/commands/create.py b/superset/charts/commands/create.py index ff127cb517..a393c68111 100644 --- a/superset/charts/commands/create.py +++ b/superset/charts/commands/create.py @@ -28,15 +28,15 @@ from superset.charts.commands.exceptions import ( DashboardsNotFoundValidationError, ) from superset.charts.dao import ChartDAO -from superset.commands.base import BaseCommand -from superset.commands.utils import get_datasource_by_id, populate_owners +from superset.commands.base import BaseCommand, CreateMixin +from superset.commands.utils import get_datasource_by_id from superset.dao.exceptions import DAOCreateFailedError from superset.dashboards.dao import DashboardDAO logger = logging.getLogger(__name__) -class CreateChartCommand(BaseCommand): +class CreateChartCommand(CreateMixin, BaseCommand): def __init__(self, user: User, data: Dict[str, Any]): self._actor = user self._properties = data.copy() @@ -73,7 +73,7 @@ class CreateChartCommand(BaseCommand): self._properties["dashboards"] = dashboards try: - owners = populate_owners(self._actor, owner_ids) + owners = self.populate_owners(self._actor, owner_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/charts/commands/update.py b/superset/charts/commands/update.py index 7ac3c60b10..e5fe64384f 100644 --- a/superset/charts/commands/update.py +++ b/superset/charts/commands/update.py @@ -31,8 +31,8 @@ from superset.charts.commands.exceptions import ( DatasourceTypeUpdateRequiredValidationError, ) from superset.charts.dao import ChartDAO -from superset.commands.base import BaseCommand -from superset.commands.utils import get_datasource_by_id, populate_owners +from superset.commands.base import BaseCommand, UpdateMixin +from superset.commands.utils import get_datasource_by_id from superset.dao.exceptions import DAOUpdateFailedError from superset.dashboards.dao import DashboardDAO from superset.exceptions import SupersetSecurityException @@ -42,7 +42,7 @@ from superset.views.base import check_ownership logger = logging.getLogger(__name__) -class UpdateChartCommand(BaseCommand): +class UpdateChartCommand(UpdateMixin, BaseCommand): def __init__(self, user: User, model_id: int, data: Dict[str, Any]): self._actor = user self._model_id = model_id @@ -100,7 +100,7 @@ class UpdateChartCommand(BaseCommand): # Validate/Populate owner try: - owners = populate_owners(self._actor, owner_ids) + owners = self.populate_owners(self._actor, owner_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/commands/base.py b/superset/commands/base.py index 0bcf3a7bee..2db12bb6d2 100644 --- a/superset/commands/base.py +++ b/superset/commands/base.py @@ -15,7 +15,11 @@ # specific language governing permissions and limitations # under the License. from abc import ABC, abstractmethod -from typing import Any +from typing import Any, List, Optional + +from flask_appbuilder.security.sqla.models import User + +from superset.commands.utils import populate_owners class BaseCommand(ABC): @@ -37,3 +41,38 @@ class BaseCommand(ABC): Will raise exception if validation fails :raises: CommandException """ + + +class CreateMixin: + @staticmethod + def populate_owners( + user: User, owner_ids: Optional[List[int]] = None + ) -> List[User]: + """ + Populate list of owners, defaulting to the current user if `owner_ids` is + undefined or empty. If current user is missing in `owner_ids`, current user + is added unless belonging to the Admin role. + + :param user: current user + :param owner_ids: list of owners by id's + :raises OwnersNotFoundValidationError: if at least one owner can't be resolved + :returns: Final list of owners + """ + return populate_owners(user, owner_ids, default_to_user=True) + + +class UpdateMixin: + @staticmethod + def populate_owners( + user: User, owner_ids: Optional[List[int]] = None + ) -> List[User]: + """ + Populate list of owners. If current user is missing in `owner_ids`, current user + is added unless belonging to the Admin role. + + :param user: current user + :param owner_ids: list of owners by id's + :raises OwnersNotFoundValidationError: if at least one owner can't be resolved + :returns: Final list of owners + """ + return populate_owners(user, owner_ids, default_to_user=False) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index b39cf6434f..0cecb2e15d 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -29,17 +29,25 @@ from superset.datasets.commands.exceptions import DatasetNotFoundError from superset.extensions import db, security_manager -def populate_owners(user: User, owner_ids: Optional[List[int]] = None) -> List[User]: +def populate_owners( + user: User, owner_ids: Optional[List[int]], default_to_user: bool, +) -> List[User]: """ Helper function for commands, will fetch all users from owners id's - Can raise ValidationError - :param user: The current user - :param owner_ids: A List of owners by id's + :param user: current user + :param owner_ids: list of owners by id's + :param default_to_user: make user the owner if `owner_ids` is None or empty + :raises OwnersNotFoundValidationError: if at least one owner id can't be resolved + :returns: Final list of owners """ + owner_ids = owner_ids or [] owners = list() - if not owner_ids: + if not owner_ids and default_to_user: return [user] - if user.id not in owner_ids: + if user.id not in owner_ids and "admin" not in [ + role.name.lower() for role in user.roles + ]: + # make sure non-admins can't remove themselves as owner by mistake owners.append(user) for owner_id in owner_ids: owner = security_manager.get_user_by_id(owner_id) diff --git a/superset/dashboards/commands/create.py b/superset/dashboards/commands/create.py index e351d675af..de136d20e1 100644 --- a/superset/dashboards/commands/create.py +++ b/superset/dashboards/commands/create.py @@ -21,8 +21,8 @@ from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User from marshmallow import ValidationError -from superset.commands.base import BaseCommand -from superset.commands.utils import populate_owners, populate_roles +from superset.commands.base import BaseCommand, CreateMixin +from superset.commands.utils import populate_roles from superset.dao.exceptions import DAOCreateFailedError from superset.dashboards.commands.exceptions import ( DashboardCreateFailedError, @@ -34,7 +34,7 @@ from superset.dashboards.dao import DashboardDAO logger = logging.getLogger(__name__) -class CreateDashboardCommand(BaseCommand): +class CreateDashboardCommand(CreateMixin, BaseCommand): def __init__(self, user: User, data: Dict[str, Any]): self._actor = user self._properties = data.copy() @@ -60,7 +60,7 @@ class CreateDashboardCommand(BaseCommand): exceptions.append(DashboardSlugExistsValidationError()) try: - owners = populate_owners(self._actor, owner_ids) + owners = self.populate_owners(self._actor, owner_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/dashboards/commands/update.py b/superset/dashboards/commands/update.py index 9c885c635a..877c712fea 100644 --- a/superset/dashboards/commands/update.py +++ b/superset/dashboards/commands/update.py @@ -21,8 +21,8 @@ from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User from marshmallow import ValidationError -from superset.commands.base import BaseCommand -from superset.commands.utils import populate_owners, populate_roles +from superset.commands.base import BaseCommand, UpdateMixin +from superset.commands.utils import populate_roles from superset.dao.exceptions import DAOUpdateFailedError from superset.dashboards.commands.exceptions import ( DashboardForbiddenError, @@ -39,7 +39,7 @@ from superset.views.base import check_ownership logger = logging.getLogger(__name__) -class UpdateDashboardCommand(BaseCommand): +class UpdateDashboardCommand(UpdateMixin, BaseCommand): def __init__(self, user: User, model_id: int, data: Dict[str, Any]): self._actor = user self._model_id = model_id @@ -80,7 +80,7 @@ class UpdateDashboardCommand(BaseCommand): if owners_ids is None: owners_ids = [owner.id for owner in self._model.owners] try: - owners = populate_owners(self._actor, owners_ids) + owners = self.populate_owners(self._actor, owners_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/datasets/commands/create.py b/superset/datasets/commands/create.py index d2169ca7ae..ca6bf8faed 100644 --- a/superset/datasets/commands/create.py +++ b/superset/datasets/commands/create.py @@ -22,8 +22,7 @@ from flask_appbuilder.security.sqla.models import User from marshmallow import ValidationError from sqlalchemy.exc import SQLAlchemyError -from superset.commands.base import BaseCommand -from superset.commands.utils import populate_owners +from superset.commands.base import BaseCommand, CreateMixin from superset.dao.exceptions import DAOCreateFailedError from superset.datasets.commands.exceptions import ( DatabaseNotFoundValidationError, @@ -38,7 +37,7 @@ from superset.extensions import db, security_manager logger = logging.getLogger(__name__) -class CreateDatasetCommand(BaseCommand): +class CreateDatasetCommand(CreateMixin, BaseCommand): def __init__(self, user: User, data: Dict[str, Any]): self._actor = user self._properties = data.copy() @@ -90,7 +89,7 @@ class CreateDatasetCommand(BaseCommand): exceptions.append(TableNotFoundValidationError(table_name)) try: - owners = populate_owners(self._actor, owner_ids) + owners = self.populate_owners(self._actor, owner_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/datasets/commands/update.py b/superset/datasets/commands/update.py index 5be57ffccd..1a82376869 100644 --- a/superset/datasets/commands/update.py +++ b/superset/datasets/commands/update.py @@ -22,8 +22,7 @@ from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User from marshmallow import ValidationError -from superset.commands.base import BaseCommand -from superset.commands.utils import populate_owners +from superset.commands.base import BaseCommand, UpdateMixin from superset.connectors.sqla.models import SqlaTable from superset.dao.exceptions import DAOUpdateFailedError from superset.datasets.commands.exceptions import ( @@ -47,7 +46,7 @@ from superset.views.base import check_ownership logger = logging.getLogger(__name__) -class UpdateDatasetCommand(BaseCommand): +class UpdateDatasetCommand(UpdateMixin, BaseCommand): def __init__( self, user: User, @@ -101,7 +100,7 @@ class UpdateDatasetCommand(BaseCommand): exceptions.append(DatabaseChangeValidationError()) # Validate/Populate owner try: - owners = populate_owners(self._actor, owner_ids) + owners = self.populate_owners(self._actor, owner_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/reports/commands/create.py b/superset/reports/commands/create.py index 79949dfcbd..518a95e105 100644 --- a/superset/reports/commands/create.py +++ b/superset/reports/commands/create.py @@ -22,7 +22,7 @@ from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User from marshmallow import ValidationError -from superset.commands.utils import populate_owners +from superset.commands.base import CreateMixin from superset.dao.exceptions import DAOCreateFailedError from superset.databases.dao import DatabaseDAO from superset.models.reports import ReportScheduleType @@ -40,7 +40,7 @@ from superset.reports.dao import ReportScheduleDAO logger = logging.getLogger(__name__) -class CreateReportScheduleCommand(BaseReportScheduleCommand): +class CreateReportScheduleCommand(CreateMixin, BaseReportScheduleCommand): def __init__(self, user: User, data: Dict[str, Any]): self._actor = user self._properties = data.copy() @@ -90,7 +90,7 @@ class CreateReportScheduleCommand(BaseReportScheduleCommand): ) try: - owners = populate_owners(self._actor, owner_ids) + owners = self.populate_owners(self._actor, owner_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/reports/commands/update.py b/superset/reports/commands/update.py index fcb3dd0f57..09809807f5 100644 --- a/superset/reports/commands/update.py +++ b/superset/reports/commands/update.py @@ -22,7 +22,7 @@ from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User from marshmallow import ValidationError -from superset.commands.utils import populate_owners +from superset.commands.base import UpdateMixin from superset.dao.exceptions import DAOUpdateFailedError from superset.databases.dao import DatabaseDAO from superset.exceptions import SupersetSecurityException @@ -42,7 +42,7 @@ from superset.views.base import check_ownership logger = logging.getLogger(__name__) -class UpdateReportScheduleCommand(BaseReportScheduleCommand): +class UpdateReportScheduleCommand(UpdateMixin, BaseReportScheduleCommand): def __init__(self, user: User, model_id: int, data: Dict[str, Any]): self._actor = user self._model_id = model_id @@ -117,7 +117,7 @@ class UpdateReportScheduleCommand(BaseReportScheduleCommand): if owner_ids is None: owner_ids = [owner.id for owner in self._model.owners] try: - owners = populate_owners(self._actor, owner_ids) + owners = self.populate_owners(self._actor, owner_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index b1a8f823c1..7499e9e3a4 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -568,7 +568,7 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): self.assertEqual(model.created_by, admin) self.assertEqual(model.slice_name, "title1_changed") self.assertEqual(model.description, "description1") - self.assertIn(admin, model.owners) + self.assertNotIn(admin, model.owners) self.assertIn(gamma, model.owners) self.assertEqual(model.viz_type, "viz_type1") self.assertEqual(model.params, """{"a": 1}""") @@ -580,20 +580,39 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): db.session.delete(model) db.session.commit() - def test_update_chart_new_owner(self): + def test_update_chart_new_owner_not_admin(self): """ - Chart API: Test update set new owner to current user + Chart API: Test update set new owner implicitly adds logged in owner + """ + gamma = self.get_user("gamma") + alpha = self.get_user("alpha") + chart_id = self.insert_chart("title", [alpha.id], 1).id + chart_data = {"slice_name": "title1_changed", "owners": [gamma.id]} + self.login(username="alpha") + uri = f"api/v1/chart/{chart_id}" + rv = self.put_assert_metric(uri, chart_data, "put") + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + self.assertIn(alpha, model.owners) + self.assertIn(gamma, model.owners) + db.session.delete(model) + db.session.commit() + + def test_update_chart_new_owner_admin(self): + """ + Chart API: Test update set new owner as admin to other than current user """ gamma = self.get_user("gamma") admin = self.get_user("admin") - chart_id = self.insert_chart("title", [gamma.id], 1).id - chart_data = {"slice_name": "title1_changed"} + chart_id = self.insert_chart("title", [admin.id], 1).id + chart_data = {"slice_name": "title1_changed", "owners": [gamma.id]} self.login(username="admin") uri = f"api/v1/chart/{chart_id}" rv = self.put_assert_metric(uri, chart_data, "put") self.assertEqual(rv.status_code, 200) model = db.session.query(Slice).get(chart_id) - self.assertIn(admin, model.owners) + self.assertNotIn(admin, model.owners) + self.assertIn(gamma, model.owners) db.session.delete(model) db.session.commit() diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 30eaffdceb..039ee20dd7 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -1109,7 +1109,7 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi for slice in slices: self.assertIn(user_alpha1, slice.owners) self.assertIn(user_alpha2, slice.owners) - self.assertIn(admin, slice.owners) + self.assertNotIn(admin, slice.owners) # Revert owners on slice slice.owners = [] db.session.commit() @@ -1149,22 +1149,45 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi db.session.delete(model) db.session.commit() - def test_update_dashboard_new_owner(self): + def test_update_dashboard_new_owner_not_admin(self): """ - Dashboard API: Test update set new owner to current user + Dashboard API: Test update set new owner implicitly adds logged in owner """ - gamma_id = self.get_user("gamma").id + gamma = self.get_user("gamma") + alpha = self.get_user("alpha") + dashboard_id = self.insert_dashboard("title1", "slug1", [alpha.id]).id + dashboard_data = {"dashboard_title": "title1_changed", "owners": [gamma.id]} + self.login(username="alpha") + uri = f"api/v1/dashboard/{dashboard_id}" + rv = self.client.put(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard_id) + self.assertIn(gamma, model.owners) + self.assertIn(alpha, model.owners) + for slc in model.slices: + self.assertIn(gamma, slc.owners) + self.assertIn(alpha, slc.owners) + db.session.delete(model) + db.session.commit() + + def test_update_dashboard_new_owner_admin(self): + """ + Dashboard API: Test update set new owner as admin to other than current user + """ + gamma = self.get_user("gamma") admin = self.get_user("admin") - dashboard_id = self.insert_dashboard("title1", "slug1", [gamma_id]).id - dashboard_data = {"dashboard_title": "title1_changed"} + dashboard_id = self.insert_dashboard("title1", "slug1", [admin.id]).id + dashboard_data = {"dashboard_title": "title1_changed", "owners": [gamma.id]} self.login(username="admin") uri = f"api/v1/dashboard/{dashboard_id}" rv = self.client.put(uri, json=dashboard_data) self.assertEqual(rv.status_code, 200) model = db.session.query(Dashboard).get(dashboard_id) - self.assertIn(admin, model.owners) + self.assertIn(gamma, model.owners) + self.assertNotIn(admin, model.owners) for slc in model.slices: - self.assertIn(admin, slc.owners) + self.assertIn(gamma, slc.owners) + self.assertNotIn(admin, slc.owners) db.session.delete(model) db.session.commit()