Still redirect to signed URL for secure uploads

if SiteSetting.secure_media is disabled we still want to
redirect to the signed url for uploads that are marked as
secure because their ACLs are probably still private

app/controllers/uploads_controller.rb

@@ -126,14 +126,17 @@ class UploadsController < ApplicationController
     upload = Upload.find_by(sha1: sha1)
     return render_404 if upload.blank?
 
-    if SiteSetting.secure_media?
-      return redirect_to Discourse.store.signed_url_for_path(path_with_ext)
-    end
+    signed_secure_url = Discourse.store.signed_url_for_path(path_with_ext)
+    return redirect_to signed_secure_url if SiteSetting.secure_media?
 
     # we don't want to 404 here if secure media gets disabled
     # because all posts with secure uploads will show broken media
     # until rebaked, which could take some time
-    redirect_to Discourse.store.cdn_url(upload.url)
+    #
+    # if the upload is still secure, that means the ACL is probably still
+    # private, so we don't want to go to the CDN url just yet otherwise we
+    # will get a 403. if the upload is not secure we assume the ACL is public
+    redirect_to upload.secure? ? signed_secure_url : Discourse.store.cdn_url(upload.url)
   end
 
   def metadata

spec/requests/uploads_controller_spec.rb

@@ -437,15 +437,38 @@ describe UploadsController do
           SiteSetting.secure_media = false
         end
 
-        it "should redirect to the regular show route" do
-          secure_url = upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads")
-          sign_in(user)
-          stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+        context "if the upload is secure false, meaning the ACL is probably public" do
+          before do
+            upload.update(secure: false)
+          end
 
-          get secure_url
+          it "should redirect to the regular show route" do
+            secure_url = upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads")
+            sign_in(user)
+            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
 
-          expect(response.status).to eq(302)
-          expect(response.redirect_url).to eq(Discourse.store.cdn_url(upload.url))
+            get secure_url
+
+            expect(response.status).to eq(302)
+            expect(response.redirect_url).to eq(Discourse.store.cdn_url(upload.url))
+          end
+        end
+
+        context "if the upload is secure true, meaning the ACL is probably private" do
+          before do
+            upload.update(secure: true)
+          end
+
+          it "should redirect to the presigned URL still otherwise we will get a 403" do
+            secure_url = upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads")
+            sign_in(user)
+            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+
+            get secure_url
+
+            expect(response.status).to eq(302)
+            expect(response.redirect_url).to match("Amz-Expires")
+          end
         end
       end
     end