diff --git a/zerver/lib/rest.py b/zerver/lib/rest.py index 3d2f1b1c6e..d14b2e491f 100644 --- a/zerver/lib/rest.py +++ b/zerver/lib/rest.py @@ -147,7 +147,10 @@ def rest_dispatch(request: HttpRequest, **kwargs: Any) -> HttpResponse: target_function = authenticated_rest_api_view( allow_webhook_access="allow_incoming_webhooks" in view_flags, )(target_function) - elif request.path.startswith("/json") and "allow_anonymous_user_web" in view_flags: + elif ( + request.path.startswith(("/json", "/avatar")) + and "allow_anonymous_user_web" in view_flags + ): # For endpoints that support anonymous web access, we do that. # TODO: Allow /api calls when this is stable enough. auth_kwargs = dict(allow_unauthenticated=True) diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index f600f8eb79..5b190de1d8 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -1047,28 +1047,44 @@ class AvatarTest(UploadSerializeMixin, ZulipTestCase): self.logout() - # Test /avatar/ endpoint with HTTP basic auth. - response = self.api_get(hamlet, "/avatar/cordelia@zulip.com", {"foo": "bar"}) - redirect_url = response["Location"] - self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia)) + "&foo=bar")) + with self.settings(WEB_PUBLIC_STREAMS_ENABLED=False): + # Test /avatar/ endpoint with HTTP basic auth. + response = self.api_get(hamlet, "/avatar/cordelia@zulip.com", {"foo": "bar"}) + redirect_url = response["Location"] + self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia)) + "&foo=bar")) - response = self.api_get(hamlet, f"/avatar/{cordelia.id}", {"foo": "bar"}) - redirect_url = response["Location"] - self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia)) + "&foo=bar")) + response = self.api_get(hamlet, f"/avatar/{cordelia.id}", {"foo": "bar"}) + redirect_url = response["Location"] + self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia)) + "&foo=bar")) - # Test cross_realm_bot avatar access using email. - response = self.api_get(hamlet, "/avatar/welcome-bot@zulip.com", {"foo": "bar"}) - redirect_url = response["Location"] - self.assertTrue(redirect_url.endswith(str(avatar_url(cross_realm_bot)) + "&foo=bar")) + # Test cross_realm_bot avatar access using email. + response = self.api_get(hamlet, "/avatar/welcome-bot@zulip.com", {"foo": "bar"}) + redirect_url = response["Location"] + self.assertTrue(redirect_url.endswith(str(avatar_url(cross_realm_bot)) + "&foo=bar")) - # Test cross_realm_bot avatar access using id. - response = self.api_get(hamlet, f"/avatar/{cross_realm_bot.id}", {"foo": "bar"}) - redirect_url = response["Location"] - self.assertTrue(redirect_url.endswith(str(avatar_url(cross_realm_bot)) + "&foo=bar")) + # Test cross_realm_bot avatar access using id. + response = self.api_get(hamlet, f"/avatar/{cross_realm_bot.id}", {"foo": "bar"}) + redirect_url = response["Location"] + self.assertTrue(redirect_url.endswith(str(avatar_url(cross_realm_bot)) + "&foo=bar")) + # Without spectators enabled, no unauthenticated access. + response = self.client_get("/avatar/cordelia@zulip.com", {"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"}) + self.assertEqual(302, response.status_code) + + # Disallow unauthenticated/spectator requests by email. response = self.client_get("/avatar/cordelia@zulip.com", {"foo": "bar"}) self.assert_json_error( - response, "Not logged in: API authentication or user session required", status_code=401 + response, + "Not logged in: API authentication or user session required", + status_code=401, ) def test_get_user_avatar_medium(self) -> None: @@ -1090,18 +1106,34 @@ class AvatarTest(UploadSerializeMixin, ZulipTestCase): self.logout() - # Test /avatar//medium endpoint with HTTP basic auth. - response = self.api_get(hamlet, "/avatar/cordelia@zulip.com/medium", {"foo": "bar"}) - redirect_url = response["Location"] - self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia, True)) + "&foo=bar")) + with self.settings(WEB_PUBLIC_STREAMS_ENABLED=False): + # Test /avatar//medium endpoint with HTTP basic auth. + response = self.api_get(hamlet, "/avatar/cordelia@zulip.com/medium", {"foo": "bar"}) + redirect_url = response["Location"] + self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia, True)) + "&foo=bar")) - response = self.api_get(hamlet, f"/avatar/{cordelia.id}/medium", {"foo": "bar"}) - redirect_url = response["Location"] - self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia, True)) + "&foo=bar")) + response = self.api_get(hamlet, f"/avatar/{cordelia.id}/medium", {"foo": "bar"}) + redirect_url = response["Location"] + self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia, True)) + "&foo=bar")) + # Without spectators enabled, no unauthenticated access. + response = self.client_get("/avatar/cordelia@zulip.com/medium", {"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}/medium", {"foo": "bar"}) + self.assertEqual(302, response.status_code) + + # Disallow unauthenticated/spectator requests by email. response = self.client_get("/avatar/cordelia@zulip.com/medium", {"foo": "bar"}) self.assert_json_error( - response, "Not logged in: API authentication or user session required", status_code=401 + response, + "Not logged in: API authentication or user session required", + status_code=401, ) def test_non_valid_user_avatar(self) -> None: diff --git a/zerver/views/users.py b/zerver/views/users.py index 51e2246393..db75d0c7ba 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -1,10 +1,12 @@ from typing import Any, Dict, List, Optional, Union from django.conf import settings +from django.contrib.auth.models import AnonymousUser from django.http import HttpRequest, HttpResponse from django.shortcuts import redirect from django.utils.translation import gettext as _ +from zerver.context_processors import get_valid_realm_from_request from zerver.decorator import require_member_or_admin, require_realm_admin from zerver.forms import PASSWORD_TOO_WEAK_ERROR, CreateUserForm from zerver.lib.actions import ( @@ -32,6 +34,7 @@ from zerver.lib.email_validation import email_allowed_for_realm from zerver.lib.exceptions import ( CannotDeactivateLastUserError, JsonableError, + MissingAuthenticationError, OrganizationOwnerRequired, ) from zerver.lib.integrations import EMBEDDED_BOTS @@ -219,7 +222,10 @@ def update_user_backend( def avatar( - request: HttpRequest, user_profile: UserProfile, email_or_id: str, medium: bool = False + request: HttpRequest, + maybe_user_profile: Union[UserProfile, AnonymousUser], + email_or_id: str, + medium: bool = False, ) -> HttpResponse: """Accepts an email address or user ID and returns the avatar""" is_email = False @@ -228,8 +234,25 @@ def avatar( except ValueError: is_email = True + if not maybe_user_profile.is_authenticated: + # Allow anonynous access to avatars only if spectators are + # enabled in the organization. + realm = get_valid_realm_from_request(request) + # TODO: Replace with realm.allow_web_public_streams_access() + # when the method is available. + if not realm.has_web_public_streams(): + 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() + else: + realm = maybe_user_profile.realm + try: - realm = user_profile.realm if is_email: avatar_user_profile = get_user_including_cross_realm(email_or_id, realm) else: diff --git a/zproject/urls.py b/zproject/urls.py index 4b7516962b..048870233c 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -669,9 +669,14 @@ urls += [ rest_path("thumbnail", GET=(backend_serve_thumbnail, {"override_api_url_scheme"})), # 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"})), rest_path( - "avatar//medium", {"medium": True}, GET=(avatar, {"override_api_url_scheme"}) + "avatar/", + GET=(avatar, {"override_api_url_scheme", "allow_anonymous_user_web"}), + ), + rest_path( + "avatar//medium", + {"medium": True}, + GET=(avatar, {"override_api_url_scheme", "allow_anonymous_user_web"}), ), ]