This is an automated email from the ASF dual-hosted git repository. suddjian pushed a commit to branch fix-dash-edit-permission in repository https://gitbox.apache.org/repos/asf/superset.git
commit 87b5c62452c69e1a76f3c6f1c88fcc92b5e9829f Author: David Aaron Suddjian <[email protected]> AuthorDate: Thu May 13 12:17:12 2021 -0700 fix(dashboard): check edit permissions correctly on frontend --- superset-frontend/src/dashboard/actions/hydrate.js | 11 +- .../src/dashboard/util/findPermission.test.ts | 150 ++++++++++++++++----- .../src/dashboard/util/findPermission.ts | 25 ++++ superset-frontend/src/types/Dashboard.ts | 8 +- superset-frontend/src/types/bootstrapTypes.ts | 2 +- 5 files changed, 152 insertions(+), 44 deletions(-) diff --git a/superset-frontend/src/dashboard/actions/hydrate.js b/superset-frontend/src/dashboard/actions/hydrate.js index 4f1bd4c..2928df6 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 0dda552..cf52c5b 100644 --- a/superset-frontend/src/dashboard/util/findPermission.test.ts +++ b/superset-frontend/src/dashboard/util/findPermission.test.ts @@ -16,44 +16,124 @@ * 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', () => { + test('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); + }); + + test('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, + ); + }); }); -test('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); +describe('canUserEditDashboard', () => { + const ownerUser: UserWithPermissionsAndRoles = { + createdOn: '2021-05-12T16:56:22.116839', + email: '[email protected]', + firstName: 'Test', + isActive: true, + lastName: 'User', + userId: 1, + username: 'owner', + permissions: {}, + roles: { Alpha: [['can_write', 'Dashboard']] }, + }; + + const adminUser: UserWithPermissionsAndRoles = { + ...ownerUser, + roles: { + ...ownerUser.roles, + Admin: [['can_write', 'Dashboard']], + }, + userId: 2, + username: 'admin', + }; + + const outsiderUser: UserWithPermissionsAndRoles = { + ...ownerUser, + userId: 3, + username: 'outsider', + }; + + 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 613d189..2cfc69c 100644 --- a/superset-frontend/src/dashboard/util/findPermission.ts +++ b/superset-frontend/src/dashboard/util/findPermission.ts @@ -17,6 +17,8 @@ * 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][]>; @@ -28,3 +30,26 @@ const findPermission = memoizeOne( ); 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, +) => { + return ( + !!user && + (isUserAdmin(user) || isUserDashboardOwner(dashboard, user)) && + findPermission('can_write', 'Dashboard', user.roles) + ); +}; diff --git a/superset-frontend/src/types/Dashboard.ts b/superset-frontend/src/types/Dashboard.ts index 9608cc1..14de6d1 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 7ef7caa..15e7eba 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<string, string[][]>; + roles: Record<string, [string, string][]>; } export type Dashboard = {
