aminghadersohi commented on PR #45816:
URL: https://github.com/apache/superset/pull/45816#issuecomment-6472461718
Product acceptance at 6949fcc5d02b24dd9ef3a4e76da3a5dd123293b0, run against
a real Superset (Postgres metadata DB, Redis cache, `EMBEDDED_SUPERSET` on),
with the merge base running side by side on the same metadata DB. Embedded
dashboards were loaded through the Embedded SDK with real guest tokens.
**Verdict: ship.**
- **Leak reproduced at the merge base, closed here.** A guest token for one
embedded dashboard posting `/api/v1/chart/data` with `{type: "query", id:
<another user's ad hoc SQL Lab query>}`: merge base 200 with rows, plus the
query's SQL text in the `query` field; this branch 403 `{"message":
"Forbidden"}`. The same holds with `result_type=query`, with a
`slice_id`/`dashboardId` from the dashboard, and with a token that carries a
`datasets` claim. Explore GET and `/superset/fetch_datasource_metadata` also
returned the SQL text to the guest at the merge base; both are 403 here.
- **Non-guest users unchanged.** Admin, a SQL Lab user, a restricted viewer
with table access and a Gamma-only user got identical results on both builds
(chart data on another user's query and on their own, Explore GET).
- **Embedded dashboards still work.** Both charts render with values equal
to the SQL totals, one chart-data call per chart. Guest drill to detail (200
rows) and drill by on dataset charts are unchanged.
Observations, none blocking:
1. A real database cannot hold a chart whose `datasource_type` is `query`.
Migration `7e67aecbf3f1` rewrote such charts to `table` and added the
`ck_chart_datasource` CHECK constraint, and chart create/update reject `query`
as well. So `_guest_dashboard_authorizes_query` never returns True on a
migrated DB, and in practice guests are refused every `query` datasource. That
is the safe outcome. It does mean the helper, the `query_context=` pass-through
and the positive-control unit tests cover a state the schema forbids, and the
summary line about embedded drill to detail on query-backed charts describes a
case that can't occur. A plain "a guest never reads a `query` datasource" check
would be simpler. I checked the helper's allow path with the constraint
temporarily dropped: the chart renders for the guest on both builds.
2. Outside this diff, already present at the merge base: `POST
/api/v1/explore/form_data` with a denied `query` datasource returns 500 "Fatal
error" instead of 403 (`check_access` in `commands/explore/form_data/utils.py`
does not map `SupersetSecurityException`). A Gamma-only user hits it on master.
This branch adds guests to that path; no data is returned.
3. Outside this diff, already present at the merge base: the guest branch of
`/datasource/samples` calls `self.response_404()`, which the `Datasource` view
does not have, so a non-dataset type returns 500.
--
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]
