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

@@ -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[params.fetch(:id)]
-    if backup = Backup[filename]
+      StaffActionLogger.new(current_user).log_backup_download(backup)
-      headers['Content-Length'] = File.size(backup.path)
+      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 = Backup[params.fetch(:id)]
-    if backup
+      StaffActionLogger.new(current_user).log_backup_destroy(backup)
       backup.remove
       render nothing: true
     else

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)

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

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],

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."

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
+        path = File.join(Backup.base_directory, backup_filename)
-        File.open(Backup.base_directory << "/" << backup_filename, "w") do |f|
+        File.open(path, "w") { |f| f.write("hello") }
-          f.write("hello")
-        end
 
         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