You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/08/16 19:49:24 UTC
[GitHub] [superset] john-bodley opened a new pull request, #25008: fix: Address regression introduced in #24789
john-bodley opened a new pull request, #25008:
URL: https://github.com/apache/superset/pull/25008
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
In https://github.com/apache/superset/pull/24996, @jfrag1 identified an issue with https://github.com/apache/superset/pull/24789 as there were no guarantees that form-data provided was backed by a trusted source.
Per https://github.com/apache/superset/pull/24996#issuecomment-1679813683, I looked into seeing whether the access request could be more context aware, though regrettably the caller is of type `QueryContext` which is dashboard agnostic. The actual fix, per @nytai's [suggestion](https://github.com/apache/superset/pull/24996#issuecomment-1679896140), was to add an additional check to verify that the dashboard (per the `dashboardId` field) is associated with the datasource in question.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
Added unit tests.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "nytai (via GitHub)" <gi...@apache.org>.
nytai commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296565871
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
There's a few scenarios we need to enumerate and document, this is what makes the most sense to me.
- DASHBOARDS_RBAC disabled: superset should behave the same way it did before this feature was introduced and permissions are applied at the dataset level via "datasource access on x"
- DASHBOARD_RBAC enabled:
- The dashboard has roles: If the user has rbac access via their role, all charts/fitlers/etc in the dashboard should render and the user has access to all of them.
- The dashboard doesn't have any attached roles: the access perms should work the same way they do as if the feature is disabled: charts only render if the user has dataset access, users see the dashboard in their list if they have the "can read Dashboard" permission
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296488000
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
@jfrag1 I think the problem we're all struggling with is what the expected behavior should be. Superset's security manager was already complex and difficult to grok—which doesn't bode well from an understanding perspective—which we made worse with the introduction of the dashboard RBAC feature and embedded dashboards (which potentially have access patterns which conflict with the core manager). As the rules become more complex and/or conflicting we end up playing the game of Whac-A-Mole—where closing one attack vector likely opens another.
Fundamentally I think this—per your problem statement—comes down to "context". Let's say your two charts are A' and B' which are powered by datasets A and B respectively. With dashboard RBAC enabled the user can view chart A' in the "context" of the dashboard, but not in standalone/explorer mode. The challenge is the request is coming from the `ChartDataCommand` command which is dashboard agnostic.
I here the point you're making. There's an issue where the access to a dashboard is circumventing the need to check whether the user has access to the datasource. So in addition to checking whether the user can access the dashboard we also need to check (if the check is at the datasource, query-context, or viz level) whether the dashboard access check is specific to RBAC, i.e.,
```python
and self.can_access_dashboard(dashboard)
```
should likely be,
```python
and self.can_access_dashboard(dashboard)
and is_feature_enabled("DASHBOARD_RBAC")
and dashboard.roles
```
Again we're adding complex logic to try to model a complex security manager which likely exposes more corner cases/issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296414387
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
Review Comment:
I don't think I understand this logic completely, but if `dashboardId` can be spoofed in the form what's preventing users from reading any datasource?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jfrag1 commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296398563
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
I think we need to bring back `should_check_dashboard_access`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Vitor-Avila commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1297137388
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
@nytai I think that makes a lot of sense! Just a quick question, if `DASHBOARD_RBAC` is enabled and the dashboard has roles, the regular/default access validation should still work, right? For example:
* If the FF is enabled and the dashboard has roles, both someone with role-based access and a user with regular access (granted via `datasource access on x`/`schema access on x`) should work.
Another thing to consider is if we still want to handle guest users (embedded) and `DASHBOARD_RBAC` access similarly. Guest users can't access explore, they are only granted access to dashboards during the token generation, and should be able to access the dashboard entirely (including filters and etc). On the other hand, `DASHBOARD_RBAC` users are authenticated users that can access explore/other parts of the app, so here the context is really important.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley merged pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #25008:
URL: https://github.com/apache/superset/pull/25008
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jfrag1 commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296396394
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
My concern is that we're applying security rules intended for `DASHBOARD_RBAC` even when the flag isn't enabled. With the flag disabled, a user has access to any published dashboard containing a chart they have access to. With this logic, the user would then automatically get access to all other charts/datasets on the dashboard (albeit only with the dashboard context included), which I don't think should happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25008:
URL: https://github.com/apache/superset/pull/25008#issuecomment-1681288301
@betodealmeida and @jfrag1 the caller for this check is coming from [here](https://github.com/apache/superset/blob/master/superset/charts/data/api.py#L175-L186), which mentions,
> Take a query context constructed in the client and return payload data response for the given query
where access is checked [here](https://github.com/apache/superset/blob/master/superset/charts/data/commands/get_data_command.py#L67-L68). The challenge here is given that the query context is constructed in the client then isn't any `self._query_context.raise_for_access()` check is subject to being spoofed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jfrag1 commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296438381
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
Review Comment:
There's validation in the lines below that:
a. The user has access the the dashboard, and
b. The datasource being requested is present on the dashboard
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296488000
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
@jfrag1 I think the problem we're all struggling with is what the expected behavior should be. Superset's security manager was already complex and difficult to grok—which doesn't bode well from an understanding perspective—which we made worse with the introduction of the dashboard RBAC feature and embedded dashboards (which potentially have access patterns which conflict with the core manager). As the rules become more complex and/or conflicting we end up playing the game of Whac-A-Mole—where closing one attack vector likely opens another.
Fundamentally I think this—per your problem statement—comes down to "context". Let's say your two charts are A' and B' which are powered by datasets A and B respectively. With dashboard RBAC enabled the user can view chart A' in the "context" of the dashboard, but not in standalone/explorer mode. The challenge is the request is coming from the `ChartDataCommand` command which is dashboard agnostic.
I here the point you're making. There's an issue where the access to a dashboard is circumventing the need to check whether the user has access to the datasource. So in addition to checking whether the user can access the dashboard we also need to check (if the check is at the datasource, query-context, or viz level) whether the dashboard access check is specific to RBAC, i.e.,
```python
and self.can_access_dashboard(dashboard)
```
should likely be,
```pyhton
and self.can_access_dashboard(dashboard)
and is_feature_enabled("DASHBOARD_RBAC")
and dashboard.roles
```
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
@jfrag1 I think the problem we're all struggling with is what the expected behavior should be. Superset's security manager was already complex and difficult to grok—which doesn't bode well from an understanding perspective—which we made worse with the introduction of the dashboard RBAC feature and embedded dashboards (which potentially have access patterns which conflict with the core manager). As the rules become more complex and/or conflicting we end up playing the game of Whac-A-Mole—where closing one attack vector likely opens another.
Fundamentally I think this—per your problem statement—comes down to "context". Let's say your two charts are A' and B' which are powered by datasets A and B respectively. With dashboard RBAC enabled the user can view chart A' in the "context" of the dashboard, but not in standalone/explorer mode. The challenge is the request is coming from the `ChartDataCommand` command which is dashboard agnostic.
I here the point you're making. There's an issue where the access to a dashboard is circumventing the need to check whether the user has access to the datasource. So in addition to checking whether the user can access the dashboard we also need to check (if the check is at the datasource, query-context, or viz level) whether the dashboard access check is specific to RBAC, i.e.,
```python
and self.can_access_dashboard(dashboard)
```
should likely be,
```pyhton
and self.can_access_dashboard(dashboard)
and is_feature_enabled("DASHBOARD_RBAC")
and dashboard.roles
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25008:
URL: https://github.com/apache/superset/pull/25008#issuecomment-1682872006
Thanks—drum roll please—to @betodealmeida, @eschutho, @jfrag1 , @michael-s-molina, @nytai , and @villebro for jumping on the call earlier to discuss an interim solution to this regression which should (🤞) allow us to roll forward.
I've updated the PR to extend the previous logic (stemming from @nytai's suggestion) I had drafted to ensure that:
1. Per @villebro's suggestion we also leverage the chart information and verify that i) the chart is associated with the dashboard, and ii) the chart is associated with the said datasource. This is a tighter handshake than what we had previously, i.e., ensuring that the datasource was associated with said dashboard.
2. Per @nytai's [comment](https://github.com/apache/superset/pull/25008#discussion_r1296565871) I added a check that made sure the dashboard fallback is only invoked when the `DASHBOARD_RBAC` feature is enabled and the dashboard has associated roles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1297657364
##########
superset/security/manager.py:
##########
@@ -1858,10 +1859,27 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Grant access to the datasource only if dashboard RBAC is enabled
+ # and said datasource is associated with the dashboard chart in
+ # question.
form_data
+ and is_feature_enabled("DASHBOARD_RBAC")
and (dashboard_id := form_data.get("dashboardId"))
- and (dashboard := DashboardDAO.find_by_id(dashboard_id))
- and self.can_access_dashboard(dashboard)
+ and (
+ dashboard_ := self.get_session.query(Dashboard)
Review Comment:
The `DashboardDAO` and `ChartDAO` have additional filtering and/or security checks when fetching objects and thus I had to revert to using the raw SQLAlchemy ORM for fetching the dashboard and chart associated with the specified IDs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296423667
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
@jfrag1 the `self.can_access_dashboard(dashboard)` will always raise if a user doesn't have access to the dashboard regardless of what the context is.
The other formulation I can think of is to use the referrer to identify the dashboard in question, however I guess per [here](https://en.wikipedia.org/wiki/Referer_spoofing) one can also spoof the referrer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296387970
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
@jfrag1 I updated the PR description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jfrag1 commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296396394
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
My concern is that we're applying security rules intended for `DASHBOARD_RBAC` even when the flag isn't enabled. With the flag disabled, a user has access to any published dashboard containing a chart they have access to. With this logic, the user would then automatically get access to all other charts/datasets on the dashboard, which I don't think should happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296488000
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
@jfrag1 I think the problem we're all struggling with is what the expected behavior should be. Superset's security manager was already complex and difficult to grok—which doesn't bode well from an understanding perspective—which we made worse with the introduction of the dashboard RBAC feature and embedded dashboards (which potentially have access patterns which conflict with the core manager). As the rules become more complex and/or conflicting we end up playing the game of Whac-A-Mole—where closing one attack vector likely opens another.
Fundamentally I think this—per your problem statement—comes down to "context". Let's say your two charts are A' and B' which are powered by datasets A and B respectively. With dashboard RBAC enabled the user can view chart A' in the "context" of the dashboard, but not in standalone/explorer mode. The challenge is the request is coming from the `ChartDataCommand` command which is dashboard agnostic.
I here the point you're making. There's an issue where the access to a dashboard is circumventing the need to check whether the user has access to the datasource. So in addition to checking whether the user can access the dashboard we also need to check (if the check is at the datasource, query-context, or viz level) whether the dashboard access check is specific to RBAC, i.e.,
```python
and self.can_access_dashboard(dashboard)
```
should likely be,
```python
and self.can_access_dashboard(dashboard)
and is_feature_enabled("DASHBOARD_RBAC")
and dashboard.roles
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296423667
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
@jfrag1 the `self.can_access_dashboard(dashboard)` check is still being invoked and will always raise if a user doesn't have access to the dashboard regardless of what the context is.
The other formulation I can think of is to use the referrer to identify the dashboard in question, however I guess per [here](https://en.wikipedia.org/wiki/Referer_spoofing) one can also spoof the referrer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "nytai (via GitHub)" <gi...@apache.org>.
nytai commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296565871
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
There's a few scenarios we need to enumerate and document, this is what makes the most sense to me.
- DASHBOARDS_RBAC disabled: superset should behave the same way it did before this feature was introduced and permissions are applies at the dataset level via "datasource access on x"
- DASHBOARD_RBAC enabled:
- The dashboard has roles: If the user has rbac access via their role, all charts/fitlers/etc in the dashboard should render and the user has access to all of them.
- The dashboard doesn't have any attached roles: the access perms should work the same way they do as if the feature is disabled, charts only render if the user has dataset access and users can list dashboards if they have the "can read dashboard" permission
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "nytai (via GitHub)" <gi...@apache.org>.
nytai commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296565871
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
There's a few scenarios we need to enumerate and document, this is what makes the most sense to me.
- DASHBOARDS_RBAC disabled: superset should behave the same way it did before this feature was introduced and permissions are applied at the dataset level via "datasource access on x"
- DASHBOARD_RBAC enabled:
- The dashboard has roles: If the user has rbac access via their role, all charts/fitlers/etc in the dashboard should render and the user has access to all of them.
- The dashboard doesn't have any attached roles: the access perms should work the same way they do as if the feature is disabled, charts only render if the user has dataset access and users can list dashboards if they have the "can read dashboard" permission
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jfrag1 commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296437242
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
I understand that, here's a more concrete example to illustrate what I'm trying to get at:
`DASHBOARD_RBAC` is disabled. There's a published dashboard with 2 charts, powered by datasets A and B. A user has access to dataset A, but not dataset B. The user has access to the dashboard because they have access to dataset A. The question I'd then like to pose is, what is the expected behavior when the user goes to view the dashboard?
My understanding is that the expected behavior is that the user can view the chart powered by dataset A, but the chart powered by dataset B shows an error because they don't have access to the dataset.
However, after https://github.com/apache/superset/pull/24789 removed the `should_check_dashboard_access` check, the user would be able to view both charts on the dashboard. The user's access to dataset A grants them access to the dashboard, which then grants them access to _all_ other charts on the dashboard, which I'd argue is another regression from https://github.com/apache/superset/pull/24789
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jfrag1 commented on a diff in pull request #25008: fix: Address regression introduced in #24789
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25008:
URL: https://github.com/apache/superset/pull/25008#discussion_r1296370781
##########
superset/security/manager.py:
##########
@@ -1858,9 +1858,13 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Check whether the datasource is associated with a dashboard in a
+ # trustworthy manner, i.e., we need to validate that the specified
+ # dashboard is actually associated with said datasource.
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
+ and any(slc.datasource == datasource for slc in dashboard.slices)
and self.can_access_dashboard(dashboard)
Review Comment:
Another thing that occurred to me recently, shouldn't we only be doing this check if dashboard rbac is enabled (or if the user is a guest user for embedded purposes).
For example before https://github.com/apache/superset/pull/24789 there was the following logic:
```python
should_check_dashboard_access = (
feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC")
or self.is_guest_user()
)
if not (
self.can_access_schema(datasource)
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
should_check_dashboard_access
and self.can_access_based_on_dashboard(datasource)
```
but now, we're checking if if the user can access the dataset via the dashboard no matter what
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org