fix(explore): require chart write permission for Edit chart properties

canModifySlice alone let an editor who lacks can_write on Chart see
"Edit chart properties", even though ChartRestApi.put (and
restore_version) require that permission at the route level and would
turn the request away. Gate that menu item on state.explore.can_add as
well, matching what the API actually enforces. Version history is left
on canModifySlice alone since its own listing endpoints only need read
access; only its restore action needs write, and that's already
enforced server-side.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx

@@ -170,7 +170,10 @@ describe('ExploreChartHeader', () => {
 
   test('Cancelling changes to the properties should reset previous properties', async () => {
     const props = createProps();
-    render(<ExploreHeader {...props} />, { useRedux: true });
+    render(<ExploreHeader {...props} />, {
+      useRedux: true,
+      initialState: { explore: { can_overwrite: true, can_add: true } },
+    });
     const newChartName = 'New chart name';
     const prevChartName = props.sliceName;
 
@@ -626,6 +629,7 @@ describe('Additional actions tests', () => {
     const props = createProps();
     render(<ExploreHeader {...props} />, {
       useRedux: true,
+      initialState: { explore: { can_overwrite: true, can_add: true } },
     });
 
     userEvent.click(screen.getByLabelText('Menu actions trigger'));
@@ -720,6 +724,7 @@ describe('Additional actions tests', () => {
     const props = createProps();
     render(<ExploreHeader {...props} />, {
       useRedux: true,
+      initialState: { explore: { can_overwrite: true, can_add: true } },
     });
     expect(props.actions.redirectSQLLab).toHaveBeenCalledTimes(0);
     userEvent.click(screen.getByLabelText('Menu actions trigger'));

superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx

@@ -279,6 +279,7 @@ interface ExploreState {
     chartStates?: Record<number, JsonObject>;
     can_export_image?: boolean;
     can_overwrite?: boolean;
+    can_add?: boolean;
   };
   common?: {
     conf?: {
@@ -335,17 +336,30 @@ export const useExploreAdditionalActionsMenu = (
   const canOverwrite = useSelector<ExploreState, boolean>(
     state => state.explore?.can_overwrite ?? false,
   );
+  // Mirrors the `can_write` permission on the `Chart` view, the same
+  // permission `ChartRestApi.put` (and `restore_version`) require. An editor
+  // who satisfies `canOverwriteSlice` but lacks it would still be turned away
+  // by the API, so the properties editor stays hidden for them too.
+  const canWriteChart = useSelector<ExploreState, boolean>(
+    state => state.explore?.can_add ?? false,
+  );
   const user = useSelector<
     ExploreState,
     UserWithPermissionsAndRoles | undefined
   >(state => state.user);
-  // `can_overwrite` alone hides version history on any chart without explicit
-  // editors — every seeded chart — even from admins. Same predicate SaveModal
-  // uses, so a user who can save a chart can also see its history.
+  // `can_overwrite` alone hides version history (and edit-properties) on any
+  // chart without explicit editors — every seeded chart — even from admins.
+  // Same predicate SaveModal uses, so a user who can save a chart can also
+  // see its history and edit its properties.
   const canModifySlice = useMemo(
     () => canOverwriteSlice({ slice, user, canOverwrite }),
     [slice, user, canOverwrite],
   );
+  // `canModifySlice` alone governs version history, whose own read-only
+  // listing needs no write permission (only its restore action does, and
+  // that's gated server-side). Editing properties, however, always PUTs the
+  // chart, so it additionally needs the write permission above.
+  const canEditProperties = canModifySlice && canWriteChart;
 
   const dataExportDisabled = !canDownloadCSV;
   const imageExportDisabled = !canExportImage;
@@ -601,7 +615,7 @@ export const useExploreAdditionalActionsMenu = (
     const menuItems = [];
 
     // Edit chart properties
-    if (slice) {
+    if (slice && canEditProperties) {
       menuItems.push({
         key: MENU_KEYS.EDIT_PROPERTIES,
         label: t('Edit chart properties'),
@@ -1084,6 +1098,7 @@ export const useExploreAdditionalActionsMenu = (
   }, [
     addDangerToast,
     canDownloadCSV,
+    canEditProperties,
     canModifySlice,
     copyLink,
     dashboards,

superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.tsx

@@ -27,6 +27,7 @@ import {
   getExportScreenshotMenuItems,
 } from './index';
 import * as exploreUtils from 'src/explore/exploreUtils';
+import { Slice } from 'src/types/Chart';
 
 jest.mock('src/explore/exploreUtils', () => ({
   __esModule: true,
@@ -74,13 +75,22 @@ jest.mock('@superset-ui/core', () => ({
   })),
 }));
 
+jest.mock('src/utils/getBootstrapData', () => ({
+  __esModule: true,
+  default: jest.fn(() => ({
+    common: {
+      user_subjects: [1],
+    },
+  })),
+}));
+
 const defaultProps = {
   latestQueryFormData: {
     datasource: '1__table',
     viz_type: 'pivot_table_v2',
   },
   canDownloadCSV: true,
-  slice: { slice_id: 1, slice_name: 'Test Chart' },
+  slice: { slice_id: 1, slice_name: 'Test Chart' } as unknown as Slice,
   ownState: {},
   dashboards: [],
   onOpenInEditor: jest.fn(),
@@ -113,6 +123,63 @@ beforeEach(() => {
   mockExportChart.mockResolvedValue(undefined);
 });
 
+test('hides Edit chart properties from a user who is not an owner/editor of the chart (regression #38884)', async () => {
+  render(
+    <TestComponent
+      {...defaultProps}
+      slice={
+        {
+          slice_id: 1,
+          slice_name: 'Test Chart',
+          editors: [2],
+        } as unknown as Slice
+      }
+    />,
+    { useRedux: true },
+  );
+
+  expect(await screen.findByText('Data Export Options')).toBeInTheDocument();
+  expect(screen.queryByText('Edit chart properties')).not.toBeInTheDocument();
+});
+
+test('shows Edit chart properties for a chart editor with chart write permission', async () => {
+  render(
+    <TestComponent
+      {...defaultProps}
+      slice={
+        {
+          slice_id: 1,
+          slice_name: 'Test Chart',
+          editors: [1],
+        } as unknown as Slice
+      }
+    />,
+    { useRedux: true, initialState: { explore: { can_add: true } } },
+  );
+
+  expect(await screen.findByText('Data Export Options')).toBeInTheDocument();
+  expect(screen.getByText('Edit chart properties')).toBeInTheDocument();
+});
+
+test('hides Edit chart properties from a chart editor lacking chart write permission', async () => {
+  render(
+    <TestComponent
+      {...defaultProps}
+      slice={
+        {
+          slice_id: 1,
+          slice_name: 'Test Chart',
+          editors: [1],
+        } as unknown as Slice
+      }
+    />,
+    { useRedux: true, initialState: { explore: { can_add: false } } },
+  );
+
+  expect(await screen.findByText('Data Export Options')).toBeInTheDocument();
+  expect(screen.queryByText('Edit chart properties')).not.toBeInTheDocument();
+});
+
 test('shows 413 error toast when exportCSV fails with 413', async () => {
   mockExportChart.mockRejectedValue({ status: 413 });