From f49c9f6c43278189c8502909f95c9f8c2b34127e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 16 Jan 2017 19:53:31 +0100 Subject: [PATCH] FIX: log backups download/destroy staff action FIX: clean up junk left by the specs RENAME: 'backup_operation' to 'backup_create' to match other backup log types --- app/controllers/admin/backups_controller.rb | 12 +++++----- app/models/backup.rb | 4 +++- app/models/user_history.rb | 13 ++++++----- app/services/staff_action_logger.rb | 22 +++++++++++++++++-- config/locales/client.en.yml | 4 +++- .../admin/backups_controller_spec.rb | 22 ++++++++++++++----- 6 files changed, 56 insertions(+), 21 deletions(-) diff --git a/app/controllers/admin/backups_controller.rb b/app/controllers/admin/backups_controller.rb index feed2a709d1..b0449ec6069 100644 --- a/app/controllers/admin/backups_controller.rb +++ b/app/controllers/admin/backups_controller.rb @@ -32,7 +32,7 @@ class Admin::BackupsController < Admin::AdminController rescue BackupRestore::OperationRunningError render json: failed_json.merge(message: I18n.t("backup.operation_already_running")) else - StaffActionLogger.new(current_user).log_backup_operation + StaffActionLogger.new(current_user).log_backup_create render json: success_json end @@ -46,9 +46,9 @@ class Admin::BackupsController < Admin::AdminController # download def show - filename = params.fetch(:id) - if backup = Backup[filename] - headers['Content-Length'] = File.size(backup.path) + if backup = Backup[params.fetch(: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 @@ -56,8 +56,8 @@ class Admin::BackupsController < Admin::AdminController end def destroy - backup = Backup[params.fetch(:id)] - if backup + if backup = Backup[params.fetch(:id)] + StaffActionLogger.new(current_user).log_backup_destroy(backup) backup.remove render nothing: true else diff --git a/app/models/backup.rb b/app/models/backup.rb index dfa5381219b..d23d459d008 100644 --- a/app/models/backup.rb +++ b/app/models/backup.rb @@ -61,7 +61,9 @@ class Backup end def self.base_directory - File.join(Rails.root, "public", "backups", RailsMultisite::ConnectionManagement.current_db) + base_directory = File.join(Rails.root, "public", "backups", RailsMultisite::ConnectionManagement.current_db) + FileUtils.mkdir_p(base_directory) unless Dir.exists?(base_directory) + base_directory end def self.chunk_path(identifier, filename, chunk_number) diff --git a/app/models/user_history.rb b/app/models/user_history.rb index ddc48286630..50b616391c3 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -51,7 +51,7 @@ class UserHistory < ActiveRecord::Base revoke_admin: 33, grant_moderation: 34, revoke_moderation: 35, - backup_operation: 36, + backup_create: 36, rate_limited_like: 37, # not used anymore revoke_email: 38, deactivate_user: 39, @@ -59,8 +59,9 @@ class UserHistory < ActiveRecord::Base lock_trust_level: 41, unlock_trust_level: 42, activate_user: 43, - change_readonly_mode: 44 - ) + change_readonly_mode: 44, + backup_download: 45, + backup_destroy: 46) end # Staff actions is a subset of all actions, used to audit actions taken by staff users. @@ -93,13 +94,15 @@ class UserHistory < ActiveRecord::Base :revoke_admin, :grant_moderation, :revoke_moderation, - :backup_operation, + :backup_create, :revoke_email, :deactivate_user, :lock_trust_level, :unlock_trust_level, :activate_user, - :change_readonly_mode] + :change_readonly_mode, + :backup_download, + :backup_destroy] end def self.staff_action_ids diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index c872fdd0f6b..2bd24889fe7 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -336,13 +336,31 @@ class StaffActionLogger })) end - def log_backup_operation(opts={}) + def log_backup_create(opts={}) UserHistory.create(params(opts).merge({ - action: UserHistory.actions[:backup_operation], + action: UserHistory.actions[:backup_create], ip_address: @admin.ip_address.to_s })) end + def log_backup_download(backup, opts={}) + raise Discourse::InvalidParameters.new(:backup) unless backup + UserHistory.create(params(opts).merge({ + action: UserHistory.actions[:backup_download], + ip_address: @admin.ip_address.to_s, + details: backup.filename + })) + end + + def log_backup_destroy(backup, opts={}) + raise Discourse::InvalidParameters.new(:backup) unless backup + UserHistory.create(params(opts).merge({ + action: UserHistory.actions[:backup_destroy], + ip_address: @admin.ip_address.to_s, + details: backup.filename + })) + end + def log_revoke_email(user, reason, opts={}) UserHistory.create(params(opts).merge({ action: UserHistory.actions[:revoke_email], diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 0620de21f37..5b38645a9e6 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2911,7 +2911,7 @@ en: revoke_admin: "revoke admin" grant_moderation: "grant moderation" revoke_moderation: "revoke moderation" - backup_operation: "backup operation" + backup_create: "create backup" deleted_tag: "deleted tag" renamed_tag: "renamed tag" revoke_email: "revoke email" @@ -2920,6 +2920,8 @@ en: activate_user: "activate user" deactivate_user: "deactivate user" change_readonly_mode: "change readonly mode" + backup_download: "download backup" + backup_destroy: "destroy backup" screened_emails: title: "Screened Emails" description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed." diff --git a/spec/controllers/admin/backups_controller_spec.rb b/spec/controllers/admin/backups_controller_spec.rb index 270dc9a2317..0669f667b3e 100644 --- a/spec/controllers/admin/backups_controller_spec.rb +++ b/spec/controllers/admin/backups_controller_spec.rb @@ -90,16 +90,18 @@ describe Admin::BackupsController do describe ".show" do it "uses send_file to transmit the backup" do - FileUtils.mkdir_p Backup.base_directory - File.open(Backup.base_directory << "/" << backup_filename, "w") do |f| - f.write("hello") - end + path = File.join(Backup.base_directory, backup_filename) + File.open(path, "w") { |f| f.write("hello") } Backup.create_from_filename(backup_filename) + StaffActionLogger.any_instance.expects(:log_backup_download).once + get :show, id: backup_filename - expect(response.headers['Content-Length']).to eq(5) + File.delete(path) rescue nil + + expect(response.headers['Content-Length']).to eq("5") expect(response.headers['Content-Disposition']).to match(/attachment; filename/) end @@ -120,7 +122,11 @@ describe Admin::BackupsController do it "removes the backup if found" do Backup.expects(:[]).with(backup_filename).returns(b) b.expects(:remove) + + StaffActionLogger.any_instance.expects(:log_backup_destroy).with(b).once + xhr :delete, :destroy, id: backup_filename + expect(response).to be_success end @@ -223,8 +229,10 @@ describe Admin::BackupsController do it "should upload the file successfully" do described_class.any_instance.expects(:has_enough_space_on_disk?).returns(true) + filename = 'test_Site-0123456789.tar.gz' + xhr :post, :upload_backup_chunk, - resumableFilename: 'test_Site-0123456789.tar.gz', + resumableFilename: filename, resumableTotalSize: 1, resumableIdentifier: 'test', resumableChunkNumber: '1', @@ -234,6 +242,8 @@ describe Admin::BackupsController do expect(response.status).to eq(200) expect(response.body).to eq("") + + File.delete(File.join(Backup.base_directory, filename)) rescue nil end end end