Validate LTI message_type for placements

why:
- Some schools were reporting errors while launching certain LTI tools.
- These errors were due to a misconfiguration within the LTI tools
  themselves, where they were configured to initiate a deep linking
  request at placements that don't support it.
- This caused Canvas to throw an error and not launch the tool.
- To prevent this, during launch time, we only initiate a deep linking
  request if the tool is configured to do so and the placement supports
  it.
- Additionally, we now validate all configured placements within the
  Lti::ToolConfiguration on save, so that no invalid
  message_type/placement pairings get through.

fixes INTEROP-8509

flag=none

test-plan:
- Before this commit:
- Install the LTI 1.3 test tool with a placement that only supports
  resource link requests, such as account_navigation or
  course_navigation.
- Modify the config using the JSON editor in the UI (or the Rails
  console), so that the course_navigation placement has a message_type
  of "LtiDeepLinkingRequest".
- Try to launch the tool. You should get a 500 that looks much like the
  error_report linked in the ticket.
- Pull this commit down.
- Try launching the tool again. It should launch successfully as a
  resource link request, which can be verified in the decoded JWT the
  tool shows.
- Modify the placement to have the appropriate message_type again, using
  the UI or the rails console.
- Try to modify it to have the deep linking message type. You should
  get a flash alert error in the UI stating that the placement doesn't
  support that message type.

Change-Id: I58908450c9784509c79df1fd93f5392cfb16b61a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/344102
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Paul Gray <paul.gray@instructure.com>
QA-Review: Paul Gray <paul.gray@instructure.com>
Migration-Review: Andrea Cirulli <andrea.cirulli@instructure.com>
Product-Review: Mark Starkman <mark.starkman@instructure.com>

app/models/context_external_tool.rb

@@ -800,7 +800,11 @@ class ContextExternalTool < ActiveRecord::Base
     when :selection_height
       400
     when :message_type
-      if type == :resource_selection
+      if use_1_3? && type == :editor_button
+        LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE
+      elsif use_1_3?
+        LtiAdvantage::Messages::ResourceLinkRequest::MESSAGE_TYPE
+      elsif type == :resource_selection
         "resource_selection"
       else
         "basic-lti-launch-request"

app/models/lti/lti_advantage_adapter.rb

@@ -179,6 +179,12 @@ module Lti
       end
 
       message_type = @tool.extension_setting(resource_type, :message_type)
+      unless Lti::ResourcePlacement.supported_message_type?(resource_type, message_type)
+        e = Lti::InvalidMessageTypeForPlacementError.new
+        CanvasErrors.capture(e, { tags: { developer_key_id: @tool.global_developer_key_id } }, :error)
+        # We explicitly want to stop the launch at this point.
+        raise e
+      end
       if message_type == LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE
         deep_linking_request.generate_post_payload
       else

app/models/lti/resource_placement.rb

@@ -20,6 +20,8 @@
 require "lti_advantage"
 
 module Lti
+  class InvalidMessageTypeForPlacementError < StandardError; end
+
   class ResourcePlacement < ActiveRecord::Base
     ACCOUNT_NAVIGATION = "account_navigation"
     ASSIGNMENT_EDIT = "assignment_edit"
@@ -136,5 +138,12 @@ module Lti
       end
       item_banks_tab
     end
+
+    def self.supported_message_type?(placement, message_type)
+      return true if message_type.blank?
+      return false if placement.blank? || PLACEMENTS.exclude?(placement.to_sym)
+
+      PLACEMENTS_BY_MESSAGE_TYPE[message_type.to_s]&.include?(placement.to_sym)
+    end
   end
 end

app/models/lti/tool_configuration.rb

@@ -180,6 +180,12 @@ module Lti
     end
 
     def validate_placements
+      placements.each do |p|
+        unless Lti::ResourcePlacement.supported_message_type?(p["placement"], p["message_type"])
+          errors.add(:placements, "Placement #{p["placement"]} does not support message type #{p["message_type"]}")
+        end
+      end
+
       return if disabled_placements.blank?
 
       invalid = disabled_placements.reject { |p| Lti::ResourcePlacement::PLACEMENTS.include?(p.to_sym) }

db/migrate/20240412200754_fixup_invalid_dev_key_placement_configurations.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+#
+# Copyright (C) 2023 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+class FixupInvalidDevKeyPlacementConfigurations < ActiveRecord::Migration[7.0]
+  tag :postdeploy
+  def change
+    DataFixup::Lti::FixInvalidPlacementConfigurations
+      .delay_if_production(priority: Delayed::LOWER_PRIORITY)
+      .run
+  end
+end

lib/data_fixup/lti/fix_invalid_placement_configurations.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+#
+# Copyright (C) 2024 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+module DataFixup::Lti::FixInvalidPlacementConfigurations
+  def self.run
+    DeveloperKey.nondeleted.where(is_lti_key: true).preload(:tool_configuration).find_each do |dk|
+      # Skip keys associated with Dynamic Registration. They don't have a tool config and are already good to go.
+      next unless dk.referenced_tool_configuration.present?
+
+      actual_placements = dk.referenced_tool_configuration.configuration["extensions"]
+                            &.find { |e| e["platform"] == Lti::ToolConfiguration::CANVAS_EXTENSION_LABEL }
+                            &.dig("settings", "placements")
+      actual_placements&.each do |placement|
+        next unless placement_needs_fixing?(placement)
+
+        placement["message_type"] = opposite_message_type(placement["message_type"])
+      end
+
+      dk.referenced_tool_configuration.save! if dk.referenced_tool_configuration.changed?
+    end
+  end
+
+  def self.placement_needs_fixing?(p)
+    return false unless Lti::ResourcePlacement::PLACEMENTS.include?(p["placement"]&.to_sym) && Lti::ResourcePlacement::PLACEMENTS_BY_MESSAGE_TYPE.key?(p["message_type"])
+    return false if Lti::ResourcePlacement.supported_message_type?(p["placement"], p["message_type"]) || p["message_type"].blank?
+
+    !Lti::ResourcePlacement.supported_message_type?(p["placement"], p["message_type"])
+  end
+
+  def self.opposite_message_type(message_type)
+    if message_type == LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE
+      LtiAdvantage::Messages::ResourceLinkRequest::MESSAGE_TYPE
+    else
+      LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE
+    end
+  end
+end

spec/controllers/external_tools_controller_spec.rb

@@ -1944,9 +1944,11 @@ describe ExternalToolsController do
         context "during a basic launch" do
           let(:message_type) { "LtiResourceLinkRequest" }
 
-          it_behaves_like "includes editor variables" do
+          it "doesn't launch, as LtiResourceLinkRequest is not supported in the RCE" do
-            let(:selection_launch_param) { launch_params.dig("https://purl.imsglobal.org/spec/lti/claim/custom", "selection") }
+            allow(CanvasErrors).to receive(:capture)
-            let(:contents_launch_param) { launch_params.dig("https://purl.imsglobal.org/spec/lti/claim/custom", "contents") }
+            subject
+            expect(response).not_to be_successful
+            expect(CanvasErrors).to have_received(:capture).with(an_instance_of(Lti::InvalidMessageTypeForPlacementError), hash_including({ tags: { developer_key_id: tool.global_developer_key_id } }), :error)
           end
         end
 

spec/factories/lti_ims_registration_factory.rb

@@ -83,7 +83,7 @@ module Factories
 
   def lti_ims_registration_model(**params)
     params = LTI_IMS_REGISTRATION_BASE_ATTRS.merge(params)
-    params[:developer_key] ||= developer_key_model
+    params[:developer_key] ||= developer_key_model(public_jwk_url: LTI_IMS_REGISTRATION_BASE_ATTRS[:jwks_uri], account: params.delete(:account) || account_model)
     @ims_registration = Lti::IMS::Registration.create!(params)
   end
 end

spec/lib/data_fixup/lti/fix_invalid_placement_configurations_spec.rb

@@ -0,0 +1,102 @@
+# frozen_string_literal: true
+
+#
+# Copyright (C) 2024 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+require "lti_1_3_tool_configuration_spec_helper"
+
+RSpec.describe DataFixup::Lti::FixInvalidPlacementConfigurations do
+  subject { described_class.run }
+
+  include_context "lti_1_3_tool_configuration_spec_helper"
+
+  let(:account) { Account.default }
+  let(:developer_key) { DeveloperKey.create!(account:, workflow_state: "active", is_lti_key: true, public_jwk:) }
+
+  before do
+    tool_configuration.configuration["extensions"].first["settings"]["placements"] = []
+    tool_configuration.save!
+  end
+
+  def placement_config(tool_config, placement_name)
+    tool_config
+      .configuration["extensions"]
+      .find { |e| e["platform"] == Lti::ToolConfiguration::CANVAS_EXTENSION_LABEL }["settings"]["placements"]
+      .find { |p| p["placement"] == placement_name }
+  end
+
+  def add_placement(tool_config, placement)
+    tool_config.configuration["extensions"].first["settings"]["placements"] << placement
+  end
+
+  it "doesn't change the config of a properly configured tool" do
+    add_placement(tool_configuration, { placement: "course_navigation", message_type: "LtiResourceLinkRequest" })
+    expect { subject }.not_to change { tool_configuration.configuration }
+  end
+
+  context "placements that only support deep linking requests" do
+    (Lti::ResourcePlacement::PLACEMENTS_BY_MESSAGE_TYPE["LtiDeepLinkingRequest"] - Lti::ResourcePlacement::PLACEMENTS_BY_MESSAGE_TYPE["LtiResourceLinkRequest"]).each do |placement|
+      it "swaps the message_type for a misconfigured #{placement} placement" do
+        add_placement(tool_configuration, { placement: placement.to_s, message_type: "LtiResourceLinkRequest" })
+        # Have to avoid the validation that would prevent this from saving
+        tool_configuration.save(validate: false)
+        expect { subject }
+          .to change { placement_config(tool_configuration.reload, placement.to_s)["message_type"] }
+          .from("LtiResourceLinkRequest")
+          .to("LtiDeepLinkingRequest")
+      end
+    end
+  end
+
+  context "placements that only support resource link requests" do
+    (Lti::ResourcePlacement::PLACEMENTS_BY_MESSAGE_TYPE["LtiResourceLinkRequest"] - Lti::ResourcePlacement::PLACEMENTS_BY_MESSAGE_TYPE["LtiDeepLinkingRequest"]).each do |placement|
+      it "swaps the message_type for a misconfigured #{placement} placement" do
+        add_placement(tool_configuration, { placement: placement.to_s, message_type: "LtiDeepLinkingRequest" })
+        # Have to avoid the validation that would prevent this from saving
+        tool_configuration.save(validate: false)
+        expect { subject }
+          .to change { placement_config(tool_configuration.reload, placement.to_s)["message_type"] }
+          .from("LtiDeepLinkingRequest")
+          .to("LtiResourceLinkRequest")
+      end
+    end
+  end
+
+  it "avoids placements that don't have a message type" do
+    add_placement(tool_configuration, { placement: "course_navigation" })
+    tool_configuration.save(validate: false)
+    expect { subject }.not_to change { tool_configuration.configuration }
+  end
+
+  it "avoids tools that have an invalid placement" do
+    add_placement(tool_configuration, { placement: "invalid_placement", message_type: "LtiDeepLinkingRequest" })
+    tool_configuration.save(validate: false)
+    expect { subject }.not_to change { tool_configuration.configuration }
+  end
+
+  it "avoids tools that have an invalid message type" do
+    add_placement(tool_configuration, { placement: "course_navigation", message_type: "invalid_message_type" })
+    tool_configuration.save(validate: false)
+    expect { subject }.not_to change { tool_configuration.configuration }
+  end
+
+  it "skips developer keys that were created by Dynamic Registration" do
+    dyn_reg = lti_ims_registration_model(account:)
+
+    expect { subject }.not_to change { dyn_reg.reload.configuration }
+  end
+end

spec/models/context_external_tool_spec.rb

@@ -2847,6 +2847,24 @@ describe ContextExternalTool do
     it "returns resource_selection when the type is 'resource_selection'" do
       expect(subject.extension_default_value(:resource_selection, :message_type)).to eq "resource_selection"
     end
+
+    it "returns basic-lti-launch-request for all other types" do
+      expect(subject.extension_default_value(:course_navigation, :message_type)).to eq "basic-lti-launch-request"
+    end
+
+    context "the tool uses 1.3" do
+      let(:tool) do
+        external_tool_1_3_model(context: @root_account)
+      end
+
+      it "returns LtiResourceLinkRequest when the property is 'message_type'" do
+        expect(tool.extension_default_value(:course_navigation, :message_type)).to eq "LtiResourceLinkRequest"
+      end
+
+      it "returns LtiDeepLinkingRequest when the property is 'message_type' and type is editor_button" do
+        expect(tool.extension_default_value(:editor_button, :message_type)).to eq "LtiDeepLinkingRequest"
+      end
+    end
   end
 
   describe "change_domain" do

spec/models/lti/lti_advantage_adapter_spec.rb

@@ -60,7 +60,7 @@ describe Lti::LtiAdvantageAdapter do
       shared_secret: "secret",
       url: "http://www.example.com/basic_lti"
     )
-    tool.course_navigation = { enabled: true, message_type: "ResourceLinkRequest" }
+    tool.course_navigation = { enabled: true, message_type: "LtiResourceLinkRequest" }
     tool.use_1_3 = true
     tool.developer_key = DeveloperKey.create!
     tool.save!
@@ -107,6 +107,26 @@ describe Lti::LtiAdvantageAdapter do
       it "caches a deep linking request" do
         expect(params["https://purl.imsglobal.org/spec/lti/claim/message_type"]).to eq "LtiDeepLinkingRequest"
       end
+
+      context "and the placement does not support LtiDeepLinkingRequest" do
+        let(:opts) { { resource_type: "course_navigation", domain: "test.com" } }
+
+        before do
+          tool.course_navigation = {
+            enabled: true,
+            message_type: LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE,
+          }
+          tool.save!
+        end
+
+        it "raises an Lti::InvalidMessageTypeForPlacementError and tags with the dev_key_id" do
+          expect(CanvasErrors).to receive(:capture)
+            .with(an_instance_of(Lti::InvalidMessageTypeForPlacementError),
+                  { tags: { developer_key_id: tool.global_developer_key_id } },
+                  :error)
+          expect { login_message }.to raise_error(Lti::InvalidMessageTypeForPlacementError)
+        end
+      end
     end
 
     context "when target_link_uri is set" do
@@ -132,7 +152,7 @@ describe Lti::LtiAdvantageAdapter do
       end
     end
 
-    it "generates a resource link request if the tool's resource type setting is 'ResourceLinkRequest'" do
+    it "generates a resource link request if the tool's resource type setting is 'LtiResourceLinkRequest'" do
       expect(params["https://purl.imsglobal.org/spec/lti/claim/message_type"]).to eq "LtiResourceLinkRequest"
     end
 
@@ -301,7 +321,7 @@ describe Lti::LtiAdvantageAdapter do
     it "returns the resource-specific launch URL if set" do
       tool.course_navigation = {
         enabled: true,
-        message_type: "ResourceLinkRequest",
+        message_type: "LtiResourceLinkRequest",
         target_link_uri: "https://www.launch.com/course-navigation"
       }
       tool.save!

spec/models/lti/resource_placement_spec.rb

@@ -65,6 +65,50 @@ module Lti
       end
     end
 
+    describe ".supported_message_type?" do
+      subject do
+        described_class.method(:supported_message_type?)
+      end
+
+      it "returns false when no placement is passed in" do
+        expect(subject.call(nil, "message_type")).to be false
+      end
+
+      it "returns true when no message_type is passed in" do
+        expect(subject.call(Lti::ResourcePlacement::PLACEMENTS.first, nil)).to be true
+      end
+
+      it "returns false if an invalid placement is passed in" do
+        expect(subject.call("invalid_placement", LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE)).to be false
+      end
+
+      it "returns false if a valid placement but unsupported message_type is passed in" do
+        expect(subject.call(:course_navigation, LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE)).to be false
+      end
+
+      it "returns true if a valid placement is passed in" do
+        expect(subject.call(:assignment_selection, LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE)).to be true
+      end
+
+      it "works with strings and symbols for the placement" do
+        expect(subject.call("assignment_selection", LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE)).to be true
+        expect(subject.call(:assignment_selection, LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE)).to be true
+      end
+
+      it "works with strings and symbols for the message_type" do
+        expect(subject.call(:assignment_selection, LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE)).to be true
+        expect(subject.call(:assignment_selection, LtiAdvantage::Messages::DeepLinkingRequest::MESSAGE_TYPE.to_sym)).to be true
+      end
+
+      Lti::ResourcePlacement::PLACEMENTS_BY_MESSAGE_TYPE.each do |message_type, placements|
+        placements.each do |placement|
+          it "returns true for #{placement} and #{message_type}" do
+            expect(subject.call(placement, message_type)).to be true
+          end
+        end
+      end
+    end
+
     describe "update_tabs_and_return_item_banks_tab" do
       let(:tabs_with_item_banks) do
         [

spec/models/lti/tool_configuration_spec.rb

@@ -162,6 +162,25 @@ module Lti
         it { is_expected.to be false }
       end
 
+      context "when one of the configured placements has an unsupported message_type" do
+        before do
+          tool_configuration.developer_key = developer_key
+          tool_configuration.settings["extensions"].first["settings"]["placements"] = [
+            {
+              "placement" => "account_navigation",
+              "message_type" => "LtiDeepLinkingRequest",
+            }
+          ]
+        end
+
+        it { is_expected.to be false }
+
+        it "includes a friendly error message" do
+          subject
+          expect(tool_configuration.errors[:placements].first.message).to include("does not support message type")
+        end
+      end
+
       context "when extensions have non-Canvas platform" do
         let(:settings) do
           sets = super()