From 2c9fe8350561c8b10429b483bf5662002d63ce25 Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Tue, 16 Feb 2021 08:51:09 +0200 Subject: [PATCH 1/3] feat(dashboard_rbac): manage roles for dashboard --- .../dashboard/components/PropertiesModal.jsx | 161 ++++++++++++++---- superset-frontend/src/featureFlags.ts | 1 + 2 files changed, 131 insertions(+), 31 deletions(-) diff --git a/superset-frontend/src/dashboard/components/PropertiesModal.jsx b/superset-frontend/src/dashboard/components/PropertiesModal.jsx index 411eda0faed80..9be97f8ac13ae 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal.jsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal.jsx @@ -38,6 +38,7 @@ import ColorSchemeControlWrapper from 'src/dashboard/components/ColorSchemeContr import { getClientErrorObject } from '../../utils/getClientErrorObject'; import withToasts from '../../messageToasts/enhancers/withToasts'; import '../stylesheets/buttons.less'; +import { FeatureFlag, isFeatureEnabled } from '../../featureFlags'; const StyledJsonEditor = styled(JsonEditor)` border-radius: ${({ theme }) => theme.borderRadius}px; @@ -85,10 +86,10 @@ const handleErrorResponse = async response => { }); }; -const loadOwnerOptions = (input = '') => { +const loadAccessOptions = accessType => (input = '') => { const query = rison.encode({ filter: input }); return SupersetClient.get({ - endpoint: `/api/v1/dashboard/related/owners?q=${query}`, + endpoint: `/api/v1/dashboard/related/${accessType}?q=${query}`, }).then( response => response.json.result.map(item => ({ @@ -111,6 +112,7 @@ class PropertiesModal extends React.PureComponent { dashboard_title: '', slug: '', owners: [], + roles: [], json_metadata: '', colorScheme: props.colorScheme, }, @@ -120,9 +122,12 @@ class PropertiesModal extends React.PureComponent { this.onChange = this.onChange.bind(this); this.onMetadataChange = this.onMetadataChange.bind(this); this.onOwnersChange = this.onOwnersChange.bind(this); + this.onRolesChange = this.onRolesChange.bind(this); this.submit = this.submit.bind(this); this.toggleAdvanced = this.toggleAdvanced.bind(this); this.onColorSchemeChange = this.onColorSchemeChange.bind(this); + this.getRowsWithRoles = this.getRowsWithRoles.bind(this); + this.getRowsWithoutRoles = this.getRowsWithoutRoles.bind(this); } componentDidMount() { @@ -163,6 +168,10 @@ class PropertiesModal extends React.PureComponent { this.updateFormState('owners', value); } + onRolesChange(value) { + this.updateFormState('roles', value); + } + onMetadataChange(metadata) { this.updateFormState('json_metadata', metadata); } @@ -202,7 +211,12 @@ class PropertiesModal extends React.PureComponent { value: owner.id, label: `${owner.first_name} ${owner.last_name}`, })); + const initialSelectedRoles = dashboard.roles.map(role => ({ + value: role.id, + label: `${role.name}`, + })); this.onOwnersChange(initialSelectedOwners); + this.onRolesChange(initialSelectedRoles); }, handleErrorResponse); } @@ -231,10 +245,12 @@ class PropertiesModal extends React.PureComponent { dashboard_title: dashboardTitle, colorScheme, owners: ownersValue, + roles: rolesValue, }, } = this.state; const { onlyApply } = this.props; const owners = ownersValue.map(o => o.value); + const roles = rolesValue.map(o => o.value); let metadataColorScheme; // update color scheme to match metadata @@ -247,6 +263,12 @@ class PropertiesModal extends React.PureComponent { } } + const moreProps = {}; + const morePutProps = {}; + if (isFeatureEnabled(FeatureFlag.DASHBOARD_RBAC)) { + moreProps.rolesIds = roles; + morePutProps.roles = roles; + } if (onlyApply) { this.props.onSubmit({ id: this.props.dashboardId, @@ -255,6 +277,7 @@ class PropertiesModal extends React.PureComponent { jsonMetadata, ownerIds: owners, colorScheme: metadataColorScheme || colorScheme, + ...moreProps, }); this.props.onHide(); } else { @@ -266,8 +289,13 @@ class PropertiesModal extends React.PureComponent { slug: slug || null, json_metadata: jsonMetadata || null, owners, + ...morePutProps, }), }).then(({ json: { result } }) => { + const moreResultProps = {}; + if (isFeatureEnabled(FeatureFlag.DASHBOARD_RBAC)) { + moreResultProps.rolesIds = result.roles; + } this.props.addSuccessToast(t('The dashboard has been saved')); this.props.onSubmit({ id: this.props.dashboardId, @@ -276,12 +304,109 @@ class PropertiesModal extends React.PureComponent { jsonMetadata: result.json_metadata, ownerIds: result.owners, colorScheme: metadataColorScheme || colorScheme, + ...moreResultProps, }); this.props.onHide(); }, handleErrorResponse); } } + getRowsWithoutRoles() { + const { values, isDashboardLoaded } = this.state; + return ( + + +

{t('Access')}

+ {t('Owners')} + +

+ {t( + 'Owners is a list of users who can alter the dashboard. Searchable by name or username.', + )} +

+ + +

{t('Colors')}

+ + + + ); + } + + getRowsWithRoles() { + const { values, isDashboardLoaded } = this.state; + return ( + <> + + +

{t('Access')}

+ + + + + {t('Owners')} + +

+ {t( + 'Owners is a list of users who can alter the dashboard. Searchable by name or username.', + )} +

+ + + {t('Roles')} + +

+ {t( + 'Roles is a list which defines access to the dashboard. These roles are always applied in addition to restrictions on dataset level access. If no roles defined then the dashboard is available to all roles.', + )} +

+ + + + + + + + + ); + } + render() { const { values, isDashboardLoaded, isAdvancedOpen, errors } = this.state; const { onHide, onlyApply } = this.props; @@ -352,35 +477,9 @@ class PropertiesModal extends React.PureComponent {

- - -

{t('Access')}

- {t('Owners')} - -

- {t( - 'Owners is a list of users who can alter the dashboard. Searchable by name or username.', - )} -

- - -

{t('Colors')}

- - - + {isFeatureEnabled(FeatureFlag.DASHBOARD_RBAC) + ? this.getRowsWithRoles() + : this.getRowsWithoutRoles()}

diff --git a/superset-frontend/src/featureFlags.ts b/superset-frontend/src/featureFlags.ts index c8b8acc20c893..a09fc08730173 100644 --- a/superset-frontend/src/featureFlags.ts +++ b/superset-frontend/src/featureFlags.ts @@ -36,6 +36,7 @@ export enum FeatureFlag { ESCAPE_MARKDOWN_HTML = 'ESCAPE_MARKDOWN_HTML', DASHBOARD_NATIVE_FILTERS = 'DASHBOARD_NATIVE_FILTERS', DASHBOARD_CROSS_FILTERS = 'DASHBOARD_CROSS_FILTERS', + DASHBOARD_RBAC = 'DASHBOARD_RBAC', VERSIONED_EXPORT = 'VERSIONED_EXPORT', GLOBAL_ASYNC_QUERIES = 'GLOBAL_ASYNC_QUERIES', ENABLE_TEMPLATE_PROCESSING = 'ENABLE_TEMPLATE_PROCESSING', From 2d5e23156367fdadb740bfe06ec2422244af7b88 Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Tue, 16 Feb 2021 10:02:01 +0200 Subject: [PATCH 2/3] test: fix tests --- .../javascripts/dashboard/components/PropertiesModal_spec.jsx | 3 +++ .../javascripts/views/CRUD/dashboard/DashboardList_spec.jsx | 1 + 2 files changed, 4 insertions(+) diff --git a/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx index 07dcf7d9a2634..42acdb409d957 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx @@ -38,6 +38,7 @@ const dashboardResult = { slug: '/new', json_metadata: '{"something":"foo"}', owners: [], + roles: [], }, }, }; @@ -54,6 +55,7 @@ fetchMock.get('glob:*/api/v1/dashboard/*', { slug: '/new', json_metadata: '{"something":"foo"}', owners: [], + roles: [], }, }); @@ -207,6 +209,7 @@ describe('PropertiesModal', () => { slug: '/new', json_metadata: '{"something":"foo"}', owners: [{ id: 1, first_name: 'Al', last_name: 'Pacino' }], + roles: [], }, }, }); diff --git a/superset-frontend/spec/javascripts/views/CRUD/dashboard/DashboardList_spec.jsx b/superset-frontend/spec/javascripts/views/CRUD/dashboard/DashboardList_spec.jsx index 2af4dc189945c..c8d26862494a0 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/dashboard/DashboardList_spec.jsx +++ b/superset-frontend/spec/javascripts/views/CRUD/dashboard/DashboardList_spec.jsx @@ -56,6 +56,7 @@ const mockDashboards = [...new Array(3)].map((_, i) => ({ changed_on_utc: new Date().toISOString(), changed_on_delta_humanized: '5 minutes ago', owners: [{ id: 1, first_name: 'admin', last_name: 'admin_user' }], + roles: [{ id: 1, name: 'adminUser' }], thumbnail_url: '/thumbnail', })); From 5e72312c75b78d4444c70f3d00e6f26227c83acf Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Mon, 22 Feb 2021 10:26:06 +0200 Subject: [PATCH 3/3] fix: fix empty roles --- .../components/PropertiesModal_spec.jsx | 21 +++++++++++++++++++ .../dashboard/components/PropertiesModal.jsx | 4 ++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx index 42acdb409d957..92f78876032be 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx @@ -222,6 +222,27 @@ describe('PropertiesModal', () => { ]); }); + it('should call onRolesChange', async () => { + const wrapper = setup(); + const modalInstance = wrapper.find('PropertiesModal').instance(); + const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { + result: { + dashboard_title: 'New Title', + slug: '/new', + json_metadata: '{"something":"foo"}', + owners: [], + roles: [{ id: 1, name: 'Alpha' }], + }, + }, + }); + const onRolwesSpy = jest.spyOn(modalInstance, 'onRolesChange'); + modalInstance.fetchDashboardDetails(); + await fetchSpy(); + expect(modalInstance.state.values.colorScheme).toBeUndefined(); + expect(onRolwesSpy).toHaveBeenCalledWith([{ value: 1, label: 'Alpha' }]); + }); + describe('when colorScheme is undefined as a prop', () => { describe('when color_scheme is defined in json_metadata', () => { const wrapper = setup(); diff --git a/superset-frontend/src/dashboard/components/PropertiesModal.jsx b/superset-frontend/src/dashboard/components/PropertiesModal.jsx index 9be97f8ac13ae..481893fb47926 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal.jsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal.jsx @@ -249,8 +249,8 @@ class PropertiesModal extends React.PureComponent { }, } = this.state; const { onlyApply } = this.props; - const owners = ownersValue.map(o => o.value); - const roles = rolesValue.map(o => o.value); + const owners = ownersValue?.map(o => o.value) ?? []; + const roles = rolesValue?.map(o => o.value) ?? []; let metadataColorScheme; // update color scheme to match metadata