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

[SIP-126] Fine-grained access control to Superset entities #28021

Closed
villebro opened this issue Apr 13, 2024 · 18 comments
Closed

[SIP-126] Fine-grained access control to Superset entities #28021

villebro opened this issue Apr 13, 2024 · 18 comments
Labels
sip Superset Improvement Proposal

Comments

@villebro
Copy link
Member

villebro commented Apr 13, 2024

Motivation

For the main entities, like charts and dashboards, Superset employs an implicit access control model, where access is granted if the user has access to the underlying datasource. While this makes sense from a data access perspective (=you shouldn't be able to view a chart if you can't access the underlying table), it has the following drawbacks:

  • Users that have access to lots of datasources face a lot of clutter in the form of seeing all charts and dashboards
  • It's impossible to publish a dashboard only to user A if user B also has access to the same underlying datasource. This is especially problematic in a context where multiple orgs have access to the same database, but analyze the data from different perspectives. Think sales vs internal audit, where it can be very problematic if salespeople see how internal audit is tracking their activity.

In addition, other than via Roles, Superset doesn't have a concept of groups that multiple users can belong to. This makes it difficult to manage collective ownership of assets, as each asset has to be explicitly owned by users. While SIP-51 Dashboard Level Access proposed a solution for this, it utilized the Role model for non-permission based access controls, which is a deviation from what FAB Roles are originally intended for. This is also the case for SIP-29 Row Level Security, where roles are being used due to the absence of pure non-permission based groups.

This user-centric access control model becomes particularly challenging in large organizations, where users routinely flow in and out of teams, or become absent, like when a user resigns. In these cases an admin usually has to step in and manually re-assign ownership of an asset to another team member. For this reason, it would be practical to be able to assign ownership of assets to a group or groups, rather to one or many users.

Proposed Change

To introduce granular access controls to Superset entities, and clarify access controls, we propose the following changes:

1. Introduce concept of Editors and Viewers

These new properties will replace the current "Owner" property and implicit access model as follows:

  • Editor: similar to Owners, Editors are able to change the objects.
  • Viewer: Viewers are able to view, but not change entities.

The properties are added to the following models:

  • Dashboard
  • Chart
  • Alerts & Reports
  • Row Level Security

For Alerts & Reports and RLS we only introduce Editor for now. The list views are updated to only show entities that the user has access to, either via Editors or Viewers.

2. Add a Group model

The Group, similar to the Role model, will make it possible to group multiple users together. This can be used to create arbitrary groups, e.g. "Finance", "Marketing", "USA", "Finland" etc. Typically, membership to these would be synced from some upstream source, like LDAP, and id token or similar.

3. Add a Subject model

The Editor and Viewer properties would reference a new Subject model, which in turn would reference one of three models:

  • User
  • Group
  • Role

The Editor/Viewer dropdowns would thus make it possible to assign editorship/viewership not only to Users, but also Groups or Roles. By default, only Users and Groups would be displayed in the dropdowns, but for backward compatibility with DASHBOARD_RBAC, it would be possible to also reference Roles as a subject. Exposing Roles would be done via a query filter (disabled by default).

New or Changed Public Interfaces

  • The Subject model is introduced, that has an entry for each User, Group and Role
  • The affected models (dashboard, chart etc) introduce new reference fields for editors and viewers that are linked to the Subject model via link tables.
  • A new React component is introduced that references the Subject model, which makes it possible to choose Subjects for the Editor and Viewer properties.

Migration Plan and Compatibility

  1. A database migration would introduce the new Subject, Group and link tables that connect the entity tables to the Subject table.
  2. A cli command would be introduced that migrates existing Owners to the new Editors property. This populates the Editors property with the current set of Owners, and populates the Viewers property with the Roles that have access to the entity via the current implicit access model.
  3. A SQLAlchemy event listener would keep the Subject model in sync with the User, Grooup and Role tables whenever changes are made to those entities.
  4. A feature flag is introduced, that replaces the current access controls with the new ones.
  5. The old implicit access control model is deprecated.
  6. In a forthcoming major release (I assume 6.0), the old implicit model is fully removed, and the new Subject-based model is assumed as the new access control model. The published flag is removed from dashboards, after which any user that has access via the Viewer property is assumed to be able to view the dashboard.

Rejected Alternatives

We considered enabling the DASHBOARD_RBAC flag. However, this didn't solve the ownership issue. Also, the Role model is not designed for non-permission based groups. Therefore, it felt more intuitive to introduce a new model that can be directly linked with existing enterprise ACL solutions, like LDAP.

Screenshots

It would be possible to assign subjects (Users, Roles or Groups) to both the Viewers and Editors property. Notice, how both users and groups can be referenced:

image

On the list view, you would only be able to see the entities that you have access to, either via the Editor or Viewer property:

image
@villebro villebro added the sip Superset Improvement Proposal label Apr 13, 2024
@villebro
Copy link
Member Author

villebro commented Apr 13, 2024

Btw, I already have a working branch for this, so happy to open it up as a PR for y'all to test (I just need to rebase it).

@villebro villebro changed the title [SIP]-126 Fine-grained access controls to Superset entities [SIP]-126 Fine-grained access control to Superset entities Apr 13, 2024
@rusackas
Copy link
Member

By default, only Users and Groups would be displayed in the dropdowns, but for backward compatibility with DASHBOARD_RBAC, it would be possible to also reference Roles as a subject

Would it be the DASHBOARD_RBAC flag that enables this option? If so, would that flag and the role-based viewership be deprecated as well?

@rusackas
Copy link
Member

cli command would be introduced that migrates existing Owners to the new Editors property

I assume both models will be supported for now then, and this CLI would be removed as part of the 5.0 deprecation and 6.0 removal plan? Then we would force the migration in 6.0?

@rusackas
Copy link
Member

I agree that Users and Groups seems sufficient... Role-based access seems like an unnecessary complication, if there's a way out of supporting that. 🤔

@villebro
Copy link
Member Author

I agree that Users and Groups seems sufficient... Role-based access seems like an unnecessary complication, if there's a way out of supporting that. 🤔

Yes, this feature is a superset of Dashboard RBAC, So we would deprecate and ultimately remove it in a fortcoming major release.

@villebro
Copy link
Member Author

cli command would be introduced that migrates existing Owners to the new Editors property

I assume both models will be supported for now then, and this CLI would be removed as part of the 5.0 deprecation and 6.0 removal plan? Then we would force the migration in 6.0?

Yes

@villebro
Copy link
Member Author

By default, only Users and Groups would be displayed in the dropdowns, but for backward compatibility with DASHBOARD_RBAC, it would be possible to also reference Roles as a subject

Would it be the DASHBOARD_RBAC flag that enables this option? If so, would that flag and the role-based viewership be deprecated as well?

Yes, that's the plan

@mistercrunch
Copy link
Member

As discussed this morning:

  • overall we'd like to consider a federated model for all RBAC/ABAC, including data access (database, catalog, schema, tables, RLS) and objects (Dashboard, Alert & Reports, ...)
  • this includes per-object, or per "object pattern" rules as opposed to entity-based rules like we have now (can_update Alerts)
  • centralize all rule-checking to a central authority through a consistent model
  • bring out of FAB and into Superset

Some TODO:

  • review existing frameworks like Casbin and opa to see whether they'd be viable/desirable
  • define entities, hierarchy and related actions required in V1 - has to be forward compatible, covering per object on(Database, Schema, Dataset, RLS, Dashboard) + CRUD on all other models (FAB allows this currently).

@villebro villebro changed the title [SIP]-126 Fine-grained access control to Superset entities [SIP-126] Fine-grained access control to Superset entities Apr 16, 2024
@villebro
Copy link
Member Author

Thanks for the summary @mistercrunch 👍 I hadn't thought of looking into existing libraries/frameworks for the ABAC system. I'll look into these. I also agree on getting started on the modelling to make this work for the use cases identified in SIPs 125 and 126, along with future proofing it without stepping on FAB's toes.

@mistercrunch
Copy link
Member

Wondering if it's easier to alter this SIP or start over from scratch to make sure everything is captured. Let me give it a quick shot with GPT here:


SIP-127: Federated Model for RBAC/ABAC/ACL in Apache Superset

Motivation

Current implementations of RBAC/ABAC in Apache Superset are managed through various models and permissions systems, primarily leveraging Flask-AppBuilder (FAB) for app-level permissions. This split approach introduces complexity in managing data access permissions and results in user interface clutter, inefficient permissions synchronization, and scaling issues. A unified, centralized policy manager is proposed to address these challenges by consolidating all aspects of RBAC, ABAC, and ACL within Superset.

Goals

  • Federate all access control mechanisms under a single, centralized policy manager within Superset.
  • Replace the rule-checking mechanisms currently utilizing FAB with a native, more scalable and flexible Superset system.
  • Ensure forward compatibility and extensibility for a wide range of entities and actions.

Proposed Changes

1. Central Policy Manager

  • Implement a centralized authority within Superset for all access controls, replacing the current systems that depend on FAB.
  • Define and manage permissions using a structured, normalized model for granular control from databases to rows.

2. Hierarchy-Aware Access Control

  • The policy manager will inherently understand and utilize the hierarchical nature of data access:
    • Atomic Entities: Each entity like Dashboard, Database, Schema, Dataset, and RLS will be considered an atomic unit.
    • Hierarchies: Entities will be organized in a hierarchy, with explicit parent-child relationships where access permissions can inherit or override based on the hierarchy.
    • Verbs/Actions: Define specific actions (CRUD for app models and binary for data access) applicable at each level of the hierarchy.
    • Set-Definitions/Pattern-matching: Create sets or groups of entities, either as named groups or based on pattern/attribute matching
    • User groups: Bring user groups as a core concept. Roles are collections of groups and/or individual users

3. Integration with Existing Frameworks

  • Evaluate and integrate with frameworks like Casbin and OPA for backend policy management.

4. Migration from FAB

  • Transition from FAB-based controls to the new centralized policy manager while ensuring backward compatibility.

5. User Interface Enhancements

  • Develop a new UI to support the centralized, hierarchy-aware policy manager.

6. Implementation Details

  • Detail the required database schema changes.
  • Develop API endpoints and internal methods for the policy manager.
  • Implement synchronization mechanisms to maintain data integrity across user and group changes.

Migration Plan and Compatibility

  • Comprehensive database migration strategies will be implemented to shift existing permissions to the new model.
  • CLI tools will be provided to help admins migrate permissions and verify the integrity of the new system.

Rejected Alternatives

  • Continued use of FAB for data access controls: Rejected due to lack of scalability and complexity.
  • Separate models for each entity type and permission: Considered too fragmented and challenging to manage.

Request for Comments

  • Integration of the groups model: Should it remain in FAB or be integrated into Superset?
  • Should we use a single secondary table with an entity field for different objects, or separate tables per object?
  • Use of UUIDs for easier rule migration and management.
  • Considerations for direct vs. inherited access in the hierarchy.
  • The feasibility of adding more sophisticated permissions management, such as conditional access based on attributes.

This SIP aims to significantly overhaul how permissions are managed in Apache Superset, simplifying the user experience, enhancing security, and providing a framework that can scale effectively in large and dynamic environments.

@mistercrunch
Copy link
Member

mistercrunch commented Apr 16, 2024

GPT did a decent first pass at merging both, @villebro @michael-s-molina feel free to take it form here, though I feel like we should meet again soon

@villebro
Copy link
Member Author

Thanks @mistercrunch , I think this is moving in a great direction! As the federated security model is a big chunk to digest on its own, I would prefer to keep the fine-grained access control SIP separate from the proposed SIP for a federated security model. This to make sure we can let interested parties review both SIPs in isolation, without having to fully grasp both. Having said that, I'm completely committed to tackling the requirements of the new security model when implementing the fine-grained access control feature. I can open up the ABAC/RBAC/ACL SIP, and make sure these stay in sync.

Thoughts @mistercrunch @michael-s-molina ?

@michael-s-molina
Copy link
Member

GPT did a decent first pass at merging both, @villebro @michael-s-molina feel free to take it form here, though I feel like we should meet again soon

The merging is great @mistercrunch! I also think we should sync again. Maybe even a weekly meeting until we have everything figured out.

I would prefer to keep the fine-grained access control SIP separate from the proposed SIP for a federated security model. This to make sure we can let interested parties review both SIPs in isolation, without having to fully grasp both.

@villebro I think it's important to analyze all the requirements together, think about all uses cases, and then start with a simple implementation. When we design the API, the hierarchy of resources, the types of actions we'll support, how we are going to store this information, we need to be able to test our design against all requirements to make sure the consolidated policy manager will work. That being said, we can definitely discuss splitting the SIPs in our next meeting if it's beneficial for reviewers.

@mistercrunch
Copy link
Member

One thing is to do an assessment and maybe rationalization/simplification of the current set of permissions. Maybe a matrix of ViewMenu/Permission in FAB on a clean install would be a nice place to start.

@villebro
Copy link
Member Author

GPT did a decent first pass at merging both, @villebro @michael-s-molina feel free to take it form here, though I feel like we should meet again soon

The merging is great @mistercrunch! I also think we should sync again. Maybe even a weekly meeting until we have everything figured out.

Agreed, a weekly for now would make sense, as this effort will likely otherwise stall due to to its complexity.

I would prefer to keep the fine-grained access control SIP separate from the proposed SIP for a federated security model. This to make sure we can let interested parties review both SIPs in isolation, without having to fully grasp both.

@villebro I think it's important to analyze all the requirements together, think about all uses cases, and then start with a simple implementation. When we design the API, the hierarchy of resources, the types of actions we'll support, how we are going to store this information, we need to be able to test our design against all requirements to make sure the consolidated policy manager will work. That being said, we can definitely discuss splitting the SIPs in our next meeting if it's beneficial for reviewers.

I agree, for the implementation of the new security system, they do need to be assessed together. However, I feel it's important to give room to discuss this new direction of fine-grained entity-level access controls from a pure usability and governance perspective, as many people will likely not be interested, or have the expertise, to go deep into the new security policy proposal.

@rusackas @mistercrunch @michael-s-molina would on of you be able to setup a weekly sync for this? I would otherwise, but I don't have a Zoom account to attach to it. The same time slot we had last time should usually work for me, except next Monday, when I could join at 11:30 am PST.

@rusackas
Copy link
Member

Sent out an invite... DM me if the time is an issue and we'll sort it out.

@rusackas
Copy link
Member

Should we close this out in anticipation of the new/revised one?

@villebro
Copy link
Member Author

@rusackas sure, I'll try to get the new one started today, I think we got all the necessary requirements mapped yesterday in the sync meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sip Superset Improvement Proposal
Projects
Status: Denied / Closed / Discarded
Development

No branches or pull requests

4 participants