From 82960d9bc2fdc118ec3ede3cd83c4a22c17a847e Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@zulip.com>
Date: Tue, 21 Nov 2023 21:23:22 +0000
Subject: [PATCH] upload: Redirect unauthorized anonymous requests to login.

Note that this also redirects rate-limited anonymous requests to the
login page, as we do not currently differentiate the cases.
---
 zerver/tests/test_upload.py    | 21 +++++++++++++--------
 zerver/tests/test_upload_s3.py |  3 ++-
 zerver/views/upload.py         |  3 +++
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py
index 3476d315b6..403b5277f0 100644
--- a/zerver/tests/test_upload.py
+++ b/zerver/tests/test_upload.py
@@ -216,9 +216,10 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
         # without being logged in.
         self.logout()
         self.assertEqual(self.client_get(url_only_url).getvalue(), b"zulip!")
-        # The original url shouldn't work when logged out:
+        # The original url shouldn't work when logged out -- it redirects to the login page
         result = self.client_get(url)
-        self.assertEqual(result.status_code, 403)
+        self.assertEqual(result.status_code, 302)
+        self.assertTrue(result.headers["Location"].endswith(f"/login/?next={url}"))
 
     def test_serve_file_unauthed(self) -> None:
         self.login("hamlet")
@@ -237,7 +238,8 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
 
             self.logout()
             response = self.client_get(url)
-            self.assertEqual(response.status_code, 403)
+            self.assertEqual(response.status_code, 302)
+            self.assertTrue(response.headers["Location"].endswith(f"/login/?next={url}"))
 
             # Allow file access for web-public stream
             self.login("hamlet")
@@ -253,7 +255,8 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
         # Deny file access since rate limited
         with ratelimit_rule(86400, 0, domain="spectator_attachment_access_by_file"):
             response = self.client_get(url)
-            self.assertEqual(response.status_code, 403)
+            self.assertEqual(response.status_code, 302)
+            self.assertTrue(response.headers["Location"].endswith(f"/login/?next={url}"))
 
         # Check that the /download/ variant works as well
         download_url = url.replace("/user_uploads/", "/user_uploads/download/")
@@ -262,7 +265,8 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
             self.assertEqual(response.status_code, 200)
         with ratelimit_rule(86400, 0, domain="spectator_attachment_access_by_file"):
             response = self.client_get(download_url)
-            self.assertEqual(response.status_code, 403)
+            self.assertEqual(response.status_code, 302)
+            self.assertTrue(response.headers["Location"].endswith(f"/login/?next={download_url}"))
 
         # Deny random file access
         response = self.client_get(
@@ -323,8 +327,8 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
 
         self.logout()
         response = self.client_get(url)
-        self.assertEqual(response.status_code, 403)
-        self.assert_in_response("<p>You are not authorized to view this file.</p>", response)
+        self.assertEqual(response.status_code, 302)
+        self.assertTrue(response.headers["Location"].endswith(f"/login/?next={url}"))
 
     def test_image_download_unauthed(self) -> None:
         """
@@ -687,7 +691,8 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
         # Verify that cross-realm access to files for spectators is denied.
         self.logout()
         response = self.client_get(url, subdomain=test_subdomain)
-        self.assertEqual(response.status_code, 403)
+        self.assertEqual(response.status_code, 302)
+        self.assertTrue(response.headers["Location"].endswith(f"/login/?next={url}"))
 
     def test_file_download_authorization_invite_only(self) -> None:
         hamlet = self.example_user("hamlet")
diff --git a/zerver/tests/test_upload_s3.py b/zerver/tests/test_upload_s3.py
index e30598db70..359be18674 100644
--- a/zerver/tests/test_upload_s3.py
+++ b/zerver/tests/test_upload_s3.py
@@ -238,7 +238,8 @@ class S3Test(ZulipTestCase):
         # The original url shouldn't work when logged out:
         with self.settings(DEVELOPMENT=False):
             result = self.client_get(url)
-        self.assertEqual(result.status_code, 403)
+        self.assertEqual(result.status_code, 302)
+        self.assertTrue(result.headers["Location"].endswith(f"/login/?next={url}"))
 
         hamlet = self.example_user("hamlet")
         self.subscribe(hamlet, "Denmark")
diff --git a/zerver/views/upload.py b/zerver/views/upload.py
index b7cd96a9ab..4641d2631d 100644
--- a/zerver/views/upload.py
+++ b/zerver/views/upload.py
@@ -25,6 +25,7 @@ from django.utils.http import content_disposition_header
 from django.utils.translation import gettext as _
 
 from zerver.context_processors import get_valid_realm_from_request
+from zerver.decorator import zulip_redirect_to_login
 from zerver.lib.exceptions import JsonableError
 from zerver.lib.response import json_success
 from zerver.lib.storage import static_path
@@ -215,6 +216,8 @@ def serve_file(
     if not is_authorized:
         if preferred_accept(request, ["text/html", "image/png"]) == "image/png":
             response = serve_image_error(403, "images/errors/image-no-auth.png")
+        elif isinstance(maybe_user_profile, AnonymousUser):
+            response = zulip_redirect_to_login(request)
         else:
             response = HttpResponseForbidden(_("<p>You are not authorized to view this file.</p>"))
         patch_vary_headers(response, ("Accept",))