Use LTI 1.3 lti_id for observees custom field

Previously, when any LTI tool, regardless of version, used the
custom field `com.instructure.User.observees`, a comma separated
list that contained the LTI 1.1 lti_user_id of all the students
the current user was observing in the current context was returned.
Now, if the tool being launched uses LTI 1.3, that list now
uses the lti_id of the students the current user is observing
in the current context instead.

fixes INTEROP-5991

flag = none

test-plan:
* Have an LTI 1.3 and 1.1 tool installed, both of which use the
  `com.instructure.User.observees` custom field.
* Have a user with an observer in the current context (typically
  a course)
* As the observer, launch both tools.
* For the LTI 1.3 tool, verify that the list of students the
  observer is observing matches the students' lti_id's.
* For the LTI 1.1 tool, verify that the list of students the
  observer is observing matches the students' lti_user_ids.
  This can be verified (and is calculated) using
  Lti::Asset.opaque_identifier_for(<your_user>)

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

app/models/user.rb

@@ -861,6 +861,15 @@ class User < ActiveRecord::Base
     true
   end
 
+  # Because some user's can have old lti ids that differ from self.lti_id,
+  # which also depends on the current context.
+  def lookup_lti_id(context)
+    old_lti_id = context.shard.activate do
+      self.past_lti_ids.where(context: context).take&.user_lti_id
+    end
+    old_lti_id || self.lti_id
+  end
+
   def preserve_lti_id
     errors.add(:lti_id, 'Cannot change lti_id!') if lti_id_changed? && lti_id_was != nil
   end

doc/api/tools_variable_substitutions.md

@@ -164,7 +164,10 @@ particular placement:
 If the current user is an observer in the launch
 context, this substitution returns a comma-separated
 list of user IDs linked to the current user for
-observing.
+observing. For LTI 1.3 tools, the user IDs will
+correspond to the "sub" claim made in LTI 1.3 launches
+(a UUIDv4), while for all other tools, the user IDs will
+be the user's typical LTI ID.
 
 Returns an empty string otherwise.
 
@@ -172,7 +175,8 @@ Returns an empty string otherwise.
 **Launch Parameter**: *com_instructure_user_observees*  
 
 ```
- "86157096483e6b3a50bfedc6bac902c0b20a824f","c0ddd6c90cbe1ef0f32fbce5c3bf654204be186c"
+ LTI 1.3: "a6e2e413-4afb-4b60-90d1-8b0344df3e91",
+ All Others: "c0ddd6c90cbe1ef0f32fbce5c3bf654204be186c"
 ```
 ## Context.title
 The title of the context.

lib/lti/messages/jwt_message.rb

@@ -72,7 +72,7 @@ module Lti::Messages
       @message.iat = Time.zone.now.to_i
       @message.iss = Canvas::Security.config['lti_iss']
       @message.nonce = SecureRandom.uuid
-      @message.sub = old_lti_id || @user.lti_id
+      @message.sub = @user.lookup_lti_id(@context)
       @message.target_link_uri = target_link_uri
     end
 
@@ -82,12 +82,6 @@ module Lti::Messages
       @tool.url
     end
 
-    def old_lti_id
-      @context.shard.activate do
-        @user.past_lti_ids.where(context: @context).take&.user_lti_id
-      end
-    end
-
     def add_context_claims!
       @message.context.id = Lti::Asset.opaque_identifier_for(@context)
       @message.context.label = @context.course_code if @context.respond_to?(:course_code)

lib/lti/variable_expander.rb

@@ -140,21 +140,28 @@ module Lti
     # If the current user is an observer in the launch
     # context, this substitution returns a comma-separated
     # list of user IDs linked to the current user for
-    # observing.
+    # observing. For LTI 1.3 tools, the user IDs will
+    # correspond to the "sub" claim made in LTI 1.3 launches
+    # (a UUIDv4), while for all other tools, the user IDs will
+    # be the user's typical LTI ID.
     #
     # Returns an empty string otherwise.
     #
     # @launch_parameter com_instructure_user_observees
     # @example
     #   ```
-    #    "86157096483e6b3a50bfedc6bac902c0b20a824f","c0ddd6c90cbe1ef0f32fbce5c3bf654204be186c"
+    #    LTI 1.3: "a6e2e413-4afb-4b60-90d1-8b0344df3e91",
+    #    All Others: "c0ddd6c90cbe1ef0f32fbce5c3bf654204be186c"
     #   ```
     register_expansion 'com.instructure.User.observees', [],
                       -> do
-                        ObserverEnrollment.observed_students(@context, @current_user).
-                          keys.
-                          map { |u| Lti::Asset.opaque_identifier_for(u) }.
-                          join(',')
+                        observed_users = ObserverEnrollment.observed_students(@context, @current_user).
+                          keys
+                          if @tool.use_1_3?
+                            observed_users.map{ |u| u.lookup_lti_id(@context) }.join(',')
+                          else
+                            observed_users.map{ |u| Lti::Asset.opaque_identifier_for(u) }.join(',')
+                          end
                       end,
                       COURSE_GUARD,
                       default_name: 'com_instructure_user_observees'

spec/lib/lti/variable_expander_spec.rb

@@ -1072,6 +1072,18 @@ module Lti
                 Lti::Asset.opaque_identifier_for(student)
               ]
             end
+
+            context 'the tool in use is an LTI 1.3 tool' do
+              before do
+                allow(tool).to receive(:use_1_3?).and_return(true)
+              end
+
+              it "returns the users' lti id instead of lti 1.1 user_id" do
+                expect(subject.split(',')).to match_array [
+                  student.lti_id
+                ]
+              end
+            end
           end
 
           context 'when the current user is not observing users in the context' do