fix(dashboard rbac): make a dashboard with no role assigned accessible only when PUBLISHED instead of DRAFT

[giovannipapini-agilelab]

SUMMARY

Minor bug fix on dashboard RBAC faulty behaviour

TESTING INSTRUCTIONS

See issue #22640

ADDITIONAL INFORMATION

• Fixes Dashboard RBAC access doesn't conform to documentation (and access permission exposes a possible security risk) #22640
• Has associated issue:
• Required feature flags: 'DASHBOARD_RBAC' = True
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️
We hope to see you in our Slack community too!

[frabenetti]

We have the same issue and I see this PR will solve the problem, I tested the modification in a docker in versione 2.0.1.

[frabenetti]

Any news about the approval of this PR?

[codecov]

Codecov Report

Merging #22642 (7a4beaf) into master (30dab3a) will decrease coverage by 9.86%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #22642      +/-   ##
==========================================
- Coverage   65.66%   55.79%   -9.87%     
==========================================
  Files        1859     1859              
  Lines       71093    71096       +3     
  Branches     7777     7777              
==========================================
- Hits        46680    39667    -7013     
- Misses      22388    29404    +7016     
  Partials     2025     2025              

Flag	Coverage Δ
hive	?
mysql	?
postgres	?
presto	52.46% <ø> (+0.13%)	⬆️
python	57.89% <ø> (-20.60%)	⬇️
sqlite	?
unit	51.43% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/security/manager.py	62.24% <ø> (-33.51%)	⬇️
superset/utils/dashboard_import_export.py	0.00% <0.00%> (-100.00%)	⬇️
superset/tags/core.py	4.54% <0.00%> (-95.46%)	⬇️
superset/key_value/commands/update.py	0.00% <0.00%> (-90.91%)	⬇️
superset/key_value/commands/delete.py	0.00% <0.00%> (-87.88%)	⬇️
superset/db_engines/hive.py	0.00% <0.00%> (-85.19%)	⬇️
superset/key_value/commands/delete_expired.py	0.00% <0.00%> (-84.00%)	⬇️
superset/dashboards/commands/importers/v0.py	15.62% <0.00%> (-76.25%)	⬇️
superset/datasets/commands/create.py	30.61% <0.00%> (-69.39%)	⬇️
superset/datasets/commands/update.py	25.00% <0.00%> (-69.05%)	⬇️
... and 325 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[eschutho]

@giovannipapini-agilelab is this PR ready for review?

[giovannipapini-agilelab]

Hi @eschutho, yes, it is a quite basic fix but fundamental for DASHBOARD_RBAC feature flag working. We have already applied this patch on our prod env, where gives the desired result.

[CeelamkotiVinod]

I tried applying your changes, but still issue persists. I installed superset using pip

[mattitoo]

It seems to work for us

[mdeshmu]

@giovannipapini-agilelab How will this behave if DASHBOARD_RBAC is enabled with no roles assigned in a published dashboard's properties? Will it gives access to every logged in user or will it fall back to dataset permissions ?

[giovannipapini-agilelab]

Will it gives access to every logged in user or will it fall back to dataset permissions ?

It will fall back to user's dataset permissions. It seems to me the right behaviour, coherent with the text box under dashboard roles If no roles are defined, then the dashboard is available to all roles.

[mdeshmu]

My understanding of this text:

If no roles are defined, then the dashboard is available to all roles.

is

dashboard will be available to any logged in user which is then a security issue.

[giovannipapini-agilelab]

We could also fix that text box writing something like

If no roles are defined, then the dashboard is visibile to users on the base of general datasource permissions.

that is the most precise behaviour definition.

[mdeshmu]

IMO, correction of that text is also needed. Something like this as shown in this documentation https://superset.apache.org/docs/creating-charts-dashboards/creating-your-first-dashboard/#manage-access-to-dashboards

If no roles are defined, then the access will fallback to Dataset permissions.

@amitmiran137 I feel there is a fullstop (.) missing in the documentation in here ->

[giovannipapini-agilelab]

@mdeshmu may I add those also those fixes in this pr?

[mdeshmu]

@giovannipapini-agilelab I would love to see clarity in the texts.

[mdeshmu]

@giovannipapini-agilelab With your changes in this PR, Can you please test that if dashboard is published and no role is assigned to dashboard, is it accessible by a user with Public or Gamma Role ?

[giovannipapini-agilelab]

@giovannipapini-agilelab With your changes in this PR, Can you please test that if dashboard is published and no role is assigned to dashboard, is it accessible by a user with Public or Gamma Role ?

Hi, we verified this case and in indeed the behaviour is this:

• If the dashboard is draft it is not listed in dashboard menu and only owners and admins can open it.
• If the dashboard is published has some rbac role assigned, then the it is listed in dashboard menu (d.m.) and visible to and only to those roles (with the exception of dashboard owners and admins). Other roles do not see it listed in d.m. and when they open it via permalink they get a "You cannot access this dashboard!" error page.

Imo this is the correct behaviour (and note that it is not the same as before this PR).

• If the dashboard is published and has no rbac role assigned, then it is visible in d.m. only to those who have dataset-level permissions, but it is accessible and visible via permalink also to everyone else (Gamma or Public roles).
  This could be seen as a security issue, even more if combined with the PUBLIC_ROLE_LIKE = "Gamma" setting.

Probably we should fix the fallback behaviour, in this or in some other PR, what do you think @mdeshmu?

[sfirke]

Good summary @giovannipapini-agilelab. FYI I was testing out a separate feature on version 2.1.0rc1 and found that the DASHBOARD_RBAC behavior is slightly different from 2.01. If a dashboard is draft and has no RBAC role assigned, it still tries to load - its name appears in the browser - but it errors before it can load. I suppose this is the same behavior since it's trying to load, but heads up in case that complicates how this is discussed.

[ivan-price-acted]

Hi there,

Just discovered this 'feature' whilst testing for our organisation: the fact that (currently) Draft dashboards provide a window for unauthorised users onto data they wouldn't normally be allowed to see, (if no RBAC roles are assigned) if this PR merges than those same unauthorised users will see the data when the dashboards are published.

I'm surprised we don't simply delete the line of code in question, i.e. be explicit in the RBAC rules that if we want a world-readable dashboard we assign a role that we know all users have ?

Why do we assume no roles == everyone cas access the data ? It is very likely users creating dashboards will forget to publish or unpublish their dashboards and not assign any RBAC rules, and will not understand the implications in terms of who can access their data between the two states.

As an administrator this presents a risk, trusting the users to do the right thing here (manage the draft / published AND the RBAC rules) is not realistic for us.

Can we make a plea for removing line 1994

or (dashboard.published and not dashboard.roles)

completely, and require the user to add an explicit role instead ?

-ivan

[villebro]

@ivan-price-acted you may have missed this PR, which I believe addresses at least part of the permission issue: #23586

[ivan-price-acted]

Hi @villebro indeed i had missed that and indeed that appears to address our concerns !

Looking forward to release 2.1.1 , thanks for the heads up !

[eschutho]

I'm going to close this pr for #23586 with hopes that the latter covers this use case.