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/18 18:44:49 UTC
[GitHub] [superset] john-bodley opened a new pull request, #25029: fix: Native dashboard aware dataset permission
john-bodley opened a new pull request, #25029:
URL: https://github.com/apache/superset/pull/25029
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Somewhat related to https://github.com/apache/superset/pull/25008, this PR ensures that access is granted to a datasource (in the context of a dashboard native filter) when dashboard RBAC is enabled. Thanks to @michael-s-molina for https://github.com/apache/superset/pull/25025 which exposed the `dashboardId` in the client form data.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
#### BEFORE
<img width="1440" alt="Screenshot 2023-08-18 at 11 38 14 AM" src="https://github.com/apache/superset/assets/4567245/d90a3e83-bad8-4299-b777-02f24f77b3f3">
#### AFTER
<img width="1440" alt="Screenshot 2023-08-18 at 11 37 21 AM" src="https://github.com/apache/superset/assets/4567245/8b7ee0c7-4959-40ed-a4f1-16bb23772255">
### 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] john-bodley commented on a diff in pull request #25029: fix: Native filter dashboard RBAC aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1300899204
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,36 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
Review Comment:
Phew! I'm glad to hear that this was a false alarm. Thanks again for testing the change.
--
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 #25029: fix: Native filter dashboard RBAC aware dataset permission
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1299528497
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,36 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
Review Comment:
hey @john-bodley I just tested this branch locally and I still see the permission error -- it seems that `dashboardId` is still not included in the filter's payload data (despite https://github.com/apache/superset/pull/25025). Curious to know if you are facing a different behavior on your end.
--
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] eschutho commented on a diff in pull request #25029: fix: Native dashboard aware dataset permission
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298889891
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,35 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
Review Comment:
maybe @Vitor-Avila or @jfrag1 can help add if there's anything else we need to do for embedded with the below logic? Check for a guest user?
--
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 #25029: fix: Native dashboard aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298899549
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,35 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
Review Comment:
@eschutho Black formats the code how it sees fit.
--
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 #25029: fix: Native filter dashboard RBAC aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1300965219
##########
tests/integration_tests/security_tests.py:
##########
@@ -1718,14 +1719,28 @@ def test_raise_for_access_rbac(
world_health = self.get_dash_by_slug("world_health")
treemap = self.get_slice("Treemap", db.session, expunge_from_session=False)
+ births.json_metadata = json.dumps(
+ {
+ "native_filter_configuration": [
+ {
+ "id": "NATIVE_FILTER-ABCDEFGH",
+ "targets": [{"datasetId": birth_names.id}],
+ },
+ {
+ "id": "NATIVE_FILTER-IJKLMNOP",
+ "targets": [{"datasetId": treemap.id}],
+ },
+ ]
+ }
+ )
+
mock_g.user = security_manager.find_user("gamma")
mock_is_owner.return_value = False
mock_can_access.return_value = False
mock_can_access_schema.return_value = False
for kwarg in ["query_context", "viz"]:
births.roles = []
- db.session.flush()
Review Comment:
No need to flush these updates to the database transaction given the logic in the test is confined to the same Flask-SQLAlchemy session.
--
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 #25029: fix: Native dashboard aware dataset permission
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298898796
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,35 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
Review Comment:
Yes I will add logic for embedded in a separate PR
--
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 #25029: fix: Native dashboard aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298820457
##########
superset/security/manager.py:
##########
@@ -1872,15 +1872,20 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ form_data.get("type") == "NATIVE_FILTER"
Review Comment:
@jfrag1:
1. Oops. Yeah I missed moving `self.can_access_dashboard(dashboard_)` outside of the `or` block. I've placed it at the end of the logic checks as it's likely the most expensive check.
2. That is an option.
--
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 #25029: fix: Native dashboard aware dataset permission
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298820271
##########
superset/security/manager.py:
##########
@@ -1872,15 +1872,20 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ form_data.get("type") == "NATIVE_FILTER"
Review Comment:
good catch, @jfrag1! I agree it's a valid check and with this improved version that we know the `dashboardId` shouldn't be too expensive.
--
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 #25029: fix: Native dashboard aware dataset permission
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298816993
##########
superset/security/manager.py:
##########
@@ -1872,15 +1872,20 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ form_data.get("type") == "NATIVE_FILTER"
Review Comment:
I think this needs a few extra validations here in order to grant access:
1. We still need to check that the user has access to the dashboard (I'd suggest moving the `self.can_access_dashboard(dashboard_)` check to be right after `and dashboard_.roles` since we want to check dashboard access whether it's via a chart or native filter
2. That the datasource is actually used as a native filter on the dashboard. We can probably borrow logic from @Vitor-Avila's [PR here](https://github.com/apache/superset/pull/24808/files#diff-bcb3868b9e29aea5f6e87ec641120dae35a86d732501f488a7a6dbdeb450f5c1R2082); performance shouldn't be a concern here since we only need to check one 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 #25029: fix: Native dashboard aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298861638
##########
superset-frontend/src/dashboard/components/nativeFilters/utils.ts:
##########
@@ -94,6 +95,7 @@ export const getFormData = ({
viz_type: filterType,
type,
dashboardId,
+ native_filter_id: id,
Review Comment:
Calling this `id` seemed a tad terse. I'm not sure if this should be snake or camel case.
--
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 #25029: fix: Native filter dashboard RBAC aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #25029:
URL: https://github.com/apache/superset/pull/25029
--
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 pull request #25029: fix: Native dashboard aware dataset permission
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #25029:
URL: https://github.com/apache/superset/pull/25029#issuecomment-1684374881
thank you so much for working on this PR @john-bodley! 🙏
--
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] eschutho commented on a diff in pull request #25029: fix: Native dashboard aware dataset permission
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298896323
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,35 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
+ and (json_metadata := json.loads(dashboard_.json_metadata))
+ and any(
+ target.get("datasetId") == datasource.id
+ for fltr in json_metadata.get(
+ "native_filter_configuration",
+ [],
+ )
+ for target in fltr.get("targets", [])
+ if native_filter_id == fltr.get("id")
Review Comment:
đź‘Ť
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,35 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
+ and (json_metadata := json.loads(dashboard_.json_metadata))
+ and any(
+ target.get("datasetId") == datasource.id
+ for fltr in json_metadata.get(
+ "native_filter_configuration",
+ [],
+ )
+ for target in fltr.get("targets", [])
+ if native_filter_id == fltr.get("id")
Review Comment:
đź‘Ť
--
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 #25029: fix: Native filter dashboard RBAC aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1300572959
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,36 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
Review Comment:
@Vitor-Avila just to confirm have you rebuilt the frontend assets? If the problem still persists would you mind sharing a URL—leveraging the example dataset—so I can further debug this issue?
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,36 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
Review Comment:
@Vitor-Avila just to confirm have you rebuilt the frontend assets? If the problem still persists would you mind sharing a URL—leveraging the example dataset—so I can further debug this issue?
--
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 #25029: fix: Native filter dashboard RBAC aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1300899204
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,36 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
Review Comment:
Phew! I'm glad to hear that this was a false alarm.
--
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 #25029: fix: Native dashboard aware dataset permission
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298816993
##########
superset/security/manager.py:
##########
@@ -1872,15 +1872,20 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ form_data.get("type") == "NATIVE_FILTER"
Review Comment:
I think this needs a few extra validations needed here in order to grant access:
1. We still need to check that the user has access the the dashboard (I'd suggest moving the `self.can_access_dashboard(dashboard_)` check to be right after `and dashboard_.roles` since we want to check dashboard access whether it's via a chart or native filter
2. That the datasource is actually used as a native filter on the dashboard. We can probably borrow logic from @Vitor-Avila's [PR here](https://github.com/apache/superset/pull/24808/files#diff-bcb3868b9e29aea5f6e87ec641120dae35a86d732501f488a7a6dbdeb450f5c1R2082); performance shouldn't be a concern here since we only need to check one dashboard
##########
superset/security/manager.py:
##########
@@ -1872,15 +1872,20 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ form_data.get("type") == "NATIVE_FILTER"
Review Comment:
I think this needs a few extra validations needed here in order to grant access:
1. We still need to check that the user has access to the dashboard (I'd suggest moving the `self.can_access_dashboard(dashboard_)` check to be right after `and dashboard_.roles` since we want to check dashboard access whether it's via a chart or native filter
2. That the datasource is actually used as a native filter on the dashboard. We can probably borrow logic from @Vitor-Avila's [PR here](https://github.com/apache/superset/pull/24808/files#diff-bcb3868b9e29aea5f6e87ec641120dae35a86d732501f488a7a6dbdeb450f5c1R2082); performance shouldn't be a concern here since we only need to check one 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] jfrag1 commented on a diff in pull request #25029: fix: Native dashboard aware dataset permission
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298816993
##########
superset/security/manager.py:
##########
@@ -1872,15 +1872,20 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ form_data.get("type") == "NATIVE_FILTER"
Review Comment:
I think this needs a few extra validations needed here in order the grant access:
1. We still need to check that the user has access the the dashboard (I'd suggest moving the `self.can_access_dashboard(dashboard_)` check to be right after `and dashboard_.roles` since we want to check dashboard access whether it's via a chart or native filter
2. That the datasource is actually used as a native filter on the dashboard. We can probably borrow logic from @Vitor-Avila's [PR here](https://github.com/apache/superset/pull/24808/files#diff-bcb3868b9e29aea5f6e87ec641120dae35a86d732501f488a7a6dbdeb450f5c1R2082); performance shouldn't be a concern here since we only need to check one 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 #25029: fix: Native dashboard aware dataset permission
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298820457
##########
superset/security/manager.py:
##########
@@ -1872,15 +1872,20 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ form_data.get("type") == "NATIVE_FILTER"
Review Comment:
@jfrag1:
1. Oops. Yeah I missed moving `self.can_access_dashboard(dashboard_)` outside of the `or` block. I've placed it at the end of the logic checks as it's likely the most expensive check.
2. That is an option. Note in theory we should be including the filter ID in the query-context to check for the specific filter rather than seeing if any filter matches.
--
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] eschutho commented on a diff in pull request #25029: fix: Native dashboard aware dataset permission
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298888116
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,35 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
Review Comment:
```suggestion
(
```
--
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 #25029: fix: Native dashboard aware dataset permission
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1298820271
##########
superset/security/manager.py:
##########
@@ -1872,15 +1872,20 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ form_data.get("type") == "NATIVE_FILTER"
Review Comment:
good catch, @jfrag1!
--
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 #25029: fix: Native filter dashboard RBAC aware dataset permission
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #25029:
URL: https://github.com/apache/superset/pull/25029#discussion_r1300668002
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,36 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
Review Comment:
@john-bodley ohh that's true! I didn't have rebuilt the frontend assets 🤦 thanks for the heads up and I just confirmed it works properly after doing so. Filter box are showing an error, but I can take a look to implement a similar approach to fix it. Again apologies for the false alarm.
##########
superset/security/manager.py:
##########
@@ -1872,14 +1873,36 @@ def raise_for_access(
.one_or_none()
)
and dashboard_.roles
- and (slice_id := form_data.get("slice_id"))
and (
- slc := self.get_session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ (
+ # Native filter.
+ form_data.get("type") == "NATIVE_FILTER"
+ and (native_filter_id := form_data.get("native_filter_id"))
+ and dashboard_.json_metadata
Review Comment:
@john-bodley ohh that's true! I didn't have rebuilt the frontend assets 🤦 thanks for the heads up and I just confirmed it works properly after doing so. Filter box are showing an error, but I can take a look to implement a similar approach to fix it. Again apologies for the false alarm.
--
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