From 9fa60672e6766c836e306c6141cf9cc2dfb36f29 Mon Sep 17 00:00:00 2001 From: sahil839 Date: Fri, 29 May 2020 19:42:09 +0530 Subject: [PATCH] users: Modify update user API endpoint to accept role as parameter. This commit changes the update user API endpoint to accept role as parameter instead of the bool parameters is_guest and is_admin. User role dropdown in user info modal is also modified to use "dropdown_options_widget". Modified by tabbott to document the API change. --- static/js/settings_config.js | 15 +++++++++++++ static/js/settings_users.js | 21 +++++++++++++----- static/templates/admin_human_form.hbs | 6 ++--- templates/zerver/api/changelog.md | 3 +++ zerver/models.py | 6 +++++ zerver/openapi/zulip.yaml | 27 ++++++++++++---------- zerver/tests/test_users.py | 32 ++++++--------------------- zerver/views/users.py | 31 +++++--------------------- 8 files changed, 69 insertions(+), 72 deletions(-) diff --git a/static/js/settings_config.js b/static/js/settings_config.js index 54b88a3bac..0a7285b204 100644 --- a/static/js/settings_config.js +++ b/static/js/settings_config.js @@ -158,6 +158,21 @@ exports.msg_delete_limit_dropdown_values = time_limit_dropdown_values; exports.retain_message_forever = -1; +exports.user_role_values = { + guest: { + code: 600, + description: i18n.t("Guest"), + }, + member: { + code: 400, + description: i18n.t("Member"), + }, + admin: { + code: 200, + description: i18n.t("Administrator"), + }, +}; + // NOTIFICATIONS exports.general_notifications_table_labels = { diff --git a/static/js/settings_users.js b/static/js/settings_users.js index 0cfeff43ad..9e992d6b10 100644 --- a/static/js/settings_users.js +++ b/static/js/settings_users.js @@ -1,4 +1,5 @@ const settings_data = require("./settings_data"); +const settings_config = require("./settings_config"); const render_admin_user_list = require("../templates/admin_user_list.hbs"); const render_admin_human_form = require('../templates/admin_human_form.hbs'); const render_admin_bot_form = require('../templates/admin_bot_form.hbs'); @@ -66,6 +67,16 @@ function get_user_info_row(user_id) { return $("tr.user_row[data-user-id='" + user_id + "']"); } +function set_user_role_dropdown(person) { + let role_value = settings_config.user_role_values.member.code; + if (person.is_admin) { + role_value = settings_config.user_role_values.admin.code; + } else if (person.is_guest) { + role_value = settings_config.user_role_values.guest.code; + } + $('#user-role-select').val(role_value); +} + function update_view_on_deactivate(row) { const button = row.find("button.deactivate"); const user_role = row.find(".user_role"); @@ -377,14 +388,13 @@ function open_human_form(person) { user_id: user_id, email: person.email, full_name: person.full_name, - is_admin: person.is_admin, - is_guest: person.is_guest, - is_member: !person.is_admin && !person.is_guest, + user_role_values: settings_config.user_role_values, }); const div = $(html); const modal_container = $('#user-info-form-modal-container'); modal_container.empty().append(div); overlays.open_modal('#admin-human-form'); + set_user_role_dropdown(person); const element = "#admin-human-form .custom-profile-field-form"; $(element).html(""); @@ -587,15 +597,14 @@ function handle_human_form(tbody, status_field) { e.preventDefault(); e.stopPropagation(); - const user_role_select_value = modal.find('#user-role-select').val(); + const role = parseInt(modal.find('#user-role-select').val().trim(), 10); const full_name = modal.find("input[name='full_name']"); const profile_data = get_human_profile_data(fields_user_pills); const url = "/json/users/" + encodeURIComponent(user_id); const data = { full_name: JSON.stringify(full_name.val()), - is_admin: JSON.stringify(user_role_select_value === 'admin'), - is_guest: JSON.stringify(user_role_select_value === 'guest'), + role: JSON.stringify(role), profile_data: JSON.stringify(profile_data), }; diff --git a/static/templates/admin_human_form.hbs b/static/templates/admin_human_form.hbs index bb53c6085d..e8e3e4538b 100644 --- a/static/templates/admin_human_form.hbs +++ b/static/templates/admin_human_form.hbs @@ -17,10 +17,8 @@
- + {{> settings/dropdown_options_widget option_values=user_role_values}}
diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 68bce39bab..a2a002e790 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -16,6 +16,9 @@ below features are supported. the user; use the `user_id` field instead. Previously, some (but not all) events of these types contained an `email` key in addition to to `user_id`) for identifying the modified user. +* [`PATCH /users/{user_id}`](/api/update-user): The `is_admin` and + `is_guest` parameters were removed in favor of the more general + `role` parameter for specifying a change in user role. **Feature level 6** * [`GET /events`](/api/get-events-from-queue): `realm_user` events to diff --git a/zerver/models.py b/zerver/models.py index 32315b1a2a..dcfa0b2b1c 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -880,6 +880,12 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): ROLE_GUEST = 600 role: int = models.PositiveSmallIntegerField(default=ROLE_MEMBER, db_index=True) + ROLE_TYPES = [ + ROLE_REALM_ADMINISTRATOR, + ROLE_MEMBER, + ROLE_GUEST, + ] + # Whether the user has been "soft-deactivated" due to weeks of inactivity. # For these users we avoid doing UserMessage table work, as an optimization # for large Zulip organizations with lots of single-visit users. diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index c413900a87..457b99d839 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1579,21 +1579,24 @@ paths: type: string example: NewName required: false - - name: is_admin + - name: role in: query description: | - Whether the target user is an administrator. + New [role](/help/roles-and-permissions) for the user. Supported roles include: + * Organization owner: 100 + * Organization administrator: 200 + * Member: 400 + * Guest: 600 + + Only organization owners can add or remove the owner permission. + + The owner permission cannot be removed from the only organization owner. + + **Changes**: New in Zulip 2.2 (feature level 8), replacing the previous + pair of `is_admin` and `is_guest` boolean parameters. schema: - type: boolean - example: false - required: false - - name: is_guest - in: query - description: | - Whether the target user is a guest. - schema: - type: boolean - example: true + type: integer + example: 400 required: false - name: profile_data in: query diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 4a4ac70577..f687282620 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -138,7 +138,7 @@ class PermissionTest(ZulipTestCase): self.assertFalse(othello_dict['is_admin']) # Giveth - req = dict(is_admin=ujson.dumps(True)) + req = dict(role=ujson.dumps(UserProfile.ROLE_REALM_ADMINISTRATOR)) events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): @@ -151,7 +151,7 @@ class PermissionTest(ZulipTestCase): self.assertEqual(person['is_admin'], True) # Taketh away - req = dict(is_admin=ujson.dumps(False)) + req = dict(role=ujson.dumps(UserProfile.ROLE_MEMBER)) events = [] with tornado_redirected_to_list(events): result = self.client_patch('/json/users/{}'.format(othello.id), req) @@ -164,7 +164,7 @@ class PermissionTest(ZulipTestCase): # Cannot take away from last admin self.login('iago') - req = dict(is_admin=ujson.dumps(False)) + req = dict(role=ujson.dumps(UserProfile.ROLE_MEMBER)) events = [] with tornado_redirected_to_list(events): result = self.client_patch('/json/users/{}'.format(desdemona.id), req) @@ -355,15 +355,7 @@ class PermissionTest(ZulipTestCase): hamlet = self.example_user("hamlet") self.assertFalse(hamlet.is_guest) - # Test failure of making user both admin and guest - req = dict(is_guest=ujson.dumps(True), is_admin=ujson.dumps(True)) - result = self.client_patch('/json/users/{}'.format(hamlet.id), req) - self.assert_json_error(result, 'Guests cannot be organization administrators') - self.assertFalse(hamlet.is_guest) - self.assertFalse(hamlet.is_realm_admin) - hamlet = self.example_user("hamlet") - - req = dict(is_guest=ujson.dumps(True)) + req = dict(role=ujson.dumps(UserProfile.ROLE_GUEST)) events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): result = self.client_patch('/json/users/{}'.format(hamlet.id), req) @@ -382,7 +374,7 @@ class PermissionTest(ZulipTestCase): polonius = self.example_user("polonius") self.assertTrue(polonius.is_guest) - req = dict(is_guest=ujson.dumps(False)) + req = dict(role=ujson.dumps(UserProfile.ROLE_MEMBER)) events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): result = self.client_patch('/json/users/{}'.format(polonius.id), req) @@ -402,15 +394,10 @@ class PermissionTest(ZulipTestCase): self.assertFalse(hamlet.is_guest) self.assertTrue(hamlet.is_realm_admin) - # Test failure of making a admin to guest without revoking admin status - req = dict(is_guest=ujson.dumps(True)) - result = self.client_patch('/json/users/{}'.format(hamlet.id), req) - self.assert_json_error(result, 'Guests cannot be organization administrators') - # Test changing a user from admin to guest and revoking admin status hamlet = self.example_user("hamlet") self.assertFalse(hamlet.is_guest) - req = dict(is_admin=ujson.dumps(False), is_guest=ujson.dumps(True)) + req = dict(role=ujson.dumps(UserProfile.ROLE_GUEST)) events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): result = self.client_patch('/json/users/{}'.format(hamlet.id), req) @@ -435,15 +422,10 @@ class PermissionTest(ZulipTestCase): self.assertTrue(polonius.is_guest) self.assertFalse(polonius.is_realm_admin) - # Test failure of making a guest to admin without revoking guest status - req = dict(is_admin=ujson.dumps(True)) - result = self.client_patch('/json/users/{}'.format(polonius.id), req) - self.assert_json_error(result, 'Guests cannot be organization administrators') - # Test changing a user from guest to admin and revoking guest status polonius = self.example_user("polonius") self.assertFalse(polonius.is_realm_admin) - req = dict(is_admin=ujson.dumps(True), is_guest=ujson.dumps(False)) + req = dict(role=ujson.dumps(UserProfile.ROLE_REALM_ADMINISTRATOR)) events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): result = self.client_patch('/json/users/{}'.format(polonius.id), req) diff --git a/zerver/views/users.py b/zerver/views/users.py index c2807cdb74..ce3c101a34 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -24,7 +24,8 @@ from zerver.lib.request import has_request_variables, REQ from zerver.lib.response import json_error, json_success from zerver.lib.streams import access_stream_by_name from zerver.lib.upload import upload_avatar_image -from zerver.lib.validator import check_bool, check_string, check_int, check_url, check_dict, check_list +from zerver.lib.validator import check_bool, check_string, check_int, check_url, check_dict, check_list, \ + check_int_in from zerver.lib.url_encoding import add_query_arg_to_redirect_url from zerver.lib.users import check_valid_bot_type, check_bot_creation_policy, \ check_full_name, check_short_name, check_valid_interface_type, check_valid_bot_config, \ @@ -80,36 +81,16 @@ def reactivate_user_backend(request: HttpRequest, user_profile: UserProfile, @has_request_variables def update_user_backend(request: HttpRequest, user_profile: UserProfile, user_id: int, full_name: Optional[str]=REQ(default="", validator=check_string), - is_admin: Optional[bool]=REQ(default=None, validator=check_bool), - is_guest: Optional[bool]=REQ(default=None, validator=check_bool), + role: Optional[int]=REQ(default=None, validator=check_int_in( + UserProfile.ROLE_TYPES)), profile_data: Optional[List[Dict[str, Union[int, str, List[int]]]]]= REQ(default=None, validator=check_list(check_dict([('id', check_int)])))) -> HttpResponse: target = access_user_by_id(user_profile, user_id, allow_deactivated=True, allow_bots=True) - # Historically, UserProfile had two fields, is_guest and is_realm_admin. - # This condition protected against situations where update_user_backend - # could cause both is_guest and is_realm_admin to be set. - # Once we update the frontend to just send a 'role' value, we can remove this check. - if (((is_guest is None and target.is_guest) or is_guest) and - ((is_admin is None and target.is_realm_admin) or is_admin)): - return json_error(_("Guests cannot be organization administrators")) - - role = None - if is_admin is not None and target.is_realm_admin != is_admin: - if not is_admin and check_last_admin(user_profile): - return json_error(_('Cannot remove the only organization administrator')) - role = UserProfile.ROLE_MEMBER - if is_admin: - role = UserProfile.ROLE_REALM_ADMINISTRATOR - - if is_guest is not None and target.is_guest != is_guest: - if is_guest: - role = UserProfile.ROLE_GUEST - if role is None: - role = UserProfile.ROLE_MEMBER - if role is not None and target.role != role: + if target.role == UserProfile.ROLE_REALM_ADMINISTRATOR and check_last_admin(user_profile): + return json_error(_('Cannot remove the only organization administrator')) do_change_user_role(target, role) if (full_name is not None and target.full_name != full_name and