Skip to content

Commit

Permalink
fix: check orderby (apache#31156)
Browse files Browse the repository at this point in the history
  • Loading branch information
@betodealmeida
betodealmeida authored Nov 26, 2024
1 parent 97683ec commit 7f2e752
Show file tree
Hide file tree
Showing 2 changed files with 212 additions and 23 deletions.
2 changes: 1 addition & 1 deletion superset/security/manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ def query_context_modified(query_context: "QueryContext") -> bool:
("metrics", ["metrics"]),
("columns", ["columns", "groupby"]),
("groupby", ["columns", "groupby"]),
("orderby", ["orderby"]),
]:
requested_values = {freeze_value(value) for value in form_data.get(key) or []}
stored_values = {
Expand Down Expand Up @@ -2172,7 +2173,6 @@ def raise_for_access(
:param schema: Optional schema name
:raises SupersetSecurityException: If the user cannot access the resource
"""

# pylint: disable=import-outside-toplevel
from superset import is_feature_enabled
from superset.connectors.sqla.models import SqlaTable
Expand Down
233 changes: 211 additions & 22 deletions tests/unit_tests/security/manager_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -699,34 +699,34 @@ def test_query_context_modified_sankey_tampered(mocker: MockerFixture) -> None:
"""
query_context = mocker.MagicMock()
query_context.queries = [
{
"apply_fetch_values_predicate": False,
"columns": ["bot_id", "channel_id"],
"extras": {"having": "", "where": ""},
"filter": [
QueryObject(
apply_fetch_values_predicate=False,
columns=["bot_id", "channel_id"],
extras={"having": "", "where": ""},
filter=[
{
"col": "bot_profile__updated",
"op": "TEMPORAL_RANGE",
"val": "No filter",
}
],
"from_dttm": None,
"granularity": None,
"inner_from_dttm": None,
"inner_to_dttm": None,
"is_rowcount": False,
"is_timeseries": False,
"metrics": ["count"],
"order_desc": True,
"orderby": [],
"row_limit": 10000,
"row_offset": 0,
"series_columns": [],
"series_limit": 0,
"series_limit_metric": None,
"time_shift": None,
"to_dttm": None,
}
from_dttm=None,
granularity=None,
inner_from_dttm=None,
inner_to_dttm=None,
is_rowcount=False,
is_timeseries=False,
metrics=["count"],
order_desc=True,
orderby=[],
row_limit=10000,
row_offset=0,
series_columns=[],
series_limit=0,
series_limit_metric=None,
time_shift=None,
to_dttm=None,
),
]
query_context.form_data = {
"datasource": "12__table",
Expand Down Expand Up @@ -838,6 +838,195 @@ def test_query_context_modified_sankey_tampered(mocker: MockerFixture) -> None:
assert not query_context_modified(query_context)


def test_query_context_modified_orderby(mocker: MockerFixture) -> None:
"""
Test the `query_context_modified` function when the ORDER BY is modified.
"""
tampered_groupby: AdhocMetric = {
"aggregate": "",
"column": None,
"expressionType": "SQL",
"hasCustomLabel": False,
"label": "random()",
"sqlExpression": "random()",
}

query_context = mocker.MagicMock()
query_context.queries = [
QueryObject(
apply_fetch_values_predicate=False,
columns=["gender"],
extras={"having": "", "where": ""},
filter=[{"col": "ds", "op": "TEMPORAL_RANGE", "val": "No filter"}],
from_dttm=None,
granularity=None,
inner_from_dttm=None,
inner_to_dttm=None,
is_rowcount=False,
is_timeseries=False,
metrics=["count"],
order_desc=True,
orderby=[(tampered_groupby, False)],
row_limit=1000,
row_offset=0,
series_columns=[],
series_limit=0,
series_limit_metric=tampered_groupby,
time_shift=None,
to_dttm=None,
),
]
query_context.form_data = {
"datasource": "2__table",
"viz_type": "table",
"slice_id": 101,
"url_params": {
"datasource_id": "2",
"datasource_type": "table",
"save_action": "saveas",
"slice_id": "101",
},
"query_mode": "aggregate",
"groupby": ["gender"],
"time_grain_sqla": "P1D",
"temporal_columns_lookup": {"ds": True},
"metrics": ["count"],
"all_columns": [],
"percent_metrics": [],
"adhoc_filters": [
{
"clause": "WHERE",
"comparator": "No filter",
"expressionType": "SIMPLE",
"operator": "TEMPORAL_RANGE",
"subject": "ds",
}
],
"timeseries_limit_metric": {
"aggregate": None,
"column": None,
"datasourceWarning": False,
"expressionType": "SQL",
"hasCustomLabel": False,
"label": "random()",
"optionName": "metric_3kwbghgzkv9_wz84h9j1p5d",
"sqlExpression": "random()",
},
"order_by_cols": [],
"row_limit": 1000,
"server_page_length": 10,
"order_desc": True,
"table_timestamp_format": "smart_date",
"allow_render_html": True,
"show_cell_bars": True,
"color_pn": True,
"comparison_color_scheme": "Green",
"comparison_type": "values",
"extra_form_data": {},
"force": False,
"result_format": "json",
"result_type": "full",
}
query_context.slice_.id = 101
query_context.slice_.params_dict = {
"datasource": "2__table",
"viz_type": "table",
"query_mode": "aggregate",
"groupby": ["gender"],
"time_grain_sqla": "P1D",
"temporal_columns_lookup": {"ds": True},
"metrics": ["count"],
"all_columns": [],
"percent_metrics": [],
"adhoc_filters": [
{
"clause": "WHERE",
"subject": "ds",
"operator": "TEMPORAL_RANGE",
"comparator": "No filter",
"expressionType": "SIMPLE",
}
],
"order_by_cols": [],
"row_limit": 1000,
"server_page_length": 10,
"order_desc": True,
"table_timestamp_format": "smart_date",
"allow_render_html": True,
"show_cell_bars": True,
"color_pn": True,
"comparison_color_scheme": "Green",
"comparison_type": "values",
"extra_form_data": {},
"dashboards": [],
}
query_context.slice_.query_context = json.dumps(
{
"datasource": {"id": 2, "type": "table"},
"force": False,
"queries": [
{
"filters": [
{"col": "ds", "op": "TEMPORAL_RANGE", "val": "No filter"}
],
"extras": {"having": "", "where": ""},
"applied_time_extras": {},
"columns": ["gender"],
"metrics": ["count"],
"orderby": [],
"annotation_layers": [],
"row_limit": 1000,
"series_limit": 0,
"order_desc": True,
"url_params": {},
"custom_params": {},
"custom_form_data": {},
"post_processing": [],
"time_offsets": [],
}
],
"form_data": {
"datasource": "2__table",
"viz_type": "table",
"query_mode": "aggregate",
"groupby": ["gender"],
"time_grain_sqla": "P1D",
"temporal_columns_lookup": {"ds": True},
"metrics": ["count"],
"all_columns": [],
"percent_metrics": [],
"adhoc_filters": [
{
"clause": "WHERE",
"subject": "ds",
"operator": "TEMPORAL_RANGE",
"comparator": "No filter",
"expressionType": "SIMPLE",
}
],
"order_by_cols": [],
"row_limit": 1000,
"server_page_length": 10,
"order_desc": True,
"table_timestamp_format": "smart_date",
"allow_render_html": True,
"show_cell_bars": True,
"color_pn": True,
"comparison_color_scheme": "Green",
"comparison_type": "values",
"extra_form_data": {},
"dashboards": [],
"force": False,
"result_format": "json",
"result_type": "full",
},
"result_format": "json",
"result_type": "full",
}
)
assert query_context_modified(query_context)


def test_get_catalog_perm() -> None:
"""
Test the `get_catalog_perm` method.
Expand Down

0 comments on commit 7f2e752

Please sign in to comment.