gabotorresruiz commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4020486844
##########
superset/models/slice.py:
##########
@@ -168,6 +169,15 @@ class Slice( # pylint: disable=too-many-public-methods
remote_side="SqlaTable.id",
lazy="subquery",
)
+ semantic_view = relationship(
Review Comment:
Confirmed empirically: on this branch
`tests/unit_tests/commands/deletion_retention/` has 14 failures, all
`RuntimeError: Incomplete purge policy for Slice:
missing=[relationship:slices->semantic_views:->:manytoone:semantic_view]`, and
the same suite passes on the parent commit. Since `get_purge_policy(Slice)`
raises, chart purging itself breaks, not just the validation tests.
##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -703,6 +704,109 @@ def
test_create_chart_from_saved_query_rejected_cleanly(self):
db.session.delete(db.session.query(SavedQuery).get(saved_query_id))
db.session.commit()
+ def test_create_chart_from_semantic_view(self):
+ """
+ Chart API: creating a chart with datasource_type="semantic_view" must
+ succeed (apache/superset#44167). Semantic views are first-class
+ resolvable datasources (Slice resolves them through the type-guarded
+ ``semantic_view`` relationship), so the non-table datasource_type
+ guard must explicitly allow them rather than rejecting them the way
+ it rejects saved_query. This reproduces the exact API call shape from
+ the bug report: a real semantic view row, then POST /api/v1/chart/
+ with datasource_type="semantic_view".
+ """
+ self.login(ADMIN_USERNAME)
+ suffix = uuid.uuid4().hex
+ layer = SemanticLayer(
+ uuid=uuid.uuid4(),
+ name=f"issue-44167-layer-{suffix}",
+ type="test",
+ configuration="{}",
+ )
+ view = SemanticView(
+ uuid=uuid.uuid4(),
+ name=f"issue-44167-view-{suffix}",
+ semantic_layer_uuid=layer.uuid,
+ configuration="{}",
+ )
+ db.session.add_all([layer, view])
+ db.session.commit()
+ view_id = view.id
+
+ chart_data = {
+ "slice_name": "issue-44167-repro-chart",
+ "datasource_id": view_id,
+ "datasource_type": "semantic_view",
+ "viz_type": "table",
+ }
+ chart_id = None
+ try:
+ rv = self.post_assert_metric("/api/v1/chart/", chart_data, "post")
+
+ assert rv.status_code == 201
+ data = json.loads(rv.data.decode("utf-8"))
+ chart_id = data.get("id")
+ model = db.session.query(Slice).get(chart_id)
+ assert model.datasource_type == "semantic_view"
+ assert model.datasource_id == view_id
+
+ # The saved chart is now resolvable: its owner (admin) can
+ # retrieve it, and the chart's perm carries the view perm.
+ rv = self.get_assert_metric(f"/api/v1/chart/{chart_id}", "get")
+ assert rv.status_code == 200
+ assert model.perm == view.perm
+
+ gamma = self.get_user("gamma")
+ uri = "api/v1/chart/?q=" + rison.dumps(
+ {
+ "filters": [
+ {
+ "col": "slice_name",
+ "opr": "ct",
+ "value": "issue-44167-repro-chart",
+ }
+ ]
+ }
+ )
+
+ # Without the view's datasource_access perm, a non-owner cannot
+ # list/open the chart.
+ with self.temporary_user(gamma, login=True):
Review Comment:
This test cannot pass as written. I ran it in a real environment (sqlite
metadata DB plus Redis) and it fails here with `1 == 0`, for two independent
reasons:
- `self.login(ADMIN_USERNAME)` above leaves an authenticated session, and
FAB's `AuthDBView.login` redirects an already authenticated session without
re-authenticating (the `g.user.is_authenticated` early return in
`flask_appbuilder/security/views.py`). So every `temporary_user(...,
login=True)` block here still runs as admin, and admin sees the chart. Adding
`self.logout()` before the first block fixes this; I verified it on this branch.
- The `all_database_access` block then still fails, because
`ChartFilter.apply` returns the unfiltered query when
`security_manager.can_access_all_datasources()` is true, and that helper
includes `all_database_access`. The short circuit happens before any join, so
this assertion does not exercise the id ride-along protection the comment
describes, and the expectation contradicts `SemanticView.raise_for_access`,
which also grants on `can_access_all_datasources()`. To pin the ride-along
protection, grant access to one specific database and create a table whose id
collides with the view id, then assert the chart stays hidden.
With `self.logout()` added and that block corrected, the rest passes for me
on this branch: 201 on create, perm synced, hidden without the view perm,
visible with it.
##########
superset/charts/filters.py:
##########
@@ -151,10 +151,22 @@ def _apply_viewers(self, query: Query) -> Query:
# (C) No-viewer fallback: charts with no viewers → dataset-based access
chart_has_viewers = Slice.viewers.any()
+
+ # TABLE charts keep the table/database join for dataset-based access.
+ # The datasource_type guard keeps the join unambiguous: a semantic-view
+ # chart's datasource_id (shared auto-increment id space) must never be
+ # matched against a coincidental SqlaTable.id. TABLE rows are otherwise
+ # matched exactly as before.
table_alias = aliased(SqlaTable)
- no_viewer_query = (
+ no_viewer_table_query = (
db.session.query(Slice.id)
- .join(table_alias, Slice.datasource_id == table_alias.id)
+ .join(
+ table_alias,
+ and_(
+ Slice.datasource_id == table_alias.id,
+ Slice.datasource_type == DatasourceType.TABLE,
Review Comment:
The guard here is right, but its sibling is still unguarded:
`DashboardAccessFilter._apply_viewers` in `superset/dashboards/filters.py` has
the identical join, `.join(SqlaTable, Slice.datasource_id == SqlaTable.id)`,
with no `datasource_type` condition. Once semantic view charts land on
dashboards, a published no-viewer dashboard whose semantic view chart collides
with an accessible `SqlaTable.id` becomes listable through that join, and
conversely a dashboard containing only semantic view charts can never match the
fallback even for users who hold the view perm, because the joined rows do not
exist and `get_dataset_access_filters(Slice)` never gets to check `Slice.perm`.
I would apply the same `datasource_type == DatasourceType.TABLE` guard there
and add a semantic view arm mirroring this one, in this PR, since this PR is
what makes such charts reachable.
##########
superset/charts/filters.py:
##########
@@ -163,7 +175,21 @@ def _apply_viewers(self, query: Query) -> Query:
)
)
)
- filters.append(Slice.id.in_(no_viewer_query))
+ filters.append(Slice.id.in_(no_viewer_table_query))
+
+ # SEMANTIC_VIEW charts have no SqlaTable/Database row to join; access
is
+ # evaluated against the chart's own perm, which set_related_perm keeps
in
+ # sync with the view's ``datasource_access`` perm (no numeric-id join).
+ no_viewer_semantic_view_query = db.session.query(Slice.id).filter(
+ and_(
+ Slice.datasource_type == DatasourceType.SEMANTIC_VIEW,
+ ~chart_has_viewers,
+ Slice.perm.in_(
Review Comment:
Not a blocker, a consistency question: `SemanticView.raise_for_access` also
grants when the user holds `datasource_access` on the parent layer's perm, but
both this branch and the chart access path (`can_access_datasource`, which
checks only `datasource.perm`) look at the view perm alone. A user granted at
the layer level can explore the view but cannot open or list a chart saved on
it. Everything fails closed, so no security concern, but is the asymmetry
intended?
##########
superset/models/slice.py:
##########
@@ -190,8 +200,10 @@ def __repr__(self) -> str:
return self.slice_name or str(self.id)
@property
- def datasource(self) -> SqlaTable | None:
- return self.table
+ def datasource(self) -> SqlaTable | SemanticView | None:
+ if table := self.table:
+ return table
+ return self.semantic_view
Review Comment:
While tracing this I found two more bare id comparisons the type pinning
should cover in the same sweep:
`_guest_token_allows_dataset(chart.datasource.id ...)` in the chart branch of
`raise_for_access`, where a guest token's dataset allowlist of table ids is
compared against a semantic view id, and the native filter arm of the
datasource branch, which compares `target.get("datasetId")` from dashboard
metadata against `datasource.data["id"]`, where `SemanticView.data["id"]` is
the bare integer id.
--
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]
