This is an automated email from the ASF dual-hosted git repository. beto pushed a commit to branch sc-79188 in repository https://gitbox.apache.org/repos/asf/superset.git
commit f74908e8aeb9a707f6c531ba526aa990ce3d40cb Author: Beto Dealmeida <[email protected]> AuthorDate: Tue Mar 12 12:38:40 2024 -0400 fix: check if guest user modified query --- superset/security/manager.py | 52 +++++++++++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 83e12fb2dc..66bf4c623a 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -67,6 +67,7 @@ from superset.security.guest_token import ( GuestTokenUser, GuestUser, ) +from superset.superset_typing import AdhocMetric from superset.utils.core import ( DatasourceName, DatasourceType, @@ -143,6 +144,43 @@ RoleModelView.edit_columns = ["name", "permissions", "user"] RoleModelView.related_views = [] +def query_context_modified(query_context: "QueryContext") -> bool: + """ + Check if a query context has been modified. + + This is used to ensure guest users don't modify the payload and fetch data + different from what was shared with them in dashboards. + """ + form_data = query_context.form_data + stored_chart = query_context.slice_ + + # sanity checks + if form_data is None or stored_chart is None: + return True + + # cannot request a different chart + if form_data.get("slice_id") != stored_chart.id: + return True + + # compare form_data + requested_metrics = set(form_data.get("metrics") or []) + stored_metrics = set(stored_chart.params_dict.get("metrics") or []) + if requested_metrics != stored_metrics: + return True + + # compare queries in query_context + queries_metrics: set[Union[AdhocMetric, str]] = set() + for query in query_context.queries: + queries_metrics.update(query.metrics or []) + + 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(query.get("metrics") or []) # type: ignore + + return queries_metrics != stored_metrics + + class SupersetSecurityManager( # pylint: disable=too-many-public-methods SecurityManager ): @@ -1944,19 +1982,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods if self.is_guest_user() and query_context: # Guest users MUST not modify the payload so it's requesting a different # chart or different ad-hoc metrics from what's saved. - form_data = query_context.form_data - stored_chart = query_context.slice_ - - if ( - form_data is None - or stored_chart is None - or form_data.get("slice_id") != stored_chart.id - or form_data.get("metrics", []) != stored_chart.params_dict["metrics"] - or any( - query.metrics != stored_chart.params_dict["metrics"] - for query in query_context.queries - ) - ): + if query_context_modified(query_context): raise SupersetSecurityException( SupersetError( error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
