EnxDev commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4276225029
##########
superset/security/manager.py:
##########
@@ -5363,10 +5366,24 @@ def raise_for_access( # noqa: C901
catalog=effective_catalog,
)
for datasource_ in datasources:
+ # ``datasource_access`` on a registered dataset is always
+ # sufficient. Editorship of the dataset is accepted only on
+ # the non-strict path (chart-data, dataset CRUD,
+ # ``/table_metadata/``, ``/select_star/``), where it is the
+ # dataset-owner "explore your own dataset" grant from
+ # #20382. Under strict scoping (SQL Lab raw queries,
MetaDB,
+ # MCP ``execute_sql``) editorship is deliberately excluded:
+ # returning raw rows from the underlying warehouse table
+ # must follow the user's live data-access grants, not
+ # editorship of a same-named dataset. Any principal able to
+ # create a dataset can make themselves an editor of one
+ # named after an arbitrary table, so accepting editorship
+ # here would let dataset creation stand in for data access
+ # and would outlive revocation of every real grant.
if self.can_access(
"datasource_access",
datasource_.perm or "",
- ) or self.is_editor(datasource_):
+ ) or (not force_dataset_match and
self.is_editor(datasource_)):
Review Comment:
Adding to the update.py point already raised here:
`CreateDatasetCommand.validate` (`commands/dataset/create.py:223`) also calls
`raise_for_access(database=..., sql=...)` without the flag. So a role that can
write datasets but has no `datasource_access` on the table (a custom Gamma +
dataset write role, say) creates a `SELECT 1` decoy named
`public.customer_pii`, then a second virtual dataset with `SELECT * FROM
public.customer_pii`, and chart-data on that one passes via `is_editor`.
The comment above says editorship here would let dataset creation stand in
for data access, and that's still true one hop away. Could the SQL-bearing
dataset create/update checks pass `force_dataset_match=True`, or if that's out
of scope, can the PR description say the decoy path stays open there?
##########
superset/security/manager.py:
##########
@@ -5363,10 +5366,24 @@ def raise_for_access( # noqa: C901
catalog=effective_catalog,
)
for datasource_ in datasources:
+ # ``datasource_access`` on a registered dataset is always
+ # sufficient. Editorship of the dataset is accepted only on
+ # the non-strict path (chart-data, dataset CRUD,
+ # ``/table_metadata/``, ``/select_star/``), where it is the
+ # dataset-owner "explore your own dataset" grant from
+ # #20382. Under strict scoping (SQL Lab raw queries,
MetaDB,
+ # MCP ``execute_sql``) editorship is deliberately excluded:
Review Comment:
Nit, take it or leave it. This list drops alert/report queries, which the
docstring above now includes after 23fd487.
Worth keeping the two in sync so the next reader doesn't wonder whether
alerts are strict.
--
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]
