Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Dashboard aware RBAC dataset permission #24789

Merged
merged 1 commit into from
Aug 4, 2023
Merged

fix: Dashboard aware RBAC dataset permission #24789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 9 additions & 34 deletions superset/security/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@

from flask import current_app, Flask, g, Request
from flask_appbuilder import Model
from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_appbuilder.security.sqla.manager import SecurityManager
from flask_appbuilder.security.sqla.models import (
assoc_permissionview_role,
Expand Down Expand Up @@ -1781,7 +1780,7 @@ def raise_for_access(

# pylint: disable=import-outside-toplevel
from superset.connectors.sqla.models import SqlaTable
from superset.extensions import feature_flag_manager
from superset.daos.dashboard import DashboardDAO
from superset.sql_parse import Table

if database and table or query:
Expand Down Expand Up @@ -1827,25 +1826,26 @@ def raise_for_access(
)

if datasource or query_context or viz:
form_data = None

if query_context:
datasource = query_context.datasource
form_data = query_context.form_data
elif viz:
datasource = viz.datasource
form_data = viz.form_data

assert datasource

should_check_dashboard_access = (
feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC")
or self.is_guest_user()
)

if not (
self.can_access_schema(datasource)
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
should_check_dashboard_access
and self.can_access_based_on_dashboard(datasource)
form_data
and (dashboard_id := form_data.get("dashboardId"))
and (dashboard := DashboardDAO.find_by_id(dashboard_id))
and self.can_access_dashboard(dashboard)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with this feature either, but just noticed that the previous version checks for datasource access and the new one checks for dashboard access only. Is that what we want?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eschutho the logic is as follows. If you don't have explicit access to dataset you then fallback to the form-data of either the visualization or query-context to try to decipher the context. If it's in the context of a dashboard, i.e., you're viewing a chart in a dashboard, one checks whether you have access to the dashboard—per the can_access_dashboard method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check that the purported chart whose data is being loaded actually exists on the dashboard?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@villebro is the logic on line #1846 not suffice, i.e., extracting the dashboard ID from the form-data (which could stem from either a legacy visualization or query-context)?

)
):
raise SupersetSecurityException(
Expand Down Expand Up @@ -2036,31 +2036,6 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:

raise DashboardAccessDeniedError()

@staticmethod
def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool:
# pylint: disable=import-outside-toplevel
from superset import db
from superset.dashboards.filters import DashboardAccessFilter
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice

dashboard_ids = db.session.query(Dashboard.id)
dashboard_ids = DashboardAccessFilter(
"id", SQLAInterface(Dashboard, db.session)
).apply(dashboard_ids, None)

datasource_class = type(datasource)
query = (
db.session.query(Dashboard.id)
.join(Slice, Dashboard.slices)
.join(Slice.table)
.filter(datasource_class.id == datasource.id)
.filter(Dashboard.id.in_(dashboard_ids))
)

exists = db.session.query(query.exists()).scalar()
return exists

@staticmethod
def _get_current_epoch_time() -> float:
"""This is used so the tests can mock time"""
Expand Down
3 changes: 1 addition & 2 deletions tests/integration_tests/dashboards/security/security_rbac_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ def test_get_dashboard_view__user_access_with_dashboard_permission(self):
return

# arrange

username = random_str()
new_role = f"role_{random_str()}"
self.create_user_with_roles(username, [new_role], should_create_roles=True)
Expand All @@ -191,7 +190,7 @@ def test_get_dashboard_view__user_access_with_dashboard_permission(self):

request_payload = get_query_context("birth_names")
rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
self.assertEqual(rv.status_code, 200)
self.assertEqual(rv.status_code, 403)
Copy link
Member Author

@john-bodley john-bodley Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test (alongside others) was failing after the change which likely indicates that either i) RBAC was supposed to expand beyond the scope/context of the dashboard (and thus my understanding of said issue was misconstrued), or ii) was a mistake.


# post
revoke_access_to_dashboard(dashboard_to_access, new_role)
Expand Down
35 changes: 0 additions & 35 deletions tests/integration_tests/security/guest_token_security_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,41 +159,6 @@ def test_has_guest_access__unauthorized_guest_user__different_resource_type(self
has_guest_access = security_manager.has_guest_access(self.dash)
self.assertFalse(has_guest_access)

def test_chart_raise_for_access_as_guest(self):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional on my logic being correct, these tests are now superfluous as the irrespective of whether a guest is authorized or not, they don't have access to the chart, dataset, etc. unless it's in the context of RBAC. The logic previously outlined here condenses to the same logic which is defined in the security_tests.py.

chart = self.dash.slices[0]
g.user = self.authorized_guest

security_manager.raise_for_access(viz=chart)

def test_chart_raise_for_access_as_unauthorized_guest(self):
chart = self.dash.slices[0]
g.user = self.unauthorized_guest

with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(viz=chart)

def test_dataset_raise_for_access_as_guest(self):
dataset = self.dash.slices[0].datasource
g.user = self.authorized_guest

security_manager.raise_for_access(datasource=dataset)

def test_dataset_raise_for_access_as_unauthorized_guest(self):
dataset = self.dash.slices[0].datasource
g.user = self.unauthorized_guest

with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(datasource=dataset)

def test_guest_token_does_not_grant_access_to_underlying_table(self):
sqla_table = self.dash.slices[0].table
table = Table(table=sqla_table.table_name)

g.user = self.authorized_guest

with self.assertRaises(Exception):
security_manager.raise_for_access(table=table, database=sqla_table.database)

def test_raise_for_dashboard_access_as_guest(self):
g.user = self.authorized_guest

Expand Down
47 changes: 45 additions & 2 deletions tests/integration_tests/security_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
from superset.utils.urls import get_url_host

from .base_tests import SupersetTestCase
from tests.integration_tests.conftest import with_feature_flags
from tests.integration_tests.fixtures.public_role import (
public_role_like_gamma,
public_role_like_test_role,
Expand Down Expand Up @@ -1643,17 +1644,19 @@ def test_raise_for_access_query(self, mock_can_access, mock_is_owner):
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(query=query)

@patch("superset.security.manager.g")
@patch("superset.security.SupersetSecurityManager.is_owner")
@patch("superset.security.SupersetSecurityManager.can_access")
@patch("superset.security.SupersetSecurityManager.can_access_schema")
def test_raise_for_access_query_context(
self, mock_can_access_schema, mock_can_access, mock_is_owner
self, mock_can_access_schema, mock_can_access, mock_is_owner, mock_g
):
query_context = Mock(datasource=self.get_datasource_mock())

mock_can_access_schema.return_value = True
security_manager.raise_for_access(query_context=query_context)

mock_g.user = security_manager.find_user("gamma")
mock_can_access.return_value = False
mock_can_access_schema.return_value = False
mock_is_owner.return_value = False
Expand All @@ -1674,24 +1677,64 @@ def test_raise_for_access_table(self, mock_can_access):
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(database=database, table=table)

@patch("superset.security.manager.g")
@patch("superset.security.SupersetSecurityManager.is_owner")
@patch("superset.security.SupersetSecurityManager.can_access")
@patch("superset.security.SupersetSecurityManager.can_access_schema")
def test_raise_for_access_viz(
self, mock_can_access_schema, mock_can_access, mock_is_owner
self, mock_can_access_schema, mock_can_access, mock_is_owner, mock_g
):
test_viz = viz.TimeTableViz(self.get_datasource_mock(), form_data={})

mock_can_access_schema.return_value = True
security_manager.raise_for_access(viz=test_viz)

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

with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(viz=test_viz)

@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is needed, i.e., test_get_dashboard_view__user_access_with_dashboard_permission might be suffice.

@with_feature_flags(DASHBOARD_RBAC=True)
@patch("superset.security.manager.g")
@patch("superset.security.SupersetSecurityManager.is_owner")
@patch("superset.security.SupersetSecurityManager.can_access")
@patch("superset.security.SupersetSecurityManager.can_access_schema")
def test_raise_for_access_rbac(
self,
mock_can_access_schema,
mock_can_access,
mock_is_owner,
mock_g,
):
dashboard = self.get_dash_by_slug("births")

obj = Mock(
datasource=self.get_datasource_mock(),
form_data={"dashboardId": dashboard.id},
)

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 = []
db.session.flush()

with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(**{kwarg: obj})

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

db.session.rollback()

@patch("superset.security.manager.g")
def test_get_user_roles(self, mock_g):
admin = security_manager.find_user("admin")
Expand Down