From b500949ef688277c53c396e4d7e90cdbf4918d06 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 28 Jul 2021 08:42:25 +1000 Subject: [PATCH] FEATURE: Initial implementation of direct S3 uploads with uppy and stubs (#13787) This adds a few different things to allow for direct S3 uploads using uppy. **These changes are still not the default.** There are hidden `enable_experimental_image_uploader` and `enable_direct_s3_uploads` settings that must be turned on for any of this code to be used, and even if they are turned on only the User Card Background for the user profile actually uses uppy-image-uploader. A new `ExternalUploadStub` model and database table is introduced in this pull request. This is used to keep track of uploads that are uploaded to a temporary location in S3 with the direct to S3 code, and they are eventually deleted a) when the direct upload is completed and b) after a certain time period of not being used. ### Starting a direct S3 upload When an S3 direct upload is initiated with uppy, we first request a presigned PUT URL from the new `generate-presigned-put` endpoint in `UploadsController`. This generates an S3 key in the `temp` folder inside the correct bucket path, along with any metadata from the clientside (e.g. the SHA1 checksum described below). This will also create an `ExternalUploadStub` and store the details of the temp object key and the file being uploaded. Once the clientside has this URL, uppy will upload the file direct to S3 using the presigned URL. Once the upload is complete we go to the next stage. ### Completing a direct S3 upload Once the upload to S3 is done we call the new `complete-external-upload` route with the unique identifier of the `ExternalUploadStub` created earlier. Only the user who made the stub can complete the external upload. One of two paths is followed via the `ExternalUploadManager`. 1. If the object in S3 is too large (currently 100mb defined by `ExternalUploadManager::DOWNLOAD_LIMIT`) we do not download and generate the SHA1 for that file. Instead we create the `Upload` record via `UploadCreator` and simply copy it to its final destination on S3 then delete the initial temp file. Several modifications to `UploadCreator` have been made to accommodate this. 2. If the object in S3 is small enough, we download it. When the temporary S3 file is downloaded, we compare the SHA1 checksum generated by the browser with the actual SHA1 checksum of the file generated by ruby. The browser SHA1 checksum is stored on the object in S3 with metadata, and is generated via the `UppyChecksum` plugin. Keep in mind that some browsers will not generate this due to compatibility or other issues. We then follow the normal `UploadCreator` path with one exception. To cut down on having to re-upload the file again, if there are no changes (such as resizing etc) to the file in `UploadCreator` we follow the same copy + delete temp path that we do for files that are too large. 3. Finally we return the serialized upload record back to the client There are several errors that could happen that are handled by `UploadsController` as well. Also in this PR is some refactoring of `displayErrorForUpload` to handle both uppy and jquery file uploader errors. --- .../app/components/composer-editor.js | 2 +- .../app/components/create-invite-uploader.js | 2 +- .../app/components/uppy-image-uploader.js | 3 + .../javascripts/discourse/app/lib/uploads.js | 40 ++-- .../discourse/app/lib/uppy-checksum-plugin.js | 94 ++++++++ .../discourse/app/mixins/upload.js | 2 +- .../discourse/app/mixins/uppy-upload.js | 101 +++++++- .../components/uppy-image-uploader.hbs | 5 +- .../discourse/tests/unit/lib/uploads-test.js | 54 +++++ app/controllers/uploads_controller.rb | 89 ++++++- app/jobs/scheduled/clean_up_uploads.rb | 2 + app/models/external_upload_stub.rb | 68 ++++++ app/services/external_upload_manager.rb | 84 +++++++ config/locales/server.en.yml | 4 + config/routes.rb | 3 + config/site_settings.yml | 7 + ...0709042135_create_external_upload_stubs.rb | 18 ++ lib/file_store/base_store.rb | 10 + lib/file_store/s3_store.rb | 75 +++++- lib/s3_helper.rb | 19 +- lib/upload_creator.rb | 74 ++++-- spec/components/file_store/s3_store_spec.rb | 24 +- .../external_upload_stub_fabricator.rb | 19 ++ spec/jobs/clean_up_uploads_spec.rb | 9 + spec/multisite/s3_store_spec.rb | 62 ++++- spec/requests/uploads_controller_spec.rb | 155 ++++++++++++ spec/services/external_upload_manager_spec.rb | 222 ++++++++++++++++++ 27 files changed, 1167 insertions(+), 80 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/lib/uppy-checksum-plugin.js create mode 100644 app/models/external_upload_stub.rb create mode 100644 app/services/external_upload_manager.rb create mode 100644 db/migrate/20210709042135_create_external_upload_stubs.rb create mode 100644 spec/fabricators/external_upload_stub_fabricator.rb create mode 100644 spec/services/external_upload_manager_spec.rb diff --git a/app/assets/javascripts/discourse/app/components/composer-editor.js b/app/assets/javascripts/discourse/app/components/composer-editor.js index ce73e3b597a..c9d5566e671 100644 --- a/app/assets/javascripts/discourse/app/components/composer-editor.js +++ b/app/assets/javascripts/discourse/app/components/composer-editor.js @@ -845,7 +845,7 @@ export default Component.extend({ this._xhr = null; if (!userCancelled) { - displayErrorForUpload(data, this.siteSettings); + displayErrorForUpload(data, this.siteSettings, data.files[0].name); } }); }); diff --git a/app/assets/javascripts/discourse/app/components/create-invite-uploader.js b/app/assets/javascripts/discourse/app/components/create-invite-uploader.js index 79fe2a1c956..c98064d6bee 100644 --- a/app/assets/javascripts/discourse/app/components/create-invite-uploader.js +++ b/app/assets/javascripts/discourse/app/components/create-invite-uploader.js @@ -75,7 +75,7 @@ export default Component.extend({ $upload.on("fileuploadfail", (e, data) => { if (data.errorThrown !== "abort") { - displayErrorForUpload(data, this.siteSettings); + displayErrorForUpload(data, this.siteSettings, data.files[0].name); } this.reset(); }); diff --git a/app/assets/javascripts/discourse/app/components/uppy-image-uploader.js b/app/assets/javascripts/discourse/app/components/uppy-image-uploader.js index 7e1ecbf8ca0..7b416ca07c8 100644 --- a/app/assets/javascripts/discourse/app/components/uppy-image-uploader.js +++ b/app/assets/javascripts/discourse/app/components/uppy-image-uploader.js @@ -1,4 +1,5 @@ import Component from "@ember/component"; +import { or } from "@ember/object/computed"; import UppyUploadMixin from "discourse/mixins/uppy-upload"; import { ajax } from "discourse/lib/ajax"; import discourseComputed from "discourse-common/utils/decorators"; @@ -25,6 +26,8 @@ export default Component.extend(UppyUploadMixin, { } }, + uploadingOrProcessing: or("uploading", "processing"), + @discourseComputed("imageUrl", "placeholderUrl") showingPlaceholder(imageUrl, placeholderUrl) { return !imageUrl && placeholderUrl; diff --git a/app/assets/javascripts/discourse/app/lib/uploads.js b/app/assets/javascripts/discourse/app/lib/uploads.js index 480a300f48e..04500a06c58 100644 --- a/app/assets/javascripts/discourse/app/lib/uploads.js +++ b/app/assets/javascripts/discourse/app/lib/uploads.js @@ -1,4 +1,5 @@ import I18n from "I18n"; +import deprecated from "discourse-common/lib/deprecated"; import bootbox from "bootbox"; import { isAppleDevice } from "discourse/lib/utilities"; @@ -279,12 +280,29 @@ export function getUploadMarkdown(upload) { } } -export function displayErrorForUpload(data, siteSettings) { +export function displayErrorForUpload(data, siteSettings, fileName) { + if (!fileName) { + deprecated( + "Calling displayErrorForUpload without a fileName is deprecated and will be removed in a future version." + ); + fileName = data.files[0].name; + } + if (data.jqXHR) { const didError = displayErrorByResponseStatus( data.jqXHR.status, data.jqXHR.responseJSON, - data.files[0].name, + fileName, + siteSettings + ); + if (didError) { + return; + } + } else if (data.body && data.status) { + const didError = displayErrorByResponseStatus( + data.status, + data.body, + fileName, siteSettings ); if (didError) { @@ -294,25 +312,7 @@ export function displayErrorForUpload(data, siteSettings) { bootbox.alert(data.errors.join("\n")); return; } - // otherwise, display a generic error message - bootbox.alert(I18n.t("post.errors.upload")); -} -export function displayErrorForUppyUpload(response, fileName, siteSettings) { - if (response.body.errors && response.body.errors.length > 0) { - bootbox.alert(response.body.errors.join("\n")); - return; - } else { - const didError = displayErrorByResponseStatus( - response.status, - response.body, - fileName, - siteSettings - ); - if (didError) { - return; - } - } // otherwise, display a generic error message bootbox.alert(I18n.t("post.errors.upload")); } diff --git a/app/assets/javascripts/discourse/app/lib/uppy-checksum-plugin.js b/app/assets/javascripts/discourse/app/lib/uppy-checksum-plugin.js new file mode 100644 index 00000000000..c14e03f3de4 --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/uppy-checksum-plugin.js @@ -0,0 +1,94 @@ +import { Plugin } from "@uppy/core"; +import { warn } from "@ember/debug"; +import { Promise } from "rsvp"; + +export default class UppyChecksum extends Plugin { + constructor(uppy, opts) { + super(uppy, opts); + this.id = opts.id || "uppy-checksum"; + this.capabilities = opts.capabilities; + this.type = "preprocessor"; + } + + canUseSubtleCrypto() { + if (!window.isSecureContext) { + this.warnPrefixed( + "Cannot generate cryptographic digests in an insecure context (not HTTPS)." + ); + return false; + } + if (this.capabilities.isIE11) { + this.warnPrefixed( + "The required cipher suite is unavailable in Internet Explorer 11." + ); + return false; + } + if ( + !(window.crypto && window.crypto.subtle && window.crypto.subtle.digest) + ) { + this.warnPrefixed( + "The required cipher suite is unavailable in this browser." + ); + return false; + } + + return true; + } + + generateChecksum(fileIds) { + if (!this.canUseSubtleCrypto()) { + return Promise.resolve(); + } + + let promises = fileIds.map((fileId) => { + let file = this.uppy.getFile(fileId); + + this.uppy.emit("preprocess-progress", file, { + mode: "indeterminate", + message: "generating checksum", + }); + + return file.data.arrayBuffer().then((arrayBuffer) => { + return window.crypto.subtle + .digest("SHA-1", arrayBuffer) + .then((hash) => { + const hashArray = Array.from(new Uint8Array(hash)); + const hashHex = hashArray + .map((b) => b.toString(16).padStart(2, "0")) + .join(""); + this.uppy.setFileMeta(fileId, { sha1_checksum: hashHex }); + }) + .catch((err) => { + if ( + err.message.toString().includes("Algorithm: Unrecognized name") + ) { + this.warnPrefixed( + "SHA-1 algorithm is unsupported in this browser." + ); + } + }); + }); + }); + + const emitPreprocessCompleteForAll = () => { + fileIds.forEach((fileId) => { + const file = this.uppy.getFile(fileId); + this.uppy.emit("preprocess-complete", file); + }); + }; + + return Promise.all(promises).then(emitPreprocessCompleteForAll); + } + + warnPrefixed(message) { + warn(`[uppy-checksum-plugin] ${message}`); + } + + install() { + this.uppy.addPreProcessor(this.generateChecksum.bind(this)); + } + + uninstall() { + this.uppy.removePreProcessor(this.generateChecksum.bind(this)); + } +} diff --git a/app/assets/javascripts/discourse/app/mixins/upload.js b/app/assets/javascripts/discourse/app/mixins/upload.js index b626f6c994c..780b37536b8 100644 --- a/app/assets/javascripts/discourse/app/mixins/upload.js +++ b/app/assets/javascripts/discourse/app/mixins/upload.js @@ -108,7 +108,7 @@ export default Mixin.create({ }); $upload.on("fileuploadfail", (e, data) => { - displayErrorForUpload(data, this.siteSettings); + displayErrorForUpload(data, this.siteSettings, data.files[0].name); reset(); }); }), diff --git a/app/assets/javascripts/discourse/app/mixins/uppy-upload.js b/app/assets/javascripts/discourse/app/mixins/uppy-upload.js index 60a028dd16c..0408b8892a2 100644 --- a/app/assets/javascripts/discourse/app/mixins/uppy-upload.js +++ b/app/assets/javascripts/discourse/app/mixins/uppy-upload.js @@ -1,6 +1,7 @@ import Mixin from "@ember/object/mixin"; +import { ajax } from "discourse/lib/ajax"; import { - displayErrorForUppyUpload, + displayErrorForUpload, validateUploadedFile, } from "discourse/lib/uploads"; import { deepMerge } from "discourse-common/lib/object"; @@ -9,6 +10,8 @@ import I18n from "I18n"; import Uppy from "@uppy/core"; import DropTarget from "@uppy/drop-target"; import XHRUpload from "@uppy/xhr-upload"; +import AwsS3 from "@uppy/aws-s3"; +import UppyChecksum from "discourse/lib/uppy-checksum-plugin"; import { on } from "discourse-common/utils/decorators"; import { warn } from "@ember/debug"; @@ -42,13 +45,17 @@ export default Mixin.create({ @on("willDestroyElement") _destroy() { - this.messageBus && this.messageBus.unsubscribe("/uploads/" + this.type); + if (this.messageBus) { + this.messageBus.unsubscribe(`/uploads/${this.type}`); + } this.uppyInstance && this.uppyInstance.close(); }, @on("didInsertElement") _initialize() { - this.set("fileInputEl", this.element.querySelector(".hidden-upload-field")); + this.setProperties({ + fileInputEl: this.element.querySelector(".hidden-upload-field"), + }); this.set("allowMultipleFiles", this.fileInputEl.multiple); this._bindFileInputChangeListener(); @@ -78,6 +85,7 @@ export default Mixin.create({ bypassNewUserRestriction: true, user: this.currentUser, siteSettings: this.siteSettings, + validateSize: true, }, this.validateUploadedFilesOptions() ); @@ -114,24 +122,43 @@ export default Mixin.create({ ); this.uppyInstance.use(DropTarget, { target: this.element }); + this.uppyInstance.use(UppyChecksum, { capabilities: this.capabilities }); this.uppyInstance.on("progress", (progress) => { this.set("uploadProgress", progress); }); - this.uppyInstance.on("upload-success", (_file, response) => { - this.uploadDone(response.body); - this._reset(); + this.uppyInstance.on("upload-success", (file, response) => { + if (this.usingS3Uploads) { + this.setProperties({ uploading: false, processing: true }); + this._completeExternalUpload(file) + .then((completeResponse) => { + this.uploadDone(completeResponse); + this._reset(); + }) + .catch((errResponse) => { + displayErrorForUpload(errResponse, this.siteSettings, file.name); + this._reset(); + }); + } else { + this.uploadDone(response.body); + this._reset(); + } }); this.uppyInstance.on("upload-error", (file, error, response) => { - displayErrorForUppyUpload(response, file.name, this.siteSettings); + displayErrorForUpload(response, this.siteSettings, file.name); this._reset(); }); - // later we will use the uppy direct s3 uploader based on enable_s3_uploads, - // for now we always just use XHR uploads - this._useXHRUploads(); + if ( + this.siteSettings.enable_s3_uploads && + this.siteSettings.enable_direct_s3_uploads // hidden setting like enable_experimental_image_uploader + ) { + this._useS3Uploads(); + } else { + this._useXHRUploads(); + } }, _useXHRUploads() { @@ -143,6 +170,45 @@ export default Mixin.create({ }); }, + _useS3Uploads() { + this.set("usingS3Uploads", true); + this.uppyInstance.use(AwsS3, { + getUploadParameters: (file) => { + const data = { file_name: file.name, type: this.type }; + + // the sha1 checksum is set by the UppyChecksum plugin, except + // for in cases where the browser does not support the required + // crypto mechanisms or an error occurs. it is an additional layer + // of security, and not required. + if (file.meta.sha1_checksum) { + data.metadata = { "sha1-checksum": file.meta.sha1_checksum }; + } + + return ajax(getUrl("/uploads/generate-presigned-put"), { + type: "POST", + data, + }) + .then((response) => { + this.uppyInstance.setFileMeta(file.id, { + uniqueUploadIdentifier: response.unique_identifier, + }); + + return { + method: "put", + url: response.url, + headers: { + "Content-Type": file.type, + }, + }; + }) + .catch((errResponse) => { + displayErrorForUpload(errResponse, this.siteSettings, file.name); + this._reset(); + }); + }, + }); + }, + _xhrUploadUrl() { return ( getUrl(this.getWithDefault("uploadUrl", "/uploads")) + @@ -172,8 +238,21 @@ export default Mixin.create({ }); }, + _completeExternalUpload(file) { + return ajax(getUrl("/uploads/complete-external-upload"), { + type: "POST", + data: { + unique_identifier: file.meta.uniqueUploadIdentifier, + }, + }); + }, + _reset() { this.uppyInstance && this.uppyInstance.reset(); - this.setProperties({ uploading: false, uploadProgress: 0 }); + this.setProperties({ + uploading: false, + processing: false, + uploadProgress: 0, + }); }, }); diff --git a/app/assets/javascripts/discourse/app/templates/components/uppy-image-uploader.hbs b/app/assets/javascripts/discourse/app/templates/components/uppy-image-uploader.hbs index 04096a287ad..1038cfe06cc 100644 --- a/app/assets/javascripts/discourse/app/templates/components/uppy-image-uploader.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/uppy-image-uploader.hbs @@ -3,9 +3,9 @@
{{/if}}
-
{{#if imageUrl}} diff --git a/app/assets/javascripts/discourse/tests/unit/lib/uploads-test.js b/app/assets/javascripts/discourse/tests/unit/lib/uploads-test.js index 8e316cfb673..c11fcbbc2e0 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/uploads-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/uploads-test.js @@ -3,6 +3,7 @@ import { allowsAttachments, allowsImages, authorizedExtensions, + displayErrorForUpload, getUploadMarkdown, isImage, validateUploadedFiles, @@ -312,4 +313,57 @@ discourseModule("Unit | Utility | uploads", function () { "![image|100x200](/uploads/123/abcdef.ext)" ); }); + + test("displayErrorForUpload - jquery file upload - jqXHR present", function (assert) { + sinon.stub(bootbox, "alert"); + displayErrorForUpload( + { + jqXHR: { status: 422, responseJSON: { message: "upload failed" } }, + }, + { max_attachment_size_kb: 1024, max_image_size_kb: 1024 }, + "test.png" + ); + assert.ok(bootbox.alert.calledWith("upload failed"), "the alert is called"); + }); + + test("displayErrorForUpload - jquery file upload - jqXHR missing, errors present", function (assert) { + sinon.stub(bootbox, "alert"); + displayErrorForUpload( + { + errors: ["upload failed"], + }, + { max_attachment_size_kb: 1024, max_image_size_kb: 1024 }, + "test.png" + ); + assert.ok(bootbox.alert.calledWith("upload failed"), "the alert is called"); + }); + + test("displayErrorForUpload - jquery file upload - no errors", function (assert) { + sinon.stub(bootbox, "alert"); + displayErrorForUpload( + {}, + { + max_attachment_size_kb: 1024, + max_image_size_kb: 1024, + }, + "test.png" + ); + assert.ok( + bootbox.alert.calledWith(I18n.t("post.errors.upload")), + "the alert is called" + ); + }); + + test("displayErrorForUpload - uppy - with response status and body", function (assert) { + sinon.stub(bootbox, "alert"); + displayErrorForUpload( + { + status: 422, + body: { message: "upload failed" }, + }, + "test.png", + { max_attachment_size_kb: 1024, max_image_size_kb: 1024 } + ); + assert.ok(bootbox.alert.calledWith("upload failed"), "the alert is called"); + }); }); diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 3e35e967679..d2e1619ebf7 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -9,8 +9,14 @@ class UploadsController < ApplicationController protect_from_forgery except: :show before_action :is_asset_path, :apply_cdn_headers, only: [:show, :show_short, :show_secure] + before_action :external_store_check, only: [:show_secure, :generate_presigned_put, :complete_external_upload] SECURE_REDIRECT_GRACE_SECONDS = 5 + PRESIGNED_PUT_RATE_LIMIT_PER_MINUTE = 5 + + def external_store_check + return render_404 if !Discourse.store.external? + end def create # capture current user for block later on @@ -125,7 +131,6 @@ class UploadsController < ApplicationController def show_secure # do not serve uploads requested via XHR to prevent XSS return xhr_not_allowed if request.xhr? - return render_404 if !Discourse.store.external? path_with_ext = "#{params[:path]}.#{params[:extension]}" @@ -187,6 +192,84 @@ class UploadsController < ApplicationController } end + def generate_presigned_put + return render_404 if !SiteSetting.enable_direct_s3_uploads + + RateLimiter.new( + current_user, "generate-presigned-put-upload-stub", PRESIGNED_PUT_RATE_LIMIT_PER_MINUTE, 1.minute + ).performed! + + file_name = params.require(:file_name) + type = params.require(:type) + + # don't want people posting arbitrary S3 metadata so we just take the + # one we need. all of these will be converted to x-amz-meta- metadata + # fields in S3 so it's best to use dashes in the names for consistency + # + # this metadata is baked into the presigned url and is not altered when + # sending the PUT from the clientside to the presigned url + metadata = if params[:metadata].present? + meta = {} + if params[:metadata]["sha1-checksum"].present? + meta["sha1-checksum"] = params[:metadata]["sha1-checksum"] + end + meta + end + + url = Discourse.store.signed_url_for_temporary_upload( + file_name, metadata: metadata + ) + key = Discourse.store.path_from_url(url) + + upload_stub = ExternalUploadStub.create!( + key: key, + created_by: current_user, + original_filename: file_name, + upload_type: type + ) + + render json: { url: url, key: key, unique_identifier: upload_stub.unique_identifier } + end + + def complete_external_upload + return render_404 if !SiteSetting.enable_direct_s3_uploads + + unique_identifier = params.require(:unique_identifier) + external_upload_stub = ExternalUploadStub.find_by( + unique_identifier: unique_identifier, created_by: current_user + ) + return render_404 if external_upload_stub.blank? + + raise Discourse::InvalidAccess if external_upload_stub.created_by_id != current_user.id + external_upload_manager = ExternalUploadManager.new(external_upload_stub) + + hijack do + begin + upload = external_upload_manager.promote_to_upload! + if upload.errors.empty? + external_upload_manager.destroy! + render json: UploadsController.serialize_upload(upload), status: 200 + else + render_json_error(upload.errors.to_hash.values.flatten, status: 422) + end + rescue ExternalUploadManager::ChecksumMismatchError => err + debug_upload_error(err, "upload.checksum_mismatch_failure") + render_json_error(I18n.t("upload.failed"), status: 422) + rescue ExternalUploadManager::CannotPromoteError => err + debug_upload_error(err, "upload.cannot_promote_failure") + render_json_error(I18n.t("upload.failed"), status: 422) + rescue ExternalUploadManager::DownloadFailedError, Aws::S3::Errors::NotFound => err + debug_upload_error(err, "upload.download_failure") + render_json_error(I18n.t("upload.failed"), status: 422) + rescue => err + Discourse.warn_exception( + err, message: "Complete external upload failed unexpectedly for user #{current_user.id}" + ) + render_json_error(I18n.t("upload.failed"), status: 422) + end + end + end + protected def force_download? @@ -274,4 +357,8 @@ class UploadsController < ApplicationController send_file(file_path, opts) end + def debug_upload_error(translation_key, err) + return if !SiteSetting.enable_upload_debug_mode + Discourse.warn_exception(err, message: I18n.t(translation_key)) + end end diff --git a/app/jobs/scheduled/clean_up_uploads.rb b/app/jobs/scheduled/clean_up_uploads.rb index 55223318d5e..77f65031295 100644 --- a/app/jobs/scheduled/clean_up_uploads.rb +++ b/app/jobs/scheduled/clean_up_uploads.rb @@ -45,6 +45,8 @@ module Jobs end end + ExternalUploadStub.cleanup! + self.last_cleanup = Time.zone.now.to_i end diff --git a/app/models/external_upload_stub.rb b/app/models/external_upload_stub.rb new file mode 100644 index 00000000000..82a20ff2064 --- /dev/null +++ b/app/models/external_upload_stub.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require "digest/sha1" + +class ExternalUploadStub < ActiveRecord::Base + CREATED_EXPIRY_HOURS = 1 + UPLOADED_EXPIRY_HOURS = 24 + + belongs_to :created_by, class_name: 'User' + + scope :expired_created, -> { + where( + "status = ? AND created_at <= ?", + ExternalUploadStub.statuses[:created], + CREATED_EXPIRY_HOURS.hours.ago + ) + } + + scope :expired_uploaded, -> { + where( + "status = ? AND created_at <= ?", + ExternalUploadStub.statuses[:uploaded], + UPLOADED_EXPIRY_HOURS.hours.ago + ) + } + + before_create do + self.unique_identifier = SecureRandom.uuid + self.status = ExternalUploadStub.statuses[:created] if self.status.blank? + end + + def self.statuses + @statuses ||= Enum.new( + created: 1, + uploaded: 2, + failed: 3 + ) + end + + # TODO (martin): Lifecycle rule would be best to clean stuff up in the external + # systems, I don't think we really want to be calling out to the external systems + # here right? + def self.cleanup! + expired_created.delete_all + expired_uploaded.delete_all + end +end + +# == Schema Information +# +# Table name: external_upload_stubs +# +# id :bigint not null, primary key +# key :string not null +# original_filename :string not null +# status :integer default(1), not null +# unique_identifier :uuid not null +# created_by_id :integer not null +# upload_type :string not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_external_upload_stubs_on_created_by_id (created_by_id) +# index_external_upload_stubs_on_key (key) UNIQUE +# index_external_upload_stubs_on_status (status) +# diff --git a/app/services/external_upload_manager.rb b/app/services/external_upload_manager.rb new file mode 100644 index 00000000000..aacfb99ecad --- /dev/null +++ b/app/services/external_upload_manager.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +class ExternalUploadManager + DOWNLOAD_LIMIT = 100.megabytes + + class ChecksumMismatchError < StandardError; end + class DownloadFailedError < StandardError; end + class CannotPromoteError < StandardError; end + + attr_reader :external_upload_stub + + def initialize(external_upload_stub) + @external_upload_stub = external_upload_stub + end + + def can_promote? + external_upload_stub.status == ExternalUploadStub.statuses[:created] + end + + def promote_to_upload! + raise CannotPromoteError if !can_promote? + + external_upload_stub.update!(status: ExternalUploadStub.statuses[:uploaded]) + external_stub_object = Discourse.store.object_from_path(external_upload_stub.key) + external_etag = external_stub_object.etag + external_size = external_stub_object.size + external_sha1 = external_stub_object.metadata["sha1-checksum"] + + # This could be legitimately nil, if it's too big to download on the + # server, or it could have failed. To this end we set a should_download + # variable as well to check. + tempfile = nil + should_download = external_size < DOWNLOAD_LIMIT + if should_download + tempfile = download(external_upload_stub.key, external_upload_stub.upload_type) + + raise DownloadFailedError if tempfile.blank? + + actual_sha1 = Upload.generate_digest(tempfile) + if external_sha1 && external_sha1 != actual_sha1 + raise ChecksumMismatchError + end + end + + # TODO (martin): See if these additional opts will be needed + # + # for_private_message: for_private_message, + # for_site_setting: for_site_setting, + # pasted: pasted, + # + # also check if retain_hours is needed + opts = { + type: external_upload_stub.upload_type, + existing_external_upload_key: external_upload_stub.key, + external_upload_too_big: external_size > DOWNLOAD_LIMIT, + filesize: external_size + } + + UploadCreator.new(tempfile, external_upload_stub.original_filename, opts).create_for( + external_upload_stub.created_by_id + ) + rescue + external_upload_stub.update!(status: ExternalUploadStub.statuses[:failed]) + raise + ensure + tempfile&.close! + end + + def destroy! + external_upload_stub.destroy! + end + + private + + def download(key, type) + url = Discourse.store.signed_url_for_path(external_upload_stub.key) + FileHelper.download( + url, + max_file_size: DOWNLOAD_LIMIT, + tmp_file_name: "discourse-upload-#{type}", + follow_redirect: true + ) + end +end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index ce7d3b1279f..eb6cb5a4877 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -4019,8 +4019,12 @@ en: store_failure: "Failed to store upload #%{upload_id} for user #%{user_id}." file_missing: "Sorry, you must provide a file to upload." empty: "Sorry, but the file you provided is empty." + failed: "Sorry, but your upload failed. Please try again." png_to_jpg_conversion_failure_message: "An error happened when converting from PNG to JPG." optimize_failure_message: "An error occurred while optimizing the uploaded image." + download_failure: "Downloading the file from the external provider failed." + checksum_mismatch_failure: "The checksum of the file you uploaded does not match. The file contents may have changed on upload. Please try again." + cannot_promote_failure: "The upload cannot be completed, it may have already completed or previously failed." attachments: too_large: "Sorry, the file you are trying to upload is too big (maximum size is %{max_size_kb}KB)." images: diff --git a/config/routes.rb b/config/routes.rb index 6c111ec05f0..211a4ff65de 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -537,6 +537,9 @@ Discourse::Application.routes.draw do post "uploads" => "uploads#create" post "uploads/lookup-urls" => "uploads#lookup_urls" + post "uploads/generate-presigned-put" => "uploads#generate_presigned_put" + post "uploads/complete-external-upload" => "uploads#complete_external_upload" + # used to download original images get "uploads/:site/:sha(.:extension)" => "uploads#show", constraints: { site: /\w+/, sha: /\h{40}/, extension: /[a-z0-9\._]+/i } get "uploads/short-url/:base62(.:extension)" => "uploads#show_short", constraints: { site: /\w+/, base62: /[a-zA-Z0-9]+/, extension: /[a-zA-Z0-9\._-]+/i }, as: :upload_short diff --git a/config/site_settings.yml b/config/site_settings.yml index 363aacc249a..7ef0cb3df52 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -271,6 +271,13 @@ basic: client: true default: false hidden: true + enable_direct_s3_uploads: + client: true + default: false + hidden: true + enable_upload_debug_mode: + default: false + hidden: true default_theme_id: default: -1 hidden: true diff --git a/db/migrate/20210709042135_create_external_upload_stubs.rb b/db/migrate/20210709042135_create_external_upload_stubs.rb new file mode 100644 index 00000000000..a9e243945b9 --- /dev/null +++ b/db/migrate/20210709042135_create_external_upload_stubs.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CreateExternalUploadStubs < ActiveRecord::Migration[6.1] + def change + create_table :external_upload_stubs do |t| + t.string :key, null: false + t.string :original_filename, null: false + t.integer :status, default: 1, null: false, index: true + t.uuid :unique_identifier, null: false + t.integer :created_by_id, null: false, index: true + t.string :upload_type, null: false + + t.timestamps + end + + add_index :external_upload_stubs, [:key], unique: true + end +end diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb index 80f1fb6680c..49759e7735b 100644 --- a/lib/file_store/base_store.rb +++ b/lib/file_store/base_store.rb @@ -5,6 +5,7 @@ module FileStore class BaseStore UPLOAD_PATH_REGEX = %r|/(original/\d+X/.*)| OPTIMIZED_IMAGE_PATH_REGEX = %r|/(optimized/\d+X/.*)| + TEMPORARY_UPLOAD_PREFIX ||= "temp/" def store_upload(file, upload, content_type = nil) upload.url = nil @@ -40,6 +41,15 @@ module FileStore File.join(path, "test_#{ENV['TEST_ENV_NUMBER'].presence || '0'}") end + def temporary_upload_path(file_name) + File.join( + upload_path, + TEMPORARY_UPLOAD_PREFIX, + SecureRandom.hex, + file_name + ) + end + def has_been_uploaded?(url) not_implemented end diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index c0671ca9f40..2c85c77eba2 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -35,6 +35,22 @@ module FileStore url end + def move_existing_stored_upload(existing_external_upload_key, upload, content_type = nil) + upload.url = nil + path = get_path_for_upload(upload) + url, upload.etag = store_file( + nil, + path, + filename: upload.original_filename, + content_type: content_type, + cache_locally: false, + private_acl: upload.secure?, + move_existing: true, + existing_external_upload_key: existing_external_upload_key + ) + url + end + def store_optimized_image(file, optimized_image, content_type = nil, secure: false) optimized_image.url = nil path = get_path_for_optimized_image(optimized_image) @@ -42,10 +58,18 @@ module FileStore url end + # File is an actual Tempfile on disk + # + # An existing_external_upload_key is given for cases where move_existing is specified. + # This is an object already uploaded directly to S3 that we are now moving + # to its final resting place with the correct sha and key. + # # options # - filename # - content_type # - cache_locally + # - move_existing + # - existing_external_upload_key def store_file(file, path, opts = {}) path = path.dup @@ -72,7 +96,16 @@ module FileStore path.prepend(File.join(upload_path, "/")) if Rails.configuration.multisite # if this fails, it will throw an exception - path, etag = s3_helper.upload(file, path, options) + if opts[:move_existing] && opts[:existing_external_upload_key] + path, etag = s3_helper.copy( + opts[:existing_external_upload_key], + path, + options: options + ) + s3_helper.delete_object(opts[:existing_external_upload_key]) + else + path, etag = s3_helper.upload(file, path, options) + end # return the upload url and etag [File.join(absolute_base_url, path), etag] @@ -162,10 +195,14 @@ module FileStore def url_for(upload, force_download: false) upload.secure? || force_download ? - presigned_url(get_upload_key(upload), force_download: force_download, filename: upload.original_filename) : + presigned_get_url(get_upload_key(upload), force_download: force_download, filename: upload.original_filename) : upload.url end + def path_from_url(url) + URI.parse(url).path.delete_prefix("/") + end + def cdn_url(url) return url if SiteSetting.Upload.s3_cdn_url.blank? schema = url[/^(https?:)?\/\//, 1] @@ -175,7 +212,21 @@ module FileStore def signed_url_for_path(path, expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS, force_download: false) key = path.sub(absolute_base_url + "/", "") - presigned_url(key, expires_in: expires_in, force_download: force_download) + presigned_get_url(key, expires_in: expires_in, force_download: force_download) + end + + def signed_url_for_temporary_upload(file_name, expires_in: S3Helper::UPLOAD_URL_EXPIRES_AFTER_SECONDS, metadata: {}) + key = temporary_upload_path(file_name) + presigned_put_url(key, expires_in: expires_in, metadata: metadata) + end + + def temporary_upload_path(file_name) + path = super(file_name) + s3_bucket_folder_path.nil? ? path : File.join(s3_bucket_folder_path, path) + end + + def object_from_path(path) + s3_helper.object(path) end def cache_avatar(avatar, user_id) @@ -248,7 +299,19 @@ module FileStore private - def presigned_url( + def presigned_put_url(key, expires_in: S3Helper::UPLOAD_URL_EXPIRES_AFTER_SECONDS, metadata: {}) + signer = Aws::S3::Presigner.new(client: s3_helper.s3_client) + signer.presigned_url( + :put_object, + bucket: s3_bucket_name, + key: key, + acl: "private", + expires_in: expires_in, + metadata: metadata + ) + end + + def presigned_get_url( url, force_download: false, filename: false, @@ -262,7 +325,7 @@ module FileStore ) end - obj = s3_helper.object(url) + obj = object_from_path(url) obj.presigned_url(:get, opts) end @@ -276,7 +339,7 @@ module FileStore def update_ACL(key, secure) begin - s3_helper.object(key).acl.put(acl: secure ? "private" : "public-read") + object_from_path(key).acl.put(acl: secure ? "private" : "public-read") rescue Aws::S3::Errors::NoSuchKey Rails.logger.warn("Could not update ACL on upload with key: '#{key}'. Upload is missing.") end diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index ed4d857951b..712929af6f6 100644 --- a/lib/s3_helper.rb +++ b/lib/s3_helper.rb @@ -15,7 +15,13 @@ class S3Helper # * cache time for secure-media URLs # * expiry time for S3 presigned URLs, which include backup downloads and # any upload that has a private ACL (e.g. secure uploads) - DOWNLOAD_URL_EXPIRES_AFTER_SECONDS ||= 300 + DOWNLOAD_URL_EXPIRES_AFTER_SECONDS ||= 5.minutes.to_i + + ## + # Controls the following: + # + # * presigned put_object URLs for direct S3 uploads + UPLOAD_URL_EXPIRES_AFTER_SECONDS ||= 10.minutes.to_i def initialize(s3_bucket_name, tombstone_prefix = '', options = {}) @s3_client = options.delete(:client) @@ -80,6 +86,7 @@ class S3Helper end def copy(source, destination, options: {}) + destination = get_path_for_s3_upload(destination) if !Rails.configuration.multisite options[:copy_source] = File.join(@s3_bucket_name, source) else @@ -87,16 +94,16 @@ class S3Helper options[:copy_source] = File.join(@s3_bucket_name, source) elsif @s3_bucket_folder_path folder, filename = begin - source.split("/", 2) - end + source.split("/", 2) + end options[:copy_source] = File.join(@s3_bucket_name, folder, multisite_upload_path, filename) else options[:copy_source] = File.join(@s3_bucket_name, multisite_upload_path, source) end end - s3_bucket - .object(destination) - .copy_from(options) + + response = s3_bucket.object(destination).copy_from(options) + [destination, response.copy_object_result.etag.gsub('"', '')] end # make sure we have a cors config for assets diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 30b4ff99e6d..d29a3c5d2b5 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -30,6 +30,7 @@ class UploadCreator @filename = (filename || "").gsub(/[^[:print:]]/, "") @upload = Upload.new(original_filename: @filename, filesize: 0) @opts = opts + @filesize = @opts[:filesize] if @opts[:external_upload_too_big] @opts[:validate] = opts[:skip_validations].present? ? !ActiveRecord::Type::Boolean.new.cast(opts[:skip_validations]) : true end @@ -44,14 +45,22 @@ class UploadCreator is_image ||= @image_info && FileHelper.is_supported_image?("test.#{@image_info.type}") is_image = false if @opts[:for_theme] + # if this is present then it means we are creating an upload record from + # an external_upload_stub and the file is > ExternalUploadManager::DOWNLOAD_LIMIT, + # so we have not downloaded it to a tempfile. no modifications can be made to the + # file in this case because it does not exist; we simply move it to its new location + # in S3 + external_upload_too_big = @opts[:external_upload_too_big] + sha1_before_changes = Upload.generate_digest(@file) if @file + DistributedMutex.synchronize("upload_#{user_id}_#{@filename}") do # We need to convert HEIFs early because FastImage does not consider them as images - if convert_heif_to_jpeg? + if convert_heif_to_jpeg? && !external_upload_too_big convert_heif! is_image = FileHelper.is_supported_image?("test.#{@image_info.type}") end - if is_image + if is_image && !external_upload_too_big extract_image_info! return @upload if @upload.errors.present? @@ -72,14 +81,18 @@ class UploadCreator # compute the sha of the file and generate a unique hash # which is only used for secure uploads - sha1 = Upload.generate_digest(@file) - unique_hash = SecureRandom.hex(20) if SiteSetting.secure_media + if !external_upload_too_big + sha1 = Upload.generate_digest(@file) + end + if SiteSetting.secure_media || external_upload_too_big + unique_hash = generate_fake_sha1_hash + end # we do not check for duplicate uploads if secure media is # enabled because we use a unique access hash to differentiate # between uploads instead of the sha1, and to get around various # access/permission issues for uploads - if !SiteSetting.secure_media + if !SiteSetting.secure_media && !external_upload_too_big # do we already have that upload? @upload = Upload.find_by(sha1: sha1) @@ -99,7 +112,7 @@ class UploadCreator fixed_original_filename = nil - if is_image + if is_image && !external_upload_too_big current_extension = File.extname(@filename).downcase.sub("jpeg", "jpg") expected_extension = ".#{image_type}".downcase.sub("jpeg", "jpg") @@ -117,13 +130,13 @@ class UploadCreator @upload.user_id = user_id @upload.original_filename = fixed_original_filename || @filename @upload.filesize = filesize - @upload.sha1 = SiteSetting.secure_media? ? unique_hash : sha1 + @upload.sha1 = (SiteSetting.secure_media? || external_upload_too_big) ? unique_hash : sha1 @upload.original_sha1 = SiteSetting.secure_media? ? sha1 : nil @upload.url = "" @upload.origin = @opts[:origin][0...1000] if @opts[:origin] @upload.extension = image_type || File.extname(@filename)[1..10] - if is_image + if is_image && !external_upload_too_big if @image_info.type.to_s == 'svg' w, h = [0, 0] @@ -157,19 +170,41 @@ class UploadCreator # Callbacks using this event to validate the upload or the file must add errors to the # upload errors object. - DiscourseEvent.trigger(:before_upload_creation, @file, is_image, @upload, @opts[:validate]) + # + # Can't do any validation of the file if external_upload_too_big because we don't have + # the actual file. + if !external_upload_too_big + DiscourseEvent.trigger(:before_upload_creation, @file, is_image, @upload, @opts[:validate]) + end return @upload unless @upload.errors.empty? && @upload.save(validate: @opts[:validate]) - # store the file and update its url - File.open(@file.path) do |f| - url = Discourse.store.store_upload(f, @upload) - - if url.present? - @upload.url = url - @upload.save!(validate: @opts[:validate]) + should_move = false + upload_changed = \ + if external_upload_too_big + false else - @upload.errors.add(:url, I18n.t("upload.store_failure", upload_id: @upload.id, user_id: user_id)) + Upload.generate_digest(@file) != sha1_before_changes end + + if @opts[:existing_external_upload_key] && Discourse.store.external? + should_move = external_upload_too_big || !upload_changed + end + + if should_move + # move the file in the store instead of reuploading + url = Discourse.store.move_existing_stored_upload(@opts[:existing_external_upload_key], @upload) + else + # store the file and update its url + File.open(@file.path) do |f| + url = Discourse.store.store_upload(f, @upload) + end + end + + if url.present? + @upload.url = url + @upload.save!(validate: @opts[:validate]) + else + @upload.errors.add(:url, I18n.t("upload.store_failure", upload_id: @upload.id, user_id: user_id)) end if @upload.errors.empty? && is_image && @opts[:type] == "avatar" && @upload.extension != "svg" @@ -430,7 +465,7 @@ class UploadCreator end def filesize - File.size?(@file.path).to_i + @filesize || File.size?(@file.path).to_i end def max_image_size @@ -484,4 +519,7 @@ class UploadCreator end end + def generate_fake_sha1_hash + SecureRandom.hex(20) + end end diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb index 5ac16351ef7..695ee24ba08 100644 --- a/spec/components/file_store/s3_store_spec.rb +++ b/spec/components/file_store/s3_store_spec.rb @@ -154,9 +154,7 @@ describe FileStore::S3Store do s3_bucket.expects(:object).with(destination).returns(s3_object) - s3_object.expects(:copy_from).with( - copy_source: "s3-upload-bucket/#{source}" - ) + expect_copy_from(s3_object, "s3-upload-bucket/#{source}") store.copy_file(upload.url, source, destination) end @@ -171,7 +169,7 @@ describe FileStore::S3Store do s3_object = stub s3_bucket.expects(:object).with("tombstone/original/1X/#{upload.sha1}.png").returns(s3_object) - s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/original/1X/#{upload.sha1}.png") + expect_copy_from(s3_object, "s3-upload-bucket/original/1X/#{upload.sha1}.png") s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object) s3_object.expects(:delete) @@ -188,7 +186,7 @@ describe FileStore::S3Store do s3_object = stub s3_bucket.expects(:object).with("tombstone/#{path}").returns(s3_object) - s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/#{path}") + expect_copy_from(s3_object, "s3-upload-bucket/#{path}") s3_bucket.expects(:object).with(path).returns(s3_object) s3_object.expects(:delete) @@ -206,7 +204,7 @@ describe FileStore::S3Store do s3_object = stub s3_bucket.expects(:object).with("discourse-uploads/tombstone/original/1X/#{upload.sha1}.png").returns(s3_object) - s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/discourse-uploads/original/1X/#{upload.sha1}.png") + expect_copy_from(s3_object, "s3-upload-bucket/discourse-uploads/original/1X/#{upload.sha1}.png") s3_bucket.expects(:object).with("discourse-uploads/original/1X/#{upload.sha1}.png").returns(s3_object) s3_object.expects(:delete) @@ -231,7 +229,7 @@ describe FileStore::S3Store do s3_object = stub s3_bucket.expects(:object).with("tombstone/#{image_path}").returns(s3_object) - s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/#{image_path}") + expect_copy_from(s3_object, "s3-upload-bucket/#{image_path}") s3_bucket.expects(:object).with("#{image_path}").returns(s3_object) s3_object.expects(:delete) @@ -257,9 +255,7 @@ describe FileStore::S3Store do .with("discourse-uploads/tombstone/#{image_path}") .returns(s3_object) - s3_object.expects(:copy_from).with( - copy_source: "s3-upload-bucket/discourse-uploads/#{image_path}" - ) + expect_copy_from(s3_object, "s3-upload-bucket/discourse-uploads/#{image_path}") s3_bucket.expects(:object).with( "discourse-uploads/#{image_path}" @@ -425,4 +421,12 @@ describe FileStore::S3Store do expect(store.signed_url_for_path("special/optimized/file.png")).not_to eq(upload.url) end end + + def expect_copy_from(s3_object, source) + s3_object.expects(:copy_from).with( + copy_source: source + ).returns( + stub(copy_object_result: stub(etag: '"etagtest"')) + ) + end end diff --git a/spec/fabricators/external_upload_stub_fabricator.rb b/spec/fabricators/external_upload_stub_fabricator.rb new file mode 100644 index 00000000000..57d26d10a3b --- /dev/null +++ b/spec/fabricators/external_upload_stub_fabricator.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +Fabricator(:external_upload_stub) do + created_by { Fabricate(:user) } + original_filename "test.txt" + key { Discourse.store.temporary_upload_path("test.txt") } + upload_type "card_background" + status 1 +end + +Fabricator(:image_external_upload_stub, from: :external_upload_stub) do + original_filename "logo.png" + key { Discourse.store.temporary_upload_path("logo.png") } +end + +Fabricator(:attachment_external_upload_stub, from: :external_upload_stub) do + original_filename "file.pdf" + key { Discourse.store.temporary_upload_path("file.pdf") } +end diff --git a/spec/jobs/clean_up_uploads_spec.rb b/spec/jobs/clean_up_uploads_spec.rb index 26275cac180..219b2f4c19e 100644 --- a/spec/jobs/clean_up_uploads_spec.rb +++ b/spec/jobs/clean_up_uploads_spec.rb @@ -305,4 +305,13 @@ describe Jobs::CleanUpUploads do expect(Upload.exists?(id: expired_upload.id)).to eq(false) expect(Upload.exists?(id: badge_image.id)).to eq(true) end + + it "deletes external upload stubs that have expired" do + external_stub1 = Fabricate(:external_upload_stub, status: ExternalUploadStub.statuses[:created], created_at: 10.minutes.ago) + external_stub2 = Fabricate(:external_upload_stub, status: ExternalUploadStub.statuses[:created], created_at: (ExternalUploadStub::CREATED_EXPIRY_HOURS.hours + 10.minutes).ago) + external_stub3 = Fabricate(:external_upload_stub, status: ExternalUploadStub.statuses[:uploaded], created_at: 10.minutes.ago) + external_stub4 = Fabricate(:external_upload_stub, status: ExternalUploadStub.statuses[:uploaded], created_at: (ExternalUploadStub::UPLOADED_EXPIRY_HOURS.hours + 10.minutes).ago) + Jobs::CleanUpUploads.new.execute(nil) + expect(ExternalUploadStub.pluck(:id)).to contain_exactly(external_stub1.id, external_stub3.id) + end end diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb index 27248b22657..9d2d38eacaf 100644 --- a/spec/multisite/s3_store_spec.rb +++ b/spec/multisite/s3_store_spec.rb @@ -113,7 +113,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do s3_object = stub s3_bucket.expects(:object).with("uploads/tombstone/default/original/1X/#{upload.sha1}.png").returns(s3_object) - s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/#{upload_path}/original/1X/#{upload.sha1}.png") + expect_copy_from(s3_object, "s3-upload-bucket/#{upload_path}/original/1X/#{upload.sha1}.png") s3_bucket.expects(:object).with("#{upload_path}/original/1X/#{upload.sha1}.png").returns(s3_object) s3_object.expects(:delete) @@ -129,7 +129,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do s3_object = stub s3_bucket.expects(:object).with("uploads/tombstone/second/original/1X/#{upload.sha1}.png").returns(s3_object) - s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/#{upload_path}/original/1X/#{upload.sha1}.png") + expect_copy_from(s3_object, "s3-upload-bucket/#{upload_path}/original/1X/#{upload.sha1}.png") s3_bucket.expects(:object).with("#{upload_path}/original/1X/#{upload.sha1}.png").returns(s3_object) s3_object.expects(:delete) @@ -150,7 +150,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do s3_object = stub s3_bucket.expects(:object).with("discourse-uploads/uploads/tombstone/default/original/1X/#{upload.sha1}.png").returns(s3_object) - s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/discourse-uploads/#{upload_path}/original/1X/#{upload.sha1}.png") + expect_copy_from(s3_object, "s3-upload-bucket/discourse-uploads/#{upload_path}/original/1X/#{upload.sha1}.png") s3_bucket.expects(:object).with("discourse-uploads/#{upload_path}/original/1X/#{upload.sha1}.png").returns(s3_object) s3_object.expects(:delete) @@ -298,4 +298,60 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do expect(store.has_been_uploaded?(link)).to eq(false) end end + + describe "#signed_url_for_temporary_upload" do + before do + setup_s3 + end + + let(:store) { FileStore::S3Store.new } + + context "for a bucket with no folder path" do + before { SiteSetting.s3_upload_bucket = "s3-upload-bucket" } + + it "returns a presigned url with the correct params and the key for the temporary file" do + url = store.signed_url_for_temporary_upload("test.png") + key = store.path_from_url(url) + expect(url).to match(/Amz-Expires/) + expect(key).to match(/uploads\/default\/test_[0-9]\/temp\/[a-zA-z0-9]{0,32}\/test.png/) + end + + it "presigned url contans the metadata when provided" do + url = store.signed_url_for_temporary_upload("test.png", metadata: { "test-meta": "testing" }) + expect(url).to include("&x-amz-meta-test-meta=testing") + end + end + + context "for a bucket with a folder path" do + before { SiteSetting.s3_upload_bucket = "s3-upload-bucket/site" } + + it "returns a presigned url with the correct params and the key for the temporary file" do + url = store.signed_url_for_temporary_upload("test.png") + key = store.path_from_url(url) + expect(url).to match(/Amz-Expires/) + expect(key).to match(/site\/uploads\/default\/test_[0-9]\/temp\/[a-zA-z0-9]{0,32}\/test.png/) + end + end + + context "for a multisite site" do + before { SiteSetting.s3_upload_bucket = "s3-upload-bucket/standard99" } + + it "returns a presigned url with the correct params and the key for the temporary file" do + test_multisite_connection('second') do + url = store.signed_url_for_temporary_upload("test.png") + key = store.path_from_url(url) + expect(url).to match(/Amz-Expires/) + expect(key).to match(/standard99\/uploads\/second\/test_[0-9]\/temp\/[a-zA-z0-9]{0,32}\/test.png/) + end + end + end + end + + def expect_copy_from(s3_object, source) + s3_object.expects(:copy_from).with( + copy_source: source + ).returns( + stub(copy_object_result: stub(etag: '"etagtest"')) + ) + end end diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb index 86db3f77456..ae78d21329b 100644 --- a/spec/requests/uploads_controller_spec.rb +++ b/spec/requests/uploads_controller_spec.rb @@ -704,4 +704,159 @@ describe UploadsController do end end end + + describe "#generate_presigned_put" do + before do + sign_in(user) + end + + context "when the store is external" do + before do + SiteSetting.enable_direct_s3_uploads = true + setup_s3 + end + + it "errors if the correct params are not provided" do + post "/uploads/generate-presigned-put.json", params: { file_name: "test.png" } + expect(response.status).to eq(400) + post "/uploads/generate-presigned-put.json", params: { type: "card_background" } + expect(response.status).to eq(400) + end + + it "generates a presigned URL and creates an external upload stub" do + post "/uploads/generate-presigned-put.json", params: { file_name: "test.png", type: "card_background" } + expect(response.status).to eq(200) + + result = response.parsed_body + + external_upload_stub = ExternalUploadStub.where( + unique_identifier: result["unique_identifier"], + original_filename: "test.png", + created_by: user, + upload_type: "card_background" + ) + expect(external_upload_stub.exists?).to eq(true) + expect(result["key"]).to include(FileStore::S3Store::TEMPORARY_UPLOAD_PREFIX) + expect(result["url"]).to include(FileStore::S3Store::TEMPORARY_UPLOAD_PREFIX) + expect(result["url"]).to include("Amz-Expires") + end + + it "includes accepted metadata in the presigned url when provided" do + post "/uploads/generate-presigned-put.json", { + params: { + file_name: "test.png", + type: "card_background", + metadata: { + "sha1-checksum" => "testing", + "blah" => "wontbeincluded" + } + } + } + expect(response.status).to eq(200) + + result = response.parsed_body + expect(result['url']).to include("&x-amz-meta-sha1-checksum=testing") + expect(result['url']).not_to include("&x-amz-meta-blah=wontbeincluded") + end + + it "rate limits" do + RateLimiter.enable + stub_const(UploadsController, "PRESIGNED_PUT_RATE_LIMIT_PER_MINUTE", 1) do + post "/uploads/generate-presigned-put.json", params: { file_name: "test.png", type: "card_background" } + post "/uploads/generate-presigned-put.json", params: { file_name: "test.png", type: "card_background" } + end + expect(response.status).to eq(429) + end + end + + context "when the store is not external" do + it "returns 404" do + post "/uploads/generate-presigned-put.json", params: { file_name: "test.png", type: "card_background" } + expect(response.status).to eq(404) + end + end + end + + describe "#complete_external_upload" do + before do + sign_in(user) + end + + context "when the store is external" do + fab!(:external_upload_stub) { Fabricate(:external_upload_stub, created_by: user) } + let(:upload) { Fabricate(:upload) } + + before do + SiteSetting.enable_direct_s3_uploads = true + SiteSetting.enable_upload_debug_mode = true + setup_s3 + end + + it "returns 404 when the upload stub does not exist" do + post "/uploads/complete-external-upload.json", params: { unique_identifier: "unknown" } + expect(response.status).to eq(404) + end + + it "returns 404 when the upload stub does not belong to the user" do + external_upload_stub.update!(created_by: Fabricate(:user)) + post "/uploads/complete-external-upload.json", params: { unique_identifier: external_upload_stub.unique_identifier } + expect(response.status).to eq(404) + end + + it "handles ChecksumMismatchError" do + ExternalUploadManager.any_instance.stubs(:promote_to_upload!).raises(ExternalUploadManager::ChecksumMismatchError) + post "/uploads/complete-external-upload.json", params: { unique_identifier: external_upload_stub.unique_identifier } + expect(response.status).to eq(422) + expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed")) + end + + it "handles CannotPromoteError" do + ExternalUploadManager.any_instance.stubs(:promote_to_upload!).raises(ExternalUploadManager::CannotPromoteError) + post "/uploads/complete-external-upload.json", params: { unique_identifier: external_upload_stub.unique_identifier } + expect(response.status).to eq(422) + expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed")) + end + + it "handles DownloadFailedError and Aws::S3::Errors::NotFound" do + ExternalUploadManager.any_instance.stubs(:promote_to_upload!).raises(ExternalUploadManager::DownloadFailedError) + post "/uploads/complete-external-upload.json", params: { unique_identifier: external_upload_stub.unique_identifier } + expect(response.status).to eq(422) + expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed")) + ExternalUploadManager.any_instance.stubs(:promote_to_upload!).raises(Aws::S3::Errors::NotFound.new("error", "not found")) + post "/uploads/complete-external-upload.json", params: { unique_identifier: external_upload_stub.unique_identifier } + expect(response.status).to eq(422) + expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed")) + end + + it "handles a generic upload failure" do + ExternalUploadManager.any_instance.stubs(:promote_to_upload!).raises(StandardError) + post "/uploads/complete-external-upload.json", params: { unique_identifier: external_upload_stub.unique_identifier } + expect(response.status).to eq(422) + expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed")) + end + + it "handles validation errors on the upload" do + upload.errors.add(:base, "test error") + ExternalUploadManager.any_instance.stubs(:promote_to_upload!).returns(upload) + post "/uploads/complete-external-upload.json", params: { unique_identifier: external_upload_stub.unique_identifier } + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to eq(["test error"]) + end + + it "deletes the stub and returns the serialized upload when complete" do + ExternalUploadManager.any_instance.stubs(:promote_to_upload!).returns(upload) + post "/uploads/complete-external-upload.json", params: { unique_identifier: external_upload_stub.unique_identifier } + expect(ExternalUploadStub.exists?(id: external_upload_stub.id)).to eq(false) + expect(response.status).to eq(200) + expect(response.parsed_body).to eq(UploadsController.serialize_upload(upload)) + end + end + + context "when the store is not external" do + it "returns 404" do + post "/uploads/generate-presigned-put.json", params: { file_name: "test.png", type: "card_background" } + expect(response.status).to eq(404) + end + end + end end diff --git a/spec/services/external_upload_manager_spec.rb b/spec/services/external_upload_manager_spec.rb new file mode 100644 index 00000000000..59a7ad92818 --- /dev/null +++ b/spec/services/external_upload_manager_spec.rb @@ -0,0 +1,222 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ExternalUploadManager do + fab!(:user) { Fabricate(:user) } + let(:type) { "card_background" } + let!(:logo_file) { file_from_fixtures("logo.png") } + let!(:pdf_file) { file_from_fixtures("large.pdf", "pdf") } + let(:object_size) { 1.megabyte } + let(:etag) { "e696d20564859cbdf77b0f51cbae999a" } + let(:client_sha1) { Upload.generate_digest(object_file) } + let(:sha1) { Upload.generate_digest(object_file) } + let(:object_file) { logo_file } + let(:metadata_headers) { {} } + let!(:external_upload_stub) { Fabricate(:image_external_upload_stub, created_by: user) } + let(:upload_base_url) { "https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com" } + + subject do + ExternalUploadManager.new(external_upload_stub) + end + + before do + SiteSetting.authorized_extensions += "|pdf" + SiteSetting.max_attachment_size_kb = 210.megabytes / 1000 + + setup_s3 + stub_head_object + stub_download_object_filehelper + stub_copy_object + stub_delete_object + end + + describe "#can_promote?" do + it "returns false if the external stub status is not created" do + external_upload_stub.update!(status: ExternalUploadStub.statuses[:uploaded]) + expect(subject.can_promote?).to eq(false) + end + end + + describe "#promote_to_upload!" do + context "when stubbed upload is < DOWNLOAD_LIMIT (small enough to download + generate sha)" do + let!(:external_upload_stub) { Fabricate(:image_external_upload_stub, created_by: user) } + let(:object_size) { 1.megabyte } + let(:object_file) { logo_file } + + context "when the download of the s3 file fails" do + before do + FileHelper.stubs(:download).returns(nil) + end + + it "raises an error" do + expect { subject.promote_to_upload! }.to raise_error(ExternalUploadManager::DownloadFailedError) + end + end + + context "when the upload is not in the created status" do + before do + external_upload_stub.update!(status: ExternalUploadStub.statuses[:uploaded]) + end + it "raises an error" do + expect { subject.promote_to_upload! }.to raise_error(ExternalUploadManager::CannotPromoteError) + end + end + + context "when the upload does not get changed in UploadCreator (resized etc.)" do + it "copies the stubbed upload on S3 to its new destination and deletes it" do + upload = subject.promote_to_upload! + expect(WebMock).to have_requested( + :put, + "#{upload_base_url}/original/1X/#{upload.sha1}.png", + ).with(headers: { 'X-Amz-Copy-Source' => "#{SiteSetting.s3_upload_bucket}/#{external_upload_stub.key}" }) + expect(WebMock).to have_requested( + :delete, + "#{upload_base_url}/#{external_upload_stub.key}" + ) + end + + it "errors if the image upload is too big" do + SiteSetting.max_image_size_kb = 1 + upload = subject.promote_to_upload! + expect(upload.errors.full_messages).to include( + "Filesize " + I18n.t("upload.images.too_large", max_size_kb: SiteSetting.max_image_size_kb) + ) + end + + it "errors if the extension is not supported" do + SiteSetting.authorized_extensions = "" + upload = subject.promote_to_upload! + expect(upload.errors.full_messages).to include( + "Original filename " + I18n.t("upload.unauthorized", authorized_extensions: "") + ) + end + end + + context "when the upload does get changed by the UploadCreator" do + let(:file) { file_from_fixtures("should_be_jpeg.heic", "images") } + + it "creates a new upload in s3 (not copy) and deletes the original stubbed upload" do + upload = subject.promote_to_upload! + expect(WebMock).to have_requested( + :put, + "#{upload_base_url}/original/1X/#{upload.sha1}.png" + ) + expect(WebMock).to have_requested( + :delete, "#{upload_base_url}/#{external_upload_stub.key}" + ) + end + end + + context "when the sha has been set on the s3 object metadata by the clientside JS" do + let(:metadata_headers) { { "x-amz-meta-sha1-checksum" => client_sha1 } } + + context "when the downloaded file sha1 does not match the client sha1" do + let(:client_sha1) { "blahblah" } + + it "raises an error and marks upload as failed" do + expect { subject.promote_to_upload! }.to raise_error(ExternalUploadManager::ChecksumMismatchError) + expect(external_upload_stub.reload.status).to eq(ExternalUploadStub.statuses[:failed]) + end + end + end + end + + context "when stubbed upload is > DOWNLOAD_LIMIT (too big to download, generate a fake sha)" do + let(:object_size) { 200.megabytes } + let(:object_file) { pdf_file } + let!(:external_upload_stub) { Fabricate(:attachment_external_upload_stub, created_by: user) } + + before do + UploadCreator.any_instance.stubs(:generate_fake_sha1_hash).returns("testbc60eb18e8f974cbfae8bb0f069c3a311024") + end + + it "does not try and download the file" do + FileHelper.expects(:download).never + subject.promote_to_upload! + end + + it "generates a fake sha for the upload record" do + upload = subject.promote_to_upload! + expect(upload.sha1).not_to eq(sha1) + expect(upload.original_sha1).to eq(nil) + expect(upload.filesize).to eq(object_size) + end + + it "marks the stub as uploaded" do + subject.promote_to_upload! + expect(external_upload_stub.reload.status).to eq(ExternalUploadStub.statuses[:uploaded]) + end + + it "copies the stubbed upload on S3 to its new destination and deletes it" do + upload = subject.promote_to_upload! + expect(WebMock).to have_requested( + :put, + "#{upload_base_url}/original/1X/#{upload.sha1}.pdf" + ).with(headers: { 'X-Amz-Copy-Source' => "#{SiteSetting.s3_upload_bucket}/#{external_upload_stub.key}" }) + expect(WebMock).to have_requested( + :delete, "#{upload_base_url}/#{external_upload_stub.key}" + ) + end + end + end + + def stub_head_object + stub_request( + :head, + "#{upload_base_url}/#{external_upload_stub.key}" + ).to_return( + status: 200, + headers: { + ETag: etag, + "Content-Length" => object_size, + "Content-Type" => "image/png", + }.merge(metadata_headers) + ) + end + + def stub_download_object_filehelper + signed_url = Discourse.store.signed_url_for_path(external_upload_stub.key) + uri = URI.parse(signed_url) + signed_url = uri.to_s.gsub(uri.query, "") + stub_request(:get, signed_url).with(query: hash_including({})).to_return( + status: 200, + body: object_file.read + ) + end + + def stub_copy_object + copy_object_result = <<~BODY + \n + + 2021-07-19T04:10:41.000Z + "#{etag}" + + BODY + stub_request( + :put, + "#{upload_base_url}/original/1X/testbc60eb18e8f974cbfae8bb0f069c3a311024.pdf" + ).to_return( + status: 200, + headers: { "ETag" => etag }, + body: copy_object_result + ) + stub_request( + :put, + "#{upload_base_url}/original/1X/bc975735dfc6409c1c2aa5ebf2239949bcbdbd65.png" + ).to_return( + status: 200, + headers: { "ETag" => etag }, + body: copy_object_result + ) + end + + def stub_delete_object + stub_request( + :delete, "#{upload_base_url}/#{external_upload_stub.key}" + ).to_return( + status: 200 + ) + end +end