From 6c4c6f88890882378351d10320a467637bb72ca0 Mon Sep 17 00:00:00 2001 From: Thorsten Claus Date: Sun, 19 Sep 2021 14:48:38 +0200 Subject: [PATCH 1/6] Migration Backend Part --- .gitignore | 1 + app/controllers/users_controller.rb | 17 ++++ app/models/account_migration.rb | 3 +- app/models/photo.rb | 52 +++++----- app/models/user.rb | 27 ++++-- app/services/import_service.rb | 96 +++++++++++++++++++ app/services/migration_service.rb | 16 ++++ app/uploaders/exported_photos.rb | 10 +- app/uploaders/exported_user.rb | 8 +- app/uploaders/secure_uploader.rb | 3 +- app/workers/import_user.rb | 36 +++++++ app/workers/process_photo.rb | 2 +- lib/archive_importer.rb | 6 +- lib/tasks/accounts.rake | 52 ++++------ .../receive_federation_messages_spec.rb | 9 -- spec/lib/archive_importer_spec.rb | 1 - spec/models/user_spec.rb | 2 +- 17 files changed, 257 insertions(+), 84 deletions(-) create mode 100644 app/services/import_service.rb create mode 100644 app/workers/import_user.rb diff --git a/.gitignore b/.gitignore index a91f73413..69a800c39 100644 --- a/.gitignore +++ b/.gitignore @@ -77,3 +77,4 @@ diaspora.iml # WebTranslateIt .wti +/__MACOSX/ diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 726f46f2e..45bec9817 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -154,6 +154,8 @@ class UsersController < ApplicationController :post_default_public, :otp_required_for_login, :otp_secret, + :exported_photos_file, + :export, email_preferences: UserPreference::VALID_EMAIL_TYPES.map(&:to_sym) ) end @@ -172,6 +174,8 @@ class UsersController < ApplicationController change_post_default(user_data) elsif user_data[:color_theme] change_settings(user_data, "users.update.color_theme_changed", "users.update.color_theme_not_changed") + elsif user_data[:export] || user_data[:exported_photos_file] + upload_export_files(user_data) else change_settings(user_data) end @@ -235,6 +239,19 @@ class UsersController < ApplicationController end end + def upload_export_files(user_data) + logger.info "Start importing account" + @user.export = user_data[:export] if user_data[:export] + @user.exported_photos_file = user_data[:exported_photos_file] if user_data[:exported_photos_file] + if @user.save + flash.now[:notice] = "Your account migration has been scheduled" + else + flash.now[:error] = "Your account migration could not be scheduled for the following reason:"\ + " #{@user.errors.full_messages}" + end + Workers::ImportUser.perform_async(@user.id) + end + def change_settings(user_data, successful="users.update.settings_updated", error="users.update.settings_not_updated") if @user.update_attributes(user_data) flash.now[:notice] = t(successful) diff --git a/app/models/account_migration.rb b/app/models/account_migration.rb index 478179725..b971b881b 100644 --- a/app/models/account_migration.rb +++ b/app/models/account_migration.rb @@ -7,7 +7,7 @@ class AccountMigration < ApplicationRecord belongs_to :new_person, class_name: "Person" validates :old_person, uniqueness: true - validates :new_person, uniqueness: true + validates :new_person, presence: true after_create :lock_old_user! @@ -28,7 +28,6 @@ class AccountMigration < ApplicationRecord @sender ||= old_user || ephemeral_sender end - # executes a migration plan according to this AccountMigration object def perform! raise "already performed" if performed? diff --git a/app/models/photo.rb b/app/models/photo.rb index 9678db4ac..0a77df62a 100644 --- a/app/models/photo.rb +++ b/app/models/photo.rb @@ -22,7 +22,7 @@ class Photo < ApplicationRecord large: photo.url(:scaled_full), raw: photo.url } - }, :as => :sizes + }, as: :sizes t.add lambda { |photo| { height: photo.height, @@ -48,25 +48,25 @@ class Photo < ApplicationRecord before_destroy :ensure_user_picture after_destroy :clear_empty_status_message - after_commit :on => :create do - queue_processing_job if self.author.local? + after_commit on: :create do + queue_processing_job if author.local? end scope :on_statuses, ->(post_guids) { - where(:status_message_guid => post_guids) + where(status_message_guid: post_guids) } def clear_empty_status_message - if self.status_message && self.status_message.text_and_photos_blank? - self.status_message.destroy + if status_message&.text_and_photos_blank? + status_message.destroy else true end end def ownership_of_status_message - message = StatusMessage.find_by_guid(self.status_message_guid) + message = StatusMessage.find_by(guid: status_message_guid) return unless status_message_guid && message && diaspora_handle != message.diaspora_handle errors.add(:base, "Photo must have the same owner as status message") @@ -96,26 +96,22 @@ class Photo < ApplicationRecord end def update_remote_path - unless self.unprocessed_image.url.match(/^https?:\/\//) - remote_path = "#{AppConfig.pod_uri.to_s.chomp("/")}#{self.unprocessed_image.url}" - else - remote_path = self.unprocessed_image.url - end + remote_path = if unprocessed_image.url.match(%r{^https?://}) + unprocessed_image.url + else + "#{AppConfig.pod_uri.to_s.chomp('/')}#{unprocessed_image.url}" + end - name_start = remote_path.rindex '/' + name_start = remote_path.rindex "/" self.remote_photo_path = "#{remote_path.slice(0, name_start)}/" self.remote_photo_name = remote_path.slice(name_start + 1, remote_path.length) end - def url(name = nil) - if remote_photo_path - name = name.to_s + '_' if name + def url(name=nil) + if remote_photo_path.present? && remote_photo_name.present? + name = "#{name}_" if name image_url = remote_photo_path + name.to_s + remote_photo_name - if AppConfig.privacy.camo.proxy_remote_pod_images? - Diaspora::Camo.image_url(image_url) - else - image_url - end + camo_image_url(image_url) elsif processed? processed_image.url(name) else @@ -124,7 +120,7 @@ class Photo < ApplicationRecord end def ensure_user_picture - profiles = Profile.where(:image_url => url(:thumb_large)) + profiles = Profile.where(image_url: url(:thumb_large)) profiles.each { |profile| profile.image_url = nil profile.save @@ -132,7 +128,7 @@ class Photo < ApplicationRecord end def queue_processing_job - Workers::ProcessPhoto.perform_async(self.id) + Workers::ProcessPhoto.perform_async(id) end def self.visible(current_user, person, limit=:all, max_time=nil) @@ -143,4 +139,14 @@ class Photo < ApplicationRecord end photos.where(pending: false).order("created_at DESC") end + + private + + def camo_image_url(image_url) + if AppConfig.privacy.camo.proxy_remote_pod_images? + Diaspora::Camo.image_url(image_url) + else + image_url + end + end end diff --git a/app/models/user.rb b/app/models/user.rb index 788fe6aba..28813c604 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -325,9 +325,9 @@ class User < ApplicationRecord else update exporting: false end - rescue => error - logger.error "Unexpected error while exporting user '#{username}': #{error.class}: #{error.message}\n" \ - "#{error.backtrace.first(15).join("\n")}" + rescue StandardError => e + logger.error "Unexpected error while exporting data for '#{username}': #{e.class}: #{e.message}\n" \ + "#{e.backtrace.first(15).join("\n")}" update exporting: false end @@ -335,7 +335,7 @@ class User < ApplicationRecord ActiveSupport::Gzip.compress Diaspora::Exporter.new(self).execute end - ######### Photos export ################## + ######### Photo export ################## mount_uploader :exported_photos_file, ExportedPhotos def queue_export_photos @@ -345,9 +345,9 @@ class User < ApplicationRecord def perform_export_photos! PhotoExporter.new(self).perform - rescue => error - logger.error "Unexpected error while exporting photos for '#{username}': #{error.class}: #{error.message}\n" \ - "#{error.backtrace.first(15).join("\n")}" + rescue StandardError => e + logger.error "Unexpected error while exporting photos for '#{username}': #{e.class}: #{e.message}\n" \ + "#{e.backtrace.first(15).join("\n")}" update exporting_photos: false end @@ -403,13 +403,19 @@ class User < ApplicationRecord tag_followings.any? || profile[:image_url] end - ###Helpers############ - def self.build(opts = {}) + ### Helpers ############ + def self.build(opts={}) u = User.new(opts.except(:person, :id)) u.setup(opts) u end + def self.find_or_build(opts={}) + user = User.find_by(username: opts[:username]) + user ||= User.build(opts) + user + end + def setup(opts) self.username = opts[:username] self.email = opts[:email] @@ -417,10 +423,11 @@ class User < ApplicationRecord self.language ||= I18n.locale.to_s self.color_theme = opts[:color_theme] self.color_theme ||= AppConfig.settings.default_color_theme - self.valid? + valid? errors = self.errors errors.delete :person return if errors.size > 0 + self.set_person(Person.new((opts[:person] || {}).except(:id))) self.generate_keys self diff --git a/app/services/import_service.rb b/app/services/import_service.rb new file mode 100644 index 000000000..13ca0654a --- /dev/null +++ b/app/services/import_service.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +class ImportService + include Diaspora::Logging + + def import_by_user(user) + import_by_files(user.export.current_path, user.exported_photos_file.current_path, user.username) + end + + def import_by_files(path_to_profile, path_to_photos, username) + if path_to_profile.present? + logger.info "Import for profile #{username} at path #{path_to_profile} requested" + import_user_profile(path_to_profile, username) + end + + user = User.find_by(username: username) + raise ArgumentError, "Username #{username} should exist before uploading photos." if user.nil? + + if path_to_photos.present? + logger.info("Importing photos from import file for '#{username}' from #{path_to_photos}") + import_user_photos(user, path_to_photos) + end + remove_file_references(user) + end + + private + + def import_user_profile(path_to_profile, username) + raise ArgumentError, "Profile file not found at path: #{path_to_profile}" unless File.exist?(path_to_profile) + + service = MigrationService.new(path_to_profile, username) + logger.info "Start validating user profile #{username}" + service.validate + logger.info "Start importing user profile for '#{username}'" + service.perform! + logger.info "Successfully imported profile: #{username}" + rescue MigrationService::ArchiveValidationFailed => e + logger.error "Errors in the archive found: #{e.message}" + rescue MigrationService::MigrationAlreadyExists + logger.error "Migration record already exists for the user, can't continue" + rescue MigrationService::SelfMigrationNotAllowed + logger.error "You can't migrate onto your own account" + end + + def import_user_photos(user, path_to_photos) + raise ArgumentError, "Photos file not found at path: #{path_to_photos}" unless File.exist?(path_to_photos) + + uncompressed_photos_folder = unzip_photos_file(path_to_photos) + user.posts.find_in_batches do |posts| + import_photos_for_posts(posts, uncompressed_photos_folder) + end + FileUtils.rmdir(uncompressed_photos_folder) + end + + def import_photos_for_posts(posts, source_dir) + posts.each do |post| + post.photos.each do |photo| + uploaded_file = "#{source_dir}/#{photo.remote_photo_name}" + next unless File.exist?(uploaded_file) && photo.remote_photo_name.present? + + File.open(uploaded_file) do |file| + photo.random_string = File.basename(uploaded_file, ".*") + photo.unprocessed_image = file + photo.save(touch: false) + end + photo.queue_processing_job + end + end + end + + def unzip_photos_file(photo_file_path) + folder = create_folder(photo_file_path) + Zip::File.open(photo_file_path) do |zip_file| + zip_file.each do |file| + target_name = "#{folder}#{Pathname::SEPARATOR_LIST}#{file}" + zip_file.extract(file, target_name) unless File.exist?(target_name) + rescue Errno::ENOENT => e + logger.error e.to_s + end + end + folder + end + + def create_folder(compressed_file_name) + extension = File.extname(compressed_file_name) + folder = compressed_file_name.delete_suffix(extension) + FileUtils.mkdir(folder) unless File.exist?(folder) + folder + end + + def remove_file_references(user) + user.remove_exported_photos_file + user.remove_export + user.save + end +end diff --git a/app/services/migration_service.rb b/app/services/migration_service.rb index 68f891b1f..56c13a45b 100644 --- a/app/services/migration_service.rb +++ b/app/services/migration_service.rb @@ -10,9 +10,12 @@ class MigrationService end def validate + return unless archive_file_exists? + archive_validator.validate raise ArchiveValidationFailed, errors.join("\n") if errors.any? raise MigrationAlreadyExists if AccountMigration.where(old_person: old_person).any? + raise SelfMigrationNotAllowed if self_import? end def perform! @@ -23,6 +26,12 @@ class MigrationService remove_intermediate_file end + def self_import? + source_diaspora_id = archive_validator.archive_author_diaspora_id + target_diaspora_id = "#{new_user_name}#{User.diaspora_id_host}" + source_diaspora_id == target_diaspora_id + end + # when old person can't be resolved we still import data but we don't create&perform AccountMigration instance def only_import? old_person.nil? @@ -73,6 +82,10 @@ class MigrationService File.new(archive_path, "r") end + def archive_file_exists? + File.exist?(archive_path) + end + def zip_file? filetype = MIME::Types.type_for(archive_path).first.content_type filetype.eql?("application/zip") @@ -122,4 +135,7 @@ class MigrationService class MigrationAlreadyExists < RuntimeError end + + class SelfMigrationNotAllowed < RuntimeError + end end diff --git a/app/uploaders/exported_photos.rb b/app/uploaders/exported_photos.rb index b19c4edb1..724e65ef4 100644 --- a/app/uploaders/exported_photos.rb +++ b/app/uploaders/exported_photos.rb @@ -9,7 +9,15 @@ class ExportedPhotos < SecureUploader "uploads/users" end + def extension_allowlist + %w[zip] + end + def filename - "#{model.username}_photos_#{secure_token}.zip" if original_filename.present? + return if original_filename.blank? + + filename_parts = original_filename.split(".") + extensions = filename_parts.join(".") + "#{model.username}_photos_#{secure_token}.#{extensions}" if original_filename.present? end end diff --git a/app/uploaders/exported_user.rb b/app/uploaders/exported_user.rb index 3fc0172e6..2f707eb8c 100644 --- a/app/uploaders/exported_user.rb +++ b/app/uploaders/exported_user.rb @@ -10,10 +10,14 @@ class ExportedUser < SecureUploader end def extension_allowlist - %w[gz] + %w[gz zip json] end def filename - "#{model.username}_diaspora_data_#{secure_token}.json.gz" if original_filename.present? + return if original_filename.blank? + + filename_parts = original_filename.split(".") + extensions = filename_parts.join(".") + "#{model.username}_data_#{secure_token}.#{extensions}" end end diff --git a/app/uploaders/secure_uploader.rb b/app/uploaders/secure_uploader.rb index d5e62e77b..963c047c1 100644 --- a/app/uploaders/secure_uploader.rb +++ b/app/uploaders/secure_uploader.rb @@ -2,7 +2,8 @@ class SecureUploader < CarrierWave::Uploader::Base protected - def secure_token(bytes = 16) + + def secure_token(bytes=16) var = :"@#{mounted_as}_secure_token" model.instance_variable_get(var) or model.instance_variable_set(var, SecureRandom.urlsafe_base64(bytes)) end diff --git a/app/workers/import_user.rb b/app/workers/import_user.rb new file mode 100644 index 000000000..8fb1801b3 --- /dev/null +++ b/app/workers/import_user.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Workers + class ImportUser < Base + sidekiq_options queue: :low + + include Diaspora::Logging + + def perform(user_id) + if currently_running_exports >= AppConfig.settings.export_concurrency.to_i + + logger.info "Already the maximum number of parallel user imports running, " \ + "scheduling import for User:#{user_id} in 5 minutes." + self.class.perform_in(5.minutes + rand(30), user_id) + else + import_user(user_id) + end + end + + private + + def import_user(user_id) + user = User.find(user_id) + ImportService.new.import_by_user(user) + end + + def currently_running_exports + return 0 if AppConfig.environment.single_process_mode? + + Sidekiq::Workers.new.count do |process_id, thread_id, work| + !(Process.pid.to_s == process_id.split(":")[1] && Thread.current.object_id.to_s(36) == thread_id) && + work["payload"]["class"] == self.class.to_s + end + end + end +end diff --git a/app/workers/process_photo.rb b/app/workers/process_photo.rb index 41ef597d2..9b12bd8f4 100644 --- a/app/workers/process_photo.rb +++ b/app/workers/process_photo.rb @@ -16,9 +16,9 @@ module Workers return false if photo.processed? || unprocessed_image.path.try(:include?, ".gif") photo.processed_image.store!(unprocessed_image) - photo.save! rescue ActiveRecord::RecordNotFound # Deleted before the job was run + # Ignored end end end diff --git a/lib/archive_importer.rb b/lib/archive_importer.rb index 4b307aa88..1b0bab6cb 100644 --- a/lib/archive_importer.rb +++ b/lib/archive_importer.rb @@ -31,12 +31,14 @@ class ArchiveImporter username: attr[:username], password: attr[:password], password_confirmation: attr[:password], - getting_started: false, person: { profile_attributes: profile_attributes } ) - self.user = User.build(data) + self.user = User.find_or_build(data) + user.show_community_spotlight_in_stream = data.fetch(:show_community_spotlight_in_stream, true) + user.strip_exif = data.fetch(:strip_exif, true) + user.getting_started = false user.save! end diff --git a/lib/tasks/accounts.rake b/lib/tasks/accounts.rake index c33c528fd..24077ec3e 100644 --- a/lib/tasks/accounts.rake +++ b/lib/tasks/accounts.rake @@ -2,43 +2,33 @@ namespace :accounts do desc "Perform migration" - task :migration, %i[archive_path new_user_name] => :environment do |_t, args| - puts "Account migration is requested" - args = %i[archive_path new_user_name].map {|name| [name, args[name]] }.to_h + task :migration, %i[archive_path photos_path new_user_name] => :environment do |_t, args| + puts "Account migration is requested. You can import a profile or a photos archive or booth." + args = %i[archive_path photos_path new_user_name].map {|name| [name, args[name]] }.to_h process_arguments(args) - - begin - service = MigrationService.new(args[:archive_path], args[:new_user_name]) - service.validate - puts "Warnings:\n#{service.warnings.join("\n")}\n-----" if service.warnings.any? - if service.only_import? - puts "Warning: Archive owner is not fetchable. Proceeding with data import, but account migration record "\ - "won't be created" - end - print "Do you really want to execute the archive import? Note: this is irreversible! [y/N]: " - next unless $stdin.gets.strip.casecmp?("y") - - start_time = Time.now.getlocal - service.perform! - puts service.only_import? ? "Data import complete!" : "Data import and migration complete!" - puts "Migration took #{Time.now.getlocal - start_time} seconds" - rescue MigrationService::ArchiveValidationFailed => exception - puts "Errors in the archive found:\n#{exception.message}\n-----" - rescue MigrationService::MigrationAlreadyExists - puts "Migration record already exists for the user, can't continue" + start_time = Time.now.getlocal + if args[:new_user_name].present? && (args[:archive_path].present? || args[:photos_path].present?) + ImportService.new.import_by_files(args[:archive_path], args[:photos_path], args[:new_user_name]) + puts "\n Migration completed in #{Time.now.getlocal - start_time} seconds. (Photos might still be processed in)" + else + puts "Must set a user name and a archive file path or photos file path" end end def process_arguments(args) - if args[:archive_path].nil? - print "Enter the archive path: " - args[:archive_path] = $stdin.gets.strip - end - if args[:new_user_name].nil? - print "Enter the new user name: " - args[:new_user_name] = $stdin.gets.strip - end + args[:archive_path] = request_parameter(args[:archive_path], "Enter the archive (.json, .gz, .zip) path: ") + args[:photos_path] = request_parameter(args[:photos_path], "Enter the photos (.zip) path: ") + args[:new_user_name] = request_parameter(args[:new_user_name], "Enter the new user name: ") + puts "Archive path: #{args[:archive_path]}" + puts "Photos path: #{args[:photos_path]}" puts "New username: #{args[:new_user_name]}" end end + +def request_parameter(arg, text) + return arg unless arg.nil? + + print text + $stdin.gets.strip +end diff --git a/spec/integration/federation/receive_federation_messages_spec.rb b/spec/integration/federation/receive_federation_messages_spec.rb index 382e676ec..db919b397 100644 --- a/spec/integration/federation/receive_federation_messages_spec.rb +++ b/spec/integration/federation/receive_federation_messages_spec.rb @@ -76,15 +76,6 @@ describe "Receive federation messages feature" do }.to raise_error(ActiveRecord::RecordInvalid) end - it "doesn't accept second migration for the same new user profile" do - run_migration - expect { - sender = create_remote_user("example.org") - entity = create_account_migration_entity(sender.diaspora_handle, new_user) - post_message(generate_payload(entity, sender)) - }.to raise_error(ActiveRecord::RecordInvalid) - end - context "when our pod was left" do let(:sender) { FactoryBot.create(:user) } diff --git a/spec/lib/archive_importer_spec.rb b/spec/lib/archive_importer_spec.rb index 99858f147..6ee361067 100644 --- a/spec/lib/archive_importer_spec.rb +++ b/spec/lib/archive_importer_spec.rb @@ -147,7 +147,6 @@ describe ArchiveImporter do expect { archive_importer.create_user(username: "new_name", password: "123456") }.to change(User, :count).by(1) - expect(archive_importer.user.email).to eq("user@example.com") expect(archive_importer.user.strip_exif).to eq(false) expect(archive_importer.user.show_community_spotlight_in_stream).to eq(false) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6784f4a92..268538209 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1006,7 +1006,7 @@ describe User, type: :model do expect(user.export).to be_present expect(user.exported_at).to be_present expect(user.exporting).to be_falsey - expect(user.export.filename).to match(/.json/) + expect(user.export.filename).to match(/\.json\.gz$/) expect(ActiveSupport::Gzip.decompress(user.export.file.read)).to include user.username end From 1eb2c59cce90c50f8ec862ee161a273525509c8f Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Tue, 9 Nov 2021 01:10:31 +0100 Subject: [PATCH 2/6] Move extension logic to SecureUploader class --- app/uploaders/exported_photos.rb | 6 +----- app/uploaders/exported_user.rb | 6 +----- app/uploaders/secure_uploader.rb | 4 ++++ spec/models/user_spec.rb | 7 ++++--- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/app/uploaders/exported_photos.rb b/app/uploaders/exported_photos.rb index 724e65ef4..09d2490b2 100644 --- a/app/uploaders/exported_photos.rb +++ b/app/uploaders/exported_photos.rb @@ -14,10 +14,6 @@ class ExportedPhotos < SecureUploader end def filename - return if original_filename.blank? - - filename_parts = original_filename.split(".") - extensions = filename_parts.join(".") - "#{model.username}_photos_#{secure_token}.#{extensions}" if original_filename.present? + "diaspora_#{model.username}_photos_#{secure_token}#{extension}" end end diff --git a/app/uploaders/exported_user.rb b/app/uploaders/exported_user.rb index 2f707eb8c..83f6a1f31 100644 --- a/app/uploaders/exported_user.rb +++ b/app/uploaders/exported_user.rb @@ -14,10 +14,6 @@ class ExportedUser < SecureUploader end def filename - return if original_filename.blank? - - filename_parts = original_filename.split(".") - extensions = filename_parts.join(".") - "#{model.username}_data_#{secure_token}.#{extensions}" + "diaspora_#{model.username}_data_#{secure_token}#{extension}" end end diff --git a/app/uploaders/secure_uploader.rb b/app/uploaders/secure_uploader.rb index 963c047c1..486c70e0a 100644 --- a/app/uploaders/secure_uploader.rb +++ b/app/uploaders/secure_uploader.rb @@ -3,6 +3,10 @@ class SecureUploader < CarrierWave::Uploader::Base protected + def extension + ".#{original_filename.split('.').drop(1).join('.')}" if original_filename.present? + end + def secure_token(bytes=16) var = :"@#{mounted_as}_secure_token" model.instance_variable_get(var) or model.instance_variable_set(var, SecureRandom.urlsafe_base64(bytes)) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 268538209..5e0fbf0c6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -976,13 +976,14 @@ describe User, type: :model do end describe "#export" do - it "doesn't change the filename when the user is saved" do + it "doesn't change the url when the user is saved" do user = FactoryBot.create(:user) - filename = user.export.filename + user.perform_export! + url = user.export.url user.save! - expect(User.find(user.id).export.filename).to eq(filename) + expect(User.find(user.id).export.url).to eq(url) end end From 96493b4a5c19072763ce8cb58f3a18ae0fa89d4c Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Tue, 9 Nov 2021 02:56:44 +0100 Subject: [PATCH 3/6] Refactory archive concurrency so the same logic can be reused --- app/workers/archive_base.rb | 38 ++++++++++++++++++++++++++++++++ app/workers/export_user.rb | 36 ++++++------------------------ app/workers/import_user.rb | 28 ++--------------------- config/defaults.yml | 2 +- config/diaspora.toml.example | 8 +++---- spec/workers/export_user_spec.rb | 2 +- 6 files changed, 53 insertions(+), 61 deletions(-) create mode 100644 app/workers/archive_base.rb diff --git a/app/workers/archive_base.rb b/app/workers/archive_base.rb new file mode 100644 index 000000000..e1b641ae6 --- /dev/null +++ b/app/workers/archive_base.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +# Copyright (c) 2010-2011, Diaspora Inc. This file is +# licensed under the Affero General Public License version 3 or later. See +# the COPYRIGHT file. + +module Workers + class ArchiveBase < Base + sidekiq_options queue: :low + + include Diaspora::Logging + + def perform(*args) + if currently_running_archive_jobs >= AppConfig.settings.archive_jobs_concurrency.to_i + logger.info "Already the maximum number of parallel archive jobs running, " \ + "scheduling #{self.class}:#{args} in 5 minutes." + self.class.perform_in(5.minutes + rand(30), *args) + else + perform_archive_job(*args) + end + end + + private + + def perform_archive_job(_args) + raise NotImplementedError, "You must override perform_archive_job" + end + + def currently_running_archive_jobs + return 0 if AppConfig.environment.single_process_mode? + + Sidekiq::Workers.new.count do |process_id, thread_id, work| + !(Process.pid.to_s == process_id.split(":")[1] && Thread.current.object_id.to_s(36) == thread_id) && + ArchiveBase.subclasses.map(&:to_s).include?(work["payload"]["class"]) + end + end + end +end diff --git a/app/workers/export_user.rb b/app/workers/export_user.rb index 247b8a6e3..20afbdfcf 100644 --- a/app/workers/export_user.rb +++ b/app/workers/export_user.rb @@ -5,39 +5,17 @@ # the COPYRIGHT file. module Workers - class ExportUser < Base - sidekiq_options queue: :low - - include Diaspora::Logging - - def perform(user_id) - if currently_running_exports >= AppConfig.settings.export_concurrency.to_i - logger.info "Already the maximum number of parallel user exports running, " \ - "scheduling export for User:#{user_id} in 5 minutes." - self.class.perform_in(5.minutes + rand(30), user_id) - else - export_user(user_id) - end - end - + class ExportUser < ArchiveBase private - def export_user(user_id) - @user = User.find(user_id) - @user.perform_export! + def perform_archive_job(user_id) + user = User.find(user_id) + user.perform_export! - if @user.reload.export.present? - ExportMailer.export_complete_for(@user).deliver_now + if user.reload.export.present? + ExportMailer.export_complete_for(user).deliver_now else - ExportMailer.export_failure_for(@user).deliver_now - end - end - - def currently_running_exports - return 0 if AppConfig.environment.single_process_mode? - Sidekiq::Workers.new.count do |process_id, thread_id, work| - !(Process.pid.to_s == process_id.split(":")[1] && Thread.current.object_id.to_s(36) == thread_id) && - work["payload"]["class"] == self.class.to_s + ExportMailer.export_failure_for(user).deliver_now end end end diff --git a/app/workers/import_user.rb b/app/workers/import_user.rb index 8fb1801b3..645a26d4d 100644 --- a/app/workers/import_user.rb +++ b/app/workers/import_user.rb @@ -1,36 +1,12 @@ # frozen_string_literal: true module Workers - class ImportUser < Base - sidekiq_options queue: :low - - include Diaspora::Logging - - def perform(user_id) - if currently_running_exports >= AppConfig.settings.export_concurrency.to_i - - logger.info "Already the maximum number of parallel user imports running, " \ - "scheduling import for User:#{user_id} in 5 minutes." - self.class.perform_in(5.minutes + rand(30), user_id) - else - import_user(user_id) - end - end - + class ImportUser < ArchiveBase private - def import_user(user_id) + def perform_archive_job(user_id) user = User.find(user_id) ImportService.new.import_by_user(user) end - - def currently_running_exports - return 0 if AppConfig.environment.single_process_mode? - - Sidekiq::Workers.new.count do |process_id, thread_id, work| - !(Process.pid.to_s == process_id.split(":")[1] && Thread.current.object_id.to_s(36) == thread_id) && - work["payload"]["class"] == self.class.to_s - end - end end end diff --git a/config/defaults.yml b/config/defaults.yml index fed76531f..3b3919f00 100644 --- a/config/defaults.yml +++ b/config/defaults.yml @@ -97,7 +97,7 @@ defaults: suggest_email: typhoeus_verbose: false typhoeus_concurrency: 20 - export_concurrency: 1 + archive_jobs_concurrency: 1 username_blacklist: - 'admin' - 'administrator' diff --git a/config/diaspora.toml.example b/config/diaspora.toml.example index c6c4fbea2..6304fb3fe 100644 --- a/config/diaspora.toml.example +++ b/config/diaspora.toml.example @@ -344,10 +344,10 @@ ## of your Sidekiq workers. #typhoeus_concurrency = 20 -## Maximum number of parallel user data export jobs (default=1) -## Be careful, exports of big/old profiles can use a lot of memory, running -## many of them in parallel can be a problem for small servers. -#export_concurrency = 1 +## Maximum number of parallel user data import/export jobs (default=1) +## Be careful, imports and exports of big/old profiles can use a lot of memory, +## running many of them in parallel can be a problem for small servers. +#archive_jobs_concurrency = 1 ## Welcome Message settings [configuration.settings.welcome_message] diff --git a/spec/workers/export_user_spec.rb b/spec/workers/export_user_spec.rb index e72410149..657b801d8 100644 --- a/spec/workers/export_user_spec.rb +++ b/spec/workers/export_user_spec.rb @@ -26,7 +26,7 @@ describe Workers::ExportUser do context "concurrency" do before do AppConfig.environment.single_process_mode = false - AppConfig.settings.export_concurrency = 1 + AppConfig.settings.archive_jobs_concurrency = 1 end after :all do From 34528521f27607eb1729e3e9dd53409d9e68a6f7 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Tue, 9 Nov 2021 04:04:27 +0100 Subject: [PATCH 4/6] Allow to choose to overwrite settings and profile data --- app/services/import_service.rb | 12 +-- app/services/migration_service.rb | 11 +-- lib/archive_importer.rb | 30 +++++-- lib/tasks/accounts.rake | 40 ++++++--- spec/lib/archive_importer_spec.rb | 139 +++++++++++++++++++++++++----- 5 files changed, 179 insertions(+), 53 deletions(-) diff --git a/app/services/import_service.rb b/app/services/import_service.rb index 13ca0654a..9185ee90c 100644 --- a/app/services/import_service.rb +++ b/app/services/import_service.rb @@ -3,14 +3,14 @@ class ImportService include Diaspora::Logging - def import_by_user(user) - import_by_files(user.export.current_path, user.exported_photos_file.current_path, user.username) + def import_by_user(user, opts={}) + import_by_files(user.export.current_path, user.exported_photos_file.current_path, user.username, opts) end - def import_by_files(path_to_profile, path_to_photos, username) + def import_by_files(path_to_profile, path_to_photos, username, opts={}) if path_to_profile.present? logger.info "Import for profile #{username} at path #{path_to_profile} requested" - import_user_profile(path_to_profile, username) + import_user_profile(path_to_profile, username, opts) end user = User.find_by(username: username) @@ -25,10 +25,10 @@ class ImportService private - def import_user_profile(path_to_profile, username) + def import_user_profile(path_to_profile, username, opts) raise ArgumentError, "Profile file not found at path: #{path_to_profile}" unless File.exist?(path_to_profile) - service = MigrationService.new(path_to_profile, username) + service = MigrationService.new(path_to_profile, username, opts) logger.info "Start validating user profile #{username}" service.validate logger.info "Start importing user profile for '#{username}'" diff --git a/app/services/migration_service.rb b/app/services/migration_service.rb index 56c13a45b..0e6b0d917 100644 --- a/app/services/migration_service.rb +++ b/app/services/migration_service.rb @@ -1,12 +1,14 @@ # frozen_string_literal: true class MigrationService - attr_reader :archive_path, :new_user_name + attr_reader :archive_path, :new_user_name, :opts + delegate :errors, :warnings, to: :archive_validator - def initialize(archive_path, new_user_name) + def initialize(archive_path, new_user_name, opts={}) @archive_path = archive_path @new_user_name = new_user_name + @opts = opts end def validate @@ -40,12 +42,11 @@ class MigrationService private def find_or_create_user - archive_importer.user = User.find_by(username: new_user_name) - archive_importer.create_user(username: new_user_name, password: SecureRandom.hex) if archive_importer.user.nil? + archive_importer.find_or_create_user(username: new_user_name, password: SecureRandom.hex) end def import_archive - archive_importer.import + archive_importer.import(opts) end def run_migration diff --git a/lib/archive_importer.rb b/lib/archive_importer.rb index 1b0bab6cb..8776edca8 100644 --- a/lib/archive_importer.rb +++ b/lib/archive_importer.rb @@ -10,7 +10,7 @@ class ArchiveImporter @archive_hash = archive_hash end - def import + def import(opts={}) import_tag_followings import_aspects import_contacts @@ -19,12 +19,12 @@ class ArchiveImporter import_subscriptions import_others_relayables import_blocks + import_settings if opts.fetch(:import_settings, true) + import_profile if opts.fetch(:import_profile, true) end - def create_user(attr) - allowed_keys = %w[ - email strip_exif show_community_spotlight_in_stream language disable_mail auto_follow_back - ] + def find_or_create_user(attr) + allowed_keys = %w[email language] data = convert_keys(archive_hash["user"], allowed_keys) # setting getting_started to false as the user doesn't need to see the getting started wizard data.merge!( @@ -36,8 +36,6 @@ class ArchiveImporter } ) self.user = User.find_or_build(data) - user.show_community_spotlight_in_stream = data.fetch(:show_community_spotlight_in_stream, true) - user.strip_exif = data.fetch(:strip_exif, true) user.getting_started = false user.save! end @@ -63,7 +61,7 @@ class ArchiveImporter return if name.nil? aspect = user.aspects.find_by(name: name) - user.update(auto_follow_back_aspect: aspect) if aspect + user.update(auto_follow_back: true, auto_follow_back_aspect: aspect) if aspect end def import_aspects @@ -74,7 +72,6 @@ class ArchiveImporter logger.warn "#{self}: #{e}" end end - set_auto_follow_back_aspect end def import_posts @@ -125,6 +122,21 @@ class ArchiveImporter end end + def import_settings + allowed_keys = %w[language show_community_spotlight_in_stream strip_exif] + convert_keys(archive_hash["user"], allowed_keys).each do |key, value| + user.update(key => value) unless value.nil? + end + + set_auto_follow_back_aspect if archive_hash.fetch("user").fetch("auto_follow_back", false) + end + + def import_profile + profile_attributes.each do |key, value| + user.person.profile.update(key => value) unless value.nil? + end + end + def convert_keys(hash, allowed_keys) hash .slice(*allowed_keys) diff --git a/lib/tasks/accounts.rake b/lib/tasks/accounts.rake index 24077ec3e..f6075de3d 100644 --- a/lib/tasks/accounts.rake +++ b/lib/tasks/accounts.rake @@ -2,13 +2,16 @@ namespace :accounts do desc "Perform migration" - task :migration, %i[archive_path photos_path new_user_name] => :environment do |_t, args| - puts "Account migration is requested. You can import a profile or a photos archive or booth." - args = %i[archive_path photos_path new_user_name].map {|name| [name, args[name]] }.to_h + task :migration, + %i[archive_path photos_path new_user_name import_settings import_profile] => :environment do |_t, args| + puts "Account migration is requested. You can import a profile or a photos archive or both." + args = %i[archive_path photos_path new_user_name import_settings import_profile] + .map {|name| [name, args[name]] }.to_h process_arguments(args) start_time = Time.now.getlocal if args[:new_user_name].present? && (args[:archive_path].present? || args[:photos_path].present?) - ImportService.new.import_by_files(args[:archive_path], args[:photos_path], args[:new_user_name]) + ImportService.new.import_by_files(args[:archive_path], args[:photos_path], args[:new_user_name], + args.slice(:import_settings, :import_profile)) puts "\n Migration completed in #{Time.now.getlocal - start_time} seconds. (Photos might still be processed in)" else puts "Must set a user name and a archive file path or photos file path" @@ -19,16 +22,31 @@ namespace :accounts do args[:archive_path] = request_parameter(args[:archive_path], "Enter the archive (.json, .gz, .zip) path: ") args[:photos_path] = request_parameter(args[:photos_path], "Enter the photos (.zip) path: ") args[:new_user_name] = request_parameter(args[:new_user_name], "Enter the new user name: ") + args[:import_settings] = request_boolean_parameter(args[:import_settings], "Import and overwrite settings [Y/n]: ") + args[:import_profile] = request_boolean_parameter(args[:import_profile], "Import and overwrite profile [Y/n]: ") puts "Archive path: #{args[:archive_path]}" puts "Photos path: #{args[:photos_path]}" puts "New username: #{args[:new_user_name]}" + puts "Import settings: #{args[:import_settings]}" + puts "Import profile: #{args[:import_profile]}" + end + + def request_parameter(arg, text) + return arg unless arg.nil? + + print text + $stdin.gets.strip + end + + def request_boolean_parameter(arg, text, default: true) + return arg == "true" unless arg.nil? + + print text + response = $stdin.gets.strip.downcase + + return default if response == "" + + response[0] == "y" end end - -def request_parameter(arg, text) - return arg unless arg.nil? - - print text - $stdin.gets.strip -end diff --git a/spec/lib/archive_importer_spec.rb b/spec/lib/archive_importer_spec.rb index 6ee361067..1579371c9 100644 --- a/spec/lib/archive_importer_spec.rb +++ b/spec/lib/archive_importer_spec.rb @@ -56,17 +56,13 @@ describe ArchiveImporter do let(:archive_hash) { { "user" => { - "auto_follow_back_aspect" => "Friends", - "profile" => { + "profile" => { "entity_data" => { "author" => "old_id@old_pod.nowhere" } }, - "contact_groups" => [{ - "name" => "Friends" - }], - "followed_tags" => [target.tag_followings.first.tag.name], - "post_subscriptions" => [target.participations.first.target.guid] + "followed_tags" => [target.tag_followings.first.tag.name], + "post_subscriptions" => [target.participations.first.target.guid] } } } @@ -109,14 +105,122 @@ describe ArchiveImporter do }.not_to raise_error end end + + context "with settings" do + let(:archive_hash) { + { + "user" => { + "profile" => { + "entity_data" => { + "author" => "old_id@old_pod.nowhere" + } + }, + "contact_groups" => [{ + "name" => "Follow" + }], + "strip_exif" => false, + "show_community_spotlight_in_stream" => false, + "language" => "ru", + "auto_follow_back" => true, + "auto_follow_back_aspect" => "Follow" + } + } + } + + it "imports the settings" do + expect { + archive_importer.import + }.not_to raise_error + + expect(archive_importer.user.strip_exif).to eq(false) + expect(archive_importer.user.show_community_spotlight_in_stream).to eq(false) + expect(archive_importer.user.language).to eq("ru") + expect(archive_importer.user.auto_follow_back).to eq(true) + expect(archive_importer.user.auto_follow_back_aspect.name).to eq("Follow") + end + + it "does not overwrite settings if import_settings is disabled" do + expect { + archive_importer.import(import_settings: false) + }.not_to raise_error + + expect(archive_importer.user.strip_exif).to eq(true) + expect(archive_importer.user.show_community_spotlight_in_stream).to eq(true) + expect(archive_importer.user.language).to eq("en") + expect(archive_importer.user.auto_follow_back).to eq(false) + end + end + + context "with profile" do + let(:archive_hash) { + { + "user" => { + "profile" => { + "entity_data" => { + "author" => "old_id@old_pod.nowhere", + "first_name" => "First", + "last_name" => "Last", + "full_name" => "Full Name", + "image_url" => "https://example.com/my_avatar.png", + "bio" => "I'm just a test account", + "gender" => "Robot", + "birthday" => "2006-01-01", + "location" => "diaspora* specs", + "searchable" => false, + "public" => true, + "nsfw" => true, + "tag_string" => "#diaspora #linux #partying" + } + } + } + } + } + + it "imports the profile data" do + expect { + archive_importer.import + }.not_to raise_error + + expect(archive_importer.user.profile.first_name).to eq("First") + expect(archive_importer.user.profile.last_name).to eq("Last") + expect(archive_importer.user.profile.image_url).to eq("https://example.com/my_avatar.png") + expect(archive_importer.user.profile.bio).to eq("I'm just a test account") + expect(archive_importer.user.profile.gender).to eq("Robot") + expect(archive_importer.user.profile.birthday).to eq(Date.new(2006, 1, 1)) + expect(archive_importer.user.profile.location).to eq("diaspora* specs") + expect(archive_importer.user.profile.searchable).to eq(false) + expect(archive_importer.user.profile.public_details).to eq(true) + expect(archive_importer.user.profile.nsfw).to eq(true) + expect(archive_importer.user.profile.tag_string).to eq("#diaspora #linux #partying") + end + + it "does not overwrite profile if import_profile is disabled" do + original_profile = target.profile.dup + + expect { + archive_importer.import(import_profile: false) + }.not_to raise_error + + expect(archive_importer.user.profile.first_name).to eq(original_profile.first_name) + expect(archive_importer.user.profile.last_name).to eq(original_profile.last_name) + expect(archive_importer.user.profile.image_url).to eq(original_profile.image_url) + expect(archive_importer.user.profile.bio).to eq(original_profile.bio) + expect(archive_importer.user.profile.gender).to eq(original_profile.gender) + expect(archive_importer.user.profile.birthday).to eq(original_profile.birthday) + expect(archive_importer.user.profile.location).to eq(original_profile.location) + expect(archive_importer.user.profile.searchable).to eq(original_profile.searchable) + expect(archive_importer.user.profile.public_details).to eq(original_profile.public_details) + expect(archive_importer.user.profile.nsfw).to eq(original_profile.nsfw) + expect(archive_importer.user.profile.tag_string).to eq(original_profile.tag_string) + end + end end - describe "#create_user" do - let(:archive_importer) { ArchiveImporter.new(archive_hash) } + describe "#find_or_create_user" do let(:archive_hash) { { "user" => { - "profile" => { + "profile" => { "entity_data" => { "author" => "old_id@old_pod.nowhere", "first_name" => "First", @@ -133,26 +237,17 @@ describe ArchiveImporter do "tag_string" => "#diaspora #linux #partying" } }, - "email" => "user@example.com", - "strip_exif" => false, - "show_community_spotlight_in_stream" => false, - "language" => "ru", - "disable_mail" => false, - "auto_follow_back" => true + "email" => "user@example.com" } } } + let(:archive_importer) { ArchiveImporter.new(archive_hash) } it "creates user" do expect { - archive_importer.create_user(username: "new_name", password: "123456") + archive_importer.find_or_create_user(username: "new_name", password: "123456") }.to change(User, :count).by(1) expect(archive_importer.user.email).to eq("user@example.com") - expect(archive_importer.user.strip_exif).to eq(false) - expect(archive_importer.user.show_community_spotlight_in_stream).to eq(false) - expect(archive_importer.user.language).to eq("ru") - expect(archive_importer.user.disable_mail).to eq(false) - expect(archive_importer.user.auto_follow_back).to eq(true) expect(archive_importer.user.getting_started).to be_falsey expect(archive_importer.user.profile.first_name).to eq("First") From e9f7bf382eda5d50858bc03e2f0b832cbcebd4bd Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Tue, 23 Nov 2021 03:20:47 +0100 Subject: [PATCH 5/6] Send new remote_photo_path in migration message --- app/services/import_service.rb | 2 +- app/services/migration_service.rb | 13 ++++++++++- lib/diaspora/federation/entities.rb | 7 +++--- spec/integration/migration_service_spec.rb | 26 ++++++++++++++++++++++ 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/app/services/import_service.rb b/app/services/import_service.rb index 9185ee90c..b7805e11a 100644 --- a/app/services/import_service.rb +++ b/app/services/import_service.rb @@ -10,7 +10,7 @@ class ImportService def import_by_files(path_to_profile, path_to_photos, username, opts={}) if path_to_profile.present? logger.info "Import for profile #{username} at path #{path_to_profile} requested" - import_user_profile(path_to_profile, username, opts) + import_user_profile(path_to_profile, username, opts.merge(photo_migration: path_to_photos.present?)) end user = User.find_by(username: username) diff --git a/app/services/migration_service.rb b/app/services/migration_service.rb index 0e6b0d917..78c6c38f6 100644 --- a/app/services/migration_service.rb +++ b/app/services/migration_service.rb @@ -60,7 +60,8 @@ class MigrationService new_person: archive_importer.user.person, old_private_key: archive_importer.serialized_private_key, old_person_diaspora_id: archive_importer.archive_author_diaspora_id, - archive_contacts: archive_importer.contacts + archive_contacts: archive_importer.contacts, + remote_photo_path: remote_photo_path ) end @@ -131,6 +132,16 @@ class MigrationService File.delete(@intermediate_file) end + def remote_photo_path + return unless opts.fetch(:photo_migration, false) + + if AppConfig.environment.s3.enable? + return "https://#{AppConfig.environment.s3.bucket.get}.s3.amazonaws.com/uploads/images/" + end + + "#{AppConfig.pod_uri}uploads/images/" + end + class ArchiveValidationFailed < RuntimeError end diff --git a/lib/diaspora/federation/entities.rb b/lib/diaspora/federation/entities.rb index 4a77f8ef7..fa3425f57 100644 --- a/lib/diaspora/federation/entities.rb +++ b/lib/diaspora/federation/entities.rb @@ -26,9 +26,10 @@ module Diaspora def self.account_migration(account_migration) DiasporaFederation::Entities::AccountMigration.new( - author: account_migration.sender.diaspora_handle, - profile: profile(account_migration.new_person.profile), - signature: account_migration.signature + author: account_migration.sender.diaspora_handle, + profile: profile(account_migration.new_person.profile), + remote_photo_path: account_migration.remote_photo_path, + signature: account_migration.signature ) end diff --git a/spec/integration/migration_service_spec.rb b/spec/integration/migration_service_spec.rb index 67f8513ee..84de21a6a 100644 --- a/spec/integration/migration_service_spec.rb +++ b/spec/integration/migration_service_spec.rb @@ -312,6 +312,32 @@ describe MigrationService do end end + context "photo migration" do + it "doesn't include a new remote_photo_path" do + service = MigrationService.new(archive_file.path, new_username) + service.send(:find_or_create_user) + account_migration = service.send(:account_migration) + expect(account_migration.remote_photo_path).to be_nil + end + + it "includes url to new pod image upload folder in remote_photo_path" do + service = MigrationService.new(archive_file.path, new_username, photo_migration: true) + service.send(:find_or_create_user) + account_migration = service.send(:account_migration) + expect(account_migration.remote_photo_path).to eq("#{AppConfig.pod_uri}uploads/images/") + end + + it "includes url to S3 image upload folder in remote_photo_path when S3 is enabled" do + AppConfig.environment.s3.enable = true + AppConfig.environment.s3.bucket = "test-bucket" + + service = MigrationService.new(archive_file.path, new_username, photo_migration: true) + service.send(:find_or_create_user) + account_migration = service.send(:account_migration) + expect(account_migration.remote_photo_path).to eq("https://test-bucket.s3.amazonaws.com/uploads/images/") + end + end + context "compressed archives" do it "uncompresses gz archive" do gz_compressed_file = create_gz_archive From d3c2407df102fec656ac5c048f1f753661123100 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Wed, 24 Nov 2021 02:42:31 +0100 Subject: [PATCH 6/6] Don't overwrite photos of other users during import If a photo with the same filename already exists, generate a new random filename, and re-federate the photo with that filename. This ensures users can't modify their archive to overwrite other users photos. --- app/services/import_service.rb | 31 +++++++++---- spec/integration/import_service_spec.rb | 60 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 spec/integration/import_service_spec.rb diff --git a/app/services/import_service.rb b/app/services/import_service.rb index b7805e11a..cb6d02e3b 100644 --- a/app/services/import_service.rb +++ b/app/services/import_service.rb @@ -47,27 +47,40 @@ class ImportService uncompressed_photos_folder = unzip_photos_file(path_to_photos) user.posts.find_in_batches do |posts| - import_photos_for_posts(posts, uncompressed_photos_folder) + import_photos_for_posts(user, posts, uncompressed_photos_folder) end - FileUtils.rmdir(uncompressed_photos_folder) + FileUtils.rm_r(uncompressed_photos_folder) end - def import_photos_for_posts(posts, source_dir) + def import_photos_for_posts(user, posts, source_dir) posts.each do |post| post.photos.each do |photo| uploaded_file = "#{source_dir}/#{photo.remote_photo_name}" next unless File.exist?(uploaded_file) && photo.remote_photo_name.present? - File.open(uploaded_file) do |file| - photo.random_string = File.basename(uploaded_file, ".*") - photo.unprocessed_image = file - photo.save(touch: false) - end - photo.queue_processing_job + # Don't overwrite existing photos if they have the same filename. + # Generate a new random filename if a conflict exists and re-federate the photo to update on remote pods. + random_string = File.basename(uploaded_file, ".*") + conflicting_photo_exists = Photo.where.not(id: photo.id).exists?(random_string: random_string) + random_string = SecureRandom.hex(10) if conflicting_photo_exists + + store_and_process_photo(photo, uploaded_file, random_string) + + Diaspora::Federation::Dispatcher.build(user, photo).dispatch if conflicting_photo_exists end end end + def store_and_process_photo(photo, uploaded_file, random_string) + File.open(uploaded_file) do |file| + photo.random_string = random_string + photo.unprocessed_image.store! file + photo.update_remote_path + photo.save(touch: false) + end + photo.queue_processing_job + end + def unzip_photos_file(photo_file_path) folder = create_folder(photo_file_path) Zip::File.open(photo_file_path) do |zip_file| diff --git a/spec/integration/import_service_spec.rb b/spec/integration/import_service_spec.rb new file mode 100644 index 000000000..878249d6c --- /dev/null +++ b/spec/integration/import_service_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +describe ImportService do + context "import photos archive" do + let(:user) { FactoryBot.create(:user) } + let(:photo) { FactoryBot.create(:status_message_with_photo, author: user.person).photos.first } + let(:photo_archive) { + user.perform_export_photos! + photo_archive = user.exported_photos_file + + # cleanup photo after creating the archive, so it's like it was imported from a remote pod + photo.unprocessed_image = nil + photo.random_string = nil + photo.remote_photo_path = "https://old.example.com/uploads/images/" + photo.save + + photo_archive + } + + it "imports the photo with the same name" do + old_random_string = photo.random_string + + inlined_jobs { ImportService.new.import_by_files(nil, photo_archive.current_path, user.username) } + + imported_photo = photo.reload + expect(imported_photo.random_string).to include(old_random_string) + expect(imported_photo.unprocessed_image.path).to include(old_random_string) + expect(imported_photo.processed_image.path).to include(old_random_string) + expect(imported_photo.remote_photo_name).to include(old_random_string) + expect(imported_photo.remote_photo_path).to eq("#{AppConfig.pod_uri}uploads/images/") + end + + it "imports the photo with a new random name if a conflicting photo already exists" do + old_random_string = photo.random_string + photo_archive_path = photo_archive.current_path + + sm = FactoryBot.create(:status_message) + FactoryBot.create(:photo, author: sm.author, status_message: sm, random_string: old_random_string) + + expect(Diaspora::Federation::Dispatcher).to receive(:build) do |user_param, photo_param| + expect(user_param).to eq(user) + expect(photo_param.id).to eq(photo.id) + + dispatcher = double + expect(dispatcher).to receive(:dispatch) + dispatcher + end + + inlined_jobs { ImportService.new.import_by_files(nil, photo_archive_path, user.username) } + + imported_photo = photo.reload + new_random_string = imported_photo.random_string + expect(new_random_string).not_to include(old_random_string) + expect(imported_photo.unprocessed_image.path).to include(new_random_string) + expect(imported_photo.processed_image.path).to include(new_random_string) + expect(imported_photo.remote_photo_name).to include(new_random_string) + expect(imported_photo.remote_photo_path).to eq("#{AppConfig.pod_uri}uploads/images/") + end + end +end