FIX: create upload record for exported csv files

app/controllers/export_csv_controller.rb

@@ -8,24 +8,7 @@ class ExportCsvController < ApplicationController
     render json: success_json
   end
 
-  # download
-  def show
-    params.require(:id)
-    filename = params.fetch(:id)
-    export_id = filename.split('-')[-1].split('.')[0]
-    export_initiated_by_user_id = 0
-    export_initiated_by_user_id = UserExport.where(id: export_id)[0].user_id unless UserExport.where(id: export_id).empty?
-    export_csv_path = UserExport.get_download_path(filename)
-
-    if export_csv_path && current_user.present? && export_initiated_by_user_id == current_user.id
-      send_file export_csv_path
-    else
-      render body: nil, status: 404
-    end
-  end
-
   private
-
     def export_params
       @_export_params ||= begin
         params.require(:entity)

app/jobs/regular/export_csv_file.rb

@@ -1,5 +1,6 @@
 require 'csv'
 require_dependency 'system_message'
+require_dependency 'upload_creator'
 
 module Jobs
 
@@ -35,8 +36,8 @@ module Jobs
         "#{@entity.split('_').join('-')}-#{Time.now.strftime("%y%m%d-%H%M%S")}"
       end
 
-      file = UserExport.create(file_name: file_name_prefix, user_id: @current_user.id)
-      file_name = "#{file_name_prefix}-#{file.id}.csv"
+      user_export = UserExport.create(file_name: file_name_prefix, user_id: @current_user.id)
+      file_name = "#{file_name_prefix}-#{user_export.id}.csv"
       absolute_path = "#{UserExport.base_directory}/#{file_name}"
 
       # ensure directory exists
@@ -50,8 +51,33 @@ module Jobs
 
       # compress CSV file
       system('gzip', '-5', absolute_path)
+
+      # create upload
+      download_link = nil
+      compressed_file_path = "#{absolute_path}.gz"
+      file_size = number_to_human_size(File.size(compressed_file_path))
+
+      if File.exist?(compressed_file_path)
+        File.open(compressed_file_path) do |file|
+          upload = UploadCreator.new(
+            file,
+            File.basename(compressed_file_path),
+            type: 'csv_export',
+            for_export: 'true'
+          ).create_for(@current_user.id)
+
+          if upload.persisted?
+            user_export.update_columns(upload_id: upload.id)
+            download_link = "#{Discourse.base_uri}/#{upload.url}"
+          else
+            Rails.logger.warn("Failed to upload the file #{Discourse.base_uri}/export_csv/#{file_name}.gz")
+          end
+        end
+        File.delete(compressed_file_path)
+      end
+
     ensure
-      notify_user(file_name, absolute_path)
+      notify_user(download_link, file_name, file_size)
     end
 
     def user_archive_export
@@ -325,15 +351,15 @@ module Jobs
         screened_url_array
       end
 
-      def notify_user(file_name, absolute_path)
+      def notify_user(download_link, file_name, file_size)
         if @current_user
-          if file_name.present? && File.exists?("#{absolute_path}.gz")
+          if download_link.present?
             SystemMessage.create_from_system_user(
               @current_user,
               :csv_export_succeeded,
-              download_link: "#{Discourse.base_uri}/export_csv/#{file_name}.gz",
+              download_link: download_link,
               file_name: "#{file_name}.gz",
-              file_size: number_to_human_size(File.size("#{absolute_path}.gz"))
+              file_size: file_size
             )
           else
             SystemMessage.create_from_system_user(@current_user, :csv_export_failed)

app/jobs/scheduled/clean_up_uploads.rb

@@ -40,6 +40,7 @@ module Jobs
         .joins("LEFT JOIN categories c ON c.uploaded_logo_id = uploads.id OR c.uploaded_background_id = uploads.id")
         .joins("LEFT JOIN custom_emojis ce ON ce.upload_id = uploads.id")
         .joins("LEFT JOIN theme_fields tf ON tf.upload_id = uploads.id")
+        .joins("LEFT JOIN user_exports ue ON ue.upload_id = uploads.id")
         .where("pu.upload_id IS NULL")
         .where("u.uploaded_avatar_id IS NULL")
         .where("ua.gravatar_upload_id IS NULL AND ua.custom_upload_id IS NULL")
@@ -47,6 +48,7 @@ module Jobs
         .where("c.uploaded_logo_id IS NULL AND c.uploaded_background_id IS NULL")
         .where("ce.upload_id IS NULL")
         .where("tf.upload_id IS NULL")
+        .where("ue.upload_id IS NULL")
 
       result = result.where("uploads.url NOT IN (?)", ignore_urls) if ignore_urls.present?
 

app/models/upload.rb

@@ -17,6 +17,7 @@ class Upload < ActiveRecord::Base
   attr_accessor :for_group_message
   attr_accessor :for_theme
   attr_accessor :for_private_message
+  attr_accessor :for_export
 
   validates_presence_of :filesize
   validates_presence_of :original_filename

app/models/user_export.rb

@@ -1,10 +1,5 @@
 class UserExport < ActiveRecord::Base
 
-  def self.get_download_path(filename)
-    path = File.join(UserExport.base_directory, filename)
-    File.exists?(path) ? path : nil
-  end
-
   def self.remove_old_exports
     UserExport.where('created_at < ?', 2.days.ago).find_each do |expired_export|
       file_name = "#{expired_export.file_name}-#{expired_export.id}.csv.gz"
@@ -30,4 +25,5 @@ end
 #  user_id    :integer          not null
 #  created_at :datetime         not null
 #  updated_at :datetime         not null
+#  upload_id  :integer
 #

config/routes.rb

@@ -710,9 +710,6 @@ Discourse::Application.routes.draw do
     collection do
       post "export_entity" => "export_csv#export_entity"
     end
-    member do
-      get "" => "export_csv#show", constraints: { id: /[^\/]+/ }
-    end
   end
 
   get "onebox" => "onebox#show"

config/site_settings.yml

@@ -822,6 +822,10 @@ files:
     default: ''
     refresh: true
     type: list
+  export_authorized_extensions:
+    hidden: true
+    default: 'gz'
+    type: list
   crawl_images:
     default: true
   max_image_width:

db/migrate/20180419095326_add_upload_id_to_user_exports.rb

@@ -0,0 +1,5 @@
+class AddUploadIdToUserExports < ActiveRecord::Migration[5.1]
+  def change
+    add_column :user_exports, :upload_id, :integer
+  end
+end

lib/upload_creator.rb

@@ -20,6 +20,7 @@ class UploadCreator
   #  - for_theme (boolean)
   #  - for_private_message (boolean)
   #  - pasted (boolean)
+  #  - for_export (boolean)
   def initialize(file, filename, opts = {})
     @file = file
     @filename = filename || ''
@@ -84,6 +85,7 @@ class UploadCreator
       @upload.for_private_message = true if @opts[:for_private_message]
       @upload.for_group_message   = true if @opts[:for_group_message]
       @upload.for_theme           = true if @opts[:for_theme]
+      @upload.for_export          = true if @opts[:for_export]
 
       return @upload unless @upload.save
 

lib/validators/upload_validator.rb

@@ -63,7 +63,13 @@ class Validators::UploadValidator < ActiveModel::Validator
   end
 
   def authorized_extensions(upload)
-    extensions = upload.for_theme ? SiteSetting.theme_authorized_extensions : SiteSetting.authorized_extensions
+    extensions = if upload.for_theme
+      SiteSetting.theme_authorized_extensions
+    elsif upload.for_export
+      SiteSetting.export_authorized_extensions
+    else
+      SiteSetting.authorized_extensions
+    end
     extensions_to_set(extensions)
   end
 
@@ -79,7 +85,13 @@ class Validators::UploadValidator < ActiveModel::Validator
     if upload.user&.staff?
       return true if SiteSetting.authorized_extensions_for_staff.include?("*")
     end
-    extensions = upload.for_theme ? SiteSetting.theme_authorized_extensions : SiteSetting.authorized_extensions
+    extensions = if upload.for_theme
+      SiteSetting.theme_authorized_extensions
+    elsif upload.for_export
+      SiteSetting.export_authorized_extensions
+    else
+      SiteSetting.authorized_extensions
+    end
     extensions.include?("*")
   end
 

spec/components/validators/upload_validator_spec.rb

@@ -0,0 +1,28 @@
+require 'rails_helper'
+
+describe Validators::UploadValidator do
+  subject(:validator) { described_class.new }
+
+  describe 'validate' do
+    let(:user) { Fabricate(:user) }
+    let(:filename) { "discourse.csv" }
+    let(:csv_file) { file_from_fixtures(filename, "csv") }
+
+    it "should create an invalid upload when the filename is blank" do
+      SiteSetting.authorized_extensions = "*"
+      created_upload = UploadCreator.new(csv_file, nil).create_for(user.id)
+      validator.validate(created_upload)
+      expect(created_upload).to_not be_valid
+      expect(created_upload.errors.full_messages.first)
+            .to include(I18n.t("activerecord.errors.messages.blank"))
+    end
+
+    it "allows 'gz' as extension when uploading export file" do
+      SiteSetting.authorized_extensions = ""
+
+      created_upload = UploadCreator.new(csv_file, "#{filename}.gz", for_export: true).create_for(user.id)
+      expect(created_upload).to be_valid
+    end
+
+  end
+end

spec/controllers/export_csv_controller_spec.rb

@@ -3,15 +3,6 @@ require "rails_helper"
 describe ExportCsvController do
   let(:export_filename) { "user-archive-codinghorror-150115-234817-999.csv.gz" }
 
-  context "while not logged in" do
-    describe ".download" do
-      it "returns 404 when the unauthorized user tries to export csv file" do
-        get :show, params: { id: export_filename }
-        expect(response.status).to eq(404)
-      end
-    end
-  end
-
   context "while logged in as normal user" do
     before { @user = log_in(:user) }
 
@@ -34,30 +25,6 @@ describe ExportCsvController do
         expect(response).not_to be_success
       end
     end
-
-    describe ".download" do
-      it "uses send_file to transmit the export file" do
-        file = UserExport.create(file_name: "user-archive-codinghorror-150116-003249", user_id: @user.id)
-        file_name = "user-archive-codinghorror-150116-003249-#{file.id}.csv.gz"
-        controller.stubs(:render)
-        export = UserExport.new()
-        UserExport.expects(:get_download_path).with(file_name).returns(export)
-        subject.expects(:send_file).with(export)
-        get :show, params: { id: file_name }
-        expect(response).to be_success
-      end
-
-      it "returns 404 when the user tries to export another user's csv file" do
-        get :show, params: { id: export_filename }
-        expect(response).to be_not_found
-      end
-
-      it "returns 404 when the export file does not exist" do
-        UserExport.expects(:get_download_path).returns(nil)
-        get :show, params: { id: export_filename }
-        expect(response).to be_not_found
-      end
-    end
   end
 
   context "while logged in as an admin" do
@@ -77,25 +44,5 @@ describe ExportCsvController do
         expect(response).to be_success
       end
     end
-
-    describe ".download" do
-      it "uses send_file to transmit the export file" do
-        file = UserExport.create(file_name: "screened-email-150116-010145", user_id: @admin.id)
-        file_name = "screened-email-150116-010145-#{file.id}.csv.gz"
-        controller.stubs(:render)
-        export = UserExport.new()
-        UserExport.expects(:get_download_path).with(file_name).returns(export)
-        subject.expects(:send_file).with(export)
-        get :show, params: { id: file_name }
-        expect(response).to be_success
-      end
-
-      it "returns 404 when the export file does not exist" do
-        UserExport.expects(:get_download_path).returns(nil)
-        get :show, params: { id: export_filename }
-        expect(response).to be_not_found
-      end
-    end
   end
-
 end

spec/jobs/clean_up_uploads_spec.rb

@@ -167,4 +167,13 @@ describe Jobs::CleanUpUploads do
     expect(Upload.find_by(id: upload.id)).to eq(upload)
   end
 
+  it "does not delete user exported csv uploads" do
+    csv_file = fabricate_upload
+    UserExport.create(file_name: "export.csv", user_id: Fabricate(:user).id, upload_id: csv_file.id)
+
+    Jobs::CleanUpUploads.new.execute(nil)
+
+    expect(Upload.find_by(id: @upload.id)).to eq(nil)
+    expect(Upload.find_by(id: csv_file.id)).to eq(csv_file)
+  end
 end