From 91bf10bd12291adddab3edab2d788050fd484d43 Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Thu, 19 Apr 2018 17:00:31 +0530 Subject: [PATCH] FIX: create upload record for exported csv files --- app/controllers/export_csv_controller.rb | 17 ------ app/jobs/regular/export_csv_file.rb | 40 +++++++++++--- app/jobs/scheduled/clean_up_uploads.rb | 2 + app/models/upload.rb | 1 + app/models/user_export.rb | 6 +-- config/routes.rb | 3 -- config/site_settings.yml | 4 ++ ...419095326_add_upload_id_to_user_exports.rb | 5 ++ lib/upload_creator.rb | 2 + lib/validators/upload_validator.rb | 16 +++++- .../validators/upload_validator_spec.rb | 28 ++++++++++ .../controllers/export_csv_controller_spec.rb | 53 ------------------- spec/jobs/clean_up_uploads_spec.rb | 9 ++++ 13 files changed, 99 insertions(+), 87 deletions(-) create mode 100644 db/migrate/20180419095326_add_upload_id_to_user_exports.rb create mode 100644 spec/components/validators/upload_validator_spec.rb diff --git a/app/controllers/export_csv_controller.rb b/app/controllers/export_csv_controller.rb index 4b74781dbd9..836ad48d70d 100644 --- a/app/controllers/export_csv_controller.rb +++ b/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) diff --git a/app/jobs/regular/export_csv_file.rb b/app/jobs/regular/export_csv_file.rb index 1212bb1c875..02ee3fcd4c2 100644 --- a/app/jobs/regular/export_csv_file.rb +++ b/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) diff --git a/app/jobs/scheduled/clean_up_uploads.rb b/app/jobs/scheduled/clean_up_uploads.rb index e2b9353edee..75d53dd2839 100644 --- a/app/jobs/scheduled/clean_up_uploads.rb +++ b/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? diff --git a/app/models/upload.rb b/app/models/upload.rb index dc1a9c0bc1b..055b396e272 100644 --- a/app/models/upload.rb +++ b/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 diff --git a/app/models/user_export.rb b/app/models/user_export.rb index 34d8fa7ed94..10314c029b5 100644 --- a/app/models/user_export.rb +++ b/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 # diff --git a/config/routes.rb b/config/routes.rb index 53db9bcecea..87cb48e948c 100644 --- a/config/routes.rb +++ b/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" diff --git a/config/site_settings.yml b/config/site_settings.yml index 80b326cde67..de79862c76d 100644 --- a/config/site_settings.yml +++ b/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: diff --git a/db/migrate/20180419095326_add_upload_id_to_user_exports.rb b/db/migrate/20180419095326_add_upload_id_to_user_exports.rb new file mode 100644 index 00000000000..d29887e51ed --- /dev/null +++ b/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 diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 1086b8333ce..6d49f34340b 100644 --- a/lib/upload_creator.rb +++ b/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 diff --git a/lib/validators/upload_validator.rb b/lib/validators/upload_validator.rb index 8f6c4cede1e..5b9da9793df 100644 --- a/lib/validators/upload_validator.rb +++ b/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 diff --git a/spec/components/validators/upload_validator_spec.rb b/spec/components/validators/upload_validator_spec.rb new file mode 100644 index 00000000000..fc7db5544b2 --- /dev/null +++ b/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 diff --git a/spec/controllers/export_csv_controller_spec.rb b/spec/controllers/export_csv_controller_spec.rb index 53d5be2ea14..2e1256653c9 100644 --- a/spec/controllers/export_csv_controller_spec.rb +++ b/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 diff --git a/spec/jobs/clean_up_uploads_spec.rb b/spec/jobs/clean_up_uploads_spec.rb index 73b778d0146..6539655807e 100644 --- a/spec/jobs/clean_up_uploads_spec.rb +++ b/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