From 389b851f81b8ad907e9a9cbd7a57210fb4e633e3 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Tue, 3 Sep 2024 21:41:18 +0200 Subject: [PATCH] update_user_backend: Allow authorized org owners to change user emails. This adds a new special UserProfile flag can_change_user_emails(disabled by default) and the ability for changing the email address of users in the realm via update_user_backend. This is useful for allowing organizations to update user emails without needing to set up a SCIM integration, but since it gives the ability to hijack user accounts, it needs to be behind this additional permission and can't be just given to organization owners by default. Analogical to how the create_user_backend endpoint works. --- tools/test-api | 9 ++-- version.py | 2 +- zerver/actions/users.py | 22 ++++++++ .../management/commands/change_user_role.py | 15 +++++- ...0616_userprofile_can_change_user_emails.py | 17 +++++++ zerver/models/users.py | 2 + zerver/openapi/zulip.yaml | 10 ++++ zerver/tests/test_users.py | 51 +++++++++++++++++++ zerver/views/users.py | 28 +++++++++- 9 files changed, 150 insertions(+), 6 deletions(-) create mode 100644 zerver/migrations/0616_userprofile_can_change_user_emails.py diff --git a/tools/test-api b/tools/test-api index 73fa6acd1b..b3b2310973 100755 --- a/tools/test-api +++ b/tools/test-api @@ -59,7 +59,8 @@ with test_server_running( # Prepare the admin client email = "iago@zulip.com" # Iago is an admin realm = get_realm("zulip") - user = get_user(email, realm) + iago = get_user(email, realm) + user = iago # Iago needs permission to manage all user groups. admins_group = NamedUserGroup.objects.get( @@ -69,9 +70,10 @@ with test_server_running( realm, "can_manage_all_groups", admins_group, acting_user=None ) - # Required to test can_create_users endpoints. + # Required to test can_create_users and can_change_user_emails endpoints. user.can_create_users = True - user.save(update_fields=["can_create_users"]) + user.can_change_user_emails = True + user.save(update_fields=["can_create_users", "can_change_user_emails"]) api_key = get_api_key(user) site = "http://zulip.zulipdev.com:9981" @@ -85,6 +87,7 @@ with test_server_running( email = "desdemona@zulip.com" # desdemona is an owner realm = get_realm("zulip") user = get_user(email, realm) + api_key = get_api_key(user) site = "http://zulip.zulipdev.com:9981" owner_client = Client( diff --git a/version.py b/version.py index c0c47a6a89..3e74f3ec6a 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 312 # Last bumped for adding 'realm_export_consent' event type. +API_FEATURE_LEVEL = 313 # Last bumped for adding `new_email` to /users/{user_id} # 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/actions/users.py b/zerver/actions/users.py index 81ae6a0ef7..ac8fb55c61 100644 --- a/zerver/actions/users.py +++ b/zerver/actions/users.py @@ -749,6 +749,28 @@ def do_change_can_create_users(user_profile: UserProfile, value: bool) -> None: ) +@transaction.atomic(savepoint=False) +def do_change_can_change_user_emails(user_profile: UserProfile, value: bool) -> None: + event_time = timezone_now() + old_value = user_profile.can_change_user_emails + + user_profile.can_change_user_emails = value + user_profile.save(update_fields=["can_change_user_emails"]) + + RealmAuditLog.objects.create( + realm=user_profile.realm, + event_type=AuditLogEventType.USER_SPECIAL_PERMISSION_CHANGED, + event_time=event_time, + acting_user=None, + modified_user=user_profile, + extra_data={ + RealmAuditLog.OLD_VALUE: old_value, + RealmAuditLog.NEW_VALUE: value, + "property": "can_change_user_emails", + }, + ) + + @transaction.atomic(durable=True) def do_update_outgoing_webhook_service( bot_profile: UserProfile, service_interface: int, service_payload_url: str diff --git a/zerver/management/commands/change_user_role.py b/zerver/management/commands/change_user_role.py index e0c49256b1..b8ff9518e5 100644 --- a/zerver/management/commands/change_user_role.py +++ b/zerver/management/commands/change_user_role.py @@ -6,6 +6,7 @@ from django.core.management.base import CommandError from typing_extensions import override from zerver.actions.users import ( + do_change_can_change_user_emails, do_change_can_create_users, do_change_can_forge_sender, do_change_is_billing_admin, @@ -23,6 +24,7 @@ ROLE_CHOICES = [ "guest", "can_forge_sender", "can_create_users", + "can_change_user_emails", "is_billing_admin", ] @@ -65,7 +67,12 @@ ONLY perform this on customer request from an authorized person. "guest": UserProfile.ROLE_GUEST, } - if options["new_role"] not in ["can_forge_sender", "can_create_users", "is_billing_admin"]: + if options["new_role"] not in [ + "can_forge_sender", + "can_create_users", + "can_change_user_emails", + "is_billing_admin", + ]: new_role = user_role_map[options["new_role"]] if not options["grant"]: raise CommandError( @@ -109,6 +116,12 @@ ONLY perform this on customer request from an authorized person. 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"]) + elif options["new_role"] == "can_change_user_emails": + if user.can_change_user_emails and options["grant"]: + raise CommandError("User can already change user emails for this realm.") + elif not user.can_change_user_emails and not options["grant"]: + raise CommandError("User can't change user emails for this realm.") + do_change_can_change_user_emails(user, options["grant"]) else: assert options["new_role"] == "is_billing_admin" if user.is_billing_admin and options["grant"]: diff --git a/zerver/migrations/0616_userprofile_can_change_user_emails.py b/zerver/migrations/0616_userprofile_can_change_user_emails.py new file mode 100644 index 0000000000..b29d1ca843 --- /dev/null +++ b/zerver/migrations/0616_userprofile_can_change_user_emails.py @@ -0,0 +1,17 @@ +# Generated by Django 5.0.8 on 2024-09-02 20:36 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0615_system_bot_avatars"), + ] + + operations = [ + migrations.AddField( + model_name="userprofile", + name="can_change_user_emails", + field=models.BooleanField(db_index=True, default=False), + ), + ] diff --git a/zerver/models/users.py b/zerver/models/users.py index b32e3efe4c..406271b38f 100644 --- a/zerver/models/users.py +++ b/zerver/models/users.py @@ -571,6 +571,8 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings): can_forge_sender = models.BooleanField(default=False, db_index=True) # Users with this flag set can create other users via API. can_create_users = models.BooleanField(default=False, db_index=True) + # Users with this flag can change email addresses of users in the realm via the API. + can_change_user_emails = models.BooleanField(default=False, db_index=True) # Used for rate-limiting certain automated messages generated by bots last_reminder = models.DateTimeField(default=None, null=True) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index f3b3762193..268d9394c8 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -12779,6 +12779,16 @@ paths: type: object example: [{"id": 4, "value": "0"}, {"id": 5, "value": "1909-04-05"}] + new_email: + description: | + New email address for the user. Requires the user making + the request to be an organization administrator and + additionally have the `.can_change_user_emails` special + permission. + + **Changes**: New in Zulip 10.0 (feature level 313). + type: string + example: username@example.com encoding: role: contentType: application/json diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index cb51ea802d..bbdf86fa96 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -23,6 +23,7 @@ from zerver.actions.user_settings import bulk_regenerate_api_keys, do_change_use from zerver.actions.user_topics import do_set_user_topic_visibility_policy from zerver.actions.users import ( change_user_is_active, + do_change_can_change_user_emails, do_change_can_create_users, do_change_can_forge_sender, do_change_is_billing_admin, @@ -1120,6 +1121,56 @@ class BulkCreateUserTest(ZulipTestCase): ) +class AdminChangeUserEmailTest(ZulipTestCase): + def test_change_user_email_backend(self) -> None: + cordelia = self.example_user("cordelia") + realm_admin = self.example_user("iago") + self.login_user(realm_admin) + + valid_params = dict(new_email="cordelia_new@zulip.com") + + self.assertEqual(realm_admin.can_change_user_emails, False) + result = self.client_patch(f"/json/users/{cordelia.id}", valid_params) + self.assert_json_error(result, "User not authorized to change user emails") + + do_change_can_change_user_emails(realm_admin, True) + # can_change_user_emails is insufficient without being a realm administrator: + do_change_user_role(realm_admin, UserProfile.ROLE_MEMBER, acting_user=None) + result = self.client_patch(f"/json/users/{cordelia.id}", valid_params) + self.assert_json_error(result, "Insufficient permission") + + do_change_user_role(realm_admin, UserProfile.ROLE_REALM_ADMINISTRATOR, acting_user=None) + result = self.client_patch( + f"/json/users/{cordelia.id}", + dict(new_email="invalid"), + ) + self.assert_json_error(result, "Invalid new email address.") + + result = self.client_patch( + f"/json/users/{UserProfile.objects.latest('id').id + 1}", + dict(new_email="new@zulip.com"), + ) + self.assert_json_error(result, "No such user") + + result = self.client_patch( + f"/json/users/{cordelia.id}", + dict(new_email=realm_admin.delivery_email), + ) + self.assert_json_error(result, "New email value error: Already has an account.") + + result = self.client_patch(f"/json/users/{cordelia.id}", valid_params) + self.assert_json_success(result) + + cordelia.refresh_from_db() + self.assertEqual(cordelia.delivery_email, "cordelia_new@zulip.com") + + last_realm_audit_log = RealmAuditLog.objects.last() + assert last_realm_audit_log is not None + self.assertEqual(last_realm_audit_log.event_type, AuditLogEventType.USER_EMAIL_CHANGED) + self.assertEqual(last_realm_audit_log.modified_user, cordelia) + self.assertEqual(last_realm_audit_log.acting_user, realm_admin) + + class AdminCreateUserTest(ZulipTestCase): def test_create_user_backend(self) -> None: # This test should give us complete coverage on diff --git a/zerver/views/users.py b/zerver/views/users.py index ce69de8feb..b16f84a298 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -4,6 +4,8 @@ from typing import Annotated, Any, TypeAlias from django.conf import settings from django.contrib.auth.models import AnonymousUser +from django.core import validators +from django.core.exceptions import ValidationError from django.core.files.uploadedfile import UploadedFile from django.db import transaction from django.http import HttpRequest, HttpResponse @@ -26,6 +28,7 @@ from zerver.actions.user_settings import ( check_change_bot_full_name, check_change_full_name, do_change_avatar_fields, + do_change_user_delivery_email, do_regenerate_api_key, ) from zerver.actions.users import ( @@ -39,7 +42,7 @@ from zerver.decorator import require_member_or_admin, require_realm_admin from zerver.forms import PASSWORD_TOO_WEAK_ERROR, CreateUserForm from zerver.lib.avatar import avatar_url, get_avatar_for_inaccessible_user, get_gravatar_url from zerver.lib.bot_config import set_bot_config -from zerver.lib.email_validation import email_allowed_for_realm +from zerver.lib.email_validation import email_allowed_for_realm, validate_email_not_already_in_realm from zerver.lib.exceptions import ( CannotDeactivateLastUserError, JsonableError, @@ -207,11 +210,17 @@ def update_user_backend( full_name: str | None = None, role: Json[RoleParamType] | None = None, profile_data: Json[list[ProfileDataElement]] | None = None, + new_email: str | None = None, ) -> HttpResponse: target = access_user_by_id( user_profile, user_id, allow_deactivated=True, allow_bots=True, for_admin=True ) + if new_email is not None and ( + not user_profile.can_change_user_emails or not user_profile.is_realm_admin + ): + raise JsonableError(_("User not authorized to change user emails")) + if role is not None and target.role != role: # Require that the current user has permissions to # grant/remove the role in question. @@ -261,6 +270,23 @@ def update_user_backend( ) do_update_user_custom_profile_data_if_changed(target, clean_profile_data) + if new_email is not None and target.delivery_email != new_email: + assert user_profile.can_change_user_emails and user_profile.is_realm_admin + try: + validators.validate_email(new_email) + except ValidationError: + raise JsonableError(_("Invalid new email address.")) + try: + validate_email_not_already_in_realm( + user_profile.realm, + new_email, + verbose=False, + ) + except ValidationError as e: + raise JsonableError(_("New email value error: {message}").format(message=e.message)) + + do_change_user_delivery_email(target, new_email, acting_user=user_profile) + return json_success(request)