aminghadersohi commented on code in PR #41753:
URL: https://github.com/apache/superset/pull/41753#discussion_r3522122303
##########
superset/charts/filters.py:
##########
@@ -105,6 +108,10 @@ def apply(self, query: Query, value: Any) -> Query:
if security_manager.can_access_all_datasources():
return query
+ # Embedded guests: scope to charts on the dashboards in their token.
+ if (guest_dashboards := guest_embedded_dashboard_filter()) is not None:
Review Comment:
HIGH: this new guest branch has no direct test coverage.
`tests/unit_tests/mcp_service/test_guest_scope.py` and
`tests/unit_tests/utils/filters_test.py` only unit-test the leaf helpers
(`guest_dashboard_id`, `authorize_query`, `guest_embedded_dashboard_filter`) in
isolation with mocks — none exercises `ChartFilter.apply()` itself with a guest
user against a chart on an out-of-scope dashboard. Deleting these two lines
would not fail any test added or existing in this PR. Given this is the actual
SQL-level gate that scopes chart resolution for guests (the tool-level
`guest_scope` checks are defense-in-depth on top of it, not the primary
control), it should have a test that instantiates `ChartFilter` (or goes
through `ChartDAO.find_by_id`) with a mocked guest user and asserts a chart on
a dashboard *not* in the token's resources is excluded from the result set, and
one that's in scope is included.
##########
superset/mcp_service/guest_scope.py:
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Embedded-guest data-query authorization for MCP chart tools.
+
+Chart *resolution* is scoped for guests by the core ``ChartFilter`` (mirroring
+``DashboardAccessFilter``). This module handles the remaining MCP-layer
concern:
+attaching the embedded dashboard context to a chart's data query so the
existing
+guest ``raise_for_access`` branch authorizes it, exactly as the embedded
+dashboard frontend does.
+"""
+
+from __future__ import annotations
+
+import logging
+from typing import Any
+
+logger = logging.getLogger(__name__)
+
+
+def guest_dashboard_id(chart: Any) -> int | None:
Review Comment:
NIT: the "is this an embedded-guest read that should skip the dataset RBAC
pre-check" predicate is re-implemented 3 different ways at the call sites
(`guest_dashboard_id is None` in get_chart_data.py,
`security_manager.is_guest_user()` in get_chart_info.py,
`guest_scope.guest_dashboard_id(chart) is None` in get_chart_preview.py).
Consider a single `guest_scope.should_skip_dataset_check(chart) -> bool` (or
similar) so the three call sites can't drift out of sync as this evolves — e.g.
get_chart_info.py's version only checks `is_guest_user()` without also
confirming the chart is actually in the guest's dashboard scope, whereas the
other two derive it from `guest_dashboard_id`.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -508,11 +517,16 @@ async def get_chart_data( # noqa: C901
from superset.common.query_context_factory import
QueryContextFactory
factory = QueryContextFactory()
- row_limit = (
- request.limit
- or form_data.get("row_limit")
- or current_app.config["ROW_LIMIT"]
- )
+ # row_limit from chart.params may be a str; coerce for
+ # apply_max_row_limit's int comparison.
+ try:
+ row_limit = int(
Review Comment:
NIT: this `int()` coercion of `row_limit` looks like an unrelated bug fix
(str `row_limit` from `chart.params` breaking `apply_max_row_limit`'s int
comparison) bundled into a security-focused PR, with no dedicated test for the
coercion/except path. Not objecting to the fix itself, just flagging the scope
creep — a security-boundary PR is easier to review and to revert cleanly when
it stays narrowly scoped to the guest-authorization change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
