diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 3ca1dece29..a4f9c08a94 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -222,18 +222,28 @@ def access_bot_by_id(user_profile: UserProfile, user_id: int) -> UserProfile: raise JsonableError(_("Insufficient permission")) return target -def access_user_by_id(user_profile: UserProfile, user_id: int, - allow_deactivated: bool=False, allow_bots: bool=False, - read_only: bool=False) -> UserProfile: +def access_user_by_id( + user_profile: UserProfile, + target_user_id: int, + *, + allow_deactivated: bool=False, + allow_bots: bool=False, + for_admin: bool, +) -> UserProfile: + """Master function for accessing another user by ID in API code; + verifies the user ID is in the same realm, and if requested checks + for administrative privileges, with flags for various special + cases. + """ try: - target = get_user_profile_by_id_in_realm(user_id, user_profile.realm) + target = get_user_profile_by_id_in_realm(target_user_id, user_profile.realm) except UserProfile.DoesNotExist: raise JsonableError(_("No such user")) if target.is_bot and not allow_bots: raise JsonableError(_("No such user")) if not target.is_active and not allow_deactivated: raise JsonableError(_("User is deactivated")) - if read_only: + if not for_admin: # Administrative access is not required just to read a user. return target if not user_profile.can_admin_user(target): diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 3d25d07418..dce099283c 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -382,29 +382,30 @@ class PermissionTest(ZulipTestCase): # Must be a valid user ID in the realm with self.assertRaises(JsonableError): - access_user_by_id(iago, 1234) + access_user_by_id(iago, 1234, for_admin=False) with self.assertRaises(JsonableError): - access_user_by_id(iago, self.mit_user("sipbtest").id) + access_user_by_id(iago, self.mit_user("sipbtest").id, for_admin=False) - # Can only access bot users if allow_deactivated is passed + # Can only access bot users if allow_bots is passed bot = self.example_user("default_bot") - access_user_by_id(iago, bot.id, allow_bots=True) + access_user_by_id(iago, bot.id, allow_bots=True, for_admin=True) with self.assertRaises(JsonableError): - access_user_by_id(iago, bot.id) + access_user_by_id(iago, bot.id, for_admin=True) # Can only access deactivated users if allow_deactivated is passed hamlet = self.example_user("hamlet") do_deactivate_user(hamlet) with self.assertRaises(JsonableError): - access_user_by_id(iago, hamlet.id) - access_user_by_id(iago, hamlet.id, allow_deactivated=True) + access_user_by_id(iago, hamlet.id, for_admin=False) + with self.assertRaises(JsonableError): + access_user_by_id(iago, hamlet.id, for_admin=True) + access_user_by_id(iago, hamlet.id, allow_deactivated=True, for_admin=True) # Non-admin user can't admin another user with self.assertRaises(JsonableError): - access_user_by_id(self.example_user("cordelia"), self.example_user("aaron").id) + access_user_by_id(self.example_user("cordelia"), self.example_user("aaron").id, for_admin=True) # But does have read-only access to it. - access_user_by_id(self.example_user("cordelia"), self.example_user("aaron").id, - read_only=True) + access_user_by_id(self.example_user("cordelia"), self.example_user("aaron").id, for_admin=False) def test_change_regular_member_to_guest(self) -> None: iago = self.example_user("iago") diff --git a/zerver/views/users.py b/zerver/views/users.py index cd69742ffd..12b4cf7709 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -89,7 +89,7 @@ def check_last_owner(user_profile: UserProfile) -> bool: def deactivate_user_backend(request: HttpRequest, user_profile: UserProfile, user_id: int) -> HttpResponse: - target = access_user_by_id(user_profile, user_id) + target = access_user_by_id(user_profile, user_id, for_admin=True) if target.is_realm_owner and not user_profile.is_realm_owner: raise OrganizationOwnerRequired() if check_last_owner(target): @@ -117,7 +117,8 @@ def _deactivate_user_profile_backend(request: HttpRequest, user_profile: UserPro def reactivate_user_backend(request: HttpRequest, user_profile: UserProfile, user_id: int) -> HttpResponse: - target = access_user_by_id(user_profile, user_id, allow_deactivated=True, allow_bots=True) + target = access_user_by_id(user_profile, user_id, + allow_deactivated=True, allow_bots=True, for_admin=True) if target.is_bot: assert target.bot_type is not None check_bot_creation_policy(user_profile, target.bot_type) @@ -144,13 +145,19 @@ def update_user_backend( default=None, validator=check_profile_data, ), ) -> HttpResponse: - target = access_user_by_id(user_profile, user_id, allow_deactivated=True, allow_bots=True) + target = access_user_by_id(user_profile, user_id, + allow_deactivated=True, allow_bots=True, for_admin=True) if role is not None and target.role != role: - if target.role == UserProfile.ROLE_REALM_OWNER and check_last_owner(user_profile): - return json_error(_('The owner permission cannot be removed from the only organization owner.')) + # Require that the current user has permissions to + # grant/remove the role in question. access_user_by_id has + # already verified we're an administrator; here we enforce + # that only owners can toggle the is_realm_owner flag. if UserProfile.ROLE_REALM_OWNER in [role, target.role] and not user_profile.is_realm_owner: raise OrganizationOwnerRequired() + + if target.role == UserProfile.ROLE_REALM_OWNER and check_last_owner(user_profile): + return json_error(_('The owner permission cannot be removed from the only organization owner.')) do_change_user_role(target, role, acting_user=user_profile) if (full_name is not None and target.full_name != full_name and @@ -470,7 +477,7 @@ def get_members_backend(request: HttpRequest, user_profile: UserProfile, user_id target_user = None if user_id is not None: target_user = access_user_by_id(user_profile, user_id, allow_deactivated=True, - allow_bots=True, read_only=True) + allow_bots=True, for_admin=False) members = get_raw_user_data(realm, user_profile, target_user=target_user, client_gravatar=client_gravatar, @@ -548,7 +555,7 @@ def get_subscription_backend(request: HttpRequest, user_profile: UserProfile, user_id: int=REQ(validator=check_int, path_only=True), stream_id: int=REQ(validator=check_int, path_only=True), ) -> HttpResponse: - target_user = access_user_by_id(user_profile, user_id, read_only=True) + target_user = access_user_by_id(user_profile, user_id, for_admin=False) (stream, sub) = access_stream_by_id(user_profile, stream_id) subscription_status = {'is_subscribed': subscribed_to_stream(target_user, stream_id)}