feat(dao): admin can remove self from object owners (apache#15149)

(cherry picked from commit d6f9c48)

superset/charts/commands/create.py

@@ -28,15 +28,15 @@
     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 @@ def validate(self) -> None:
         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)

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 @@ def validate(self) -> None:
         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)

superset/commands/utils.py

@@ -29,17 +29,25 @@
 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)

superset/dashboards/commands/create.py

@@ -21,8 +21,8 @@
 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 @@
 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 @@ def validate(self) -> None:
             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)

superset/dashboards/commands/update.py

@@ -21,8 +21,8 @@
 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 @@
 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 @@ def validate(self) -> None:
         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)

superset/datasets/commands/create.py

@@ -22,8 +22,7 @@
 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 @@
 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 @@ def validate(self) -> None:
             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)

superset/datasets/commands/update.py

@@ -22,8 +22,7 @@
 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 @@
 logger = logging.getLogger(__name__)
 
 
-class UpdateDatasetCommand(BaseCommand):
+class UpdateDatasetCommand(UpdateMixin, BaseCommand):
     def __init__(
         self,
         user: User,
@@ -101,7 +100,7 @@ def validate(self) -> None:
             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)

superset/reports/commands/create.py

@@ -22,7 +22,7 @@
 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 @@
 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 @@ def validate(self) -> None:
             )
 
         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)

superset/reports/commands/update.py

@@ -22,7 +22,7 @@
 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 @@
 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 @@ def validate(self) -> None:
         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)

tests/integration_tests/charts/api_tests.py

@@ -568,7 +568,7 @@ def test_update_chart(self):
         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 @@ def test_update_chart(self):
         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()
 

tests/integration_tests/dashboards/api_tests.py

@@ -1110,7 +1110,7 @@ def test_update_dashboard_chart_owners(self):
         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()
@@ -1150,22 +1150,45 @@ def test_update_partial_dashboard(self):
         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()