[GitHub] john-bodley commented on a change in pull request #4004: [BUGFIX]: Fixing permission rollup database/schema to table in SupersetFilter

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

john-bodley commented on a change in pull request #4004: [BUGFIX]: Fixing permission rollup database/schema to table in SupersetFilter
URL: https://github.com/apache/incubator-superset/pull/4004#discussion_r178601736
 
 

 ##########
 File path: superset/views/base.py
 ##########
 @@ -316,28 +326,52 @@ def has_role(self, role_name_or_list):
 
     def has_perm(self, permission_name, view_menu_name):
         """Whether the user has this perm"""
-        return (permission_name, view_menu_name) in self.get_all_permissions()
+        return permission_name in self.get_all_permissions().get(
+          view_menu_name, {})
 
     def get_view_menus(self, permission_name):
         """Returns the details of view_menus for a perm name"""
         vm = set()
-        for perm_name, vm_name in self.get_all_permissions():
-            if perm_name == permission_name:
+        for vm_name, perm_names in list(self.get_all_permissions().items()):
+            if permission_name in perm_names:
                 vm.add(vm_name)
         return vm
 
     def has_all_datasource_access(self):
         return (
             self.has_role(['Admin', 'Alpha']) or
-            self.has_perm('all_datasource_access', 'all_datasource_access'))
+            self.has_perm(ALL_DATASOURCE_ACCESS, ALL_DATASOURCE_ACCESS))
+
+    def datasources_with_access(self):
+        datasources = ConnectorRegistry.get_all_datasources(db.session)
+        permissions = self.get_all_permissions()
+        if (ALL_DATASOURCE_ACCESS in permissions.get(
 
 Review comment:
   @fabianmenges per my comment in https://github.com/apache/incubator-superset/issues/4737 I'm concerned that this could make the `IN` clause contain thousands of perm strings depending on the number of entities in database. That said if a role has a whitelist of databases one must enumerate all the datasources in it.
   
   A possible performance improvement would for the case of a role where it contains either `all_datasource_access` or `all_database_access` to return `None` (or `True`) to signify that no additional filter is required (as opposed from listing every single datasource) for a given database. One would have to augment the logic in `query.filter(self.model.perm.in_(perms))` to be database specific and included only when neither of `all_datasource_access` or `all_database_access` are present.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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