From ed6d62a9e70b97d49eba276c8ae555835ecedd6c Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Tue, 6 Dec 2022 21:24:43 +0000 Subject: [PATCH] avatars: Serve /user_avatars/ through Django, which offloads to nginx. Moving `/user_avatars/` to being served partially through Django removes the need for the `no_serve_uploads` nginx reconfiguring when switching between S3 and local backends. This is important because a subsequent commit will move S3 attachments to being served through nginx, which would make `no_serve_uploads` entirely nonsensical of a name. Serve the files through Django, with an offload for the actual image response to an internal nginx route. In development, serve the files directly in Django. We do _not_ mark the contents as immutable for caching purposes, since the path for avatar images is hashed only by their user-id and a salt, and as such are reused when a user's avatar is updated. --- docs/production/deployment.md | 9 ---- docs/production/upload-backends.md | 20 ------- .../uploads-internal.conf | 3 +- puppet/zulip/manifests/app_frontend_base.pp | 44 ++++++--------- zerver/lib/test_runner.py | 8 --- zerver/tests/test_upload.py | 53 +++++++++++++++++++ zerver/views/upload.py | 39 +++++++++++++- zproject/dev_urls.py | 9 ---- zproject/urls.py | 6 +++ 9 files changed, 114 insertions(+), 77 deletions(-) rename puppet/zulip/files/nginx/{zulip-include-maybe => zulip-include-frontend}/uploads-internal.conf (92%) diff --git a/docs/production/deployment.md b/docs/production/deployment.md index fad5550534..585569c860 100644 --- a/docs/production/deployment.md +++ b/docs/production/deployment.md @@ -661,15 +661,6 @@ SSL/TLS termination. Set to the port number if you [prefer to listen on a port other than 443](#using-an-alternate-port). -#### `no_serve_uploads` - -To enable the [the S3 uploads backend][s3-uploads], one needs to both -configure `settings.py` and set this to true to configure -`nginx`. Remove this field to return to the local uploads backend (any -non-empty value is currently equivalent to true). - -[s3-uploads]: upload-backends.md#s3-backend-configuration - #### `queue_workers_multiprocess` By default, Zulip automatically detects whether the system has enough diff --git a/docs/production/upload-backends.md b/docs/production/upload-backends.md index 92750be624..147a509518 100644 --- a/docs/production/upload-backends.md +++ b/docs/production/upload-backends.md @@ -45,26 +45,6 @@ backend. To enable this backend, you need to do the following: For certain AWS regions, you may need to set the `S3_REGION` setting to your default AWS region's code (e.g. `"eu-central-1"`). -1. You will need to configure `nginx` to direct requests for uploaded - files to the Zulip server (which will then serve a redirect to the - appropriate place in S3), rather than serving them directly. - - With Zulip 1.9.0 and newer, you can do this automatically with the - following commands run as root: - - ```bash - crudini --set /etc/zulip/zulip.conf application_server no_serve_uploads true - /home/zulip/deployments/current/scripts/zulip-puppet-apply - ``` - - (The first line will update your `/etc/zulip/zulip.conf`). - - With older Zulip, you need to edit - `/etc/nginx/sites-available/zulip-enterprise` to comment out the - `nginx` configuration block for `/user_avatars` and the - `include /etc/nginx/zulip-include/uploads.route` line and then - reload the `nginx` service (`service nginx reload`). - 1. Finally, restart the Zulip server so that your settings changes take effect (`/home/zulip/deployments/current/scripts/restart-server`). diff --git a/puppet/zulip/files/nginx/zulip-include-maybe/uploads-internal.conf b/puppet/zulip/files/nginx/zulip-include-frontend/uploads-internal.conf similarity index 92% rename from puppet/zulip/files/nginx/zulip-include-maybe/uploads-internal.conf rename to puppet/zulip/files/nginx/zulip-include-frontend/uploads-internal.conf index a525d4d849..a75bf15642 100644 --- a/puppet/zulip/files/nginx/zulip-include-maybe/uploads-internal.conf +++ b/puppet/zulip/files/nginx/zulip-include-frontend/uploads-internal.conf @@ -6,7 +6,8 @@ location /internal/uploads { alias /home/zulip/uploads/files; } -location /user_avatars { +location /internal/user_avatars { + internal; include /etc/nginx/zulip-include/headers; add_header Content-Security-Policy "default-src 'none' img-src 'self'"; include /etc/nginx/zulip-include/uploads.types; diff --git a/puppet/zulip/manifests/app_frontend_base.pp b/puppet/zulip/manifests/app_frontend_base.pp index 620952bd33..f38b58a688 100644 --- a/puppet/zulip/manifests/app_frontend_base.pp +++ b/puppet/zulip/manifests/app_frontend_base.pp @@ -69,37 +69,23 @@ class zulip::app_frontend_base { notify => Service['nginx'], } - # Configuration for how uploaded files and profile pictures are - # served. The default is to serve uploads using using the `nginx` - # `internal` feature via X-Accel-Redirect, which basically does an - # internal redirect and returns the file content from nginx in an - # HttpResponse that would otherwise have been a redirect. Profile - # pictures are served directly off disk. - # - # For installations using S3 to serve uploaded files, we want Django - # to handle the /internal/uploads and /user_avatars routes, so that it - # can serve a redirect (after doing authentication, for uploads). - $no_serve_uploads = zulipconf('application_server', 'no_serve_uploads', false) - if $no_serve_uploads { - file { '/etc/nginx/zulip-include/app.d/uploads-internal.conf': - ensure => absent, - } - } else { - file { '/etc/nginx/zulip-include/app.d/uploads-internal.conf': - ensure => file, - require => Package[$zulip::common::nginx], - owner => 'root', - group => 'root', - mode => '0644', - notify => Service['nginx'], - source => 'puppet:///modules/zulip/nginx/zulip-include-maybe/uploads-internal.conf', - } + file { '/etc/nginx/zulip-include/app.d/uploads-internal.conf': + ensure => file, + require => Package[$zulip::common::nginx], + owner => 'root', + group => 'root', + mode => '0644', + notify => Service['nginx'], + source => 'puppet:///modules/zulip/nginx/zulip-include-frontend/uploads-internal.conf', } - # TODO/compatibility: Removed 2021-04 in Zulip 4.0; these lines can - # be removed once one must have upgraded through Zulip 4.0 or higher - # to get to the next release. - file { ['/etc/nginx/zulip-include/uploads.route', '/etc/nginx/zulip-include/app.d/thumbor.conf']: + file { [ + # TODO/compatibility: Removed 2021-04 in Zulip 4.0; these lines can + # be removed once one must have upgraded through Zulip 4.0 or higher + # to get to the next release. + '/etc/nginx/zulip-include/uploads.route', + '/etc/nginx/zulip-include/app.d/thumbor.conf', + ]: ensure => absent, } diff --git a/zerver/lib/test_runner.py b/zerver/lib/test_runner.py index f3600e4cba..0e5b4e2b83 100644 --- a/zerver/lib/test_runner.py +++ b/zerver/lib/test_runner.py @@ -208,14 +208,6 @@ def init_worker( create_test_databases(_worker_id) initialize_worker_path(_worker_id) - # We manually update the upload directory path in the URL regex. - from zproject.dev_urls import avatars_url - - assert settings.LOCAL_UPLOADS_DIR is not None - assert settings.LOCAL_AVATARS_DIR is not None - assert avatars_url.default_args is not None - avatars_url.default_args["document_root"] = settings.LOCAL_AVATARS_DIR - class ParallelTestSuite(django_runner.ParallelTestSuite): run_subsuite = run_subsuite diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index 1d6b1fd795..b443377d28 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -8,6 +8,7 @@ import urllib from io import StringIO from unittest import mock from unittest.mock import patch +from urllib.parse import urlparse import botocore.exceptions import orjson @@ -1853,6 +1854,39 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): path_id = re.sub("/user_uploads/", "", response_dict["uri"]) self.assertTrue(delete_message_image(path_id)) + def test_avatar_url_local(self) -> None: + self.login("hamlet") + with get_test_image_file("img.png") as image_file: + result = self.client_post("/json/users/me/avatar", {"file": image_file}) + + response_dict = self.assert_json_success(result) + self.assertIn("avatar_url", response_dict) + base = "/user_avatars/" + url = self.assert_json_success(result)["avatar_url"] + self.assertEqual(base, url[: len(base)]) + + # That URL is accessible when logged out + self.logout() + result = self.client_get(url) + self.assertEqual(result.status_code, 200) + + # We get a resized avatar from it + image_data = read_test_image_file("img.png") + resized_avatar = resize_avatar(image_data) + assert isinstance(result, StreamingHttpResponse) + self.assertEqual(resized_avatar, b"".join(result.streaming_content)) + + with self.settings(DEVELOPMENT=False): + # In production, this is an X-Accel-Redirect to the + # on-disk content, which nginx serves + result = self.client_get(url) + self.assertEqual(result.status_code, 200) + internal_redirect_path = urlparse(url).path.replace( + "/user_avatars/", "/internal/user_avatars/" + ) + self.assertEqual(result["X-Accel-Redirect"], internal_redirect_path) + self.assertEqual(b"", result.content) + def test_ensure_avatar_image_local(self) -> None: user_profile = self.example_user("hamlet") file_path = user_avatar_path(user_profile) @@ -2099,6 +2133,25 @@ class S3Test(ZulipTestCase): body = f"First message ...[zulip.txt](http://{hamlet.realm.host}" + uri + ")" self.send_stream_message(hamlet, "Denmark", body, "test") + @use_s3_backend + def test_user_avatars_redirect(self) -> None: + create_s3_buckets(settings.S3_AVATAR_BUCKET)[0] + self.login("hamlet") + with get_test_image_file("img.png") as image_file: + result = self.client_post("/json/users/me/avatar", {"file": image_file}) + + response_dict = self.assert_json_success(result) + self.assertIn("avatar_url", response_dict) + base = f"https://{settings.S3_AVATAR_BUCKET}.s3.amazonaws.com/" + url = self.assert_json_success(result)["avatar_url"] + self.assertEqual(base, url[: len(base)]) + + # Try hitting the equivalent `/user_avatars` endpoint + wrong_url = "/user_avatars/" + url[len(base) :] + result = self.client_get(wrong_url) + self.assertEqual(result.status_code, 301) + self.assertEqual(result["Location"], url) + @use_s3_backend def test_upload_avatar_image(self) -> None: bucket = create_s3_buckets(settings.S3_AVATAR_BUCKET)[0] diff --git a/zerver/views/upload.py b/zerver/views/upload.py index da6a4bd02d..fb45cbad1f 100644 --- a/zerver/views/upload.py +++ b/zerver/views/upload.py @@ -21,7 +21,11 @@ from django.utils.translation import gettext as _ from zerver.context_processors import get_valid_realm_from_request from zerver.lib.exceptions import JsonableError from zerver.lib.response import json_success -from zerver.lib.upload import check_upload_within_quota, upload_message_image_from_request +from zerver.lib.upload import ( + check_upload_within_quota, + get_public_upload_root_url, + upload_message_image_from_request, +) from zerver.lib.upload.base import INLINE_MIME_TYPES from zerver.lib.upload.local import ( assert_is_local_storage_path, @@ -181,6 +185,39 @@ def serve_local_file_unauthed(request: HttpRequest, token: str, filename: str) - return serve_local(request, path_id, url_only=False) +def serve_local_avatar_unauthed(request: HttpRequest, path: str) -> HttpResponseBase: + """Serves avatar images off disk, via nginx (or directly in dev), with no auth. + + This is done unauthed because these need to be accessed from HTML + emails, where the client does not have any auth. We rely on the + URL being generated using the AVATAR_SALT secret. + + """ + if settings.LOCAL_AVATARS_DIR is None: + # We do not expect clients to hit this URL when using the S3 + # backend; however, there is no reason to not serve the + # redirect to S3 where the content lives. + return redirect( + get_public_upload_root_url() + path + "?" + request.GET.urlencode(), permanent=True + ) + + local_path = os.path.join(settings.LOCAL_AVATARS_DIR, path) + assert_is_local_storage_path("avatars", local_path) + if not os.path.isfile(local_path): + return HttpResponseNotFound("

File not found

") + + if settings.DEVELOPMENT: + response: HttpResponseBase = FileResponse(open(local_path, "rb")) + else: + response = internal_nginx_redirect(quote(f"/internal/user_avatars/{path}")) + + # We do _not_ mark the contents as immutable for caching purposes, + # since the path for avatar images is hashed only by their user-id + # and a salt, and as such are reused when a user's avatar is + # updated. + return response + + def upload_file_backend(request: HttpRequest, user_profile: UserProfile) -> HttpResponse: if len(request.FILES) == 0: raise JsonableError(_("You must specify a file to upload")) diff --git a/zproject/dev_urls.py b/zproject/dev_urls.py index d6fc220a23..77c487be12 100644 --- a/zproject/dev_urls.py +++ b/zproject/dev_urls.py @@ -123,12 +123,3 @@ i18n_urls = [ path("confirmation_key/", confirmation_key), ] urls += i18n_urls - -# On a production instance, these files would be served by nginx. -if settings.LOCAL_AVATARS_DIR is not None: - avatars_url = path( - "user_avatars/", - serve, - {"document_root": os.path.join(settings.LOCAL_AVATARS_DIR)}, - ) - urls += [avatars_url] diff --git a/zproject/urls.py b/zproject/urls.py index ece1c834e2..ee74d927e5 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -166,6 +166,7 @@ from zerver.views.upload import ( serve_file_backend, serve_file_download_backend, serve_file_url_backend, + serve_local_avatar_unauthed, serve_local_file_unauthed, upload_file_backend, ) @@ -668,6 +669,11 @@ urls += [ {"override_api_url_scheme", "allow_anonymous_user_web"}, ), ), + path( + "user_avatars/", + serve_local_avatar_unauthed, + name="local_avatar_unauthed", + ), ] # This URL serves as a way to receive CSP violation reports from the users.