fix(frontend): fix edit/delete permissions for charts, dashboards, and datasets (#18870) (#32981)

superset-frontend/spec/helpers/mockBootstrapData.ts

@@ -0,0 +1,47 @@
+/**
+ * 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.
+ */
+
+/**
+ * Shared `jest.mock('src/utils/getBootstrapData')` factory for list view
+ * tests that rely on `isUserEditorOrAdmin`'s subject-based check
+ * (`common.user_subjects`) to treat a mock user as a subject editor.
+ *
+ * Usage:
+ *   jest.mock('src/utils/getBootstrapData', () =>
+ *     mockUserSubjectsBootstrapData([1]),
+ *   );
+ *
+ * NOTE: the `mock` name prefix is required so babel-plugin-jest-hoist allows
+ * referencing this imported function from inside a `jest.mock()` factory.
+ */
+export const mockUserSubjectsBootstrapData = (userSubjects: number[]) => {
+  const actual = jest.requireActual('src/utils/getBootstrapData');
+  const { DEFAULT_BOOTSTRAP_DATA } = jest.requireActual('src/constants');
+  return {
+    __esModule: true,
+    ...actual,
+    default: jest.fn(() => ({
+      ...DEFAULT_BOOTSTRAP_DATA,
+      common: {
+        ...DEFAULT_BOOTSTRAP_DATA.common,
+        user_subjects: userSubjects,
+      },
+    })),
+  };
+};

superset-frontend/src/dashboard/components/IconButton.test.tsx

@@ -0,0 +1,75 @@
+/**
+ * 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 { render, screen, fireEvent } from 'spec/helpers/testing-library';
+import IconButton from './IconButton';
+
+const icon = <span data-test="fake-icon">icon</span>;
+
+test('calls onClick when clicked and not disabled', () => {
+  const onClick = jest.fn();
+  render(<IconButton icon={icon} onClick={onClick} data-test="my-button" />);
+
+  fireEvent.click(screen.getByTestId('my-button'));
+
+  expect(onClick).toHaveBeenCalledTimes(1);
+  // aria-disabled is only rendered when the `disabled` prop is set
+  expect(screen.getByTestId('my-button')).not.toHaveAttribute(
+    'aria-disabled',
+    'true',
+  );
+});
+
+test('does not call onClick and sets aria-disabled when disabled', () => {
+  const onClick = jest.fn();
+  render(
+    <IconButton icon={icon} onClick={onClick} disabled data-test="my-button" />,
+  );
+
+  const button = screen.getByTestId('my-button');
+  fireEvent.click(button);
+
+  expect(onClick).not.toHaveBeenCalled();
+  expect(button).toHaveAttribute('aria-disabled', 'true');
+});
+
+test('does not prevent default keyboard navigation', () => {
+  const onKeyDown = jest.fn();
+  render(
+    <IconButton
+      icon={icon}
+      onClick={jest.fn()}
+      onKeyDown={onKeyDown}
+      data-test="my-button"
+    />,
+  );
+
+  const eventWasNotCancelled = fireEvent.keyDown(
+    screen.getByTestId('my-button'),
+    { key: 'Tab' },
+  );
+
+  expect(eventWasNotCancelled).toBe(true);
+  expect(onKeyDown).toHaveBeenCalledTimes(1);
+});
+
+test('renders the provided label', () => {
+  render(<IconButton icon={icon} onClick={jest.fn()} label="My Label" />);
+
+  expect(screen.getByText('My Label')).toBeInTheDocument();
+});

superset-frontend/src/dashboard/components/IconButton.tsx

@@ -16,41 +16,81 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { MouseEventHandler } from 'react';
-import { styled } from '@apache-superset/core/theme';
+import { forwardRef, HTMLAttributes, MouseEventHandler } from 'react';
+import { styled, SupersetTheme } from '@apache-superset/core/theme';
 
-interface IconButtonProps {
+interface IconButtonProps extends HTMLAttributes<HTMLDivElement> {
   icon: JSX.Element;
   label?: string;
   onClick: MouseEventHandler<HTMLDivElement>;
+  disabled?: boolean;
+  'data-test'?: string;
 }
 
-const StyledDiv = styled.div`
+const disabledCss = `
+  cursor: not-allowed;
+  opacity: 0.5;
+`;
+
+const activeCss = ({ theme }: { theme: SupersetTheme }) => `
+  &:hover {
+    color: ${theme.colorPrimary};
+    background: ${theme.colorBgTextHover};
+  }
+`;
+
+const StyledDiv = styled.div<{ isDisabled?: boolean }>`
   display: flex;
   align-items: center;
   cursor: pointer;
   color: ${({ theme }) => theme.colorIcon};
-  &:hover {
-    color: ${({ theme }) => theme.colorPrimary};
-  }
+  padding: ${({ theme }) => theme.paddingXXS}px;
+  border-radius: ${({ theme }) => theme.borderRadiusXS}px;
+
+  ${({ isDisabled, theme }) => (isDisabled ? disabledCss : activeCss({ theme }))}
 `;
 
 const StyledSpan = styled.span`
   margin-left: ${({ theme }) => theme.sizeUnit * 2}px;
 `;
 
-const IconButton = ({ icon, label, onClick }: IconButtonProps) => (
-  <StyledDiv
-    tabIndex={0}
-    role="button"
-    onClick={e => {
-      e.preventDefault();
-      onClick(e);
-    }}
-  >
-    {icon}
-    {label && <StyledSpan>{label}</StyledSpan>}
-  </StyledDiv>
+const IconButton = forwardRef<HTMLDivElement, IconButtonProps>(
+  (
+    {
+      icon,
+      label,
+      onClick,
+      onKeyDown,
+      disabled,
+      'data-test': dataTest,
+      ...rest
+    },
+    ref,
+  ) => (
+    <StyledDiv
+      {...rest}
+      ref={ref}
+      tabIndex={disabled ? -1 : 0}
+      role="button"
+      isDisabled={disabled}
+      aria-disabled={disabled}
+      data-test={dataTest}
+      onClick={e => {
+        e.preventDefault();
+        if (!disabled) {
+          onClick(e);
+        }
+      }}
+      onKeyDown={e => {
+        if (!disabled) {
+          onKeyDown?.(e);
+        }
+      }}
+    >
+      {icon}
+      {label && <StyledSpan>{label}</StyledSpan>}
+    </StyledDiv>
+  ),
 );
 
 export default IconButton;

superset-frontend/src/dashboard/util/permissionUtils.test.ts

@@ -29,6 +29,7 @@ import {
   canUserSaveAsDashboard,
   isUserAdmin,
   isUserDashboardEditor,
+  isUserEditorOrAdmin,
 } from './permissionUtils';
 
 const editorUser: UserWithPermissionsAndRoles = {
@@ -196,6 +197,32 @@ test('isUserAdmin returns false for non-admin user', () => {
   expect(isUserAdmin(editorUser)).toEqual(false);
 });
 
+describe('isUserEditorOrAdmin', () => {
+  test('returns true when the user is a subject editor', () => {
+    expect(isUserEditorOrAdmin(editorUser, [editorSubject])).toEqual(true);
+  });
+
+  test('returns true when the user is an admin, regardless of subject membership', () => {
+    const nonMatchingSubject: Subject = { id: 999, label: 'Other', type: 1 };
+    expect(isUserEditorOrAdmin(adminUser, [nonMatchingSubject])).toEqual(true);
+  });
+
+  test('returns false when the user is neither a subject editor nor an admin', () => {
+    const nonMatchingSubject: Subject = { id: 999, label: 'Other', type: 1 };
+    expect(isUserEditorOrAdmin(outsiderUser, [nonMatchingSubject])).toEqual(
+      false,
+    );
+  });
+
+  test('returns false when editors is empty', () => {
+    expect(isUserEditorOrAdmin(editorUser, [])).toEqual(false);
+  });
+
+  test('returns false when editors is omitted', () => {
+    expect(isUserEditorOrAdmin(outsiderUser)).toEqual(false);
+  });
+});
+
 test('userHasPermission always returns true for admin user', () => {
   arbitraryPermissions.forEach(permissionView => {
     expect(

superset-frontend/src/dashboard/util/permissionUtils.ts

@@ -22,6 +22,7 @@ import {
   UserWithPermissionsAndRoles,
 } from 'src/types/bootstrapTypes';
 import { Dashboard } from 'src/types/Dashboard';
+import Subject from 'src/types/Subject';
 import { findPermission } from 'src/utils/findPermission';
 import getBootstrapData from 'src/utils/getBootstrapData';
 
@@ -32,6 +33,11 @@ const ADMIN_ROLE_NAME = 'admin';
 const getUserSubjects = (): number[] =>
   getBootstrapData()?.common?.user_subjects ?? [];
 
+const isUserInEditors = (editors: Subject[] = []): boolean => {
+  const userSubjects = getUserSubjects();
+  return editors.some(editor => userSubjects.includes(editor.id));
+};
+
 export const isUserAdmin = (
   user?: UserWithPermissionsAndRoles | UndefinedUser,
 ) =>
@@ -40,12 +46,13 @@ export const isUserAdmin = (
     role => role.toLowerCase() === ADMIN_ROLE_NAME,
   );
 
-export const isUserDashboardEditor = (dashboard: Dashboard): boolean => {
-  const userSubjects = getUserSubjects();
-  return (
-    dashboard.editors?.some(editor => userSubjects.includes(editor.id)) ?? false
-  );
-};
+export const isUserEditorOrAdmin = (
+  user?: UserWithPermissionsAndRoles | UndefinedUser,
+  editors: Subject[] = [],
+): boolean => isUserInEditors(editors) || isUserAdmin(user);
+
+export const isUserDashboardEditor = (dashboard: Dashboard): boolean =>
+  isUserInEditors(dashboard.editors);
 
 export const canUserEditDashboard = (
   dashboard: Dashboard,