diff --git a/app/jobs/account/data_import_job.rb b/app/jobs/account/data_import_job.rb index 67862d4c71..a41ba05436 100644 --- a/app/jobs/account/data_import_job.rb +++ b/app/jobs/account/data_import_job.rb @@ -8,13 +8,13 @@ def perform(import) step :check do |step| import.check \ start: step.cursor, - callback: ->(record_set:, file:) { step.set!([ record_set.model.name, file ]) } + callback: ->(record_set:, file:) { step.set!([ record_set.cursor_key, file ]) } end step :process do |step| import.process \ start: step.cursor, - callback: ->(record_set:, files:) { step.set!([ record_set.model.name, files.last ]) } + callback: ->(record_set:, files:) { step.set!([ record_set.cursor_key, files.last ]) } end end end diff --git a/app/models/account/data_transfer/action_text/rich_text_record_set.rb b/app/models/account/data_transfer/action_text/rich_text_record_set.rb index 01e046dbe2..57ec42e463 100644 --- a/app/models/account/data_transfer/action_text/rich_text_record_set.rb +++ b/app/models/account/data_transfer/action_text/rich_text_record_set.rb @@ -51,7 +51,12 @@ def check_record(file_path) raise IntegrityError, "#{file_path} is missing required fields: #{missing.join(', ')}" end + if ::ActionText::RichText.exists?(id: data["id"]) + raise ConflictError, "ActionTextRichText record with ID #{data['id']} already exists" + end + check_associations_dont_exist(data) + check_unique_values_arent_duplicated(data) end def transform_body_for_export(content) diff --git a/app/models/account/data_transfer/active_storage/file_record_set.rb b/app/models/account/data_transfer/active_storage/file_record_set.rb index f64f53d3a3..687cac6f19 100644 --- a/app/models/account/data_transfer/active_storage/file_record_set.rb +++ b/app/models/account/data_transfer/active_storage/file_record_set.rb @@ -21,7 +21,7 @@ def export_record(blob) end def files - zip.glob("storage/*") + zip.glob("storage/**/*") end def import_batch(files) diff --git a/app/models/account/data_transfer/board/publication_record_set.rb b/app/models/account/data_transfer/board/publication_record_set.rb new file mode 100644 index 0000000000..241ff519b7 --- /dev/null +++ b/app/models/account/data_transfer/board/publication_record_set.rb @@ -0,0 +1,15 @@ +class Account::DataTransfer::Board::PublicationRecordSet < Account::DataTransfer::RecordSet + def initialize(account) + super(account: account, model: ::Board::Publication) + end + + private + def check_record(file_path) + super + + data = load(file_path) + if !data["key"].nil? && model.exists?(key: data["key"]) + raise ConflictError, "#{model} record with key #{data['key']} already exists" + end + end +end diff --git a/app/models/account/data_transfer/manifest.rb b/app/models/account/data_transfer/manifest.rb index 11aaf6a49d..e2e6fe703d 100644 --- a/app/models/account/data_transfer/manifest.rb +++ b/app/models/account/data_transfer/manifest.rb @@ -9,16 +9,18 @@ def each_record_set(start: nil) raise ArgumentError, "No block given" unless block_given? started = start.nil? - record_class, last_id = start if start + cursor_key, last_id = start if start record_sets.each do |record_set| if started yield record_set - elsif record_set.model.name == record_class + elsif record_set.cursor_key == cursor_key started = true yield record_set, last_id end end + + raise ArgumentError, "Unknown record set cursor: #{cursor_key}" unless started end private @@ -33,8 +35,8 @@ def record_sets ::Column ), Account::DataTransfer::EntropyRecordSet.new(account), + Account::DataTransfer::Board::PublicationRecordSet.new(account), *record_sets_for( - ::Board::Publication, ::Webhook, ::Access, ::Card, diff --git a/app/models/account/data_transfer/record_set.rb b/app/models/account/data_transfer/record_set.rb index c46c348961..ac7a8e4f71 100644 --- a/app/models/account/data_transfer/record_set.rb +++ b/app/models/account/data_transfer/record_set.rb @@ -15,6 +15,10 @@ def initialize(account:, model:, attributes: nil, importable_model_names: nil) @importable_model_names = importable_model_names || [ model.name ] end + def cursor_key + "#{self.class.name}/#{model.name}" + end + def export(to:, start: nil) with_zip(to) do block = lambda do |record| @@ -40,6 +44,7 @@ def import(from:, start: nil, callback: nil) def check(from:, start: nil, callback: nil) with_zip(from) do file_list = files + check_file_names_arent_ambiguous(file_list) file_list = skip_to(file_list, start) if start file_list.each do |file_path| @@ -47,6 +52,8 @@ def check(from:, start: nil, callback: nil) callback&.call(record_set: self, file: file_path) end end + ensure + clear_duplicate_detector end private @@ -83,6 +90,14 @@ def import_batch(files) model.insert_all!(batch_data) end + def check_file_names_arent_ambiguous(file_list) + duplicates = file_list.map(&:downcase).tally.filter_map { |name, count| name if count > 1 } + + if duplicates.any? + raise IntegrityError, "#{model} has multiple files for the same record: #{duplicates.join(', ')}" + end + end + def check_record(file_path) data = load(file_path) expected_id = File.basename(file_path, ".json") @@ -101,6 +116,7 @@ def check_record(file_path) end check_associations_dont_exist(data) + check_unique_values_arent_duplicated(data) end def check_associations_dont_exist(data) @@ -134,6 +150,30 @@ def verify_model_type(type_name) end end + def check_unique_values_arent_duplicated(data) + # Remap the data being imported to the account we are importing it to. + # This ensures that uniquness checks are valid, else someone could change + # the account_id in the export to point to different accounts and circumvent + # this check. + data = data.merge("account_id" => account.id) + + if columns = duplicate_detector.detect(data) + raise IntegrityError, "#{model} has multiple records with the same #{columns.to_sentence}: #{data.values_at(*columns).join(', ')}" + end + end + + def duplicate_detector + @duplicate_detector ||= DuplicateDetector.new(unique_key_sets) + end + + def clear_duplicate_detector + @duplicate_detector = nil + end + + def unique_key_sets + @unique_key_sets ||= model.connection.indexes(model.table_name).select(&:unique).map(&:columns) + end + def skip_to(file_list, last_id) index = file_list.index(last_id) diff --git a/app/models/account/data_transfer/record_set/duplicate_detector.rb b/app/models/account/data_transfer/record_set/duplicate_detector.rb new file mode 100644 index 0000000000..e5052fabf8 --- /dev/null +++ b/app/models/account/data_transfer/record_set/duplicate_detector.rb @@ -0,0 +1,27 @@ +class Account::DataTransfer::RecordSet::DuplicateDetector + def initialize(key_sets) + @key_sets = key_sets + @seen = Hash.new { |seen, columns| seen[columns] = Set.new } + end + + def detect(data) + key_sets.find do |columns| + values = data.values_at(*columns) + values.none?(&:nil?) && !seen[columns].add?(digest(values)) + end + end + + private + attr_reader :key_sets, :seen + + # NOTE: The digest is here to avoid storing whole row values in memory, which could be quite large. + # (remeber, the value is user controlled, someone could craft a malicious import to baloon memory) + # This is an improvement, but it still could be a problem if the number of unique values is very large. + # In that case, we may need to use a more memory-efficient approach, such as storing the digests in a + # temporary file, or we could switch to a Bloom filter, or a Merkel tree. + # MySQL's default collation compares strings case-insensitively, and UUID columns + # cast their values through base36, so values differing only in case are duplicates. + def digest(values) + Digest::SHA256.digest(values.map { |value| value.is_a?(String) ? value.downcase : value }.to_json) + end +end diff --git a/app/models/account/data_transfer/user_record_set.rb b/app/models/account/data_transfer/user_record_set.rb index e7ac41c568..724487b0ce 100644 --- a/app/models/account/data_transfer/user_record_set.rb +++ b/app/models/account/data_transfer/user_record_set.rb @@ -57,5 +57,19 @@ def check_record(file_path) unless (ATTRIBUTES - data.keys).empty? raise IntegrityError, "#{file_path} is missing required fields" end + + if User.exists?(id: data["id"]) + raise ConflictError, "User record with ID #{data['id']} already exists" + end + + check_unique_values_arent_duplicated data.merge("email_address" => normalized_email_address(data)) + end + + def normalized_email_address(data) + Identity.normalize_value_for(:email_address, data["email_address"]) + end + + def unique_key_sets + super + [ %w[ email_address ] ] end end diff --git a/app/models/account/import.rb b/app/models/account/import.rb index 2bf1b88e7b..e2d3f10541 100644 --- a/app/models/account/import.rb +++ b/app/models/account/import.rb @@ -23,8 +23,9 @@ def check(start: nil, callback: nil) processing! ZipFile.read_from(file.blob) do |zip| - Account::DataTransfer::Manifest.new(account).each_record_set(start: start) do |record_set, last_id| - record_set.check(from: zip, start: last_id, callback: callback) + Account::DataTransfer::Manifest.new(account).each_record_set(start: start) do |record_set| + # The last_id is ignored so that we can check for record uniquness when interrupted + record_set.check(from: zip, callback: callback) end end rescue Account::DataTransfer::RecordSet::ConflictError => e diff --git a/app/models/zip_file/reader.rb b/app/models/zip_file/reader.rb index 12ddd6fae2..fcf9ef78cc 100644 --- a/app/models/zip_file/reader.rb +++ b/app/models/zip_file/reader.rb @@ -2,6 +2,8 @@ class ZipFile::Reader def initialize(io) @io = io @reader = ZipKit::FileReader.read_zip_structure(io: io) + + ensure_unique_entry_names rescue ZipKit::FileReader::ReadError, ZipKit::FileReader::MissingEOCD, ZipKit::FileReader::UnsupportedFeature => e raise ZipFile::InvalidFileError, e.message end @@ -19,10 +21,21 @@ def read(file_path) end def glob(pattern) - @reader.map(&:filename).select { |name| File.fnmatch(pattern, name) }.sort + @reader.map(&:filename).select { |name| File.fnmatch(pattern, name, File::FNM_PATHNAME) }.sort end def exists?(file_path) @reader.any? { |e| e.filename == file_path } end + + private + # ZIP files can technically contian duplicate entries in their manifest. + # But that doesn't produce a valid filesystem, so we flat-out reject them here. + def ensure_unique_entry_names + duplicates = @reader.map(&:filename).tally.filter_map { |name, count| name if count > 1 } + + if duplicates.any? + raise ZipFile::InvalidFileError, "Duplicate entries in zip: #{duplicates.join(', ')}" + end + end end diff --git a/test/models/account/data_transfer/manifest_test.rb b/test/models/account/data_transfer/manifest_test.rb new file mode 100644 index 0000000000..719e6b15e4 --- /dev/null +++ b/test/models/account/data_transfer/manifest_test.rb @@ -0,0 +1,40 @@ +require "test_helper" + +class Account::DataTransfer::ManifestTest < ActiveSupport::TestCase + setup do + @manifest = Account::DataTransfer::Manifest.new(accounts("37s")) + end + + test "each_record_set resumes at the record set identified by the cursor" do + file_record_set = record_sets.find { |record_set| record_set.is_a?(Account::DataTransfer::ActiveStorage::FileRecordSet) } + + yielded = [] + @manifest.each_record_set(start: [ file_record_set.cursor_key, "storage/somekey" ]) do |record_set, last_id| + yielded << record_set + end + + assert_equal 1, yielded.size + assert_instance_of Account::DataTransfer::ActiveStorage::FileRecordSet, yielded.first + end + + test "each_record_set distinguishes record sets sharing a model" do + blob_record_set = record_sets.find { |record_set| record_set.is_a?(Account::DataTransfer::ActiveStorage::BlobRecordSet) } + file_record_set = record_sets.find { |record_set| record_set.is_a?(Account::DataTransfer::ActiveStorage::FileRecordSet) } + + assert_equal blob_record_set.model, file_record_set.model + assert_not_equal blob_record_set.cursor_key, file_record_set.cursor_key + end + + test "each_record_set rejects a cursor that matches no record set" do + assert_raises(ArgumentError) do + @manifest.each_record_set(start: [ "Nonexistent/RecordSet", "somefile" ]) { } + end + end + + private + def record_sets + [].tap do |sets| + @manifest.each_record_set { |record_set| sets << record_set } + end + end +end diff --git a/test/models/account/data_transfer/record_set_test.rb b/test/models/account/data_transfer/record_set_test.rb index 8c8d75e5ef..13c31947b5 100644 --- a/test/models/account/data_transfer/record_set_test.rb +++ b/test/models/account/data_transfer/record_set_test.rb @@ -41,6 +41,154 @@ class Account::DataTransfer::RecordSetTest < ActiveSupport::TestCase assert_match(/unrecognized.*type/i, error.message) end + test "check rejects board publication with a key that already exists" do + existing_publication = boards(:writebook).publish + + record_set = Account::DataTransfer::Board::PublicationRecordSet.new(importing_account) + + error = assert_raises(Account::DataTransfer::RecordSet::ConflictError) do + record_set.check(from: build_reader(dir: "board_publications", data: build_publication_data(key: existing_publication.key))) + end + + assert_match(/key.*already exists/i, error.message) + end + + test "check accepts board publication with a new key" do + record_set = Account::DataTransfer::Board::PublicationRecordSet.new(importing_account) + + assert_nothing_raised do + record_set.check(from: build_reader(dir: "board_publications", data: build_publication_data(key: "brand-new-key"))) + end + end + + test "check rejects records that duplicate each other's unique values" do + closures = [ + build_closure_data(id: "test_closure_id_1234567890123", card_id: "nonexistent_card_id_123456789"), + build_closure_data(id: "test_closure_id_1234567890124", card_id: "nonexistent_card_id_123456789") + ] + + record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Closure) + + error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + record_set.check(from: build_reader(dir: "closures", data: closures)) + end + + assert_match(/multiple records with the same card_id/i, error.message) + end + + test "check accepts records with distinct unique values" do + closures = [ + build_closure_data(id: "test_closure_id_1234567890123", card_id: "nonexistent_card_id_123456789"), + build_closure_data(id: "test_closure_id_1234567890124", card_id: "nonexistent_card_id_123456780") + ] + + record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Closure) + + assert_nothing_raised do + record_set.check(from: build_reader(dir: "closures", data: closures)) + end + end + + test "check rejects records whose unique values collide only under the target account" do + tags = [ + build_tag_data(id: "test_tag_id_12345678901234567", account_id: "nonexistent_account_a_12345678", title: "Urgent"), + build_tag_data(id: "test_tag_id_12345678901234568", account_id: "nonexistent_account_b_12345678", title: "Urgent") + ] + + record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Tag) + + error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + record_set.check(from: build_reader(dir: "tags", data: tags)) + end + + assert_match(/multiple records with the same account_id and title/i, error.message) + end + + test "check rejects unique values that differ only in case" do + tags = [ + build_tag_data(id: "test_tag_id_12345678901234567", title: "Urgent"), + build_tag_data(id: "test_tag_id_12345678901234568", title: "URGENT") + ] + + record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Tag) + + error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + record_set.check(from: build_reader(dir: "tags", data: tags)) + end + + assert_match(/multiple records with the same account_id and title/i, error.message) + end + + test "record sets track the model's unique indexes but not the primary key" do + record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Closure) + + assert_includes record_set.send(:unique_key_sets), [ "card_id" ] + assert_not_includes record_set.send(:unique_key_sets), [ "id" ] + end + + test "check rejects file names differing only in case" do + closures = [ + build_closure_data(id: "test_closure_id_123456789012a", card_id: "nonexistent_card_id_123456789"), + build_closure_data(id: "test_closure_id_123456789012A", card_id: "nonexistent_card_id_123456780") + ] + + record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Closure) + + error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + record_set.check(from: build_reader(dir: "closures", data: closures)) + end + + assert_match(/multiple files for the same record/i, error.message) + end + + test "check rejects users that share an email address" do + users = [ + build_user_data(id: "test_user_id_1234567890123456", email_address: "dupe@example.com"), + build_user_data(id: "test_user_id_1234567890123457", email_address: "dupe@example.com") + ] + + record_set = Account::DataTransfer::UserRecordSet.new(importing_account) + + error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + record_set.check(from: build_reader(dir: "users", data: users)) + end + + assert_match(/multiple records with the same email_address/i, error.message) + end + + test "check rejects users whose email addresses normalize to the same identity" do + users = [ + build_user_data(id: "test_user_id_1234567890123456", email_address: "Dupe@Example.com "), + build_user_data(id: "test_user_id_1234567890123457", email_address: "dupe@example.com") + ] + + record_set = Account::DataTransfer::UserRecordSet.new(importing_account) + + error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + record_set.check(from: build_reader(dir: "users", data: users)) + end + + assert_match(/multiple records with the same email_address/i, error.message) + end + + test "check rejects a user whose ID already exists" do + user_data = build_user_data(id: users(:david).id, email_address: "someone@example.com") + + record_set = Account::DataTransfer::UserRecordSet.new(importing_account) + + assert_raises(Account::DataTransfer::RecordSet::ConflictError) do + record_set.check(from: build_reader(dir: "users", data: user_data)) + end + end + + test "check accepts a board publication without a key" do + record_set = Account::DataTransfer::Board::PublicationRecordSet.new(importing_account) + + assert_nothing_raised do + record_set.check(from: build_reader(dir: "board_publications", data: build_publication_data(key: nil))) + end + end + test "check accepts polymorphic type in the importable models allowlist" do event_data = build_event_data(eventable_type: "Card") @@ -71,12 +219,59 @@ def build_event_data(eventable_type:) } end + def build_tag_data(id:, title:, account_id: "nonexistent_account_id_1234567") + { + "id" => id, + "account_id" => account_id, + "title" => title, + "created_at" => Time.current.iso8601, + "updated_at" => Time.current.iso8601 + } + end + + def build_closure_data(id:, card_id:) + { + "id" => id, + "account_id" => "nonexistent_account_id_1234567", + "card_id" => card_id, + "user_id" => "nonexistent_user_id_123456789", + "created_at" => Time.current.iso8601, + "updated_at" => Time.current.iso8601 + } + end + + def build_user_data(id:, email_address:) + { + "id" => id, + "email_address" => email_address, + "name" => "Imported User", + "role" => "member", + "active" => true, + "verified_at" => Time.current.iso8601, + "created_at" => Time.current.iso8601, + "updated_at" => Time.current.iso8601 + } + end + + def build_publication_data(key:) + { + "id" => "test_publication_id_123456789", + "account_id" => "nonexistent_account_id_1234567", + "board_id" => "nonexistent_board_id_12345678", + "key" => key, + "created_at" => Time.current.iso8601, + "updated_at" => Time.current.iso8601 + } + end + def build_reader(dir:, data:) tempfile = Tempfile.new([ "import_test", ".zip" ]) tempfile.binmode writer = ZipFile::Writer.new(tempfile) - writer.add_file("data/#{dir}/#{data['id']}.json", data.to_json) + Array.wrap(data).each do |record| + writer.add_file("data/#{dir}/#{record['id']}.json", record.to_json) + end writer.close tempfile.rewind diff --git a/test/models/zip_file_test.rb b/test/models/zip_file_test.rb index 3484f9f923..efcaadaab6 100644 --- a/test/models/zip_file_test.rb +++ b/test/models/zip_file_test.rb @@ -119,6 +119,31 @@ class ZipFileTest < ActiveSupport::TestCase end end + test "reader raises InvalidFileError for duplicate entry names" do + tempfile = create_test_zip("data/a.json" => "{}", "data/b.json" => "{}") + + # Our writer refuses to produce duplicate entry names, so forge them by renaming + # the second entry onto the first in both its local header and the central directory. + forged = StringIO.new(tempfile.read.gsub("data/b.json", "data/a.json")) + + error = assert_raises(ZipFile::InvalidFileError) { ZipFile::Reader.new(forged) } + assert_match(/duplicate entries/i, error.message) + ensure + tempfile&.close + tempfile&.unlink + end + + test "reader glob does not match nested paths" do + tempfile = create_test_zip("data/a.json" => "{}", "data/nested/b.json" => "{}") + + reader = ZipFile::Reader.new(tempfile) + + assert_equal [ "data/a.json" ], reader.glob("data/*.json") + ensure + tempfile&.close + tempfile&.unlink + end + test "reader raises InvalidFileError for non-zip file" do tempfile = Tempfile.new([ "not_a_zip", ".zip" ]) tempfile.write("this is not a zip file at all")