fix(dashboard): check edit permissions correctly on frontend (#14626)

* fix(dashboard): check edit permissions correctly on frontend

* fix types, appease linter

* handle nulls better

superset-frontend/spec/javascripts/profile/fixtures.tsx

@@ -1,3 +1,5 @@
+import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
+
 /**
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
@@ -16,7 +18,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-export const user = {
+export const user: UserWithPermissionsAndRoles = {
   username: 'alpha',
   roles: {
     Alpha: [

superset-frontend/src/common/hooks/apiResources/dashboards.ts

@@ -25,8 +25,9 @@ export const useDashboard = (idOrSlug: string | number) =>
     useApiV1Resource<Dashboard>(`/api/v1/dashboard/${idOrSlug}`),
     dashboard => ({
       ...dashboard,
-      metadata: JSON.parse(dashboard.json_metadata),
-      position_data: JSON.parse(dashboard.position_json),
+      metadata: dashboard.json_metadata && JSON.parse(dashboard.json_metadata),
+      position_data:
+        dashboard.position_json && JSON.parse(dashboard.position_json),
     }),
   );
 

superset-frontend/src/dashboard/actions/hydrate.js

@@ -30,7 +30,9 @@ import { getInitialState as getInitialNativeFilterState } from 'src/dashboard/re
 import { getParam } from 'src/modules/utils';
 import { applyDefaultFormData } from 'src/explore/store';
 import { buildActiveFilters } from 'src/dashboard/util/activeDashboardFilters';
-import findPermission from 'src/dashboard/util/findPermission';
+import findPermission, {
+  canUserEditDashboard,
+} from 'src/dashboard/util/findPermission';
 import {
   DASHBOARD_FILTER_SCOPE_GLOBAL,
   dashboardFilter,
@@ -319,7 +321,8 @@ export const hydrateDashboard = (dashboardData, chartData, datasourcesData) => (
     });
   }
 
-  const { roles } = getState().user;
+  const { roles } = user;
+  const canEdit = canUserEditDashboard(dashboardData, user);
 
   return dispatch({
     type: HYDRATE_DASHBOARD,
@@ -332,7 +335,7 @@ export const hydrateDashboard = (dashboardData, chartData, datasourcesData) => (
         ...dashboardData,
         metadata,
         userId: String(user.userId), // legacy, please use state.user instead
-        dash_edit_perm: findPermission('can_write', 'Dashboard', roles),
+        dash_edit_perm: canEdit,
         dash_save_perm: findPermission('can_save_dash', 'Superset', roles),
         dash_share_perm: findPermission(
           'can_share_dashboard',
@@ -368,7 +371,7 @@ export const hydrateDashboard = (dashboardData, chartData, datasourcesData) => (
         css: dashboardData.css || '',
         colorNamespace: metadata?.color_namespace || null,
         colorScheme: metadata?.color_scheme || null,
-        editMode: findPermission('can_write', 'Dashboard', roles) && editMode,
+        editMode: canEdit && editMode,
         isPublished: dashboardData.published,
         hasUnsavedChanges: false,
         maxUndoHistoryExceeded: false,

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

@@ -16,44 +16,128 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import findPermission from './findPermission';
+import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
+import Dashboard from 'src/types/Dashboard';
+import Owner from 'src/types/Owner';
+import findPermission, { canUserEditDashboard } from './findPermission';
 
-test('findPermission for single role', () => {
-  expect(findPermission('abc', 'def', { role: [['abc', 'def']] })).toEqual(
-    true,
-  );
+describe('findPermission', () => {
+  it('findPermission for single role', () => {
+    expect(findPermission('abc', 'def', { role: [['abc', 'def']] })).toEqual(
+      true,
+    );
 
-  expect(findPermission('abc', 'def', { role: [['abc', 'de']] })).toEqual(
-    false,
-  );
+    expect(findPermission('abc', 'def', { role: [['abc', 'de']] })).toEqual(
+      false,
+    );
 
-  expect(findPermission('abc', 'def', { role: [] })).toEqual(false);
+    expect(findPermission('abc', 'def', { role: [] })).toEqual(false);
+  });
+
+  it('findPermission for multiple roles', () => {
+    expect(
+      findPermission('abc', 'def', {
+        role1: [
+          ['ccc', 'aaa'],
+          ['abc', 'def'],
+        ],
+        role2: [['abc', 'def']],
+      }),
+    ).toEqual(true);
+
+    expect(
+      findPermission('abc', 'def', {
+        role1: [['abc', 'def']],
+        role2: [['abc', 'dd']],
+      }),
+    ).toEqual(true);
+
+    expect(
+      findPermission('abc', 'def', {
+        role1: [['ccc', 'aaa']],
+        role2: [['aaa', 'ddd']],
+      }),
+    ).toEqual(false);
+
+    expect(findPermission('abc', 'def', { role1: [], role2: [] })).toEqual(
+      false,
+    );
+  });
+
+  it('handles nonexistent roles', () => {
+    expect(findPermission('abc', 'def', null)).toEqual(false);
+  });
 });
 
-test('findPermission for multiple roles', () => {
-  expect(
-    findPermission('abc', 'def', {
-      role1: [
-        ['ccc', 'aaa'],
-        ['abc', 'def'],
-      ],
-      role2: [['abc', 'def']],
-    }),
-  ).toEqual(true);
+describe('canUserEditDashboard', () => {
+  const ownerUser: UserWithPermissionsAndRoles = {
+    createdOn: '2021-05-12T16:56:22.116839',
+    email: 'user@example.com',
+    firstName: 'Test',
+    isActive: true,
+    lastName: 'User',
+    userId: 1,
+    username: 'owner',
+    permissions: {},
+    roles: { Alpha: [['can_write', 'Dashboard']] },
+  };
 
-  expect(
-    findPermission('abc', 'def', {
-      role1: [['abc', 'def']],
-      role2: [['abc', 'dd']],
-    }),
-  ).toEqual(true);
+  const adminUser: UserWithPermissionsAndRoles = {
+    ...ownerUser,
+    roles: {
+      ...ownerUser.roles,
+      Admin: [['can_write', 'Dashboard']],
+    },
+    userId: 2,
+    username: 'admin',
+  };
 
-  expect(
-    findPermission('abc', 'def', {
-      role1: [['ccc', 'aaa']],
-      role2: [['aaa', 'ddd']],
-    }),
-  ).toEqual(false);
+  const outsiderUser: UserWithPermissionsAndRoles = {
+    ...ownerUser,
+    userId: 3,
+    username: 'outsider',
+  };
 
-  expect(findPermission('abc', 'def', { role1: [], role2: [] })).toEqual(false);
+  const owner: Owner = {
+    first_name: 'Test',
+    id: ownerUser.userId,
+    last_name: 'User',
+    username: ownerUser.username,
+  };
+
+  const dashboard: Dashboard = {
+    id: 1,
+    dashboard_title: 'Test Dash',
+    url: 'https://dashboard.example.com/1',
+    thumbnail_url: 'https://dashboard.example.com/1/thumbnail.png',
+    published: true,
+    css: null,
+    changed_by_name: 'Test User',
+    changed_by: owner,
+    changed_on: '2021-05-12T16:56:22.116839',
+    charts: [],
+    owners: [owner],
+    roles: [],
+  };
+
+  it('allows owners to edit', () => {
+    expect(canUserEditDashboard(dashboard, ownerUser)).toEqual(true);
+  });
+  it('allows admin users to edit regardless of ownership', () => {
+    expect(canUserEditDashboard(dashboard, adminUser)).toEqual(true);
+  });
+  it('rejects non-owners', () => {
+    expect(canUserEditDashboard(dashboard, outsiderUser)).toEqual(false);
+  });
+  it('rejects nonexistent users', () => {
+    expect(canUserEditDashboard(dashboard, null)).toEqual(false);
+  });
+  it('rejects "admins" if the admin role does not have edit rights for some reason', () => {
+    expect(
+      canUserEditDashboard(dashboard, {
+        ...adminUser,
+        roles: { Admin: [] },
+      }),
+    ).toEqual(false);
+  });
 });

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

@@ -17,14 +17,37 @@
  * under the License.
  */
 import memoizeOne from 'memoize-one';
+import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
+import Dashboard from 'src/types/Dashboard';
 
 type UserRoles = Record<string, [string, string][]>;
 
 const findPermission = memoizeOne(
-  (perm: string, view: string, roles: UserRoles) =>
+  (perm: string, view: string, roles?: UserRoles | null) =>
+    !!roles &&
     Object.values(roles).some(permissions =>
       permissions.some(([perm_, view_]) => perm_ === perm && view_ === view),
     ),
 );
 
 export default findPermission;
+
+// this should really be a config value,
+// but is hardcoded in backend logic already, so...
+const ADMIN_ROLE_NAME = 'admin';
+
+const isUserAdmin = (user: UserWithPermissionsAndRoles) =>
+  Object.keys(user.roles).some(role => role.toLowerCase() === ADMIN_ROLE_NAME);
+
+const isUserDashboardOwner = (
+  dashboard: Dashboard,
+  user: UserWithPermissionsAndRoles,
+) => dashboard.owners.some(owner => owner.username === user.username);
+
+export const canUserEditDashboard = (
+  dashboard: Dashboard,
+  user?: UserWithPermissionsAndRoles | null,
+) =>
+  !!user &&
+  (isUserAdmin(user) || isUserDashboardOwner(dashboard, user)) &&
+  findPermission('can_write', 'Dashboard', user.roles);

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.test.tsx

@@ -17,13 +17,13 @@
  * under the License.
  */
 import React from 'react';
+import { GenericDataType } from '@superset-ui/core';
 import { render, screen } from 'spec/helpers/testing-library';
 import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetric';
 import AdhocFilter, {
   EXPRESSION_TYPES,
 } from 'src/explore/components/controls/FilterControl/AdhocFilter';
 import { DndFilterSelect } from 'src/explore/components/controls/DndColumnSelectControl/DndFilterSelect';
-import { GenericDataType } from '@superset-ui/core';
 
 const defaultProps = {
   name: 'Filter',

superset-frontend/src/types/Dashboard.ts

@@ -21,14 +21,14 @@ import Role from './Role';
 
 type Dashboard = {
   id: number;
-  slug: string;
+  slug?: string | null;
   url: string;
   dashboard_title: string;
   thumbnail_url: string;
   published: boolean;
-  css: string;
-  json_metadata: string;
-  position_json: string;
+  css?: string | null;
+  json_metadata?: string | null;
+  position_json?: string | null;
   changed_by_name: string;
   changed_by: Owner;
   changed_on: string;

superset-frontend/src/types/bootstrapTypes.ts

@@ -33,7 +33,7 @@ export interface UserWithPermissionsAndRoles extends User {
     database_access?: string[];
     datasource_access?: string[];
   };
-  roles: Record<string, string[][]>;
+  roles: Record<string, [string, string][]>;
 }
 
 export type Dashboard = {