fix: Revert "fix: Dashboard aware RBAC dataset permission (#24789)"

[jfrag1]

SUMMARY

This reverts #24789 due to a security issue that could allow any user to retrieve data from any datasource.

The issue is that the dashboard id being checked here for the dashboard context comes from the request payload from the client, not a trusted backend source. Therefore a user can copy a valid request for a datasource they shouldn't have access to and simply change the dashboard id in the form data to a dashboard they have access to, bypassing this security check.

I can't think of a secure way to assert the presence of a dashboard context on the backend when retrieving chart data; the old approach may be the best we can do

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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]

Thanks @jfrag1 for raising this. I did wonder if the form-data was trustworthy, but had assumed (wrongfully) that it was given it came from either the query-context or visualization.

In #24804 I refactored the raise_for_access method so it could be dashboard aware and thus 🤞 that’s a viable trusted route, i.e., find the caller and pass in the dashboard.

[jfrag1]

Thanks @jfrag1 for raising this. I did wonder if the form-data was trustworthy, but had assumed (wrongfully) that it was given it came from either the query-context or visualization.

In #24804 I refactored the raise_for_access method so it could be dashboard aware and thus 🤞 that’s a viable trusted route, i.e., find the caller and pass in the dashboard.

I see what you're saying, but in the case of the chart data endpoint, I simply don't see a way to determine the dashboard to pass. The only way we can determine a dashboard context is via the request payload, which is forgeable

[john-bodley]

@jfrag1 per #24789 with dashboard RBAC enabled access should only be granted in the context of a dashboard and thus in the case of the chart data endpoint dashboard RBAC shouldn't be a factor.

[nytai]

I spent a while trying to untangle this feature a while back and found there was a lot of undefined behavior or the logic broke superset's original security model.

I think the previous PR was missing a check to ensure the dashboardId passed in the chart's formData hasn't been spoofed, ie the chart is in fact part of said dashboard. Otherwise an attacker you could find the id of a dashboard they have rbac access on and modify a chart's data request to include that id in the context. This boils down to the formData being generated client side and the integrity cannot be trusted.

Actually, it might even make sense to check the datasets being queried by the chart are a subset of the datasets of the dashboard. That would change the security model to "DASHBOARD_RBAC grants a user access to the dashboard and all datasets contained in the dashboard" which is closer to superset's original data access model.

The only other way I can think this could be more fine-grained, and secure, would be to enumerate all the dimensions and metrics in the dashboard and ensure the chart's formData isn't querying anything outside of that set. But that would probably be far too expensive.

[john-bodley]

@jfrag1 let me look into an alternative formulation. The issue with reverting this PR is it reopens a previous security vulnerability.

[sadpandajoe]

@jfrag1 let me look into an alternative formulation. The issue with reverting this PR is it reopens a previous security vulnerability.

@john-bodley as you are looking into a fix forward, not sure if this issue I reported may be related to the original PR that this is reverting. Looks like it has a similar error message from the screenshot in your original PR.

[jinghua-qa]

/testenv up

[github-actions]

@jinghua-qa Ephemeral environment spinning up at http://34.210.72.178:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[john-bodley]

@jfrag1 I authored #25008 as an alternative to this revert.

[jfrag1]

Closing in favor of the fix forward #25008

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.