chore(security): Updating assert logic

UPDATING.md

@@ -23,6 +23,8 @@ assists people when migrating to a new version.
 
 ## Next
 
+* [10034](https://github.com/apache/incubator-superset/pull/10034): Deprecates the public security manager  `assert_datasource_permission`, `assert_query_context_permission`, and `assert_viz_permission` methods with the `raise_for_access` method which also handles assertion logic for SQL tables.
+
 * [10031](https://github.com/apache/incubator-superset/pull/10030): Renames the following public security manager methods: `can_access_datasource` to `can_access_table`, `all_datasource_access` to `can_access_all_datasources`, `all_database_access` to `can_access_all_databases`, `database_access` to `can_access_database`, `schema_access` to `can_access_schema`, and
 `datasource_access` to `can_access_datasource`. Regrettably it is not viable to provide aliases for the deprecated methods as this would result in a name clash. Finally the `can_access_table` (previously `can_access_database`) method signature has changed, i.e., the optional `schema` argument no longer exists.
 

superset/charts/api.py

@@ -53,7 +53,7 @@
 )
 from superset.constants import RouteMethod
 from superset.exceptions import SupersetSecurityException
-from superset.extensions import event_logger, security_manager
+from superset.extensions import event_logger
 from superset.models.slice import Slice
 from superset.tasks.thumbnails import cache_chart_thumbnail
 from superset.utils.core import ChartDataResultFormat, json_int_dttm_ser
@@ -454,7 +454,7 @@ def data(self) -> Response:
         except KeyError:
             return self.response_400(message="Request is incorrect")
         try:
-            security_manager.assert_query_context_permission(query_context)
+            query_context.raise_for_access()
         except SupersetSecurityException:
             return self.response_401()
         payload = query_context.get_payload()

superset/common/query_context.py

@@ -286,3 +286,12 @@ def get_df_payload(  # pylint: disable=too-many-locals,too-many-statements
             "stacktrace": stacktrace,
             "rowcount": len(df.index),
         }
+
+    def raise_for_access(self) -> None:
+        """
+        Raise an exception if the user cannot access the resource.
+
+        :raises SupersetSecurityException: If the user cannot access the resource
+        """
+
+        security_manager.raise_for_access(query_context=self)

superset/connectors/base/models.py

@@ -23,6 +23,7 @@
 from sqlalchemy.ext.declarative import declared_attr
 from sqlalchemy.orm import foreign, Query, relationship, RelationshipProperty
 
+from superset import security_manager
 from superset.constants import NULL_STRING
 from superset.models.helpers import AuditMixinNullable, ImportMixin, QueryResult
 from superset.models.slice import Slice
@@ -496,6 +497,15 @@ def __eq__(self, other: object) -> bool:
             return NotImplemented
         return self.uid == other.uid
 
+    def raise_for_access(self) -> None:
+        """
+        Raise an exception if the user cannot access the resource.
+
+        :raises SupersetSecurityException: If the user cannot access the resource
+        """
+
+        security_manager.raise_for_access(datasource=self)
+
 
 class BaseColumn(AuditMixinNullable, ImportMixin):
     """Interface for column"""

superset/security/manager.py

@@ -273,9 +273,12 @@ def can_access_datasource(self, datasource: "BaseDatasource") -> bool:
         :returns: Whether the user can access the Superset datasource
         """
 
-        return self.can_access_schema(datasource) or self.can_access(
-            "datasource_access", datasource.perm or ""
-        )
+        try:
+            self.raise_for_access(datasource=datasource)
+        except SupersetSecurityException:
+            return False
+
+        return True
 
     def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str:
         """
@@ -369,23 +372,14 @@ def can_access_table(self, database: "Database", table: "Table") -> bool:
         :returns: Whether the user can access the SQL table
         """
 
-        from superset import db
-        from superset.connectors.sqla.models import SqlaTable
-
-        if self.can_access_database(database):
-            return True
+        from superset.sql_parse import Table
 
-        schema_perm = self.get_schema_perm(database, schema=table.schema)
-        if schema_perm and self.can_access("schema_access", schema_perm):
-            return True
+        try:
+            self.raise_for_access(database=database, table=table)
+        except SupersetSecurityException:
+            return False
 
-        datasources = SqlaTable.query_datasources_by_name(
-            db.session, database, table.table, schema=table.schema
-        )
-        for datasource in datasources:
-            if self.can_access("datasource_access", datasource.perm):
-                return True
-        return False
+        return True
 
     def rejected_tables(
         self, sql: str, database: "Database", schema: str
@@ -860,45 +854,73 @@ def set_perm(
                     )
                 )
 
-    def assert_datasource_permission(self, datasource: "BaseDatasource") -> None:
+    def raise_for_access(
+        self,
+        database: Optional["Database"] = None,
+        datasource: Optional["BaseDatasource"] = None,
+        query_context: Optional["QueryContext"] = None,
+        table: Optional["Table"] = None,
+        viz: Optional["BaseViz"] = None,
+    ) -> None:
         """
-        Assert the the user has permission to access the Superset datasource.
+        Raise an exception if the user cannot access the resource.
 
+        :param database: The Superset database (see table)
         :param datasource: The Superset datasource
-        :raises SupersetSecurityException: If the user does not have permission
+        :param query_context: The query context
+        :param table: The Superset table (see database)
+        :param viz: The visualization
+        :raises SupersetSecurityException: If the user cannot access the resource
         """
 
-        if not self.can_access_datasource(datasource):
-            raise SupersetSecurityException(
-                self.get_datasource_access_error_object(datasource),
+        from superset import db
+        from superset.connectors.sqla.models import SqlaTable
+
+        if database and table:
+            if self.can_access_database(database):
+                return
+
+            schema_perm = self.get_schema_perm(database, schema=table.schema)
+
+            if schema_perm and self.can_access("schema_access", schema_perm):
+                return
+
+            datasources = SqlaTable.query_datasources_by_name(
+                db.session, database, table.table, schema=table.schema
             )
 
-    def assert_query_context_permission(self, query_context: "QueryContext") -> None:
-        """
-        Assert the the user has permission to access the query context.
+            for datasource in datasources:
+                if self.can_access("datasource_access", datasource.perm):
+                    return
 
-        :param query_context: The query context
-        :raises SupersetSecurityException: If the user does not have permission
-        """
+            raise SupersetSecurityException(
+                self.get_table_access_error_object({table}),
+            )
 
-        self.assert_datasource_permission(query_context.datasource)
+        if datasource or query_context or viz:
+            if query_context:
+                datasource = query_context.datasource
+            elif viz:
+                datasource = viz.datasource
 
-    def assert_viz_permission(self, viz: "BaseViz") -> None:
-        """
-        Assert the the user has permission to access the visualization.
+            assert datasource
 
-        :param viz: The visualization
-        :raises SupersetSecurityException: If the user does not have permission
-        """
+            if self.can_access_schema(datasource) or self.can_access(
+                "datasource_access", datasource.perm or ""
+            ):
+                return
 
-        self.assert_datasource_permission(viz.datasource)
+            raise SupersetSecurityException(
+                self.get_datasource_access_error_object(datasource),
+            )
 
     def get_rls_filters(self, table: "BaseDatasource") -> List[Query]:
         """
-        Retrieves the appropriate row level security filters for the current user and the passed table.
+        Retrieves the appropriate row level security filters for the current user and
+        the passed table.
 
         :param table: The table to check against
-        :returns: A list of filters.
+        :returns: A list of filters
         """
         if hasattr(g, "user") and hasattr(g.user, "id"):
             from superset import db
@@ -929,10 +951,11 @@ def get_rls_filters(self, table: "BaseDatasource") -> List[Query]:
 
     def get_rls_ids(self, table: "BaseDatasource") -> List[int]:
         """
-        Retrieves the appropriate row level security filters IDs for the current user and the passed table.
+        Retrieves the appropriate row level security filters IDs for the current user
+        and the passed table.
 
         :param table: The table to check against
-        :returns: A list of IDs.
+        :returns: A list of IDs
         """
         ids = [f.id for f in self.get_rls_filters(table)]
         ids.sort()  # Combinations rather than permutations

superset/views/api.py

@@ -20,7 +20,7 @@
 from flask_appbuilder import expose
 from flask_appbuilder.security.decorators import has_access_api
 
-from superset import db, event_logger, security_manager
+from superset import db, event_logger
 from superset.common.query_context import QueryContext
 from superset.legacy import update_time_range
 from superset.models.slice import Slice
@@ -39,10 +39,11 @@ def query(self) -> FlaskResponse:
         """
         Takes a query_obj constructed in the client and returns payload data response
         for the given query_obj.
-        params: query_context: json_blob
+
+        raises SupersetSecurityException: If the user cannot access the resource
         """
         query_context = QueryContext(**json.loads(request.form["query_context"]))
-        security_manager.assert_query_context_permission(query_context)
+        query_context.raise_for_access()
         payload_json = query_context.get_payload()
         return json.dumps(
             payload_json, default=utils.json_int_dttm_ser, ignore_nan=True

superset/views/core.py

@@ -728,15 +728,17 @@ def filter(
         :param datasource_type: Type of datasource e.g. table
         :param datasource_id: Datasource id
         :param column: Column name to retrieve values for
-        :return:
+        :returns: The Flask response
+        :raises SupersetSecurityException: If the user cannot access the resource
         """
         # TODO: Cache endpoint by user, datasource and column
         datasource = ConnectorRegistry.get_datasource(
             datasource_type, datasource_id, db.session
         )
         if not datasource:
             return json_error_response(DATASOURCE_MISSING_ERR)
-        security_manager.assert_datasource_permission(datasource)
+
+        datasource.raise_for_access()
         payload = json.dumps(
             datasource.values_for_column(column, config["FILTER_SELECT_ROW_LIMIT"]),
             default=utils.json_int_dttm_ser,
@@ -2399,6 +2401,13 @@ def csv(self, client_id: str) -> FlaskResponse:
     @expose("/fetch_datasource_metadata")
     @event_logger.log_this
     def fetch_datasource_metadata(self) -> FlaskResponse:
+        """
+        Fetch the datasource metadata.
+
+        :returns: The Flask response
+        :raises SupersetSecurityException: If the user cannot access the resource
+        """
+
         datasource_id, datasource_type = request.args["datasourceKey"].split("__")
         datasource = ConnectorRegistry.get_datasource(
             datasource_type, datasource_id, db.session
@@ -2407,8 +2416,7 @@ def fetch_datasource_metadata(self) -> FlaskResponse:
         if not datasource:
             return json_error_response(DATASOURCE_MISSING_ERR)
 
-        # Check permission for datasource
-        security_manager.assert_datasource_permission(datasource)
+        datasource.raise_for_access()
         return json_success(json.dumps(datasource.data))
 
     @has_access_api

superset/views/utils.py

@@ -34,7 +34,6 @@
     db,
     is_feature_enabled,
     result_set,
-    security_manager,
 )
 from superset.connectors.connector_registry import ConnectorRegistry
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
@@ -433,7 +432,7 @@ def check_datasource_perms(
         force=False,
     )
 
-    security_manager.assert_viz_permission(viz_obj)
+    viz_obj.raise_for_access()
 
 
 def check_slice_perms(_self: Any, slice_id: int) -> None:
@@ -442,6 +441,9 @@ def check_slice_perms(_self: Any, slice_id: int) -> None:
 
     This function takes `self` since it must have the same signature as the
     the decorated method.
+
+    :param slice_id: The slice ID
+    :raises SupersetSecurityException: If the user cannot access the resource
     """
 
     form_data, slc = get_form_data(slice_id, use_slice_data=True)
@@ -454,7 +456,7 @@ def check_slice_perms(_self: Any, slice_id: int) -> None:
             force=False,
         )
 
-        security_manager.assert_viz_permission(viz_obj)
+        viz_obj.raise_for_access()
 
 
 def _deserialize_results_payload(

superset/viz.py

@@ -552,6 +552,15 @@ def get_data(self, df: pd.DataFrame) -> VizData:
     def json_data(self) -> str:
         return json.dumps(self.data)
 
+    def raise_for_access(self) -> None:
+        """
+        Raise an exception if the user cannot access the resource.
+
+        :raises SupersetSecurityException: If the user cannot access the resource
+        """
+
+        security_manager.raise_for_access(viz=self)
+
 
 class TableViz(BaseViz):
 

superset/viz_sip38.py

@@ -592,6 +592,15 @@ def get_data(self, df: pd.DataFrame) -> VizData:
     def json_data(self):
         return json.dumps(self.data)
 
+    def raise_for_access(self) -> None:
+        """
+        Raise an exception if the user cannot access the resource.
+
+        :raises SupersetSecurityException: If the user cannot access the resource
+        """
+
+        security_manager.raise_for_access(viz=self)
+
 
 class TableViz(BaseViz):