diff --git a/UPDATING.md b/UPDATING.md index 739c3b32d7902..10f29bbce4c86 100644 --- a/UPDATING.md +++ b/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. diff --git a/superset/charts/api.py b/superset/charts/api.py index d309ef17ab08d..c6ff00b6dd1ba 100644 --- a/superset/charts/api.py +++ b/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() diff --git a/superset/common/query_context.py b/superset/common/query_context.py index 7cb82a43c0253..fb70f0ae8f570 100644 --- a/superset/common/query_context.py +++ b/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) diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py index f2dad886654e5..1f6f032c9bf42 100644 --- a/superset/connectors/base/models.py +++ b/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""" diff --git a/superset/security/manager.py b/superset/security/manager.py index 0107115282230..1dd6569f4efd5 100644 --- a/superset/security/manager.py +++ b/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 diff --git a/superset/views/api.py b/superset/views/api.py index d37059876e465..a5090b31c9fcd 100644 --- a/superset/views/api.py +++ b/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 diff --git a/superset/views/core.py b/superset/views/core.py index e08d92a1894cd..1f992edfee5ec 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -728,7 +728,8 @@ 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( @@ -736,7 +737,8 @@ def filter( ) 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 diff --git a/superset/views/utils.py b/superset/views/utils.py index 6248865401532..1f7e04e5f0646 100644 --- a/superset/views/utils.py +++ b/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( diff --git a/superset/viz.py b/superset/viz.py index 154841cb58026..79f7a1d64b8b0 100644 --- a/superset/viz.py +++ b/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): diff --git a/superset/viz_sip38.py b/superset/viz_sip38.py index a3e00de068fca..9ef4841f0aab6 100644 --- a/superset/viz_sip38.py +++ b/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): diff --git a/tests/security_tests.py b/tests/security_tests.py index 92fccab594b2f..3d8de6c3512bf 100644 --- a/tests/security_tests.py +++ b/tests/security_tests.py @@ -26,9 +26,11 @@ from superset import app, appbuilder, db, security_manager, viz from superset.connectors.druid.models import DruidCluster, DruidDatasource from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetSecurityException from superset.models.core import Database from superset.models.slice import Slice +from superset.sql_parse import Table from superset.utils.core import get_example_database from .base_tests import SupersetTestCase @@ -774,48 +776,98 @@ class SecurityManagerTests(SupersetTestCase): Testing the Security Manager. """ - @patch("superset.security.SupersetSecurityManager.can_access_datasource") - def test_assert_datasource_permission(self, mock_can_access_datasource): + @patch("superset.security.SupersetSecurityManager.raise_for_access") + def test_can_access_table(self, mock_raise_for_access): + database = get_example_database() + table = Table("bar", "foo") + + mock_raise_for_access.return_value = None + self.assertTrue(security_manager.can_access_table(database, table)) + + mock_raise_for_access.side_effect = SupersetSecurityException( + SupersetError( + "dummy", + SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR, + ErrorLevel.ERROR, + ) + ) + + self.assertFalse(security_manager.can_access_table(database, table)) + + @patch("superset.security.SupersetSecurityManager.raise_for_access") + def test_can_access_datasource(self, mock_raise_for_access): + datasource = self.get_datasource_mock() + + mock_raise_for_access.return_value = None + self.assertTrue(security_manager.can_access_datasource(datasource=datasource)) + + mock_raise_for_access.side_effect = SupersetSecurityException( + SupersetError( + "dummy", + SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + ErrorLevel.ERROR, + ) + ) + + self.assertFalse(security_manager.can_access_datasource(datasource=datasource)) + + @patch("superset.security.SupersetSecurityManager.can_access") + @patch("superset.security.SupersetSecurityManager.can_access_schema") + def test_raise_for_access_datasource(self, mock_can_access_schema, mock_can_access): datasource = self.get_datasource_mock() - # Datasource with the "datasource_access" permission. - mock_can_access_datasource.return_value = True - security_manager.assert_datasource_permission(datasource) + mock_can_access_schema.return_value = True + security_manager.raise_for_access(datasource=datasource) + + mock_can_access.return_value = False + mock_can_access_schema.return_value = False + + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access(datasource=datasource) + + @patch("superset.security.SupersetSecurityManager.can_access") + def test_raise_for_access_table(self, mock_can_access): + database = get_example_database() + table = Table("bar", "foo") + + mock_can_access.return_value = True + security_manager.raise_for_access(database=database, table=table) - # Datasource without the "datasource_access" permission. - mock_can_access_datasource.return_value = False + mock_can_access.return_value = False with self.assertRaises(SupersetSecurityException): - security_manager.assert_datasource_permission(datasource) + security_manager.raise_for_access(database=database, table=table) - @patch("superset.security.SupersetSecurityManager.can_access_datasource") - def test_assert_query_context_permission(self, mock_can_access_datasource): + @patch("superset.security.SupersetSecurityManager.can_access") + @patch("superset.security.SupersetSecurityManager.can_access_schema") + def test_raise_for_access_query_context( + self, mock_can_access_schema, mock_can_access + ): query_context = Mock() query_context.datasource = self.get_datasource_mock() - # Query context with the "datasource_access" permission. - mock_can_access_datasource.return_value = True - security_manager.assert_query_context_permission(query_context) + mock_can_access_schema.return_value = True + security_manager.raise_for_access(query_context=query_context) - # Query context without the "datasource_access" permission. - mock_can_access_datasource.return_value = False + mock_can_access.return_value = False + mock_can_access_schema.return_value = False with self.assertRaises(SupersetSecurityException): - security_manager.assert_query_context_permission(query_context) + security_manager.raise_for_access(query_context=query_context) - @patch("superset.security.SupersetSecurityManager.can_access_datasource") - def test_assert_viz_permission(self, mock_can_access_datasource): + @patch("superset.security.SupersetSecurityManager.can_access") + @patch("superset.security.SupersetSecurityManager.can_access_schema") + def test_raise_for_access_viz(self, mock_can_access_schema, mock_can_access): test_viz = viz.TableViz(self.get_datasource_mock(), form_data={}) - # Visualization with the "datasource_access" permission. - mock_can_access_datasource.return_value = True - security_manager.assert_viz_permission(test_viz) + mock_can_access_schema.return_value = True + security_manager.raise_for_access(viz=test_viz) - # Visualization without the "datasource_access" permission. - mock_can_access_datasource.return_value = False + mock_can_access.return_value = False + mock_can_access_schema.return_value = False with self.assertRaises(SupersetSecurityException): - security_manager.assert_viz_permission(test_viz) + security_manager.raise_for_access(viz=test_viz) class RowLevelSecurityTests(SupersetTestCase):