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.

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

Add permissions decider for delegating access controls. #4409

Closed

Conversation

timifasubaa
Copy link
Contributor

@timifasubaa timifasubaa commented Feb 13, 2018

This PR creates the perms_decider object in the config which gives different deployments the ability to delegate data access control with the
is_eligible_datasource() which decides if the perms_decider can handle access decisions for a datasource and
is_allowed_access() which decides if the user has access.

I also added in a case where we consult the perms decider before loading from the cache. With the default options, the cache flow doesn't change since is_eligible_datasource returns False.

@fabianmenges @john-bodley @mistercrunch

@timifasubaa timifasubaa force-pushed the add_permissions_decider branch 3 times, most recently from dc60303 to a722485 Compare February 13, 2018 00:21
@fabianmenges
Copy link
Contributor

@jeffreythewang @GeorgeSirois FYI


# System to handle delegated data access. Implement both is_allowed_access() and
# is_eligible_datasource() to delegate access controls.
class PermsDecider:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that be a static class (not meant to be instantiated)? Also note that in py2 it's always preferable to derive object as in class PermsDecider(object):.

Also this base class should probably be declared outside of config.py (maybe security.py)?

What about simplifying the interface with a simple has_datasource_access(user, datasource) callable that only applies if it's defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that it should be a static class.
The concern is that if it is defined outside of config.py, there's no way to delegate access decisions to a system outside of superset.

Since perms_decider might only apply to a subset of databases ot datasources, If we use has_datasource_access, it's not clear what to return when the datasource in question is out of the jurisdiction of the perms_decider.

Copy link
Member

Choose a reason for hiding this comment

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

The interface could be such that has_datasource_access could return None to say that it's out of its jurisdiction.

def has_datasource_access(user, datasource):
    if in_my_jurisdiction(datasource):
        return has_access_according_to_me(user, datasource)
    return None

Copy link
Contributor Author

@timifasubaa timifasubaa Feb 13, 2018

Choose a reason for hiding this comment

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

But None is falsey and so it will be easy to mix up access denied with I don't know enough to decide for this datasource (from this db)

superset/viz.py Outdated
@@ -307,7 +307,12 @@ def get_df_payload(self, query_obj=None):
cached_dttm = datetime.utcnow().isoformat().split('.')[0]
if cache_key and cache and not self.force:
cache_value = cache.get(cache_key)
if cache_value:
perms_decider = config.get('DATA_PERMS_DECIDER')
perms_decider_disapproves = (
Copy link
Member

Choose a reason for hiding this comment

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

viz.py doesn't currently handle authorization, I'd rather avoid scattering the auth logic further across modules. Seems like ideally we'd have most security-related logic in security.py and have views/base.py wrap that up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In our case, we don't want to provide info from the cache based on superset permissions. Rather from an outside system. I moved the check from viz.py to explore_json. and set force=true when the perms_decider denied the person access.

@timifasubaa timifasubaa force-pushed the add_permissions_decider branch from a722485 to aaa8398 Compare February 13, 2018 22:04
@timifasubaa timifasubaa force-pushed the add_permissions_decider branch from aaa8398 to e5f3d05 Compare February 13, 2018 22:45
@john-bodley
Copy link
Member

An alternative solution would be to refactor the datasource permissions to be handled by an abstract security class. The current FAB logic could be be transferred to a derived class (which would be the default implementation), which would allows others to implement their own security model, bypassing FAB completely for datasource access if desired.

@timifasubaa
Copy link
Contributor Author

Superceeded by #4565 and following PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants