You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by en...@apache.org on 2026/08/21 16:04:55 UTC
(superset) 01/01: fix(security): honor extra_editors in dashboard/chart list responses
This is an automated email from the ASF dual-hosted git repository.
EnxDev pushed a commit to branch enxdev/fix/dashboard-list
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 5aa2b451f15ca6ab3845aa5ebbcdd41984c2f693
Author: Enzo Martellucci <en...@gmail.com>
AuthorDate: Fri Aug 21 18:04:06 2026 +0200
fix(security): honor extra_editors in dashboard/chart list responses
---
.../src/dashboard/util/permissionUtils.test.ts | 15 ++++
.../src/dashboard/util/permissionUtils.ts | 7 +-
.../src/features/charts/ChartCard.tsx | 6 +-
.../src/features/dashboards/DashboardCard.tsx | 6 +-
superset-frontend/src/pages/ChartList/index.tsx | 6 +-
.../src/pages/DashboardList/index.tsx | 8 ++-
superset-frontend/src/types/Chart.ts | 2 +
superset-frontend/src/views/CRUD/types.ts | 2 +
superset/charts/api.py | 14 +++-
superset/dashboards/api.py | 14 +++-
superset/security/manager.py | 30 ++++++++
tests/integration_tests/dashboards/api_tests.py | 84 ++++++++++++++++++++++
12 files changed, 184 insertions(+), 10 deletions(-)
diff --git a/superset-frontend/src/dashboard/util/permissionUtils.test.ts b/superset-frontend/src/dashboard/util/permissionUtils.test.ts
index 04d7b239450..d3cf09b5c7d 100644
--- a/superset-frontend/src/dashboard/util/permissionUtils.test.ts
+++ b/superset-frontend/src/dashboard/util/permissionUtils.test.ts
@@ -259,6 +259,21 @@ describe('isUserEditorOrAdmin', () => {
test('returns false when editors is omitted', () => {
expect(isUserEditorOrAdmin(outsiderUser)).toEqual(false);
});
+
+ test('returns true when the user is granted editorship only through extra_editors', () => {
+ expect(isUserEditorOrAdmin(editorUser, [], [10])).toEqual(true);
+ });
+
+ test('unions editors and extra_editors rather than preferring one', () => {
+ const nonMatchingSubject: Subject = { id: 999, label: 'Other', type: 1 };
+ expect(isUserEditorOrAdmin(editorUser, [nonMatchingSubject], [10])).toEqual(
+ true,
+ );
+ });
+
+ test('returns false when extra_editors names other subjects', () => {
+ expect(isUserEditorOrAdmin(editorUser, [], [999])).toEqual(false);
+ });
});
// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
diff --git a/superset-frontend/src/dashboard/util/permissionUtils.ts b/superset-frontend/src/dashboard/util/permissionUtils.ts
index 448fd60b502..bf746dec402 100644
--- a/superset-frontend/src/dashboard/util/permissionUtils.ts
+++ b/superset-frontend/src/dashboard/util/permissionUtils.ts
@@ -55,9 +55,6 @@ export const isUserInSubjects = (
);
};
-const isUserInEditors = (editors: Subject[] = []): boolean =>
- isUserInSubjects(editors);
-
export const isUserAdmin = (
user?: UserWithPermissionsAndRoles | UndefinedUser,
) =>
@@ -66,10 +63,12 @@ export const isUserAdmin = (
role => role.toLowerCase() === ADMIN_ROLE_NAME.toLowerCase(),
);
+/** `extraEditors` is editorship granted via a deployment's EXTRA_EDITORS_RESOLVER. */
export const isUserEditorOrAdmin = (
user?: UserWithPermissionsAndRoles | UndefinedUser,
editors: Subject[] = [],
-): boolean => isUserInEditors(editors) || isUserAdmin(user);
+ extraEditors?: SubjectRef[] | null,
+): boolean => isUserInSubjects(editors, extraEditors) || isUserAdmin(user);
/**
* Editorship of *dashboard*, matching the server's `is_editor`: the explicit
diff --git a/superset-frontend/src/features/charts/ChartCard.tsx b/superset-frontend/src/features/charts/ChartCard.tsx
index a4c4d47bf79..494d6a3c052 100644
--- a/superset-frontend/src/features/charts/ChartCard.tsx
+++ b/superset-frontend/src/features/charts/ChartCard.tsx
@@ -96,7 +96,11 @@ export default function ChartCard({
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canExport = hasPerm('can_export');
- const allowEdit = isUserEditorOrAdmin(user, chart.editors);
+ const allowEdit = isUserEditorOrAdmin(
+ user,
+ chart.editors,
+ chart.extra_editors,
+ );
const menuItems: MenuItem[] = [];
if (canEdit) {
diff --git a/superset-frontend/src/features/dashboards/DashboardCard.tsx b/superset-frontend/src/features/dashboards/DashboardCard.tsx
index 63e854a388a..02b8171074e 100644
--- a/superset-frontend/src/features/dashboards/DashboardCard.tsx
+++ b/superset-frontend/src/features/dashboards/DashboardCard.tsx
@@ -83,7 +83,11 @@ function DashboardCard({
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canExport = hasPerm('can_export');
- const allowEdit = isUserEditorOrAdmin(user, dashboard.editors);
+ const allowEdit = isUserEditorOrAdmin(
+ user,
+ dashboard.editors,
+ dashboard.extra_editors,
+ );
const digest = dashboard.changed_on_utc || dashboard.changed_on;
const thumbnailUrl =
isFeatureEnabled(FeatureFlag.Thumbnails) && dashboard.id && digest
diff --git a/superset-frontend/src/pages/ChartList/index.tsx b/superset-frontend/src/pages/ChartList/index.tsx
index 09666a74dc5..c4e192882ab 100644
--- a/superset-frontend/src/pages/ChartList/index.tsx
+++ b/superset-frontend/src/pages/ChartList/index.tsx
@@ -527,7 +527,11 @@ function ChartList(props: ChartListProps) {
},
{
Cell: ({ row: { original } }: CellProps<Chart>) => {
- const allowEdit = isUserEditorOrAdmin(user, original.editors);
+ const allowEdit = isUserEditorOrAdmin(
+ user,
+ original.editors,
+ original.extra_editors,
+ );
const handleDelete = () =>
handleChartDelete(
original,
diff --git a/superset-frontend/src/pages/DashboardList/index.tsx b/superset-frontend/src/pages/DashboardList/index.tsx
index a5513422098..6bc23634086 100644
--- a/superset-frontend/src/pages/DashboardList/index.tsx
+++ b/superset-frontend/src/pages/DashboardList/index.tsx
@@ -122,6 +122,8 @@ export interface Dashboard {
description?: string;
thumbnail_url?: string | null;
editors?: Subject[];
+ // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+ extra_editors?: number[];
viewers?: Subject[];
tags: TagType[];
created_by: object;
@@ -505,7 +507,11 @@ function DashboardList(props: DashboardListProps) {
},
{
Cell: ({ row: { original } }: CellProps<Dashboard>) => {
- const allowEdit = isUserEditorOrAdmin(user, original.editors);
+ const allowEdit = isUserEditorOrAdmin(
+ user,
+ original.editors,
+ original.extra_editors,
+ );
const handleDelete = () =>
handleDashboardDelete(
original,
diff --git a/superset-frontend/src/types/Chart.ts b/superset-frontend/src/types/Chart.ts
index 499d5115dd2..cc5f369be49 100644
--- a/superset-frontend/src/types/Chart.ts
+++ b/superset-frontend/src/types/Chart.ts
@@ -45,6 +45,8 @@ export interface Chart {
cache_timeout: number | null;
thumbnail_url?: string;
editors?: Subject[];
+ // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+ extra_editors?: number[];
viewers?: Subject[];
tags?: TagType[];
last_saved_at?: string;
diff --git a/superset-frontend/src/views/CRUD/types.ts b/superset-frontend/src/views/CRUD/types.ts
index 83fef7ab1f6..08206bb0382 100644
--- a/superset-frontend/src/views/CRUD/types.ts
+++ b/superset-frontend/src/views/CRUD/types.ts
@@ -67,6 +67,8 @@ export interface Dashboard {
url: string;
thumbnail_url?: string | null;
editors?: Subject[];
+ // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+ extra_editors?: number[];
viewers?: Subject[];
loading?: boolean;
}
diff --git a/superset/charts/api.py b/superset/charts/api.py
index 1e3626bd77d..7f19a9136aa 100644
--- a/superset/charts/api.py
+++ b/superset/charts/api.py
@@ -92,7 +92,10 @@ from superset.exceptions import (
)
from superset.extensions import event_logger, security_manager
from superset.models.slice import Slice
-from superset.security.manager import get_extra_editor_subject_ids
+from superset.security.manager import (
+ get_extra_editor_subject_ids,
+ get_extra_editors_by_pk,
+)
from superset.subjects.filters import (
FilterRelatedSubjects,
subject_type_filter,
@@ -410,6 +413,15 @@ class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
except ChartNotFoundError:
return self.response_404()
+ def pre_get_list(self, data: dict[str, Any]) -> None:
+ """Attach ``extra_editors`` to each row, matching the single-object GET."""
+ super().pre_get_list(data)
+ ids = data.get("ids", [])
+ extra_editors_by_id = get_extra_editors_by_pk(Slice, ids)
+ for row, row_id in zip(data.get("result", []), ids, strict=False):
+ if row_id in extra_editors_by_id:
+ row["extra_editors"] = extra_editors_by_id[row_id]
+
@expose("/<pk>/deck_layers/", methods=("GET",))
@protect()
@safe
diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py
index cbde7183106..b51d6cc4ee0 100644
--- a/superset/dashboards/api.py
+++ b/superset/dashboards/api.py
@@ -140,7 +140,10 @@ from superset.extensions import event_logger, security_manager
from superset.models.dashboard import Dashboard
from superset.models.embedded_dashboard import EmbeddedDashboard
from superset.security.guest_token import GuestUser
-from superset.security.manager import get_extra_editor_subject_ids
+from superset.security.manager import (
+ get_extra_editor_subject_ids,
+ get_extra_editors_by_pk,
+)
from superset.subjects.filters import (
FilterRelatedSubjects,
subject_type_filter,
@@ -431,6 +434,15 @@ class DashboardRestApi(
"""
return super().get_list(**kwargs)
+ def pre_get_list(self, data: dict[str, Any]) -> None:
+ """Attach ``extra_editors`` to each row, matching the single-object GET."""
+ super().pre_get_list(data)
+ ids = data.get("ids", [])
+ extra_editors_by_id = get_extra_editors_by_pk(Dashboard, ids)
+ for row, row_id in zip(data.get("result", []), ids, strict=False):
+ if row_id in extra_editors_by_id:
+ row["extra_editors"] = extra_editors_by_id[row_id]
+
list_select_columns = list_columns + ["changed_on", "created_on", "changed_by_fk"]
order_columns = [
"changed_by.first_name",
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 79c262dfddc..609d9651ca0 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -170,6 +170,36 @@ def get_extra_editor_subject_ids(resource: Model) -> list[int]:
return subject_ids
+def get_extra_editors_by_pk(
+ model_cls: type[Model], primary_keys: list[Any]
+) -> dict[Any, list[int]]:
+ """
+ Resolve extra editor subject IDs for a batch of resources, keyed by
+ primary key. List responses only have serialized rows, not model
+ instances, so this re-queries the page's rows in one batched query.
+ """
+ if not primary_keys or not (
+ has_app_context() and current_app.config.get("EXTRA_EDITORS_RESOLVER")
+ ):
+ return {}
+
+ # pylint: disable=import-outside-toplevel
+ from superset import db
+ from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES
+
+ pk_col = inspect(model_cls).primary_key[0]
+ resources = (
+ db.session.query(model_cls)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model_cls}})
+ .filter(pk_col.in_(primary_keys))
+ .all()
+ )
+ return {
+ getattr(resource, pk_col.name): get_extra_editor_subject_ids(resource)
+ for resource in resources
+ }
+
+
def _render_permission_instructions_link(
*,
datasource_id: str = "",
diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py
index 52857d5f572..456e634071c 100644
--- a/tests/integration_tests/dashboards/api_tests.py
+++ b/tests/integration_tests/dashboards/api_tests.py
@@ -921,6 +921,55 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboards_list_omits_extra_editors_by_default(self):
+ """No EXTRA_EDITORS_RESOLVER configured: list rows omit extra_editors."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard(
+ "no_extra_editors_list_dashboard",
+ "no-extra-editors-list-dashboard",
+ [admin.id],
+ )
+ try:
+ self.login(ADMIN_USERNAME)
+ rv = self.client.get("api/v1/dashboard/")
+ assert rv.status_code == 200
+ data = json.loads(rv.data.decode("utf-8"))
+ row = next(
+ d
+ for d in data["result"]
+ if d["dashboard_title"] == dashboard.dashboard_title
+ )
+ assert "extra_editors" not in row
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @with_config({"EXTRA_EDITORS_RESOLVER": lambda resource: [123]})
+ def test_get_dashboards_list_includes_extra_editors_when_resolver_configured(
+ self,
+ ):
+ """List rows get extra_editors too, mirroring the single-object GET."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard(
+ "extra_editors_list_dashboard",
+ "extra-editors-list-dashboard",
+ [admin.id],
+ )
+ try:
+ self.login(ADMIN_USERNAME)
+ rv = self.client.get("api/v1/dashboard/")
+ assert rv.status_code == 200
+ data = json.loads(rv.data.decode("utf-8"))
+ row = next(
+ d
+ for d in data["result"]
+ if d["dashboard_title"] == dashboard.dashboard_title
+ )
+ assert row["extra_editors"] == [123]
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
def test_get_charts_admin_sees_existing_charts(self):
"""Regression for #25890: GET /api/v1/chart/ as an Admin user should
return existing charts, not an empty list."""
@@ -943,6 +992,41 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
db.session.delete(chart)
db.session.commit()
+ def test_get_charts_list_omits_extra_editors_by_default(self):
+ """No EXTRA_EDITORS_RESOLVER configured: list rows omit extra_editors."""
+ admin = self.get_user("admin")
+ chart = self.insert_chart(
+ "no_extra_editors_list_chart", [admin.id], 1, params="{}"
+ )
+ try:
+ self.login(ADMIN_USERNAME)
+ rv = self.client.get("api/v1/chart/")
+ assert rv.status_code == 200
+ data = json.loads(rv.data.decode("utf-8"))
+ row = next(c for c in data["result"] if c["slice_name"] == chart.slice_name)
+ assert "extra_editors" not in row
+ finally:
+ db.session.delete(chart)
+ db.session.commit()
+
+ @with_config({"EXTRA_EDITORS_RESOLVER": lambda resource: [123]})
+ def test_get_charts_list_includes_extra_editors_when_resolver_configured(self):
+ """List rows get extra_editors too, mirroring the single-object GET."""
+ admin = self.get_user("admin")
+ chart = self.insert_chart(
+ "extra_editors_list_chart", [admin.id], 1, params="{}"
+ )
+ try:
+ self.login(ADMIN_USERNAME)
+ rv = self.client.get("api/v1/chart/")
+ assert rv.status_code == 200
+ data = json.loads(rv.data.decode("utf-8"))
+ row = next(c for c in data["result"] if c["slice_name"] == chart.slice_name)
+ assert row["extra_editors"] == [123]
+ finally:
+ db.session.delete(chart)
+ db.session.commit()
+
def test_get_dashboards_filter(self):
"""
Dashboard API: Test get dashboards filter