fix: Address dashboard permission regression in #23586

[john-bodley]

SUMMARY

This PR addresses an issue in #23586 where previously the necessary dashboard access checks were a combination of logic which resided in the security manager as well as the view with conflicting logic. This was problematic say if a user was deemed to have access to a dashboard (from a security perspective) but was then denied within the view which relied on additional dataset access checks meaning it was near impossible to bypass with custom security logic.

This PR simply moves the entirety of the permission checks to the security manager.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

• Has associated issue:
• 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

[john-bodley]

This decorator seemed superfluous as said logic could be handled elsewhere.

[john-bodley]

The TL;DR is this check now includes logic for the legacy access controls which were based (for right or wrong) around the notion that a user has access to the dashboard if they can access at least one of the underlying datasets which backs the dashboard. Said logic previously resided in the dashboard view.

[john-bodley]

This logic has been inlined below.

[john-bodley]

This was a tad hard to digest, but it essence is checking if RBAC is disabled. A more digestible form (used below) to check if RBAC is enabled is,

if is_feature_enabled("DASHBOARD_RBAC") and dashboard.roles:
    ...

[john-bodley]

I'm struggling to grok how/why the published state alters things. Note the challenge is logic outlined here isn't the entire corpus of rules from an access standpoint given that the view also had additional (potentially conflicting) logic based on whether the user had access to the underlying datasets.

Personally I feel the access rubric is already quite complex and rather difficult to grok as it pertains to embedded dashboards and the like. Additionally I'm not sure I agree with the logic with regards to RBAC when no roles are defined. Personally this feels like one shouldn't strictly have access as opposed to falling back to the legacy logic. Do dashboard owners et al. grok how this works?

[villebro]

Personally this feels like one shouldn't strictly have access as opposed to falling back to the legacy logic. Do dashboard owners et al. grok how this works?

The reason why it's like this is because otherwise it's impossible to support both regular RBAC and "dashboard RBAC" on the same running instance. I know it may be slightly confusing, but it's clearly documented, both in the modal and the docs, so changing this would be a pretty substantial breaking change.

[john-bodley]

@villebro is the desired end state to support only one or do you perceive both coexisting for some time?

[villebro]

@john-bodley I kind of have a SIP brewing for a more universal object level RBAC model. I'll circulate it with you once I have the main design ready, as I'm also curious to hear your thoughts on it.

[codecov]

Codecov Report

Merging #24350 (6b9484f) into master (0e3f1f6) will decrease coverage by 0.02%.
The diff coverage is 72.72%.

❗ Current head 6b9484f differs from pull request most recent head 9bba03b. Consider uploading reports for the commit 9bba03b to get more accurate results

@@            Coverage Diff             @@
##           master   #24350      +/-   ##
==========================================
- Coverage   69.09%   69.07%   -0.02%     
==========================================
  Files        1903     1903              
  Lines       74608    74585      -23     
  Branches     8107     8107              
==========================================
- Hits        51550    51523      -27     
- Misses      20947    20951       +4     
  Partials     2111     2111              

Flag	Coverage Δ
hive	53.79% <4.54%> (+<0.01%)	⬆️
mysql	79.37% <72.72%> (-0.03%)	⬇️
postgres	79.44% <72.72%> (-0.03%)	⬇️
presto	53.71% <4.54%> (+<0.01%)	⬆️
python	83.36% <72.72%> (-0.03%)	⬇️
sqlite	77.96% <72.72%> (-0.03%)	⬇️
unit	54.20% <4.54%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
superset/utils/decorators.py	96.22% <ø> (+1.78%)	⬆️
superset/security/manager.py	95.11% <64.70%> (-0.81%)	⬇️
superset/views/core.py	75.99% <100.00%> (-0.26%)	⬇️

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

[john-bodley]

Aren't dashboards roles only a thing if RBAC is enabled?

[john-bodley]

Moved to RBAC tests given dashboard roles are only associated with RBAC.

[john-bodley]

This is concerning that this is different than the DashboardDAO.get_by_id_or_slug and that the DAO logic differs (from a filter perspective) whether you pass in a UUID versus a slug/ID.

Personally the UUID logic is just adding more complexity to an already partially intractable problem in terms of understanding (and thus enforcing) the security model. Adhering to the KISS principle in terms of the security model will actually make the service more secure as it'll be easier to grok and enforce.

[john-bodley]

This is inlined later for improved readability.

[john-bodley]

This is inlined later.

[john-bodley]

Previously this was non-idempotent.

[villebro]

nice 👍 One more down, I wonder how many to go..

[villebro]

Great work, this really cleans up this logic. As there's clearly a need for more advanced RBAC models to complement the default "datasource centric" RBAC model (I believe #10408 which introduced Dashboard RBAC is the most liked SIP of all time), it may be a good idea to consider improving/clarifying this functionality when we start considering breaking changes for 4.0. I'd personally like to see the possibility of also separating charts from the datasource permission model, so that one could control who can see certain charts, irrespective of which datasets they're referencing.

[villebro]

nice 👍 One more down, I wonder how many to go..

[villebro]

Personally this feels like one shouldn't strictly have access as opposed to falling back to the legacy logic. Do dashboard owners et al. grok how this works?

The reason why it's like this is because otherwise it's impossible to support both regular RBAC and "dashboard RBAC" on the same running instance. I know it may be slightly confusing, but it's clearly documented, both in the modal and the docs, so changing this would be a pretty substantial breaking change.

[michael-s-molina]

it may be a good idea to consider improving/clarifying this functionality when we start considering breaking changes for 4.0

@villebro If you already have something in mind, you can add cards to the Punt to 4.0 column in our board.