diff --git a/superset-frontend/spec/javascripts/profile/fixtures.tsx b/superset-frontend/spec/javascripts/profile/fixtures.tsx index cd434248cbb..90e97da22c7 100644 --- a/superset-frontend/spec/javascripts/profile/fixtures.tsx +++ b/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: [ diff --git a/superset-frontend/src/common/hooks/apiResources/dashboards.ts b/superset-frontend/src/common/hooks/apiResources/dashboards.ts index 0bb21f16bfb..553049aac6a 100644 --- a/superset-frontend/src/common/hooks/apiResources/dashboards.ts +++ b/superset-frontend/src/common/hooks/apiResources/dashboards.ts @@ -25,8 +25,9 @@ export const useDashboard = (idOrSlug: string | number) => useApiV1Resource(`/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), }), ); diff --git a/superset-frontend/src/dashboard/actions/hydrate.js b/superset-frontend/src/dashboard/actions/hydrate.js index 4f1bd4cdc8f..2928df65737 100644 --- a/superset-frontend/src/dashboard/actions/hydrate.js +++ b/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, diff --git a/superset-frontend/src/dashboard/util/findPermission.test.ts b/superset-frontend/src/dashboard/util/findPermission.test.ts index 0dda552c2a1..1fbb791e3dd 100644 --- a/superset-frontend/src/dashboard/util/findPermission.test.ts +++ b/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); + }); }); diff --git a/superset-frontend/src/dashboard/util/findPermission.ts b/superset-frontend/src/dashboard/util/findPermission.ts index 613d1899f31..995c5d79674 100644 --- a/superset-frontend/src/dashboard/util/findPermission.ts +++ b/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; 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); diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.test.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.test.tsx index ae88ff2aa6a..bf7d135b788 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.test.tsx +++ b/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', diff --git a/superset-frontend/src/types/Dashboard.ts b/superset-frontend/src/types/Dashboard.ts index 9608cc1d08b..14de6d110c4 100644 --- a/superset-frontend/src/types/Dashboard.ts +++ b/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; diff --git a/superset-frontend/src/types/bootstrapTypes.ts b/superset-frontend/src/types/bootstrapTypes.ts index 7ef7caaab48..15e7eba0a03 100644 --- a/superset-frontend/src/types/bootstrapTypes.ts +++ b/superset-frontend/src/types/bootstrapTypes.ts @@ -33,7 +33,7 @@ export interface UserWithPermissionsAndRoles extends User { database_access?: string[]; datasource_access?: string[]; }; - roles: Record; + roles: Record; } export type Dashboard = {