From 8cf104b6430e316f465324a95b96e986fc61365c Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 13 Aug 2018 10:09:09 -0700 Subject: [PATCH] avatar: Allow API authentication for /avatar/ routes. This makes it feasibly for the mobile apps to correctly render user avatars generated by the `!avatar()` syntax. --- zerver/tests/test_upload.py | 47 +++++++++++++++++++++++++++++++++++-- zerver/views/users.py | 15 +++++------- zproject/urls.py | 35 +++++++++++++++------------ 3 files changed, 71 insertions(+), 26 deletions(-) diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index 467f45e2a8..8368113483 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -862,8 +862,10 @@ class AvatarTest(UploadSerializeMixin, ZulipTestCase): self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia)) + '&foo=bar')) def test_get_user_avatar(self) -> None: - self.login(self.example_email("hamlet")) + hamlet = self.example_email("hamlet") + self.login(hamlet) cordelia = self.example_user('cordelia') + cross_realm_bot = self.example_user('welcome_bot') cordelia.avatar_source = UserProfile.AVATAR_FROM_USER cordelia.save() @@ -878,8 +880,34 @@ class AvatarTest(UploadSerializeMixin, ZulipTestCase): response = self.client_get("/avatar/") self.assertEqual(response.status_code, 404) + 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')) + + response = self.api_get(hamlet, "/avatar/%s?foo=bar" % (cordelia.id)) + 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 id. + response = self.api_get(hamlet, "/avatar/%s?foo=bar" % (cross_realm_bot.id)) + redirect_url = response['Location'] + self.assertTrue(redirect_url.endswith(str(avatar_url(cross_realm_bot)) + '&foo=bar')) + + 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) + def test_get_user_avatar_medium(self) -> None: - self.login(self.example_email("hamlet")) + hamlet = self.example_email("hamlet") + self.login(hamlet) cordelia = self.example_user('cordelia') cordelia.avatar_source = UserProfile.AVATAR_FROM_USER @@ -892,6 +920,21 @@ class AvatarTest(UploadSerializeMixin, ZulipTestCase): redirect_url = response['Location'] self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia, True)) + '&foo=bar')) + 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')) + + response = self.api_get(hamlet, "/avatar/%s/medium?foo=bar" % (cordelia.id,)) + redirect_url = response['Location'] + self.assertTrue(redirect_url.endswith(str(avatar_url(cordelia, True)) + '&foo=bar')) + + 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) + def test_non_valid_user_avatar(self) -> None: # It's debatable whether we should generate avatars for non-users, diff --git a/zerver/views/users.py b/zerver/views/users.py index 08e13f1527..ea7d0199f8 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -34,9 +34,9 @@ from zerver.lib.users import check_valid_bot_type, check_bot_creation_policy, \ access_bot_by_id, add_service, access_user_by_id from zerver.lib.utils import generate_api_key, generate_random_token from zerver.models import UserProfile, Stream, Message, email_allowed_for_realm, \ - get_user_profile_by_id, get_user, Service, get_user_including_cross_realm, \ + get_user, Service, get_user_including_cross_realm, \ DomainNotAllowedForRealmError, DisposableEmailError, get_user_profile_by_id_in_realm, \ - EmailContainsPlusError + EmailContainsPlusError, get_user_by_id_in_realm_including_cross_realm def deactivate_user_backend(request: HttpRequest, user_profile: UserProfile, user_id: int) -> HttpResponse: @@ -94,11 +94,8 @@ def update_user_backend(request: HttpRequest, user_profile: UserProfile, user_id return json_success() -# TODO: Since eventually we want to support using the same email with -# different organizations, we'll eventually want this to be a -# logged-in endpoint so that we can access the realm_id. -@zulip_login_required -def avatar(request: HttpRequest, email_or_id: str, medium: bool=False) -> HttpResponse: +def avatar(request: HttpRequest, user_profile: UserProfile, + email_or_id: str, medium: bool=False) -> HttpResponse: """Accepts an email address or user ID and returns the avatar""" is_email = False try: @@ -107,11 +104,11 @@ def avatar(request: HttpRequest, email_or_id: str, medium: bool=False) -> HttpRe is_email = True try: + realm = user_profile.realm if is_email: - realm = request.user.realm avatar_user_profile = get_user_including_cross_realm(email_or_id, realm) else: - avatar_user_profile = get_user_profile_by_id(email_or_id) + avatar_user_profile = get_user_by_id_in_realm_including_cross_realm(int(email_or_id), realm) # If there is a valid user account passed in, use its avatar url = avatar_url(avatar_user_profile, medium=medium) except UserProfile.DoesNotExist: diff --git a/zproject/urls.py b/zproject/urls.py index 0b28a871f9..d88a608b80 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -413,12 +413,6 @@ i18n_urls = [ zerver.views.auth.show_deactivation_notice, name='zerver.views.auth.show_deactivation_notice'), - # Avatar - url(r'^avatar/(?P[\S]+)/(?P[\S]+)?', zerver.views.users.avatar, - name='zerver.views.users.avatar'), - url(r'^avatar/(?P[\S]+)', zerver.views.users.avatar, - name='zerver.views.users.avatar'), - # Registration views, require a confirmation ID. url(r'^accounts/register/social/(\w+)$', zerver.views.auth.start_social_signup, @@ -539,15 +533,26 @@ urls += [ # easily support the mobile apps fetching uploaded files without # having to rewrite URLs, and is implemented using the # 'override_api_url_scheme' flag passed to rest_dispatch -urls += url(r'^user_uploads/(?P(\d*|unk))/(?P.*)', - rest_dispatch, - {'GET': ('zerver.views.upload.serve_file_backend', - {'override_api_url_scheme'})}), -# This endpoint serves thumbnailed versions of images using thumbor; -# it requires an exception for the same reason. -urls += url(r'^thumbnail', rest_dispatch, - {'GET': ('zerver.views.thumbnail.backend_serve_thumbnail', - {'override_api_url_scheme'})}), +urls += [ + url(r'^user_uploads/(?P(\d*|unk))/(?P.*)', + rest_dispatch, + {'GET': ('zerver.views.upload.serve_file_backend', + {'override_api_url_scheme'})}), + # This endpoint serves thumbnailed versions of images using thumbor; + # it requires an exception for the same reason. + url(r'^thumbnail', rest_dispatch, + {'GET': ('zerver.views.thumbnail.backend_serve_thumbnail', + {'override_api_url_scheme'})}), + # Avatars have the same constraint due to `!avatar` syntax. + url(r'^avatar/(?P[\S]+)/(?P[\S]+)?', + rest_dispatch, + {'GET': ('zerver.views.users.avatar', + {'override_api_url_scheme'})}), + url(r'^avatar/(?P[\S]+)', + rest_dispatch, + {'GET': ('zerver.views.users.avatar', + {'override_api_url_scheme'})}), +] # This url serves as a way to recieve CSP violation reports from the users. # We use this endpoint to just log these reports.