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

Jump to bottom

[BUGFIX]: Fixing permission rollup database/schema to table in SupersetFilter #4004

Closed
wants to merge 1 commit into from
Closed

[BUGFIX]: Fixing permission rollup database/schema to table in SupersetFilter #4004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 56 additions & 22 deletions superset/views/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@
FRONTEND_CONF_KEYS = ('SUPERSET_WEBSERVER_TIMEOUT',)


ALL_DATASOURCE_ACCESS = 'all_datasource_access'
ALL_DATABASE_ACCESS = 'all_database_access'
DATABASE_ACCESS = 'database_access'
SCHEMA_ACCESS = 'schema_access'
DATASOURCE_ACCESS = 'datasource_access'


def get_error_msg():
if conf.get('SHOW_STACKTRACE'):
error_msg = traceback.format_exc()
Expand Down Expand Up @@ -76,26 +83,26 @@ def can_access(self, permission_name, view_name, user=None):

def all_datasource_access(self, user=None):
return self.can_access(
'all_datasource_access', 'all_datasource_access', user=user)
ALL_DATASOURCE_ACCESS, ALL_DATASOURCE_ACCESS, user=user)

def database_access(self, database, user=None):
return (
self.can_access(
'all_database_access', 'all_database_access', user=user) or
self.can_access('database_access', database.perm, user=user)
ALL_DATABASE_ACCESS, ALL_DATABASE_ACCESS, user=user) or
self.can_access(DATABASE_ACCESS, database.perm, user=user)
)

def schema_access(self, datasource, user=None):
return (
self.database_access(datasource.database, user=user) or
self.all_datasource_access(user=user) or
self.can_access('schema_access', datasource.schema_perm, user=user)
self.can_access(SCHEMA_ACCESS, datasource.schema_perm, user=user)
)

def datasource_access(self, datasource, user=None):
return (
self.schema_access(datasource, user=user) or
self.can_access('datasource_access', datasource.perm, user=user)
self.can_access(DATASOURCE_ACCESS, datasource.perm, user=user)
)

def datasource_access_by_name(
Expand All @@ -104,13 +111,13 @@ def datasource_access_by_name(
return True

schema_perm = utils.get_schema_perm(database, schema)
if schema and self.can_access('schema_access', schema_perm):
if schema and self.can_access(SCHEMA_ACCESS, schema_perm):
return True

datasources = ConnectorRegistry.query_datasources_by_name(
db.session, database, datasource_name, schema=schema)
for datasource in datasources:
if self.can_access('datasource_access', datasource.perm):
if self.can_access(DATASOURCE_ACCESS, datasource.perm):
return True
return False

Expand Down Expand Up @@ -138,7 +145,7 @@ def user_datasource_perms(self):
for perm in r.permissions:
if (
perm.permission and
'datasource_access' == perm.permission.name):
DATASOURCE_ACCESS == perm.permission.name):
datasource_perms.add(perm.view_menu.name)
return datasource_perms

Expand All @@ -149,7 +156,7 @@ def schemas_accessible_by_user(self, database, schemas):
subset = set()
for schema in schemas:
schema_perm = utils.get_schema_perm(database, schema)
if self.can_access('schema_access', schema_perm):
if self.can_access(SCHEMA_ACCESS, schema_perm):
subset.add(schema)

perms = self.user_datasource_perms()
Expand All @@ -173,7 +180,7 @@ def accessible_by_user(self, database, datasource_names, schema=None):

if schema:
schema_perm = utils.get_schema_perm(database, schema)
if self.can_access('schema_access', schema_perm):
if self.can_access(SCHEMA_ACCESS, schema_perm):
return datasource_names

user_perms = self.user_datasource_perms()
Expand Down Expand Up @@ -299,13 +306,16 @@ def get_user_roles(self):
return get_user_roles()

def get_all_permissions(self):
"""Returns a set of tuples with the perm name and view menu name"""
perms = set()
"""Returns a map of view menu name to permission name"""
permissions = dict()
for role in self.get_user_roles():
for perm_view in role.permissions:
t = (perm_view.permission.name, perm_view.view_menu.name)
perms.add(t)
return perms
for perm in role.permissions:
view_name = perm.view_menu.name
perm_name = perm.permission.name
perm_set = permissions.get(view_name, set())
perm_set.add(perm_name)
permissions[perm.view_menu.name] = perm_set
return permissions

def has_role(self, role_name_or_list):
"""Whether the user has this role name"""
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

It should be possible to avoid doing 2 database roundtrips when loading the slices or dashboard lists. If that doesn't work the second best option is to @memoize this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can probably refactor and get rid of one call.

Copy link
Contributor Author

@fabianmenges fabianmenges Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So this is not easy, for the DatasourceFilter I can refactor to something like this:

        permissions = self.get_all_permissions()
        return query.filter(or_(
          self.model.perm.in_(permissions(DATASOURCE_ACCESS)),
          self.model.schema_perm.in_(permissions(SCHEMA_ACCESS)),
          self.model.database.perm.in_(permissions(DATABASE_ACCESS)),
        ))

But that does not work for slices, since slices don't have a foreign key relationship to the datasource they reference (because we have different tables for Druid and SQL datasources).

If I'm memoizing the call (lets say for datasources_with_access), I have to handle cache invalidation when a new datasources is added. Cache invalidation does not seem to be implemented right now so I don't really want to pen that can of worms for this fix.

I think the best I can do is refactor the DatasourceFilter and leave the SliceFilter and DasboardFilter as is.

Copy link
Member

Choose a reason for hiding this comment

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

It should be possible even without the FK, may be tricky though. You'll probably have to left join to LEFT JOIN to both tables and look for whether the it joins to either.

permissions = self.get_all_permissions()
if (ALL_DATASOURCE_ACCESS in permissions.get(
Copy link
Member

Choose a reason for hiding this comment

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

@fabianmenges per my comment in #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.

ALL_DATASOURCE_ACCESS, {}) or
ALL_DATABASE_ACCESS in permissions.get(
ALL_DATABASE_ACCESS, {})):
return list(datasources)
else:
filtered = list()
for ds in datasources:
if (ds.database and
DATABASE_ACCESS in permissions.get(
ds.database.perm, {})):
filtered.append(ds)
if (ds.schema_perm
and SCHEMA_ACCESS in permissions.get(ds.schema_perm, {})):
filtered.append(ds)
if DATASOURCE_ACCESS in permissions.get(ds.perm, {}):
filtered.append(ds)
return filtered


class DatasourceFilter(SupersetFilter):

def apply(self, query, func): # noqa
if self.has_all_datasource_access():
return query
perms = self.get_view_menus('datasource_access')
# TODO(bogdan): add `schema_access` support here

perms = [d.perm for d in self.datasources_with_access()]
return query.filter(self.model.perm.in_(perms))


Expand Down
6 changes: 2 additions & 4 deletions superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,7 @@ class SliceFilter(SupersetFilter):
def apply(self, query, func): # noqa
if self.has_all_datasource_access():
return query
perms = self.get_view_menus('datasource_access')
# TODO(bogdan): add `schema_access` support here
perms = [d.perm for d in self.datasources_with_access()]
return query.filter(self.model.perm.in_(perms))


Expand All @@ -143,8 +142,7 @@ def apply(self, query, func): # noqa
return query
Slice = models.Slice # noqa
Dash = models.Dashboard # noqa
# TODO(bogdan): add `schema_access` support here
datasource_perms = self.get_view_menus('datasource_access')
datasource_perms = [d.perm for d in self.datasources_with_access()]
slice_ids_qry = (
db.session
.query(Slice.id)
Expand Down