MSFT sync: fix case bug

Microsoft UPNs are case-insensitive, and the Microsoft API may send back
UPN mappings in a different case from that which we requested them. When
that happens, we weren't using the UPN mapping returned. This fixes that
and also raises an error if we get some other UPN back from the API that
we don't expect (so would otherwise ignore), in case there is some other
similar issue.

closes INTEROP-6788
flag=microsoft_group_enrollments_syncing

Test plan:
- Make a new course with the following users enrolled. Note: it's
  probably easiest to use users you've never used in Canvas before so you
  can create users with email address with varying case easier. Look at
  the Microsoft admin portal to see users. Canvas seems to find users
  with an equivalent case-insensitive email, unless adding both users as
  new users to the course at the same time.
  - one user that is in our test tenant but has different casing,
    e.g. INTEROP_Teacher@...
  - two users that are in our test tenant but with different casing from
    the test tenant and with each other, e.g. InterOp_STUDENT@ and
    INTEROP_student@
- clear the UserMapping table
- run a sync
- check that the UserMapping table has entries for all three users, with
  the users having the same (downcased) email addresses having the same
  AAD id.
- check in Microsoft admin portal that both users were added to the
  group (since it's case insensitive, users with the same casing will
  only be there once of course)

Change-Id: Id0bf4cc3a7340715e4df930afcfb98ad83cb077a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/264784
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Weston Dransfield <wdransfield@instructure.com>
QA-Review: Evan Battaglia <ebattaglia@instructure.com>
Product-Review: Evan Battaglia <ebattaglia@instructure.com>

lib/microsoft_sync/graph_service_helpers.rb

@@ -26,6 +26,13 @@ module MicrosoftSync
   class GraphServiceHelpers
     attr_reader :graph_service
 
+    class UnexpectedResponseError < Errors::PublicError
+      def public_message
+        I18n.t('Unexpected response from Microsoft API. This is likely a bug. ' \
+               'Please contact support.')
+      end
+    end
+
     MAX_MAIL_NICKNAME_LENGTH = 64
 
     def initialize(tenant)
@@ -70,17 +77,42 @@ module MicrosoftSync
     # property that Canvas has.  An AAD (short term for AAD [Azure Active
     # Directory] object ID) is the ID for the user on the Microsoft side, which
     # is what Microsoft references in their groups/teams.
+    #
+    # UPNs are case-insensitive on the Microsoft side; this method downcases
+    # them before requesting them from Microsoft, but the keys in the return
+    # hash match the case of the upns that were passed in.
     def users_upns_to_aads(upns)
-      if upns.length > USERS_UPNS_TO_AADS_BATCH_SIZE
+      downcased_uniqued = upns.map(&:downcase).uniq
+      if downcased_uniqued.length > USERS_UPNS_TO_AADS_BATCH_SIZE
         raise ArgumentError, "Can't look up #{upns.length} UPNs at once"
       end
 
+      upns_downcased_to_given_forms = upns.group_by(&:downcase)
+
+      unexpected = []
+      result_hash = {}
+
       graph_service.list_users(
         select: %w[id userPrincipalName],
-        filter: {userPrincipalName: upns}
-      ).map do |result|
-        [result['userPrincipalName'], result['id']]
-      end.to_h
+        filter: {userPrincipalName: downcased_uniqued}
+      ).each do |result|
+        given_forms = upns_downcased_to_given_forms[result['userPrincipalName'].downcase]
+        if given_forms
+          given_forms.each do |given_form|
+            result_hash[given_form] = result['id']
+          end
+        else
+          unexpected << result['userPrincipalName']
+        end
+      end
+
+      if unexpected.present?
+        raise UnexpectedResponseError,
+              "/users returned unexpected UPN(s) #{unexpected.inspect}, " \
+              "asked for #{downcased_uniqued}"
+      end
+
+      result_hash
     end
 
     def get_group_users_aad_ids(group_id, owners: false)

lib/microsoft_sync/syncer_steps.rb

@@ -146,6 +146,8 @@ module MicrosoftSync
         users_upns_finder = MicrosoftSync::UsersUpnsFinder.new(user_ids, group.root_account)
         users_and_upns = users_upns_finder.call
 
+        # If some users in different slices have the same UPNs, this could end up
+        # looking up the same UPN multiple times; but this should be very rare
         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

spec/lib/microsoft_sync/graph_service_helpers_spec.rb

@@ -188,20 +188,55 @@ describe MicrosoftSync::GraphServiceHelpers do
   end
 
   describe '#users_upns_to_aads' do
-    it 'returns a hash from UPN to AAD object id' do
-      expect(subject.graph_service).to \
-        receive(:list_users).
-        with(select: %w[id userPrincipalName], filter: {userPrincipalName: %w[a b c d]}).
-        and_return([
-          {'id' => '789', 'userPrincipalName' => 'd'},
-          {'id' => '456', 'userPrincipalName' => 'b'},
+    let(:requested) { %w[a b c d] }
+
+    before do
+      allow(subject.graph_service).to \
+        receive(:list_users)
+        .with(select: %w[id userPrincipalName], filter: {userPrincipalName: requested})
+        .and_return([
+          {'id' => '789', 'userPrincipalName' => 'D'},
           {'id' => '123', 'userPrincipalName' => 'a'},
+          {'id' => '456', 'userPrincipalName' => 'b'},
         ])
-      expect(subject.users_upns_to_aads(%w[a b c d])).to eq(
-        'a' => '123',
-        'b' => '456',
-        'd' => '789'
-      )
+    end
+
+    context "when the graph service sends a UPN back that differs in case" do
+      it 'maps the response back to case of the UPN in the input array' do
+        expect(subject.users_upns_to_aads(%w[a b c d])).to eq(
+          'a' => '123',
+          'b' => '456',
+          'd' => '789'
+        )
+      end
+    end
+
+    context "when the graph service sends a upn back that it didn't ask for" do
+      let(:requested) { %w[c d] }
+
+      it "raises an error" do
+        expect { subject.users_upns_to_aads(%w[c D]) }.to \
+          raise_error do |err|
+          expect(err.message).to eq(
+            '/users returned unexpected UPN(s) ["a", "b"], asked for ["c", "d"]'
+          )
+          expect(err.public_message).to eq(
+            'Unexpected response from Microsoft API. This is likely a bug. Please contact support.'
+          )
+          expect(err).to be_a(MicrosoftSync::Errors::PublicError)
+        end
+      end
+    end
+
+    context 'when given different-case duplicates of the same UPN' do
+      it "only requests one and copies the AAD on all matching UPNs" do
+        expect(subject.users_upns_to_aads(%w[a b c A C D])).to eq(
+          'a' => '123',
+          'A' => '123',
+          'b' => '456',
+          'D' => '789'
+        )
+      end
     end
 
     context 'when passed in more than 15' do