sha174n commented on PR #45622: URL: https://github.com/apache/superset/pull/45622#issuecomment-6352449987
Thanks Mike, this was a great review — all three addressed in the latest push. - **500 → 403**: the check now runs in each command's `validate()` and raises `DashboardForbiddenError` / `ChartForbiddenError` (which `put`/`post` catch → 403). `set_dash_metadata` no longer raises; it's out of the DAO write path entirely. - **Effective datasource on repoint**: the chart path now checks the datasource the request leaves on the chart — the incoming one when the same request repoints it, else the stored one. Added a unit test for the repoint-plus-link case. - **Ordering**: because the check is in `validate()` (first thing in `run()`), it fires before `process_tab_diff`/`process_native_filter_diff`, so a refused save no longer deactivates reports or sends their emails first. Smaller things: the rule + the admin / no-request-user / unresolvable-datasource exemptions now live once in `security_manager.raise_for_datasources_access`; the `None`-skip is documented there (the read paths re-check at query time); deduped the layout parsing into `_slice_ids_from_positions`; added type annotations. Tests: retargeted the unit tests to the command/helper layer (incl. one asserting `set_dash_metadata` no longer enforces), and added integration tests for dashboard PUT and POST asserting 403 + unchanged membership for an editor without the dataset grant, plus a positive case. On #45538 — happy to land this first and have it rebase onto the shared helper, or the reverse if you'd rather; whichever you prefer. -- 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]
