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 <svc.cloudjenkins@instructure.com>
Reviewed-by: Ryan Hawkins <ryan.hawkins@instructure.com>
Reviewed-by: Mysti Lilla <mysti@instructure.com>
QA-Review: Ryan Hawkins <ryan.hawkins@instructure.com>
Product-Review: Evan Battaglia <ebattaglia@instructure.com>

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

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
 

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
 

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