diff --git a/superset/security/manager.py b/superset/security/manager.py index 942ed66776d8a..28354ac18dafe 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2006,28 +2006,37 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: 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 86d02975d093a..e0517c9b28ae3 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 @@ def test_raise_for_dashboard_access_as_unauthorized_guest(self): 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()