[SIP-19] Simplify Superset's set of permissions #18383
Replies: 15 comments
-
Issue-Label Bot is automatically applying the label Links: app homepage, dashboard and code for this bot. |
Beta Was this translation helpful? Give feedback.
-
This will much simplify Superset Permissions nightmare |
Beta Was this translation helpful? Give feedback.
-
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. |
Beta Was this translation helpful? Give feedback.
-
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 |
Beta Was this translation helpful? Give feedback.
-
+1 from my side as well, these changes look really great! |
Beta Was this translation helpful? Give feedback.
-
+1 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 |
Beta Was this translation helpful? Give feedback.
-
Superset permission management now is really so complex and hard to grasp. I suggest that we can refer to metabase for permission management. |
Beta Was this translation helpful? Give feedback.
-
@mistercrunch did you want to put this up for a VOTE? |
Beta Was this translation helpful? Give feedback.
-
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
Beta Was this translation helpful? Give feedback.
-
As a new Superset admin, this simplification would be great 👍 |
Beta Was this translation helpful? Give feedback.
-
[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:
can_show
,can_list
,can_add
,can_download
,can_delete
,can_update
,can_muldelete
) for eachModelView
. Typically each model has one or many ModelViewsModelView
methodNow 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:
permission_view
, composed of:view_menu
: often representing an object or a classpermission
: often representing an action or a methodAgain for context, on top of that Superset adds data-access-related permissions. One for each
database
,schema
anddataset
in the system.Proposed Change
First, group ModelView-related permissions into 2 simple permissions:
read
andwrite
, based on this mapping rule:This assumes that if you can edit, you can also delete or add (
write
). Similarly if you canread
you canshow
orlist
.Second, rename and group ModelView names. For clarity drop the "ModelView" suffix and match the Model's name. For examples,
DashboardModelView
view_menu becomesDashboard
for permission purposes. This in turns takes care of the secondary ModelView derivatives likeDashboardAsyncModelView
and groups it with otherDashboard
-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 onDatasource
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 viewSuperset.save_dashboard
can be attached toDashboard.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
andQuery
have a notion of ownership as defined by having a many-to-many relationship to ourUser
model under aowners
relationship. This acts as an implicit restrictive modifier to thewrite
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 aOwnershipModelMixin
would help formalizing this permission-related pattern and logic.Remaining Questions
can_delete
should be part ofcan_write
!?SecurityManager
out there? how to communicate about itNew or Changed Public Interfaces
While
Alpha
andGamma
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 - #7501
Beta Was this translation helpful? Give feedback.
All reactions