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

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

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`).

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;

puppet/zulip/manifests/app_frontend_base.pp

@@ -69,22 +69,6 @@ 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],
@@ -92,14 +76,16 @@ class zulip::app_frontend_base {
     group   => 'root',
     mode    => '0644',
     notify  => Service['nginx'],
-      source  => 'puppet:///modules/zulip/nginx/zulip-include-maybe/uploads-internal.conf',
-    }
+    source  => 'puppet:///modules/zulip/nginx/zulip-include-frontend/uploads-internal.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.
-  file { ['/etc/nginx/zulip-include/uploads.route', '/etc/nginx/zulip-include/app.d/thumbor.conf']:
+    '/etc/nginx/zulip-include/uploads.route',
+    '/etc/nginx/zulip-include/app.d/thumbor.conf',
+  ]:
     ensure => absent,
   }
 

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

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]

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("<p>File not found</p>")
+
+    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"))

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/<path:path>",
-        serve,
-        {"document_root": os.path.join(settings.LOCAL_AVATARS_DIR)},
-    )
-    urls += [avatars_url]

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/<path:path>",
+        serve_local_avatar_unauthed,
+        name="local_avatar_unauthed",
+    ),
 ]
 
 # This URL serves as a way to receive CSP violation reports from the users.