From 46da4ee7daf1ecaa2fc47a260ccb58e119a1b5ea Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 11:05:20 -0500 Subject: [PATCH] Switch to simpler signed_id for blob rather than full GlobalID We don't need to lookup multiple different classes, so no need to use a globalid. --- .../active_storage/direct_uploads_controller.rb | 5 ++++- .../active_storage/variants_controller.rb | 17 ++++++++--------- app/models/active_storage/blob.rb | 9 +++++++++ .../active_storage/service/disk_service.rb | 1 + config/routes.rb | 17 ++++++++++------- lib/active_storage/attached.rb | 2 +- .../direct_uploads_controller_test.rb | 8 ++++---- test/controllers/variants_controller_test.rb | 2 +- test/models/attachments_test.rb | 2 +- test/models/blob_test.rb | 1 + .../models/verified_key_with_expiration_test.rb | 1 + test/test_helper.rb | 6 ------ 12 files changed, 41 insertions(+), 30 deletions(-) diff --git a/app/controllers/active_storage/direct_uploads_controller.rb b/app/controllers/active_storage/direct_uploads_controller.rb index dccd864e8d..0d1b806f9f 100644 --- a/app/controllers/active_storage/direct_uploads_controller.rb +++ b/app/controllers/active_storage/direct_uploads_controller.rb @@ -1,7 +1,10 @@ +# Creates a new blob on the server side in anticipation of a direct-to-service upload from the client side. +# When the client-side upload is completed, the signed_blob_id can be submitted as part of the form to reference +# the blob that was created up front. class ActiveStorage::DirectUploadsController < ActionController::Base def create blob = ActiveStorage::Blob.create_before_direct_upload!(blob_args) - render json: { url: blob.url_for_direct_upload, sgid: blob.to_sgid.to_param } + render json: { upload_to_url: blob.url_for_direct_upload, signed_blob_id: blob.signed_id } end private diff --git a/app/controllers/active_storage/variants_controller.rb b/app/controllers/active_storage/variants_controller.rb index d5e97e63fa..a65d7d7571 100644 --- a/app/controllers/active_storage/variants_controller.rb +++ b/app/controllers/active_storage/variants_controller.rb @@ -1,22 +1,21 @@ +require "active_storage/variant" + class ActiveStorage::VariantsController < ActionController::Base def show - if blob_key = decode_verified_blob_key - redirect_to processed_variant_for(blob_key).url(disposition: disposition_param) + if blob = find_signed_blob + redirect_to ActiveStorage::Variant.new(blob, decoded_variation).processed.url(disposition: disposition_param) else head :not_found end end private - def decode_verified_blob_key - ActiveStorage::VerifiedKeyWithExpiration.decode(params[:encoded_blob_key]) + def find_signed_blob + ActiveStorage::Blob.find_signed(params[:signed_blob_id]) end - def processed_variant_for(blob_key) - ActiveStorage::Variant.new( - ActiveStorage::Blob.find_by!(key: blob_key), - ActiveStorage::Variation.decode(params[:variation_key]) - ).processed + def decoded_variation + ActiveStorage::Variation.decode(params[:variation_key]) end def disposition_param diff --git a/app/models/active_storage/blob.rb b/app/models/active_storage/blob.rb index 6bd3941cd8..7b45d3ad25 100644 --- a/app/models/active_storage/blob.rb +++ b/app/models/active_storage/blob.rb @@ -2,6 +2,7 @@ require "active_storage/filename" require "active_storage/purge_job" require "active_storage/variant" +require "active_storage/variation" # Schema: id, key, filename, content_type, metadata, byte_size, checksum, created_at class ActiveStorage::Blob < ActiveRecord::Base @@ -13,6 +14,10 @@ class ActiveStorage::Blob < ActiveRecord::Base class_attribute :service class << self + def find_signed(id) + find ActiveStorage.verifier.verify(id) + end + def build_after_upload(io:, filename:, content_type: nil, metadata: nil) new.tap do |blob| blob.filename = filename @@ -33,6 +38,10 @@ def create_before_direct_upload!(filename:, byte_size:, checksum:, content_type: end + def signed_id + ActiveStorage.verifier.generate(id) + end + def key # We can't wait until the record is first saved to have a key for it self[:key] ||= self.class.generate_unique_secure_token diff --git a/app/models/active_storage/service/disk_service.rb b/app/models/active_storage/service/disk_service.rb index a2a27528c1..905f41c138 100644 --- a/app/models/active_storage/service/disk_service.rb +++ b/app/models/active_storage/service/disk_service.rb @@ -2,6 +2,7 @@ require "pathname" require "digest/md5" require "active_support/core_ext/numeric/bytes" +require "active_storage/verified_key_with_expiration" class ActiveStorage::Service::DiskService < ActiveStorage::Service attr_reader :root diff --git a/config/routes.rb b/config/routes.rb index 80e7f46184..78fa0e707b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,16 +1,19 @@ Rails.application.routes.draw do - get "/rails/active_storage/disk/:encoded_key/*filename" => "active_storage/disk#show", as: :rails_disk_blob - post "/rails/active_storage/direct_uploads" => "active_storage/direct_uploads#create", as: :rails_direct_uploads - get "/rails/active_storage/variants/:encoded_blob_key/:variation_key/*filename" => "active_storage/variants#show", as: :rails_blob_variation + + get "/rails/active_storage/variants/:signed_blob_id/:variation_key/*filename" => "active_storage/variants#show", as: :rails_blob_variation direct :rails_variant do |variant| - encoded_blob_key = ActiveStorage::VerifiedKeyWithExpiration.encode(variant.blob.key) - variation_key = variant.variation.key - filename = variant.blob.filename + signed_blob_id = variant.blob.signed_id + variation_key = variant.variation.key + filename = variant.blob.filename - route_for(:rails_blob_variation, encoded_blob_key, variation_key, filename) + route_for(:rails_blob_variation, signed_blob_id, variation_key, filename) end resolve("ActiveStorage::Variant") { |variant| route_for(:rails_variant, variant) } + + + get "/rails/active_storage/disk/:encoded_key/*filename" => "active_storage/disk#show", as: :rails_disk_blob + post "/rails/active_storage/direct_uploads" => "active_storage/direct_uploads#create", as: :rails_direct_uploads end diff --git a/lib/active_storage/attached.rb b/lib/active_storage/attached.rb index d5ded51e2b..9fa7b8e021 100644 --- a/lib/active_storage/attached.rb +++ b/lib/active_storage/attached.rb @@ -26,7 +26,7 @@ def create_blob_from(attachable) when Hash ActiveStorage::Blob.create_after_upload!(attachable) when String - GlobalID::Locator.locate_signed(attachable) + ActiveStorage::Blob.find_signed(attachable) else nil end diff --git a/test/controllers/direct_uploads_controller_test.rb b/test/controllers/direct_uploads_controller_test.rb index 0083492929..06e76cfa8b 100644 --- a/test/controllers/direct_uploads_controller_test.rb +++ b/test/controllers/direct_uploads_controller_test.rb @@ -24,8 +24,8 @@ class ActiveStorage::S3DirectUploadsControllerTest < ActionController::TestCase details = JSON.parse(@response.body) - assert_match /#{SERVICE_CONFIGURATIONS[:s3][:bucket]}\.s3.(\S+)?amazonaws\.com/, details["url"] - assert_equal "hello.txt", GlobalID::Locator.locate_signed(details["sgid"]).filename.to_s + assert_match /#{SERVICE_CONFIGURATIONS[:s3][:bucket]}\.s3.(\S+)?amazonaws\.com/, details["upload_to_url"] + assert_equal "hello.txt", ActiveStorage::Blob.find_signed(details["signed_blob_id"]).filename.to_s end end else @@ -54,8 +54,8 @@ class ActiveStorage::GCSDirectUploadsControllerTest < ActionController::TestCase details = JSON.parse(@response.body) - assert_match %r{storage\.googleapis\.com/#{@config[:bucket]}}, details["url"] - assert_equal "hello.txt", GlobalID::Locator.locate_signed(details["sgid"]).filename.to_s + assert_match %r{storage\.googleapis\.com/#{@config[:bucket]}}, details["upload_to_url"] + assert_equal "hello.txt", ActiveStorage::Blob.find_signed(details["signed_blob_id"]).filename.to_s end end else diff --git a/test/controllers/variants_controller_test.rb b/test/controllers/variants_controller_test.rb index d2b5c32df7..8d437bedbb 100644 --- a/test/controllers/variants_controller_test.rb +++ b/test/controllers/variants_controller_test.rb @@ -14,7 +14,7 @@ class ActiveStorage::VariantsControllerTest < ActionController::TestCase test "showing variant inline" do get :show, params: { filename: @blob.filename, - encoded_blob_key: ActiveStorage::VerifiedKeyWithExpiration.encode(@blob.key, expires_in: 5.minutes), + signed_blob_id: @blob.signed_id, variation_key: ActiveStorage::Variation.encode(resize: "100x100") } assert_redirected_to /racecar.jpg\?disposition=inline/ diff --git a/test/models/attachments_test.rb b/test/models/attachments_test.rb index bfe631e5cb..c0f5db819d 100644 --- a/test/models/attachments_test.rb +++ b/test/models/attachments_test.rb @@ -21,7 +21,7 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase end test "attach existing sgid blob" do - @user.avatar.attach create_blob(filename: "funky.jpg").to_sgid.to_s + @user.avatar.attach create_blob(filename: "funky.jpg").signed_id assert_equal "funky.jpg", @user.avatar.filename.to_s end diff --git a/test/models/blob_test.rb b/test/models/blob_test.rb index 02fe653c33..ded11e5dbe 100644 --- a/test/models/blob_test.rb +++ b/test/models/blob_test.rb @@ -1,5 +1,6 @@ require "test_helper" require "database/setup" +require "active_storage/verified_key_with_expiration" class ActiveStorage::BlobTest < ActiveSupport::TestCase test "create after upload sets byte size and checksum" do diff --git a/test/models/verified_key_with_expiration_test.rb b/test/models/verified_key_with_expiration_test.rb index ee4dc7e02e..dd69e7cb10 100644 --- a/test/models/verified_key_with_expiration_test.rb +++ b/test/models/verified_key_with_expiration_test.rb @@ -1,5 +1,6 @@ require "test_helper" require "active_support/core_ext/securerandom" +require "active_storage/verified_key_with_expiration" class ActiveStorage::VerifiedKeyWithExpirationTest < ActiveSupport::TestCase FIXTURE_KEY = SecureRandom.base58(24) diff --git a/test/test_helper.rb b/test/test_helper.rb index 1d9737c4a4..650e997205 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -28,11 +28,6 @@ ActiveStorage::Blob.service = ActiveStorage::Service::DiskService.new(root: Dir.mktmpdir("active_storage_tests")) ActiveStorage::Service.logger = ActiveSupport::Logger.new(STDOUT) -require "active_storage/verified_key_with_expiration" -ActiveStorage::VerifiedKeyWithExpiration.verifier = ActiveSupport::MessageVerifier.new("Testing") - -require "active_storage/variation" -ActiveStorage::Variation.verifier = ActiveSupport::MessageVerifier.new("Testing") ActiveStorage.verifier = ActiveSupport::MessageVerifier.new("Testing") class ActiveSupport::TestCase @@ -71,4 +66,3 @@ class ActionController::TestCase require "global_id" GlobalID.app = "ActiveStorageExampleApp" ActiveRecord::Base.send :include, GlobalID::Identification -SignedGlobalID.verifier = ActiveStorage::VerifiedKeyWithExpiration.verifier -- GitLab