From 9bc248cf6b6b9a66c4113ccf60bc4f7f2e904fe8 Mon Sep 17 00:00:00 2001 From: Evan Battaglia Date: Sat, 12 Jun 2021 18:56:24 -0400 Subject: [PATCH] MSFT Sync: fix UserMappings race condition Fix sort of a race condition that happens when the User changes tenant or login_attribute while the job is running. In this case, the job may look up the AADs with the wrong tenant or login_attribute. This prevents us from writing those incorrect values to the DB. closes INTEROP-6721 flag=microsoft_group_enrollments_syncing Test plan: - add a 'byebug' right before 'result_hash' in GraphServiceHelpers.users_upns_to_aads - clear out UserMappings (MicrosoftSync::UserMapping.delete_all) and run a sync - when the breakpoint hits, in a console modify the Account settings' microsoft_sync_tenant. Make sure to save the changes to the database. - continue sync - sync should fail with AccountSettingsChanged. check that the UserMapping table is still empty. - Go to the course settings integration tab and you should see that error. - remove the byebug and add a byebug in SyncerSteps#ensure_user_mappings after 'users_and_upns = users_upns_finder.call' - repeat steps above for microsoft_sync_login_attribute instead of tenant - remove byebugs, clear out UserMappings, and run a sync just to make sure it still works Change-Id: I4fad4722279bb44bf6ddd7e608deac3607dc97a2 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/267004 Tested-by: Service Cloud Jenkins Reviewed-by: Ryan Hawkins Reviewed-by: Mysti Lilla QA-Review: Ryan Hawkins Product-Review: Evan Battaglia --- app/models/microsoft_sync/user_mapping.rb | 35 ++++++-- lib/microsoft_sync/syncer_steps.rb | 5 +- spec/lib/microsoft_sync/syncer_steps_spec.rb | 39 ++++++++ .../microsoft_sync/user_mapping_spec.rb | 88 +++++++++++++++---- 4 files changed, 141 insertions(+), 26 deletions(-) diff --git a/app/models/microsoft_sync/user_mapping.rb b/app/models/microsoft_sync/user_mapping.rb index ff9b6692948..c1815dc531a 100644 --- a/app/models/microsoft_sync/user_mapping.rb +++ b/app/models/microsoft_sync/user_mapping.rb @@ -41,6 +41,10 @@ class MicrosoftSync::UserMapping < ActiveRecord::Base validates_uniqueness_of :user_id, scope: :root_account MAX_ENROLLMENT_MEMBERS = MicrosoftSync::MembershipDiff::MAX_ENROLLMENT_MEMBERS + class AccountSettingsChanged < StandardError + include MicrosoftSync::Errors::GracefulCancelErrorMixin + end + # Get the IDs of users enrolled in a course which do not have UserMappings # for the Course's root account. Works in batches, yielding arrays of user ids. def self.find_enrolled_user_ids_without_mappings(course:, batch_size:, &blk) @@ -69,21 +73,40 @@ class MicrosoftSync::UserMapping < ActiveRecord::Base # user1.id => 'aad1', user1.id => 'aad2') # Uses Rails 6's insert_all, which unlike our bulk_insert(), ignores # duplicates. (Don't need the partition support that bulk_insert provides.) - def self.bulk_insert_for_root_account_id(root_account_id, user_id_to_aad_hash) + # + # This method also refetches the Account settings after adding to make sure + # the UPN type and tenant haven't changed from the root_account that is + # passed in. The settings in root_account should be what was used to fetch + # the aads. This ensures that the values we are adding are actually for the + # UPN type and tenant currently in the Account settings. If the settings + # have changed, the just-added values will be deleted and this method will + # raise an AccountSettingsChanged error. + def self.bulk_insert_for_root_account(root_account, user_id_to_aad_hash) return if user_id_to_aad_hash.empty? now = Time.zone.now records = user_id_to_aad_hash.map do |user_id, aad_id| { - root_account_id: root_account_id, + root_account_id: root_account.id, created_at: now, updated_at: now, user_id: user_id, aad_id: aad_id, } end - # TODO: either check UPN type in Account settings transactionally when adding, or - # check after adding and delete what we just added. - insert_all(records) + result = insert_all(records) + + if account_microsoft_sync_settings_changed?(root_account) + ids_added = result.rows.map(&:first) + where(id: ids_added).delete_all if ids_added.present? + raise AccountSettingsChanged + end + end + + private_class_method def self.account_microsoft_sync_settings_changed?(root_account) + current_settings = Account.where(id: root_account.id).select(:settings).take.settings + %i[microsoft_sync_tenant microsoft_sync_login_attribute].any? do |key| + root_account.settings[key] != current_settings[key] + end end # Find the enrollments for course which have a UserMapping for the user. @@ -109,8 +132,6 @@ class MicrosoftSync::UserMapping < ActiveRecord::Base end def self.delete_user_mappings_for_account(account, batch_size) - # TODO: When we figure out a way to ensure a UserMapping is up-to-date with an account's Sync settings - # (like by using a hash), we'll need to update this query to filter by the account's current settings. while self.where(root_account: account).limit(batch_size).delete_all > 0; end end end diff --git a/lib/microsoft_sync/syncer_steps.rb b/lib/microsoft_sync/syncer_steps.rb index 21611d3fd3a..b4ffe8e3092 100644 --- a/lib/microsoft_sync/syncer_steps.rb +++ b/lib/microsoft_sync/syncer_steps.rb @@ -182,7 +182,10 @@ module MicrosoftSync users_and_upns.each_slice(GraphServiceHelpers::USERS_UPNS_TO_AADS_BATCH_SIZE) do |slice| upn_to_aad = graph_service_helpers.users_upns_to_aads(slice.map(&:last)) user_id_to_aad = slice.map{|user_id, upn| [user_id, upn_to_aad[upn]]}.to_h.compact - UserMapping.bulk_insert_for_root_account_id(course.root_account_id, user_id_to_aad) + # NOTE: root_account here must be the same (values loaded into memory at the same time) + # as passed into UsersUpnsFinder AND as used in #tenant, for the "have settings changed?" + # check to work. For example, using course.root_account here would NOT be correct. + UserMapping.bulk_insert_for_root_account(group.root_account, user_id_to_aad) end end diff --git a/spec/lib/microsoft_sync/syncer_steps_spec.rb b/spec/lib/microsoft_sync/syncer_steps_spec.rb index a1847f13794..5467fa92b74 100644 --- a/spec/lib/microsoft_sync/syncer_steps_spec.rb +++ b/spec/lib/microsoft_sync/syncer_steps_spec.rb @@ -449,6 +449,45 @@ describe MicrosoftSync::SyncerSteps do expect(upns_looked_up).to include("student1@example.com") end end + + context 'when the Account tenant changes while the job is running' do + before do + orig_root_account_method = syncer_steps.group.method(:root_account) + + allow(syncer_steps.group).to receive(:root_account) do + result = orig_root_account_method.call + # Change account settings right after we have used them. + # This tests that we are using the same root_account for the GraphService tenant + # as we are passing into UserMapping.bulk_insert_for_root_account + acct = Account.find(result.id) + acct.settings[:microsoft_sync_tenant] = "EXTRA" + acct.settings[:microsoft_sync_tenant] + acct.save + result + end + end + + it 'raises a UserMapping::AccountSettingsChanged error' do + expect{subject}.to raise_error(MicrosoftSync::UserMapping::AccountSettingsChanged) + end + end + + context 'when the Account login attribute changes while the job is running' do + before do + orig_root_account_method = MicrosoftSync::UsersUpnsFinder.method(:new) + + allow(MicrosoftSync::UsersUpnsFinder).to receive(:new) do |user_ids, root_account| + result = orig_root_account_method.call(user_ids, root_account) + acct = Account.find(root_account.id) + acct.settings[:microsoft_sync_login_attribute] = 'somethingelse' + acct.save + result + end + end + + it 'raises a UserMapping::AccountSettingsChanged error' do + expect{subject}.to raise_error(MicrosoftSync::UserMapping::AccountSettingsChanged) + end + end end end diff --git a/spec/models/microsoft_sync/user_mapping_spec.rb b/spec/models/microsoft_sync/user_mapping_spec.rb index 98d469fa507..f130f9f7fbf 100644 --- a/spec/models/microsoft_sync/user_mapping_spec.rb +++ b/spec/models/microsoft_sync/user_mapping_spec.rb @@ -58,25 +58,77 @@ describe MicrosoftSync::UserMapping do end end - describe '.bulk_insert_for_root_account_id' do - it "creates UserMappings if they don't already exist" do - account = account_model - user1 = user_model - user2 = user_model - described_class.create!(root_account_id: account.id, user_id: user1.id, aad_id: 'manual') - described_class.create!(root_account_id: 0, user_id: user2.id, aad_id: 'manual-wrong-ra-id') - described_class.bulk_insert_for_root_account_id( - account.id, - user1.id => 'user1', - user2.id => 'user2' - ) - expect(described_class.where(root_account_id: account.id).pluck(:user_id, :aad_id).sort).to \ - eq([[user1.id, 'manual'], [user2.id, 'user2']].sort) + describe '.bulk_insert_for_root_account' do + context 'when user_id_to_aad_hash is not empty' do + subject do + described_class.bulk_insert_for_root_account( + account, + user1.id => 'user1', + user2.id => 'user2' + ) + end + + let(:account) do + account_model(settings: { + microsoft_sync_login_attribute: 'email', + microsoft_sync_tenant: 'myinstructuretenant.onmicrosoft.com' + }) + end + let(:user1) { user_model } + let(:user2) { user_model } + + before do + described_class.create!(root_account_id: account.id, user_id: user1.id, aad_id: 'manual') + described_class.create!(root_account_id: 0, user_id: user2.id, aad_id: 'manual-wrong-ra-id') + end + + it "creates UserMappings if they don't already exist" do + subject + expect(described_class.where(root_account_id: account.id).pluck(:user_id, :aad_id).sort).to \ + eq([[user1.id, 'manual'], [user2.id, 'user2']].sort) + end + + context 'when the tenant in the Account settings has changed since fetching the account' do + before do + acct = Account.find(account.id) + acct.settings[:microsoft_sync_tenant] = 'EXTRA' + acct.settings[:microsoft_sync_tenant] + acct.save + end + + it "raises an AccountSettingsChanged error and doesn't add/change mappings" do + expect { subject } + .to raise_error(described_class::AccountSettingsChanged) + .and not_change{described_class.order(:id).map(&:attributes)} + end + end + + context 'when the upn type in the Account settings has changed since fetching the account' do + before do + acct = Account.find(account.id) + acct.settings[:microsoft_sync_login_attribute] = 'sis_user_id' + acct.save + end + + it "raises an AccountSettingsChanged error and doesn't add/change mappings" do + expect { subject } + .to raise_error(described_class::AccountSettingsChanged) + .and not_change{described_class.order(:id).map(&:attributes)} + end + end end - it "doesn't raise an error on an empty hash" do - expect { described_class.bulk_insert_for_root_account_id(0, {}) }.to_not \ - change { described_class.count }.from(0) + context 'when user_id_to_aad_hash is empty' do + it "doesn't raise an error" do + expect { described_class.bulk_insert_for_root_account(account_model, {}) }.to_not \ + change { described_class.count }.from(0) + end + end + end + + describe 'AccountSettingsChanged' do + it 'is a graceful cancel error' do + expect(described_class::AccountSettingsChanged.new).to \ + be_a(MicrosoftSync::Errors::GracefulCancelErrorMixin) end end @@ -146,7 +198,7 @@ describe MicrosoftSync::UserMapping do end def setup_microsoft_sync_data(account, id_to_aad_hash) - MicrosoftSync::UserMapping.bulk_insert_for_root_account_id(account.id, id_to_aad_hash) + MicrosoftSync::UserMapping.bulk_insert_for_root_account(account, id_to_aad_hash) end before(:each) do