From e11d479c39b277406f89a16f11fd39c8a193e9be Mon Sep 17 00:00:00 2001 From: Gabriel Dutra Date: Wed, 27 Mar 2019 16:29:48 -0300 Subject: [PATCH] Manage user groups in UserEdit (#3450) --- client/app/assets/less/ant.less | 1 + .../components/dynamic-form/DynamicForm.jsx | 24 +++++ client/app/components/proptypes.js | 22 ++++- client/app/components/users/UserEdit.jsx | 52 ++++++++++- client/app/components/users/UserShow.jsx | 88 +++++++++++++------ client/app/components/users/UserShow.test.js | 6 ++ .../users/__snapshots__/UserShow.test.js.snap | 6 ++ client/app/services/group.js | 2 +- client/app/services/user.js | 1 + .../integration/user/edit_profile_spec.js | 2 + redash/handlers/users.py | 17 +++- tests/handlers/test_users.py | 9 ++ 12 files changed, 194 insertions(+), 36 deletions(-) diff --git a/client/app/assets/less/ant.less b/client/app/assets/less/ant.less index 337bc1d744..3d67ce8ffc 100644 --- a/client/app/assets/less/ant.less +++ b/client/app/assets/less/ant.less @@ -20,6 +20,7 @@ @import '~antd/lib/tag/style/index'; @import '~antd/lib/grid/style/index'; @import '~antd/lib/switch/style/index'; +@import '~antd/lib/empty/style/index'; @import '~antd/lib/drawer/style/index'; @import '~antd/lib/divider/style/index'; @import '~antd/lib/dropdown/style/index'; diff --git a/client/app/components/dynamic-form/DynamicForm.jsx b/client/app/components/dynamic-form/DynamicForm.jsx index b4be5c407e..8bfe38bdad 100644 --- a/client/app/components/dynamic-form/DynamicForm.jsx +++ b/client/app/components/dynamic-form/DynamicForm.jsx @@ -7,6 +7,7 @@ import Checkbox from 'antd/lib/checkbox'; import Button from 'antd/lib/button'; import Upload from 'antd/lib/upload'; import Icon from 'antd/lib/icon'; +import Select from 'antd/lib/select'; import notification from '@/services/notification'; import { includes } from 'lodash'; import { react2angular } from 'react2angular'; @@ -132,6 +133,25 @@ export const DynamicForm = Form.create()(class DynamicForm extends React.Compone return getFieldDecorator(name, fileOptions)(upload); } + renderSelect(field, props) { + const { getFieldDecorator } = this.props.form; + const { name, options, mode, initialValue, readOnly, loading } = field; + const { Option } = Select; + + const decoratorOptions = { + rules: fieldRules(field), + initialValue, + }; + + return getFieldDecorator(name, decoratorOptions)( + , + ); + } + renderField(field, props) { const { getFieldDecorator } = this.props.form; const { name, type, initialValue } = field; @@ -147,6 +167,10 @@ export const DynamicForm = Form.create()(class DynamicForm extends React.Compone return getFieldDecorator(name, options)({fieldLabel}); } else if (type === 'file') { return this.renderUpload(field, props); + } else if (type === 'select') { + return this.renderSelect(field, props); + } else if (type === 'content') { + return field.content; } else if (type === 'number') { return getFieldDecorator(name, options)(); } diff --git a/client/app/components/proptypes.js b/client/app/components/proptypes.js index 7567727fac..43f368cadf 100644 --- a/client/app/components/proptypes.js +++ b/client/app/components/proptypes.js @@ -34,12 +34,30 @@ export const RefreshScheduleDefault = { export const Field = PropTypes.shape({ name: PropTypes.string.isRequired, title: PropTypes.string, - type: PropTypes.oneOf(['text', 'email', 'password', 'number', 'checkbox', 'file']).isRequired, - initialValue: PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool]), + type: PropTypes.oneOf([ + 'text', + 'email', + 'password', + 'number', + 'checkbox', + 'file', + 'select', + 'content', + ]).isRequired, + initialValue: PropTypes.oneOfType([ + PropTypes.string, + PropTypes.number, + PropTypes.bool, + PropTypes.arrayOf(PropTypes.string), + PropTypes.arrayOf(PropTypes.number), + ]), + content: PropTypes.node, + mode: PropTypes.string, required: PropTypes.bool, readOnly: PropTypes.bool, minLength: PropTypes.number, placeholder: PropTypes.string, + loading: PropTypes.bool, props: PropTypes.object, // eslint-disable-line react/forbid-prop-types }); diff --git a/client/app/components/users/UserEdit.jsx b/client/app/components/users/UserEdit.jsx index 190cc15ab4..4b3bcd50c9 100644 --- a/client/app/components/users/UserEdit.jsx +++ b/client/app/components/users/UserEdit.jsx @@ -1,9 +1,12 @@ import React, { Fragment } from 'react'; +import { includes } from 'lodash'; import Alert from 'antd/lib/alert'; import Button from 'antd/lib/button'; import Form from 'antd/lib/form'; import Modal from 'antd/lib/modal'; +import Tag from 'antd/lib/tag'; import { User } from '@/services/user'; +import { Group } from '@/services/group'; import { currentUser } from '@/services/auth'; import { absoluteUrl } from '@/services/utils'; import { UserProfile } from '../proptypes'; @@ -20,6 +23,8 @@ export default class UserEdit extends React.Component { super(props); this.state = { user: this.props.user, + groups: [], + loadingGroups: true, regeneratingApiKey: false, sendingPasswordEmail: false, resendingInvitation: false, @@ -27,6 +32,15 @@ export default class UserEdit extends React.Component { }; } + componentDidMount() { + Group.query((groups) => { + this.setState({ + groups: groups.map(({ id, name }) => ({ value: id, title: name })), + loadingGroups: false, + }); + }); + } + changePassword = () => { ChangePasswordDialog.showModal({ user: this.props.user }); }; @@ -102,8 +116,9 @@ export default class UserEdit extends React.Component { }); }; - renderBasicInfoForm() { - const { user } = this.state; + renderUserInfoForm() { + const { user, groups, loadingGroups } = this.state; + const formFields = [ { name: 'name', @@ -117,7 +132,22 @@ export default class UserEdit extends React.Component { type: 'email', initialValue: user.email, }, - ].map(field => ({ ...field, readOnly: user.isDisabled, required: true })); + (!user.isDisabled && currentUser.id !== user.id) ? { + name: 'group_ids', + title: 'Groups', + type: 'select', + mode: 'multiple', + options: groups, + initialValue: groups.filter(group => includes(user.groupIds, group.value)).map(group => group.value), + loading: loadingGroups, + placeholder: loadingGroups ? 'Loading...' : '', + } : { + name: 'group_ids', + title: 'Groups', + type: 'content', + content: this.renderUserGroups(), + }, + ].map(field => ({ readOnly: user.isDisabled, required: true, ...field })); return ( + {groups.filter(group => includes(user.groupIds, group.value)).map((group => ( + + {group.title} + + )))} + + ); + } + renderApiKey() { const { user, regeneratingApiKey } = this.state; @@ -227,7 +271,7 @@ export default class UserEdit extends React.Component { />

{user.name}

- {this.renderBasicInfoForm()} + {this.renderUserInfoForm()} {!user.isDisabled && ( {this.renderApiKey()} diff --git a/client/app/components/users/UserShow.jsx b/client/app/components/users/UserShow.jsx index 87de7f5d14..8e41ff3770 100644 --- a/client/app/components/users/UserShow.jsx +++ b/client/app/components/users/UserShow.jsx @@ -1,30 +1,66 @@ import React from 'react'; +import { includes } from 'lodash'; +import Tag from 'antd/lib/tag'; +import { Group } from '@/services/group'; import { UserProfile } from '../proptypes'; -export default function UserShow({ user: { name, email, profileImageUrl } }) { - return ( -
- profile - -

{name}

- -
- -
-
Name:
-
{name}
-
Email:
-
{email}
-
-
- ); -} +export default class UserShow extends React.Component { + static propTypes = { + user: UserProfile.isRequired, + }; + + constructor(props) { + super(props); + this.state = { groups: [], loadingGroups: true }; + } + + componentDidMount() { + Group.query((groups) => { + this.setState({ groups, loadingGroups: false }); + }); + } + + renderUserGroups() { + const { groupIds } = this.props.user; + const { groups } = this.state; + + return ( +
+ {groups.filter(group => includes(groupIds, group.id)).map((group => ( + + {group.name} + + )))} +
+ ); + } -UserShow.propTypes = { - user: UserProfile.isRequired, -}; + render() { + const { name, email, profileImageUrl } = this.props.user; + const { loadingGroups } = this.state; + + return ( +
+ profile + +

{name}

+ +
+ +
+
Name:
+
{name}
+
Email:
+
{email}
+
Groups:
+
{loadingGroups ? 'Loading...' : this.renderUserGroups()}
+
+
+ ); + } +} diff --git a/client/app/components/users/UserShow.test.js b/client/app/components/users/UserShow.test.js index 7fd859e8a0..2badc694f3 100644 --- a/client/app/components/users/UserShow.test.js +++ b/client/app/components/users/UserShow.test.js @@ -1,12 +1,18 @@ import React from 'react'; import renderer from 'react-test-renderer'; +import { Group } from '@/services/group'; import UserShow from './UserShow'; +beforeEach(() => { + Group.query = jest.fn(dataCallback => dataCallback([])); +}); + test('renders correctly', () => { const user = { id: 2, name: 'John Doe', email: 'john@doe.com', + groupIds: [], profileImageUrl: 'http://www.images.com/llama.jpg', }; diff --git a/client/app/components/users/__snapshots__/UserShow.test.js.snap b/client/app/components/users/__snapshots__/UserShow.test.js.snap index 4455d79d84..20debdd5cf 100644 --- a/client/app/components/users/__snapshots__/UserShow.test.js.snap +++ b/client/app/components/users/__snapshots__/UserShow.test.js.snap @@ -31,6 +31,12 @@ exports[`renders correctly 1`] = `
john@doe.com
+
+ Groups: +
+
+
+
`; diff --git a/client/app/services/group.js b/client/app/services/group.js index e10aca9ef0..db12bc53cd 100644 --- a/client/app/services/group.js +++ b/client/app/services/group.js @@ -1,4 +1,4 @@ -export let Group = null; // eslint-disable-line import/no-mutable-exports +export let Group = {}; // eslint-disable-line import/no-mutable-exports function GroupService($resource) { const actions = { diff --git a/client/app/services/user.js b/client/app/services/user.js index 50b21899a5..2e3407dd5c 100644 --- a/client/app/services/user.js +++ b/client/app/services/user.js @@ -66,6 +66,7 @@ function convertUserInfo(user) { email: user.email, profileImageUrl: user.profile_image_url, apiKey: user.api_key, + groupIds: user.groups, isDisabled: user.is_disabled, isInvitationPending: user.is_invitation_pending, }; diff --git a/client/cypress/integration/user/edit_profile_spec.js b/client/cypress/integration/user/edit_profile_spec.js index 91e5236d60..f93ff27bf8 100644 --- a/client/cypress/integration/user/edit_profile_spec.js +++ b/client/cypress/integration/user/edit_profile_spec.js @@ -41,6 +41,8 @@ describe('Edit Profile', () => { $apiKey.val('secret'); }); + cy.getByTestId('Groups').should('contain', 'admin'); + cy.percySnapshot('User Profile'); }); diff --git a/redash/handlers/users.py b/redash/handlers/users.py index 7a57e0a99f..8549818828 100644 --- a/redash/handlers/users.py +++ b/redash/handlers/users.py @@ -4,6 +4,7 @@ from flask_restful import abort from flask_login import current_user, login_user from funcy import project +from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.exc import IntegrityError from disposable_email_domains import blacklist from funcy import partial @@ -207,7 +208,7 @@ def post(self, user_id): req = request.get_json(True) - params = project(req, ('email', 'name', 'password', 'old_password', 'groups')) + params = project(req, ('email', 'name', 'password', 'old_password', 'group_ids')) if 'password' in params and 'old_password' not in params: abort(403, message="Must provide current password to update password.") @@ -219,8 +220,18 @@ def post(self, user_id): user.hash_password(params.pop('password')) params.pop('old_password') - if 'groups' in params and not self.current_user.has_permission('admin'): - abort(403, message="Must be admin to change groups membership.") + if 'group_ids' in params: + if not self.current_user.has_permission('admin'): + abort(403, message="Must be admin to change groups membership.") + + for group_id in params['group_ids']: + try: + models.Group.get_by_id_and_org(group_id, self.current_org) + except NoResultFound: + abort(400, message="Group id {} is invalid.".format(group_id)) + + if len(params['group_ids']) == 0: + params.pop('group_ids') if 'email' in params: _, domain = params['email'].split('@', 1) diff --git a/tests/handlers/test_users.py b/tests/handlers/test_users.py index 7f4d55e0d8..c5cb375220 100644 --- a/tests/handlers/test_users.py +++ b/tests/handlers/test_users.py @@ -307,6 +307,15 @@ def test_changing_email_does_not_end_current_session(self): # make sure the session's `user_id` has changed to reflect the new identity, thus not logging the user out self.assertNotEquals(previous, current) + def test_admin_can_change_user_groups(self): + admin_user = self.factory.create_admin() + other_user = self.factory.create_user(group_ids=[1]) + + rv = self.make_request('post', "/api/users/{}".format(other_user.id), data={"group_ids": [1, 2]}, user=admin_user) + + self.assertEqual(rv.status_code, 200) + self.assertEqual(models.User.query.get(other_user.id).group_ids, [1,2]) + def test_admin_can_delete_user(self): admin_user = self.factory.create_admin() other_user = self.factory.create_user(is_invitation_pending=True)