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.

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: Allow embedded guest user datasource access with dashboard context #25081

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 8 additions & 2 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -1862,17 +1862,23 @@ def raise_for_access(
or self.is_owner(datasource)
or (
# Grant access to the datasource only if dashboard RBAC is enabled
# or the user is an embedded guest user with access to the dashboard
# 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_ := self.get_session.query(Dashboard)
.filter(Dashboard.id == dashboard_id)
.one_or_none()
)
and dashboard_.roles
and (
(is_feature_enabled("DASHBOARD_RBAC") and dashboard_.roles)
or (
is_feature_enabled("EMBEDDED_SUPERSET")
and self.is_guest_user()
)
)
and (
(
# Native filter.
Expand Down
1 change: 1 addition & 0 deletions superset/viz.py
Original file line number Diff line number Diff line change
Expand Up @@ -1653,6 +1653,7 @@ def run_extra_queries(self) -> None:
query_obj["orderby"] = [(metric, asc)]
self.get_query_context_factory().create(
datasource={"id": self.datasource.id, "type": self.datasource.type},
form_data=self.form_data,
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes #25006, form data wasn't being passed through to raise_for_access

queries=[query_obj],
).raise_for_access()
df = self.get_df_payload(query_obj=query_obj).get("df")
Expand Down
8 changes: 8 additions & 0 deletions tests/integration_tests/fixtures/birth_names_dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ def load_birth_names_dashboard_with_slices_module_scope(load_birth_names_data):
_cleanup(dash_id_to_delete, slices_ids_to_delete)


@pytest.fixture(scope="class")
def load_birth_names_dashboard_with_slices_class_scope(load_birth_names_data):
with app.app_context():
dash_id_to_delete, slices_ids_to_delete = _create_dashboards()
yield
_cleanup(dash_id_to_delete, slices_ids_to_delete)


def _create_dashboards():
table = _create_table(
table_name=BIRTH_NAMES_TBL_NAME,
Expand Down
8 changes: 8 additions & 0 deletions tests/integration_tests/fixtures/world_bank_dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,14 @@ def load_world_bank_dashboard_with_slices_module_scope(load_world_bank_data):
_cleanup(dash_id_to_delete, slices_ids_to_delete)


@pytest.fixture(scope="class")
def load_world_bank_dashboard_with_slices_class_scope(load_world_bank_data):
with app.app_context():
dash_id_to_delete, slices_ids_to_delete = create_dashboard_for_loaded_data()
yield
_cleanup(dash_id_to_delete, slices_ids_to_delete)


def create_dashboard_for_loaded_data():
with app.app_context():
table = create_table_metadata(WB_HEALTH_POPULATION, get_example_database())
Expand Down
258 changes: 251 additions & 7 deletions tests/integration_tests/security/guest_token_security_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,34 @@
# specific language governing permissions and limitations
# under the License.
"""Unit tests for Superset"""
from unittest import mock
import json
from unittest.mock import Mock, patch

import pytest
from flask import g

from superset import db, security_manager
from superset.connectors.sqla.models import SqlaTable
from superset.daos.dashboard import EmbeddedDashboardDAO
from superset.exceptions import SupersetSecurityException
from superset.models.dashboard import Dashboard
from superset.security.guest_token import GuestTokenResourceType
from superset.sql_parse import Table
from superset.utils.core import get_example_default_schema
from superset.utils.database import get_example_database
from tests.integration_tests.base_tests import SupersetTestCase
from tests.integration_tests.fixtures.birth_names_dashboard import (
load_birth_names_dashboard_with_slices,
load_birth_names_dashboard_with_slices_class_scope,
load_birth_names_data,
)
from tests.integration_tests.fixtures.world_bank_dashboard import (
load_world_bank_dashboard_with_slices,
load_world_bank_dashboard_with_slices_class_scope,
load_world_bank_data,
)


@mock.patch.dict(
@patch.dict(
"superset.extensions.feature_flag_manager._feature_flags",
EMBEDDED_SUPERSET=True,
)
Expand All @@ -55,7 +64,7 @@ def test_is_guest_user__guest_user(self):
is_guest = security_manager.is_guest_user(self.authorized_guest())
self.assertTrue(is_guest)

@mock.patch.dict(
@patch.dict(
"superset.extensions.feature_flag_manager._feature_flags",
EMBEDDED_SUPERSET=False,
)
Expand Down Expand Up @@ -91,14 +100,14 @@ def test_get_guest_user_roles_implicit(self):
self.assertEqual(guest.roles, roles)


@mock.patch.dict(
@patch.dict(
"superset.extensions.feature_flag_manager._feature_flags",
EMBEDDED_SUPERSET=True,
)
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices_class_scope")
Copy link
Member Author

Choose a reason for hiding this comment

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

This change speeds these tests up significantly

class TestGuestUserDashboardAccess(SupersetTestCase):
def setUp(self) -> None:
self.dash = db.session.query(Dashboard).filter_by(slug="births").first()
self.dash = self.get_dash_by_slug("births")
self.embedded = EmbeddedDashboardDAO.upsert(self.dash, [])
self.authorized_guest = security_manager.get_guest_user_from_token(
{
Expand Down Expand Up @@ -195,3 +204,238 @@ def test_raise_for_access_dashboard_as_guest_no_rbac(self):

db.session.delete(dash)
db.session.commit()


@patch.dict(
"superset.extensions.feature_flag_manager._feature_flags",
EMBEDDED_SUPERSET=True,
)
@pytest.mark.usefixtures(
"create_dataset",
"load_birth_names_dashboard_with_slices_class_scope",
"load_world_bank_dashboard_with_slices_class_scope",
)
class TestGuestUserDatasourceAccess(SupersetTestCase):
"""
Guest users should only have access to datasources that are associated with a
dashboard they have access to, and only with that dashboard context present
"""

@pytest.fixture(scope="class")
def create_dataset(self):
with self.create_app().app_context():
dataset = SqlaTable(
table_name="dummy_sql_table",
database=get_example_database(),
schema=get_example_default_schema(),
sql="select 123 as intcol, 'abc' as strcol",
)
session = db.session
session.add(dataset)
session.commit()
Copy link
Member

Choose a reason for hiding this comment

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

I suspect the commit is unnecessary. This would mean that lines 240 and 241 become.

db.session.rollback()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I guess it's unnecessary for these and we could instead just run these tests within an uncommitted transaction. Do you feel that's preferable? Here I pretty much just copied the pattern from existing fixtures I saw which all committed their changes.


yield dataset

# rollback
session.delete(dataset)
session.commit()

def setUp(self) -> None:
self.dash = self.get_dash_by_slug("births")
self.other_dash = self.get_dash_by_slug("world_health")
self.embedded = EmbeddedDashboardDAO.upsert(self.dash, [])
self.authorized_guest = security_manager.get_guest_user_from_token(
{
"user": {},
"resources": [{"type": "dashboard", "id": str(self.embedded.uuid)}],
}
)
self.unauthorized_guest = security_manager.get_guest_user_from_token(
{
"user": {},
"resources": [
{"type": "dashboard", "id": "06383667-3e02-4e5e-843f-44e9c5896b6c"}
],
}
)
self.chart = self.get_slice("Girls", db.session, expunge_from_session=False)
self.datasource = self.chart.datasource
self.other_chart = self.get_slice(
"Treemap", db.session, expunge_from_session=False
)
self.other_datasource = self.other_chart.datasource
self.native_filter_datasource = (
db.session.query(SqlaTable).filter_by(table_name="dummy_sql_table").first()
)
self.dash.json_metadata = json.dumps(
{
"native_filter_configuration": [
{
"id": "NATIVE_FILTER-ABCDEFGH",
"targets": [{"datasetId": self.native_filter_datasource.id}],
},
]
}
)

def test_raise_for_access__happy_path(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.datasource,
form_data={
"dashboardId": self.dash.id,
"slice_id": self.chart.id,
},
)
}
)

def test_raise_for_access__native_filter_happy_path(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.native_filter_datasource,
form_data={
"dashboardId": self.dash.id,
"native_filter_id": "NATIVE_FILTER-ABCDEFGH",
"type": "NATIVE_FILTER",
},
)
}
)

def test_raise_for_access__no_dashboard_in_form_data(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.datasource,
form_data={
"slice_id": self.chart.id,
},
)
}
)

def test_raise_for_access__no_chart_in_form_data(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.datasource,
form_data={
"dashboardId": self.dash.id,
},
)
}
)

def test_raise_for_access__chart_not_on_dashboard(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.other_datasource,
form_data={
"dashboardId": self.dash.id,
"slice_id": self.other_chart.id,
},
)
}
)

def test_raise_for_access__chart_doesnt_belong_to_datasource(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.other_datasource,
form_data={
"dashboardId": self.dash.id,
"slice_id": self.chart.id,
},
)
}
)

def test_raise_for_access__native_filter_no_id_in_form_data(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.native_filter_datasource,
form_data={
"dashboardId": self.dash.id,
"type": "NATIVE_FILTER",
},
)
}
)

def test_raise_for_access__native_filter_datasource_not_associated(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.other_datasource,
form_data={
"dashboardId": self.dash.id,
"native_filter_id": "NATIVE_FILTER-ABCDEFGH",
"type": "NATIVE_FILTER",
},
)
}
)

@patch.dict(
"superset.extensions.feature_flag_manager._feature_flags",
EMBEDDED_SUPERSET=False,
)
def test_raise_for_access__embedded_feature_flag_off(self):
g.user = self.authorized_guest
for kwarg in ["viz", "query_context"]:
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.datasource,
form_data={
"dashboardId": self.dash.id,
"slice_id": self.chart.id,
},
)
}
)

def test_raise_for_access__unauthorized_guest_user(self):
g.user = self.unauthorized_guest
for kwarg in ["viz", "query_context"]:
with self.assertRaises(SupersetSecurityException):
security_manager.raise_for_access(
**{
kwarg: Mock(
datasource=self.datasource,
form_data={
"dashboardId": self.dash.id,
"slice_id": self.chart.id,
},
)
}
)
Copy link
Member

Choose a reason for hiding this comment

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

nice tests!

2 changes: 1 addition & 1 deletion tests/integration_tests/security_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1652,7 +1652,7 @@ def test_raise_for_access_query(self, mock_can_access, mock_is_owner):
def test_raise_for_access_query_context(
self, mock_can_access_schema, mock_can_access, mock_is_owner, mock_g
):
query_context = Mock(datasource=self.get_datasource_mock())
query_context = Mock(datasource=self.get_datasource_mock(), form_data={})

mock_can_access_schema.return_value = True
security_manager.raise_for_access(query_context=query_context)
Expand Down
Loading