Skip to content

Add Filter on DatabaseView that filters DBs Based on Role Access #7618

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

Merged
mistercrunch merged 2 commits into from
Jun 5, 2019
Merged

Add Filter on DatabaseView that filters DBs Based on Role Access #7618

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
c79a1cf
Add Filter on DatabaseView that filters DBs Based on Role Access
dflionis May 29, 2019
61d4032
Update with mistercrunch's feedback
dflionis Jun 5, 2019
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: 4 additions & 5 deletions superset/security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,11 @@ def all_datasource_access(self):
return self.can_access(
'all_datasource_access', 'all_datasource_access')

def all_database_access(self):

@dflionis dflionis May 31, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A different contributor came along and added some tests to this module, but since the tests were failing and it was after Maxime's approval, I did not attempt to port them over. Happy to try to write tests if there is demand for adding them.

return self.can_access('all_database_access', 'all_database_access')

@dflionis dflionis May 31, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the original PR, this was sending a user keyword argument. However, can_access() does not currently accept a keyword arg so I removed it


def database_access(self, database):
return (
self.can_access(
'all_database_access', 'all_database_access') or
self.can_access('database_access', database.perm)
)
return self.can_access('database_access', database.perm)

@mistercrunch mistercrunch Jun 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we make this:
return self.all_database_access() or self.can_access('database_access', database.perm) ?

@mistercrunch mistercrunch Jun 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wondering if we should also checkout for all_datasource_access (which is redundant with all_database_access) here for backwards compatibility reasons. Say if someone setup a local role Users with access to all data that has all_datasource_access, I feel like they should see all databases when opening SQL Lab.

@mistercrunch mistercrunch Jun 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW I feel like I wrote/merged this same PR a few months back but got reverted because of a perm issue (Alpha couldn't see the database list in SQL Lab).

PR that got reverted:
#7009

Related fix:
#7271

@dflionis dflionis Jun 5, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking this out--the PR has been updated. Regarding the last comment about a similar PR having gone in and gotten reverted, please let me know if that changes anything about how I should structure this PR and I would be happy to make the changes.


def schema_access(self, datasource):
return (
Expand Down
9 changes: 9 additions & 0 deletions superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,14 @@ def apply(self, query, func): # noqa
return query.filter(self.model.perm.in_(perms))


class DatabaseFilter(SupersetFilter):
def apply(self, query, func): # noqa
if security_manager.all_database_access():
return query
perms = self.get_view_menus('database_access')
return query.filter(self.model.perm.in_(perms))


class DashboardFilter(SupersetFilter):

"""List dashboards for which users have access to at least one slice or are owners"""
Expand Down Expand Up @@ -287,6 +295,7 @@ class DatabaseView(SupersetModelView, DeleteMixin, YamlExportMixin): # noqa
'allow_csv_upload': _(
'If selected, please set the schemas allowed for csv upload in Extra.'),
}
base_filters = [['id', DatabaseFilter, lambda: []]]
label_columns = {
'expose_in_sqllab': _('Expose in SQL Lab'),
'allow_ctas': _('Allow CREATE TABLE AS'),
Expand Down