From 586a5facc9763e4c55651b7bed38a889502d227a Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sun, 8 Dec 2019 20:08:25 +0100 Subject: [PATCH] models: Add is_realm_admin and is_guest setters. Fixes #13452. The migration from UserProfile.is_realm_admin/UserProfile.is_guest in e10361a832217b331a8b71525378a2eba4882bbe broke our LDAP-based support for setting a user's role via LDAP properties, which relied on setting those fields. Because the django-auth-ldap feature powering that only supports booleans (and in any case, we don't want to expose constants like `ROLE_REALM_ADMINISTRATOR` to the LDAP configuration interface), it makes sense to provide setters for these legacy fields for backwards-compatibility. We lint against using these setters directly in Zulip's codebase directly. The issue with using these is that when changing user's .role we want to create appropriate RealmAuditLog entries and send events. This isn't possible when using these setters - the log entries and events should be created if the role change in the UserProfile is actually save()-ed to the database - and on the level of the setter function, it's not known whether the change will indeed be saved. It would have to be somehow figured out on the level of post_save signal handlers, but it doesn't seem like a good design to have such complexity there, for the sake of setters that generally shouldn't be used anyway - because we prefer the do_change_is_* functions. The purpose of this change is narrowly to handle use cases like the setattr on these boolean properties. --- tools/linter_lib/custom_check.py | 14 ++++++++++++++ zerver/models.py | 18 ++++++++++++++++++ zerver/tests/test_users.py | 27 +++++++++++++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/tools/linter_lib/custom_check.py b/tools/linter_lib/custom_check.py index 0baadeff0a..e0a0e961b7 100644 --- a/tools/linter_lib/custom_check.py +++ b/tools/linter_lib/custom_check.py @@ -500,6 +500,20 @@ python_rules = RuleList( 'include_only': set(["/management/commands/"]), 'description': 'Raise CommandError to exit with failure in management commands', }, + {'pattern': '.is_realm_admin =', + 'description': 'Use do_change_is_admin function rather than setting UserProfile\'s is_realm_admin attribute directly.', + 'exclude': set([ + 'zerver/migrations/0248_userprofile_role_start.py', + 'zerver/tests/test_users.py', + ]), + }, + {'pattern': '.is_guest =', + 'description': 'Use do_change_is_guest function rather than setting UserProfile\'s is_guest attribute directly.', + 'exclude': set([ + 'zerver/migrations/0248_userprofile_role_start.py', + 'zerver/tests/test_users.py', + ]), + }, *whitespace_rules, *comma_whitespace_rule, ], diff --git a/zerver/models.py b/zerver/models.py index 2a53aa26c8..baff0159d6 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1055,10 +1055,28 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): def is_realm_admin(self) -> bool: return self.role == UserProfile.ROLE_REALM_ADMINISTRATOR + @is_realm_admin.setter + def is_realm_admin(self, value: bool) -> None: + if value: + self.role = UserProfile.ROLE_REALM_ADMINISTRATOR + elif self.role == UserProfile.ROLE_REALM_ADMINISTRATOR: + # We need to be careful to not accidentally change + # ROLE_GUEST to ROLE_MEMBER here. + self.role = UserProfile.ROLE_MEMBER + @property def is_guest(self) -> bool: return self.role == UserProfile.ROLE_GUEST + @is_guest.setter + def is_guest(self, value: bool) -> None: + if value: + self.role = UserProfile.ROLE_GUEST + elif self.role == UserProfile.ROLE_GUEST: + # We need to be careful to not accidentally change + # ROLE_REALM_ADMINISTRATOR to ROLE_MEMBER here. + self.role = UserProfile.ROLE_MEMBER + @property def is_incoming_webhook(self) -> bool: return self.bot_type == UserProfile.INCOMING_WEBHOOK_BOT diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 2288b12fd9..dd1338dd38 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -75,6 +75,33 @@ class PermissionTest(ZulipTestCase): 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') + + user_profile.is_realm_admin = True + self.assertEqual(user_profile.is_realm_admin, True) + self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR) + + user_profile.is_guest = False + self.assertEqual(user_profile.is_guest, False) + self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR) + + user_profile.is_realm_admin = False + self.assertEqual(user_profile.is_realm_admin, False) + self.assertEqual(user_profile.role, UserProfile.ROLE_MEMBER) + + user_profile.is_guest = True + self.assertEqual(user_profile.is_guest, True) + self.assertEqual(user_profile.role, UserProfile.ROLE_GUEST) + + user_profile.is_realm_admin = False + self.assertEqual(user_profile.is_guest, True) + self.assertEqual(user_profile.role, UserProfile.ROLE_GUEST) + + user_profile.is_guest = False + self.assertEqual(user_profile.is_guest, False) + self.assertEqual(user_profile.role, UserProfile.ROLE_MEMBER) + def test_get_admin_users(self) -> None: user_profile = self.example_user('hamlet') do_change_is_admin(user_profile, False)