aminghadersohi commented on code in PR #36933:
URL: https://github.com/apache/superset/pull/36933#discussion_r2932171782
##########
superset/security/manager.py:
##########
@@ -3109,6 +3131,28 @@ def has_guest_access(self, dashboard: "Dashboard") ->
bool:
return True
return False
+ def has_guest_chart_permalink_access(self, datasource: Any) -> bool:
+ """
+ Check if a guest user has access to a datasource via a chart permalink
resource.
+
+ For embedded charts, the guest token contains chart_permalink
resources.
+ This grants datasource access when the user holds at least one
+ chart_permalink resource. The embedded chart page constrains the actual
+ query to the datasource specified in the permalink's formData.
+
+ TODO: For stricter security, resolve each permalink and compare the
+ datasource in its formData against the requested datasource. This would
+ prevent a guest user from crafting requests to other datasources.
+ """
+ user = self.get_current_guest_user_if_guest()
+ if not user or not isinstance(user, GuestUser):
+ return False
+
+ return any(
+ r.get("type") == GuestTokenResourceType.CHART_PERMALINK.value
+ for r in user.resources
+ )
Review Comment:
This is a valid concern and is explicitly documented in the code as a known
trade-off. See the docstring and TODO at lines 3135-3145:
```python
def has_guest_chart_permalink_access(self, datasource: Any) -> bool:
"""
...
This grants datasource access when the user holds at least one
chart_permalink resource. The embedded chart page constrains the actual
query to the datasource specified in the permalink's formData.
TODO: For stricter security, resolve each permalink and compare the
datasource in its formData against the requested datasource. This would
prevent a guest user from crafting requests to other datasources.
"""
```
The current approach grants datasource access to any guest user holding a
`chart_permalink` resource. The rationale is that the embedded chart frontend
only queries the specific datasource from the permalink's `formData`, so in
normal usage the scope is constrained. The TODO acknowledges that for stricter
security, each permalink should be resolved and the datasource compared — this
is a deliberate design choice to avoid the performance cost of permalink
resolution on every datasource access check, with a planned follow-up for
tighter validation.
##########
superset-frontend/src/pages/EmbeddedChartsList/index.tsx:
##########
@@ -0,0 +1,254 @@
+/**
+ * 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 { useCallback, useEffect, useMemo, useState } from 'react';
+import { Link } from 'react-router-dom';
+import { SupersetClient } from '@superset-ui/core';
+import { styled, css } from '@apache-superset/core/theme';
+import { t } from '@apache-superset/core/translation';
+import { DeleteModal, Tooltip } from '@superset-ui/core/components';
+import { Icons } from '@superset-ui/core/components/Icons';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import { ListView } from 'src/components';
+import SubMenu, { SubMenuProps } from 'src/features/home/SubMenu';
+
+const PAGE_SIZE = 25;
+
+interface EmbeddedChart {
+ uuid: string;
+ chart_id: number;
+ chart_name: string | null;
+ allowed_domains: string[];
+ changed_on: string | null;
+}
+
+interface EmbeddedChartsListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+}
+
+const ActionsWrapper = styled.div`
+ display: flex;
+ gap: 8px;
+`;
+
+const ActionButton = styled.button`
+ ${({ theme }) => css`
+ background: none;
+ border: none;
+ cursor: pointer;
+ padding: 4px;
+ color: ${theme.colorTextSecondary};
+ &:hover {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+function EmbeddedChartsList({
+ addDangerToast,
+ addSuccessToast,
+}: EmbeddedChartsListProps) {
+ const [embeddedCharts, setEmbeddedCharts] = useState<EmbeddedChart[]>([]);
+ const [loading, setLoading] = useState(true);
+ const [chartToDelete, setChartToDelete] = useState<EmbeddedChart | null>(
+ null,
+ );
+
+ const fetchEmbeddedCharts = useCallback(async () => {
+ setLoading(true);
+ try {
+ const response = await SupersetClient.get({
+ endpoint: '/api/v1/chart/embedded',
+ });
+ setEmbeddedCharts(response.json.result || []);
+ } catch (error) {
+ addDangerToast(t('Error loading embedded charts'));
+ } finally {
+ setLoading(false);
+ }
+ }, [addDangerToast]);
+
+ useEffect(() => {
+ fetchEmbeddedCharts();
+ }, [fetchEmbeddedCharts]);
+
+ const handleDelete = useCallback(
+ async (chart: EmbeddedChart) => {
+ try {
+ await SupersetClient.delete({
+ endpoint: `/api/v1/chart/${chart.chart_id}/embedded`,
+ });
+ addSuccessToast(t('Embedding disabled for %s', chart.chart_name));
+ fetchEmbeddedCharts();
+ } catch (error) {
+ addDangerToast(t('Error disabling embedding for %s',
chart.chart_name));
Review Comment:
This is already handled. All places where `chart_name` is used have an `||
t('Untitled')` fallback:
- **Delete success toast** (line 98): `chart.chart_name || t('Untitled')`
- **Delete error toast** (line 101): `chart.chart_name || t('Untitled')`
- **Chart name column** (line 136): `chart_name || t('Untitled')`
- **Delete modal description** (line 242): `chartToDelete.chart_name ||
t('Untitled')`
The `EmbeddedChart` interface correctly types `chart_name` as `string |
null` (line 36), and every usage provides the `|| t('Untitled')` fallback to
prevent 'null' from appearing in the UI.
--
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]
