Revert "chore(security): Renaming access methods (apache#10031)"

This reverts commit 9532bff.

UPDATING.md

@@ -23,9 +23,6 @@ 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`.

superset/charts/filters.py

@@ -45,7 +45,7 @@ def apply(self, query: Query, value: Any) -> Query:
 
 class ChartFilter(BaseFilter):  # pylint: disable=too-few-public-methods
     def apply(self, query: Query, value: Any) -> Query:
-        if security_manager.can_access_all_datasources():
+        if security_manager.all_datasource_access():
             return query
         perms = security_manager.user_view_menu_names("datasource_access")
         schema_perms = security_manager.user_view_menu_names("schema_access")

superset/dashboards/filters.py

@@ -62,6 +62,7 @@ def apply(self, query: Query, value: Any) -> Query:
 
         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)
@@ -71,7 +72,7 @@ def apply(self, query: Query, value: Any) -> Query:
                     or_(
                         Slice.perm.in_(datasource_perms),
                         Slice.schema_perm.in_(schema_perms),
-                        security_manager.can_access_all_datasources(),
+                        all_datasource_access,
                     ),
                 )
             )

superset/security/manager.py

@@ -188,14 +188,15 @@ def unpack_schema_perm(self, schema_permission: str) -> Tuple[str, str]:
 
     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 user can access the FAB permission/view
+        :returns: Whether the use can access the FAB permission/view
         """
 
         user = g.user
@@ -205,14 +206,13 @@ def can_access(self, permission_name: str, view_name: str) -> bool:
 
     def can_access_all_queries(self) -> bool:
         """
-        Return True if the user can access all SQL Lab queries, False otherwise.
+        Return True if the user can access all queries, False otherwise.
 
         :returns: Whether the user can access all queries
         """
-
         return self.can_access("all_query_access", "all_query_access")
 
-    def can_access_all_datasources(self) -> bool:
+    def all_datasource_access(self) -> bool:
         """
         Return True if the user can access all Superset datasources, False otherwise.
 
@@ -221,7 +221,7 @@ def can_access_all_datasources(self) -> bool:
 
         return self.can_access("all_datasource_access", "all_datasource_access")
 
-    def can_access_all_databases(self) -> bool:
+    def all_database_access(self) -> bool:
         """
         Return True if the user can access all Superset databases, False otherwise.
 
@@ -230,47 +230,47 @@ def can_access_all_databases(self) -> bool:
 
         return self.can_access("all_database_access", "all_database_access")
 
-    def can_access_database(self, database: "Database") -> bool:
+    def database_access(self, database: "Database") -> bool:
         """
         Return True if the user can access the Superset database, False otherwise.
 
         :param database: The Superset database
         :returns: Whether the user can access the Superset database
         """
         return (
-            self.can_access_all_datasources()
-            or self.can_access_all_databases()
+            self.all_datasource_access()
+            or self.all_database_access()
             or self.can_access("database_access", database.perm)
         )
 
-    def can_access_schema(self, datasource: "BaseDatasource") -> bool:
+    def schema_access(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 respectively, i.e., [schema.]datasource.
+        and datasource name prefix, i.e., [schema.]datasource, respectively.
 
         :param datasource: The Superset datasource
         :returns: Whether the user can access the datasource's schema
         """
-
+        schema_perm = datasource.schema_perm or ""
         return (
-            self.can_access_all_datasources()
-            or self.can_access_database(datasource.database)
-            or self.can_access("schema_access", datasource.schema_perm or "")
+            self.all_datasource_access()
+            or self.database_access(datasource.database)
+            or self.can_access("schema_access", schema_perm)
         )
 
-    def can_access_datasource(self, datasource: "BaseDatasource") -> bool:
+    def datasource_access(self, datasource: "BaseDatasource") -> bool:
         """
         Return True if the user can access the Superset datasource, False otherwise.
 
         :param datasource: The Superset datasource
-        :returns: Whether the user can access the Superset datasource
+        :returns: Whether the use can access the Superset datasource
         """
-
-        return self.can_access_schema(datasource) or self.can_access(
-            "datasource_access", datasource.perm or ""
+        perm = datasource.perm or ""
+        return self.schema_access(datasource) or self.can_access(
+            "datasource_access", perm
         )
 
     def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str:
@@ -356,27 +356,30 @@ def get_table_access_link(self, tables: Set["Table"]) -> Optional[str]:
 
         return conf.get("PERMISSION_INSTRUCTIONS_LINK")
 
-    def can_access_table(self, database: "Database", table: "Table") -> bool:
+    def can_access_datasource(
+        self, database: "Database", table: "Table", schema: Optional[str] = None
+    ) -> bool:
         """
         Return True if the user can access the SQL table, False otherwise.
 
         :param database: The SQL database
         :param table: The SQL table
-        :returns: Whether the user can access the SQL table
+        :param schema: The fallback SQL schema if not present in the table
+        :returns: Whether the use can access the SQL table
         """
 
         from superset import db
         from superset.connectors.sqla.models import SqlaTable
 
-        if self.can_access_database(database):
+        if self.database_access(database) or self.all_datasource_access():
             return True
 
-        schema_perm = self.get_schema_perm(database, schema=table.schema)
+        schema_perm = self.get_schema_perm(database, schema=table.schema or 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
+            db.session, database, table.table, schema=table.schema or schema
         )
         for datasource in datasources:
             if self.can_access("datasource_access", datasource.perm):
@@ -394,15 +397,12 @@ def rejected_tables(
         :param schema: The SQL database schema
         :returns: The rejected tables
         """
-
-        from superset.sql_parse import Table
+        query = sql_parse.ParsedQuery(sql)
 
         return {
             table
-            for table in sql_parse.ParsedQuery(sql).tables
-            if not self.can_access_table(
-                database, Table(table.table, table.schema or schema)
-            )
+            for table in query.tables
+            if not self.can_access_datasource(database, table, schema)
         }
 
     def get_public_role(self) -> Optional[Any]:  # Optional[self.role_model]
@@ -463,7 +463,9 @@ def get_schemas_accessible_by_user(
         from superset import db
         from superset.connectors.sqla.models import SqlaTable
 
-        if hierarchical and self.can_access_database(database):
+        if hierarchical and (
+            self.database_access(database) or self.all_datasource_access()
+        ):
             return schemas
 
         # schema_access
@@ -505,7 +507,7 @@ def get_datasources_accessible_by_user(
 
         from superset import db
 
-        if self.can_access_database(database):
+        if self.database_access(database) or self.all_datasource_access():
             return datasource_names
 
         if schema:
@@ -864,7 +866,7 @@ def assert_datasource_permission(self, datasource: "BaseDatasource") -> None:
         :raises SupersetSecurityException: If the user does not have permission
         """
 
-        if not self.can_access_datasource(datasource):
+        if not self.datasource_access(datasource):
             raise SupersetSecurityException(
                 self.get_datasource_access_error_object(datasource),
             )

superset/views/base.py

@@ -433,7 +433,7 @@ def muldelete(self: BaseView, items: List[Model]) -> FlaskResponse:
 
 class DatasourceFilter(BaseFilter):  # pylint: disable=too-few-public-methods
     def apply(self, query: Query, value: Any) -> Query:
-        if security_manager.can_access_all_datasources():
+        if security_manager.all_datasource_access():
             return query
         datasource_perms = security_manager.user_view_menu_names("datasource_access")
         schema_perms = security_manager.user_view_menu_names("schema_access")

superset/views/chart/filters.py

@@ -25,7 +25,7 @@
 
 class SliceFilter(BaseFilter):  # pylint: disable=too-few-public-methods
     def apply(self, query: Query, value: Any) -> Query:
-        if security_manager.can_access_all_datasources():
+        if security_manager.all_datasource_access():
             return query
         perms = security_manager.user_view_menu_names("datasource_access")
         schema_perms = security_manager.user_view_menu_names("schema_access")

superset/views/core.py

@@ -488,7 +488,7 @@ def request_access(self) -> FlaskResponse:
 
         has_access = all(
             (
-                datasource and security_manager.can_access_datasource(datasource)
+                datasource and security_manager.datasource_access(datasource)
                 for datasource in datasources
             )
         )
@@ -520,7 +520,7 @@ def clean_fulfilled_requests(session: Session) -> None:
                 datasource = ConnectorRegistry.get_datasource(
                     r.datasource_type, r.datasource_id, session
                 )
-                if not datasource or security_manager.can_access_datasource(datasource):
+                if not datasource or security_manager.datasource_access(datasource):
                     # datasource does not exist anymore
                     session.delete(r)
             session.commit()
@@ -560,7 +560,7 @@ def clean_fulfilled_requests(session: Session) -> None:
             return json_error_response(ACCESS_REQUEST_MISSING_ERR)
 
         # check if you can approve
-        if security_manager.can_access_all_datasources() or check_ownership(
+        if security_manager.all_datasource_access() or check_ownership(
             datasource, raise_if_false=False
         ):
             # can by done by admin only
@@ -868,7 +868,7 @@ def explore(
             return redirect(error_redirect)
 
         if config["ENABLE_ACCESS_REQUEST"] and (
-            not security_manager.can_access_datasource(datasource)
+            not security_manager.datasource_access(datasource)
         ):
             flash(
                 __(security_manager.get_datasource_access_error_msg(datasource)),
@@ -1874,9 +1874,7 @@ def dashboard(self, dashboard_id_or_slug: str) -> FlaskResponse:
 
         if config["ENABLE_ACCESS_REQUEST"]:
             for datasource in datasources:
-                if datasource and not security_manager.can_access_datasource(
-                    datasource
-                ):
+                if datasource and not security_manager.datasource_access(datasource):
                     flash(
                         __(
                             security_manager.get_datasource_access_error_msg(datasource)
@@ -2139,7 +2137,10 @@ def select_star(
             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
-        if not self.appbuilder.sm.can_access_table(database, Table(table_name, schema)):
+        # Check that the user can access the datasource
+        if not self.appbuilder.sm.can_access_datasource(
+            database, Table(table_name, schema), schema
+        ):
             stats_logger.incr(
                 f"deprecated.{self.__class__.__name__}.select_star.permission_denied"
             )
@@ -2895,7 +2896,10 @@ def schemas_access_for_csv_upload(self) -> FlaskResponse:
         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.can_access_database(database):
+            if (
+                security_manager.database_access(database)
+                or security_manager.all_datasource_access()
+            ):
                 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

superset/views/dashboard/filters.py

@@ -47,6 +47,7 @@ def apply(self, query: Query, value: Any) -> Query:
 
         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)
@@ -56,7 +57,7 @@ def apply(self, query: Query, value: Any) -> Query:
                     or_(
                         Slice.perm.in_(datasource_perms),
                         Slice.schema_perm.in_(schema_perms),
-                        security_manager.can_access_all_datasources(),
+                        all_datasource_access,
                     ),
                 )
             )

superset/views/database/decorators.py

@@ -50,8 +50,9 @@ def wraps(
                 f"database_not_found_{self.__class__.__name__}.select_star"
             )
             return self.response_404()
-        if not self.appbuilder.sm.can_access_table(
-            database, Table(table_name_parsed, schema_name_parsed),
+        # 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
         ):
             self.stats_logger.incr(
                 f"permisssion_denied_{self.__class__.__name__}.select_star"

superset/views/database/filters.py

@@ -32,7 +32,7 @@ def schema_access_databases(self) -> Set[str]:  # noqa pylint: disable=no-self-u
         }
 
     def apply(self, query: Query, value: Any) -> Query:
-        if security_manager.can_access_all_databases():
+        if security_manager.all_database_access():
             return query
         database_perms = security_manager.user_view_menu_names("database_access")
         # TODO(bogdan): consider adding datasource access here as well.

superset/views/database/forms.py

@@ -70,7 +70,10 @@ def at_least_one_schema_is_allowed(database: Database) -> bool:
                 b) if database supports schema
                     user is able to upload to schema in schemas_allowed_for_csv_upload
         """
-        if security_manager.can_access_database(database):
+        if (
+            security_manager.database_access(database)
+            or security_manager.all_datasource_access()
+        ):
             return True
         schemas = database.get_schema_access_for_csv_upload()
         if schemas and security_manager.get_schemas_accessible_by_user(

superset/views/database/validators.py

@@ -50,4 +50,7 @@ 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.can_access_database(database)
+    return (
+        security_manager.database_access(database)
+        or security_manager.all_datasource_access()
+    )

tests/core_tests.py

@@ -986,18 +986,15 @@ def test_slice_payload_no_datasource(self):
     @mock.patch(
         "superset.security.SupersetSecurityManager.get_schemas_accessible_by_user"
     )
-    @mock.patch("superset.security.SupersetSecurityManager.can_access_database")
-    @mock.patch("superset.security.SupersetSecurityManager.can_access_all_datasources")
+    @mock.patch("superset.security.SupersetSecurityManager.database_access")
+    @mock.patch("superset.security.SupersetSecurityManager.all_datasource_access")
     def test_schemas_access_for_csv_upload_endpoint(
-        self,
-        mock_can_access_all_datasources,
-        mock_can_access_database,
-        mock_schemas_accessible,
+        self, mock_all_datasource_access, mock_database_access, mock_schemas_accessible
     ):
         self.login(username="admin")
         dbobj = self.create_fake_db()
-        mock_can_access_all_datasources.return_value = False
-        mock_can_access_database.return_value = False
+        mock_all_datasource_access.return_value = False
+        mock_database_access.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(