fix: Native filter dashboard RBAC aware dataset permission #25029

john-bodley · 2023-08-18T18:44:47Z

SUMMARY

Somewhat related to #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 #25025 which exposed the dashboardId in the client form data.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

TESTING INSTRUCTIONS

Added unit tests.

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

cc: @jfrag1

jfrag1 · 2023-08-18T19:48:45Z

superset/security/manager.py

-                        slc := self.get_session.query(Slice)
-                        .filter(Slice.id == slice_id)
-                        .one_or_none()
+                        form_data.get("type") == "NATIVE_FILTER"


I think this needs a few extra validations here in order to grant access:

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

That the datasource is actually used as a native filter on the dashboard. We can probably borrow logic from @Vitor-Avila's PR here; performance shouldn't be a concern here since we only need to check one dashboard

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.

@jfrag1:

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.

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.

Vitor-Avila · 2023-08-18T19:56:16Z

thank you so much for working on this PR @john-bodley! 🙏

john-bodley · 2023-08-18T20:59:13Z

superset-frontend/src/dashboard/components/nativeFilters/utils.ts

@@ -94,6 +95,7 @@ export const getFormData = ({
    viz_type: filterType,
    type,
    dashboardId,
+    native_filter_id: id,


Calling this id seemed a tad terse. I'm not sure if this should be snake or camel case.

eschutho · 2023-08-18T21:51:20Z

superset/security/manager.py

-                        slc := self.get_session.query(Slice)
-                        .filter(Slice.id == slice_id)
-                        .one_or_none()
+                        (


Suggested change

(

(

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?

Yes I will add logic for embedded in a separate PR

@eschutho Black formats the code how it sees fit.

eschutho · 2023-08-18T22:11:21Z

superset/security/manager.py

+                                    [],
+                                )
+                                for target in fltr.get("targets", [])
+                                if native_filter_id == fltr.get("id")


Vitor-Avila · 2023-08-21T02:49:43Z

superset/security/manager.py

+                            # Native filter.
+                            form_data.get("type") == "NATIVE_FILTER"
+                            and (native_filter_id := form_data.get("native_filter_id"))
+                            and dashboard_.json_metadata


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 #25025). Curious to know if you are facing a different behavior on your end.

@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?

@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.

Phew! I'm glad to hear that this was a false alarm. Thanks again for testing the change.

john-bodley · 2023-08-22T05:35:12Z

tests/integration_tests/security_tests.py

        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()


No need to flush these updates to the database transaction given the logic in the test is confined to the same Flask-SQLAlchemy session.

michael-s-molina

LGTM. Thank you for the fix @john-bodley.

(cherry picked from commit 60889d2)

)

pull-request-size bot added the size/M label Aug 18, 2023

john-bodley marked this pull request as ready for review August 18, 2023 19:23

john-bodley requested review from eschutho and michael-s-molina August 18, 2023 19:23

michael-s-molina requested review from nytai and villebro August 18, 2023 19:37

jfrag1 reviewed Aug 18, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-dashboard-rbac-native-filters branch 3 times, most recently from f15ee5d to 10a7bc1 Compare August 18, 2023 20:58

john-bodley commented Aug 18, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-dashboard-rbac-native-filters branch from 10a7bc1 to ef763bf Compare August 18, 2023 21:00

john-bodley added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Aug 18, 2023

eschutho reviewed Aug 18, 2023

View reviewed changes

superset/security/manager.py

[],

)

for target in fltr.get("targets", [])

if native_filter_id == fltr.get("id")

Copy link

Member

eschutho Aug 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

john-bodley force-pushed the john-bodley--fix-dashboard-rbac-native-filters branch from ef763bf to 30a2d35 Compare August 18, 2023 22:16

jfrag1 approved these changes Aug 18, 2023

View reviewed changes

john-bodley changed the title ~~fix: Native dashboard aware dataset permission~~ fix: Native filter dashboard RBAC aware dataset permission Aug 19, 2023

Vitor-Avila reviewed Aug 21, 2023

View reviewed changes

Vitor-Avila approved these changes Aug 21, 2023

View reviewed changes

fix: Native dashboard aware dataset permission

b843dc1

john-bodley force-pushed the john-bodley--fix-dashboard-rbac-native-filters branch from 30a2d35 to b843dc1 Compare August 22, 2023 04:45

pull-request-size bot added size/L and removed size/M labels Aug 22, 2023

john-bodley commented Aug 22, 2023

View reviewed changes

michael-s-molina approved these changes Aug 22, 2023

View reviewed changes

john-bodley merged commit 60889d2 into apache:master Aug 22, 2023
29 checks passed

michael-s-molina pushed a commit that referenced this pull request Aug 23, 2023

fix: Native filter dashboard RBAC aware dataset permission (#25029)

1af6df3

(cherry picked from commit 60889d2)

mistercrunch added the 🍒 3.0.0 label Mar 8, 2024

mistercrunch added 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix: Native filter dashboard RBAC aware dataset permission (apache#25029

ca52a28

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Native filter dashboard RBAC aware dataset permission #25029

fix: Native filter dashboard RBAC aware dataset permission #25029

john-bodley commented Aug 18, 2023 •

edited

Loading

jfrag1 Aug 18, 2023 •

edited

Loading

Vitor-Avila Aug 18, 2023 •

edited

Loading

john-bodley Aug 18, 2023 •

edited

Loading

Vitor-Avila commented Aug 18, 2023

john-bodley Aug 18, 2023

eschutho Aug 18, 2023

eschutho Aug 18, 2023

jfrag1 Aug 18, 2023

john-bodley Aug 18, 2023

eschutho Aug 18, 2023

Vitor-Avila Aug 21, 2023

john-bodley Aug 21, 2023

Vitor-Avila Aug 21, 2023

john-bodley Aug 22, 2023 •

edited

Loading

john-bodley Aug 22, 2023

michael-s-molina left a comment

fix: Native filter dashboard RBAC aware dataset permission #25029

fix: Native filter dashboard RBAC aware dataset permission #25029

Conversation

john-bodley commented Aug 18, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

jfrag1 Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Vitor-Avila Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

john-bodley Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Vitor-Avila commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

john-bodley commented Aug 18, 2023 •

edited

Loading

jfrag1 Aug 18, 2023 •

edited

Loading

Vitor-Avila Aug 18, 2023 •

edited

Loading

john-bodley Aug 18, 2023 •

edited

Loading

john-bodley Aug 22, 2023 •

edited

Loading