Skip to content

Commit

Permalink
fix: Address regression introduced in #24789 (#25008)
Browse files Browse the repository at this point in the history
  • Loading branch information
john-bodley authored Aug 18, 2023
1 parent 52c7186 commit 3f93755
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 12 deletions.
24 changes: 21 additions & 3 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -1797,7 +1797,8 @@ def raise_for_access(
# pylint: disable=import-outside-toplevel
from superset import is_feature_enabled
from superset.connectors.sqla.models import SqlaTable
from superset.daos.dashboard import DashboardDAO
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
from superset.sql_parse import Table

if database and table or query:
Expand Down Expand Up @@ -1859,10 +1860,27 @@ def raise_for_access(
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
# Grant access to the datasource only if dashboard RBAC is enabled
# and said datasource is associated with the dashboard chart in
# question.
form_data
and is_feature_enabled("DASHBOARD_RBAC")
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
and self.can_access_dashboard(dashboard)
and (
dashboard_ := self.get_session.query(Dashboard)
.filter(Dashboard.id == dashboard_id)
.one_or_none()
)
and dashboard_.roles
and (slice_id := form_data.get("slice_id"))
and (
slc := self.get_session.query(Slice)
.filter(Slice.id == slice_id)
.one_or_none()
)
and slc in dashboard_.slices
and slc.datasource == datasource
and self.can_access_dashboard(dashboard_)
)
):
raise SupersetSecurityException(
Expand Down
92 changes: 83 additions & 9 deletions tests/integration_tests/security_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1698,6 +1698,7 @@ def test_raise_for_access_viz(
security_manager.raise_for_access(viz=test_viz)

@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
@pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
@with_feature_flags(DASHBOARD_RBAC=True)
@patch("superset.security.manager.g")
@patch("superset.security.SupersetSecurityManager.is_owner")
Expand All @@ -1710,28 +1711,101 @@ def test_raise_for_access_rbac(
mock_is_owner,
mock_g,
):
dashboard = self.get_dash_by_slug("births")
births = self.get_dash_by_slug("births")
girls = self.get_slice("Girls", db.session, expunge_from_session=False)
birth_names = girls.datasource

obj = Mock(
datasource=self.get_datasource_mock(),
form_data={"dashboardId": dashboard.id},
)
world_health = self.get_dash_by_slug("world_health")
treemap = self.get_slice("Treemap", db.session, expunge_from_session=False)

mock_g.user = security_manager.find_user("gamma")
mock_is_owner.return_value = False
mock_can_access.return_value = False
mock_can_access_schema.return_value = False

for kwarg in ["query_context", "viz"]:
dashboard.roles = []
births.roles = []
db.session.flush()

# No dashboard roles.
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(**{kwarg: obj})
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=birth_names,
form_data={
"dashboardId": births.id,
"slice_id": girls.id,
},
)
}
)

dashboard.roles = [self.get_role("Gamma")]
births.roles = [self.get_role("Gamma")]
db.session.flush()
security_manager.raise_for_access(**{kwarg: obj})

# Undefined dashboard.
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=birth_names,
form_data={},
)
}
)

# Undefined dashboard chart.
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=birth_names,
form_data={"dashboardId": births.id},
)
}
)

# Ill-defined dashboard chart.
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=birth_names,
form_data={
"dashboardId": births.id,
"slice_id": treemap.id,
},
)
}
)

# Dashboard chart not associated with said datasource.
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=birth_names,
form_data={
"dashboardId": world_health.id,
"slice_id": treemap.id,
},
)
}
)

# Dashboard chart associated with said datasource.
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=birth_names,
form_data={
"dashboardId": births.id,
"slice_id": girls.id,
},
)
}
)

db.session.rollback()

Expand Down

0 comments on commit 3f93755

Please sign in to comment.