You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jo...@apache.org on 2020/06/11 20:12:45 UTC
[incubator-superset] branch master updated: chore(security):
Renaming access methods (#10031)
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 <45...@users.noreply.github.com>
AuthorDate: Thu Jun 11 13:12:23 2020 -0700
chore(security): Renaming access methods (#10031)
Co-authored-by: John Bodley <jo...@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)