From 50215b0b8eab37e4c95698762811ba54ba3d92ae Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 10 Jan 2018 11:55:02 +0100 Subject: [PATCH] Fixed issue #1663 - Sequencer ExternalSync lookup and storage is affected by ID collisions. --- app/models/external_sync.rb | 4 ++ .../common/model/attributes/remote_id.rb | 2 +- .../common/model/external_sync/integrity.rb | 10 ++- .../common/model/lookup/external_sync.rb | 54 +++++++++++++-- .../unit/import/ldap/users/lost/ids.rb | 18 ++--- spec/lib/external_sync_spec.rb | 21 ++++++ .../common/model/attributes/remote_id_spec.rb | 5 +- .../common/model/lookup/external_sync_spec.rb | 68 ++++++++++++++++++- 8 files changed, 160 insertions(+), 22 deletions(-) diff --git a/app/models/external_sync.rb b/app/models/external_sync.rb index 1eff33857..bb4fdf1b8 100644 --- a/app/models/external_sync.rb +++ b/app/models/external_sync.rb @@ -5,6 +5,10 @@ class ExternalSync < ApplicationModel class << self + def sanitized_source_id(source_id) + Digest::SHA2.hexdigest(source_id) + end + def changed?(object:, previous_changes: {}, current_changes:) changed = false previous_changes ||= {} diff --git a/lib/sequencer/unit/import/common/model/attributes/remote_id.rb b/lib/sequencer/unit/import/common/model/attributes/remote_id.rb index ca2dae12c..00b263de3 100644 --- a/lib/sequencer/unit/import/common/model/attributes/remote_id.rb +++ b/lib/sequencer/unit/import/common/model/attributes/remote_id.rb @@ -12,7 +12,7 @@ class Sequencer def process state.provide(:remote_id) do - resource.fetch(attribute).dup.to_s.downcase + resource.fetch(attribute).dup.to_s end rescue KeyError => e handle_failure(e) diff --git a/lib/sequencer/unit/import/common/model/external_sync/integrity.rb b/lib/sequencer/unit/import/common/model/external_sync/integrity.rb index ffc7535b7..ed7e80be1 100644 --- a/lib/sequencer/unit/import/common/model/external_sync/integrity.rb +++ b/lib/sequencer/unit/import/common/model/external_sync/integrity.rb @@ -19,8 +19,8 @@ class Sequencer def up_to_date? return false if entry.blank? - return true if entry.source_id == remote_id - entry.update!(source_id: remote_id) + return true if entry.source_id == sanitized_remote_id + entry.update!(source_id: sanitized_remote_id) true end @@ -37,11 +37,15 @@ class Sequencer def create ::ExternalSync.create( source: external_sync_source, - source_id: remote_id, + source_id: sanitized_remote_id, object: model_class.name, o_id: instance.id ) end + + def sanitized_remote_id + @sanitized_remote_id ||= ::ExternalSync.sanitized_source_id(remote_id) + end end end end diff --git a/lib/sequencer/unit/import/common/model/lookup/external_sync.rb b/lib/sequencer/unit/import/common/model/lookup/external_sync.rb index 7ea7a2d8e..5d8cea80a 100644 --- a/lib/sequencer/unit/import/common/model/lookup/external_sync.rb +++ b/lib/sequencer/unit/import/common/model/lookup/external_sync.rb @@ -14,11 +14,6 @@ class Sequencer provides :instance def process - synced_instance = ::ExternalSync.find_by( - source: external_sync_source, - source_id: remote_id, - object: model_class.name, - ) return if !synced_instance state.provide(:instance) do @@ -27,6 +22,55 @@ class Sequencer rescue => e handle_failure(e) end + + private + + def synced_instance + @synced_instance ||= correct_entry || corrected_entry + end + + def correct_entry + ::ExternalSync.find_by( + source: external_sync_source, + source_id: sanitized_remote_id, + object: model_class.name, + ) + end + + def sanitized_remote_id + @sanitized_remote_id ||= ::ExternalSync.sanitized_source_id(remote_id) + end + + def corrected_entry + return if obsolete_entry.blank? + obsolete_entry.update!(source_id: sanitized_remote_id) + obsolete_entry + end + + def obsolete_entry + @obsolete_entry ||= begin + if Rails.application.config.db_case_sensitive + case_sensitive_entry + else + case_insensitive_entry + end + end + end + + def case_sensitive_entry + ::ExternalSync.where( + source: external_sync_source, + object: model_class.name, + ).where('LOWER(source_id) = LOWER(?)', remote_id).first + end + + def case_insensitive_entry + ::ExternalSync.find_by( + source: external_sync_source, + source_id: remote_id, + object: model_class.name, + ) + end end end end diff --git a/lib/sequencer/unit/import/ldap/users/lost/ids.rb b/lib/sequencer/unit/import/ldap/users/lost/ids.rb index 94a3ed9f5..9a9b9e086 100644 --- a/lib/sequencer/unit/import/ldap/users/lost/ids.rb +++ b/lib/sequencer/unit/import/ldap/users/lost/ids.rb @@ -13,15 +13,15 @@ class Sequencer end def active_ids - ExternalSync.joins('INNER JOIN users ON (users.id = external_syncs.o_id)') - .where( - source: external_sync_source, - object: model_class.name, - users: { - active: true - } - ) - .pluck(:o_id) + ::ExternalSync.joins('INNER JOIN users ON (users.id = external_syncs.o_id)') + .where( + source: external_sync_source, + object: model_class.name, + users: { + active: true + } + ) + .pluck(:o_id) end end end diff --git a/spec/lib/external_sync_spec.rb b/spec/lib/external_sync_spec.rb index 14f4b4cd0..aa9671266 100644 --- a/spec/lib/external_sync_spec.rb +++ b/spec/lib/external_sync_spec.rb @@ -2,6 +2,27 @@ require 'rails_helper' RSpec.describe ExternalSync do + context '#sanitized_source_id' do + + let(:source_id) { 'AbCdEfG124' } + + it 'sanitizes source ids' do + sanitized_source_id = described_class.sanitized_source_id(source_id) + expect(sanitized_source_id).to_not eq(source_id) + end + + it 'returns case insenstive value' do + sanitized_source_id = described_class.sanitized_source_id(source_id) + expect(sanitized_source_id).to eq(sanitized_source_id.downcase) + end + + it 'avoids case sensitive collitions' do + sanitized_source_id = described_class.sanitized_source_id(source_id) + sanitized_source_id_downcased = described_class.sanitized_source_id(source_id.downcase) + expect(sanitized_source_id).to_not eq(sanitized_source_id_downcased) + end + end + context '#changed?' do it 'keeps ActiveRecord instance unchanged on local but no remote changes' do diff --git a/spec/lib/sequencer/unit/import/common/model/attributes/remote_id_spec.rb b/spec/lib/sequencer/unit/import/common/model/attributes/remote_id_spec.rb index b82648474..130165da8 100644 --- a/spec/lib/sequencer/unit/import/common/model/attributes/remote_id_spec.rb +++ b/spec/lib/sequencer/unit/import/common/model/attributes/remote_id_spec.rb @@ -38,9 +38,10 @@ RSpec.describe Sequencer::Unit::Import::Common::Model::Attributes::RemoteId, seq provided = process(parameters) expect(provided).to include(remote_id: '1337') + expect(provided[:remote_id]).to be_a(String) end - it 'downcases the value to prevent case sensivity issues with the ORM' do + it 'does not change the value to prevent id collision issues' do parameters = { resource: { id: 'AbCdEfG', @@ -49,7 +50,7 @@ RSpec.describe Sequencer::Unit::Import::Common::Model::Attributes::RemoteId, seq provided = process(parameters) - expect(provided[:remote_id]).to eq(parameters[:resource][:id].downcase) + expect(provided[:remote_id]).to eq(parameters[:resource][:id]) end it 'duplicates the value to prevent attribute changes' do diff --git a/spec/lib/sequencer/unit/import/common/model/lookup/external_sync_spec.rb b/spec/lib/sequencer/unit/import/common/model/lookup/external_sync_spec.rb index 087ee215b..e2124e161 100644 --- a/spec/lib/sequencer/unit/import/common/model/lookup/external_sync_spec.rb +++ b/spec/lib/sequencer/unit/import/common/model/lookup/external_sync_spec.rb @@ -2,14 +2,14 @@ require 'rails_helper' RSpec.describe Sequencer::Unit::Import::Common::Model::Lookup::ExternalSync, sequencer: :unit do - it 'finds model_class instances by remote_id' do + it 'finds model_class instance by remote_id' do user = create(:user) external_sync_source = 'test' remote_id = '1337' ExternalSync.create( source: external_sync_source, - source_id: remote_id, + source_id: ExternalSync.sanitized_source_id(remote_id), o_id: user.id, object: user.class, ) @@ -22,4 +22,68 @@ RSpec.describe Sequencer::Unit::Import::Common::Model::Lookup::ExternalSync, seq expect(provided[:instance]).to eq(user) end + + context 'obsolete plain remote_id' do + + let(:user) { create(:user) } + let(:external_sync_source) { 'test' } + let(:remote_id) { 'AbCdEfG' } + + it 'finds model_class instance' do + ExternalSync.create( + source: external_sync_source, + source_id: remote_id, + o_id: user.id, + object: user.class, + ) + + provided = process( + remote_id: remote_id, + model_class: user.class, + external_sync_source: external_sync_source, + ) + + expect(provided[:instance]).to eq(user) + end + + it 'corrects external sync entry' do + entry = ExternalSync.create( + source: external_sync_source, + source_id: remote_id, + o_id: user.id, + object: user.class, + ) + + process( + remote_id: remote_id, + model_class: user.class, + external_sync_source: external_sync_source, + ) + + entry.reload + + expect(entry.source_id).to eq(ExternalSync.sanitized_source_id(remote_id)) + end + + it 'operates case agnostic' do + entry = ExternalSync.create( + source: external_sync_source, + source_id: remote_id.downcase, + o_id: user.id, + object: user.class, + ) + + provided = process( + remote_id: remote_id, + model_class: user.class, + external_sync_source: external_sync_source, + ) + + expect(provided[:instance]).to eq(user) + + entry.reload + + expect(entry.source_id).to eq(ExternalSync.sanitized_source_id(remote_id)) + end + end end