From 7f2e75279694835a32bb1f33cd33b8c4693670da Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 26 Nov 2024 10:15:06 -0500 Subject: [PATCH] fix: check orderby (#31156) --- superset/security/manager.py | 2 +- tests/unit_tests/security/manager_test.py | 233 ++++++++++++++++++++-- 2 files changed, 212 insertions(+), 23 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index c0773f9f7633f..f2dd12e13636b 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -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 = { @@ -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 diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index a6022525ace0c..40d89ba356f8c 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -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", @@ -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.