DRY

8 files changed, 20 insertions, 16 deletions

diff --git a/activestorage/app/controllers/active_storage/disk_controller.rb b/activestorage/app/controllers/active_storage/disk_controller.rb
index b10d4e2cac..41e6d61bff 100644
--- a/activestorage/app/controllers/active_storage/disk_controller.rb
+++ b/activestorage/app/controllers/active_storage/disk_controller.rb
@@ -8,7 +8,7 @@ class ActiveStorage::DiskController < ActionController::Base
   def show
     if key = decode_verified_key
       send_data disk_service.download(key),
-        filename: params[:filename], disposition: disposition_param, content_type: params[:content_type]
+        disposition: disposition_param, content_type: params[:content_type]
     else
       head :not_found
     end
@@ -39,7 +39,7 @@ class ActiveStorage::DiskController < ActionController::Base
     end
 
     def disposition_param
-      params[:disposition].presence_in(%w( inline attachment )) || "inline"
+      params[:disposition].presence || "inline"
     end
 
 
diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index 9f2ed1e5ac..2e627ef118 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -127,7 +127,7 @@ class ActiveStorage::Blob < ActiveRecord::Base
   # Hiding the +service_url+ behind a redirect also gives you the power to change services without updating all URLs. And
   # it allows permanent URLs that redirect to the +service_url+ to be cached in the view.
   def service_url(expires_in: 5.minutes, disposition: :inline)
-    service.url key, expires_in: expires_in, disposition: disposition, filename: filename, content_type: content_type
+    service.url key, expires_in: expires_in, disposition: "#{disposition}; filename=\"#{filename}\"", filename: filename, content_type: content_type
   end
 
   # Returns a URL that can be used to directly upload a file for this blob on the service. This URL is intended to be
diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb
index d0ee2273c5..895cc9c2f1 100644
--- a/activestorage/lib/active_storage/service/azure_storage_service.rb
+++ b/activestorage/lib/active_storage/service/azure_storage_service.rb
@@ -59,12 +59,16 @@ module ActiveStorage
       end
     end
 
-    def url(key, expires_in:, disposition:, filename:, content_type:)
+    def url(key, expires_in:, filename:, disposition:, content_type:)
       instrument :url, key do |payload|
         base_url = url_for(key)
-        generated_url = signer.signed_uri(URI(base_url), false, permissions: "r",
-          expiry: format_expiry(expires_in), content_type: content_type,
-          content_disposition: "#{disposition}; filename=\"#{filename}\"").to_s
+        generated_url = signer.signed_uri(
+          URI(base_url), false,
+          permissions: "r",
+          expiry: format_expiry(expires_in),
+          content_disposition: disposition,
+          content_type: content_type
+        ).to_s
 
         payload[:url] = generated_url
 
diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb
index 1d121dcb6c..5aa8d74a5a 100644
--- a/activestorage/lib/active_storage/service/disk_service.rb
+++ b/activestorage/lib/active_storage/service/disk_service.rb
@@ -56,7 +56,7 @@ module ActiveStorage
       end
     end
 
-    def url(key, expires_in:, disposition:, filename:, content_type:)
+    def url(key, expires_in:, filename:, disposition:, content_type:)
       instrument :url, key do |payload|
         verified_key_with_expiration = ActiveStorage.verifier.generate(key, expires_in: expires_in, purpose: :blob_key)
 
@@ -64,7 +64,7 @@ module ActiveStorage
           if defined?(Rails.application)
             Rails.application.routes.url_helpers.rails_disk_service_path \
               verified_key_with_expiration,
-              disposition: disposition, filename: filename, content_type: content_type
+              filename: filename, disposition: disposition, content_type: content_type
           else
             "/rails/active_storage/disk/#{verified_key_with_expiration}/#{filename}?disposition=#{disposition}&content_type=#{content_type}"
           end
diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb
index e656f2c69f..ebf24a36d7 100644
--- a/activestorage/lib/active_storage/service/gcs_service.rb
+++ b/activestorage/lib/active_storage/service/gcs_service.rb
@@ -47,10 +47,10 @@ module ActiveStorage
       end
     end
 
-    def url(key, expires_in:, disposition:, filename:, content_type:)
+    def url(key, expires_in:, filename:, content_type:, disposition:)
       instrument :url, key do |payload|
         generated_url = file_for(key).signed_url expires: expires_in, query: {
-          "response-content-disposition" => "#{disposition}; filename=\"#{filename}\"",
+          "response-content-disposition" => disposition,
           "response-content-type" => content_type
         }
 
diff --git a/activestorage/lib/active_storage/service/s3_service.rb b/activestorage/lib/active_storage/service/s3_service.rb
index bef2ecbea9..2b52c3786d 100644
--- a/activestorage/lib/active_storage/service/s3_service.rb
+++ b/activestorage/lib/active_storage/service/s3_service.rb
@@ -52,10 +52,10 @@ module ActiveStorage
       end
     end
 
-    def url(key, expires_in:, disposition:, filename:, content_type:)
+    def url(key, expires_in:, disposition:, filename:, disposition:, content_type:)
       instrument :url, key do |payload|
         generated_url = object_for(key).presigned_url :get, expires_in: expires_in,
-          response_content_disposition: "#{disposition}; filename=\"#{filename}\"",
+          response_content_disposition: disposition,
           response_content_type: content_type
 
         payload[:url] = generated_url
diff --git a/activestorage/test/controllers/disk_controller_test.rb b/activestorage/test/controllers/disk_controller_test.rb
index 53a086f214..eacb9bfc11 100644
--- a/activestorage/test/controllers/disk_controller_test.rb
+++ b/activestorage/test/controllers/disk_controller_test.rb
@@ -8,7 +8,7 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
     blob = create_blob
 
     get blob.service_url
-    assert_equal "inline; filename=\"#{blob.filename.base}\"", @response.headers["Content-Disposition"]
+    assert_equal "inline; filename=\"hello.txt\"", @response.headers["Content-Disposition"]
     assert_equal "text/plain", @response.headers["Content-Type"]
   end
 
@@ -16,7 +16,7 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
     blob = create_blob
 
     get blob.service_url(disposition: :attachment)
-    assert_equal "attachment; filename=\"#{blob.filename.base}\"", @response.headers["Content-Disposition"]
+    assert_equal "attachment; filename=\"hello.txt\"", @response.headers["Content-Disposition"]
     assert_equal "text/plain", @response.headers["Content-Type"]
   end
 
diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb
index 99f2fb20b0..03bf72c0de 100644
--- a/activestorage/test/models/blob_test.rb
+++ b/activestorage/test/models/blob_test.rb
@@ -50,7 +50,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
 
   private
     def expected_url_for(blob, disposition: :inline)
-      query_string = { content_type: blob.content_type, disposition: disposition }.to_param
+      query_string = { content_type: blob.content_type, disposition: "#{disposition}; filename=\"#{blob.filename}\"" }.to_param
       "/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{blob.filename}?#{query_string}"
     end
 end