geido commented on code in PR #43458:
URL: https://github.com/apache/superset/pull/43458#discussion_r3934324665
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -258,7 +257,6 @@ export default function ExplorePage() {
}
const slice = {
...data,
Review Comment:
Confirmed the whole chain, and it turned out to be broader than
`datasource_name_text`. Fixed on the frontend in 69782fde99.
`ChartFilter` branches (A) editor and (B) viewer
(`superset/charts/filters.py:134-150`) admit an explicit chart viewer with no
dataset-access clause — only the (C) no-viewer fallback consults
`get_dataset_access_filters`. So a Gamma user who is a viewer of a chart on a
dataset they cannot access passes `ChartDAO.get_by_id_or_uuid`, and
`ChartGetResponseSchema` dumps the dataset identity into the body, which
`...data` then spread into `state.explore.slice`.
While auditing the schema I found `params` and `query_context` are the worse
half: they serialise the denied dataset's column names, adhoc-metric
`sqlExpression` and the literal comparator values in `adhoc_filters`. Both are
now dropped along with
`datasource_name_text`/`datasource_url`/`datasource_uuid`. Nothing in Explore
reads them on this path — `sliceFormData` derives from controls in
`hydrateExplore.ts:226-228`, and the sole `slice.query_context` reader
(`ExploreChartPanel/index.tsx:234`) is gated on a `slice.form_data` this code
already strips. The request only exists to show the chart name and owners on
the denial screen.
I also found a second leak on the neighbouring rolling-deploy branch: the
whole `err` was stored under `dataset.extra.error`, so a legacy payload's
`extra.datasource_name` landed in Redux where `DatasourceControl` renders
`extra.error`. That key is now deleted before the error is stored.
To be explicit about what this does **not** fix: `GET /api/v1/chart/<id>`
still returns all of it, so the same principal can read the dataset name by
calling the endpoint directly. That is pre-existing and independent of this PR,
and closing it means changing `ChartGetResponseSchema` for every consumer —
`ChartCard`, `SliceAdder` and `ChartList` all render `datasource_name_text` off
the list endpoint — plus per-row dataset checks. Happy to open a follow-up
issue if you agree that is the right scope.
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -228,13 +228,11 @@ export default function ExplorePage() {
t('Failed to load chart data.');
dispatch(addDangerToast(errorMesage));
- if (err.extra?.datasource) {
+ if (err.extra?.is_access_denial) {
Review Comment:
Yes — added in 69782fde99.
Both the Explore page guard and `DatasourceSecurityAccessErrorMessage` now
accept `extra.datasource` as a legacy discriminator alongside
`is_access_denial`:
```ts
if (err.extra?.is_access_denial || isDefined(err.extra?.datasource)) {
```
An older pod's payload therefore keeps the request-access UI. This is safe
against false positives: `extra.datasource` is written in exactly one place
(`get_datasource_access_error_object`), and the non-access errors that reuse
`DATASOURCE_SECURITY_ACCESS_ERROR` — the virtual-dataset SQL validation in
`connectors/sqla/utils.py:154-168` and the semantic-layer denial in
`semantic_layers/models.py:163-170` — carry no `extra` at all, so they still
render plainly.
On not rendering the name: the legacy payload also carries
`extra.datasource_name`, and storing the raw error under `dataset.extra.error`
put it in Redux where `DatasourceControl` renders that object. That key is now
stripped before the error is stored, so the compatibility path shows the
request-access UI without the dataset name. My first test for this asserted
`not.toHaveTextContent('"name":"Quarterly Sales"')`, which passed for the wrong
reason — the serialised key is `"datasource_name"`, so the substring never
matched. Tightened to assert on the bare value, which failed until the strip
was added.
Covered by `keeps the request-access path for an API that predates
is_access_denial` (Chart.test.tsx) and `treats a pre-is_access_denial payload
as an access denial` (DatasourceSecurityAccessErrorMessage.test.tsx).
--
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]
