Skip to content

Commit

Permalink
Use Active Record signed IDs in Active Storage
Browse files Browse the repository at this point in the history
  • Loading branch information
santib authored and georgeclaghorn committed Jul 5, 2020
1 parent fce2d6a commit 31148cd
Show file tree
Hide file tree
Showing 6 changed files with 25 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module ActiveStorage::SetBlob #:nodoc:

private
def set_blob
@blob = ActiveStorage::Blob.find_signed(params[:signed_blob_id] || params[:signed_id])
@blob = ActiveStorage::Blob.find_signed!(params[:signed_blob_id] || params[:signed_id])
rescue ActiveSupport::MessageVerifier::InvalidSignature
head :not_found
end
Expand Down
13 changes: 9 additions & 4 deletions activestorage/app/models/active_storage/blob.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class ActiveStorage::Blob < ActiveRecord::Base
include ActiveStorage::Blob::Representable

self.table_name = "active_storage_blobs"
self.signed_id_verifier = ActiveStorage.verifier

MINIMUM_TOKEN_LENGTH = 28

Expand Down Expand Up @@ -72,8 +73,8 @@ class << self
# that was created ahead of the upload itself on form submission.
#
# The signed ID is also used to create stable URLs for the blob through the BlobsController.
def find_signed(id, record: nil)
find ActiveStorage.verifier.verify(id, purpose: :blob_id)
def find_signed!(id, record: nil)
super(id, purpose: :blob_id)
end

def build_after_upload(io:, filename:, content_type: nil, metadata: nil, service_name: nil, identify: true, record: nil) #:nodoc:
Expand Down Expand Up @@ -125,12 +126,16 @@ def create_before_direct_upload!(key: nil, filename:, byte_size:, checksum:, con
def generate_unique_secure_token(length: MINIMUM_TOKEN_LENGTH)
SecureRandom.base36(length)
end

# Customize signed ID purposes for backwards compatibility.
def combine_signed_id_purposes(purpose)
purpose.to_s
end
end

# Returns a signed ID for this blob that's suitable for reference on the client-side without fear of tampering.
# It uses the framework-wide verifier on <tt>ActiveStorage.verifier</tt>, but with a dedicated purpose.
def signed_id
ActiveStorage.verifier.generate(id, purpose: :blob_id)
super(purpose: :blob_id)
end

# Returns the key pointing to the file on the service that's associated with this blob. The key is the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def find_or_build_blob
)
)
when String
ActiveStorage::Blob.find_signed(attachable, record: record)
ActiveStorage::Blob.find_signed!(attachable, record: record)
else
raise ArgumentError, "Could not find or build blob: expected attachable, got #{attachable.inspect}"
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class ActiveStorage::S3DirectUploadsControllerTest < ActionDispatch::Integration
filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain" } }

response.parsed_body.tap do |details|
assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed(details["signed_id"])
assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"])
assert_equal "hello.txt", details["filename"]
assert_equal 6, details["byte_size"]
assert_equal checksum, details["checksum"]
Expand Down Expand Up @@ -56,7 +56,7 @@ class ActiveStorage::GCSDirectUploadsControllerTest < ActionDispatch::Integratio
filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain" } }

@response.parsed_body.tap do |details|
assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed(details["signed_id"])
assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"])
assert_equal "hello.txt", details["filename"]
assert_equal 6, details["byte_size"]
assert_equal checksum, details["checksum"]
Expand Down Expand Up @@ -90,7 +90,7 @@ class ActiveStorage::AzureStorageDirectUploadsControllerTest < ActionDispatch::I
filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain" } }

@response.parsed_body.tap do |details|
assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed(details["signed_id"])
assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"])
assert_equal "hello.txt", details["filename"]
assert_equal 6, details["byte_size"]
assert_equal checksum, details["checksum"]
Expand All @@ -112,7 +112,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati
filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain" } }

@response.parsed_body.tap do |details|
assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed(details["signed_id"])
assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"])
assert_equal "hello.txt", details["filename"]
assert_equal 6, details["byte_size"]
assert_equal checksum, details["checksum"]
Expand Down
2 changes: 1 addition & 1 deletion activestorage/test/models/attached/one_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
test "attaching an existing blob from a signed ID passes record" do
blob = create_blob(filename: "funky.jpg")
arguments = [blob.signed_id, record: @user]
assert_called_with(ActiveStorage::Blob, :find_signed, arguments, returns: blob) do
assert_called_with(ActiveStorage::Blob, :find_signed!, arguments, returns: blob) do
@user.avatar.attach blob.signed_id
end
end
Expand Down
10 changes: 9 additions & 1 deletion activestorage/test/models/attachment_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ class ActiveStorage::AttachmentTest < ActiveSupport::TestCase
@user.avatar.attach(blob)

signed_id = @user.avatar.signed_id
assert_equal blob, ActiveStorage::Blob.find_signed(signed_id)
assert_equal blob, ActiveStorage::Blob.find_signed!(signed_id)
end

test "signed blob ID backwards compatibility" do
blob = create_blob
@user.avatar.attach(blob)

signed_id_generated_old_way = ActiveStorage.verifier.generate(@user.avatar.id, purpose: :blob_id)
assert_equal blob, ActiveStorage::Blob.find_signed!(signed_id_generated_old_way)
end
end

0 comments on commit 31148cd

Please sign in to comment.