gabotorresruiz commented on code in PR #43994:
URL: https://github.com/apache/superset/pull/43994#discussion_r3961014854
##########
superset/security/manager.py:
##########
@@ -2245,8 +2246,15 @@ def can_access_schema(self, datasource: "BaseDatasource
| Explorable") -> bool:
if self.can_access_all_datasources():
return True
- # SQL-specific hierarchy checks
- if isinstance(datasource, BaseDatasource):
+ # SQL-specific hierarchy checks.
+ # BaseDatasource always has database, catalog, and schema_perm.
+ # Explorable implementations (e.g. Query) may also carry these
+ # attributes — the isinstance gate below was introduced in the 6.1.0
+ # Explorable refactor and accidentally excluded Query, which is not a
+ # BaseDatasource but does expose the same hierarchy.
+ if isinstance(datasource, BaseDatasource) or (
Review Comment:
Not a blocker, a scope observation worth a line in the PR description: this
widening is live beyond the Explore GET. `can_access_datasource(Query)` is
reachable through `Slice.resolved_datasource` in the dashboard and chart
datasource fallbacks, so query-backed charts and dashboards become visible to
holders of database, catalog, or schema grants on the query's default schema,
where previously only the author and Admin passed. That reads as consistent
with the intent (visibility follows the grant; data access still goes through
the per-table checks), but since `schema_perm` here is the query's default
schema rather than the schemas its SQL touches, I want to confirm we agree this
arm grants object visibility only. Or am I misunderstanding the fallback's
reach?
##########
superset/models/sql_lab.py:
##########
@@ -371,7 +371,14 @@ def dttm_cols(self) -> list[Any]:
@property
def schema_perm(self) -> str:
- return f"{self.database.database_name}.{self.schema}"
+ return (
+ security_manager.get_schema_perm(
Review Comment:
Just a small NIT: the per-table loop in `raise_for_access` qualifies with
`query.catalog or default_catalog`, while this property passes `self.catalog`
as is. On an engine whose schema perms are catalog-qualified, a query row with
`catalog=None` produces the 2-part form and the schema arm of
`can_access_schema` quietly misses (fail-closed, so no security impact; the
grant just does not take effect on that path). `self.catalog or
self.database.get_default_catalog()` would make the two paths agree, and
`Query.catalog` is a real column so the `getattr` is unnecessary.
##########
superset/security/manager.py:
##########
@@ -4739,6 +4747,8 @@ def raise_for_access( # noqa: C901
self.get_table_access_error_object(denied)
)
+ return
Review Comment:
This is the right fix, and I verified the blast radius: the only call site
passing both `query=` and `datasource=` is `_authorize_datasource` in
`superset/commands/explore/get.py`, and every `query=`-only caller previously
fell off the end of this method as a no-op, so the `return` changes behavior
for exactly the broken path.
Could you add a regression test so a future refactor cannot silently
reintroduce the fall-through? This one fails on the parent commit with the
exact #43987 error and passes on this branch (it fits in
`tests/unit_tests/subjects/test_raise_for_access.py`):
```python
def test_raise_for_access_query_schema_access_non_author(app_context):
"""A schema_access holder can explore another author's SQL Lab query.
Mirrors _authorize_datasource in superset/commands/explore/get.py,
which passes the Query under both ``query=`` and ``datasource=``.
"""
from superset.models.sql_lab import Query
from superset.sql.parse import Table
sm = _make_sm()
database = MagicMock()
database.database_name = "examples"
database.get_default_catalog.return_value = None
database.get_default_schema_for_query.return_value = "main"
query = Query(sql="SELECT * FROM t1", schema="main", catalog=None,
user_id=2)
object.__setattr__(query, "database", database)
query.status = "success"
query.id = 42
def schema_access_main_only(permission_name, view_name):
return permission_name == "schema_access" and view_name ==
"[examples].[main]"
parse_result = MagicMock()
parse_result.tables = {Table("t1", "main", None)}
with (
patch.object(sm, "can_access_all_datasources", return_value=False),
patch.object(sm, "can_access_all_databases", return_value=False),
patch.object(sm, "can_access", side_effect=schema_access_main_only),
patch.object(sm, "is_editor", return_value=False),
patch("superset.security.manager.get_user_id", return_value=999),
patch("superset.security.manager.process_jinja_sql",
return_value=parse_result),
):
sm.raise_for_access(
query=query,
datasource=query,
allow_query_authorship_bypass=True,
)
```
A sibling case asserting the same principal is still denied when the SQL
references a table in an ungranted schema would lock in the fail-closed side
too.
--
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]
