From 80858bae2cbd602daea10eccaaca7046f293a84d Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Wed, 1 Mar 2017 08:26:18 -0700 Subject: [PATCH] FEATURE: further restrict downloading of backups - send email to logged in admin when they press the "download" button - show pop-up that email was sent - create email template - require a valid token to download backup --- .../controllers/admin-backups-index.js.es6 | 9 +- app/controllers/admin/backups_controller.rb | 26 +++++- app/jobs/regular/download_backup_email.rb | 22 +++++ app/mailers/download_backup_mailer.rb | 9 ++ app/views/admin/backups/show.html.erb | 7 ++ config/locales/client.en.yml | 3 +- config/locales/server.en.yml | 8 ++ config/routes.rb | 1 + lib/email_backup_token.rb | 28 ++++++ .../admin/backups_controller_spec.rb | 92 ++++++++++++++----- 10 files changed, 175 insertions(+), 30 deletions(-) create mode 100644 app/jobs/regular/download_backup_email.rb create mode 100644 app/mailers/download_backup_mailer.rb create mode 100644 app/views/admin/backups/show.html.erb create mode 100644 lib/email_backup_token.rb diff --git a/app/assets/javascripts/admin/controllers/admin-backups-index.js.es6 b/app/assets/javascripts/admin/controllers/admin-backups-index.js.es6 index 558b9b4973e..67b6c58d061 100644 --- a/app/assets/javascripts/admin/controllers/admin-backups-index.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-backups-index.js.es6 @@ -1,4 +1,3 @@ -import DiscourseURL from 'discourse/lib/url'; import { ajax } from 'discourse/lib/ajax'; export default Ember.Controller.extend({ @@ -39,7 +38,11 @@ export default Ember.Controller.extend({ }, download(backup) { - DiscourseURL.redirectTo(backup.get('link')); + let link = backup.get('filename'); + ajax("/admin/backups/" + link, { type: "PUT" }) + .then(() => { + bootbox.alert(I18n.t("admin.backups.operations.download.alert")); + }); } }, @@ -48,7 +51,7 @@ export default Ember.Controller.extend({ ajax("/admin/backups/readonly", { type: "PUT", data: { enable: enable } - }).then(function() { + }).then(() => { site.set("isReadOnly", enable); }); } diff --git a/app/controllers/admin/backups_controller.rb b/app/controllers/admin/backups_controller.rb index b0449ec6069..d0922f88638 100644 --- a/app/controllers/admin/backups_controller.rb +++ b/app/controllers/admin/backups_controller.rb @@ -1,4 +1,5 @@ require "backup_restore/backup_restore" +require "email_backup_token" class Admin::BackupsController < Admin::AdminController @@ -44,14 +45,33 @@ class Admin::BackupsController < Admin::AdminController render json: success_json end - # download - def show + def email if backup = Backup[params.fetch(:id)] + token = EmailBackupToken.set(current_user.id) + download_url = "#{url_for(controller: 'backups', action: 'show')}?token=#{token}" + Jobs.enqueue(:download_backup_email, to_address: current_user.email, backup_file_path: download_url) + render nothing: true + else + render nothing: true, status: 404 + end + end + + def show + + if !EmailBackupToken.compare(current_user.id, params.fetch(:token)) + @error = I18n.t('download_backup_mailer.no_token') + end + if !@error && backup = Backup[params.fetch(:id)] + EmailBackupToken.del(current_user.id) StaffActionLogger.new(current_user).log_backup_download(backup) headers['Content-Length'] = File.size(backup.path).to_s send_file backup.path else - render nothing: true, status: 404 + if @error + render layout: 'no_ember', status: 422 + else + render nothing: true, status: 404 + end end end diff --git a/app/jobs/regular/download_backup_email.rb b/app/jobs/regular/download_backup_email.rb new file mode 100644 index 00000000000..dbf12cdd130 --- /dev/null +++ b/app/jobs/regular/download_backup_email.rb @@ -0,0 +1,22 @@ +require_dependency 'email/sender' + +module Jobs + + class DownloadBackupEmail < Jobs::Base + + sidekiq_options queue: 'critical' + + def execute(args) + to_address = args[:to_address] + backup_file_path = args[:backup_file_path] + + raise Discourse::InvalidParameters.new(:to_address) if to_address.blank? + raise Discourse::InvalidParameters.new(:backup_file_path) if backup_file_path.blank? + + message = DownloadBackupMailer.send_email(to_address, backup_file_path) + Email::Sender.new(message, :download_backup_message).send + end + + end + +end diff --git a/app/mailers/download_backup_mailer.rb b/app/mailers/download_backup_mailer.rb new file mode 100644 index 00000000000..1eb47e7222c --- /dev/null +++ b/app/mailers/download_backup_mailer.rb @@ -0,0 +1,9 @@ +require_dependency 'email/message_builder' + +class DownloadBackupMailer < ActionMailer::Base + include Email::BuildEmailHelper + + def send_email(to_address, backup_file_path) + build_email(to_address, template: 'download_backup_mailer', backup_file_path: backup_file_path) + end +end diff --git a/app/views/admin/backups/show.html.erb b/app/views/admin/backups/show.html.erb new file mode 100644 index 00000000000..1171e689dc6 --- /dev/null +++ b/app/views/admin/backups/show.html.erb @@ -0,0 +1,7 @@ +
+ <%if @error%> +
+ <%= @error %> +
+ <%end%> +
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 6ba5a81f87f..413256ce17b 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2689,7 +2689,8 @@ en: without_uploads: "Yes (do not include files)" download: label: "Download" - title: "Download the backup" + title: "Send email with download link" + alert: "A link to download this backup has been emailed to you." destroy: title: "Remove the backup" confirm: "Are you sure you want to destroy this backup?" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c37d244b91b..63f8ee6f284 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1762,6 +1762,14 @@ en: (If the link above has expired, choose "I forgot my password" when logging in with your email address.) + download_backup_mailer: + title: "Download Backup Mailer" + subject_template: "[%{site_name}] Backup Download" + text_body_template: | + Click this link to download your backup file: %{backup_file_path} + no_token: | + Sorry, this backup download link has already been used or has expired. + test_mailer: title: "Test Mailer" subject_template: "[%{site_name}] Email Deliverability Test" diff --git a/config/routes.rb b/config/routes.rb index cfc2c864ee8..bc75ad0c621 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -232,6 +232,7 @@ Discourse::Application.routes.draw do resources :backups, only: [:index, :create], constraints: AdminConstraint.new do member do get "" => "backups#show", constraints: { id: BACKUP_ROUTE_FORMAT } + put "" => "backups#email", constraints: { id: BACKUP_ROUTE_FORMAT } delete "" => "backups#destroy", constraints: { id: BACKUP_ROUTE_FORMAT } post "restore" => "backups#restore", constraints: { id: BACKUP_ROUTE_FORMAT } end diff --git a/lib/email_backup_token.rb b/lib/email_backup_token.rb new file mode 100644 index 00000000000..e1f9e6dac72 --- /dev/null +++ b/lib/email_backup_token.rb @@ -0,0 +1,28 @@ +class EmailBackupToken + + def self.key(user_id) + "email-backup-token:#{user_id}" + end + + def self.generate + SecureRandom.hex + end + + def self.set(user_id) + token = self.generate + $redis.setex self.key(user_id), 1.day.to_i, token + token + end + + def self.get(user_id) + $redis.get self.key(user_id) + end + + def self.del(user_id) + $redis.del self.key(user_id) + end + + def self.compare(user_id, token) + token == self.get(user_id) + end +end diff --git a/spec/controllers/admin/backups_controller_spec.rb b/spec/controllers/admin/backups_controller_spec.rb index 0669f667b3e..00e8f46950a 100644 --- a/spec/controllers/admin/backups_controller_spec.rb +++ b/spec/controllers/admin/backups_controller_spec.rb @@ -90,25 +90,69 @@ describe Admin::BackupsController do describe ".show" do it "uses send_file to transmit the backup" do - path = File.join(Backup.base_directory, backup_filename) - File.open(path, "w") { |f| f.write("hello") } + begin + token = EmailBackupToken.set(@admin.id) + path = File.join(Backup.base_directory, backup_filename) + File.open(path, "w") { |f| f.write("hello") } - Backup.create_from_filename(backup_filename) + Backup.create_from_filename(backup_filename) - StaffActionLogger.any_instance.expects(:log_backup_download).once + StaffActionLogger.any_instance.expects(:log_backup_download).once - get :show, id: backup_filename + get :show, id: backup_filename, token: token - File.delete(path) rescue nil - expect(response.headers['Content-Length']).to eq("5") - expect(response.headers['Content-Disposition']).to match(/attachment; filename/) + expect(response.headers['Content-Length']).to eq("5") + expect(response.headers['Content-Disposition']).to match(/attachment; filename/) + ensure + File.delete(path) + EmailBackupToken.del(@admin.id) + end + end + + it "returns 422 when token is bad" do + begin + path = File.join(Backup.base_directory, backup_filename) + File.open(path, "w") { |f| f.write("hello") } + + Backup.create_from_filename(backup_filename) + + get :show, id: backup_filename, token: "bad_value" + + expect(response.status).to eq(422) + ensure + File.delete(path) + end end it "returns 404 when the backup does not exist" do + token = EmailBackupToken.set(@admin.id) Backup.expects(:[]).returns(nil) - get :show, id: backup_filename + get :show, id: backup_filename, token: token + + EmailBackupToken.del(@admin.id) + + expect(response).to be_not_found + end + + end + + describe ".email" do + + let(:b) { Backup.new(backup_filename) } + + it "enqueues email job" do + Backup.expects(:[]).with(backup_filename).returns(b) + Jobs.expects(:enqueue).with(:download_backup_email, has_entries(to_address: @admin.email)) + + xhr :put, :email, id: backup_filename + + expect(response).to be_success + end + + it "returns 404 when the backup does not exist" do + xhr :put, :email, id: backup_filename expect(response).to be_not_found end @@ -227,23 +271,25 @@ describe Admin::BackupsController do describe "when filename is valid" do it "should upload the file successfully" do - described_class.any_instance.expects(:has_enough_space_on_disk?).returns(true) + begin + described_class.any_instance.expects(:has_enough_space_on_disk?).returns(true) - filename = 'test_Site-0123456789.tar.gz' + filename = 'test_Site-0123456789.tar.gz' - xhr :post, :upload_backup_chunk, - resumableFilename: filename, - resumableTotalSize: 1, - resumableIdentifier: 'test', - resumableChunkNumber: '1', - resumableChunkSize: '1', - resumableCurrentChunkSize: '1', - file: fixture_file_upload(Tempfile.new) + xhr :post, :upload_backup_chunk, + resumableFilename: filename, + resumableTotalSize: 1, + resumableIdentifier: 'test', + resumableChunkNumber: '1', + resumableChunkSize: '1', + resumableCurrentChunkSize: '1', + file: fixture_file_upload(Tempfile.new) - expect(response.status).to eq(200) - expect(response.body).to eq("") - - File.delete(File.join(Backup.base_directory, filename)) rescue nil + expect(response.status).to eq(200) + expect(response.body).to eq("") + ensure + File.delete(File.join(Backup.base_directory, filename)) + end end end end