Merge pull request #37897 from peterzhu2118/as-public-disk-blob-fixes

Use DiskController for both public and private files

activestorage/CHANGELOG.md

+*   Use `DiskController` for both public and private files.
+
+    `DiskController` is able to handle multiple services by adding a
+    `service_name` field in the generated URL in `DiskService`.
+
+    *Peter Zhu*
+
 *   Variants are tracked in the database to avoid existence checks in the storage service.
 
     *George Claghorn*

activestorage/app/controllers/active_storage/disk_controller.rb

@@ -11,7 +11,7 @@ class ActiveStorage::DiskController < ActiveStorage::BaseController
 
   def show
     if key = decode_verified_key
-      serve_file disk_service.path_for(key[:key]), content_type: key[:content_type], disposition: key[:disposition]
+      serve_file named_disk_service(key[:service_name]).path_for(key[:key]), content_type: key[:content_type], disposition: key[:disposition]
     else
       head :not_found
     end
@@ -22,7 +22,7 @@ def show
   def update
     if token = decode_verified_token
       if acceptable_content?(token)
-        disk_service.upload token[:key], request.body, checksum: token[:checksum]
+        named_disk_service(token[:service_name]).upload token[:key], request.body, checksum: token[:checksum]
       else
         head :unprocessable_entity
       end
@@ -34,8 +34,10 @@ def update
   end
 
   private
-    def disk_service
-      ActiveStorage::Blob.service
+    def named_disk_service(name)
+      ActiveStorage::Blob.services.fetch(name) do
+        ActiveStorage::Blob.service
+      end
     end
 
 

activestorage/app/controllers/active_storage/public_disk_controller.rb

-# frozen_string_literal: true
-
-# Serves files stored in a public disk service using a permanent URL.
-class ActiveStorage::PublicDiskController < ActiveStorage::BaseController
-  include ActiveStorage::FileServer
-
-  skip_forgery_protection
-
-  def show
-    if blob = ActiveStorage::Blob.find_by(key: params[:key])
-      if blob.service.public?
-        serve_file blob.service.path_for(blob.key), content_type: blob.content_type, disposition: :inline
-      else
-        head :unauthorized
-      end
-    else
-      head :not_found
-    end
-  end
-end

activestorage/config/routes.rb

@@ -6,7 +6,6 @@
 
     get "/representations/:signed_blob_id/:variation_key/*filename" => "active_storage/representations#show", as: :rails_blob_representation
 
-    get  "/disk/public/:key/*filename" => "active_storage/public_disk#show", as: :rails_disk_service_public
     get  "/disk/:encoded_key/*filename" => "active_storage/disk#show", as: :rails_disk_service
     put  "/disk/:encoded_token" => "active_storage/disk#update", as: :update_rails_disk_service
     post "/direct_uploads" => "active_storage/direct_uploads#create", as: :rails_direct_uploads

activestorage/lib/active_storage/service/disk_service.rb

@@ -79,7 +79,8 @@ def url_for_direct_upload(key, expires_in:, content_type:, content_length:, chec
             key: key,
             content_type: content_type,
             content_length: content_length,
-            checksum: checksum
+            checksum: checksum,
+            service_name: name
           },
           expires_in: expires_in,
           purpose: :blob_token
@@ -103,12 +104,21 @@ def path_for(key) #:nodoc:
 
     private
       def private_url(key, expires_in:, filename:, content_type:, disposition:, **)
+        generate_url(key, expires_in: expires_in, filename: filename, content_type: content_type, disposition: disposition)
+      end
+
+      def public_url(key, filename:, content_type: nil, disposition: :attachment, **)
+        generate_url(key, expires_in: nil, filename: filename, content_type: content_type, disposition: disposition)
+      end
+
+      def generate_url(key, expires_in:, filename:, content_type:, disposition:)
         content_disposition = content_disposition_with(type: disposition, filename: filename)
         verified_key_with_expiration = ActiveStorage.verifier.generate(
           {
             key: key,
             disposition: content_disposition,
-            content_type: content_type
+            content_type: content_type,
+            service_name: name
           },
           expires_in: expires_in,
           purpose: :blob_key
@@ -126,17 +136,6 @@ def private_url(key, expires_in:, filename:, content_type:, disposition:, **)
         )
       end
 
-      def public_url(key, filename:, content_type: nil, disposition: :attachment, **)
-        current_uri = URI.parse(current_host)
-
-        url_helpers.rails_disk_service_public_url(key,
-          protocol: current_uri.scheme,
-          host: current_uri.host,
-          port: current_uri.port,
-          filename: filename
-        )
-      end
-
 
       def stream(key)
         File.open(path_for(key), "rb") do |file|

activestorage/test/controllers/disk_controller_test.rb

@@ -44,6 +44,27 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
     assert_response :not_found
   end
 
+  test "showing public blob" do
+    with_service("local_public") do
+      blob = create_blob(content_type: "image/jpg")
+
+      get blob.url
+      assert_response :ok
+      assert_equal "image/jpg", response.headers["Content-Type"]
+      assert_equal "Hello world!", response.body
+    end
+  end
+
+  test "showing public blob variant" do
+    with_service("local_public") do
+      blob = create_file_blob.variant(resize_to_limit: [100, 100]).processed
+
+      get blob.url
+      assert_response :ok
+      assert_equal "image/jpeg", response.headers["Content-Type"]
+    end
+  end
+
   test "directly uploading blob with integrity" do
     data = "Something else entirely!"
     blob = create_blob_before_direct_upload byte_size: data.size, checksum: Digest::MD5.base64digest(data)

activestorage/test/controllers/public_disk_controller_test.rb

-# frozen_string_literal: true
-
-require "test_helper"
-require "database/setup"
-
-class ActiveStorage::PublicDiskControllerTest < ActionDispatch::IntegrationTest
-  test "showing public blob" do
-    with_service("local_public") do
-      blob = create_blob(content_type: "image/jpg")
-
-      get blob.url
-      assert_response :ok
-      assert_equal "image/jpg", response.headers["Content-Type"]
-      assert_equal "Hello world!", response.body
-    end
-  end
-
-  test "showing private blob as a public blob" do
-    blob = create_blob(content_type: "image/jpg")
-
-    with_service("local_public") do
-      get rails_disk_service_public_url(key: blob.key, filename: "foo.jpg")
-      assert_response :unauthorized
-    end
-  end
-
-  test "showing public blob with invalid key" do
-    with_service("local_public") do
-      get rails_disk_service_public_url(key: "abc123", filename: "foo.jpg")
-      assert_response :not_found
-    end
-  end
-end

activestorage/test/models/blob_test.rb

@@ -261,12 +261,12 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
   end
 
   private
-    def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil)
+    def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil, service_name: :local)
       filename ||= blob.filename
       content_type ||= blob.content_type
 
       query = { disposition: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized), content_type: content_type }
-      key_params = { key: blob.key }.merge(query)
+      key_params = { key: blob.key }.merge(query).merge(service_name: service_name)
 
       "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(key_params, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query.to_param}"
     end

activestorage/test/models/variant_test.rb

@@ -189,7 +189,7 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   test "url doesn't grow in length despite long variant options" do
     blob = create_file_blob(filename: "racecar.jpg")
     variant = blob.variant(font: "a" * 10_000).processed
-    assert_operator variant.url.length, :<, 730
+    assert_operator variant.url.length, :<, 785
   end
 
   test "works for vips processor" do

activestorage/test/service/disk_public_service_test.rb

@@ -12,8 +12,8 @@ class ActiveStorage::Service::DiskPublicServiceTest < ActiveSupport::TestCase
   include ActiveStorage::Service::SharedServiceTests
 
   test "public URL generation" do
-    url = @service.url(@key, filename: ActiveStorage::Filename.new("avatar.png"))
+    url = @service.url(@key, disposition: :inline, filename: ActiveStorage::Filename.new("avatar.png"), content_type: "image/png")
 
-    assert_match(/^https:\/\/example.com\/rails\/active_storage\/disk\/public\/#{@key}/, url)
+    assert_match(/^https:\/\/example.com\/rails\/active_storage\/disk\/.*\/avatar\.png\?content_type=image%2Fpng&disposition=inline.*/, url)
   end
 end

railties/test/application/configuration_test.rb

@@ -2406,7 +2406,6 @@ class MyLogger < ::Logger
                            Prefix Verb URI Pattern                                                               Controller#Action
                rails_service_blob GET  /files/blobs/:signed_id/*filename(.:format)                               active_storage/blobs#show
         rails_blob_representation GET  /files/representations/:signed_blob_id/:variation_key/*filename(.:format) active_storage/representations#show
-        rails_disk_service_public GET  /files/disk/public/:key/*filename(.:format)                               active_storage/public_disk#show
                rails_disk_service GET  /files/disk/:encoded_key/*filename(.:format)                              active_storage/disk#show
         update_rails_disk_service PUT  /files/disk/:encoded_token(.:format)                                      active_storage/disk#update
              rails_direct_uploads POST /files/direct_uploads(.:format)                                           active_storage/direct_uploads#create

railties/test/application/rake/routes_test.rb

@@ -36,7 +36,6 @@ class RakeRoutesTest < ActiveSupport::TestCase
           rails_conductor_inbound_email_reroute POST   /rails/conductor/action_mailbox/:inbound_email_id/reroute(.:format)                      rails/conductor/action_mailbox/reroutes#create
                              rails_service_blob GET    /rails/active_storage/blobs/:signed_id/*filename(.:format)                               active_storage/blobs#show
                       rails_blob_representation GET    /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format) active_storage/representations#show
-                      rails_disk_service_public GET    /rails/active_storage/disk/public/:key/*filename(.:format)                               active_storage/public_disk#show
                              rails_disk_service GET    /rails/active_storage/disk/:encoded_key/*filename(.:format)                              active_storage/disk#show
                       update_rails_disk_service PUT    /rails/active_storage/disk/:encoded_token(.:format)                                      active_storage/disk#update
                            rails_direct_uploads POST   /rails/active_storage/direct_uploads(.:format)                                           active_storage/direct_uploads#create

railties/test/commands/routes_test.rb

@@ -56,7 +56,6 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase
       rails_conductor_inbound_email GET  /rails/conductor/action_mailbox/inbound_emails/:id(.:format)                             rails/conductor/action_mailbox/inbound_emails#show
                  rails_service_blob GET  /rails/active_storage/blobs/:signed_id/*filename(.:format)                               active_storage/blobs#show
           rails_blob_representation GET  /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format) active_storage/representations#show
-          rails_disk_service_public GET  /rails/active_storage/disk/public/:key/*filename(.:format)                               active_storage/public_disk#show
                  rails_disk_service GET  /rails/active_storage/disk/:encoded_key/*filename(.:format)                              active_storage/disk#show
     MESSAGE
 
@@ -182,7 +181,6 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase
       rails_conductor_inbound_email_reroute POST   /rails/conductor/action_mailbox/:inbound_email_id/reroute(.:format)                      rails/conductor/action_mailbox/reroutes#create
                          rails_service_blob GET    /rails/active_storage/blobs/:signed_id/*filename(.:format)                               active_storage/blobs#show
                   rails_blob_representation GET    /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format) active_storage/representations#show
-                  rails_disk_service_public GET    /rails/active_storage/disk/public/:key/*filename(.:format)                               active_storage/public_disk#show
                          rails_disk_service GET    /rails/active_storage/disk/:encoded_key/*filename(.:format)                              active_storage/disk#show
                   update_rails_disk_service PUT    /rails/active_storage/disk/:encoded_token(.:format)                                      active_storage/disk#update
                        rails_direct_uploads POST   /rails/active_storage/direct_uploads(.:format)                                           active_storage/direct_uploads#create
@@ -288,21 +286,16 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase
       URI               | /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format)
       Controller#Action | active_storage/representations#show
       --[ Route 18 ]-------------
-      Prefix            | rails_disk_service_public
-      Verb              | GET
-      URI               | /rails/active_storage/disk/public/:key/*filename(.:format)
-      Controller#Action | active_storage/public_disk#show
-      --[ Route 19 ]-------------
       Prefix            | rails_disk_service
       Verb              | GET
       URI               | /rails/active_storage/disk/:encoded_key/*filename(.:format)
       Controller#Action | active_storage/disk#show
-      --[ Route 20 ]-------------
+      --[ Route 19 ]-------------
       Prefix            | update_rails_disk_service
       Verb              | PUT
       URI               | /rails/active_storage/disk/:encoded_token(.:format)
       Controller#Action | active_storage/disk#update
-      --[ Route 21 ]-------------
+      --[ Route 20 ]-------------
       Prefix            | rails_direct_uploads
       Verb              | POST
       URI               | /rails/active_storage/direct_uploads(.:format)