Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make filters use security manager #5567

Merged
merged 2 commits into from
Aug 16, 2018
Merged

make filters use security manager #5567

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a581bc6
make filters use security manager
timifasubaa Aug 5, 2018
8be397a
remove the superset short-circuit
timifasubaa Aug 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions superset/security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,15 @@ def get_schema_perm(self, database, schema):
if schema:
return '[{}].[{}]'.format(database, schema)

def always_list_all_dashboards(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your comment that we should defer to the security manager to answer said question, however I’m not sure we need these always_ methods.

return False

def always_list_all_slices(self):
return False

def always_list_all_datasources(self):
return False

def can_access(self, permission_name, view_name, user=None):
"""Protecting from has_access failing from missing perms/view"""
if not user:
Expand Down
8 changes: 2 additions & 6 deletions superset/views/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,15 +248,11 @@ def get_view_menus(self, permission_name):
vm.add(vm_name)
return vm

def has_all_datasource_access(self):
return (
self.has_role(['Admin', 'Alpha']) or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does removing these role checks potentially break things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only three usage of the has_all_datasource_access and they are the ones I changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the link to where all_datasource_access is granted to Alpha.
Note that Admin is granted all the possible priviledges, which includes those contained in Alpha.
Also linked is the line in the security manager that checks the user has all_datasource_access

https://github.com/apache/incubator-superset/blob/4bf69a7260f1d4c8fbfe9d9c0300cae39ca3bf81/superset/security.py#L71

https://github.com/apache/incubator-superset/blob/4bf69a7260f1d4c8fbfe9d9c0300cae39ca3bf81/superset/security.py#L96

self.has_perm('all_datasource_access', 'all_datasource_access'))


class DatasourceFilter(SupersetFilter):
def apply(self, query, func): # noqa
if self.has_all_datasource_access():
if (security_manager.all_datasource_access() or
security_manager.always_list_all_datasources()):
return query
perms = self.get_view_menus('datasource_access')
# TODO(bogdan): add `schema_access` support here
Expand Down
6 changes: 4 additions & 2 deletions superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ def check_ownership(obj, raise_if_false=True):

class SliceFilter(SupersetFilter):
def apply(self, query, func): # noqa
if self.has_all_datasource_access():
if (security_manager.all_datasource_access() or
security_manager.always_list_all_slices()):
return query
perms = self.get_view_menus('datasource_access')
# TODO(bogdan): add `schema_access` support here
Expand All @@ -148,7 +149,8 @@ class DashboardFilter(SupersetFilter):
"""List dashboards for which users have access to at least one slice or are owners"""

def apply(self, query, func): # noqa
if self.has_all_datasource_access():
if (security_manager.all_datasource_access() or
security_manager.always_list_all_dashboards()):
return query
Slice = models.Slice # noqa
Dash = models.Dashboard # noqa
Expand Down