mikebridge commented on PR #45067:
URL: https://github.com/apache/superset/pull/45067#issuecomment-6070342074
@fitzee thanks. Both items are fixed on the branch (head `b5075dd79e`).
**1. Guest semantic reads (`9cfc92340b`).** This mirrors the table branch.
For `guest_scope.is_guest_read()`, `validate_chart_semantic_view` only checks
that the view exists. The gate is then `guest_scope.authorize_query` plus
`ChartDataCommand.validate()` →
`security_manager.raise_for_access(query_context=…)`. So the dashboard grant
(`_datasource_matches`) and the guest-RLS refusal
(`raise_for_unsupported_guest_rls`) apply exactly as on `/api/v1/chart/data`.
The guest refusal was not intentional.
`test_guest_semantic_chart_data_uses_dashboard_authorization` covers three
cases: allowed, RLS refused (raised from `command.validate()`), and missing. It
also asserts that the view's direct-grant check is not called and that
`authorize_query` attaches `{"dashboardId": 5, "slice_id": 9}`.
**2. `check_chart_data_access` (`87b7eb0abc`).** The helper now checks the
type: semantic charts go through `validate_chart_semantic_view` (view lookup
plus `SemanticView.raise_for_access()`), and table charts keep
`validate_chart_dataset`. That fixes `add_chart_to_existing_dashboard` and
`generate_dashboard` in one place. Neither tool uses the datasource after the
check.
`update_chart` needed one more step. With only the type check, an authorized
semantic chart would continue into an update path that only handles tables:
`DatasetDAO.find_by_id(chart.datasource_id)`, a `TABLE` preview, and `…__table`
form data. That would silently rebind the chart to a table with the same id. So
`update_chart` now refuses semantic charts with `UnsupportedDatasourceType`.
The refusal comes *after* the access check, so a caller without access still
gets the uniform `DatasetNotAccessible`. #44393 replaces the refusal with real
support.
Red-first: `test_chart_data_access_check_uses_the_source_type` (allowed /
missing / denied with a table at the same id 17, plus a SQL control) and
`test_update_chart_refuses_saved_semantic_charts` failed on the previous head
with the table lookup you described (5 failed). They pass now.
**Nits.** I rewrapped the docs paragraph. It now lists the three callers and
the `update_chart` refusal. I left these for whichever of #45067 / #44393 lands
second, to keep this PR small:
- catching `SQLAlchemyError` in the semantic helper
- query/saved-query typing outside `update_chart`
- deduplicating the helper with `resolve_semantic_view`
I agree with the preview scope note: `get_chart_preview` stays type-blind
until #44393.
--
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]
