fix: Dashboard aware RBAC dataset permission

[john-bodley]

SUMMARY

This PR (alongside #24806) fixes #24782.

Previously users were granted access to a dataset/chart associated with a dashboard (per the RBAC dashboard roles) however it wasn't context aware, i.e., access was granted irrespective of the context of the request. Specifically this PR ensures that role based dashboard access only occurs within the confines of a dashboard.

TESTING INSTRUCTIONS

Added unit tests.

BEFORE

AFTER

ADDITIONAL INFORMATION

• Has associated issue: Gamma user can clone a dashboard and all of its charts without being owner of dashboard, chart and its underlying dataset #24782
• Required feature flags:
• 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

cc: @mdeshmu @sfirke

[codecov]

Codecov Report

Merging #24789 (0c7cdf3) into master (9f7f2c6) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 0c7cdf3 differs from pull request most recent head a80cced. Consider uploading reports for the commit a80cced to get more accurate results

@@            Coverage Diff             @@
##           master   #24789      +/-   ##
==========================================
- Coverage   68.99%   68.99%   -0.01%     
==========================================
  Files        1906     1906              
  Lines       74133    74122      -11     
  Branches     8208     8208              
==========================================
- Hits        51151    51140      -11     
  Misses      20859    20859              
  Partials     2123     2123              

Flag	Coverage Δ
hive	54.17% <75.00%> (+0.01%)	⬆️
mysql	79.21% <100.00%> (-0.01%)	⬇️
postgres	79.31% <100.00%> (-0.01%)	⬇️
presto	54.07% <75.00%> (+0.01%)	⬆️
python	83.37% <100.00%> (-0.01%)	⬇️
sqlite	77.88% <100.00%> (-0.01%)	⬇️
unit	55.04% <25.00%> (+<0.01%)	⬆️

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

Files Changed	Coverage Δ
superset/security/manager.py	94.43% <100.00%> (+0.48%)	⬆️

... and 1 file with indirect coverage changes

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

[john-bodley]

I'm not sure this is needed, i.e., test_get_dashboard_view__user_access_with_dashboard_permission might be suffice.

[john-bodley]

This test (alongside others) was failing after the change which likely indicates that either i) RBAC was supposed to expand beyond the scope/context of the dashboard (and thus my understanding of said issue was misconstrued), or ii) was a mistake.

[john-bodley]

Conditional on my logic being correct, these tests are now superfluous as the irrespective of whether a guest is authorized or not, they don't have access to the chart, dataset, etc. unless it's in the context of RBAC. The logic previously outlined here condenses to the same logic which is defined in the security_tests.py.

[betodealmeida]

@john-bodley @Vitor-Avila did a fix in #24808 giving access to datasets that power native filters, when using Dashboard RBAC. Can we include his changes here, or would that use case already work with your changes?

[john-bodley]

Thanks @betodealmeida for sharing @Vitor-Avila's PR. Initially I can't say whether this change will resolve said issue as it's not apparent where (and with what arguments) the raise_for_access method is being invoked from regarding the native filters.

@Vitor-Avila would you mind using my branch to test your issue? Note assuming the approach outlined in this PR is correct (@villebro it's probably the definitive expert on this) and it doesn't address @Vitor-Avila's issue, I do sense the right solution there is make sure that the access for native filters is akin to that of dashboard charts.

Personally I'm a tad worried about the DASHBOARD_RBAC feature flag as I don't think all the various paths have been fully flushed out. Furthermore our default security manager/policy is already somewhat of a complex web of rules which only gets worse when you add another variable (the feature flag—which turns a lot of the existing logic on its head) to the mix.

Additionally at Airbnb we use a completely different security model—access is purely defined at the dataset level—and thus I struggle to grok what the logic should be, i.e., I likely lack the necessary context.

[Vitor-Avila]

hey @john-bodley, thanks for the quick response!

My PR was actually not created with the DASHBOARD_RBAC FF in mind, but actually focusing on embedded access for dashboards. I'm not sure if the permission flow/logic is the same between guest user/dashboard RBAC access, but I know at least for embedded access you would only specify the dashboard uuid when generating the guest_token, and I believe the expectation is that the user should be able to view all charts and filters used in the specified dashboard. For embedded dashboards, it's also not possible to access Charts directly (at least not through the iframe) -- but likely through the API.

I'll test if my issue is also addressed with your branch and report back -- thank you! 🙏

[Vitor-Avila]

Yup, I confirmed the issue still exists in this branch:

@eschutho mentioned you also have a PR to re-introduce the logic from can_access_based_on_dashboard: #24804

Should I try to re-create my changes on top of this modification?

[john-bodley]

Thanks @Vitor-Avila for confirming. I thought that might be the case as the dashboard information is likely lacking from the form-data when the native dashboard filter datasource check is happening. #24804 is simply a refactor and thus wouldn't change anything.

The challenge with the RBAC feature is opening up access only in the context of a dashboard. Furthermore I sense there's could be flaw with the model, i.e., a Gamma user (who has been granted access to a dashboard) could likely have broader access than an owner as it pertains to how access is determined for the datasets within the dashboard which are driving the native filters and charts. Again I need to provide a caveat with everything I say as I'm not overly familiar with how chart etc. access is defined.

[Vitor-Avila]

@john-bodley sorry I mis-read #24804 and thought you were re-adding the logic for can_access_based_on_dashboard somewhere else, but that's not true - I was confused with the raise_for_dashboard_access function. My apologies for the confusion.

In my previous test, I had only checked the issue I'm trying to fix for embedded users, but I just tried with the DASHBOARD_RBAC access as well and confirmed it also happens for this flow:

Not sure if we can keep the can_access_based_on_dashboard function or if that's not compatible with the work you've been doing in these PRs.

[john-bodley]

@Vitor-Avila I think we likely need to take a pause on this for now until collectively the community can agree on how the dashboard RBAC are supposed to work. Hopefully this can happen sometime next week once some of the Apache Superset committers who are well versed in this space are back online.

The challenge otherwise is we could find ourselves playing whack-a-mole with trying to fix issues whilst simultaneously creating other ones. Taking a more holistic (30,000') view of problem will likely help everyone better grok the intention and thus what access controls are required.

[Vitor-Avila]

sure, that makes sense! It could also be a good opportunity to revisit if both flows (DASHBOARD_RBAC and guest user) should be handled in a more distinct way.

[eschutho]

I'm not too familiar with this feature either, but just noticed that the previous version checks for datasource access and the new one checks for dashboard access only. Is that what we want?

[john-bodley]

@eschutho the logic is as follows. If you don't have explicit access to dataset you then fallback to the form-data of either the visualization or query-context to try to decipher the context. If it's in the context of a dashboard, i.e., you're viewing a chart in a dashboard, one checks whether you have access to the dashboard—per the can_access_dashboard method.

[villebro]

Do we need to check that the purported chart whose data is being loaded actually exists on the dashboard?

[john-bodley]

@villebro is the logic on line #1846 not suffice, i.e., extracting the dashboard ID from the form-data (which could stem from either a legacy visualization or query-context)?

[eschutho]

@john-bodley are you ok with us merging #24808 while we wait for more feedback on RBAC for this PR? @Vitor-Avila's PR isn't for the DASHBOARD_RBAC flag specifically.

[michael-s-molina]

@michael-s-molina can this be included in 3.0 ?

@mdeshmu All fix PRs that affect 3.0 will be included in the release. I'm cherry-picking them daily. If a fix PR is not merged by the time we release 3.0.0, it will be included right after it in 3.0.1. We're planning to submit patch releases every 2 weeks.

[jfrag1]

@john-bodley I discovered that this PR introduces a security issue that can allow any user to get data from any datasource. I've opened #24996 to revert this, see the PR description there for more details