geido commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4287259769
##########
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:
Confirmed both on vanilla OSS — you're right, and my earlier "the decoy
can't reach the real table" was wrong. The two-hop (a no-table decoy to pass
the create/update SQL check, then chart-data on the owned `SELECT * FROM
customer_pii` dataset) does return the real rows, and so does the
persisted-query → query-backed chart-data path.
Closed in 4abb864: added an `allow_editorship=False` mode to
`raise_for_access` that stops honoring dataset editorship while keeping the
catalog/schema fallthroughs — so `schema_access` can still *define* a dataset
(unlike `force_dataset_match`, which drops them and would break creating the
first dataset over a schema). It's now passed from every "define/repoint a
dataset over a table" check — create, update, v1 import, and the legacy
`/datasource/save/` repoint — and from query-backed chart-data (your path #1).
An adversarial pass over that first cut found two more of the same shape I'd
missed — dataset **import** (`importers/v1/utils.py`) and the legacy
**`/datasource/save/`** repoint — now closed too. Evidence (vanilla OSS): the
decoy create/update/import/repoint and query-backed chart-data paths all deny;
`schema_access` and `all_datasource_access` holders still define datasets; the
#20382 owner-explore grant is unchanged.
##########
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:
Right — closed in 4abb864. `CreateDatasetCommand.validate` (and update, v1
import, and the legacy `/datasource/save/` repoint) now pass
`allow_editorship=False`, so the SQL/table check requires real data access to
the referenced table, not editorship of a same-named decoy. That covers your
create.py:223 point and Joe's update/query paths in one mechanism.
##########
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:
Synced in 4abb864 — the per-table comment now groups the exclusions by
mechanism: `force_dataset_match` (SQL Lab / MetaDB / MCP / alert-report) and
`allow_editorship=False` (dataset create/update/import/repoint and query-backed
chart-data), so alerts aren't ambiguous.
--
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]
