Lock down submission_type_selection placement

why:
- This is a very important placement that can easily be abused by tools,
  leading to an extremely confusing assignment creation/edit experience.
- By limiting this placement to only allowed tools (list stored in a
  Setting), we can prevent excess noise and abuse of this specialized
  placement.

flag=lti_placement_restrictions

closes INTEROP-8464

test-plan:
- With the lti_placement_restrictions flag disabled, you should be able
  to modify a dev key with the submission_type_selection placement
  without any warning. Basically, nothing should change
- Now, enable the lti_placement_restrictions flag.
- Try to register the LTI 1.3 test tool at the submission_type_selection
  placement. You should receive a warning message in your browser
  notifying you that this placement is restricted to approved tools only
  (messaging is still a WIP). Crucially though, your key should still
  save.
- Update any property on your dev key, such as the name. Again, you
  should get a message about how sts is restricted to approved tools
  only.
- Add the dev key's global id to the allow list by running:
```ruby
Setting.set(submission_type_selection_allowed_dev_keys", <global_id>)
```
- Restart your Canvas
- Try and update the dev key. You should no longer get a warning
  message.

Change-Id: I2f7306ee077b0cb22b7ddc6ac93b8d5b6198488b
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/341457
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Tucker Mcknight <tmcknight@instructure.com>
Reviewed-by: Steve Mcgee <steve.mcgee@instructure.com>
QA-Review: Tucker Mcknight <tmcknight@instructure.com>
Product-Review: Mark Starkman <mark.starkman@instructure.com>

app/controllers/lti/tool_configurations_api_controller.rb

@@ -91,7 +91,7 @@ class Lti::ToolConfigurationsApiController < ApplicationController
     developer_key_redirect_uris
     tool_config = Lti::ToolConfiguration.create_tool_config_and_key!(account, tool_configuration_params)
     update_developer_key!(tool_config, developer_key_redirect_uris)
-    render json: Lti::ToolConfigurationSerializer.new(tool_config)
+    render json: Lti::ToolConfigurationSerializer.new(tool_config, include_warnings: true)
   end
 
   # @API Update Tool configuration
@@ -135,7 +135,7 @@ class Lti::ToolConfigurationsApiController < ApplicationController
     )
     update_developer_key!(tool_config)
 
-    render json: Lti::ToolConfigurationSerializer.new(tool_config)
+    render json: Lti::ToolConfigurationSerializer.new(tool_config, include_warnings: true)
   end
 
   # @API Show Tool configuration

app/models/lti/tool_configuration.rb

@@ -113,6 +113,25 @@ module Lti
       configuration["extensions"]&.find { |e| e["platform"] == CANVAS_EXTENSION_LABEL }&.dig("settings", "placements")&.deep_dup || []
     end
 
+    def domain
+      return [] if configuration.blank?
+
+      configuration["extensions"]&.find { |e| e["platform"] == CANVAS_EXTENSION_LABEL }&.dig("domain") || ""
+    end
+
+    # @return [String | nil] A warning message about any disallowed placements
+    def verify_placements
+      return unless placements.any? { |p| p["placement"] == "submission_type_selection" }
+      return unless Account.site_admin.feature_enabled?(:lti_placement_restrictions)
+
+      allowed_domains = Setting.get("submission_type_selection_allowed_launch_domains", "").split(",")
+
+      allowed_dev_keys = Setting.get("submission_type_selection_allowed_dev_keys", "").split(",")
+      return if allowed_domains.include?(domain) || allowed_dev_keys.include?(Shard.global_id_for(developer_key_id).to_s)
+
+      t("Warning: the submission_type_selection placement is only allowed for Instructure approved LTI tools. If you believe you have received this message in error, please contact your support team.")
+    end
+
     private
 
     def self.retrieve_and_extract_configuration(url)

app/serializers/lti/tool_configuration_serializer.rb

@@ -21,20 +21,24 @@ module Lti
   class ToolConfigurationSerializer
     include Api::V1::DeveloperKey
 
-    def initialize(tool_configuration)
+    def initialize(tool_configuration, include_warnings: false)
       @tool_configuration = tool_configuration
+      @include_warnings = include_warnings
     end
 
     def as_json
       key = @tool_configuration.developer_key
-      @tool_configuration.as_json.merge({
-                                          developer_key: developer_key_json(
-                                            key,
-                                            nil,
-                                            nil,
-                                            key.owner_account
-                                          )
-                                        })
+      json = @tool_configuration.as_json.merge({
+                                                 developer_key: developer_key_json(
+                                                   key,
+                                                   nil,
+                                                   nil,
+                                                   key.owner_account
+                                                 ),
+                                               })
+
+      json[:warning_message] = @tool_configuration.verify_placements if @include_warnings
+      json
     end
   end
 end

spec/lti_1_3_tool_configuration_spec_helper.rb

@@ -60,6 +60,7 @@ RSpec.shared_context "lti_1_3_tool_configuration_spec_helper", shared_context: :
           "tool_id" => "LTI 1.3 Test Tool",
           "domain" => "http://lti13testtool.docker",
           "settings" => {
+            "domain" => "lti13testtool.docker",
             "icon_url" => "https://static.thenounproject.com/png/131630-200.png",
             "selection_height" => 500,
             "selection_width" => 500,

spec/models/lti/tool_configuration_spec.rb

@@ -649,6 +649,71 @@ module Lti
       end
     end
 
+    describe "domain" do
+      subject { tool_configuration.domain }
+
+      it { is_expected.to eq(settings["extensions"].first["domain"]) }
+    end
+
+    describe "verify_placements" do
+      subject { tool_configuration.verify_placements }
+
+      before do
+        tool_configuration.developer_key = developer_key
+        tool_configuration.save!
+      end
+
+      context "when the lti_placement_restrictions feature flag is disabled" do
+        before do
+          Account.site_admin.disable_feature!(:lti_placement_restrictions)
+        end
+
+        it { is_expected.to be_nil }
+      end
+
+      context "when the lti_placement_restrictions feature flag is enabled" do
+        before do
+          Account.site_admin.enable_feature!(:lti_placement_restrictions)
+        end
+
+        it "returns nil when there are no submission_type_selection placements" do
+          expect(subject).to be_nil
+        end
+
+        context "when the tool is allowed to use the submission_type_selection placement through it's dev key" do
+          before do
+            Setting.set("submission_type_selection_allowed_dev_keys", tool_configuration.developer_key.global_id.to_s)
+          end
+
+          it { is_expected.to be_nil }
+        end
+
+        context "when the tool is allowed to use the submission_type_selection placement through it's domain" do
+          before do
+            Setting.set("submission_type_selection_allowed_launch_domains", tool_configuration.domain)
+          end
+
+          it { is_expected.to be_nil }
+        end
+
+        context "when the configuration has a submission_type_selection placement" do
+          let(:tool_configuration) do
+            tc = super()
+
+            tc.settings["extensions"].first["settings"]["placements"] << {
+              "placement" => "submission_type_selection",
+              "message_type" => "LtiResourceLinkRequest",
+              "target_link_uri" => "http://example.com/launch?placement=submission_type_selection"
+            }
+
+            tc
+          end
+
+          it { is_expected.to include("Warning").and include("submission_type_selection") }
+        end
+      end
+    end
+
     describe "privacy_level" do
       subject do
         extensions["privacy_level"] = extension_privacy_level

ui/features/developer_keys_v2/model/api/DeveloperKey.ts

@@ -16,8 +16,8 @@
  * with this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
-import type {LtiScope} from './LtiScopes'
-import type {LtiRegistration} from './LtiRegistration'
+import type {LtiScope} from '../LtiScopes'
+import type {LtiRegistration} from '../LtiRegistration'
 
 export interface DeveloperKeyAccountBinding {
   account_id: string

ui/features/developer_keys_v2/model/api/LtiToolConfiguration.ts

@@ -16,33 +16,50 @@
  * with this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
-import type {LtiPlacement} from './LtiPlacements'
+import type {LtiPlacement} from '../LtiPlacements'
+import type {LtiPrivacyLevel} from '../LtiPrivacyLevel'
+import type {LtiScope} from '../LtiScopes'
 
+/// @see lib/schemas/lti/tool_configuration.rb
 export interface LtiToolConfiguration {
+  id: string
+  privacy_level: LtiPrivacyLevel
+  developer_key_id: string
+  disabled_placements: string[]
+  settings: Configuration
+  /// ISO8601 timestamp.
+  created_at: string
+  /// ISO8601 timestamp.
+  updated_at: string
+}
+
+export interface Configuration {
   title: string
-  scopes: string[]
-  public_jwk_url?: string
-  public_jwk?: string
-  description: string | null
-  custom_parameters: Record<string, string> | null
+  description: string
   target_link_uri: string
   oidc_initiation_url: string
-  url: string
-  extensions: Extension[]
+  custom_fields?: Record<string, string>
+  oidc_initiation_urls?: Record<string, unknown>
+  public_jwk_url?: string
+  is_lti_key?: boolean
+  icon_url?: string
+  scopes?: LtiScope[]
+  extensions?: Extension[]
 }
 
 export interface Extension {
-  domain: string
+  domain?: string
   platform: string
-  tool_id: string
-  privacy_level: string
-  settings: Settings
+  /// This is *not* the actual meaningful id of any tool, but rather a tool provided value.
+  tool_id?: string
+  privacy_level?: LtiPrivacyLevel
+  settings: PlatformSettings
 }
 
-export interface Settings {
+export interface PlatformSettings {
   text: string
   icon_url: any
-  platform: string
+  platform?: string
   placements: PlacementConfig[]
 }
 
@@ -55,3 +72,13 @@ export interface PlacementConfig {
   icon_url: string
   custom_fields: Record<string, string>
 }
+
+/// @see lib/schemas/lti/public_jwk.rb
+export interface PublicJwk {
+  kty: 'RSA'
+  alg: 'RS256'
+  e: string
+  n: string
+  kid: string
+  use: string
+}

ui/features/developer_keys_v2/react/App.jsx

@@ -31,7 +31,7 @@ import AdminTable from './AdminTable'
 import InheritedTable from './InheritedTable'
 import DeveloperKey from './DeveloperKey'
 import NewKeyModal from './NewKeyModal'
-import {showFlashSuccess} from '@canvas/alerts/react/FlashAlert'
+import {showFlashAlert, showFlashSuccess} from '@canvas/alerts/react/FlashAlert'
 import DateHelper from '@canvas/datetime/dateHelper'
 import {DynamicRegistrationModal} from './dynamic_registration/DynamicRegistrationModal'
 
@@ -139,9 +139,15 @@ class DeveloperKeysApp extends React.Component {
    * workaround and do it.
    * @todo Find a better way to avoid modal-focus-screenreader-bulldozing so
    * this isn't necessary.
+   * @param {string} warningMessage - A warning message to show to the user.
    */
-  developerKeySaveSuccessfulHandler() {
-    setTimeout(showFlashSuccess(I18n.t('Save successful.')), ALERT_WAIT_TIME)
+  developerKeySaveSuccessfulHandler(warningMessage) {
+    setTimeout(() => {
+      showFlashSuccess(I18n.t('Save successful.'))()
+      if (warningMessage) {
+        showFlashAlert({message: warningMessage, type: 'warning'})
+      }
+    }, ALERT_WAIT_TIME)
   }
 
   render() {

ui/features/developer_keys_v2/react/NewKeyForm.tsx

@@ -34,7 +34,7 @@ import React from 'react'
 import Scopes from './Scopes'
 import ToolConfigurationForm from './ToolConfigurationForm'
 import type {AvailableScope} from './reducers/listScopesReducer'
-import type {DeveloperKey} from '../model/DeveloperKey'
+import type {DeveloperKey} from '../model/api/DeveloperKey'
 
 type Props = {
   dispatch: Function

ui/features/developer_keys_v2/react/NewKeyModal.tsx

@@ -30,7 +30,7 @@ import type {AvailableScope} from './reducers/listScopesReducer'
 import type {DeveloperKeyCreateOrEditState} from './reducers/createOrEditReducer'
 import actions from './actions/developerKeysActions'
 import type {AnyAction, Dispatch} from 'redux'
-import type {DeveloperKey} from '../model/DeveloperKey'
+import type {DeveloperKey} from '../model/api/DeveloperKey'
 
 const I18n = useI18nScope('react_developer_keys')
 
@@ -48,7 +48,7 @@ type Props = {
   }
   actions: typeof actions
   selectedScopes: Array<string>
-  handleSuccessfulSave: () => void
+  handleSuccessfulSave: (warningMessage?: string) => void
 }
 
 type ConfigurationMethod = 'manual' | 'json' | 'url'
@@ -216,10 +216,10 @@ export default class DeveloperKeyModal extends React.Component<Props, State> {
       .updateLtiKey(developerKey, [], this.developerKey.id, settings, settings.custom_fields)
       .then(data => {
         this.setState({isSaving: false})
-        const {developer_key, tool_configuration} = data
+        const {developer_key, tool_configuration, warning_message} = data
         developer_key.tool_configuration = tool_configuration.settings
         dispatch(actions.listDeveloperKeysReplace(developer_key))
-        this.props.handleSuccessfulSave()
+        this.props.handleSuccessfulSave(warning_message)
         this.closeModal()
       })
       .catch(() => {
@@ -286,9 +286,9 @@ export default class DeveloperKeyModal extends React.Component<Props, State> {
       return actions
         .saveLtiToolConfiguration(toSave)(dispatch)
         .then(
-          () => {
+          data => {
             this.setState({isSaving: false})
-            this.props.handleSuccessfulSave()
+            this.props.handleSuccessfulSave(data.warning_message)
             this.closeModal()
           },
           () => this.setState({isSaving: false})

ui/features/developer_keys_v2/react/RegistrationSettings/RegistrationSettings.tsx

@@ -23,7 +23,7 @@ import {Spinner} from '@instructure/ui-spinner'
 import {View} from '@instructure/ui-view'
 import page from 'page'
 import * as React from 'react'
-import type {DeveloperKey} from '../../model/DeveloperKey'
+import type {DeveloperKey} from '../../model/api/DeveloperKey'
 import type {LtiRegistration} from '../../model/LtiRegistration'
 import {updateRegistrationOverlay} from '../dynamic_registration/registrationApi'
 import {RegistrationOverlayForm} from './RegistrationOverlayForm'

ui/features/developer_keys_v2/react/__tests__/NewKeyModal.test.jsx

@@ -347,7 +347,7 @@ describe('a11y checks', () => {
       successfulSaveStub.mockClear()
     })
     describe('LTI Developer Key is being created', () => {
-      it('flashes an alert if the key saves successfully', async () => {
+      it('notifies if the key saves successfully', async () => {
         const saveLtiToolConfigurationStub = jest.fn().mockImplementation(() => {
           return () => {
             return Promise.resolve({
@@ -369,7 +369,7 @@ describe('a11y checks', () => {
         wrapper.unmount()
       })
 
-      it("doesn't flash an alert if the key fails to be created", async () => {
+      it("doesn't notify if the key fails to be created", async () => {
         const saveLtiToolConfigurationStub = jest.fn().mockImplementation(() => {
           return () => {
             return Promise.reject(new Error('testing'))
@@ -387,10 +387,33 @@ describe('a11y checks', () => {
         expect(successfulSaveStub).not.toHaveBeenCalled()
         wrapper.unmount()
       })
+
+      it('notifies and forwards if the API returns a warning_message', async () => {
+        const warning_message = 'This is a warning message'
+        const saveLtiToolConfigurationStub = jest.fn().mockImplementation(() => {
+          return () => {
+            return Promise.resolve({
+              developer_key: developerKey,
+              tool_configuration: validToolConfig,
+              warning_message,
+            })
+          }
+        })
+
+        const wrapper = createWrapper(
+          {isLtiKey: true},
+          {saveLtiToolConfiguration: saveLtiToolConfigurationStub}
+        )
+
+        await wrapper.instance().saveLtiToolConfiguration()
+
+        expect(saveLtiToolConfigurationStub).toHaveBeenCalled()
+        expect(successfulSaveStub).toHaveBeenCalledWith(warning_message)
+      })
     })
 
     describe('LTI Developer Key is being edited', () => {
-      it('flashes an alert if the key saves successfully', async () => {
+      it('notifies if the key saves successfully', async () => {
         const updateLtiKeyStub = jest.fn().mockResolvedValue({
           developer_key: developerKey,
           tool_configuration: validToolConfig,
@@ -407,7 +430,7 @@ describe('a11y checks', () => {
         wrapper.unmount()
       })
 
-      it("doesn't try to flash an alert if the key fails to save", async () => {
+      it("doesn't notifiy if the key fails to save", async () => {
         const updateLtiKeyStub = jest.fn().mockRejectedValue(null)
         const wrapper = createWrapper({}, {updateLtiKey: updateLtiKeyStub})
 
@@ -419,6 +442,24 @@ describe('a11y checks', () => {
         expect(successfulSaveStub).not.toHaveBeenCalled()
         wrapper.unmount()
       })
+
+      it('notifies if the API returns a warning_message', async () => {
+        const warning_message = 'This is a warning message'
+        const updateLtiKeyStub = jest.fn().mockResolvedValue({
+          developer_key: developerKey,
+          tool_configuration: validToolConfig,
+          warning_message,
+        })
+
+        const wrapper = createWrapper({isLtiKey: true}, {updateLtiKey: updateLtiKeyStub})
+
+        await wrapper
+          .instance()
+          .saveLTIKeyEdit(validToolConfig.extensions[0].settings, developerKey)
+
+        expect(updateLtiKeyStub).toHaveBeenCalled()
+        expect(successfulSaveStub).toHaveBeenCalledWith(warning_message)
+      })
     })
 
     // Turns out, for API keys, there's no difference in the code path taken,

ui/features/developer_keys_v2/react/actions/developerKeysActions.ts

@@ -23,10 +23,16 @@ import {LtiScope} from 'features/developer_keys_v2/model/LtiScopes'
 import $ from 'jquery'
 import parseLinkHeader from 'link-header-parsing/parseLinkHeader'
 import {AnyAction, Dispatch} from 'redux'
-import {DeveloperKey, DeveloperKeyAccountBinding} from '../../model/DeveloperKey'
+import {DeveloperKey, DeveloperKeyAccountBinding} from '../../model/api/DeveloperKey'
+import type {LtiToolConfiguration} from 'features/developer_keys_v2/model/api/LtiToolConfiguration'
 
 const I18n = useI18nScope('react_developer_keys')
 
+export type LtiDeveloperKeyApiResponse = {
+  developer_key: DeveloperKey
+  tool_configuration: LtiToolConfiguration
+  warning_message?: string
+}
 /**
  * Type function that takes a action type name in all caps and snake case
  * and converts it to a camel case action creator name.
@@ -598,7 +604,7 @@ export const actions = {
           newKey.tool_configuration = response.data.tool_configuration.settings
           dispatch(actions.setEditingDeveloperKey(newKey))
           dispatch(actions.listDeveloperKeysPrepend(newKey))
-          return response.data
+          return response.data as LtiDeveloperKeyApiResponse
         })
         .catch(err => {
           const errors = err.response.data.errors
@@ -640,7 +646,7 @@ export const actions = {
         },
       })
       .then(data => {
-        return data.data
+        return data.data as LtiDeveloperKeyApiResponse
       })
       .catch(err => {
         const errors = err.response.data.errors

ui/features/developer_keys_v2/react/reducers/createOrEditReducer.ts

@@ -16,7 +16,7 @@
  * with this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
-import type {DeveloperKey} from '../../model/DeveloperKey'
+import type {DeveloperKey} from '../../model/api/DeveloperKey'
 import ACTION_NAMES from '../actions/developerKeysActions'
 import {makeReducer} from './makeReducer'
 

ui/features/developer_keys_v2/react/reducers/listDeveloperKeysReducer.ts

@@ -17,7 +17,7 @@
  */
 
 import ACTION_NAMES from '../actions/developerKeysActions'
-import type {DeveloperKey} from '../../model/DeveloperKey'
+import type {DeveloperKey} from '../../model/api/DeveloperKey'
 import _ from 'lodash'
 import {makeReducer} from './makeReducer'