Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

users: Clarify readability issues related to access_user_by_id. 

zerver/lib/users.py has a function named access_user_by_id, which is
used in /users views to fetch a user by it's id. Along with fetching
the user this function also does important validations regarding
checking of required permissions for fetching the target user.

In an attempt to solve the above problem this commit introduces
following changes:
1. Make all the parameters except user_profile, target_user_id
   to be keyword only.
2. Use for_admin parameter instead of read_only.
3. Adds a documentary note to the function describing the reason for
   changes along with recommended way to call this function in future.
4. Changes in views and tests to call this function in this changed
   format.

Changes were tested using ./tools/test-backend.

Fixes #17111.
This commit is contained in:
m-e-l-u-h-a-n 2021-01-28 22:34:43 +05:30 committed by Tim Abbott
parent abff97df39
commit 0e6343c071
3 changed files with 40 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
zerver/lib/users.py
View File

@ -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):

21
zerver/tests/test_users.py
View File

@ -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")

21
zerver/views/users.py
View File

@ -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)}