gabotorresruiz commented on code in PR #43769: URL: https://github.com/apache/superset/pull/43769#discussion_r3961014423
########## superset/models/embedded_chart.py: ########## @@ -0,0 +1,64 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import uuid + +from flask_appbuilder import Model +from sqlalchemy import Column, ForeignKey, Integer, Text +from sqlalchemy.orm import relationship +from sqlalchemy_utils import UUIDType + +from superset.models.helpers import AuditMixinNullable + + +class EmbeddedChart(Model, AuditMixinNullable): + """ + A configuration of embedding for a chart. + + References the chart, and contains a config for embedding that chart. + Mirrors ``EmbeddedDashboard`` so both embeddable resource types share the + same guest-token and allowed-domain semantics. + + This data model allows multiple configurations for a given chart, + but at this time the API only allows setting one. + """ + + __tablename__ = "embedded_charts" + + uuid = Column(UUIDType(binary=True), default=uuid.uuid4, primary_key=True) + allow_domain_list = Column(Text) # reference the `allowed_domains` property instead + # Epoch seconds; guest tokens whose `iat` predates this are rejected. Set to + # "now" to revoke all currently-issued guest tokens for this embedded + # chart. NULL = no revocation. + guest_token_revoked_before = Column(Integer, nullable=True) Review Comment: This column promises revocation semantics that never execute for charts: `_is_guest_token_revoked_by_embedded` skips every non-DASHBOARD resource (`superset/security/manager.py:5339`) and `revoke_guest_token_access` only resolves `EmbeddedDashboardDAO`. I verified it live on this branch: with `guest_token_revoked_before` set in the future on both tables, a dashboard guest token gets a 401 while a chart guest token keeps returning data. Deleting the embed config does cut access, but it destroys the uuid, which is exactly what revocation exists to avoid. I would either add the CHART branch to both spots with a test that sets the cutoff and asserts the 401, or drop the column until charts support it. ########## superset/charts/schemas.py: ########## @@ -1998,3 +1998,15 @@ class ChartGetResponseSchema(Schema): ChartCacheScreenshotResponseSchema, GetFavStarIdsSchema, ) + + +class EmbeddedChartConfigSchema(Schema): + allowed_domains = fields.List(fields.String(), required=True) + + +class EmbeddedChartResponseSchema(Schema): + uuid = fields.String() + allowed_domains = fields.List(fields.String()) + chart_id = fields.String() Review Comment: Confirming the bots' finding empirically: I created an embed config on a running instance and the response carries no chart identifier at all, because the model exposes `slice_id` and this field never dumps. While fixing it, consider mirroring `EmbeddedDashboardResponseSchema` exactly: the dashboard twin's nested user schema exposes `username`, while this one exposes the `changed_by` user's `email`. Keeping the two response shapes identical fixes both in one move. ########## superset-frontend/src/embedded/embeddedChart/useExploreData.ts: ########## @@ -0,0 +1,89 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useEffect, useState } from 'react'; +import { SupersetClient } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import { EmbeddedChartData } from './hydrateEmbedded'; + +interface State { + data: EmbeddedChartData | null; + loading: boolean; + error: string | null; +} + +/** + * Fetches the one chart this iframe renders, in the shape `hydrateEmbedded` + * expects. Uses the explore endpoint because it returns the slice and its + * dataset together, which is exactly the pair the fabricated dashboard state + * needs and avoids a second round trip for the datasource. + */ +export default function useExploreData(chartId: string | number): State { + const [state, setState] = useState<State>({ + data: null, + loading: true, + error: null, + }); + + useEffect(() => { + let cancelled = false; + + SupersetClient.get({ + endpoint: `/api/v1/explore/?slice_id=${chartId}`, Review Comment: This is the load-bearing line for the guest flow: `/api/v1/explore/` sits under `can_read` on Explore, which no embedded guest role has by default. I verified on this branch that a chart guest token gets a 403 here with a guest role that is sufficient for embedded dashboards, so the page dead-ends in the error state. Either the required guest-role grants become a documented part of the feature, or this fetch moves under the Chart permission the guest already needs for `/api/v1/chart/data` (which also means teaching `ChartFilter`'s guest branch about chart tokens, since `GET /api/v1/chart/<pk>` 404s for them today). ########## superset-frontend/src/embedded/index.tsx: ########## @@ -47,6 +47,7 @@ import { getThemeController, } from './EmbeddedContextProviders'; import { embeddedApi } from './api'; +import EmbeddedChart from './embeddedChart'; Review Comment: Not a blocker, but worth doing before merge since CI already measured it: this static import pulls the dashboard chart stack into the initial embedded bundle for every consumer, including existing embedded dashboards that never render a chart embed (+17% on the embedded entrypoint per the benchmark alert). `LazyDashboardPage` at the top of this file is the pattern to copy: a `lazy(() => import('./embeddedChart'))` behind the existing Suspense keeps the chart stack out of the dashboard path. ########## superset/migrations/versions/2026-09-01_10-00_a1c7e4b62f18_add_embedded_charts_table.py: ########## @@ -0,0 +1,53 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""add embedded_charts table + +Revision ID: a1c7e4b62f18 +Revises: 39097d124752 Review Comment: Just a small NIT while you re-chain onto master's new head (`b3e9c1a75d24` landed after your last re-chain, which is the current merge conflict): the docstring still says `Revises: 39097d124752` while `down_revision` is `8f31c5d726ab`. For what it is worth, I ran the migration up, down, and up again on this branch and it was clean. -- 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]
