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

Jump to bottom

fix: prevent guest users from changing columns #29530

Merged
merged 4 commits into from
Jul 10, 2024
Merged

fix: prevent guest users from changing columns #29530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08833b6
fix: prevent guest users from changing columns
betodealmeida Jul 9, 2024
229fa1e
Consider groupby as well
betodealmeida Jul 10, 2024
bee3895
Rename parameters
betodealmeida Jul 10, 2024
8b6e116
Add one more test
betodealmeida Jul 10, 2024
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
58 changes: 31 additions & 27 deletions superset/security/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@
GuestUser,
)
from superset.sql_parse import extract_tables_from_jinja_sql, Table
from superset.superset_typing import Metric
from superset.utils import json
from superset.utils.core import (
DatasourceName,
Expand Down Expand Up @@ -145,11 +144,11 @@ def __init__(self, **kwargs: Any) -> None:
RoleModelView.related_views = []


def freeze_metric(metric: Metric) -> str:
def freeze_value(value: Any) -> str:
"""
Used to compare metric sets.
Used to compare column and metric sets.
"""
return json.dumps(metric, sort_keys=True)
return json.dumps(value, sort_keys=True)


def query_context_modified(query_context: "QueryContext") -> bool:
Expand All @@ -170,32 +169,37 @@ def query_context_modified(query_context: "QueryContext") -> bool:
if form_data.get("slice_id") != stored_chart.id:
return True

# compare form_data
requested_metrics = {
freeze_metric(metric) for metric in form_data.get("metrics") or []
}
stored_metrics = {
freeze_metric(metric)
for metric in stored_chart.params_dict.get("metrics") or []
}
if not requested_metrics.issubset(stored_metrics):
return True
stored_query_context = (
json.loads(cast(str, stored_chart.query_context))
if stored_chart.query_context
else None
)

# compare queries in query_context
queries_metrics = {
freeze_metric(metric)
for query in query_context.queries
for metric in query.metrics or []
}
# compare columns and metrics in form_data with stored values
for key in ["metrics", "columns", "groupby"]:
requested_values = {freeze_value(value) for value in form_data.get(key) or []}
stored_values = {
freeze_value(value) for value in stored_chart.params_dict.get(key) or []
}
if not requested_values.issubset(stored_values):
return True

if stored_chart.query_context:
stored_query_context = json.loads(cast(str, stored_chart.query_context))
for query in stored_query_context.get("queries") or []:
stored_metrics.update(
{freeze_metric(metric) for metric in query.get("metrics") or []}
)
# compare queries in query_context
queries_values = {
freeze_value(value)
for query in query_context.queries
for value in getattr(query, key, []) or []
}
if stored_query_context:
for query in stored_query_context.get("queries") or []:
stored_values.update(
{freeze_value(value) for value in query.get(key) or []}
)

if not queries_values.issubset(stored_values):
return True

return not queries_metrics.issubset(stored_metrics)
return False


class SupersetSecurityManager( # pylint: disable=too-many-public-methods
Expand Down
158 changes: 155 additions & 3 deletions tests/unit_tests/security/manager_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
SupersetSecurityManager,
)
from superset.sql_parse import Table
from superset.superset_typing import AdhocMetric
from superset.superset_typing import AdhocColumn, AdhocMetric
from superset.utils.core import override_user


Expand Down Expand Up @@ -59,10 +59,24 @@ def stored_metrics() -> list[AdhocMetric]:
]


@pytest.fixture
def stored_columns() -> list[AdhocColumn]:
"""
Return a list of columns.
"""
return [
{
"label": "My column",
"sqlExpression": "UPPER(name)",
},
]


def test_raise_for_access_guest_user_ok(
mocker: MockerFixture,
app_context: None,
stored_metrics: list[AdhocMetric],
stored_columns: list[AdhocColumn],
) -> None:
"""
Test that guest user can submit an unmodified chart payload.
Expand All @@ -76,11 +90,43 @@ def test_raise_for_access_guest_user_ok(
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"metrics": stored_metrics,
"columns": stored_columns,
}

query_context.form_data = {
"slice_id": 42,
"metrics": stored_metrics,
"columns": stored_columns,
}
query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore
sm.raise_for_access(query_context=query_context)


def test_raise_for_access_guest_user_ok_subset(
mocker: MockerFixture,
app_context: None,
stored_metrics: list[AdhocMetric],
stored_columns: list[AdhocColumn],
) -> None:
"""
Test that guest user can submit a request of a subset of the metrics/columns.
"""
sm = SupersetSecurityManager(appbuilder)
mocker.patch.object(sm, "is_guest_user", return_value=True)
mocker.patch.object(sm, "can_access", return_value=True)

query_context = mocker.MagicMock()
query_context.slice_.id = 42
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"metrics": stored_metrics,
"columns": stored_columns,
}

query_context.form_data = {
"slice_id": 42,
"metrics": [],
"columns": [],
}
query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore
sm.raise_for_access(query_context=query_context)
Expand Down Expand Up @@ -114,7 +160,7 @@ def test_raise_for_access_guest_user_tampered_id(
sm.raise_for_access(query_context=query_context)


def test_raise_for_access_guest_user_tampered_form_data(
def test_raise_for_access_guest_user_tampered_form_data_metrics(
mocker: MockerFixture,
app_context: None,
stored_metrics: list[AdhocMetric],
Expand Down Expand Up @@ -151,7 +197,77 @@ def test_raise_for_access_guest_user_tampered_form_data(
sm.raise_for_access(query_context=query_context)


def test_raise_for_access_guest_user_tampered_queries(
def test_raise_for_access_guest_user_tampered_form_data_columns(
mocker: MockerFixture,
app_context: None,
stored_columns: list[AdhocColumn],
) -> None:
"""
Test that guest user cannot modify columns in the form data.
"""
sm = SupersetSecurityManager(appbuilder)
mocker.patch.object(sm, "is_guest_user", return_value=True)
mocker.patch.object(sm, "can_access", return_value=True)

query_context = mocker.MagicMock()
query_context.slice_.id = 42
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"columns": stored_columns,
}

tampered_columns = [
{
"label": "My column",
"sqlExpression": "list_secret()",
"expressionType": "SQL",
},
]

query_context.form_data = {
"slice_id": 42,
"columns": tampered_columns,
}
with pytest.raises(SupersetSecurityException):
sm.raise_for_access(query_context=query_context)


def test_raise_for_access_guest_user_tampered_form_data_groupby(
mocker: MockerFixture,
app_context: None,
stored_columns: list[AdhocColumn],
) -> None:
"""
Test that guest user cannot modify groupby in the form data.
"""
sm = SupersetSecurityManager(appbuilder)
mocker.patch.object(sm, "is_guest_user", return_value=True)
mocker.patch.object(sm, "can_access", return_value=True)

query_context = mocker.MagicMock()
query_context.slice_.id = 42
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"groupby": stored_columns,
}

tampered_columns = [
{
"label": "My column",
"sqlExpression": "list_secret()",
"expressionType": "SQL",
},
]

query_context.form_data = {
"slice_id": 42,
"columns": tampered_columns,
}
with pytest.raises(SupersetSecurityException):
sm.raise_for_access(query_context=query_context)


def test_raise_for_access_guest_user_tampered_queries_metrics(
mocker: MockerFixture,
app_context: None,
stored_metrics: list[AdhocMetric],
Expand Down Expand Up @@ -189,6 +305,42 @@ def test_raise_for_access_guest_user_tampered_queries(
sm.raise_for_access(query_context=query_context)


def test_raise_for_access_guest_user_tampered_queries_columns(
mocker: MockerFixture,
app_context: None,
stored_columns: list[AdhocColumn],
) -> None:
"""
Test that guest user cannot modify columns in the queries.
"""
sm = SupersetSecurityManager(appbuilder)
mocker.patch.object(sm, "is_guest_user", return_value=True)
mocker.patch.object(sm, "can_access", return_value=True)

query_context = mocker.MagicMock()
query_context.slice_.id = 42
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"columns": stored_columns,
}

tampered_columns = [
{
"label": "My column",
"sqlExpression": "list_secret()",
"expressionType": "SQL",
}
]

query_context.form_data = {
"slice_id": 42,
"columns": stored_columns,
}
query_context.queries = [QueryObject(metrics=tampered_columns)] # type: ignore
with pytest.raises(SupersetSecurityException):
sm.raise_for_access(query_context=query_context)


def test_raise_for_access_query_default_schema(
mocker: MockerFixture,
app_context: None,
Expand Down
Loading