fix: Address dashboard permission regression in #23586 #24350

john-bodley · 2023-06-10T02:55:36Z

SUMMARY

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

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

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

john-bodley · 2023-06-10T02:58:16Z

superset/utils/decorators.py

@@ -114,27 +111,3 @@ def wrapped(*args: Any, **kwargs: Any) -> Any:

 def on_security_exception(self: Any, ex: Exception) -> Response:
    return self.response(403, **{"message": utils.error_msg_from_exception(ex)})
-
-
-# noinspection PyPackageRequirements


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

john-bodley · 2023-06-10T03:01:28Z

superset/security/manager.py

+            if self.is_admin() or self.is_owner(dashboard):
+                return
+
+            # RBAC and legacy (datasource inferred) access controls.


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

john-bodley · 2023-06-10T03:03:59Z

superset/security/manager.py

        # pylint: disable=import-outside-toplevel
        from superset import is_feature_enabled
        from superset.dashboards.commands.exceptions import DashboardAccessDeniedError

-        def has_rbac_access() -> bool:


This logic has been inlined below.

john-bodley · 2023-06-10T03:05:40Z

superset/security/manager.py

        # pylint: disable=import-outside-toplevel
        from superset import is_feature_enabled
        from superset.dashboards.commands.exceptions import DashboardAccessDeniedError

-        def has_rbac_access() -> bool:
-            if not is_feature_enabled("DASHBOARD_RBAC") or not dashboard.roles:


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

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

john-bodley · 2023-06-10T03:53:34Z

superset/security/manager.py

-            can_access = (
-                self.is_admin()
-                or self.is_owner(dashboard)
-                or (dashboard.published and has_rbac_access())


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

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

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

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

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

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

codecov · 2023-06-10T05:34:55Z

Codecov Report

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

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

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

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

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

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

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

john-bodley · 2023-06-10T06:29:46Z

superset/security/manager.py

-                self.is_admin()
-                or self.is_owner(dashboard)
-                or (dashboard.published and has_rbac_access())
-                or (not dashboard.published and not dashboard.roles)


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

john-bodley · 2023-06-10T20:56:58Z

tests/integration_tests/dashboards/api_tests.py

@@ -506,43 +463,6 @@ def test_get_dashboard_no_data_access(self):
        db.session.delete(dashboard)
        db.session.commit()

-    def test_get_draft_dashboard_without_roles_by_uuid(self):


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

john-bodley · 2023-06-10T21:11:03Z

superset/views/core.py

        """
+
+        dashboard = Dashboard.get(dashboard_id_or_slug)


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

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

john-bodley · 2023-06-10T21:16:03Z

superset/views/core.py

-            flash(DashboardAccessDeniedError.message, "danger")
-            return redirect(DASHBOARD_LIST_URL)
-
-        dash_edit_perm = security_manager.is_owner(


This is inlined later for improved readability.

john-bodley · 2023-06-10T21:16:31Z

superset/views/core.py

        )

-        bootstrap_data = {


This is inlined later.

john-bodley · 2023-06-10T21:42:41Z

tests/integration_tests/dashboards/security/security_dataset_tests.py

@@ -234,3 +235,4 @@ def test_get_dashboards_api_no_data_access(self):
        self.assert200(rv)
        data = json.loads(rv.data.decode("utf-8"))
        self.assertEqual(0, data["count"])
+        DashboardDAO.delete(dashboard)


Previously this was non-idempotent.

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

villebro

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

villebro · 2023-06-12T07:33:43Z

tests/integration_tests/dashboards/security/security_dataset_tests.py

@@ -234,3 +235,4 @@ def test_get_dashboards_api_no_data_access(self):
        self.assert200(rv)
        data = json.loads(rv.data.decode("utf-8"))
        self.assertEqual(0, data["count"])
+        DashboardDAO.delete(dashboard)


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

villebro · 2023-06-12T07:39:24Z

superset/security/manager.py

-            can_access = (
-                self.is_admin()
-                or self.is_owner(dashboard)
-                or (dashboard.published and has_rbac_access())


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

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

michael-s-molina · 2023-06-12T12:17:38Z

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

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

pull-request-size bot added the size/L label Jun 10, 2023