gabotorresruiz commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768911493
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1293,13 +1293,13 @@ def __init__(self, form_data: Dict[str, Any]):
logger.info("Generating preview for chart %s", getattr(chart, "id",
"NO_ID"))
logger.info("Chart datasource_id: %s", getattr(chart, "datasource_id",
"NONE"))
- # Skip the dataset pre-check for transient charts (no ID) and for
guests
- # (authorized via the dashboard context, not dataset RBAC).
+ # Skip the pre-check only for transient charts (no ID). Guests keep the
+ # existence check but skip the RBAC access check
(dashboard-authorized).
from superset.mcp_service import guest_scope
- if getattr(chart, "id", None) is not None and not
guest_scope.is_guest_read():
+ if getattr(chart, "id", None) is not None:
validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ chart.datasource_id, check_access=not
guest_scope.is_guest_read()
)
Review Comment:
Correct, transient charts use id `0`, not `None`, so the pre-check was
running for them. Switched the guard to a truthy check so a transient chart
skips it as the comment intends.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -433,30 +440,30 @@ async def get_chart_data( # noqa: C901
)
logger.info("Getting data for chart %s: %s", chart.id,
chart.slice_name)
- # Skip the dataset RBAC pre-check for guests (see
guest_scope.is_guest_read).
- if not guest_scope.is_guest_read():
- validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ # Validate the dataset for everyone. Guests skip the RBAC access check
+ # (governed by authorize_query below) but keep the existence check, so
a
+ # deleted dataset still returns the clean DatasetNotAccessible
contract.
+ validation_result = validate_chart_dataset(
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
+ )
Review Comment:
Traced this end to end and it doesn't become a foreign read:
`authorize_query` pins `slice_id` to the resolved chart,
`query_context_modified` rejects a slice mismatch, and the guest datasource
grant requires `slc.datasource == datasource`, so a swapped datasource is
denied by `raise_for_access` before `run()`. That said, a guest has no business
using the unsaved-state cache, so I gated `form_data_key` off for guests as
defense in depth. Added a test that a guest with an identifier plus a foreign
`form_data_key` never reads the cache.
##########
superset/utils/filters.py:
##########
@@ -78,7 +78,12 @@ def guest_embedded_dashboard_filter() ->
Optional[ColumnElement[bool]]:
# Route each id kind to its own column and OR them — a plain int sent to
the
# uuid-typed column would raise a bind/type error.
uuid_ids = [id_ for id_ in ids if is_uuid(id_)]
- int_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ # A non-uuid id is a numeric dashboard id or a slug; route slugs to the
slug
+ # column instead of the int id column (which would raise a cast error). The
+ # data path (has_guest_access) stays the authorization gate.
+ non_uuid_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ int_ids = [id_ for id_ in non_uuid_ids if str(id_).isdigit()]
+ slug_ids = [id_ for id_ in non_uuid_ids if not str(id_).isdigit()]
Review Comment:
This one dissolves with the slug removal above. `has_guest_access` compares
resources by string equality against `dashboard.id`, so `+123` or ` 123` never
match a real id there either. `isdigit()` captures exactly the canonical
decimal strings that can string-equal an id, so it mirrors the auth gate. Using
`is_int` here would route `+123` into `Dashboard.id.in_(...)`, which Postgres
coerces and matches, re-creating the list-yes/data-no mismatch in the other
direction. So I kept `isdigit()` on purpose.
--
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]
