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] Proposal for controlling View Only access to Dashboards #17914

Closed
rajraousb opened this issue Jan 3, 2022 · 15 comments
Closed

[SIP] Proposal for controlling View Only access to Dashboards #17914

rajraousb opened this issue Jan 3, 2022 · 15 comments
Labels
sip Superset Improvement Proposal

Comments

@rajraousb
Copy link

rajraousb commented Jan 3, 2022

Motivation

Conditions for showing Dashboards in the UI
• Dashboards for which the current user is the creator
• Dashboards for which the current user is part of "Co-owner" as defined in the Dashboard creation/edit
• Dashboards which are published
There is no option available to share Dashboard with only Read Only capability

Proposed Change

This SIP aims to provide a new set of users called Viewers
• Provide capability to have Viewers field similar to Owners field
• Viewers will be able to see the Dashboards in their account when they click on Dashboards panel
• Viewers will have only Read-Only access (Co-owners have Read-Write access)
New or Changed Public Interfaces
• Add a new table similar to "Dashboard Owners", called "Dashboard Viewers". These tables are internal Superset database tables.
• This table creation can be created in the "superset db upgrade" hook
• Add UI component called "Viewers", which will populate "Dashboard Viewers" table based on dashboard_id and user_id of viewers selected from UI
• Add handlers for Viewers in UI (e.g. label, description, composite combo box etc.)
• Modify Dashboard model to add Viewers as extra attribute of "Dashboard Viewers" type linking dashboard and user table
• We will be enhancing the capability that we implemented for Co-Owner where in we will automatically add access to the underlying charts associated withe Dashboard to the Viewers

See Document:
SIP Viewers.pdf

New dependencies

New database called dashboard_viewers

Migration Plan and Compatibility

Create a db upgrade file

Rejected Alternatives

N/A

@betodealmeida
Copy link
Member

@rajraousb do you mind formatting the post with Markdown headers, to make it easier to read? Also, instead of attaching a .docx file please include the text as Markdown directly in the description, since people might not have tools to open a proprietary Word document.

Note that Superset supports dashboard-specific access roles (#10408) via the DASHBOARD_RBAC feature flag, which seem to cover at least some of the use cases you list here.

@rajraousb
Copy link
Author

Picture1

@rajraousb
Copy link
Author

r1@r1server:~/ss35/superset-0.35$ diff superset/views/core.py ../../orig/superset-0.35/superset/views/core.py
247d246
<       4. Those which the user has Viewer access
289,294d287
<         viewer_ids_query = (
<             db.session.query(Dash.id)
<             .join(Dash.viewers)
<             .filter(User.id == User.get_user_id())
<         )
<
298d290
<                 Dash.id.in_(viewer_ids_query),
485,486d476
<
500,501c490
<         "owners",                                    
<         "viewers",
---
>         "owners",
507,508d495
<
537,541d523
<         "viewers": _("Viewers is a list of users who can view the dashboard."),
<         "published": _(
<             "Determines whether or not this dashboard is "
<             "visible in the list of all dashboards"
<         ),
543d524
550,551c531
<         "owners": _("Owners"),
<         "viewers": ("Viewers"),
---
>         "owners": _("Owners"),


New dependencies

New database called dashboard_viewers

r1@r1server:~/ss35/superset-0.35$ diff superset/models/core.py ../../orig/superset-0.35/superset/models/core.py
411,420d410
< dashboard_viewer = Table(
<     "dashboard_viewer",
<     metadata,
<     Column("id", Integer, primary_key=True),
<     Column("user_id", Integer, ForeignKey("ab_user.id")),
<     Column("dashboard_id", Integer, ForeignKey("dashboards.id")),
< )
<
<
<
435d424
<     viewers =  relationship(security_manager.user_model, secondary=dashboard_viewer)

Migration Plan and Compatibility

"""dashboard_viewers

Revision ID: b922b18232ab
Revises: 2f1d15e8a6af
Create Date: 2021-09-30 05:29:03.108813

"""

# revision identifiers, used by Alembic.
revision = 'b922b18232ab'
down_revision = '2f1d15e8a6af'

from alembic import op
import sqlalchemy as sa


def upgrade():
    op.create_table(
        "dashboard_viewers",
        sa.Column("id", sa.Integer(), nullable=False),
        sa.Column("user_id", sa.Integer(), nullable=True),
        sa.Column("dashboard_id", sa.Integer(), nullable=True),
        sa.ForeignKeyConstraint(["dashboard_id"], ["dashboards.id"]),
        sa.ForeignKeyConstraint(["user_id"], ["ab_user.id"]),
        sa.PrimaryKeyConstraint("id"),
    )


def downgrade():
    op.drop_table("dashboard_viewers")


@rajraousb rajraousb changed the title [SIP] Proposal for controlling access to Dashboards [SIP] Proposal for controlling View Only access to Dashboards Jan 3, 2022
@amitmiran137
Copy link
Member

amitmiran137 commented Jan 4, 2022

Hey @rajraousb thank you for taking the time to plan and design this.

Would you mind taking a look in #10408
And see if that proposal answer the requirements that you stated in this proposal

If that is the case so go ahead and use it

If not let me know what do you think is still missing

@rajraousb
Copy link
Author

rajraousb commented Jan 4, 2022

Hi @amitmiran137 Thanks for the comment. 10408 is at role level.
Generally, all users in a particular team will have same role(s). However within the same team, there might be a need to have e.g. user1 have Owner role, user2 as Co-Owner, user3 & user4 as just Viewers.
Creating a role just for this purpose and assigning is an administrative overhead.
Also, let's say we need to drop user4, this again needs a role change.
Also, this fits in seamlessly with the Dashboard creation/editing framework where we define co-owners rather than a separate role creation task.

An easier solution is to expand the same logic as in co-owners which I have described in the proposal. Users can easily add/remove other users from Viewers and/or Owners list.

Also, it's easy to populate a particular user's dashboard with all Dashboards for which they are Owners/Viewers.

This will also help in #17913 where other users can be restricted access as well.

By providing granularity at user level, it is possible to define access to each dashboard for specific users (like the Owners list)

@amitmiran137
Copy link
Member

amitmiran137 commented Jan 4, 2022

In general I think we should address data access control in some kind of a 2.0 version and generify all access management into a unified solution

Here in this comment is the proposal we have tried pushing for a while now.

is address access both on a user/role levels with the exact same mechanisem
#16557 (comment)

cc:
@betodealmeida @villebro @dpgaspar

@amitmiran137
Copy link
Member

amitmiran137 commented Jan 4, 2022

we have tried doing a full blown solution for a RBAC to any object in superset that can later be expanded to support individual users but got held back due to some issues:
#17057

@rajraousb
Copy link
Author

rajraousb commented Jan 4, 2022

That is good for future versions, Amit. Looking forward to that.

However for all the older versions we can use a solution like this, eliminating need for feature flags or specific roles per dashboard and also easier migration & management etc.

This solution can work in 0.3x, 1.x versions and potentially others too.

@amitmiran137
Copy link
Member

But you are referring to a solution that needs development and will only be supportrd in new releases

@BinRoq
Copy link

BinRoq commented Jan 13, 2022

@has_access
@expose("/dashboard/<dashboard_id>/")
def dashboard(self, dashboard_id):
    """Server side rendering for a dashboard"""

    def check_owner_or_viewer(obj):
    #See if current user has either owner or viewer permission

        if not obj:
            return False

        if g.user.is_anonymous:
            return False

        roles = [r.name for r in get_user_roles()]
        if "Admin" in roles:
            return True

        owners = []
        owners += obj.owners

        owners += obj.viewers

        owner_names = [o.username for o in owners if o]


        if g.user and hasattr(g.user, "username") and g.user.username in owner_names:
            return True



        return False


    session = db.session()
    qry = session.query(models.Dashboard)
    if dashboard_id.isdigit():
        qry = qry.filter_by(id=int(dashboard_id))
    else:
        qry = qry.filter_by(slug=dashboard_id)

    dash = qry.one_or_none()
    if not dash:
        abort(404)


    if check_owner_or_viewer( dash ) == False:
        bootstrap_data = {
            "user_id": g.user.get_id(),

            "user_name": g.user.username,
            "user.first_name": g.user.first_name,
            "user.last_name": g.user.last_name,

            "dashboard_id": dash.id,
            "dashboard_title": dash.dashboard_title,
            "error": "Need either Owner or Viewer privilege to view this dashboard",
        }


        flash(__("You have no permission to view this dashboard"), "danger")

        return json_success(json.dumps(bootstrap_data))




    datasources = set()
    for slc in dash.slices:
        datasource = slc.datasource
        if datasource:
            datasources.add(datasource)

@rajraousb
Copy link
Author

rajraousb commented Jan 18, 2022

@BinRoq
The code you pasted is from my proposal #17913 where we can use the Viewers feature to restrict access to Dashboards
Any comments/suggestions?

@stale
Copy link

stale bot commented Apr 17, 2022

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 .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 17, 2022
@rajraousb
Copy link
Author

any plan to implement similar for version 2.x?

@stale stale bot removed the inactive Inactive for >= 30 days label Apr 19, 2022
@rajraousb
Copy link
Author

rajraousb commented Oct 21, 2022

Thanks for all your comments.
This feature is implemented in latest version (I checked 1.3+) using RBAC role field.
Users who are in the ROLE group will have Viewer access.

@rusackas
Copy link
Member

Closing this out since the author noted it's implemented! Hooray! If there's a need to re-open it for further discussion or dialing-in, we certainly can.

@rusackas rusackas added the sip Superset Improvement Proposal label Jun 7, 2023
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

5 participants