From 7fc9fc32d13bb39d3f4a778e4b1d5b9710977bbd Mon Sep 17 00:00:00 2001 From: PieterCK Date: Mon, 4 Nov 2024 13:15:15 +0700 Subject: [PATCH] avatars: Split email-based and user ID-based avatar endpoints. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Separate `avatars//medium?` endpoints into distinct endpoints for email-based and user ID-based access. This change aligns avatar endpoints with Zulip’s existing API path conventions (e.g., the `users/` endpoint). --- zerver/tests/test_upload.py | 14 +++++++ zerver/views/users.py | 75 ++++++++++++++++++++++++------------- zproject/urls.py | 20 ++++++++-- 3 files changed, 79 insertions(+), 30 deletions(-) diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index ae216361a1..c1981ea875 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -1275,6 +1275,14 @@ class AvatarTest(UploadSerializeMixin, ZulipTestCase): "Not logged in: API authentication or user session required", status_code=401, ) + # Disallow access by id for spectators with unauthenticated access + # when realm public streams is false. + response = self.client_get(f"/avatar/{cordelia.id}", {"foo": "bar"}) + self.assert_json_error( + response, + "Not logged in: API authentication or user session required", + status_code=401, + ) # Allow unauthenticated/spectator requests by ID. response = self.client_get(f"/avatar/{cordelia.id}", {"foo": "bar"}) @@ -1301,6 +1309,12 @@ class AvatarTest(UploadSerializeMixin, ZulipTestCase): redirect_url = response["Location"] self.assertTrue(redirect_url.endswith("images/unknown-user-avatar.png?foo=bar")) + invalid_user_id = 999 + response = self.client_get(f"/avatar/{invalid_user_id}", {"foo": "bar"}) + self.assertEqual(302, response.status_code) + redirect_url = response["Location"] + self.assertTrue(redirect_url.endswith("images/unknown-user-avatar.png?foo=bar")) + def test_get_user_avatar_medium(self) -> None: hamlet = self.example_user("hamlet") self.login_user(hamlet) diff --git a/zerver/views/users.py b/zerver/views/users.py index 6a45fefef4..b0082acbd4 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -336,18 +336,13 @@ def update_user_backend( return json_success(request) -def avatar( +def avatar_by_id( request: HttpRequest, maybe_user_profile: UserProfile | AnonymousUser, - email_or_id: str, + user_id: int, medium: bool = False, ) -> HttpResponse: - """Accepts an email address or user ID and returns the avatar""" - is_email = False - try: - int(email_or_id) - except ValueError: - is_email = True + """Accepts a user ID and returns the avatar""" if not maybe_user_profile.is_authenticated: # Allow anonymous access to avatars only if spectators are @@ -356,27 +351,14 @@ def avatar( if not realm.allow_web_public_streams_access(): raise MissingAuthenticationError - # We only allow the ID format for accessing a user's avatar - # for spectators. This is mainly for defense in depth, since - # email_address_visibility should mean spectators only - # interact with fake email addresses anyway. - if is_email: - raise MissingAuthenticationError - if settings.RATE_LIMITING: - unique_avatar_key = f"{realm.id}/{email_or_id}/{medium}" + unique_avatar_key = f"{realm.id}/{user_id}/{medium}" rate_limit_spectator_attachment_access_by_file(unique_avatar_key) else: realm = maybe_user_profile.realm try: - if is_email: - avatar_user_profile = get_user_including_cross_realm(email_or_id, realm) - else: - avatar_user_profile = get_user_by_id_in_realm_including_cross_realm( - int(email_or_id), realm - ) - + avatar_user_profile = get_user_by_id_in_realm_including_cross_realm(user_id, realm) url: str | None = None if maybe_user_profile.is_authenticated and not check_can_access_user( avatar_user_profile, maybe_user_profile @@ -386,9 +368,43 @@ def avatar( # If there is a valid user account passed in, use its avatar url = avatar_url(avatar_user_profile, medium=medium) assert url is not None + except UserProfile.DoesNotExist: + url = get_avatar_for_inaccessible_user() + + assert url is not None + if request.META["QUERY_STRING"]: + url = append_url_query_string(url, request.META["QUERY_STRING"]) + return redirect(url) + + +def avatar_by_email( + request: HttpRequest, + maybe_user_profile: UserProfile | AnonymousUser, + email: str, + medium: bool = False, +) -> HttpResponse: + """Accepts an email address and returns the avatar""" + + if not maybe_user_profile.is_authenticated: + # We only allow the ID format for accessing a user's avatar + # for spectators. This is mainly for defense in depth, since + # email_address_visibility should mean spectators only + # interact with fake email addresses anyway. + raise MissingAuthenticationError + + realm = maybe_user_profile.realm + + try: + avatar_user_profile = get_user_including_cross_realm(email, realm) + url: str | None = None + if not check_can_access_user(avatar_user_profile, maybe_user_profile): + url = get_avatar_for_inaccessible_user() + else: + # If there is a valid user account passed in, use its avatar + url = avatar_url(avatar_user_profile, medium=medium) + assert url is not None except UserProfile.DoesNotExist: # If there is no such user, treat it as a new gravatar - email = email_or_id avatar_version = 1 url = get_gravatar_url(email, avatar_version, medium) @@ -399,9 +415,16 @@ def avatar( def avatar_medium( - request: HttpRequest, maybe_user_profile: UserProfile | AnonymousUser, email_or_id: str + request: HttpRequest, + maybe_user_profile: UserProfile | AnonymousUser, + email: str | None = None, + user_id: int | None = None, ) -> HttpResponse: - return avatar(request, maybe_user_profile, email_or_id, medium=True) + if email: + return avatar_by_email(request, maybe_user_profile, email, medium=True) + else: + assert user_id is not None + return avatar_by_id(request, maybe_user_profile, user_id, medium=True) def get_stream_name(stream: Stream | None) -> str | None: diff --git a/zproject/urls.py b/zproject/urls.py index 0703e9f104..e68e696a77 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -218,7 +218,8 @@ from zerver.views.user_settings import ( from zerver.views.user_topics import update_muted_topic, update_user_topic from zerver.views.users import ( add_bot_backend, - avatar, + avatar_by_email, + avatar_by_id, avatar_medium, create_user_backend, deactivate_bot_backend, @@ -680,11 +681,22 @@ urls += [ # Avatars have the same constraint because their URLs are included # in API data structures used by both the mobile and web clients. rest_path( - "avatar/", - GET=(avatar, {"override_api_url_scheme", "allow_anonymous_user_web"}), + "avatar/", + GET=(avatar_by_id, {"override_api_url_scheme", "allow_anonymous_user_web"}), ), rest_path( - "avatar//medium", + "avatar/", + GET=(avatar_by_email, {"override_api_url_scheme", "allow_anonymous_user_web"}), + ), + rest_path( + "avatar//medium", + GET=( + avatar_medium, + {"override_api_url_scheme", "allow_anonymous_user_web"}, + ), + ), + rest_path( + "avatar//medium", GET=( avatar_medium, {"override_api_url_scheme", "allow_anonymous_user_web"},