Skip to content

Commit

Permalink
fix: embedded dashboard check (apache#24690)
Browse files Browse the repository at this point in the history
  • Loading branch information
betodealmeida authored Jul 13, 2023
1 parent c54d898 commit fae4255
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 18 deletions.
45 changes: 27 additions & 18 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
29 changes: 29 additions & 0 deletions tests/integration_tests/security/guest_token_security_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

0 comments on commit fae4255

Please sign in to comment.