Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dashboard_rbac): manage roles for dashboard #13145

Merged
merged 7 commits into from
Mar 3, 2021
Merged

feat(dashboard_rbac): manage roles for dashboard #13145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c9fe83
feat(dashboard_rbac): manage roles for dashboard
simcha90 Feb 16, 2021
0de9e9a
Merge branch 'master' of github.com:apache/superset into role_permiss…
simcha90 Feb 16, 2021
2d5e231
test: fix tests
simcha90 Feb 16, 2021
723be53
Merge branch 'master' into role_permissions
amitmiran137 Feb 18, 2021
21ccbdd
Merge branch 'master' of github.com:apache/superset into role_permiss…
simcha90 Feb 22, 2021
5e72312
fix: fix empty roles
simcha90 Feb 22, 2021
26556fc
Merge branch 'role_permissions' of github.com:simchaNielsen/incubator…
simcha90 Feb 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const dashboardResult = {
slug: '/new',
json_metadata: '{"something":"foo"}',
owners: [],
roles: [],
},
},
};
Expand All @@ -54,6 +55,7 @@ fetchMock.get('glob:*/api/v1/dashboard/*', {
slug: '/new',
json_metadata: '{"something":"foo"}',
owners: [],
roles: [],
},
});

Expand Down Expand Up @@ -207,6 +209,7 @@ describe('PropertiesModal', () => {
slug: '/new',
json_metadata: '{"something":"foo"}',
owners: [{ id: 1, first_name: 'Al', last_name: 'Pacino' }],
roles: [],
},
},
});
Expand All @@ -219,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();
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/spec/javascripts/views/CRUD/dashboard/DashboardList_spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}));

Expand Down
163 changes: 131 additions & 32 deletions superset-frontend/src/dashboard/components/PropertiesModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 => ({
Expand All @@ -111,6 +112,7 @@ class PropertiesModal extends React.PureComponent {
dashboard_title: '',
slug: '',
owners: [],
roles: [],
json_metadata: '',
colorScheme: props.colorScheme,
},
Expand All @@ -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() {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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 owners = ownersValue?.map(o => o.value) ?? [];
const roles = rolesValue?.map(o => o.value) ?? [];
let metadataColorScheme;

// update color scheme to match metadata
Expand All @@ -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,
Expand All @@ -255,6 +277,7 @@ class PropertiesModal extends React.PureComponent {
jsonMetadata,
ownerIds: owners,
colorScheme: metadataColorScheme || colorScheme,
...moreProps,
});
this.props.onHide();
} else {
Expand All @@ -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,
Expand All @@ -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 (
<Row>
<Col md={6}>
<h3 style={{ marginTop: '1em' }}>{t('Access')}</h3>
<FormLabel htmlFor="owners">{t('Owners')}</FormLabel>
<AsyncSelect
name="owners"
isMulti
value={values.owners}
loadOptions={loadAccessOptions('owners')}
defaultOptions // load options on render
cacheOptions
onChange={this.onOwnersChange}
disabled={!isDashboardLoaded}
filterOption={null} // options are filtered at the api
/>
<p className="help-block">
{t(
'Owners is a list of users who can alter the dashboard. Searchable by name or username.',
)}
</p>
</Col>
<Col md={6}>
<h3 style={{ marginTop: '1em' }}>{t('Colors')}</h3>
<ColorSchemeControlWrapper
onChange={this.onColorSchemeChange}
colorScheme={values.colorScheme}
/>
</Col>
</Row>
);
}

getRowsWithRoles() {
Copy link
Member

@lilykuang lilykuang Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getRowsWithoutRoles and getRowsWithRoles are very similar. Maybe the two functions could combine into one and use DASHBOARD_RBAC feature flag to determine AsyncSelect with role show or hidden.

Copy link
Contributor Author

@simcha90 simcha90 Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it first... but except of add AsyncSelect it also require changes in layout, because of it I moved to separate functions only code that requires changes of layout

const { values, isDashboardLoaded } = this.state;
return (
<>
<Row>
<Col md={12}>
<h3 style={{ marginTop: '1em' }}>{t('Access')}</h3>
</Col>
</Row>
<Row>
<Col md={6}>
<FormLabel htmlFor="owners">{t('Owners')}</FormLabel>
<AsyncSelect
name="owners"
isMulti
value={values.owners}
loadOptions={loadAccessOptions('owners')}
defaultOptions // load options on render
cacheOptions
onChange={this.onOwnersChange}
disabled={!isDashboardLoaded}
filterOption={null} // options are filtered at the api
/>
<p className="help-block">
{t(
'Owners is a list of users who can alter the dashboard. Searchable by name or username.',
)}
</p>
</Col>
<Col md={6}>
<FormLabel htmlFor="roles">{t('Roles')}</FormLabel>
<AsyncSelect
name="roles"
isMulti
value={values.roles}
loadOptions={loadAccessOptions('roles')}
defaultOptions // load options on render
cacheOptions
onChange={this.onRolesChange}
disabled={!isDashboardLoaded}
filterOption={null} // options are filtered at the api
/>
<p className="help-block">
{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.',
)}
</p>
</Col>
</Row>
<Row>
<Col md={6}>
<ColorSchemeControlWrapper
onChange={this.onColorSchemeChange}
colorScheme={values.colorScheme}
/>
</Col>
</Row>
</>
);
}

render() {
const { values, isDashboardLoaded, isAdvancedOpen, errors } = this.state;
const { onHide, onlyApply } = this.props;
Expand Down Expand Up @@ -352,35 +477,9 @@ class PropertiesModal extends React.PureComponent {
</p>
</Col>
</Row>
<Row>
<Col md={6}>
<h3 style={{ marginTop: '1em' }}>{t('Access')}</h3>
<FormLabel htmlFor="owners">{t('Owners')}</FormLabel>
<AsyncSelect
name="owners"
isMulti
value={values.owners}
loadOptions={loadOwnerOptions}
defaultOptions // load options on render
cacheOptions
onChange={this.onOwnersChange}
disabled={!isDashboardLoaded}
filterOption={null} // options are filtered at the api
/>
<p className="help-block">
{t(
'Owners is a list of users who can alter the dashboard. Searchable by name or username.',
)}
</p>
</Col>
<Col md={6}>
<h3 style={{ marginTop: '1em' }}>{t('Colors')}</h3>
<ColorSchemeControlWrapper
onChange={this.onColorSchemeChange}
colorScheme={values.colorScheme}
/>
</Col>
</Row>
{isFeatureEnabled(FeatureFlag.DASHBOARD_RBAC)
? this.getRowsWithRoles()
: this.getRowsWithoutRoles()}
<Row>
<Col md={12}>
<h3 style={{ marginTop: '1em' }}>
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/featureFlags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
DASHBOARD_NATIVE_FILTERS_SET = 'DASHBOARD_NATIVE_FILTERS_SET',
VERSIONED_EXPORT = 'VERSIONED_EXPORT',
GLOBAL_ASYNC_QUERIES = 'GLOBAL_ASYNC_QUERIES',
Expand Down