[GitHub] [superset] villebro commented on a diff in pull request #24350: fix: Address dashboard permission regression in #23586

["villebro (via GitHub)" <gi...@apache.org>]

villebro commented on code in PR #24350:
URL: https://github.com/apache/superset/pull/24350#discussion_r1226210052


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

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



##########
superset/security/manager.py:
##########
@@ -1995,38 +2010,40 @@ def raise_for_user_activity_access(user_id: int) -> None:
     def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
         """
         Raise an exception if the user cannot access the dashboard.
-        This does not check for the required role/permission pairs,
-        it only concerns itself with entity relationships.
+
+        This does not check for the required role/permission pairs, it only concerns
+        itself with entity relationships.
 
         :param dashboard: Dashboard the user wants access to
         :raises DashboardAccessDeniedError: If the user cannot access the resource
         """
+
         # 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:
-                return True
-
-            return any(
-                dashboard_role.id
-                in [user_role.id for user_role in self.get_user_roles()]
-                for dashboard_role in dashboard.roles
-            )
-
         if self.is_guest_user() and dashboard.embedded:
-            can_access = self.has_guest_access(dashboard)
+            if self.has_guest_access(dashboard):
+                return
         else:
-            can_access = (
-                self.is_admin()
-                or self.is_owner(dashboard)
-                or (dashboard.published and has_rbac_access())

Review Comment:
   > 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.



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