Skip to content

Commit

Permalink
fix: prevent guest users from changing columns (#29530)
Browse files Browse the repository at this point in the history
  • Loading branch information
@betodealmeida
betodealmeida committed Jul 10, 2024
1 parent 1d35ca4 commit 67df4e3
Show file tree
Hide file tree
Showing 2 changed files with 186 additions and 30 deletions.
58 changes: 31 additions & 27 deletions superset/security/manager.py
View file
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
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

0 comments on commit 67df4e3

Please sign in to comment.