[superset] branch fix_guest_token created (now 04e65d5fd7)

[be...@apache.org]

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

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


      at 04e65d5fd7 fix: embedded dashboard check

This branch includes the following new commits:

     new 04e65d5fd7 fix: embedded dashboard check

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

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