(superset) branch master updated: fix(dashboard): count extra_editors when deciding who may edit a dashboard (#42708)

[ru...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git


The following commit(s) were added to refs/heads/master by this push:
     new da9f4216e19 fix(dashboard): count extra_editors when deciding who may edit a dashboard (#42708)
da9f4216e19 is described below

commit da9f4216e19834ff1e247eba72c8b73ea4826481
Author: Mike Bridge <mi...@bridgecanada.com>
AuthorDate: Mon Aug 3 20:20:52 2026 +0100

    fix(dashboard): count extra_editors when deciding who may edit a dashboard (#42708)
    
    Co-authored-by: Claude Opus 5 (1M context) <no...@anthropic.com>
---
 .../src/dashboard/util/permissionUtils.test.ts     | 38 ++++++++++++++++++++++
 .../src/dashboard/util/permissionUtils.ts          | 34 +++++++++++++++++--
 superset-frontend/src/types/Dashboard.ts           |  4 +++
 3 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/superset-frontend/src/dashboard/util/permissionUtils.test.ts b/superset-frontend/src/dashboard/util/permissionUtils.test.ts
index fa548403188..2798472604a 100644
--- a/superset-frontend/src/dashboard/util/permissionUtils.test.ts
+++ b/superset-frontend/src/dashboard/util/permissionUtils.test.ts
@@ -132,6 +132,44 @@ test('isUserDashboardEditor returns false when editors is empty', () => {
   expect(isUserDashboardEditor(dashNoEditors)).toEqual(false);
 });
 
+test('isUserDashboardEditor counts editorship granted through extra_editors', () => {
+  // The server's is_editor unions EXTRA_EDITORS_RESOLVER output into the
+  // editor set, and the API attaches it after the response schema is dumped
+  // (so it survives the columns projection). extra_editors arrives as bare
+  // ids, not Subjects.
+  const dashExtraEditor = { ...dashboard, editors: [], extra_editors: [10] };
+  expect(isUserDashboardEditor(dashExtraEditor)).toEqual(true);
+  expect(canUserEditDashboard(dashExtraEditor, editorUser)).toEqual(true);
+});
+
+test('isUserDashboardEditor unions the two lists rather than preferring one', () => {
+  // Pins the union semantics: a refactor that consulted extra_editors only
+  // when editors is empty would pass the empty-editors cases above but
+  // fail here — non-matching editors must not mask a matching extra grant.
+  const dashUnion = {
+    ...dashboard,
+    editors: [{ id: 999, label: 'Other', type: 1 }],
+    extra_editors: [10],
+  };
+  expect(isUserDashboardEditor(dashUnion)).toEqual(true);
+});
+
+test('an empty extra_editors grants nothing on its own', () => {
+  // The resolver-configured-but-returns-nothing shape.
+  const dashEmptyExtra = { ...dashboard, editors: [], extra_editors: [] };
+  expect(isUserDashboardEditor(dashEmptyExtra)).toEqual(false);
+});
+
+test('isUserDashboardEditor ignores extra_editors for other subjects', () => {
+  const dashOtherExtraEditor = {
+    ...dashboard,
+    editors: [],
+    extra_editors: [999],
+  };
+  expect(isUserDashboardEditor(dashOtherExtraEditor)).toEqual(false);
+  expect(canUserEditDashboard(dashOtherExtraEditor, editorUser)).toEqual(false);
+});
+
 test('canUserEditDashboard allows editors', () => {
   expect(canUserEditDashboard(dashboard, editorUser)).toEqual(true);
 });
diff --git a/superset-frontend/src/dashboard/util/permissionUtils.ts b/superset-frontend/src/dashboard/util/permissionUtils.ts
index 3c74b901f40..be9b8ea4999 100644
--- a/superset-frontend/src/dashboard/util/permissionUtils.ts
+++ b/superset-frontend/src/dashboard/util/permissionUtils.ts
@@ -33,11 +33,32 @@ const ADMIN_ROLE_NAME = 'admin';
 const getUserSubjects = (): number[] =>
   getBootstrapData()?.common?.user_subjects ?? [];
 
-const isUserInEditors = (editors: Subject[] = []): boolean => {
+/**
+ * Editor lists arrive either as full Subjects (`editors`) or as bare subject
+ * ids (`extra_editors`, which the API dumps straight from
+ * `get_extra_editor_subject_ids`).
+ */
+export type SubjectRef = number | { id: number };
+
+const subjectId = (subject: SubjectRef): number =>
+  typeof subject === 'number' ? subject : subject.id;
+
+/**
+ * Whether the viewer is any of the given subjects — their user, roles or
+ * groups — across every supplied list.
+ */
+export const isUserInSubjects = (
+  ...subjectLists: (SubjectRef[] | null | undefined)[]
+): boolean => {
   const userSubjects = getUserSubjects();
-  return editors.some(editor => userSubjects.includes(editor.id));
+  return subjectLists.some(subjects =>
+    (subjects ?? []).some(subject => userSubjects.includes(subjectId(subject))),
+  );
 };
 
+const isUserInEditors = (editors: Subject[] = []): boolean =>
+  isUserInSubjects(editors);
+
 export const isUserAdmin = (
   user?: UserWithPermissionsAndRoles | UndefinedUser,
 ) =>
@@ -51,8 +72,15 @@ export const isUserEditorOrAdmin = (
   editors: Subject[] = [],
 ): boolean => isUserInEditors(editors) || isUserAdmin(user);
 
+/**
+ * Editorship of *dashboard*, matching the server's `is_editor`: the explicit
+ * editor list unioned with any editorship granted indirectly by a
+ * deployment's EXTRA_EDITORS_RESOLVER. Must stay in step with the server's
+ * `is_editor` (security/manager.py): a predicate narrower than the server's
+ * hides actions the API permits.
+ */
 export const isUserDashboardEditor = (dashboard: Dashboard): boolean =>
-  isUserInEditors(dashboard.editors);
+  isUserInSubjects(dashboard.editors, dashboard.extra_editors);
 
 export const canUserEditDashboard = (
   dashboard: Dashboard,
diff --git a/superset-frontend/src/types/Dashboard.ts b/superset-frontend/src/types/Dashboard.ts
index 5af71ef63ef..d4d9cea1121 100644
--- a/superset-frontend/src/types/Dashboard.ts
+++ b/superset-frontend/src/types/Dashboard.ts
@@ -34,6 +34,10 @@ export interface Dashboard {
   changed_on: string;
   charts: string[]; // just chart names, unfortunately...
   editors?: Subject[];
+  // Subject ids resolved by a deployment's EXTRA_EDITORS_RESOLVER; the API
+  // attaches them after the response schema is dumped, so they survive the
+  // `columns` projection. Bare ids, not Subjects.
+  extra_editors?: number[];
   viewers?: Subject[];
   theme?: {
     id: number;