[superset] 01/01: fix: embedded dashboard check

[be...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

beto pushed a commit to branch fix_guest_token
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 04e65d5fd713713c056418c2d9cb643bd278c7a8
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Thu Jul 13 08:41:23 2023 -0700

    fix: embedded dashboard check
---
 superset/security/manager.py                       | 45 +++++++++++++---------
 .../security/guest_token_security_tests.py         | 29 ++++++++++++++
 2 files changed, 56 insertions(+), 18 deletions(-)

diff --git a/superset/security/manager.py b/superset/security/manager.py
index 942ed66776..28354ac18d 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -2006,28 +2006,37 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
         from superset import is_feature_enabled
         from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
 
-        if self.is_guest_user() and dashboard.embedded:
+        if self.is_guest_user():
+            # Guest user is currently used for embedded dashboards only. If the guest user
+            # doesn't have access to the dashboard, ignore all other checks.
             if self.has_guest_access(dashboard):
                 return
-        else:
-            if self.is_admin() or self.is_owner(dashboard):
-                return
+            raise DashboardAccessDeniedError()
 
-            # RBAC and legacy (datasource inferred) access controls.
-            if is_feature_enabled("DASHBOARD_RBAC") and dashboard.roles:
-                if dashboard.published and {role.id for role in dashboard.roles} & {
-                    role.id for role in self.get_user_roles()
-                }:
-                    return
-            elif (
-                not dashboard.published
-                or not dashboard.datasources
-                or any(
-                    self.can_access_datasource(datasource)
-                    for datasource in dashboard.datasources
-                )
-            ):
+        if self.is_admin() or self.is_owner(dashboard):
+            return
+
+        # RBAC and legacy (datasource inferred) access controls.
+        if is_feature_enabled("DASHBOARD_RBAC") and dashboard.roles:
+            if dashboard.published and {role.id for role in dashboard.roles} & {
+                role.id for role in self.get_user_roles()
+            }:
                 return
+        elif (
+            # To understand why we rely on status and give access to draft dashboards
+            # without roles take a look at:
+            #
+            #   - https://github.com/apache/superset/pull/24350#discussion_r1225061550
+            #   - https://github.com/apache/superset/pull/17511#issuecomment-975870169
+            #
+            not dashboard.published
+            or not dashboard.datasources
+            or any(
+                self.can_access_datasource(datasource)
+                for datasource in dashboard.datasources
+            )
+        ):
+            return
 
         raise DashboardAccessDeniedError()
 
diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py
index 86d02975d0..e0517c9b28 100644
--- a/tests/integration_tests/security/guest_token_security_tests.py
+++ b/tests/integration_tests/security/guest_token_security_tests.py
@@ -204,3 +204,32 @@ class TestGuestUserDashboardAccess(SupersetTestCase):
 
         with self.assertRaises(DashboardAccessDeniedError):
             security_manager.raise_for_dashboard_access(self.dash)
+
+    def test_raise_for_dashboard_access_as_guest_no_rbac(self):
+        """
+        Test that guest account has no access to other dashboards.
+
+        A bug in the ``raise_for_dashboard_access`` logic allowed the guest user to
+        fetch data from other dashboards, as long as the other dashboard:
+
+          - was not embedded AND
+            - was not published OR
+            - had at least 1 datasource that the user had access to.
+
+        """
+        g.user = self.unauthorized_guest
+
+        # Create a draft dashboard that is not embedded
+        dash = Dashboard()
+        dash.dashboard_title = "My Dashboard"
+        dash.owners = []
+        dash.slices = []
+        dash.published = False
+        db.session.add(dash)
+        db.session.commit()
+
+        with self.assertRaises(DashboardAccessDeniedError):
+            security_manager.raise_for_dashboard_access(dash)
+
+        db.session.delete(dash)
+        db.session.commit()