From e0714cbe743a248f31ffc6338e9b5c428ad93e90 Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Fri, 28 Jan 2022 18:06:01 -0500 Subject: [PATCH 1/2] Revert "Pass service_name param to DirectUploadsController" This reverts commit 193289dbbe146c56ec16faf8dd1a2c88611feb83. --- .../app/assets/javascripts/actiontext.js | 34 +-- .../app/helpers/action_text/tag_helper.rb | 8 - actiontext/test/template/form_helper_test.rb | 243 ++++++++---------- .../lib/action_view/helpers/form_helper.rb | 2 +- .../action_view/helpers/form_tag_helper.rb | 18 +- .../assets/javascripts/activestorage.esm.js | 22 +- .../app/assets/javascripts/activestorage.js | 22 +- .../direct_uploads_controller.rb | 8 +- .../javascript/activestorage/blob_record.js | 13 +- .../javascript/activestorage/direct_upload.js | 6 +- .../activestorage/direct_upload_controller.js | 10 +- activestorage/lib/active_storage.rb | 1 - .../lib/active_storage/direct_upload_token.rb | 59 ----- activestorage/lib/active_storage/errors.rb | 3 - .../direct_uploads_controller_test.rb | 64 +---- .../test/direct_upload_token_test.rb | 54 ---- guides/source/active_storage_overview.md | 13 +- 17 files changed, 151 insertions(+), 429 deletions(-) delete mode 100644 activestorage/lib/active_storage/direct_upload_token.rb delete mode 100644 activestorage/test/direct_upload_token_test.rb diff --git a/actiontext/app/assets/javascripts/actiontext.js b/actiontext/app/assets/javascripts/actiontext.js index f65f244028950..792a2c1fc310f 100644 --- a/actiontext/app/assets/javascripts/actiontext.js +++ b/actiontext/app/assets/javascripts/actiontext.js @@ -506,16 +506,14 @@ var activestorage = {exports: {}}; } } class BlobRecord { - constructor(file, checksum, url, directUploadToken, attachmentName) { + constructor(file, checksum, url) { this.file = file; this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, - checksum: checksum, + checksum: checksum }; - this.directUploadToken = directUploadToken; - this.attachmentName = attachmentName; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); this.xhr.responseType = "json"; @@ -543,9 +541,7 @@ var activestorage = {exports: {}}; create(callback) { this.callback = callback; this.xhr.send(JSON.stringify({ - blob: this.attributes, - direct_upload_token: this.directUploadToken, - attachment_name: this.attachmentName + blob: this.attributes })); } requestDidLoad(event) { @@ -603,12 +599,10 @@ var activestorage = {exports: {}}; } let id = 0; class DirectUpload { - constructor(file, url, directUploadToken, attachmentName, delegate) { + constructor(file, url, delegate) { this.id = ++id; this.file = file; this.url = url; - this.directUploadToken = directUploadToken; - this.attachmentName = attachmentName; this.delegate = delegate; } create(callback) { @@ -617,7 +611,7 @@ var activestorage = {exports: {}}; callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url, this.directUploadToken, this.attachmentName); + const blob = new BlobRecord(this.file, checksum, this.url); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -646,7 +640,7 @@ var activestorage = {exports: {}}; constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this.directUploadToken, this.attachmentName, this); + this.directUpload = new DirectUpload(this.file, this.url, this); this.dispatch("initialize"); } start(callback) { @@ -677,12 +671,6 @@ var activestorage = {exports: {}}; get url() { return this.input.getAttribute("data-direct-upload-url"); } - get directUploadToken() { - return this.input.getAttribute("data-direct-upload-token"); - } - get attachmentName() { - return this.input.getAttribute("data-direct-upload-attachment-name"); - } dispatch(name, detail = {}) { detail.file = this.file; detail.id = this.directUpload.id; @@ -842,7 +830,7 @@ class AttachmentUpload { constructor(attachment, element) { this.attachment = attachment; this.element = element; - this.directUpload = new activestorage.exports.DirectUpload(attachment.file, this.directUploadUrl, this.directUploadToken, this.directUploadAttachmentName, this); + this.directUpload = new activestorage.exports.DirectUpload(attachment.file, this.directUploadUrl, this); } start() { @@ -877,14 +865,6 @@ class AttachmentUpload { return this.element.dataset.directUploadUrl } - get directUploadToken() { - return this.element.dataset.directUploadToken - } - - get directUploadAttachmentName() { - return this.element.dataset.directUploadAttachmentName - } - get blobUrlTemplate() { return this.element.dataset.blobUrlTemplate } diff --git a/actiontext/app/helpers/action_text/tag_helper.rb b/actiontext/app/helpers/action_text/tag_helper.rb index 8307038f5d439..2f216737a5c08 100644 --- a/actiontext/app/helpers/action_text/tag_helper.rb +++ b/actiontext/app/helpers/action_text/tag_helper.rb @@ -32,14 +32,6 @@ def rich_text_area_tag(name, value = nil, options = {}) options[:data][:direct_upload_url] ||= main_app.rails_direct_uploads_url options[:data][:blob_url_template] ||= main_app.rails_service_blob_url(":signed_id", ":filename") - class_with_attachment = "ActionText::RichText#embeds" - options[:data][:direct_upload_attachment_name] ||= class_with_attachment - options[:data][:direct_upload_token] = ActiveStorage::DirectUploadToken.generate_direct_upload_token( - class_with_attachment, - ActiveStorage::Blob.service.name, - session - ) - editor_tag = content_tag("trix-editor", "", options) input_tag = hidden_field_tag(name, value.try(:to_trix_html) || value, id: options[:input], form: form) diff --git a/actiontext/test/template/form_helper_test.rb b/actiontext/test/template/form_helper_test.rb index 004916ce949b6..c87d53fe3c95a 100644 --- a/actiontext/test/template/form_helper_test.rb +++ b/actiontext/test/template/form_helper_test.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "test_helper" -require "minitest/mock" class ActionText::FormHelperTest < ActionView::TestCase tests ActionText::TagHelper @@ -29,184 +28,158 @@ def form_with(*, **) test "rich text area tag" do message = Message.new - with_stub_token do - form_with model: message, scope: :message do |form| - rich_text_area_tag :content, message.content, { input: "trix_input_1" } - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: message, scope: :message do |form| + rich_text_area_tag :content, message.content, { input: "trix_input_1" } end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area" do - with_stub_token do - form_with model: Message.new, scope: :message do |form| - form.rich_text_area :content - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: Message.new, scope: :message do |form| + form.rich_text_area :content end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area having class" do - with_stub_token do - form_with model: Message.new, scope: :message do |form| - form.rich_text_area :content, class: "custom-class" - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: Message.new, scope: :message do |form| + form.rich_text_area :content, class: "custom-class" end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area for non-attribute" do - with_stub_token do - form_with model: Message.new, scope: :message do |form| - form.rich_text_area :not_an_attribute - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: Message.new, scope: :message do |form| + form.rich_text_area :not_an_attribute end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "modelless form with rich text area" do - with_stub_token do - form_with url: "/messages", scope: :message do |form| - form.rich_text_area :content, { input: "trix_input_2" } - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with url: "/messages", scope: :message do |form| + form.rich_text_area :content, { input: "trix_input_2" } end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area having placeholder without locale" do - with_stub_token do - form_with model: Message.new, scope: :message do |form| - form.rich_text_area :content, placeholder: true - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: Message.new, scope: :message do |form| + form.rich_text_area :content, placeholder: true end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area having placeholder with locale" do I18n.with_locale :placeholder do form_with model: Message.new, scope: :message do |form| - with_stub_token do - form.rich_text_area :title, placeholder: true - end + form.rich_text_area :title, placeholder: true end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area with value" do - with_stub_token do - form_with model: Message.new, scope: :message do |form| - form.rich_text_area :title, value: "

hello world

" - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: Message.new, scope: :message do |form| + form.rich_text_area :title, value: "

hello world

" end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area with form attribute" do - with_stub_token do - form_with model: Message.new, scope: :message do |form| - form.rich_text_area :title, form: "other_form" - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: Message.new, scope: :message do |form| + form.rich_text_area :title, form: "other_form" end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area with data[direct_upload_url]" do - with_stub_token do - form_with model: Message.new, scope: :message do |form| - form.rich_text_area :content, data: { direct_upload_url: "http://test.host/direct_uploads" } - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: Message.new, scope: :message do |form| + form.rich_text_area :content, data: { direct_upload_url: "http://test.host/direct_uploads" } end + + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end test "form with rich text area with data[blob_url_template]" do - with_stub_token do - form_with model: Message.new, scope: :message do |form| - form.rich_text_area :content, data: { blob_url_template: "http://test.host/blobs/:signed_id/:filename" } - end - - assert_dom_equal \ - '
' \ - '' \ - '' \ - "" \ - "
", - output_buffer + form_with model: Message.new, scope: :message do |form| + form.rich_text_area :content, data: { blob_url_template: "http://test.host/blobs/:signed_id/:filename" } end - end - def with_stub_token(&block) - ActiveStorage::DirectUploadToken.stub(:generate_direct_upload_token, "token", &block) + assert_dom_equal \ + '
' \ + '' \ + '' \ + "" \ + "
", + output_buffer end end diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb index eb24cc06eeae6..321d6b43e0cf1 100644 --- a/actionview/lib/action_view/helpers/form_helper.rb +++ b/actionview/lib/action_view/helpers/form_helper.rb @@ -1241,7 +1241,7 @@ def hidden_field(object_name, method, options = {}) def file_field(object_name, method, options = {}) options = { include_hidden: multiple_file_field_include_hidden }.merge!(options) - Tags::FileField.new(object_name, method, self, convert_direct_upload_option_to_url(method, options)).render + Tags::FileField.new(object_name, method, self, convert_direct_upload_option_to_url(options.dup)).render end # Returns a textarea opening and closing tag set tailored for accessing a specified attribute (identified by +method+) diff --git a/actionview/lib/action_view/helpers/form_tag_helper.rb b/actionview/lib/action_view/helpers/form_tag_helper.rb index c6ac5012a739e..7d08cf8d0174d 100644 --- a/actionview/lib/action_view/helpers/form_tag_helper.rb +++ b/actionview/lib/action_view/helpers/form_tag_helper.rb @@ -342,7 +342,7 @@ def hidden_field_tag(name, value = nil, options = {}) # file_field_tag 'file', accept: 'text/html', class: 'upload', value: 'index.html' # # => def file_field_tag(name, options = {}) - text_field_tag(name, nil, convert_direct_upload_option_to_url(name, options.merge(type: :file))) + text_field_tag(name, nil, convert_direct_upload_option_to_url(options.merge(type: :file))) end # Creates a password field, a masked text field that will hide the users input behind a mask character. @@ -984,23 +984,9 @@ def set_default_disable_with(value, tag_options) tag_options.delete("data-disable-with") end - def convert_direct_upload_option_to_url(name, options) + def convert_direct_upload_option_to_url(options) if options.delete(:direct_upload) && respond_to?(:rails_direct_uploads_url) options["data-direct-upload-url"] = rails_direct_uploads_url - - if options[:object] && options[:object].class.respond_to?(:reflect_on_attachment) - attachment_reflection = options[:object].class.reflect_on_attachment(name) - - class_with_attachment = "#{options[:object].class.name.underscore}##{name}" - options["data-direct-upload-attachment-name"] = class_with_attachment - - service_name = attachment_reflection.options[:service_name] || ActiveStorage::Blob.service.name - options["data-direct-upload-token"] = ActiveStorage::DirectUploadToken.generate_direct_upload_token( - class_with_attachment, - service_name, - session - ) - end end options end diff --git a/activestorage/app/assets/javascripts/activestorage.esm.js b/activestorage/app/assets/javascripts/activestorage.esm.js index dad68d5373441..caa3a14860bdf 100644 --- a/activestorage/app/assets/javascripts/activestorage.esm.js +++ b/activestorage/app/assets/javascripts/activestorage.esm.js @@ -508,7 +508,7 @@ function toArray(value) { } class BlobRecord { - constructor(file, checksum, url, directUploadToken, attachmentName) { + constructor(file, checksum, url) { this.file = file; this.attributes = { filename: file.name, @@ -516,8 +516,6 @@ class BlobRecord { byte_size: file.size, checksum: checksum }; - this.directUploadToken = directUploadToken; - this.attachmentName = attachmentName; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); this.xhr.responseType = "json"; @@ -545,9 +543,7 @@ class BlobRecord { create(callback) { this.callback = callback; this.xhr.send(JSON.stringify({ - blob: this.attributes, - direct_upload_token: this.directUploadToken, - attachment_name: this.attachmentName + blob: this.attributes })); } requestDidLoad(event) { @@ -608,12 +604,10 @@ class BlobUpload { let id = 0; class DirectUpload { - constructor(file, url, serviceName, attachmentName, delegate) { + constructor(file, url, delegate) { this.id = ++id; this.file = file; this.url = url; - this.serviceName = serviceName; - this.attachmentName = attachmentName; this.delegate = delegate; } create(callback) { @@ -622,7 +616,7 @@ class DirectUpload { callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url, this.serviceName, this.attachmentName); + const blob = new BlobRecord(this.file, checksum, this.url); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -653,7 +647,7 @@ class DirectUploadController { constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this.directUploadToken, this.attachmentName, this); + this.directUpload = new DirectUpload(this.file, this.url, this); this.dispatch("initialize"); } start(callback) { @@ -684,12 +678,6 @@ class DirectUploadController { get url() { return this.input.getAttribute("data-direct-upload-url"); } - get directUploadToken() { - return this.input.getAttribute("data-direct-upload-token"); - } - get attachmentName() { - return this.input.getAttribute("data-direct-upload-attachment-name"); - } dispatch(name, detail = {}) { detail.file = this.file; detail.id = this.directUpload.id; diff --git a/activestorage/app/assets/javascripts/activestorage.js b/activestorage/app/assets/javascripts/activestorage.js index 8291629fad15d..9e6a5bf79799b 100644 --- a/activestorage/app/assets/javascripts/activestorage.js +++ b/activestorage/app/assets/javascripts/activestorage.js @@ -503,7 +503,7 @@ } } class BlobRecord { - constructor(file, checksum, url, directUploadToken, attachmentName) { + constructor(file, checksum, url) { this.file = file; this.attributes = { filename: file.name, @@ -511,8 +511,6 @@ byte_size: file.size, checksum: checksum }; - this.directUploadToken = directUploadToken; - this.attachmentName = attachmentName; this.xhr = new XMLHttpRequest; this.xhr.open("POST", url, true); this.xhr.responseType = "json"; @@ -540,9 +538,7 @@ create(callback) { this.callback = callback; this.xhr.send(JSON.stringify({ - blob: this.attributes, - direct_upload_token: this.directUploadToken, - attachment_name: this.attachmentName + blob: this.attributes })); } requestDidLoad(event) { @@ -600,12 +596,10 @@ } let id = 0; class DirectUpload { - constructor(file, url, serviceName, attachmentName, delegate) { + constructor(file, url, delegate) { this.id = ++id; this.file = file; this.url = url; - this.serviceName = serviceName; - this.attachmentName = attachmentName; this.delegate = delegate; } create(callback) { @@ -614,7 +608,7 @@ callback(error); return; } - const blob = new BlobRecord(this.file, checksum, this.url, this.serviceName, this.attachmentName); + const blob = new BlobRecord(this.file, checksum, this.url); notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr); blob.create((error => { if (error) { @@ -643,7 +637,7 @@ constructor(input, file) { this.input = input; this.file = file; - this.directUpload = new DirectUpload(this.file, this.url, this.directUploadToken, this.attachmentName, this); + this.directUpload = new DirectUpload(this.file, this.url, this); this.dispatch("initialize"); } start(callback) { @@ -674,12 +668,6 @@ get url() { return this.input.getAttribute("data-direct-upload-url"); } - get directUploadToken() { - return this.input.getAttribute("data-direct-upload-token"); - } - get attachmentName() { - return this.input.getAttribute("data-direct-upload-attachment-name"); - } dispatch(name, detail = {}) { detail.file = this.file; detail.id = this.directUpload.id; diff --git a/activestorage/app/controllers/active_storage/direct_uploads_controller.rb b/activestorage/app/controllers/active_storage/direct_uploads_controller.rb index bfd622d8f7eb9..99634597f3f90 100644 --- a/activestorage/app/controllers/active_storage/direct_uploads_controller.rb +++ b/activestorage/app/controllers/active_storage/direct_uploads_controller.rb @@ -4,10 +4,8 @@ # 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 < ActiveStorage::BaseController - include ActiveStorage::DirectUploadToken - def create - blob = ActiveStorage::Blob.create_before_direct_upload!(**blob_args.merge(service_name: verified_service_name)) + blob = ActiveStorage::Blob.create_before_direct_upload!(**blob_args) render json: direct_upload_json(blob) end @@ -16,10 +14,6 @@ def blob_args params.require(:blob).permit(:filename, :byte_size, :checksum, :content_type, metadata: {}).to_h.symbolize_keys end - def verified_service_name - ActiveStorage::DirectUploadToken.verify_direct_upload_token(params[:direct_upload_token], params[:attachment_name], session) - end - def direct_upload_json(blob) blob.as_json(root: false, methods: :signed_id).merge(direct_upload: { url: blob.service_url_for_direct_upload, diff --git a/activestorage/app/javascript/activestorage/blob_record.js b/activestorage/app/javascript/activestorage/blob_record.js index cf06d97cf17f1..997c123870aee 100644 --- a/activestorage/app/javascript/activestorage/blob_record.js +++ b/activestorage/app/javascript/activestorage/blob_record.js @@ -1,19 +1,16 @@ import { getMetaValue } from "./helpers" export class BlobRecord { - constructor(file, checksum, url, directUploadToken, attachmentName) { + constructor(file, checksum, url) { this.file = file this.attributes = { filename: file.name, content_type: file.type || "application/octet-stream", byte_size: file.size, - checksum: checksum, + checksum: checksum } - this.directUploadToken = directUploadToken - this.attachmentName = attachmentName - this.xhr = new XMLHttpRequest this.xhr.open("POST", url, true) this.xhr.responseType = "json" @@ -46,11 +43,7 @@ export class BlobRecord { create(callback) { this.callback = callback - this.xhr.send(JSON.stringify({ - blob: this.attributes, - direct_upload_token: this.directUploadToken, - attachment_name: this.attachmentName - })) + this.xhr.send(JSON.stringify({ blob: this.attributes })) } requestDidLoad(event) { diff --git a/activestorage/app/javascript/activestorage/direct_upload.js b/activestorage/app/javascript/activestorage/direct_upload.js index e5c279d173d60..c2eedf289b807 100644 --- a/activestorage/app/javascript/activestorage/direct_upload.js +++ b/activestorage/app/javascript/activestorage/direct_upload.js @@ -5,12 +5,10 @@ import { BlobUpload } from "./blob_upload" let id = 0 export class DirectUpload { - constructor(file, url, serviceName, attachmentName, delegate) { + constructor(file, url, delegate) { this.id = ++id this.file = file this.url = url - this.serviceName = serviceName - this.attachmentName = attachmentName this.delegate = delegate } @@ -21,7 +19,7 @@ export class DirectUpload { return } - const blob = new BlobRecord(this.file, checksum, this.url, this.serviceName, this.attachmentName) + const blob = new BlobRecord(this.file, checksum, this.url) notify(this.delegate, "directUploadWillCreateBlobWithXHR", blob.xhr) blob.create(error => { diff --git a/activestorage/app/javascript/activestorage/direct_upload_controller.js b/activestorage/app/javascript/activestorage/direct_upload_controller.js index 40bb1b98c51b4..987050889a750 100644 --- a/activestorage/app/javascript/activestorage/direct_upload_controller.js +++ b/activestorage/app/javascript/activestorage/direct_upload_controller.js @@ -5,7 +5,7 @@ export class DirectUploadController { constructor(input, file) { this.input = input this.file = file - this.directUpload = new DirectUpload(this.file, this.url, this.directUploadToken, this.attachmentName, this) + this.directUpload = new DirectUpload(this.file, this.url, this) this.dispatch("initialize") } @@ -41,14 +41,6 @@ export class DirectUploadController { return this.input.getAttribute("data-direct-upload-url") } - get directUploadToken() { - return this.input.getAttribute("data-direct-upload-token") - } - - get attachmentName() { - return this.input.getAttribute("data-direct-upload-attachment-name") - } - dispatch(name, detail = {}) { detail.file = this.file detail.id = this.directUpload.id diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb index 1571ec0b3dff1..1450fcaef0b40 100644 --- a/activestorage/lib/active_storage.rb +++ b/activestorage/lib/active_storage.rb @@ -41,7 +41,6 @@ module ActiveStorage autoload :Service autoload :Previewer autoload :Analyzer - autoload :DirectUploadToken mattr_accessor :logger mattr_accessor :verifier diff --git a/activestorage/lib/active_storage/direct_upload_token.rb b/activestorage/lib/active_storage/direct_upload_token.rb deleted file mode 100644 index 9b5aa1fecc194..0000000000000 --- a/activestorage/lib/active_storage/direct_upload_token.rb +++ /dev/null @@ -1,59 +0,0 @@ -# frozen_string_literal: true - -module ActiveStorage - module DirectUploadToken - extend self - - SEPARATOR = "." - DIRECT_UPLOAD_TOKEN_LENGTH = 32 - - def generate_direct_upload_token(attachment_name, service_name, session) - token = direct_upload_token(session, attachment_name) - encode_direct_upload_token([service_name, token].join(SEPARATOR)) - end - - def verify_direct_upload_token(token, attachment_name, session) - raise ActiveStorage::InvalidDirectUploadTokenError if token.nil? - - service_name, *token_components = decode_token(token).split(SEPARATOR) - decoded_token = token_components.join(SEPARATOR) - - return service_name if valid_direct_upload_token?(decoded_token, attachment_name, session) - - raise ActiveStorage::InvalidDirectUploadTokenError - end - - private - def direct_upload_token(session, attachment_name) # :doc: - direct_upload_token_hmac(session, "direct_upload##{attachment_name}") - end - - def valid_direct_upload_token?(token, attachment_name, session) # :doc: - correct_token = direct_upload_token(session, attachment_name) - ActiveSupport::SecurityUtils.fixed_length_secure_compare(token, correct_token) - rescue ArgumentError - raise ActiveStorage::InvalidDirectUploadTokenError - end - - def direct_upload_token_hmac(session, identifier) # :doc: - OpenSSL::HMAC.digest( - OpenSSL::Digest::SHA256.new, - real_direct_upload_token(session), - identifier - ) - end - - def real_direct_upload_token(session) # :doc: - session[:_direct_upload_token] ||= SecureRandom.urlsafe_base64(DIRECT_UPLOAD_TOKEN_LENGTH, padding: false) - encode_direct_upload_token(session[:_direct_upload_token]) - end - - def decode_token(encoded_token) # :nodoc: - Base64.urlsafe_decode64(encoded_token) - end - - def encode_direct_upload_token(raw_token) # :nodoc: - Base64.urlsafe_encode64(raw_token) - end - end -end diff --git a/activestorage/lib/active_storage/errors.rb b/activestorage/lib/active_storage/errors.rb index 816e5f31e09cc..df544a12bc2cd 100644 --- a/activestorage/lib/active_storage/errors.rb +++ b/activestorage/lib/active_storage/errors.rb @@ -26,7 +26,4 @@ class FileNotFoundError < Error; end # Raised when a Previewer is unable to generate a preview image. class PreviewError < Error; end - - # Raised when direct upload fails because of the invalid token - class InvalidDirectUploadTokenError < Error; end end diff --git a/activestorage/test/controllers/direct_uploads_controller_test.rb b/activestorage/test/controllers/direct_uploads_controller_test.rb index a68e5662d5de1..38d2bd542b6bf 100644 --- a/activestorage/test/controllers/direct_uploads_controller_test.rb +++ b/activestorage/test/controllers/direct_uploads_controller_test.rb @@ -2,7 +2,6 @@ require "test_helper" require "database/setup" -require "minitest/mock" if SERVICE_CONFIGURATIONS[:s3] && SERVICE_CONFIGURATIONS[:s3][:access_key_id].present? class ActiveStorage::S3DirectUploadsControllerTest < ActionDispatch::IntegrationTest @@ -28,10 +27,8 @@ class ActiveStorage::S3DirectUploadsControllerTest < ActionDispatch::Integration } } - ActiveStorage::DirectUploadToken.stub(:verify_direct_upload_token, "s3") do - post rails_direct_uploads_url, params: { blob: { - filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } - end + post rails_direct_uploads_url, params: { blob: { + filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) @@ -76,10 +73,8 @@ class ActiveStorage::GCSDirectUploadsControllerTest < ActionDispatch::Integratio } } - ActiveStorage::DirectUploadToken.stub(:verify_direct_upload_token, "gcs") do - post rails_direct_uploads_url, params: { blob: { - filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } - end + post rails_direct_uploads_url, params: { blob: { + filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } @response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) @@ -120,10 +115,8 @@ class ActiveStorage::AzureStorageDirectUploadsControllerTest < ActionDispatch::I "library_ID": "12345" } - ActiveStorage::DirectUploadToken.stub(:verify_direct_upload_token, "azure") do - post rails_direct_uploads_url, params: { blob: { - filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } - end + post rails_direct_uploads_url, params: { blob: { + filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } @response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) @@ -152,10 +145,8 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati "library_ID": "12345" } - with_valid_service_name do - post rails_direct_uploads_url, params: { blob: { - filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } - end + post rails_direct_uploads_url, params: { blob: { + filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } @response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) @@ -179,43 +170,14 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati "library_ID": "12345" } - with_valid_service_name do - set_include_root_in_json(true) do - post rails_direct_uploads_url, params: { blob: { - filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } - end - end - - @response.parsed_body.tap do |details| - assert_nil details["blob"] - assert_not_nil details["id"] - end - end - - test "handling direct upload with custom service name" do - checksum = OpenSSL::Digest::MD5.base64digest("Hello") - metadata = { - "foo": "bar", - "my_key_1": "my_value_1", - "my_key_2": "my_value_2", - "platform": "my_platform", - "library_ID": "12345" - } - - with_valid_service_name do + set_include_root_in_json(true) do post rails_direct_uploads_url, params: { blob: { filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } end @response.parsed_body.tap do |details| - 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"] - assert_equal metadata, details["metadata"].deep_transform_keys(&:to_sym) - assert_equal "text/plain", details["content_type"] - assert_match(/rails\/active_storage\/disk/, details["direct_upload"]["url"]) - assert_equal({ "Content-Type" => "text/plain" }, details["direct_upload"]["headers"]) + assert_nil details["blob"] + assert_not_nil details["id"] end end @@ -227,8 +189,4 @@ def set_include_root_in_json(value) ensure ActiveRecord::Base.include_root_in_json = original end - - def with_valid_service_name(&block) - ActiveStorage::DirectUploadToken.stub(:verify_direct_upload_token, "local", &block) - end end diff --git a/activestorage/test/direct_upload_token_test.rb b/activestorage/test/direct_upload_token_test.rb deleted file mode 100644 index ee0526c828d7a..0000000000000 --- a/activestorage/test/direct_upload_token_test.rb +++ /dev/null @@ -1,54 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -class DirectUploadTokenTest < ActionController::TestCase - setup do - @session = {} - @service_name = "local" - @avatar_attachment_name = "user#avatar" - end - - def test_validates_correct_token - token = ActiveStorage::DirectUploadToken.generate_direct_upload_token(@avatar_attachment_name, @service_name, @session) - verified_service_name = ActiveStorage::DirectUploadToken.verify_direct_upload_token(token, @avatar_attachment_name, @session) - - assert_equal verified_service_name, @service_name - end - - def test_not_validates_nil_token - token = nil - - assert_raises(ActiveStorage::InvalidDirectUploadTokenError) do - ActiveStorage::DirectUploadToken.verify_direct_upload_token(token, @avatar_attachment_name, @session) - end - end - - def test_not_validates_token_when_session_is_empty - token = ActiveStorage::DirectUploadToken.generate_direct_upload_token(@avatar_attachment_name, @service_name, {}) - - assert_raises(ActiveStorage::InvalidDirectUploadTokenError) do - ActiveStorage::DirectUploadToken.verify_direct_upload_token(token, @avatar_attachment_name, @session) - end - end - - def test_not_validates_token_from_different_attachment - background_attachment_name = "user#background" - token = ActiveStorage::DirectUploadToken.generate_direct_upload_token(background_attachment_name, @service_name, @session) - - assert_raises(ActiveStorage::InvalidDirectUploadTokenError) do - ActiveStorage::DirectUploadToken.verify_direct_upload_token(token, @avatar_attachment_name, @session) - end - end - - def test_not_validates_token_from_different_session - token = ActiveStorage::DirectUploadToken.generate_direct_upload_token(@avatar_attachment_name, @service_name, @session) - - another_session = {} - ActiveStorage::DirectUploadToken.generate_direct_upload_token(@avatar_attachment_name, @service_name, another_session) - - assert_raises(ActiveStorage::InvalidDirectUploadTokenError) do - ActiveStorage::DirectUploadToken.verify_direct_upload_token(token, @avatar_attachment_name, another_session) - end - end -end diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 0aa26b6f6ee66..8fe9abee3046f 100644 --- a/guides/source/active_storage_overview.md +++ b/guides/source/active_storage_overview.md @@ -1146,12 +1146,9 @@ input.addEventListener('change', (event) => { const uploadFile = (file) => { // your form needs the file_field direct_upload: true, which - // provides data-direct-upload-url, data-direct-upload-token - // and data-direct-upload-attachment-name + // provides data-direct-upload-url const url = input.dataset.directUploadUrl - const token = input.dataset.directUploadToken - const attachmentName = input.dataset.directUploadAttachmentName - const upload = new DirectUpload(file, url, token, attachmentName) + const upload = new DirectUpload(file, url) upload.create((error, blob) => { if (error) { @@ -1170,7 +1167,7 @@ const uploadFile = (file) => { } ``` -If you need to track the progress of the file upload, you can pass a fifth +If you need to track the progress of the file upload, you can pass a third parameter to the `DirectUpload` constructor. During the upload, DirectUpload will call the object's `directUploadWillStoreFileWithXHR` method. You can then bind your own progress handler on the XHR. @@ -1179,8 +1176,8 @@ bind your own progress handler on the XHR. import { DirectUpload } from "@rails/activestorage" class Uploader { - constructor(file, url, token, attachmentName) { - this.upload = new DirectUpload(file, url, token, attachmentName, this) + constructor(file, url) { + this.upload = new DirectUpload(this.file, this.url, this) } upload(file) { From cb8c4d550e1c7cb099865f0798bcf46235b1ef19 Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Fri, 28 Jan 2022 18:17:19 -0500 Subject: [PATCH 2/2] Revert "Multi-service direct uploads in Action Text attachment uploads" This reverts commit 0b69ad4de6ef89c285833a90dd23db25cad7b669. --- .../app/javascript/actiontext/attachment_upload.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/actiontext/app/javascript/actiontext/attachment_upload.js b/actiontext/app/javascript/actiontext/attachment_upload.js index 974a7330b5ec3..77fbc97df64db 100644 --- a/actiontext/app/javascript/actiontext/attachment_upload.js +++ b/actiontext/app/javascript/actiontext/attachment_upload.js @@ -4,7 +4,7 @@ export class AttachmentUpload { constructor(attachment, element) { this.attachment = attachment this.element = element - this.directUpload = new DirectUpload(attachment.file, this.directUploadUrl, this.directUploadToken, this.attachmentName, this) + this.directUpload = new DirectUpload(attachment.file, this.directUploadUrl, this) } start() { @@ -42,11 +42,4 @@ export class AttachmentUpload { get blobUrlTemplate() { return this.element.dataset.blobUrlTemplate } - - get directUploadToken() { - return this.element.getAttribute("data-direct-upload-token"); - } - get attachmentName() { - return this.element.getAttribute("data-direct-upload-attachment-name"); - } }