amaannawab923 commented on code in PR #43769:
URL: https://github.com/apache/superset/pull/43769#discussion_r3967295991
##########
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()
+ changed_on = fields.DateTime()
+ changed_by = fields.Nested(UserSchema)
Review Comment:
Fixed. `chart_id` is now `fields.String(attribute="slice_id")`, so it dumps
from the column the model actually has rather than resolving to nothing.
Verified against a running instance: the embedded-chart response now carries
`"chart_id": "103"`.
##########
superset/charts/api.py:
##########
@@ -1874,3 +1897,181 @@ def restore_version(self, uuid_str: str,
version_uuid_str: str) -> Response:
return restore_version_endpoint(
self, Slice, RestoreChartVersionCommand, uuid_str, version_uuid_str
)
+
+ @expose("/<pk>/embedded", methods=("GET",))
+ @protect()
+ @safe
+ @permission_name("read")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.get_embedded",
+ log_to_statsd=False,
+ )
+ def get_embedded(self, pk: int) -> Response:
+ """Get the chart's embedded configuration.
+ ---
+ get:
+ summary: Get the chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The chart id
+ responses:
+ 200:
+ description: Result contains the embedded chart config
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = ChartDAO.find_by_id(pk)
+ if not chart:
+ return self.response_404()
+ if not chart.embedded:
+ return self.response(404)
+ embedded: EmbeddedChart = chart.embedded[0]
+ result = self.embedded_response_schema.dump(embedded)
+ return self.response(200, result=result)
+
+ @expose("/<pk>/embedded", methods=("POST", "PUT"))
+ @protect()
+ @safe
+ @permission_name("set_embedded")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.set_embedded",
+ log_to_statsd=False,
+ )
+ def set_embedded(self, pk: int) -> Response:
+ """Set a chart's embedded configuration.
+ ---
+ post:
+ summary: Set a chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The chart id
+ requestBody:
+ description: The embedded configuration to set
+ required: true
+ content:
+ application/json:
+ schema: EmbeddedChartConfigSchema
+ responses:
+ 200:
+ description: Successfully set the configuration
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ put:
+ summary: Update a chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The chart id
+ requestBody:
+ description: The embedded configuration to set
+ required: true
+ content:
+ application/json:
+ schema: EmbeddedChartConfigSchema
+ responses:
+ 200:
+ description: Successfully set the configuration
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = ChartDAO.find_by_id(pk)
+ if not chart:
+ return self.response_404()
+ try:
+ body = self.embedded_config_schema.load(request.json)
+ embedded = EmbeddedChartDAO.upsert(chart, body["allowed_domains"])
+ db.session.commit() # pylint: disable=consider-using-transaction
+ result = self.embedded_response_schema.dump(embedded)
Review Comment:
Fixed in the schema rather than here — `chart_id` is now bound to the
model's `slice_id` attribute, so this endpoint's response carries the
identifier. Confirmed on a running instance.
##########
superset/daos/chart.py:
##########
@@ -166,3 +167,33 @@ def remove_favorite(chart: Slice) -> None:
)
if fav:
db.session.delete(fav)
+
+
+class EmbeddedChartDAO(BaseDAO[EmbeddedChart]):
+ # There isn't really a regular scenario where we would rather get Embedded
by id
+ id_column_name = "uuid"
+
+ @staticmethod
+ def upsert(chart: Slice, allowed_domains: list[str]) -> EmbeddedChart:
+ """
+ Sets up a chart to be embeddable.
+ Upsert is used to preserve the embedded_chart uuid across updates.
+ """
+ embedded: EmbeddedChart = (
+ chart.embedded[0] if chart.embedded else EmbeddedChart()
+ )
Review Comment:
Looked at this properly and I would rather not fix it here, though I want to
lay out why since the observation is correct.
`EmbeddedDashboardDAO.upsert` has the identical read-then-write against its
own relationship, and neither `embedded_dashboards.dashboard_id` nor
`embedded_charts.slice_id` carries a unique constraint in any migration. I did
consider adding one to `embedded_charts` while the migration is still
unshipped, but both models document the current behaviour as deliberate — "This
data model allows multiple configurations for a given [dashboard/chart], but at
this time the API only allows setting one" — so a constraint would contradict
the model's own stated contract, and only on the new table.
If that contract should tighten to one configuration per resource, the
change covers both tables and both DAOs and is worth doing on its own rather
than folded into this one. Happy to raise it separately.
##########
superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx:
##########
@@ -59,15 +62,19 @@ const ButtonRow = styled.div`
justify-content: flex-end;
`;
-export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => {
+export const DashboardEmbedControls = ({
+ dashboardId,
+ resourceType = 'dashboard',
+ onHide,
+}: Props) => {
const { addInfoToast, addDangerToast } = useToasts();
const [ready, setReady] = useState(true); // whether we have initialized yet
const [loading, setLoading] = useState(false); // whether we are currently
doing an async thing
const [embedded, setEmbedded] = useState<EmbeddedDashboard | null>(null); //
the embedded dashboard config
const [allowedDomains, setAllowedDomains] = useState<string>('');
const [showDeactivateConfirm, setShowDeactivateConfirm] = useState(false);
- const endpoint = `/api/v1/dashboard/${dashboardId}/embedded`;
+ const endpoint = `/api/v1/${resourceType}/${dashboardId}/embedded`;
Review Comment:
Fixed. The effect depended on `[dashboardId]` while the `endpoint` it calls
is derived from `resourceType` as well, so it now depends on `[endpoint]`,
which covers both.
Worth noting for accuracy: both call sites pass a fixed `resourceType` for
the component's lifetime today (`SliceHeaderControls` always `chart`, `Header`
always the default), so this is a latent stale-closure hazard rather than a
reachable bug. The dependency was wrong either way. Added a test that flips
`resourceType` via `rerender` while holding `dashboardId` constant.
##########
superset-frontend/src/embedded/embeddedChart/hydrateEmbedded.ts:
##########
@@ -0,0 +1,163 @@
+/**
+ * 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 { DataMaskWithId, JsonObject } from '@superset-ui/core';
+import { chart } from 'src/components/Chart/chartReducer';
+import { getInitialDataMask } from 'src/dataMask/reducer';
+import { applyDefaultFormData } from 'src/explore/store';
+import { CommonBootstrapData } from 'src/types/bootstrapTypes';
+import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate';
+import { Datasource } from 'src/dashboard/types';
+import {
+ DASHBOARD_ROOT_ID,
+ DASHBOARD_GRID_ID,
+} from 'src/dashboard/util/constants';
+import {
+ DASHBOARD_ROOT_TYPE,
+ DASHBOARD_GRID_TYPE,
+} from 'src/dashboard/util/componentTypes';
+
+/**
+ * A chart embedded on its own still renders through the dashboard's chart
+ * stack, because that is where cross-filtering, drill, and the header controls
+ * live. Rather than reimplement any of that, this builds the minimum slice of
+ * dashboard state a single chart needs and lets the existing components run
+ * against it unchanged.
+ *
+ * It reuses HYDRATE_DASHBOARD rather than introducing a parallel action, so
+ * every dashboard reducer stays untouched: `charts`, `sliceEntities`,
+ * `dataMask`, `dashboardInfo` and `dashboardState` all already handle it.
+ * `dashboardLayout` and `nativeFilters` handle it too but dereference their
+ * slice unconditionally, so the payload carries an empty stand-in for each.
+ * `datasources` is the one slice with no hydrate handler at all, so the caller
+ * dispatches `setDatasources` for it separately.
+ *
+ * Every slice any HYDRATE_DASHBOARD handler reads has to appear here; the
+ * accompanying test asserts that, because a missing one only fails at runtime
+ * and only in the embedded path.
+ */
+
+export interface EmbeddedChartData {
+ slice: {
+ slice_id: number;
+ slice_url: string;
+ slice_name: string;
+ form_data: JsonObject & { viz_type: string; datasource: string };
+ description?: string | null;
+ description_markeddown?: string | null;
+ modified?: string | null;
+ changed_on?: string | number | null;
+ };
+ // The explore endpoint returns the full datasource, and `setDatasources`
+ // stores it as one, so it is typed as such rather than loosely.
+ dataset: Datasource;
+}
+
+export interface HydrateEmbeddedAction {
+ type: typeof HYDRATE_DASHBOARD;
+ data: {
+ charts: Record<number, JsonObject>;
+ sliceEntities: { slices: Record<number, JsonObject> };
+ dataMask: Record<number, DataMaskWithId>;
+ dashboardInfo: JsonObject;
+ dashboardState: JsonObject;
+ dashboardLayout: { present: JsonObject };
+ nativeFilters: { filters: JsonObject };
+ };
+}
+
+const hydrateEmbedded = (
+ { slice }: EmbeddedChartData,
+ common: CommonBootstrapData,
+): HydrateEmbeddedAction => {
+ const key = slice.slice_id;
+
+ return {
+ type: HYDRATE_DASHBOARD,
+ data: {
+ charts: {
+ [key]: {
+ ...chart,
+ id: key,
+ form_data: applyDefaultFormData(slice.form_data),
+ },
+ },
+ sliceEntities: {
+ slices: {
+ [key]: {
+ slice_id: key,
+ slice_url: slice.slice_url,
+ slice_name: slice.slice_name,
+ form_data: slice.form_data,
+ viz_type: slice.form_data.viz_type,
+ datasource: slice.form_data.datasource,
+ description: slice.description,
+ description_markeddown: slice.description_markeddown,
Review Comment:
Fixed, though the diagnosis is slightly different from the one here.
`description_markeddown` is the API's own spelling — it is the field name on
the chart schema, and both `hydrate.ts` and `sliceEntities.ts` read it and
write the value out under the correctly spelled `description_markdown` key that
`Chart.tsx` reads.
The bug was that this payload passed the API spelling straight through
instead of performing that rename, so the chart stack never found the field.
The input name is unchanged; the output key is now `description_markdown`,
matching the two existing handlers. Added a regression test.
##########
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:
Moved the fetch rather than documenting the grant, and you were right that
it needed the `ChartFilter` work too.
The reason I did not go the documentation route: `/api/v1/explore/` builds
its payload from request-supplied `form_data` and resolves the datasource it
returns from that rather than from the `slice_id`, so a grant there reaches
well past the one chart a token names — and since `GUEST_ROLE_NAME` defaults to
`Public`, that grant would not have been scoped to embedding either.
The embed now reads `GET /api/v1/chart/<pk>/embedded_context`, which returns
the chart and its dataset in one payload, sits under `Chart` read, and resolves
one fixed chart. It is the chart analogue of a dashboard's `/charts` and
`/datasets` sub-resources, collapsed into one call since a chart has exactly
one of each.
`ChartFilter`'s guest branch was worse than the 404 you saw: a chart-only
token produced an empty dashboard scope, which the branch turned into a
deny-all clause, so *every* Chart-API read was refused for exactly the tokens
this feature issues. It now unions the token's dashboard scope with an `EXISTS`
over the chart's own embed rows, and a token granting neither still denies
everything.
Checked on a running instance with the guest role holding only the shipped
`PUBLIC_ROLE_PERMISSIONS` (no Explore grant): `/api/v1/explore/?slice_id=103`
returns 403, `/api/v1/chart/103/embedded_context` returns 200, and a token for
one chart gets 404 on another. Three charts render end to end through the SDK
on a third-party page.
One thing worth flagging since it differs from the dashboard path: for the
single dataset the caller was just authorised on, a guest holding that chart's
token keeps the rendering metadata — `columns`, `metrics`, `verbose_map` —
which the dashboard's rule strips. Without it the chart cannot draw. `params`
stays withheld even then, being operator-authored configuration the renderer
never reads, and `DashboardDatasetSchema`'s own `post_dump` still removes
connection and query internals for any guest.
##########
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:
Added the CHART branch to both spots and kept the column.
`_is_guest_token_revoked_by_embedded` now dispatches on resource type
instead of skipping everything that is not a dashboard, and
`revoke_guest_token_access` retries the uuid against `EmbeddedChartDAO` when it
does not resolve to an embedded dashboard. The two uuid spaces are distinct, so
the fallthrough is unambiguous — the same ordering `EmbeddedView.embedded`
already uses. Charts are only ever addressed by their embed uuid, so there is
no equivalent of the dashboard branch's legacy raw-id lookup.
Tests mirror the existing dashboard cases: cutoff in the future rejects, a
token issued after the cutoff is accepted, no cutoff accepts, a token with no
`iat` is rejected when a cutoff is set, an unresolvable uuid leaves nothing to
enforce, plus a mixed dashboard-and-chart token revoked by the chart's cutoff
alone.
One difference from what you observed: on this endpoint a revoked chart
token comes back 404 rather than 401, because the guest never resolves and
`ChartFilter` then excludes the row. Same outcome, different route. Confirmed
on a running instance — the same token returned 200 before the cutoff and 404
after.
The request-loader test needed one precaution worth mentioning:
`get_guest_user_from_request` swallows every failure into `None`, so asserting
only "returns `None`" would pass for unrelated reasons. It replays the
identical request twice, changing only the cutoff.
##########
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:
Fixed, and thanks for pinning it down empirically — that made it unambiguous.
`chart_id` is now `fields.String(attribute="slice_id")`, so it dumps from
the column the model actually has. I kept the public field name rather than
renaming to `slice_id`: the convention across both embedded response schemas is
one field named after the parent resource type, and the endpoint's own docs
already describe the path parameter as the chart id.
Took the mirroring suggestion too. `changed_by` now points at a schema
dedicated to this response that matches `EmbeddedDashboardResponseSchema`'s
nested user schema field for field, rather than changing the shared
`UserSchema` that other chart endpoints dump through.
On a running instance the two responses are now the same shape:
```
chart: {"uuid": "...", "allowed_domains": [...], "chart_id": "103",
"changed_on": "...", "changed_by": null}
dashboard: {"uuid": "...", "allowed_domains": [...], "dashboard_id": "9",
"changed_on": "...", "changed_by": null}
```
--
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]
