fix: Refactor ownership checks and ensure consistency (#20499)

Co-authored-by: John Bodley <john.bodley@airbnb.com>

.pylintrc

@@ -303,7 +303,7 @@ ignored-modules=numpy,pandas,alembic.op,sqlalchemy,alembic.context,flask_appbuil
 # List of class names for which member attributes should not be checked (useful
 # for classes with dynamically set attributes). This supports the use of
 # qualified names.
-ignored-classes=contextlib.closing,optparse.Values,thread._local,_thread._local,sqlalchemy.orm.scoping.scoped_session
+ignored-classes=contextlib.closing,optparse.Values,thread._local,_thread._local
 
 # List of members which are set dynamically and missed by pylint inference
 # system, and so shouldn't trigger E1101 when accessed. Python regular

superset/annotation_layers/annotations/api.py

@@ -17,7 +17,7 @@
 import logging
 from typing import Any, Dict
 
-from flask import g, request, Response
+from flask import request, Response
 from flask_appbuilder.api import expose, permission_name, protect, rison, safe
 from flask_appbuilder.api.schemas import get_item_schema, get_list_schema
 from flask_appbuilder.models.sqla.interface import SQLAInterface
@@ -306,7 +306,7 @@ def post(self, pk: int) -> Response:  # pylint: disable=arguments-differ
         except ValidationError as error:
             return self.response_400(message=error.messages)
         try:
-            new_model = CreateAnnotationCommand(g.user, item).run()
+            new_model = CreateAnnotationCommand(item).run()
             return self.response(201, id=new_model.id, result=item)
         except AnnotationLayerNotFoundError as ex:
             return self.response_400(message=str(ex))
@@ -381,7 +381,7 @@ def put(  # pylint: disable=arguments-differ
         except ValidationError as error:
             return self.response_400(message=error.messages)
         try:
-            new_model = UpdateAnnotationCommand(g.user, annotation_id, item).run()
+            new_model = UpdateAnnotationCommand(annotation_id, item).run()
             return self.response(200, id=new_model.id, result=item)
         except (AnnotationNotFoundError, AnnotationLayerNotFoundError):
             return self.response_404()
@@ -438,7 +438,7 @@ def delete(  # pylint: disable=arguments-differ
               $ref: '#/components/responses/500'
         """
         try:
-            DeleteAnnotationCommand(g.user, annotation_id).run()
+            DeleteAnnotationCommand(annotation_id).run()
             return self.response(200, message="OK")
         except AnnotationNotFoundError:
             return self.response_404()
@@ -495,7 +495,7 @@ def bulk_delete(self, **kwargs: Any) -> Response:
         """
         item_ids = kwargs["rison"]
         try:
-            BulkDeleteAnnotationCommand(g.user, item_ids).run()
+            BulkDeleteAnnotationCommand(item_ids).run()
             return self.response(
                 200,
                 message=ngettext(

superset/annotation_layers/annotations/commands/bulk_delete.py

@@ -17,8 +17,6 @@
 import logging
 from typing import List, Optional
 
-from flask_appbuilder.security.sqla.models import User
-
 from superset.annotation_layers.annotations.commands.exceptions import (
     AnnotationBulkDeleteFailedError,
     AnnotationNotFoundError,
@@ -32,8 +30,7 @@
 
 
 class BulkDeleteAnnotationCommand(BaseCommand):
-    def __init__(self, user: User, model_ids: List[int]):
-        self._actor = user
+    def __init__(self, model_ids: List[int]):
         self._model_ids = model_ids
         self._models: Optional[List[Annotation]] = None
 

superset/annotation_layers/annotations/commands/create.py

@@ -19,7 +19,6 @@
 from typing import Any, Dict, List, Optional
 
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 from marshmallow import ValidationError
 
 from superset.annotation_layers.annotations.commands.exceptions import (
@@ -38,8 +37,7 @@
 
 
 class CreateAnnotationCommand(BaseCommand):
-    def __init__(self, user: User, data: Dict[str, Any]):
-        self._actor = user
+    def __init__(self, data: Dict[str, Any]):
         self._properties = data.copy()
 
     def run(self) -> Model:

superset/annotation_layers/annotations/commands/delete.py

@@ -18,7 +18,6 @@
 from typing import Optional
 
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 
 from superset.annotation_layers.annotations.commands.exceptions import (
     AnnotationDeleteFailedError,
@@ -33,8 +32,7 @@
 
 
 class DeleteAnnotationCommand(BaseCommand):
-    def __init__(self, user: User, model_id: int):
-        self._actor = user
+    def __init__(self, model_id: int):
         self._model_id = model_id
         self._model: Optional[Annotation] = None
 

superset/annotation_layers/annotations/commands/update.py

@@ -19,7 +19,6 @@
 from typing import Any, Dict, List, Optional
 
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 from marshmallow import ValidationError
 
 from superset.annotation_layers.annotations.commands.exceptions import (
@@ -40,8 +39,7 @@
 
 
 class UpdateAnnotationCommand(BaseCommand):
-    def __init__(self, user: User, model_id: int, data: Dict[str, Any]):
-        self._actor = user
+    def __init__(self, model_id: int, data: Dict[str, Any]):
         self._model_id = model_id
         self._properties = data.copy()
         self._model: Optional[Annotation] = None

superset/annotation_layers/api.py

@@ -17,7 +17,7 @@
 import logging
 from typing import Any
 
-from flask import g, request, Response
+from flask import request, Response
 from flask_appbuilder.api import expose, permission_name, protect, rison, safe
 from flask_appbuilder.models.sqla.interface import SQLAInterface
 from flask_babel import ngettext
@@ -151,7 +151,7 @@ def delete(self, pk: int) -> Response:
               $ref: '#/components/responses/500'
         """
         try:
-            DeleteAnnotationLayerCommand(g.user, pk).run()
+            DeleteAnnotationLayerCommand(pk).run()
             return self.response(200, message="OK")
         except AnnotationLayerNotFoundError:
             return self.response_404()
@@ -216,7 +216,7 @@ def post(self) -> Response:
         except ValidationError as error:
             return self.response_400(message=error.messages)
         try:
-            new_model = CreateAnnotationLayerCommand(g.user, item).run()
+            new_model = CreateAnnotationLayerCommand(item).run()
             return self.response(201, id=new_model.id, result=item)
         except AnnotationLayerNotFoundError as ex:
             return self.response_400(message=str(ex))
@@ -288,7 +288,7 @@ def put(self, pk: int) -> Response:
         except ValidationError as error:
             return self.response_400(message=error.messages)
         try:
-            new_model = UpdateAnnotationLayerCommand(g.user, pk, item).run()
+            new_model = UpdateAnnotationLayerCommand(pk, item).run()
             return self.response(200, id=new_model.id, result=item)
         except AnnotationLayerNotFoundError:
             return self.response_404()
@@ -346,7 +346,7 @@ def bulk_delete(self, **kwargs: Any) -> Response:
         """
         item_ids = kwargs["rison"]
         try:
-            BulkDeleteAnnotationLayerCommand(g.user, item_ids).run()
+            BulkDeleteAnnotationLayerCommand(item_ids).run()
             return self.response(
                 200,
                 message=ngettext(

superset/annotation_layers/commands/bulk_delete.py

@@ -17,8 +17,6 @@
 import logging
 from typing import List, Optional
 
-from flask_appbuilder.security.sqla.models import User
-
 from superset.annotation_layers.commands.exceptions import (
     AnnotationLayerBulkDeleteFailedError,
     AnnotationLayerBulkDeleteIntegrityError,
@@ -33,8 +31,7 @@
 
 
 class BulkDeleteAnnotationLayerCommand(BaseCommand):
-    def __init__(self, user: User, model_ids: List[int]):
-        self._actor = user
+    def __init__(self, model_ids: List[int]):
         self._model_ids = model_ids
         self._models: Optional[List[AnnotationLayer]] = None
 

superset/annotation_layers/commands/create.py

@@ -18,7 +18,6 @@
 from typing import Any, Dict, List
 
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 from marshmallow import ValidationError
 
 from superset.annotation_layers.commands.exceptions import (
@@ -34,8 +33,7 @@
 
 
 class CreateAnnotationLayerCommand(BaseCommand):
-    def __init__(self, user: User, data: Dict[str, Any]):
-        self._actor = user
+    def __init__(self, data: Dict[str, Any]):
         self._properties = data.copy()
 
     def run(self) -> Model:

superset/annotation_layers/commands/delete.py

@@ -18,7 +18,6 @@
 from typing import Optional
 
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 
 from superset.annotation_layers.commands.exceptions import (
     AnnotationLayerDeleteFailedError,
@@ -34,8 +33,7 @@
 
 
 class DeleteAnnotationLayerCommand(BaseCommand):
-    def __init__(self, user: User, model_id: int):
-        self._actor = user
+    def __init__(self, model_id: int):
         self._model_id = model_id
         self._model: Optional[AnnotationLayer] = None
 

superset/annotation_layers/commands/update.py

@@ -18,7 +18,6 @@
 from typing import Any, Dict, List, Optional
 
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 from marshmallow import ValidationError
 
 from superset.annotation_layers.commands.exceptions import (
@@ -36,8 +35,7 @@
 
 
 class UpdateAnnotationLayerCommand(BaseCommand):
-    def __init__(self, user: User, model_id: int, data: Dict[str, Any]):
-        self._actor = user
+    def __init__(self, model_id: int, data: Dict[str, Any]):
         self._model_id = model_id
         self._properties = data.copy()
         self._model: Optional[AnnotationLayer] = None

superset/charts/api.py

@@ -21,7 +21,7 @@
 from typing import Any, Optional
 from zipfile import ZipFile
 
-from flask import g, redirect, request, Response, send_file, url_for
+from flask import redirect, request, Response, send_file, url_for
 from flask_appbuilder.api import expose, protect, rison, safe
 from flask_appbuilder.hooks import before_request
 from flask_appbuilder.models.sqla.interface import SQLAInterface
@@ -285,7 +285,7 @@ def post(self) -> Response:
         except ValidationError as error:
             return self.response_400(message=error.messages)
         try:
-            new_model = CreateChartCommand(g.user, item).run()
+            new_model = CreateChartCommand(item).run()
             return self.response(201, id=new_model.id, result=item)
         except ChartInvalidError as ex:
             return self.response_422(message=ex.normalized_messages())
@@ -356,7 +356,7 @@ def put(self, pk: int) -> Response:
         except ValidationError as error:
             return self.response_400(message=error.messages)
         try:
-            changed_model = UpdateChartCommand(g.user, pk, item).run()
+            changed_model = UpdateChartCommand(pk, item).run()
             response = self.response(200, id=changed_model.id, result=item)
         except ChartNotFoundError:
             response = self.response_404()
@@ -416,7 +416,7 @@ def delete(self, pk: int) -> Response:
               $ref: '#/components/responses/500'
         """
         try:
-            DeleteChartCommand(g.user, pk).run()
+            DeleteChartCommand(pk).run()
             return self.response(200, message="OK")
         except ChartNotFoundError:
             return self.response_404()
@@ -476,7 +476,7 @@ def bulk_delete(self, **kwargs: Any) -> Response:
         """
         item_ids = kwargs["rison"]
         try:
-            BulkDeleteChartCommand(g.user, item_ids).run()
+            BulkDeleteChartCommand(item_ids).run()
             return self.response(
                 200,
                 message=ngettext(

superset/charts/commands/bulk_delete.py

@@ -17,9 +17,9 @@
 import logging
 from typing import List, Optional
 
-from flask_appbuilder.security.sqla.models import User
 from flask_babel import lazy_gettext as _
 
+from superset import security_manager
 from superset.charts.commands.exceptions import (
     ChartBulkDeleteFailedError,
     ChartBulkDeleteFailedReportsExistError,
@@ -32,14 +32,12 @@
 from superset.exceptions import SupersetSecurityException
 from superset.models.slice import Slice
 from superset.reports.dao import ReportScheduleDAO
-from superset.views.base import check_ownership
 
 logger = logging.getLogger(__name__)
 
 
 class BulkDeleteChartCommand(BaseCommand):
-    def __init__(self, user: User, model_ids: List[int]):
-        self._actor = user
+    def __init__(self, model_ids: List[int]):
         self._model_ids = model_ids
         self._models: Optional[List[Slice]] = None
 
@@ -66,6 +64,6 @@ def validate(self) -> None:
         # Check ownership
         for model in self._models:
             try:
-                check_ownership(model)
+                security_manager.raise_for_ownership(model)
             except SupersetSecurityException as ex:
                 raise ChartForbiddenError() from ex

superset/charts/commands/create.py

@@ -18,8 +18,8 @@
 from datetime import datetime
 from typing import Any, Dict, List, Optional
 
+from flask import g
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 from marshmallow import ValidationError
 
 from superset.charts.commands.exceptions import (
@@ -37,15 +37,14 @@
 
 
 class CreateChartCommand(CreateMixin, BaseCommand):
-    def __init__(self, user: User, data: Dict[str, Any]):
-        self._actor = user
+    def __init__(self, data: Dict[str, Any]):
         self._properties = data.copy()
 
     def run(self) -> Model:
         self.validate()
         try:
             self._properties["last_saved_at"] = datetime.now()
-            self._properties["last_saved_by"] = self._actor
+            self._properties["last_saved_by"] = g.user
             chart = ChartDAO.create(self._properties)
         except DAOCreateFailedError as ex:
             logger.exception(ex.exception)
@@ -73,7 +72,7 @@ def validate(self) -> None:
         self._properties["dashboards"] = dashboards
 
         try:
-            owners = self.populate_owners(self._actor, owner_ids)
+            owners = self.populate_owners(owner_ids)
             self._properties["owners"] = owners
         except ValidationError as ex:
             exceptions.append(ex)

superset/charts/commands/delete.py

@@ -18,9 +18,9 @@
 from typing import Optional
 
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 from flask_babel import lazy_gettext as _
 
+from superset import security_manager
 from superset.charts.commands.exceptions import (
     ChartDeleteFailedError,
     ChartDeleteFailedReportsExistError,
@@ -34,14 +34,12 @@
 from superset.models.dashboard import Dashboard
 from superset.models.slice import Slice
 from superset.reports.dao import ReportScheduleDAO
-from superset.views.base import check_ownership
 
 logger = logging.getLogger(__name__)
 
 
 class DeleteChartCommand(BaseCommand):
-    def __init__(self, user: User, model_id: int):
-        self._actor = user
+    def __init__(self, model_id: int):
         self._model_id = model_id
         self._model: Optional[Slice] = None
 
@@ -69,6 +67,6 @@ def validate(self) -> None:
             )
         # Check ownership
         try:
-            check_ownership(self._model)
+            security_manager.raise_for_ownership(self._model)
         except SupersetSecurityException as ex:
             raise ChartForbiddenError() from ex