This is an automated email from the ASF dual-hosted git repository. johnbodley pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push: new 9532bff chore(security): Renaming access methods (#10031) 9532bff is described below commit 9532bff48fd2a5c01f3e5e69d3bce166822951b5 Author: John Bodley <4567245+john-bod...@users.noreply.github.com> AuthorDate: Thu Jun 11 13:12:23 2020 -0700 chore(security): Renaming access methods (#10031) Co-authored-by: John Bodley <john.bod...@airbnb.com> --- UPDATING.md | 3 ++ superset/charts/filters.py | 2 +- superset/dashboards/filters.py | 3 +- superset/security/manager.py | 70 +++++++++++++++++------------------ superset/views/base.py | 2 +- superset/views/chart/filters.py | 2 +- superset/views/core.py | 22 +++++------ superset/views/dashboard/filters.py | 3 +- superset/views/database/decorators.py | 5 +-- superset/views/database/filters.py | 2 +- superset/views/database/forms.py | 5 +-- superset/views/database/validators.py | 5 +-- tests/core_tests.py | 13 ++++--- tests/security_tests.py | 24 ++++++------ 14 files changed, 76 insertions(+), 85 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index c017930..739c3b3 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -23,6 +23,9 @@ assists people when migrating to a new version. ## Next +* [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. + * [10030](https://github.com/apache/incubator-superset/pull/10030): Renames the public security manager `schemas_accessible_by_user` method to `get_schemas_accessible_by_user`. * [9786](https://github.com/apache/incubator-superset/pull/9786): with the upgrade of `werkzeug` from version `0.16.0` to `1.0.1`, the `werkzeug.contrib.cache` module has been moved to a standalone package [cachelib](https://pypi.org/project/cachelib/). For example, to import the `RedisCache` class, please use the following import: `from cachelib.redis import RedisCache`. diff --git a/superset/charts/filters.py b/superset/charts/filters.py index 94ae2ad..2659c6c 100644 --- a/superset/charts/filters.py +++ b/superset/charts/filters.py @@ -45,7 +45,7 @@ class ChartNameOrDescriptionFilter( class ChartFilter(BaseFilter): # pylint: disable=too-few-public-methods def apply(self, query: Query, value: Any) -> Query: - if security_manager.all_datasource_access(): + if security_manager.can_access_all_datasources(): return query perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 05a6f6e..f13f36e 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -62,7 +62,6 @@ class DashboardFilter(BaseFilter): # pylint: disable=too-few-public-methods datasource_perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") - all_datasource_access = security_manager.all_datasource_access() published_dash_query = ( db.session.query(Dashboard.id) .join(Dashboard.slices) @@ -72,7 +71,7 @@ class DashboardFilter(BaseFilter): # pylint: disable=too-few-public-methods or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), - all_datasource_access, + security_manager.can_access_all_datasources(), ), ) ) diff --git a/superset/security/manager.py b/superset/security/manager.py index 85ce12e..9888bdd 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -188,15 +188,14 @@ class SupersetSecurityManager(SecurityManager): def can_access(self, permission_name: str, view_name: str) -> bool: """ - Return True if the user can access the FAB permission/view, False - otherwise. + Return True if the user can access the FAB permission/view, False otherwise. Note this method adds protection from has_access failing from missing permission/view entries. :param permission_name: The FAB permission name :param view_name: The FAB view-menu name - :returns: Whether the use can access the FAB permission/view + :returns: Whether the user can access the FAB permission/view """ user = g.user @@ -206,13 +205,14 @@ class SupersetSecurityManager(SecurityManager): def can_access_all_queries(self) -> bool: """ - Return True if the user can access all queries, False otherwise. + Return True if the user can access all SQL Lab queries, False otherwise. :returns: Whether the user can access all queries """ + return self.can_access("all_query_access", "all_query_access") - def all_datasource_access(self) -> bool: + def can_access_all_datasources(self) -> bool: """ Return True if the user can access all Superset datasources, False otherwise. @@ -221,7 +221,7 @@ class SupersetSecurityManager(SecurityManager): return self.can_access("all_datasource_access", "all_datasource_access") - def all_database_access(self) -> bool: + def can_access_all_databases(self) -> bool: """ Return True if the user can access all Superset databases, False otherwise. @@ -230,7 +230,7 @@ class SupersetSecurityManager(SecurityManager): return self.can_access("all_database_access", "all_database_access") - def database_access(self, database: "Database") -> bool: + def can_access_database(self, database: "Database") -> bool: """ Return True if the user can access the Superset database, False otherwise. @@ -238,39 +238,39 @@ class SupersetSecurityManager(SecurityManager): :returns: Whether the user can access the Superset database """ return ( - self.all_datasource_access() - or self.all_database_access() + self.can_access_all_datasources() + or self.can_access_all_databases() or self.can_access("database_access", database.perm) ) - def schema_access(self, datasource: "BaseDatasource") -> bool: + def can_access_schema(self, datasource: "BaseDatasource") -> bool: """ Return True if the user can 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, i.e., [schema.]datasource, respectively. + and datasource name prefix respectively, i.e., [schema.]datasource. :param datasource: The Superset datasource :returns: Whether the user can access the datasource's schema """ - schema_perm = datasource.schema_perm or "" + return ( - self.all_datasource_access() - or self.database_access(datasource.database) - or self.can_access("schema_access", schema_perm) + self.can_access_all_datasources() + or self.can_access_database(datasource.database) + or self.can_access("schema_access", datasource.schema_perm or "") ) - def datasource_access(self, datasource: "BaseDatasource") -> bool: + def can_access_datasource(self, datasource: "BaseDatasource") -> bool: """ Return True if the user can access the Superset datasource, False otherwise. :param datasource: The Superset datasource - :returns: Whether the use can access the Superset datasource + :returns: Whether the user can access the Superset datasource """ - perm = datasource.perm or "" - return self.schema_access(datasource) or self.can_access( - "datasource_access", perm + + return self.can_access_schema(datasource) or self.can_access( + "datasource_access", datasource.perm or "" ) def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str: @@ -356,30 +356,27 @@ class SupersetSecurityManager(SecurityManager): return conf.get("PERMISSION_INSTRUCTIONS_LINK") - def can_access_datasource( - self, database: "Database", table: "Table", schema: Optional[str] = None - ) -> bool: + def can_access_table(self, database: "Database", table: "Table") -> bool: """ Return True if the user can access the SQL table, False otherwise. :param database: The SQL database :param table: The SQL table - :param schema: The fallback SQL schema if not present in the table - :returns: Whether the use can access the SQL table + :returns: Whether the user can access the SQL table """ from superset import db from superset.connectors.sqla.models import SqlaTable - if self.database_access(database) or self.all_datasource_access(): + if self.can_access_database(database): return True - schema_perm = self.get_schema_perm(database, schema=table.schema or schema) + schema_perm = self.get_schema_perm(database, schema=table.schema) if schema_perm and self.can_access("schema_access", schema_perm): return True datasources = SqlaTable.query_datasources_by_name( - db.session, database, table.table, schema=table.schema or schema + db.session, database, table.table, schema=table.schema ) for datasource in datasources: if self.can_access("datasource_access", datasource.perm): @@ -397,12 +394,15 @@ class SupersetSecurityManager(SecurityManager): :param schema: The SQL database schema :returns: The rejected tables """ - query = sql_parse.ParsedQuery(sql) + + from superset.sql_parse import Table return { table - for table in query.tables - if not self.can_access_datasource(database, table, schema) + 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] @@ -463,9 +463,7 @@ class SupersetSecurityManager(SecurityManager): from superset import db from superset.connectors.sqla.models import SqlaTable - if hierarchical and ( - self.database_access(database) or self.all_datasource_access() - ): + if hierarchical and self.can_access_database(database): return schemas # schema_access @@ -507,7 +505,7 @@ class SupersetSecurityManager(SecurityManager): from superset import db - if self.database_access(database) or self.all_datasource_access(): + if self.can_access_database(database): return datasource_names if schema: @@ -866,7 +864,7 @@ class SupersetSecurityManager(SecurityManager): :raises SupersetSecurityException: If the user does not have permission """ - if not self.datasource_access(datasource): + if not self.can_access_datasource(datasource): raise SupersetSecurityException( self.get_datasource_access_error_object(datasource), ) diff --git a/superset/views/base.py b/superset/views/base.py index bbf18c1..6c54baf 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -433,7 +433,7 @@ class DeleteMixin: # pylint: disable=too-few-public-methods class DatasourceFilter(BaseFilter): # pylint: disable=too-few-public-methods def apply(self, query: Query, value: Any) -> Query: - if security_manager.all_datasource_access(): + if security_manager.can_access_all_datasources(): return query datasource_perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") diff --git a/superset/views/chart/filters.py b/superset/views/chart/filters.py index 89faa4b..d731930 100644 --- a/superset/views/chart/filters.py +++ b/superset/views/chart/filters.py @@ -25,7 +25,7 @@ from superset.views.base import BaseFilter class SliceFilter(BaseFilter): # pylint: disable=too-few-public-methods def apply(self, query: Query, value: Any) -> Query: - if security_manager.all_datasource_access(): + if security_manager.can_access_all_datasources(): return query perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") diff --git a/superset/views/core.py b/superset/views/core.py index db147f0..fc14d36 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -488,7 +488,7 @@ class Superset(BaseSupersetView): has_access = all( ( - datasource and security_manager.datasource_access(datasource) + datasource and security_manager.can_access_datasource(datasource) for datasource in datasources ) ) @@ -520,7 +520,7 @@ class Superset(BaseSupersetView): datasource = ConnectorRegistry.get_datasource( r.datasource_type, r.datasource_id, session ) - if not datasource or security_manager.datasource_access(datasource): + if not datasource or security_manager.can_access_datasource(datasource): # datasource does not exist anymore session.delete(r) session.commit() @@ -560,7 +560,7 @@ class Superset(BaseSupersetView): return json_error_response(ACCESS_REQUEST_MISSING_ERR) # check if you can approve - if security_manager.all_datasource_access() or check_ownership( + if security_manager.can_access_all_datasources() or check_ownership( datasource, raise_if_false=False ): # can by done by admin only @@ -868,7 +868,7 @@ class Superset(BaseSupersetView): return redirect(error_redirect) if config["ENABLE_ACCESS_REQUEST"] and ( - not security_manager.datasource_access(datasource) + not security_manager.can_access_datasource(datasource) ): flash( __(security_manager.get_datasource_access_error_msg(datasource)), @@ -1874,7 +1874,9 @@ class Superset(BaseSupersetView): if config["ENABLE_ACCESS_REQUEST"]: for datasource in datasources: - if datasource and not security_manager.datasource_access(datasource): + if datasource and not security_manager.can_access_datasource( + datasource + ): flash( __( security_manager.get_datasource_access_error_msg(datasource) @@ -2137,10 +2139,7 @@ class Superset(BaseSupersetView): return json_error_response("Not found", 404) schema = utils.parse_js_uri_path_item(schema, eval_undefined=True) table_name = utils.parse_js_uri_path_item(table_name) # type: ignore - # Check that the user can access the datasource - if not self.appbuilder.sm.can_access_datasource( - database, Table(table_name, schema), schema - ): + if not self.appbuilder.sm.can_access_table(database, Table(table_name, schema)): stats_logger.incr( f"deprecated.{self.__class__.__name__}.select_star.permission_denied" ) @@ -2895,10 +2894,7 @@ class Superset(BaseSupersetView): database = db.session.query(models.Database).filter_by(id=db_id).one() try: schemas_allowed = database.get_schema_access_for_csv_upload() - if ( - security_manager.database_access(database) - or security_manager.all_datasource_access() - ): + if security_manager.can_access_database(database): return self.json_response(schemas_allowed) # the list schemas_allowed should not be empty here # and the list schemas_allowed_processed returned from security_manager diff --git a/superset/views/dashboard/filters.py b/superset/views/dashboard/filters.py index b702fd0..495b824 100644 --- a/superset/views/dashboard/filters.py +++ b/superset/views/dashboard/filters.py @@ -47,7 +47,6 @@ class DashboardFilter(BaseFilter): # pylint: disable=too-few-public-methods datasource_perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") - all_datasource_access = security_manager.all_datasource_access() published_dash_query = ( db.session.query(Dashboard.id) .join(Dashboard.slices) @@ -57,7 +56,7 @@ class DashboardFilter(BaseFilter): # pylint: disable=too-few-public-methods or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), - all_datasource_access, + security_manager.can_access_all_datasources(), ), ) ) diff --git a/superset/views/database/decorators.py b/superset/views/database/decorators.py index 291a1af..071b661 100644 --- a/superset/views/database/decorators.py +++ b/superset/views/database/decorators.py @@ -50,9 +50,8 @@ def check_datasource_access(f: Callable[..., Any]) -> Callable[..., Any]: f"database_not_found_{self.__class__.__name__}.select_star" ) return self.response_404() - # Check that the user can access the datasource - if not self.appbuilder.sm.can_access_datasource( - database, Table(table_name_parsed, schema_name_parsed), schema_name_parsed + if not self.appbuilder.sm.can_access_table( + database, Table(table_name_parsed, schema_name_parsed), ): self.stats_logger.incr( f"permisssion_denied_{self.__class__.__name__}.select_star" diff --git a/superset/views/database/filters.py b/superset/views/database/filters.py index 1941835..6fa9339 100644 --- a/superset/views/database/filters.py +++ b/superset/views/database/filters.py @@ -32,7 +32,7 @@ class DatabaseFilter(BaseFilter): } def apply(self, query: Query, value: Any) -> Query: - if security_manager.all_database_access(): + if security_manager.can_access_all_databases(): return query database_perms = security_manager.user_view_menu_names("database_access") # TODO(bogdan): consider adding datasource access here as well. diff --git a/superset/views/database/forms.py b/superset/views/database/forms.py index 68b3913..b57caff 100644 --- a/superset/views/database/forms.py +++ b/superset/views/database/forms.py @@ -70,10 +70,7 @@ class CsvToDatabaseForm(DynamicForm): b) if database supports schema user is able to upload to schema in schemas_allowed_for_csv_upload """ - if ( - security_manager.database_access(database) - or security_manager.all_datasource_access() - ): + if security_manager.can_access_database(database): return True schemas = database.get_schema_access_for_csv_upload() if schemas and security_manager.get_schemas_accessible_by_user( diff --git a/superset/views/database/validators.py b/superset/views/database/validators.py index dd96a71..a1592a0 100644 --- a/superset/views/database/validators.py +++ b/superset/views/database/validators.py @@ -50,7 +50,4 @@ def schema_allows_csv_upload(database: Database, schema: Optional[str]) -> bool: schemas = database.get_schema_access_for_csv_upload() if schemas: return schema in schemas - return ( - security_manager.database_access(database) - or security_manager.all_datasource_access() - ) + return security_manager.can_access_database(database) diff --git a/tests/core_tests.py b/tests/core_tests.py index e7b4ace..7ea85c7 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -966,15 +966,18 @@ class CoreTests(SupersetTestCase): @mock.patch( "superset.security.SupersetSecurityManager.get_schemas_accessible_by_user" ) - @mock.patch("superset.security.SupersetSecurityManager.database_access") - @mock.patch("superset.security.SupersetSecurityManager.all_datasource_access") + @mock.patch("superset.security.SupersetSecurityManager.can_access_database") + @mock.patch("superset.security.SupersetSecurityManager.can_access_all_datasources") def test_schemas_access_for_csv_upload_endpoint( - self, mock_all_datasource_access, mock_database_access, mock_schemas_accessible + self, + mock_can_access_all_datasources, + mock_can_access_database, + mock_schemas_accessible, ): self.login(username="admin") dbobj = self.create_fake_db() - mock_all_datasource_access.return_value = False - mock_database_access.return_value = False + mock_can_access_all_datasources.return_value = False + mock_can_access_database.return_value = False mock_schemas_accessible.return_value = ["this_schema_is_allowed_too"] data = self.get_json_resp( url="/superset/schemas_access_for_csv_upload?db_id={db_id}".format( diff --git a/tests/security_tests.py b/tests/security_tests.py index 63e15c9..92fccab 100644 --- a/tests/security_tests.py +++ b/tests/security_tests.py @@ -774,45 +774,45 @@ class SecurityManagerTests(SupersetTestCase): Testing the Security Manager. """ - @patch("superset.security.SupersetSecurityManager.datasource_access") - def test_assert_datasource_permission(self, mock_datasource_access): + @patch("superset.security.SupersetSecurityManager.can_access_datasource") + def test_assert_datasource_permission(self, mock_can_access_datasource): datasource = self.get_datasource_mock() # Datasource with the "datasource_access" permission. - mock_datasource_access.return_value = True + mock_can_access_datasource.return_value = True security_manager.assert_datasource_permission(datasource) # Datasource without the "datasource_access" permission. - mock_datasource_access.return_value = False + mock_can_access_datasource.return_value = False with self.assertRaises(SupersetSecurityException): security_manager.assert_datasource_permission(datasource) - @patch("superset.security.SupersetSecurityManager.datasource_access") - def test_assert_query_context_permission(self, mock_datasource_access): + @patch("superset.security.SupersetSecurityManager.can_access_datasource") + def test_assert_query_context_permission(self, mock_can_access_datasource): query_context = Mock() query_context.datasource = self.get_datasource_mock() # Query context with the "datasource_access" permission. - mock_datasource_access.return_value = True + mock_can_access_datasource.return_value = True security_manager.assert_query_context_permission(query_context) # Query context without the "datasource_access" permission. - mock_datasource_access.return_value = False + mock_can_access_datasource.return_value = False with self.assertRaises(SupersetSecurityException): security_manager.assert_query_context_permission(query_context) - @patch("superset.security.SupersetSecurityManager.datasource_access") - def test_assert_viz_permission(self, mock_datasource_access): + @patch("superset.security.SupersetSecurityManager.can_access_datasource") + def test_assert_viz_permission(self, mock_can_access_datasource): test_viz = viz.TableViz(self.get_datasource_mock(), form_data={}) # Visualization with the "datasource_access" permission. - mock_datasource_access.return_value = True + mock_can_access_datasource.return_value = True security_manager.assert_viz_permission(test_viz) # Visualization without the "datasource_access" permission. - mock_datasource_access.return_value = False + mock_can_access_datasource.return_value = False with self.assertRaises(SupersetSecurityException): security_manager.assert_viz_permission(test_viz)