Incubator-superset: [SIP-19] Simplify Superset's set of permissions

[mistercrunch]

[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 not-so-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

Currently, 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 ModelView method
• 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/administrators/humans. The UI that exposes them suffers from too much options that are not documented. In the current state, it almost only makes sense to generate roles programmatically since the cardinality of permissions is so high, and many organizations do that.

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.

Context

For context, FAB's idea of RBAC has the following entities:

• a role
• with a many-to-many relationship to a permission_view, composed of:
  
  
  
  • a single view_menu: often representing an object or a class
  
  
  • a single permission: often representing an action or a method
  
  

Again for context, on top of that Superset adds data-access-related permissions. One for each database, schema and dataset in the system.

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.

Fourth, looking at our custom endpoints, and attempting to get them to piggy back on the existing "object-action" existing ones defined by the rules set above around the consolidate ModelView-perms. This means that the view Superset.save_dashboard can be attached to Dashboard.write.

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

Note that some models like Dashboard, Chart and Query have a notion of ownership as defined by having a many-to-many relationship to our User model under a owners relationship. This acts as an implicit restrictive modifier to the write permission on a model, where only owners are able to alter objects. Perhaps this is out-of-scope for this SIP, but clarifying this pattern with a OwnershipModelMixin would help formalizing this permission-related pattern and logic.

Remaining Questions

• is can_delete should be part of can_write!?
• how do we role this out?
• change-management, can we trust FAB's perm-reallocation logic?
• How does this affect custom SecurityManager out there? how to communicate about it

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.

Ongoing work

POC here - https://github.com/apache/incubator-superset/pull/7501

[issue-label-bot[bot]]

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

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

[aboganas]

This will much simplify Superset Permissions nightmare

[andy-clapson]

Huge +1 here - managing Superset permissions is a headache. This simplification would be very welcome - in favour of keeping things simple, being able to write implying ability to delete makes sense to me.

[mistercrunch]

Rollout strategy and change management are the more complex topics here. @dpgaspar can you talk about how FAB does the perm reassignement / merging? For instance if I have a role that contains both Dashboard - can_update and Dashboard - can_delete and they get rolled up into a new Dashboard - write, how is FAB going to merge things? Logicial OR? Logical AND?

[elukey]

+1 from my side as well, these changes look really great!

[bkyryliuk]

+1
I have a preference to have can_delete as a separate permission

I would be also looking forward read and write access separation for the data access, that potentially could be out of scope for this SIP

[isunix]

Superset permission management now is really so complex and hard to grasp. I suggest that we can refer to metabase for permission management.

[always-busy]

approve

[annakepler]

+1