[GitHub] [incubator-superset] john-bodley commented on a change in pull request #7952: [security] Adding docstrings and type hints

[GitBox <gi...@apache.org>]

john-bodley commented on a change in pull request #7952: [security] Adding docstrings and type hints
URL: https://github.com/apache/incubator-superset/pull/7952#discussion_r309389318
 
 

 ##########
 File path: superset/security.py
 ##########
 @@ -124,121 +131,276 @@ class SupersetSecurityManager(SecurityManager):
 
     ACCESSIBLE_PERMS = {"can_userinfo"}
 
-    def get_schema_perm(self, database, schema):
+    def get_schema_perm(
+        self, database: Union["Database", str], schema: Optional[str] = None
+    ) -> Optional[str]:
+        """
+        Return the database specific schema permission.
+
+        :param database: The Superset database or database name
+        :param schema: The Superset schema name
+        :return: The database specific schema permission
+        """
+
         if schema:
-            return "[{}].[{}]".format(database, schema)
+            return f"[{database}].[{schema}]"
+
+        return None
+
+    def can_access(self, permission_name: str, view_name: str) -> bool:
+        """
+        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
+        """
 
-    def can_access(self, permission_name, view_name):
-        """Protecting from has_access failing from missing perms/view"""
         user = g.user
         if user.is_anonymous:
             return self.is_item_public(permission_name, view_name)
         return self._has_view_access(user, permission_name, view_name)
 
     def can_only_access_owned_queries(self) -> bool:
         """
-        can_access check for custom can_only_access_owned_queries permissions.
+        Return True if the user can only access owned queries, False otherwise.
 
-        :returns: True if current user can access custom permissions
+        :returns: Whether the use can only access owned queries
         """
         return self.can_access(
             "can_only_access_owned_queries", "can_only_access_owned_queries"
         )
 
-    def all_datasource_access(self):
+    def all_datasource_access(self) -> bool:
+        """
+        Return True if the user can access all Superset datasources, False otherwise.
+
+        :returns: Whether the user can access all Superset datasources
+        """
+
         return self.can_access("all_datasource_access", "all_datasource_access")
 
-    def all_database_access(self):
+    def all_database_access(self) -> bool:
+        """
+        Return True if the user can access all Superset databases, False otherwise.
+
+        :returns: Whether the user can access all Superset databases
+        """
+
         return self.can_access("all_database_access", "all_database_access")
 
-    def database_access(self, database):
+    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.all_database_access()
+            self.all_datasource_access()
+            or self.all_database_access()
             or self.can_access("database_access", database.perm)
-            or self.all_datasource_access()
         )
 
-    def schema_access(self, datasource):
+    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, i.e., [schema.]datasource, respectively.
+
+        :param datasource: The Superset datasource
+        :returns: Whether the user can access the datasource's schema
+        """
+
         return (
-            self.database_access(datasource.database)
-            or self.all_datasource_access()
+            self.all_datasource_access()
+            or self.database_access(datasource.database)
             or self.can_access("schema_access", datasource.schema_perm)
         )
 
-    def datasource_access(self, datasource):
+    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 use can access the Superset datasource
+        """
+
         return self.schema_access(datasource) or self.can_access(
             "datasource_access", datasource.perm
         )
 
-    def get_datasource_access_error_msg(self, datasource):
-        return """This endpoint requires the datasource {}, database or
-            `all_datasource_access` permission""".format(
-            datasource.name
-        )
+    def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str:
+        """
+        Return the error message for the denied Superset datasource.
+
+        :param datasource: The denied Superset datasource
+        :returns: The error message
+        """
+
+        return f"""This endpoint requires the datasource {datasource.name}, database or
+            `all_datasource_access` permission"""
+
+    def get_datasource_access_link(self, datasource: "BaseDatasource") -> bool:
+        """
+        Return the link for the denied Superset datasource.
+
+        :param datasource: The denied Superset datasource
+        :returns: The access URL
+        """
 
-    def get_datasource_access_link(self, datasource):
         from superset import conf
 
         return conf.get("PERMISSION_INSTRUCTIONS_LINK")
 
-    def get_table_access_error_msg(self, table_name):
-        return """You need access to the following tables: {}, all database access or
-              `all_datasource_access` permission""".format(
-            table_name
-        )
+    def get_table_access_error_msg(self, tables: List[str]) -> str:
+        """
+        Return the error message for the denied SQL tables.
+
+        Note the table names conform to the [[cluster.]schema.]table construct.
+
+        :param tables: The list of denied SQL table names
+        :returns: The error message
+        """
+
+        return f"""You need access to the following tables: {", ".join(tables)}, all
+            database access or `all_datasource_access` permission"""
+
+    def get_table_access_link(self, tables: List[str]) -> str:
+        """
+        Return the access link for the denied SQL tables.
+
+        Note the table names conform to the [[cluster.]schema.]table construct.
+
+        :param tables: The list of denied SQL table names
+        :returns: The access URL
+        """
 
-    def get_table_access_link(self, tables):
         from superset import conf
 
         return conf.get("PERMISSION_INSTRUCTIONS_LINK")
 
-    def datasource_access_by_name(self, database, datasource_name, schema=None):
+    def _datasource_access_by_name(
+        self, database: "Database", table_name: str, schema: str = None
+    ) -> bool:
+        """
+        Return True if the user can access the SQL table, False otherwise.
+
+        :param database: The SQL database
+        :param table_name: The SQL table name
+        :param schema: The Superset schema
+        :returns: Whether the use can access the SQL table
+        """
+
         from superset import db
 
         if self.database_access(database) or self.all_datasource_access():
             return True
 
         schema_perm = self.get_schema_perm(database, schema)
-        if schema and self.can_access("schema_access", schema_perm):
+        if schema_perm and self.can_access("schema_access", schema_perm):
             return True
 
         datasources = ConnectorRegistry.query_datasources_by_name(
-            db.session, database, datasource_name, schema=schema
+            db.session, database, table_name, schema=schema
         )
         for datasource in datasources:
             if self.can_access("datasource_access", datasource.perm):
                 return True
         return False
 
-    def get_schema_and_table(self, table_in_query, schema):
+    def _get_schema_and_table(
+        self, table_in_query: str, schema: str
+    ) -> Tuple[str, str]:
+        """
+        Return the SQL schema/table tuple associated with the table extracted from the
+        SQL query.
+
+        Note the table name conforms to the [[cluster.]schema.]table construct.
+
+        :param table_in_query: The SQL table name
+        :param schema: The fallback SQL schema
+        :returns: The SQL schema/table tuple
+        """
+
         table_name_pieces = table_in_query.split(".")
         if len(table_name_pieces) == 3:
-            return tuple(table_name_pieces[1:])
+            return tuple(table_name_pieces[1:])  # noqa: T484
         elif len(table_name_pieces) == 2:
-            return tuple(table_name_pieces)
+            return tuple(table_name_pieces)  # noqa: T484
         return (schema, table_name_pieces[0])
 
-    def datasource_access_by_fullname(self, database, table_in_query, schema):
-        table_schema, table_name = self.get_schema_and_table(table_in_query, schema)
-        return self.datasource_access_by_name(database, table_name, schema=table_schema)
+    def _datasource_access_by_fullname(
 
 Review comment:
   Note I find the naming of this method somewhat confusing and there's also a method called `_datasource_access_by_name`, i.e, without the `full`. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org