FIX: `OptimizedImage#filesize` (#10095)

`OptimizedImage#filesize` calls `Discourse.store.download` with an OptimizedImage as an argument. It would in turn attempt to call `#original_filename` and `#secure?` on that object. Both would fail as these methods do not exist on OptimizedImage, only on Upload. We didn't know about these issues because:
1. `#calculate_filesize` is not called often, because the filesize is saved on OptimizedImage creation, so it's used mostly for manual filesize recalculation
2. we were using `rescue nil` which swallows all errors

app/models/optimized_image.rb

@@ -156,9 +156,7 @@ class OptimizedImage < ActiveRecord::Base
     if size = read_attribute(:filesize)
       size
     else
-      # we may have a bad optimized image so just skip for now
-      # and do not break here
-      size = calculate_filesize rescue nil
+      size = calculate_filesize
 
       write_attribute(:filesize, size)
       if !new_record?

lib/file_store/base_store.rb

@@ -76,17 +76,19 @@ module FileStore
       not_implemented
     end
 
-    def download(upload, max_file_size_kb: nil)
-      DistributedMutex.synchronize("download_#{upload.sha1}", validity: 3.minutes) do
-        filename = "#{upload.sha1}#{File.extname(upload.original_filename)}"
+    def download(object, max_file_size_kb: nil)
+      DistributedMutex.synchronize("download_#{object.sha1}", validity: 3.minutes) do
+        extension = File.extname(object.respond_to?(:original_filename) ? object.original_filename : object.url)
+        filename = "#{object.sha1}#{extension}"
         file = get_from_cache(filename)
 
         if !file
           max_file_size_kb ||= [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes
 
-          url = upload.secure? ?
-            Discourse.store.signed_url_for_path(upload.url) :
-            Discourse.store.cdn_url(upload.url)
+          secure = object.respond_to?(:secure) ? object.secure? : object.upload.secure?
+          url = secure ?
+            Discourse.store.signed_url_for_path(object.url) :
+            Discourse.store.cdn_url(object.url)
 
           url = SiteSetting.scheme + ":" + url if url =~ /^\/\//
           file = FileHelper.download(
@@ -95,6 +97,7 @@ module FileStore
             tmp_file_name: "discourse-download",
             follow_redirect: true
           )
+
           cache_file(file, filename)
           file = get_from_cache(filename)
         end

spec/models/optimized_image_spec.rb

@@ -292,68 +292,82 @@ describe OptimizedImage do
     end
 
     describe "external store" do
-      let(:s3_upload) { Fabricate(:upload_s3) }
-
       before do
         SiteSetting.enable_s3_uploads = true
         SiteSetting.s3_upload_bucket = "s3-upload-bucket"
         SiteSetting.s3_access_key_id = "some key"
         SiteSetting.s3_secret_access_key = "some secret key"
         SiteSetting.s3_region = "us-east-1"
-
-        tempfile = Tempfile.new(["discourse-external", ".png"])
-
-        %i{head get}.each do |method|
-          stub_request(method, "http://#{s3_upload.url}")
-            .to_return(
-              status: 200,
-              body: tempfile.read
-            )
-        end
       end
 
       context "when we have a bad file returned" do
         it "returns nil" do
-          # tempfile is empty
-          # this can not be resized
+          s3_upload = Fabricate(:upload_s3)
+          stub_request(:head, "http://#{s3_upload.url}").to_return(status: 200)
+          stub_request(:get, "http://#{s3_upload.url}").to_return(status: 200)
+
           expect(OptimizedImage.create_for(s3_upload, 100, 200)).to eq(nil)
         end
       end
 
       context "when the thumbnail is properly generated" do
-        before do
-          OptimizedImage.expects(:resize).returns(true)
+        context "secure media disabled" do
+          let(:s3_upload) { Fabricate(:upload_s3) }
+          let(:optimized_path) { "/optimized/1X/#{s3_upload.sha1}_2_100x200.png" }
+
+          before do
+            stub_request(:head, "http://#{s3_upload.url}").to_return(status: 200)
+            stub_request(:get, "http://#{s3_upload.url}").to_return(status: 200, body: file_from_fixtures("logo.png"))
+            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+            stub_request(:put, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com#{optimized_path}")
+              .to_return(status: 200, headers: { "ETag" => "someetag" })
+          end
+
+          it "downloads a copy of the original image" do
+            oi = OptimizedImage.create_for(s3_upload, 100, 200)
+
+            expect(oi.sha1).to_not be_nil
+            expect(oi.extension).to eq(".png")
+            expect(oi.width).to eq(100)
+            expect(oi.height).to eq(200)
+            expect(oi.url).to eq("//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com#{optimized_path}")
+            expect(oi.filesize).to be > 0
+          end
+
+          it "allows to recalculate the filesize" do
+            oi = OptimizedImage.create_for(s3_upload, 100, 200)
+            oi.filesize = nil
+
+            stub_request(
+              :get,
+              "http://#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com#{optimized_path}"
+            ).to_return(status: 200, body: file_from_fixtures("resized.png"))
+
+            expect(oi.filesize).to be > 0
+          end
         end
 
-        it "downloads a copy of the original image" do
-          optimized_path = "/optimized/1X/#{s3_upload.sha1}_2_100x200.png"
+        context "secure uploads enabled" do
+          it "allows to recalculate the filesize" do
+            SiteSetting.secure_media = true
+            s3_upload = Fabricate(:secure_upload_s3)
 
-          stub_request(
-            :head,
-            "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/"
-          )
+            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+            stub_request(:get, Discourse.store.signed_url_for_path(s3_upload.url)).to_return(status: 200, body: file_from_fixtures("logo.png"))
+            stub_request(:put, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/optimized/1X/#{s3_upload.sha1}_2_100x200.png")
+              .to_return(status: 200, headers: { "ETag" => "someetag" })
 
-          stub_request(
-            :put,
-            "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com#{optimized_path}"
-          ).to_return(
-            status: 200,
-            headers: { "ETag" => "someetag" }
-          )
+            oi = OptimizedImage.create_for(s3_upload, 100, 200)
+            oi.filesize = nil
 
-          oi = OptimizedImage.create_for(s3_upload, 100, 200)
+            stub_request(:get, Discourse.store.signed_url_for_path(oi.url))
+              .to_return(status: 200, body: file_from_fixtures("resized.png"))
 
-          expect(oi.sha1).to eq("da39a3ee5e6b4b0d3255bfef95601890afd80709")
-          expect(oi.extension).to eq(".png")
-          expect(oi.width).to eq(100)
-          expect(oi.height).to eq(200)
-          expect(oi.url).to eq("//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com#{optimized_path}")
+            expect(oi.filesize).to be > 0
+          end
         end
-
       end
-
     end
-
   end
 
   describe '#destroy' do