From e74271435e081f36bceb01ea8c64ea062e4a8313 Mon Sep 17 00:00:00 2001 From: John Bodley Date: Wed, 10 Jun 2020 14:00:30 -0700 Subject: [PATCH 1/2] chore(security): Updating assert logic --- UPDATING.md | 2 + superset/charts/api.py | 4 +- superset/common/query_context.py | 9 +++ superset/connectors/base/models.py | 10 +++ superset/security/manager.py | 107 ++++++++++++++++++----------- superset/views/api.py | 7 +- superset/views/core.py | 16 +++-- superset/views/utils.py | 8 ++- superset/viz.py | 9 +++ superset/viz_sip38.py | 9 +++ tests/security_tests.py | 100 ++++++++++++++++++++------- 11 files changed, 203 insertions(+), 78 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 739c3b32d790..10f29bbce4c8 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 d309ef17ab08..c6ff00b6dd1b 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 7cb82a43c025..fb70f0ae8f57 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 f2dad886654e..1f6f032c9bf4 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 010711528223..1dd6569f4efd 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 d37059876e46..a5090b31c9fc 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 e08d92a1894c..1f992edfee5e 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 624886540153..1f7e04e5f064 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 154841cb5802..79f7a1d64b8b 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 a3e00de068fc..9ef4841f0aab 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 92fccab594b2..3d8de6c3512b 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): From 24cf5fdcaf36c5d1f0c0d541ec90c37e7e30c12b Mon Sep 17 00:00:00 2001 From: John Bodley Date: Fri, 19 Jun 2020 19:42:17 -0700 Subject: [PATCH 2/2] Deprecating rejected_tables --- UPDATING.md | 2 +- superset/models/sql_lab.py | 9 +++ superset/security/manager.py | 119 +++++++++++++++++------------------ superset/views/core.py | 33 +++++----- superset/views/utils.py | 8 +-- tests/security_tests.py | 49 +++++++++------ 6 files changed, 115 insertions(+), 105 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 10f29bbce4c8..f5a3644b84ce 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -23,7 +23,7 @@ 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. +* [10034](https://github.com/apache/incubator-superset/pull/10034): Deprecates the public security manager `assert_datasource_permission`, `assert_query_context_permission`, `assert_viz_permission`, and `rejected_tables` 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/models/sql_lab.py b/superset/models/sql_lab.py index 9c3c239b355d..8a90e9abb1bd 100644 --- a/superset/models/sql_lab.py +++ b/superset/models/sql_lab.py @@ -148,6 +148,15 @@ def database_name(self) -> str: def username(self) -> str: return self.user.username + 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=self) + class SavedQuery(Model, AuditMixinNullable, ExtraJSONMixin): """ORM model for SQL query""" diff --git a/superset/security/manager.py b/superset/security/manager.py index 1dd6569f4efd..aac73c73a117 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -17,7 +17,7 @@ # pylint: disable=C,R,W """A set of constants and methods to manage permissions and security""" import logging -from typing import Any, Callable, List, Optional, Set, Tuple, TYPE_CHECKING, Union +from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKING, Union from flask import current_app, g from flask_appbuilder import Model @@ -38,7 +38,7 @@ from sqlalchemy import or_ from sqlalchemy.engine.base import Connection from sqlalchemy.orm.mapper import Mapper -from sqlalchemy.orm.query import Query +from sqlalchemy.orm.query import Query as SqlaQuery from superset import sql_parse from superset.connectors.connector_registry import ConnectorRegistry @@ -52,6 +52,7 @@ from superset.connectors.base.models import BaseDatasource from superset.connectors.druid.models import DruidCluster from superset.models.core import Database + from superset.models.sql_lab import Query from superset.sql_parse import Table from superset.viz import BaseViz @@ -215,30 +216,32 @@ def can_access_all_queries(self) -> bool: def can_access_all_datasources(self) -> bool: """ - Return True if the user can access all Superset datasources, False otherwise. + Return True if the user can fully access all the Superset datasources, False + otherwise. - :returns: Whether the user can access all Superset datasources + :returns: Whether the user can fully access all Superset datasources """ return self.can_access("all_datasource_access", "all_datasource_access") def can_access_all_databases(self) -> bool: """ - Return True if the user can access all Superset databases, False otherwise. + Return True if the user can fully access all the Superset databases, False + otherwise. - :returns: Whether the user can access all Superset databases + :returns: Whether the user can fully access all Superset databases """ return self.can_access("all_database_access", "all_database_access") def can_access_database(self, database: Union["Database", "DruidCluster"]) -> bool: """ - Return True if the user can access the Superset database, False otherwise. + Return True if the user can fully access the Superset database, False otherwise. Note for Druid the database is akin to the Druid cluster. :param database: The Superset database - :returns: Whether the user can access the Superset database + :returns: Whether the user can fully access the Superset database """ return ( @@ -249,14 +252,14 @@ def can_access_database(self, database: Union["Database", "DruidCluster"]) -> bo def can_access_schema(self, datasource: "BaseDatasource") -> bool: """ - Return True if the user can access the schema associated with the Superset + Return True if the user can fully access the schema associated with the Superset datasource, False otherwise. Note for Druid datasources the database and schema are akin to the Druid cluster and datasource name prefix respectively, i.e., [schema.]datasource. :param datasource: The Superset datasource - :returns: Whether the user can access the datasource's schema + :returns: Whether the user can fully access the datasource's schema """ return ( @@ -267,10 +270,11 @@ def can_access_schema(self, datasource: "BaseDatasource") -> bool: def can_access_datasource(self, datasource: "BaseDatasource") -> bool: """ - Return True if the user can access the Superset datasource, False otherwise. + Return True if the user can fully access of the Superset datasource, False + otherwise. :param datasource: The Superset datasource - :returns: Whether the user can access the Superset datasource + :returns: Whether the user can fully access the Superset datasource """ try: @@ -372,8 +376,6 @@ def can_access_table(self, database: "Database", table: "Table") -> bool: :returns: Whether the user can access the SQL table """ - from superset.sql_parse import Table - try: self.raise_for_access(database=database, table=table) except SupersetSecurityException: @@ -381,28 +383,6 @@ def can_access_table(self, database: "Database", table: "Table") -> bool: return True - def rejected_tables( - self, sql: str, database: "Database", schema: str - ) -> Set["Table"]: - """ - Return the list of rejected SQL tables. - - :param sql: The SQL statement - :param database: The SQL database - :param schema: The SQL database schema - :returns: The rejected tables - """ - - from superset.sql_parse import Table - - return { - table - for table in sql_parse.ParsedQuery(sql).tables - if not self.can_access_table( - database, Table(table.table, table.schema or schema) - ) - } - def get_public_role(self) -> Optional[Any]: # Optional[self.role_model] from superset import conf @@ -858,6 +838,7 @@ def raise_for_access( self, database: Optional["Database"] = None, datasource: Optional["BaseDatasource"] = None, + query: Optional["Query"] = None, query_context: Optional["QueryContext"] = None, table: Optional["Table"] = None, viz: Optional["BaseViz"] = None, @@ -865,37 +846,56 @@ def raise_for_access( """ Raise an exception if the user cannot access the resource. - :param database: The Superset database (see table) + :param database: The Superset database :param datasource: The Superset datasource + :param query: The SQL Lab query :param query_context: The query context - :param table: The Superset table (see database) + :param table: The Superset table (requires database) :param viz: The visualization :raises SupersetSecurityException: If the user cannot access the resource """ - from superset import db from superset.connectors.sqla.models import SqlaTable + from superset.sql_parse import Table + + if database and table or query: + if query: + database = query.database + + database = cast("Database", database) - if database and table: if self.can_access_database(database): return - schema_perm = self.get_schema_perm(database, schema=table.schema) + if query: + tables = { + Table(table_.table, table_.schema or query.schema) + for table_ in sql_parse.ParsedQuery(query.sql).tables + } + elif table: + tables = {table} - if schema_perm and self.can_access("schema_access", schema_perm): - return + denied = set() - datasources = SqlaTable.query_datasources_by_name( - db.session, database, table.table, schema=table.schema - ) + for table_ in tables: + schema_perm = self.get_schema_perm(database, schema=table_.schema) - for datasource in datasources: - if self.can_access("datasource_access", datasource.perm): - return + if not (schema_perm and self.can_access("schema_access", schema_perm)): + datasources = SqlaTable.query_datasources_by_name( + self.get_session, database, table_.table, schema=table_.schema + ) - raise SupersetSecurityException( - self.get_table_access_error_object({table}), - ) + # Access to any datasource is suffice. + for datasource in datasources: + if self.can_access("datasource_access", datasource.perm): + break + else: + denied.add(table_) + + if denied: + raise SupersetSecurityException( + self.get_table_access_error_object(denied), + ) if datasource or query_context or viz: if query_context: @@ -905,16 +905,15 @@ def raise_for_access( assert datasource - if self.can_access_schema(datasource) or self.can_access( - "datasource_access", datasource.perm or "" + if not ( + self.can_access_schema(datasource) + or self.can_access("datasource_access", datasource.perm or "") ): - return - - raise SupersetSecurityException( - self.get_datasource_access_error_object(datasource), - ) + raise SupersetSecurityException( + self.get_datasource_access_error_object(datasource), + ) - def get_rls_filters(self, table: "BaseDatasource") -> List[Query]: + def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: """ Retrieves the appropriate row level security filters for the current user and the passed table. diff --git a/superset/views/core.py b/superset/views/core.py index 1f992edfee5e..4146914dc542 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -69,6 +69,7 @@ CertificateException, DatabaseNotFound, SupersetException, + SupersetSecurityException, SupersetTimeoutException, ) from superset.jinja_context import get_template_processor @@ -1988,14 +1989,10 @@ def results_exec(self, key: str) -> FlaskResponse: status=404, ) - rejected_tables = security_manager.rejected_tables( - query.sql, query.database, query.schema - ) - if rejected_tables: - return json_errors_response( - [security_manager.get_table_access_error_object(rejected_tables)], - status=403, - ) + try: + query.raise_for_access() + except SupersetSecurityException as ex: + return json_errors_response([ex.error], status=403) payload = utils.zlib_decompress(blob, decode=not results_backend_use_msgpack) obj = _deserialize_results_payload( @@ -2293,14 +2290,12 @@ def sql_json_exec( logger.info(f"Triggering query_id: {query_id}") - rejected_tables = security_manager.rejected_tables(sql, mydb, schema) - if rejected_tables: + try: + query.raise_for_access() + except SupersetSecurityException as ex: query.status = QueryStatus.FAILED session.commit() - return json_errors_response( - [security_manager.get_table_access_error_object(rejected_tables)], - status=403, - ) + return json_errors_response([ex.error], status=403) try: template_processor = get_template_processor( @@ -2347,12 +2342,12 @@ def csv(self, client_id: str) -> FlaskResponse: logger.info("Exporting CSV file [{}]".format(client_id)) query = db.session.query(Query).filter_by(client_id=client_id).one() - rejected_tables = security_manager.rejected_tables( - query.sql, query.database, query.schema - ) - if rejected_tables: - flash(security_manager.get_table_access_error_msg(rejected_tables)) + try: + query.raise_for_access() + except SupersetSecurityException as ex: + flash(ex.error.message) return redirect("/") + blob = None if results_backend and query.results_key: logger.info( diff --git a/superset/views/utils.py b/superset/views/utils.py index 1f7e04e5f064..2a8b2ccc049d 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -28,13 +28,7 @@ from flask_appbuilder.security.sqla.models import User import superset.models.core as models -from superset import ( - app, - dataframe, - db, - is_feature_enabled, - result_set, -) +from superset import app, dataframe, db, is_feature_enabled, result_set from superset.connectors.connector_registry import ConnectorRegistry from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetException, SupersetSecurityException diff --git a/tests/security_tests.py b/tests/security_tests.py index 3d8de6c3512b..38575e29b8e0 100644 --- a/tests/security_tests.py +++ b/tests/security_tests.py @@ -777,39 +777,39 @@ class SecurityManagerTests(SupersetTestCase): """ @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") + 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_table(database, table)) + self.assertTrue(security_manager.can_access_datasource(datasource=datasource)) mock_raise_for_access.side_effect = SupersetSecurityException( SupersetError( "dummy", - SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR, + SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, ErrorLevel.ERROR, ) ) - self.assertFalse(security_manager.can_access_table(database, table)) + self.assertFalse(security_manager.can_access_datasource(datasource=datasource)) @patch("superset.security.SupersetSecurityManager.raise_for_access") - def test_can_access_datasource(self, mock_raise_for_access): - datasource = self.get_datasource_mock() + 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_datasource(datasource=datasource)) + self.assertTrue(security_manager.can_access_table(database, table)) mock_raise_for_access.side_effect = SupersetSecurityException( SupersetError( "dummy", - SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR, ErrorLevel.ERROR, ) ) - self.assertFalse(security_manager.can_access_datasource(datasource=datasource)) + self.assertFalse(security_manager.can_access_table(database, table)) @patch("superset.security.SupersetSecurityManager.can_access") @patch("superset.security.SupersetSecurityManager.can_access_schema") @@ -826,25 +826,25 @@ def test_raise_for_access_datasource(self, mock_can_access_schema, mock_can_acce 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") + def test_raise_for_access_query(self, mock_can_access): + query = Mock( + database=get_example_database(), schema="bar", sql="SELECT * FROM foo" + ) mock_can_access.return_value = True - security_manager.raise_for_access(database=database, table=table) + security_manager.raise_for_access(query=query) mock_can_access.return_value = False with self.assertRaises(SupersetSecurityException): - security_manager.raise_for_access(database=database, table=table) + security_manager.raise_for_access(query=query) @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 = Mock(datasource=self.get_datasource_mock()) mock_can_access_schema.return_value = True security_manager.raise_for_access(query_context=query_context) @@ -855,6 +855,19 @@ def test_raise_for_access_query_context( with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(query_context=query_context) + @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) + + mock_can_access.return_value = False + + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access(database=database, table=table) + @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):