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

[WiP] [SIP-19] Simplify Superset's set of permissions #7510

Open
mistercrunch opened this issue May 15, 2019 · 1 comment

Comments

Projects
None yet
1 participant
@mistercrunch
Copy link
Contributor

commented May 15, 2019

[SIP-19] Simplify Superset's permissions

Motivation

First let's clarify that this is about feature-related permissions, as opposed to data-access permissions. Also note that what I'll refer to as an "atomic permission" is a combination of what FAB calls a "view_menu" and a "permission".

Also note that this change will be powered by recently released FAB features enabling more control over permission definition, as well as migration tools making it straightforward to map and migrate existing role from old to new permission. More information about related FAB features can be found here:
https://flask-appbuilder.readthedocs.io/en/latest/security.html#permission-customization

The bulk of feature-related permissions are dynamically generated by Flask App Builder (FAB). For context, FAB generates:

  • a set of permission (can_show, can_list, can_add, can_download, can_delete, can_update, can_muldelete ) for each ModelView. Typically each model has one or many ModelViews
  • a perm for each view method (commonly referred to as "endpoint")
  • a perm for each menu entry

Now Superset overtime has grown to ship with ~280+ permissions. Most of these permissions are unintelligible to users, the UI that exposes them suffers from too much options. In the current state, it almost only makes sense to generate roles programmatically since the cardinality of permissions is so high.

While we want the atomicity of permissions to cover most use cases, we want for permissions to be easy to document and reason about. In cases where it would be unreasonable to have one permission and not another closely related one, we'd like to merge them as a single permission.

Proposed Change

First, group ModelView-related permissions into 2 simple permissions: read and write, based on this mapping rule:

    method_permission_name = {
        'add': 'write',
        'delete': 'write',
        'download': 'write',
        'edit': 'write',
        'list': 'read',
        'muldelete': 'write',
        'show': 'read',
    }

This assumes that if you can edit, you can also delete or add (write). Similarly if you can read you can show or list.

Second, rename and group ModelView names. For clarity drop the "ModelView" suffix and match the Model's name. For examples, DashboardModelView view_menu becomes Dashboard for permission purposes. This in turns takes care of the secondary ModelView derivatives like DashboardAsyncModelView and groups it with other Dashboard-related ModelView for permission-related purposes.

Third, models that are related and tightly coupled, for example every Models living around the connectors should refer to the same set of permissions. DruidDatasource, SqlaTables, and their respective Metric and Column models can all go under Datasource permissions. Either you can read or write on Datasource or you don't. No one needs to be able to edit metrics but unable to edit columns or datasource property.

This results in the following mappings (Google Spreadsheet):
https://docs.google.com/spreadsheets/d/1VzBRUsrf_aMS_QkMXoIaLwpETfJmH3qms3kePywBESI/edit?usp=sharing

Additionally the notion of can_write_owned will be added [only] for models implementing the idea of ownership, like Dashboard, Chart and Datasource. This logic is currently implemented in different places in the code and should be standardized. Implementation details aren't clear at this point, but may come in the form of Model and ModelView mixin classes.

Unclear is whether can_delete should be part of can_write! This needs to be [DISCUSSed]

New or Changed Public Interfaces

While Alpha and Gamma will be migrated and effectively the same, and existing roles converged, there may be existing scripts that would be incompatible with this new world.

New dependencies

N/A

Migration Plan and Compatibility

Users will have to run flask fab security-converge as part of releasing the new version including this feature.

Related future work

  • mapping views (endpoint) permission to existing ModelView-related permissions. For instance if we had an existing view permissions called "Superset - delete_dashboards", we could map it to the ModelView atomic permission Dashboard - can_write

Ongoing work

#7501

@mistercrunch mistercrunch added the #SIP label May 15, 2019

@issue-label-bot

This comment has been minimized.

Copy link

commented May 15, 2019

Issue-Label Bot is automatically applying the label #enhancement to this issue, with a confidence of 0.86. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.