From 160cc5120aae0870c68e6e3a12a391035bd25843 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Mon, 14 Dec 2020 22:02:22 +0100 Subject: [PATCH] api: Require can_create_users permission to create users via API. Allowing any admins to create arbitrary users is not ideal because it can lead to abuse issues. We should require something stronger that requires the server operator's approval and thus we add a new can_create_users permission. --- templates/zerver/api/changelog.md | 5 ++++ .../help/include/can-create-users-only.md | 12 ++++++++++ tools/test-api | 4 ++++ version.py | 2 +- zerver/lib/actions.py | 4 ++++ .../management/commands/change_user_role.py | 24 +++++++++++++++---- .../0309_userprofile_can_create_users.py | 18 ++++++++++++++ zerver/models.py | 2 ++ zerver/openapi/zulip.yaml | 2 +- zerver/tests/test_users.py | 23 ++++++++++++++---- zerver/views/users.py | 3 +++ 11 files changed, 87 insertions(+), 12 deletions(-) create mode 100644 templates/zerver/help/include/can-create-users-only.md create mode 100644 zerver/migrations/0309_userprofile_can_create_users.py diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 0e38dd2a9d..48f7419f96 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,11 @@ below features are supported. ## Changes in Zulip 4.0 +**Feature level 36** + +* [`POST /users`](/api/create-user): Restricted access to organization + administrators with the `can_create_users` permission. + **Feature level 35** * The peer_add and peer_remove subscription events now have plural diff --git a/templates/zerver/help/include/can-create-users-only.md b/templates/zerver/help/include/can-create-users-only.md new file mode 100644 index 0000000000..c29ae9bb6c --- /dev/null +++ b/templates/zerver/help/include/can-create-users-only.md @@ -0,0 +1,12 @@ +!!! warn "" + This endpoint is limited to organizations administrators who + additionally have the `can_create_users` permission for the Zulip organization. + + Zulip Cloud users can request the `can_create_users` permission for a bot by contacting + [Zulip Cloud support](/help/contact-support) with an explanation for why it is needed. + + Self-hosted installations can toggle `can_create_users` on an account using + the `manage.py change_user_role` command. + + **Changes**: Before Zulip 4.0 (feature level 36), this endpoint was + available to all organization administrators. diff --git a/tools/test-api b/tools/test-api index 674040c0b2..b405922ae2 100755 --- a/tools/test-api +++ b/tools/test-api @@ -47,6 +47,10 @@ with test_server_running(force=options.force, external_host='zulipdev.com:9981') email = 'iago@zulip.com' # Iago is an admin realm = get_realm('zulip') user = get_user(email, realm) + # Required to test can_create_users endpoints. + user.can_create_users = True + user.save(update_fields=["can_create_users"]) + api_key = get_api_key(user) site = 'http://zulip.zulipdev.com:9981' client = Client( diff --git a/version.py b/version.py index 01e915ee51..b34b65608d 100644 --- a/version.py +++ b/version.py @@ -28,7 +28,7 @@ DESKTOP_WARNING_VERSION = "5.2.0" # # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md. -API_FEATURE_LEVEL = 35 +API_FEATURE_LEVEL = 36 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 8385aa741b..7a77a4aa37 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -3621,6 +3621,10 @@ def do_change_can_forge_sender(user_profile: UserProfile, value: bool) -> None: user_profile.can_forge_sender = value user_profile.save(update_fields=["can_forge_sender"]) +def do_change_can_create_users(user_profile: UserProfile, value: bool) -> None: + user_profile.can_create_users = value + user_profile.save(update_fields=["can_create_users"]) + def do_change_stream_invite_only(stream: Stream, invite_only: bool, history_public_to_subscribers: Optional[bool]=None) -> None: history_public_to_subscribers = get_default_value_for_history_public_to_subscribers( diff --git a/zerver/management/commands/change_user_role.py b/zerver/management/commands/change_user_role.py index 0c5cf0702f..81ec2f07f7 100644 --- a/zerver/management/commands/change_user_role.py +++ b/zerver/management/commands/change_user_role.py @@ -3,7 +3,11 @@ from typing import Any from django.core.management.base import CommandError -from zerver.lib.actions import do_change_can_forge_sender, do_change_user_role +from zerver.lib.actions import ( + do_change_can_create_users, + do_change_can_forge_sender, + do_change_user_role, +) from zerver.lib.management import ZulipBaseCommand from zerver.models import UserProfile @@ -18,12 +22,13 @@ ONLY perform this on customer request from an authorized person. parser.add_argument('email', metavar='', help="email of user to change role") parser.add_argument('new_role', metavar='', - choices=['owner', 'admin', 'member', 'guest', 'can_forge_sender'], + choices=['owner', 'admin', 'member', 'guest', 'can_forge_sender', + 'can_create_users'], help="new role of the user") parser.add_argument('--revoke', dest='grant', action="store_false", - help='Remove can_forge_sender permission.') + help='Remove can_forge_sender or can_create_users permission.') self.add_realm_args(parser, True) def handle(self, *args: Any, **options: Any) -> None: @@ -37,7 +42,7 @@ ONLY perform this on customer request from an authorized person. 'member': UserProfile.ROLE_MEMBER, 'guest': UserProfile.ROLE_GUEST} - if options['new_role'] != 'can_forge_sender': + if options['new_role'] not in ['can_forge_sender', 'can_create_users']: new_role = user_role_map[options['new_role']] if not options['grant']: raise CommandError("Revoke not supported with this permission; please specify new role.") @@ -47,11 +52,20 @@ ONLY perform this on customer request from an authorized person. do_change_user_role(user, new_role, acting_user=None) new_role_name = UserProfile.ROLE_ID_TO_NAME_MAP[user.role] print(f"Role for {user.delivery_email} changed from {old_role_name} to {new_role_name}.") - else: + return + + if options['new_role'] == 'can_forge_sender': if user.can_forge_sender and options['grant']: raise CommandError("User can already forge messages for this realm.") elif not user.can_forge_sender and not options['grant']: raise CommandError("User can't forge messages for this realm.") do_change_can_forge_sender(user, options['grant']) + granted_text = "have" if options['grant'] else "not have" print(f"{user.delivery_email} changed to {granted_text} {options['new_role']} permission.") + else: + if user.can_create_users and options['grant']: + raise CommandError("User can already create users for this realm.") + elif not user.can_create_users and not options['grant']: + raise CommandError("User can't create users for this realm.") + do_change_can_create_users(user, options['grant']) diff --git a/zerver/migrations/0309_userprofile_can_create_users.py b/zerver/migrations/0309_userprofile_can_create_users.py new file mode 100644 index 0000000000..ae47083e87 --- /dev/null +++ b/zerver/migrations/0309_userprofile_can_create_users.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.17 on 2020-12-20 14:18 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0308_remove_reduntant_realm_meta_permissions'), + ] + + operations = [ + migrations.AddField( + model_name='userprofile', + name='can_create_users', + field=models.BooleanField(db_index=True, default=False), + ), + ] diff --git a/zerver/models.py b/zerver/models.py index 6500afcb09..e464754fc8 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -985,6 +985,8 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): # Users with this flag set are allowed to forge messages as sent by another # user and to send to private streams; also used for Zephyr/Jabber mirroring. can_forge_sender: bool = models.BooleanField(default=False, db_index=True) + # Users with this flag set can create other users via API. + can_create_users: bool = models.BooleanField(default=False, db_index=True) ### Notifications settings. ### diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index cb514f8454..4bed3f6a43 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -4179,7 +4179,7 @@ paths: operationId: create_user tags: ["users"] description: | - {!api-admin-only.md!} + {!can-create-users-only.md!} Create a new user account via the API. diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index fd0110c2f9..4ad8e24dd7 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -12,6 +12,7 @@ from django.utils.timezone import now as timezone_now from zerver.lib.actions import ( create_users, + do_change_can_create_users, do_change_user_role, do_create_user, do_deactivate_user, @@ -840,6 +841,23 @@ class AdminCreateUserTest(ZulipTestCase): admin = self.example_user('hamlet') realm = admin.realm self.login_user(admin) + do_change_user_role(admin, UserProfile.ROLE_REALM_ADMINISTRATOR) + valid_params = dict( + email='romeo@zulip.net', + password='xxxx', + full_name='Romeo Montague', + ) + + self.assertEqual(admin.can_create_users, False) + result = self.client_post("/json/users", valid_params) + self.assert_json_error(result, "User not authorized for this query") + + do_change_can_create_users(admin, True) + # can_create_users is insufficient without being a realm administrator: + do_change_user_role(admin, UserProfile.ROLE_MEMBER) + result = self.client_post("/json/users", valid_params) + self.assert_json_error(result, "Must be an organization administrator") + do_change_user_role(admin, UserProfile.ROLE_REALM_ADMINISTRATOR) result = self.client_post("/json/users", {}) @@ -882,11 +900,6 @@ class AdminCreateUserTest(ZulipTestCase): "Email 'romeo@not-zulip.com' not allowed in this organization") RealmDomain.objects.create(realm=get_realm('zulip'), domain='zulip.net') - valid_params = dict( - email='romeo@zulip.net', - password='xxxx', - full_name='Romeo Montague', - ) # Check can't use a bad password with zxcvbn enabled with self.settings(PASSWORD_MIN_LENGTH=6, PASSWORD_MIN_GUESSES=1000): result = self.client_post("/json/users", valid_params) diff --git a/zerver/views/users.py b/zerver/views/users.py index 93b1056c6f..cd69742ffd 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -493,6 +493,9 @@ def create_user_backend( password: str=REQ(), full_name_raw: str=REQ("full_name"), ) -> HttpResponse: + if not user_profile.can_create_users: + return json_error(_("User not authorized for this query")) + full_name = check_full_name(full_name_raw) form = CreateUserForm({'full_name': full_name, 'email': email}) if not form.is_valid():