From 1aebf3cab9d2701d09e3898c01aabf367d0c1eae Mon Sep 17 00:00:00 2001 From: sahil839 Date: Thu, 21 May 2020 03:43:06 +0530 Subject: [PATCH] actions: Merge do_change_is_admin and do_change_is_guest. This commit merges do_change_is_admin and do_change_is_guest to a single function do_change_user_role which will be used for changing role of users. do_change_is_api_super_user is added as a separate function for changing is_api_super_user field of UserProfile. --- .../commands/populate_analytics_db.py | 7 ++- zerver/lib/actions.py | 60 +++++-------------- zerver/lib/server_initialization.py | 4 +- zerver/management/commands/knight.py | 13 +++- zerver/tests/test_audit_log.py | 10 ++-- zerver/tests/test_email_notifications.py | 4 +- zerver/tests/test_events.py | 19 +++--- zerver/tests/test_messages.py | 6 +- zerver/tests/test_realm_domains.py | 6 +- zerver/tests/test_signup.py | 10 ++-- zerver/tests/test_subs.py | 60 +++++++++---------- zerver/tests/test_users.py | 33 +++------- zerver/views/users.py | 17 ++++-- zilencer/management/commands/populate_db.py | 6 +- 14 files changed, 113 insertions(+), 142 deletions(-) diff --git a/analytics/management/commands/populate_analytics_db.py b/analytics/management/commands/populate_analytics_db.py index fa206b45ce..8490a7bd90 100644 --- a/analytics/management/commands/populate_analytics_db.py +++ b/analytics/management/commands/populate_analytics_db.py @@ -11,10 +11,11 @@ from analytics.lib.fixtures import generate_time_series_data from analytics.lib.time_utils import time_range from analytics.models import BaseCount, FillState, InstallationCount, \ RealmCount, StreamCount, UserCount -from zerver.lib.actions import STREAM_ASSIGNMENT_COLORS, do_change_is_admin +from zerver.lib.actions import STREAM_ASSIGNMENT_COLORS, do_change_user_role from zerver.lib.create_user import create_user from zerver.lib.timestamp import floor_to_day -from zerver.models import Client, Realm, Recipient, Stream, Subscription +from zerver.models import Client, Realm, Recipient, Stream, Subscription, \ + UserProfile class Command(BaseCommand): @@ -59,7 +60,7 @@ class Command(BaseCommand): shylock = create_user('shylock@analytics.ds', 'Shylock', realm, full_name='Shylock', short_name='shylock', is_realm_admin=True) - do_change_is_admin(shylock, True) + do_change_user_role(shylock, UserProfile.ROLE_REALM_ADMINISTRATOR) stream = Stream.objects.create( name='all', realm=realm, date_created=installation_time) recipient = Recipient.objects.create(type_id=stream.id, type=Recipient.STREAM) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 280fdfec46..8e5323bf73 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -3421,60 +3421,32 @@ def do_change_default_all_public_streams(user_profile: UserProfile, value: bool, )), bot_owner_user_ids(user_profile)) -def do_change_is_admin(user_profile: UserProfile, value: bool, - permission: str='administer') -> None: - # TODO: This function and do_change_is_guest should be merged into - # a single do_change_user_role function in a future refactor. - if permission == "administer": - old_value = user_profile.role - if value: - user_profile.role = UserProfile.ROLE_REALM_ADMINISTRATOR - else: - user_profile.role = UserProfile.ROLE_MEMBER - user_profile.save(update_fields=["role"]) - elif permission == "api_super_user": - user_profile.is_api_super_user = value - user_profile.save(update_fields=["is_api_super_user"]) - else: - raise AssertionError("Invalid admin permission") - - if permission == 'administer': - RealmAuditLog.objects.create( - realm=user_profile.realm, modified_user=user_profile, - event_type=RealmAuditLog.USER_ROLE_CHANGED, event_time=timezone_now(), - extra_data=ujson.dumps({ - RealmAuditLog.OLD_VALUE: old_value, - RealmAuditLog.NEW_VALUE: UserProfile.ROLE_REALM_ADMINISTRATOR, - RealmAuditLog.ROLE_COUNT: realm_user_count_by_role(user_profile.realm), - })) - event = dict(type="realm_user", op="update", - person=dict(user_id=user_profile.id, - is_admin=value)) - send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) - -def do_change_is_guest(user_profile: UserProfile, value: bool) -> None: - # TODO: This function and do_change_is_admin should be merged into - # a single do_change_user_role function in a future refactor. +def do_change_user_role(user_profile: UserProfile, value: int) -> None: old_value = user_profile.role - if value: - user_profile.role = UserProfile.ROLE_GUEST - else: - user_profile.role = UserProfile.ROLE_MEMBER + user_profile.role = value user_profile.save(update_fields=["role"]) - RealmAuditLog.objects.create( realm=user_profile.realm, modified_user=user_profile, event_type=RealmAuditLog.USER_ROLE_CHANGED, event_time=timezone_now(), extra_data=ujson.dumps({ RealmAuditLog.OLD_VALUE: old_value, - RealmAuditLog.NEW_VALUE: UserProfile.ROLE_GUEST, + RealmAuditLog.NEW_VALUE: value, RealmAuditLog.ROLE_COUNT: realm_user_count_by_role(user_profile.realm), })) - event = dict(type="realm_user", op="update", - person=dict(user_id=user_profile.id, - is_guest=value)) - send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) + if UserProfile.ROLE_REALM_ADMINISTRATOR in [old_value, value]: + event = dict(type="realm_user", op="update", + person=dict(user_id=user_profile.id, + is_admin=value == UserProfile.ROLE_REALM_ADMINISTRATOR)) + send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) + if UserProfile.ROLE_GUEST in [old_value, value]: + event = dict(type="realm_user", op="update", + person=dict(user_id=user_profile.id, + is_guest=value == UserProfile.ROLE_GUEST)) + send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) +def do_change_is_api_super_user(user_profile: UserProfile, value: bool) -> None: + user_profile.is_api_super_user = value + user_profile.save(update_fields=["is_api_super_user"]) def do_change_stream_invite_only(stream: Stream, invite_only: bool, history_public_to_subscribers: Optional[bool]=None) -> None: diff --git a/zerver/lib/server_initialization.py b/zerver/lib/server_initialization.py index 1eb9ccdda1..cf74b46cb2 100644 --- a/zerver/lib/server_initialization.py +++ b/zerver/lib/server_initialization.py @@ -10,7 +10,7 @@ def server_initialized() -> bool: return Realm.objects.exists() def create_internal_realm() -> None: - from zerver.lib.actions import do_change_is_admin + from zerver.lib.actions import do_change_is_api_super_user realm = Realm.objects.create(string_id=settings.SYSTEM_BOT_REALM) @@ -33,7 +33,7 @@ def create_internal_realm() -> None: # Initialize the email gateway bot as an API Super User email_gateway_bot = get_system_bot(settings.EMAIL_GATEWAY_BOT) - do_change_is_admin(email_gateway_bot, True, permission="api_super_user") + do_change_is_api_super_user(email_gateway_bot, True) def create_users(realm: Realm, name_list: Iterable[Tuple[str, str]], tos_version: Optional[str]=None, diff --git a/zerver/management/commands/knight.py b/zerver/management/commands/knight.py index bbb61c5b17..418b41bca9 100644 --- a/zerver/management/commands/knight.py +++ b/zerver/management/commands/knight.py @@ -3,8 +3,9 @@ from typing import Any from django.core.management.base import CommandError -from zerver.lib.actions import do_change_is_admin +from zerver.lib.actions import do_change_user_role, do_change_is_api_super_user from zerver.lib.management import ZulipBaseCommand +from zerver.models import UserProfile class Command(ZulipBaseCommand): @@ -46,7 +47,10 @@ ONLY perform this on customer request from an authorized person. raise CommandError("User already has permission for this realm.") else: if options['ack']: - do_change_is_admin(user, True, permission=options['permission']) + if options['permission'] == "api_super_user": + do_change_is_api_super_user(user, True) + elif options['permission'] == "administer": + do_change_user_role(user, UserProfile.ROLE_REALM_ADMINISTRATOR) print("Done!") else: print("Would have granted %s %s rights for %s" % ( @@ -55,7 +59,10 @@ ONLY perform this on customer request from an authorized person. if (user.is_realm_admin and options['permission'] == "administer" or user.is_api_super_user and options['permission'] == "api_super_user"): if options['ack']: - do_change_is_admin(user, False, permission=options['permission']) + if options['permission'] == "api_super_user": + do_change_is_api_super_user(user, False) + elif options['permission'] == "administer": + do_change_user_role(user, UserProfile.ROLE_MEMBER) print("Done!") else: print("Would have removed %s's %s rights on %s" % (email, options['permission'], diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 439070cbb5..64579f0496 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -5,7 +5,7 @@ from zerver.lib.actions import do_create_user, do_deactivate_user, \ do_change_user_delivery_email, do_change_avatar_fields, do_change_bot_owner, \ do_regenerate_api_key, do_change_tos_version, \ bulk_add_subscriptions, bulk_remove_subscriptions, get_streams_traffic, \ - do_change_is_admin, do_change_is_guest, do_deactivate_realm, do_reactivate_realm + do_change_user_role, do_deactivate_realm, do_reactivate_realm from zerver.lib.test_classes import ZulipTestCase from zerver.models import RealmAuditLog, get_client, get_realm, UserProfile from analytics.models import StreamCount @@ -53,10 +53,10 @@ class TestRealmAuditLog(ZulipTestCase): realm = get_realm('zulip') now = timezone_now() user_profile = self.example_user("hamlet") - do_change_is_admin(user_profile, True) - do_change_is_admin(user_profile, False) - do_change_is_guest(user_profile, True) - do_change_is_guest(user_profile, False) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) + do_change_user_role(user_profile, UserProfile.ROLE_MEMBER) + do_change_user_role(user_profile, UserProfile.ROLE_GUEST) + do_change_user_role(user_profile, UserProfile.ROLE_MEMBER) for event in RealmAuditLog.objects.filter( event_type=RealmAuditLog.USER_ROLE_CHANGED, realm=realm, modified_user=user_profile, diff --git a/zerver/tests/test_email_notifications.py b/zerver/tests/test_email_notifications.py index abdd8a20bb..1a8c8cfe22 100644 --- a/zerver/tests/test_email_notifications.py +++ b/zerver/tests/test_email_notifications.py @@ -13,7 +13,7 @@ from typing import List, Optional from zerver.lib.email_notifications import fix_emojis, handle_missedmessage_emails, \ enqueue_welcome_emails, relative_to_full_url -from zerver.lib.actions import do_change_notification_settings, do_change_is_admin +from zerver.lib.actions import do_change_notification_settings, do_change_user_role from zerver.lib.test_classes import ZulipTestCase from zerver.lib.send_email import FromAddress, send_custom_email from zerver.models import ( @@ -91,7 +91,7 @@ class TestCustomEmails(ZulipTestCase): def test_send_custom_email_admins_only(self) -> None: admin_user = self.example_user('hamlet') - do_change_is_admin(admin_user, True) + do_change_user_role(admin_user, UserProfile.ROLE_REALM_ADMINISTRATOR) non_admin_user = self.example_user('cordelia') diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 8b3c76ca89..c3193e6145 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -44,8 +44,7 @@ from zerver.lib.actions import ( do_change_full_name, do_change_icon_source, do_change_logo_source, - do_change_is_admin, - do_change_is_guest, + do_change_user_role, do_change_notification_settings, do_change_plan_type, do_change_realm_domain, @@ -1477,7 +1476,7 @@ class EventsRegisterTest(ZulipTestCase): "This is group1", streams) group = lookup_default_stream_groups(["group1"], self.user_profile.realm)[0] - do_change_is_guest(self.user_profile, True) + do_change_user_role(self.user_profile, UserProfile.ROLE_GUEST) venice_stream = get_stream("Venice", self.user_profile.realm) self.do_test(lambda: do_add_streams_to_default_stream_group(self.user_profile.realm, group, [venice_stream]), @@ -1502,7 +1501,7 @@ class EventsRegisterTest(ZulipTestCase): self.assert_on_error(error) def test_default_streams_events_guest(self) -> None: - do_change_is_guest(self.user_profile, True) + do_change_user_role(self.user_profile, UserProfile.ROLE_GUEST) stream = get_stream("Scotland", self.user_profile.realm) self.do_test(lambda: do_add_default_stream(stream), state_change_expected = False, num_events=0) @@ -1836,9 +1835,9 @@ class EventsRegisterTest(ZulipTestCase): ])), ]) - do_change_is_admin(self.user_profile, False) - for is_admin in [True, False]: - events = self.do_test(lambda: do_change_is_admin(self.user_profile, is_admin)) + do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER) + for role in [UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_MEMBER]: + events = self.do_test(lambda: do_change_user_role(self.user_profile, role)) error = schema_checker('events[0]', events[0]) self.assert_on_error(error) @@ -2833,7 +2832,7 @@ class EventsRegisterTest(ZulipTestCase): ]))), ]) - do_change_is_admin(self.user_profile, True) + do_change_user_role(self.user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) self.login_user(self.user_profile) with mock.patch('zerver.lib.export.do_export_realm', @@ -2900,7 +2899,7 @@ class EventsRegisterTest(ZulipTestCase): ]))), ]) - do_change_is_admin(self.user_profile, True) + do_change_user_role(self.user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) self.login_user(self.user_profile) with mock.patch('zerver.lib.export.do_export_realm', @@ -2931,7 +2930,7 @@ class FetchInitialStateDataTest(ZulipTestCase): # Admin users have access to all bots in the realm_bots field def test_realm_bots_admin(self) -> None: user_profile = self.example_user('hamlet') - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) self.assertTrue(user_profile.is_realm_admin) result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) self.assertTrue(len(result['realm_bots']) > 2) diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index f71c2d2de4..4754a306d0 100644 --- a/zerver/tests/test_messages.py +++ b/zerver/tests/test_messages.py @@ -15,7 +15,7 @@ from zerver.lib.actions import ( check_send_stream_message, create_mirror_user_if_needed, do_add_alert_words, - do_change_is_admin, + do_change_is_api_super_user, do_change_stream_invite_only, do_change_stream_post_policy, do_claim_attachments, @@ -1167,7 +1167,7 @@ class StreamMessagesTest(ZulipTestCase): user = self.mit_user('starnine') self.subscribe(user, 'Verona') - do_change_is_admin(user, True, 'api_super_user') + do_change_is_api_super_user(user, True) result = self.api_post(user, "/api/v1/messages", {"type": "stream", "to": "Verona", "sender": self.mit_email("sipbtest"), @@ -1178,7 +1178,7 @@ class StreamMessagesTest(ZulipTestCase): subdomain="zephyr") self.assert_json_success(result) - do_change_is_admin(user, False, 'api_super_user') + do_change_is_api_super_user(user, False) result = self.api_post(user, "/api/v1/messages", {"type": "stream", "to": "Verona", "sender": self.mit_email("sipbtest"), diff --git a/zerver/tests/test_realm_domains.py b/zerver/tests/test_realm_domains.py index 92bc0e1fdf..872fd78f9d 100644 --- a/zerver/tests/test_realm_domains.py +++ b/zerver/tests/test_realm_domains.py @@ -1,13 +1,13 @@ from django.core.exceptions import ValidationError from django.db.utils import IntegrityError -from zerver.lib.actions import do_change_is_admin, \ +from zerver.lib.actions import do_change_user_role, \ do_change_realm_domain, do_create_realm, \ do_remove_realm_domain, do_set_realm_property from zerver.lib.email_validation import email_allowed_for_realm from zerver.lib.domains import validate_domain from zerver.lib.test_classes import ZulipTestCase -from zerver.models import get_realm, \ +from zerver.models import get_realm, UserProfile, \ RealmDomain, DomainNotAllowedForRealmError import ujson @@ -59,7 +59,7 @@ class RealmDomainTest(ZulipTestCase): mit_user_profile = self.mit_user("sipbtest") self.login_user(mit_user_profile) - do_change_is_admin(mit_user_profile, True) + do_change_user_role(mit_user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) result = self.client_post("/json/realm/domains", info=data, HTTP_HOST=mit_user_profile.realm.host) diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 39c5afbd0c..eb6b760376 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -35,7 +35,7 @@ from zerver.models import ( Stream, Subscription, flush_per_request_caches, get_system_bot, ) from zerver.lib.actions import ( - do_change_is_admin, + do_change_user_role, get_stream, do_create_default_stream_group, do_add_default_stream, @@ -3718,7 +3718,7 @@ class DeactivateUserTest(ZulipTestCase): def test_do_not_deactivate_final_admin(self) -> None: user = self.example_user('iago') user_2 = self.example_user('desdemona') - do_change_is_admin(user_2, False) + do_change_user_role(user_2, UserProfile.ROLE_MEMBER) self.assertFalse(user_2.is_realm_admin) self.login_user(user) self.assertTrue(user.is_active) @@ -3727,15 +3727,15 @@ class DeactivateUserTest(ZulipTestCase): user = self.example_user('iago') self.assertTrue(user.is_active) self.assertTrue(user.is_realm_admin) - do_change_is_admin(user_2, True) + do_change_user_role(user_2, UserProfile.ROLE_REALM_ADMINISTRATOR) self.assertTrue(user_2.is_realm_admin) result = self.client_delete('/json/users/me') self.assert_json_success(result) - do_change_is_admin(user, True) + do_change_user_role(user, UserProfile.ROLE_REALM_ADMINISTRATOR) def test_do_not_deactivate_final_user(self) -> None: realm = get_realm('zulip') - do_change_is_admin(self.example_user("desdemona"), False) + do_change_user_role(self.example_user("desdemona"), UserProfile.ROLE_MEMBER) UserProfile.objects.filter(realm=realm).exclude( role=UserProfile.ROLE_REALM_ADMINISTRATOR).update(is_active=False) user = self.example_user("iago") diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 0a34bc41d8..5ce8e78716 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -47,11 +47,11 @@ from zerver.models import ( ) from zerver.lib.actions import ( - do_add_default_stream, do_change_is_admin, do_set_realm_property, + do_add_default_stream, do_change_user_role, do_set_realm_property, do_create_realm, do_remove_default_stream, bulk_get_subscriber_user_ids, gather_subscriptions_helper, bulk_add_subscriptions, bulk_remove_subscriptions, gather_subscriptions, get_default_streams_for_realm, get_stream, - do_get_streams, do_change_is_guest, + do_get_streams, create_stream_if_needed, ensure_stream, do_deactivate_stream, @@ -337,7 +337,7 @@ class StreamAdminTest(ZulipTestCase): self.login_user(user_profile) self.make_stream('private_stream', invite_only=True) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) params = { 'stream_name': ujson.dumps('private_stream'), 'is_private': ujson.dumps(False) @@ -349,7 +349,7 @@ class StreamAdminTest(ZulipTestCase): stream = self.subscribe(user_profile, 'private_stream') self.assertFalse(stream.is_in_zephyr_realm) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) params = { 'stream_name': ujson.dumps('private_stream'), 'is_private': ujson.dumps(False) @@ -368,7 +368,7 @@ class StreamAdminTest(ZulipTestCase): realm = user_profile.realm self.make_stream('public_stream', realm=realm) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) params = { 'stream_name': ujson.dumps('public_stream'), 'is_private': ujson.dumps(True) @@ -387,7 +387,7 @@ class StreamAdminTest(ZulipTestCase): self.make_stream('target_stream', realm=realm, invite_only=True) self.subscribe(user_profile, 'target_stream') - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) params = { 'stream_name': ujson.dumps('target_stream'), 'is_private': ujson.dumps(False) @@ -406,7 +406,7 @@ class StreamAdminTest(ZulipTestCase): realm = user_profile.realm self.make_stream('public_history_stream', realm=realm) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) params = { 'stream_name': ujson.dumps('public_history_stream'), 'is_private': ujson.dumps(True), @@ -425,7 +425,7 @@ class StreamAdminTest(ZulipTestCase): realm = user_profile.realm self.make_stream('public_stream', realm=realm) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) params = { 'stream_name': ujson.dumps('public_stream'), 'is_private': ujson.dumps(False), @@ -443,7 +443,7 @@ class StreamAdminTest(ZulipTestCase): self.login_user(user_profile) stream = self.make_stream('new_stream') self.subscribe(user_profile, stream.name) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) result = self.client_delete('/json/streams/%d' % (stream.id,)) self.assert_json_success(result) @@ -499,7 +499,7 @@ class StreamAdminTest(ZulipTestCase): user_profile = self.example_user('hamlet') self.login_user(user_profile) self.make_stream('new_stream') - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) result = self.client_delete('/json/streams/999999999') self.assert_json_error(result, 'Invalid stream id') @@ -517,7 +517,7 @@ class StreamAdminTest(ZulipTestCase): user_profile = self.example_user('hamlet') self.login_user(user_profile) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) self.make_stream('private_stream', invite_only=True) self.subscribe(user_profile, 'private_stream') @@ -567,7 +567,7 @@ class StreamAdminTest(ZulipTestCase): self.login_user(user_profile) realm = user_profile.realm stream = self.subscribe(user_profile, 'stream_name1') - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) result = self.client_patch('/json/streams/%d' % (stream.id,), {'new_name': ujson.dumps('stream_name1')}) @@ -692,7 +692,7 @@ class StreamAdminTest(ZulipTestCase): self.make_stream('stream_name1') stream = self.subscribe(user_profile, 'stream_name1') - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) result = self.client_patch('/json/streams/%d' % (stream.id,), {'new_name': ujson.dumps('stream_name2')}) self.assert_json_success(result) @@ -796,7 +796,7 @@ class StreamAdminTest(ZulipTestCase): self.login_user(user_profile) self.subscribe(user_profile, 'stream_name1') - do_change_is_admin(user_profile, False) + do_change_user_role(user_profile, UserProfile.ROLE_MEMBER) stream_id = get_stream('stream_name1', user_profile.realm).id result = self.client_patch('/json/streams/%d' % (stream_id,), @@ -808,7 +808,7 @@ class StreamAdminTest(ZulipTestCase): self.login_user(user_profile) self.subscribe(user_profile, 'stream_name1') - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) stream_id = get_stream('stream_name1', user_profile.realm).id result = self.client_patch('/json/streams/%d' % (stream_id,), @@ -822,7 +822,7 @@ class StreamAdminTest(ZulipTestCase): self.login_user(user_profile) self.subscribe(user_profile, 'stream_name1') - do_change_is_admin(user_profile, False) + do_change_user_role(user_profile, UserProfile.ROLE_MEMBER) do_set_realm_property(user_profile.realm, 'waiting_period_threshold', 10) @@ -841,7 +841,7 @@ class StreamAdminTest(ZulipTestCase): test_non_admin(how_old=15, is_new=False, policy=policy) test_non_admin(how_old=5, is_new=True, policy=policy) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) for policy in policies: stream_id = get_stream('stream_name1', user_profile.realm).id @@ -864,7 +864,7 @@ class StreamAdminTest(ZulipTestCase): if subscribed: self.subscribe(user_profile, stream_name) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) return stream @@ -937,7 +937,7 @@ class StreamAdminTest(ZulipTestCase): # Even becoming a realm admin doesn't help us for an out-of-realm # stream. - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) result = self.client_delete('/json/streams/' + str(stream.id)) self.assert_json_error(result, 'Invalid stream id') @@ -1074,7 +1074,7 @@ class StreamAdminTest(ZulipTestCase): user_profile.date_joined = timezone_now() user_profile.save() self.login_user(user_profile) - do_change_is_admin(user_profile, False) + do_change_user_role(user_profile, UserProfile.ROLE_MEMBER) # Allow all members to create streams. do_set_realm_property(user_profile.realm, 'create_stream_policy', @@ -1098,7 +1098,7 @@ class StreamAdminTest(ZulipTestCase): self.assert_json_error(result, 'User cannot create streams.') # Make current user an admin. - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) # Can successfully create stream as user is now an admin. stream_name = ['admins_only'] @@ -1115,7 +1115,7 @@ class StreamAdminTest(ZulipTestCase): self.assert_json_success(result) # Make current user no longer an admin. - do_change_is_admin(user_profile, False) + do_change_user_role(user_profile, UserProfile.ROLE_MEMBER) # Cannot create stream because user is not an admin and is not older than the waiting # period. @@ -1150,7 +1150,7 @@ class StreamAdminTest(ZulipTestCase): cordelia_email = cordelia_user.email self.login_user(hamlet_user) - do_change_is_admin(hamlet_user, True) + do_change_user_role(hamlet_user, UserProfile.ROLE_REALM_ADMINISTRATOR) # Hamlet creates a stream as an admin.. stream_name = ['waitingperiodtest'] @@ -1158,7 +1158,7 @@ class StreamAdminTest(ZulipTestCase): self.assert_json_success(result) # Can only invite users to stream if their account is ten days old.. - do_change_is_admin(hamlet_user, False) + do_change_user_role(hamlet_user, UserProfile.ROLE_MEMBER) do_set_realm_property(hamlet_user.realm, 'waiting_period_threshold', 10) # Attempt and fail to invite Cordelia to the stream.. @@ -1243,7 +1243,7 @@ class DefaultStreamTest(ZulipTestCase): def test_api_calls(self) -> None: user_profile = self.example_user('hamlet') - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) self.login_user(user_profile) stream_name = 'stream ADDED via api' @@ -1376,7 +1376,7 @@ class DefaultStreamGroupTest(ZulipTestCase): self.login('hamlet') user_profile = self.example_user('hamlet') realm = user_profile.realm - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) # Test creating new default stream group stream_names = ["stream1", "stream2", "stream3"] @@ -2387,14 +2387,14 @@ class SubscriptionAPITest(ZulipTestCase): enough. """ othello = self.example_user('othello') - do_change_is_admin(othello, True) + do_change_user_role(othello, UserProfile.ROLE_REALM_ADMINISTRATOR) self.assertTrue(othello.can_subscribe_other_users()) - do_change_is_admin(othello, False) - do_change_is_guest(othello, True) + do_change_user_role(othello, UserProfile.ROLE_MEMBER) + do_change_user_role(othello, UserProfile.ROLE_GUEST) self.assertFalse(othello.can_subscribe_other_users()) - do_change_is_guest(othello, False) + do_change_user_role(othello, UserProfile.ROLE_MEMBER) do_set_realm_property(othello.realm, "waiting_period_threshold", 1000) do_set_realm_property(othello.realm, "invite_to_stream_policy", Realm.POLICY_FULL_MEMBERS_ONLY) diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 9e01c56d8b..d23caa2fe2 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -33,7 +33,7 @@ from zerver.lib.actions import ( get_recipient_info, do_deactivate_user, do_reactivate_user, - do_change_is_admin, + do_change_user_role, do_create_user, do_set_realm_property, ) @@ -60,23 +60,6 @@ def find_dict(lst: Iterable[Dict[K, V]], k: K, v: V) -> Dict[K, V]: raise AssertionError('Cannot find element in list where key %s == %s' % (k, v)) class PermissionTest(ZulipTestCase): - def test_do_change_is_admin(self) -> None: - """ - Ensures change_is_admin raises an AssertionError when invalid permissions - are provided to it. - """ - - # this should work fine - user_profile = self.example_user('hamlet') - do_change_is_admin(user_profile, True) - - # this should work a-ok as well - do_change_is_admin(user_profile, True, permission='administer') - - # this should "fail" with an AssertionError - with self.assertRaises(AssertionError): - do_change_is_admin(user_profile, True, permission='totally-not-valid-perm') - def test_role_setters(self) -> None: user_profile = self.example_user('hamlet') @@ -106,13 +89,13 @@ class PermissionTest(ZulipTestCase): def test_get_admin_users(self) -> None: user_profile = self.example_user('hamlet') - do_change_is_admin(user_profile, False) + do_change_user_role(user_profile, UserProfile.ROLE_MEMBER) admin_users = user_profile.realm.get_human_admin_users() self.assertFalse(user_profile in admin_users) admin_users = user_profile.realm.get_admin_users_and_bots() self.assertFalse(user_profile in admin_users) - do_change_is_admin(user_profile, True) + do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) admin_users = user_profile.realm.get_human_admin_users() self.assertTrue(user_profile in admin_users) admin_users = user_profile.realm.get_admin_users_and_bots() @@ -121,7 +104,7 @@ class PermissionTest(ZulipTestCase): def test_updating_non_existent_user(self) -> None: self.login('hamlet') admin = self.example_user('hamlet') - do_change_is_admin(admin, True) + do_change_user_role(admin, UserProfile.ROLE_REALM_ADMINISTRATOR) invalid_user_id = 1000 result = self.client_patch('/json/users/{}'.format(invalid_user_id), {}) @@ -406,7 +389,7 @@ class PermissionTest(ZulipTestCase): iago = self.example_user("iago") self.login_user(iago) hamlet = self.example_user("hamlet") - do_change_is_admin(hamlet, True) + do_change_user_role(hamlet, UserProfile.ROLE_REALM_ADMINISTRATOR) self.assertFalse(hamlet.is_guest) self.assertTrue(hamlet.is_realm_admin) @@ -654,7 +637,7 @@ class AdminCreateUserTest(ZulipTestCase): admin = self.example_user('hamlet') realm = admin.realm self.login_user(admin) - do_change_is_admin(admin, True) + do_change_user_role(admin, UserProfile.ROLE_REALM_ADMINISTRATOR) result = self.client_post("/json/users", dict()) self.assert_json_error(result, "Missing 'email' argument") @@ -997,7 +980,7 @@ class ActivateTest(ZulipTestCase): def test_api(self) -> None: admin = self.example_user('othello') - do_change_is_admin(admin, True) + do_change_user_role(admin, UserProfile.ROLE_REALM_ADMINISTRATOR) self.login('othello') user = self.example_user('hamlet') @@ -1042,7 +1025,7 @@ class ActivateTest(ZulipTestCase): def test_api_with_insufficient_permissions(self) -> None: non_admin = self.example_user('othello') - do_change_is_admin(non_admin, False) + do_change_user_role(non_admin, UserProfile.ROLE_MEMBER) self.login('othello') # Cannot deactivate a user with the users api diff --git a/zerver/views/users.py b/zerver/views/users.py index 9347c17482..c2807cdb74 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -9,11 +9,11 @@ from django.conf import settings from zerver.decorator import require_realm_admin, require_member_or_admin from zerver.forms import CreateUserForm, PASSWORD_TOO_WEAK_ERROR from zerver.lib.actions import do_change_avatar_fields, do_change_bot_owner, \ - do_change_is_admin, do_change_default_all_public_streams, \ + do_change_user_role, do_change_default_all_public_streams, \ do_change_default_events_register_stream, do_change_default_sending_stream, \ do_create_user, do_deactivate_user, do_reactivate_user, do_regenerate_api_key, \ check_change_full_name, notify_created_bot, do_update_outgoing_webhook_service, \ - do_update_bot_config_data, check_change_bot_full_name, do_change_is_guest, \ + do_update_bot_config_data, check_change_bot_full_name, \ do_update_user_custom_profile_data_if_changed, check_remove_custom_profile_field_value from zerver.lib.avatar import avatar_url, get_gravatar_url from zerver.lib.bot_config import set_bot_config @@ -95,13 +95,22 @@ def update_user_backend(request: HttpRequest, user_profile: UserProfile, user_id ((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')) - do_change_is_admin(target, is_admin) + role = UserProfile.ROLE_MEMBER + if is_admin: + role = UserProfile.ROLE_REALM_ADMINISTRATOR if is_guest is not None and target.is_guest != is_guest: - do_change_is_guest(target, 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: + do_change_user_role(target, role) if (full_name is not None and target.full_name != full_name and full_name.strip() != ""): diff --git a/zilencer/management/commands/populate_db.py b/zilencer/management/commands/populate_db.py index 5eb148dca4..1cb86a3551 100644 --- a/zilencer/management/commands/populate_db.py +++ b/zilencer/management/commands/populate_db.py @@ -16,7 +16,7 @@ from django.utils.timezone import timedelta as timezone_timedelta import pylibmc from zerver.lib.actions import STREAM_ASSIGNMENT_COLORS, check_add_realm_emoji, \ - do_change_is_admin, do_send_messages, do_update_user_custom_profile_data_if_changed, \ + do_change_user_role, do_send_messages, do_update_user_custom_profile_data_if_changed, \ try_add_realm_custom_profile_field, try_add_realm_default_custom_profile_field from zerver.lib.bulk_create import bulk_create_streams from zerver.lib.cache import cache_set @@ -278,12 +278,12 @@ class Command(BaseCommand): create_users(zulip_realm, names, tos_version=settings.TOS_VERSION) iago = get_user_by_delivery_email("iago@zulip.com", zulip_realm) - do_change_is_admin(iago, True) + do_change_user_role(iago, UserProfile.ROLE_REALM_ADMINISTRATOR) iago.is_staff = True iago.save(update_fields=['is_staff']) desdemona = get_user_by_delivery_email("desdemona@zulip.com", zulip_realm) - do_change_is_admin(desdemona, True) + do_change_user_role(desdemona, UserProfile.ROLE_REALM_ADMINISTRATOR) guest_user = get_user_by_delivery_email("polonius@zulip.com", zulip_realm) guest_user.role = UserProfile.ROLE_GUEST