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-51] Dashboard Level Access #10408

Closed
amitmiran137 opened this issue Jul 23, 2020 · 39 comments · Fixed by #12680, #12865 or #12875
Closed

[SIP-51] Dashboard Level Access #10408

amitmiran137 opened this issue Jul 23, 2020 · 39 comments · Fixed by #12680, #12865 or #12875
Labels
enhancement:request Enhancement request submitted by anyone from the community sip Superset Improvement Proposal

Comments

@amitmiran137
Copy link
Member

amitmiran137 commented Jul 23, 2020

Motivation

As a dashboard provider in an organization with many subgroups inside I need the ability manage user access to dashboards and different levels of permissions(read, write, granter, owner)

use cases

  1. In our org we will have hundreds of dashboards that will be based on the same dataset
    Therefore there is no way to manage dashboard access for specific dashboards for specific users
  2. access to dashboard metadata - as a dashboard creator I have sensitive data on the dashboard metadata (iframe, plain HTML, markdown) I want to restrict access to dashboard metadata
  3. current Enterprise BI solutions offer this content-type level of permissions e.g: Tableau
  4. we want to make sure that /dashboard/(/) is also enforcing access. Currently, anyone can access any dashboard by just changing the URL.
    Sometimes there is PII on the dashboard itself like plain HTML text or an iframe so it still exposes sensitive data on the dashboard which is problematic to some of our clients
  5. Airbnb and probably other orgs are fully dependent on dataset level access - they would not handle an extra dashboard level permission
  6. Need to give just dashboard viewing rights and dashboard download rights to users
  7. In certain Enterprises. for example financial services, it is often required to limit the accessibility of data to certain people and to have the ability to manage this centrally. This means that users only have a limited ability to publish results in dashboards to a broader public.]([SIP-55] Extending the security framework to provide data level security #11198)

Proposed Change

By using the RBAC principle and linking roles directly to a dashboard we can enforce an additional layer on top of the existing access mechanism that will permit access to a dashboard if the user has access to any of the Dashboard roles

100% backward compatibility to the existing dashboard security mechanism which is based on datasets

roles linked to a dashboard would provide either read or edit access

development milestones

  1. enforce dashboard security using roles
  2. allow token-based access to /explore_json and API/v1/chart/data to allow reading datasets only by have dashboard access for read-only purpose

Rejected Alternatives

LEVEL_ACCESS_MODE= //options 'Dashboard'/ 'Dataset'
this option doesn't allow both options to co-exist and prevents Dataset access based existing solution to use the new ability
none

@amitmiran137 amitmiran137 added the sip Superset Improvement Proposal label Jul 23, 2020
@issue-label-bot issue-label-bot bot added the enhancement:request Enhancement request submitted by anyone from the community label Jul 23, 2020
@issue-label-bot
Copy link

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

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

@amitmiran137 amitmiran137 changed the title Proposal for Dashboard Access Permissions [SIP] Proposal for Dashboard Access Permissions Jul 23, 2020
@itsik-avidan
Copy link

👍

@amitmiran137
Copy link
Member Author

@villebro the SIP we discussed

@altef
Copy link
Contributor

altef commented Jul 24, 2020

This would be wonderful for my use-case as well

@LiranBri
Copy link

+1

@SaharExelate
Copy link

Cool!

@amitmiran137 amitmiran137 changed the title [SIP] Proposal for Dashboard Access Permissions [SIP-51] Proposal for Dashboard Access Permissions Jul 29, 2020
@yishaifri
Copy link

yes!! cool idea

@willbarrett
Copy link
Member

To comply with the current usage of Admin/Alpha/Gamma, I'd recommend only adding all_dashboard_editor_access to Admin and Alpha - Gamma is currently a much more limited user that requires explicit access to data.

@bkyryliuk
Copy link
Member

Would it make sense to consider automating creation of the datasource permissions while granting access to the dashboard. E.g. user will request the access to the tables that back the dashboard & owner will be able to grant / approve those with 1 click ?

Creating dashboard permissions to the user opens up some edge-cases as there will be 2 ways to validate the access, access to dashboard that can change underlying sql & datasource access [ table, schema, database level]:

  1. owner changes the dashboard and grants access to the undesired
  2. what will be the explore from dashboard behavior for the read only users and how it will interact with the dashboard permissions ?

@amitmiran137
Copy link
Member Author

Hey @bkyryliuk
We plan to automate creation of the datasource permissions while granting access to the dashboard externally to superset by create a matching role for each created dashboard and assigning all the relevant permissions to it(Dashboard, datasources)

I think that explore from dashboard should be available for only dashboard edit access permission and owners

@hyramduke
Copy link

+1

@amitmiran137
Copy link
Member Author

amitmiran137 commented Aug 18, 2020

#5483

@mistercrunch
Copy link
Member

Something to clarify here: what does happen when the user has access to the dashboard, but doesn't have access to the underlying datasets?

If we assume that there's a need for people getting access to a dashboard, but not being able to explore the dataset(s), because say, there's some level of detail in the underlying dataset that the granter does not want to expose for some reason. We need to clarify the following topics:

  • when giving access to a dashboard, we need to make it clear to the granter that there are two options here: dashboard only and dashboard+explore on the datasets
  • for users who have access to dashboard but not the dataset, is the "explore this chart" button grayed out? invisible?

@amitmiran137
Copy link
Member Author

@mistercrunch

  • when giving access to a dashboard, so dashboard itself is accessible but then each one of the chart are doing additional calls to get the underlying data from the datasource, so those calls will also need to be validated that although there is no direct access to the datasource the access to that dashboard is allowing to pull data for the need of viewing it inside that specific dashboard

Giving access to a specific dashboard can be sufficient enough for the user to view the dashboard without the complexicity of adding all underlying datasources 👍 🚀

  • for users who have access to dashboard but not the dataset, the explore chart should be invisible to simplify dashboard viewers to my opinion same as edit dashboard should be invisible

@amitmiran137 amitmiran137 changed the title [SIP-51] Proposal for Dashboard Access Permissions [SIP-51] Proposal for Dashboard Level Access Sep 18, 2020
@john-bodley
Copy link
Member

john-bodley commented Sep 23, 2020

Currently only owners + admins are able to edit dashboards. I'm interested in knowing whether the current view access and ownership controls are suffice in order to provide the "viewer" and "editor" functions?

This seems like a far simpler approach as is consistent with the slice model and requires no code change.

@bkyryliuk
Copy link
Member

Currently only owners + admins are able to edit dashboards. I'm interested in knowing whether the current view access and ownership controls are suffice in order to provide the "viewer" and "editor" functions?

This seems like a far simpler approach as is consistent with the slice model and requires no code change.

agree with @john-bodley proposal, it looks like current permission model supports this use case.

@john-bodley
Copy link
Member

john-bodley commented Sep 23, 2020

I partially misspoke earlier, currently there are no access controls explicitly at the dashboard level, it's merely a series of rules.

I do think the community needs to collectively decide whether security should be at i) the datasource level (either Superset datasource or the underlying database, schema, etc.), the ii) chart/dashboard level, or iii) a combination of both (i) and (ii). Currently it's (i) (for right or wrong) and aspects of dashboard level access could be achieved by row level access and/or dashboard specific Superset datasources. There is additional overhead with this approach, however it's simpler to grok, the access patterns are likely more secure (people could exploit dashboard level access controls), and doesn't require additional logic or development of request/approval/management flow.

@amitmiran137
Copy link
Member Author

Hey @john-bodley just noticed this PR merged #11024
it actually fully covers the dashboard_edit_access permission concept so I think I'm going to change the proposal to focus only on access in the general concept of it

@amitmiran137
Copy link
Member Author

This Flag can be introduced at the global level for a default behaviour and at the dashboard level for overriding the default

@ktmud

@ktmud
Copy link
Member

ktmud commented Dec 22, 2020

@amitmiran137 If users can switch this flag at the dashboard level, it means at least part of the system will contain dataset-based access control. Will dashboards with LEVEL_ACCESS_MODE= 'Dashboard' enabled still subject to any dataset level access control configured? If yes, what's the benefit of having a flag? Can we just: 1). always apply the dataset level control to the charts; 2) apply the dashboard-level access control when it is configured per dashboard (could be default to the dashboard creators' own groups)?

@amitmiran137
Copy link
Member Author

amitmiran137 commented Dec 22, 2020

@ktmud
dashboards with LEVEL_ACCESS_MODE= 'Dashboard' enabled will not be subject to any dataset level access.

The problem with option 2 : you want to give viewing access and not ownership for managing the dashboard

How would you suggest to manage list of users have view access to a dashboard ?
Isn't a list of users equals to a role ?
Why create a new access model instead of leveraging the RBAC existing one

@ktmud
Copy link
Member

ktmud commented Dec 22, 2020

dashboards with LEVEL_ACCESS_MODE= 'Dashboard' enabled will not be subject to any dataset level access.

This will be a major turn off for any organization who needs dataset level access control.

My 1) and 2) were not two options but two steps of one solution. Basically we keep current dataset level access control unchanged but add a new layer of dashboard access control.

In terms of actual implementation, you could still leverage the existing RBAC by adding an roles attribute to dashboards and a custom can_access_dashboard/can_edit_dashboard method to SecurityManager:

  1. Add a new column roles to the Dashboard model, which stores FAB roles that corresponds to a business unit/user group's dashboard view or edit role. We don't allow specifying view access by users as it unnecessarily complicates things.
  2. When publishing a dashboard, users choose which roles/user groups have access to this dashboard
  3. Add dashboard_access to OBJECT_SPEC_PERMISSIONS
  4. Add can_access_dashboard and can_edit_dashboard to SecurityManager which passes the right permission names and view names to can_access based on roles associated with the dashboard. E.g.
    if not dashboard.roles:
        return True
    for role in dashboard.roles:
        if self.can_access("dashboard_access", dashboard.perm_for_role(role, edit=False)):
            return True
    return False
  5. Place these checks manually in the API, just like what we do for datasources.

In short, I don't think an "access mode" switch is necessary, as current security model seems to already suffice in supporting the additional layer of role-based dashboard-level access control and the only extra work is adding a new roles column (like we already have in datasource.perm and datasource.schema_perm, except we add roles instead of perm to have the ability to enforce foreign key check.)

There could be a toggle to allow pulling query results from controlled datasource even if users don't have direct access to datasource---but that has its own level of complexity and doesn't seem to be blocking us from implementing the basic dashboard-level access control.

This is obviously a popular user demand and I'm all for addressing it, but let's make sure the final solution is as prudent as possible.

@junlincc
Copy link
Member

#9725

@rajraousb
Copy link

rajraousb commented Jan 4, 2022

Hi all, I have an alternative solution here: #17914 and #17913
Can you review and provide comments?

Thanks

@shefaliaiyanna
Copy link

How do we give access to external users via superset to view the dashboard without the localhost link. How do I make it a generic URL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment