diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index eb343a3240..38a6f41aaf 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,8 +1,12 @@ +* Add support of `strict_loading_by_default` to `ActiveStorage::Representations` controllers + + *Anton Topchii*, *Andrew White* + * Allow to detach an attachment when record is not persisted *Jacopo Beschi* -* Use libvips instead of ImageMagick to analyze images when `active_storage.variant_processor = vips` +* Use libvips instead of ImageMagick to analyze images when `active_storage.variant_processor = vips` *Breno Gazzola* diff --git a/activestorage/app/controllers/active_storage/representations/base_controller.rb b/activestorage/app/controllers/active_storage/representations/base_controller.rb index 3b4cc9ca2d..9aac8c9a77 100644 --- a/activestorage/app/controllers/active_storage/representations/base_controller.rb +++ b/activestorage/app/controllers/active_storage/representations/base_controller.rb @@ -6,6 +6,10 @@ class ActiveStorage::Representations::BaseController < ActiveStorage::BaseContro before_action :set_representation private + def blob_scope + ActiveStorage::Blob.scope_for_strict_loading + end + def set_representation @representation = @blob.representation(params[:variation_key]).processed rescue ActiveSupport::MessageVerifier::InvalidSignature diff --git a/activestorage/app/controllers/concerns/active_storage/set_blob.rb b/activestorage/app/controllers/concerns/active_storage/set_blob.rb index e20d22be2b..1b3e77f6ef 100644 --- a/activestorage/app/controllers/concerns/active_storage/set_blob.rb +++ b/activestorage/app/controllers/concerns/active_storage/set_blob.rb @@ -9,8 +9,12 @@ module ActiveStorage::SetBlob #:nodoc: private def set_blob - @blob = ActiveStorage::Blob.find_signed!(params[:signed_blob_id] || params[:signed_id]) + @blob = blob_scope.find_signed!(params[:signed_blob_id] || params[:signed_id]) rescue ActiveSupport::MessageVerifier::InvalidSignature head :not_found end + + def blob_scope + ActiveStorage::Blob + end end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 6aaf6d510c..54bc235a78 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -148,6 +148,14 @@ class ActiveStorage::Blob < ActiveStorage::Record def signed_id_verifier #:nodoc: @signed_id_verifier ||= ActiveStorage.verifier end + + def scope_for_strict_loading #:nodoc: + if strict_loading_by_default? && ActiveStorage.track_variants + includes(variant_records: { image_attachment: :blob }, preview_image_attachment: :blob) + else + all + end + end end # Returns a signed ID for this blob that's suitable for reference on the client-side without fear of tampering. diff --git a/activestorage/test/controllers/representations/proxy_controller_test.rb b/activestorage/test/controllers/representations/proxy_controller_test.rb index eb62eda083..022386ed42 100644 --- a/activestorage/test/controllers/representations/proxy_controller_test.rb +++ b/activestorage/test/controllers/representations/proxy_controller_test.rb @@ -41,6 +41,29 @@ class ActiveStorage::Representations::ProxyControllerWithVariantsTest < ActionDi end end +class ActiveStorage::Representations::ProxyControllerWithVariantsWithStrictLoadingTest < ActionDispatch::IntegrationTest + setup do + @blob = create_file_blob filename: "racecar.jpg" + @blob.variant(resize: "100x100").processed + end + + test "showing existing variant record" do + with_strict_loading_by_default do + get rails_blob_representation_proxy_url( + filename: @blob.filename, + signed_blob_id: @blob.signed_id, + variation_key: ActiveStorage::Variation.encode(resize: "100x100")) + end + assert_response :ok + assert_match(/^inline/, response.headers["Content-Disposition"]) + + @blob.reload # became free of strict_loading? + image = read_image(@blob.variant(resize: "100x100")) + assert_equal 100, image.width + assert_equal 67, image.height + end +end + class ActiveStorage::Representations::ProxyControllerWithPreviewsTest < ActionDispatch::IntegrationTest setup do @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf" @@ -80,3 +103,28 @@ class ActiveStorage::Representations::ProxyControllerWithPreviewsTest < ActionDi assert_response :not_found end end + +class ActiveStorage::Representations::ProxyControllerWithPreviewsWithStrictLoadingTest < ActionDispatch::IntegrationTest + setup do + @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf" + @blob.preview(resize: "100x100").processed + end + + test "showing existing preview record" do + with_strict_loading_by_default do + get rails_blob_representation_proxy_url( + filename: @blob.filename, + signed_blob_id: @blob.signed_id, + variation_key: ActiveStorage::Variation.encode(resize: "100x100")) + end + + assert_response :ok + assert_match(/^inline/, response.headers["Content-Disposition"]) + @blob.reload # became free of strict_loading? + assert_predicate @blob.preview_image, :attached? + + image = read_image(@blob.preview_image.variant(resize: "100x100").processed) + assert_equal 77, image.width + assert_equal 100, image.height + end +end diff --git a/activestorage/test/controllers/representations/redirect_controller_test.rb b/activestorage/test/controllers/representations/redirect_controller_test.rb index a31d67418b..6dd261de25 100644 --- a/activestorage/test/controllers/representations/redirect_controller_test.rb +++ b/activestorage/test/controllers/representations/redirect_controller_test.rb @@ -42,6 +42,31 @@ class ActiveStorage::Representations::RedirectControllerWithVariantsTest < Actio end end +class ActiveStorage::Representations::RedirectControllerWithVariantsWithStrictLoadingTest < ActionDispatch::IntegrationTest + setup do + @blob = create_file_blob filename: "racecar.jpg" + @blob.variant(resize: "100x100").processed + end + + test "showing existing variant record inline" do + with_strict_loading_by_default do + get rails_blob_representation_url( + filename: @blob.filename, + signed_blob_id: @blob.signed_id, + variation_key: ActiveStorage::Variation.encode(resize: "100x100")) + end + + assert_redirected_to(/racecar\.jpg/) + follow_redirect! + assert_match(/^inline/, response.headers["Content-Disposition"]) + + @blob.reload # became free of strict_loading? + image = read_image(@blob.variant(resize: "100x100")) + assert_equal 100, image.width + assert_equal 67, image.height + end +end + class ActiveStorage::Representations::RedirectControllerWithPreviewsTest < ActionDispatch::IntegrationTest setup do @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf" @@ -81,3 +106,29 @@ class ActiveStorage::Representations::RedirectControllerWithPreviewsTest < Actio assert_response :not_found end end + +class ActiveStorage::Representations::RedirectControllerWithPreviewsWithStrictLoadingTest < ActionDispatch::IntegrationTest + setup do + @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf" + @blob.preview(resize: "100x100").processed + end + + test "showing existing preview record inline" do + with_strict_loading_by_default do + get rails_blob_representation_url( + filename: @blob.filename, + signed_blob_id: @blob.signed_id, + variation_key: ActiveStorage::Variation.encode(resize: "100x100")) + end + + assert_predicate @blob.preview_image, :attached? + assert_redirected_to(/report\.png/) + follow_redirect! + assert_match(/^inline/, response.headers["Content-Disposition"]) + + @blob.reload # became free of strict_loading? + image = read_image(@blob.preview_image.variant(resize: "100x100")) + assert_equal 77, image.width + assert_equal 100, image.height + end +end diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index c6233bc5b5..e2f7e00342 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -286,6 +286,33 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase end end + test "scope_for_strict_loading adds includes only when track_variants and strict_loading_by_default" do + assert_empty( + ActiveStorage::Blob.scope_for_strict_loading.includes_values, + "Expected ActiveStorage::Blob.scope_for_strict_loading have no includes" + ) + + with_strict_loading_by_default do + includes_values = ActiveStorage::Blob.scope_for_strict_loading.includes_values + + assert( + includes_values.any? { |values| values[:variant_records] == { image_attachment: :blob } }, + "Expected ActiveStorage::Blob.scope_for_strict_loading to have variant_records included" + ) + assert( + includes_values.any? { |values| values[:preview_image_attachment] == :blob }, + "Expected ActiveStorage::Blob.scope_for_strict_loading to have preview_image_attachment included" + ) + + without_variant_tracking do + assert_empty( + ActiveStorage::Blob.scope_for_strict_loading.includes_values, + "Expected ActiveStorage::Blob.scope_for_strict_loading have no includes" + ) + end + end + end + private def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil, service_name: :local) filename ||= blob.filename diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index 41f97e9dfa..6de61ec38e 100644 --- a/activestorage/test/test_helper.rb +++ b/activestorage/test/test_helper.rb @@ -125,6 +125,20 @@ class ActiveSupport::TestCase ensure ActiveStorage::Blob.service = previous_service end + + def with_strict_loading_by_default(&block) + strict_loading_was = ActiveRecord::Base.strict_loading_by_default + ActiveRecord::Base.strict_loading_by_default = true + yield + ActiveRecord::Base.strict_loading_by_default = strict_loading_was + end + + def without_variant_tracking(&block) + variant_tracking_was = ActiveStorage.track_variants + ActiveStorage.track_variants = false + yield + ActiveStorage.track_variants = variant_tracking_was + end end require "global_id"