Remove extraneous fields from IMS Registrations

Additionally, mark registrations as soft deletable.

why:
- previously, we were validating that application type, grant_types,
  response_types, and token_endpoint_auth_method are all the same for
  every dynamic registration request. We don't need to store these
  fields.
- We were previously hard deleting IMS Registrations, which could cause
  some unexpected headaches.

closes INTEROP-8503

test-plan:
- Pull the commit down.
- Run the migrations and ensure all is well.
- Ensure that Lti::IMS::Registrations are soft deletable:
```ruby
reg = Lti::IMS::Registration.first
reg.destroy
reg.reload.workflow_state == "deleted"
reg.update!(workflow_state: "active")
```
- Run through the dynamic reg process and ensure that all goes smoothly.
- Revert all of the migrations and ensure that all is well.

Change-Id: I2e92ce36d3b719e072bb78e3a79d6940e7131fbf
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/346408
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Migration-Review: Cody Cutrer <cody@instructure.com>
Reviewed-by: Xander Moffatt <xmoffatt@instructure.com>
QA-Review: Xander Moffatt <xmoffatt@instructure.com>
Product-Review: Ryan Hawkins <ryan.hawkins@instructure.com>

app/controllers/lti/ims/dynamic_registration_controller.rb

@@ -120,13 +120,19 @@ module Lti
           return
         end
 
-        root_account.shard.activate do
         registration_params = params.permit(*expected_registration_params)
         registration_params["lti_tool_configuration"] = registration_params["https://purl.imsglobal.org/spec/lti-tool-configuration"]
         registration_params.delete("https://purl.imsglobal.org/spec/lti-tool-configuration")
         scopes = registration_params["scope"].split
         registration_params.delete("scope")
+        error_messages = validate_registration_params(registration_params)
 
+        if error_messages.present?
+          render status: :unprocessable_entity, json: { errors: error_messages }
+          return
+        end
+
+        root_account.shard.activate do
           developer_key = DeveloperKey.new(
             name: registration_params["client_name"],
             account: root_account.site_admin? ? nil : root_account,
@@ -164,15 +170,15 @@ module Lti
       def render_registration(registration, developer_key)
         render json: {
           client_id: developer_key.global_id.to_s,
-          application_type: registration.application_type,
+          application_type: Lti::IMS::Registration::REQUIRED_APPLICATION_TYPE,
-          grant_types: registration.grant_types,
+          grant_types: Lti::IMS::Registration::REQUIRED_GRANT_TYPES,
           initiate_login_uri: registration.initiate_login_uri,
           redirect_uris: registration.redirect_uris,
-          response_types: registration.response_types,
+          response_types: Lti::IMS::Registration::REQUIRED_RESPONSE_TYPES,
           client_name: registration.client_name,
           jwks_uri: registration.jwks_uri,
           logo_uri: developer_key.icon_url,
-          token_endpoint_auth_method: registration.token_endpoint_auth_method,
+          token_endpoint_auth_method: Lti::IMS::Registration::REQUIRED_TOKEN_ENDPOINT_AUTH_METHOD,
           scope: registration.scopes.join(" "),
           "https://purl.imsglobal.org/spec/lti-tool-configuration": registration.lti_tool_configuration.merge(
             {
@@ -183,12 +189,35 @@ module Lti
       end
 
       def respond_with_error(status_code, message)
-        head status_code
+        render status: status_code,
-        render json: {
+               json: {
                  errorMessage: message
                }
       end
 
+      def validate_registration_params(registration_params)
+        grant_types = registration_params.delete("grant_types") || []
+        response_types = registration_params.delete("response_types") || []
+        application_type = registration_params.delete("application_type")
+        token_endpoint_auth_method = registration_params.delete("token_endpoint_auth_method")
+        errors = []
+        if (Lti::IMS::Registration::REQUIRED_GRANT_TYPES - grant_types).present?
+          errors << { field: :grant_types, message: "Must include #{Lti::IMS::Registration::REQUIRED_GRANT_TYPES.join(", ")}" }
+        end
+        if (Lti::IMS::Registration::REQUIRED_RESPONSE_TYPES - response_types).present?
+          errors << { field: :response_types, message: "Must include #{Lti::IMS::Registration::REQUIRED_RESPONSE_TYPES.join(", ")}" }
+        end
+
+        if token_endpoint_auth_method != Lti::IMS::Registration::REQUIRED_TOKEN_ENDPOINT_AUTH_METHOD
+          errors << { field: :token_endpoint_auth_method, message: "Must be 'private_key_jwt'" }
+        end
+
+        if application_type != Lti::IMS::Registration::REQUIRED_APPLICATION_TYPE
+          errors << { field: :application_type, message: "Must be 'web'" }
+        end
+        errors
+      end
+
       def require_dynamic_registration_flag
         unless account_context.feature_enabled? :lti_dynamic_registration
           render status: :not_found, template: "shared/errors/404_message"

app/models/lti/ims/registration.rb

@@ -18,6 +18,7 @@
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 
 class Lti::IMS::Registration < ApplicationRecord
+  include Canvas::SoftDeletable
   extend RootAccountResolver
   CANVAS_EXTENSION_LABEL = "canvas.instructure.com"
   self.table_name = "lti_ims_registrations"
@@ -31,19 +32,16 @@ class Lti::IMS::Registration < ApplicationRecord
 
   PLACEMENT_VISIBILITY_OPTIONS = %(admins members public)
 
-  validates :application_type,
+  self.ignored_columns += %i[application_type grant_types response_types token_endpoint_auth_method]
-            :grant_types,
+
-            :response_types,
+  validates :redirect_uris,
-            :redirect_uris,
             :initiate_login_uri,
             :client_name,
             :jwks_uri,
-            :token_endpoint_auth_method,
             :lti_tool_configuration,
             presence: true
 
-  validate :required_values_are_present,
+  validate :redirect_uris_contains_uris,
-           :redirect_uris_contains_uris,
            :lti_tool_configuration_is_valid,
            :scopes_are_valid
 
@@ -238,15 +236,15 @@ class Lti::IMS::Registration < ApplicationRecord
       developer_key_id: developer_key.global_id.to_s,
       overlay: registration_overlay,
       lti_tool_configuration:,
-      application_type:,
+      application_type: REQUIRED_APPLICATION_TYPE,
-      grant_types:,
+      grant_types: REQUIRED_GRANT_TYPES,
-      response_types:,
+      response_types: REQUIRED_RESPONSE_TYPES,
       redirect_uris:,
       initiate_login_uri:,
       client_name:,
       jwks_uri:,
       logo_uri:,
-      token_endpoint_auth_method:,
+      token_endpoint_auth_method: REQUIRED_TOKEN_ENDPOINT_AUTH_METHOD,
       contacts:,
       client_uri:,
       policy_uri:,
@@ -262,23 +260,6 @@ class Lti::IMS::Registration < ApplicationRecord
 
   private
 
-  def required_values_are_present
-    if (REQUIRED_GRANT_TYPES - grant_types).present?
-      errors.add(:grant_types, "Must include #{REQUIRED_GRANT_TYPES.join(", ")}")
-    end
-    if (REQUIRED_RESPONSE_TYPES - response_types).present?
-      errors.add(:response_types, "Must include #{REQUIRED_RESPONSE_TYPES.join(", ")}")
-    end
-
-    if token_endpoint_auth_method != REQUIRED_TOKEN_ENDPOINT_AUTH_METHOD
-      errors.add(:token_endpoint_auth_method, "Must be 'private_key_jwt'")
-    end
-
-    if application_type != REQUIRED_APPLICATION_TYPE
-      errors.add(:application_type, "Must be 'web'")
-    end
-  end
-
   def redirect_uris_contains_uris
     return if redirect_uris.all? { |uri| uri.match? URI::DEFAULT_PARSER.make_regexp(["http", "https"]) }
 

db/migrate/20240429163358_add_workflow_state_to_lti_ims_registrations.rb

@@ -0,0 +1,25 @@
+# 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/>.
+
+class AddWorkflowStateToLtiIMSRegistrations < ActiveRecord::Migration[7.0]
+  tag :predeploy
+  def change
+    add_column :lti_ims_registrations, :workflow_state, :string, limit: 255, default: "active"
+  end
+end

db/migrate/20240429194145_make_extraneous_ims_registration_fields_nullable.rb

@@ -0,0 +1,30 @@
+# 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/>.
+
+class MakeExtraneousIMSRegistrationFieldsNullable < ActiveRecord::Migration[7.0]
+  tag :predeploy
+  def change
+    change_table :lti_ims_registrations do |t|
+      t.change_null :application_type, true
+      t.change_null :grant_types, true
+      t.change_null :response_types, true
+      t.change_null :token_endpoint_auth_method, true
+    end
+  end
+end

db/migrate/20240429194501_remove_extraneous_ims_registration_fields.rb

@@ -0,0 +1,30 @@
+# 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/>.
+
+class RemoveExtraneousIMSRegistrationFields < ActiveRecord::Migration[7.0]
+  tag :postdeploy
+  def change
+    change_table :lti_ims_registrations, bulk: true do |t|
+      t.remove :application_type, type: :string
+      t.remove :grant_types, type: :string, array: true
+      t.remove :response_types, type: :string, array: true
+      t.remove :token_endpoint_auth_method, type: :string
+    end
+  end
+end

spec/apis/v1/developer_keys_spec.rb

@@ -171,6 +171,7 @@ describe DeveloperKeysController, type: :request do
                         format: "json",
                         account_id: account.id
                       })
+      expect(response).to be_successful
 
       key_json = json.detect { |r| r["id"] == developer_key.global_id }
       expect(key_json["lti_registration"]).to eq ims_registration.as_json

spec/controllers/lti/ims/dynamic_registration_controller_spec.rb

@@ -162,6 +162,7 @@ describe Lti::IMS::DynamicRegistrationController do
           post :create, params: invalid_registration_params
         end
 
+        context "with invalid grant types" do
           let(:invalid_registration_params) do
             wrong_grant_types = registration_params
             wrong_grant_types["grant_types"] = ["not_part_of_the_spec", "implicit"]
@@ -175,9 +176,44 @@ describe Lti::IMS::DynamicRegistrationController do
           end
 
           it "doesn't create a stray developer key" do
-          previous_key_count = DeveloperKey.count
+            expect { subject }.not_to change { DeveloperKey.count }
+          end
+        end
+
+        context "with invalid response types" do
+          let(:invalid_registration_params) do
+            wrong_response_types = registration_params
+            wrong_response_types["response_types"] = ["not_part_of_the_spec"]
+            wrong_response_types
+          end
+
+          it "returns a 422 with validation errors" do
             subject
-          expect(DeveloperKey.count).to eq(previous_key_count)
+            expect(response).to have_http_status(:unprocessable_entity)
+            expect(response.body).to include("Must include id_token")
+          end
+
+          it "doesn't create a stray developer key" do
+            expect { subject }.not_to change { DeveloperKey.count }
+          end
+        end
+
+        context "with invalid token endpoint auth method" do
+          let(:invalid_registration_params) do
+            wrong_token_endpoint_auth_method = registration_params
+            wrong_token_endpoint_auth_method["token_endpoint_auth_method"] = "not_part_of_the_spec"
+            wrong_token_endpoint_auth_method
+          end
+
+          it "returns a 422 with validation errors" do
+            subject
+            expect(response).to have_http_status(:unprocessable_entity)
+            expect(response.body).to include("Must be 'private_key_jwt'")
+          end
+
+          it "doesn't create a stray developer key" do
+            expect { subject }.not_to change { DeveloperKey.count }
+          end
         end
       end
     end

spec/factories/lti_ims_registration_factory.rb

@@ -20,19 +20,14 @@
 module Factories
   LTI_IMS_REGISTRATION_BASE_ATTRS =
     {
-      application_type: "web",
       guid: "6378c81b-5996-4754-b850-5de78e6d22f4",
       client_name: "Test Dynamic Registration",
       client_uri: "https://example.com",
-      grant_types: %w[client_credentials implicit],
       jwks_uri: "https://example.com/api/registrations/3/jwks",
       initiate_login_uri: "https://example.com/api/registrations/3/login",
       redirect_uris: [
         "https://example.com/api/registrations/3/launch"
       ],
-      response_types: [
-        "id_token"
-      ],
       scopes: %w[
         https://purl.imsglobal.org/spec/lti-ags/scope/lineitem
         https://purl.imsglobal.org/spec/lti-ags/scope/lineitem.readonly
@@ -43,7 +38,6 @@ module Factories
         https://canvas.instructure.com/lti/account_lookup/scope/show
         https://canvas.instructure.com/lti-ags/progress/scope/show
       ],
-      token_endpoint_auth_method: "private_key_jwt",
       logo_uri: "https://example.com/api/apps/1/icon.svg",
       lti_tool_configuration: {
         claims: %w[

spec/models/lti/ims/registration_spec.rb

@@ -20,9 +20,6 @@
 
 module Lti::IMS
   describe Registration do
-    let(:application_type) { :web }
-    let(:grant_types) { [:client_credentials, :implicit] }
-    let(:response_types) { [:id_token] }
     let(:redirect_uris) { ["http://example.com"] }
     let(:initiate_login_uri) { "http://example.com/login" }
     let(:client_name) { "Example Tool" }
@@ -31,7 +28,6 @@ module Lti::IMS
     let(:client_uri) { "http://example.com/" }
     let(:tos_uri) { "http://example.com/tos" }
     let(:policy_uri) { "http://example.com/policy" }
-    let(:token_endpoint_auth_method) { "private_key_jwt" }
     let(:lti_tool_configuration) do
       {
         domain: "example.com",
@@ -43,9 +39,6 @@ module Lti::IMS
 
     let(:registration) do
       r = Registration.new({
-        application_type:,
-        grant_types:,
-        response_types:,
         redirect_uris:,
         initiate_login_uri:,
         client_name:,
@@ -54,7 +47,6 @@ module Lti::IMS
         client_uri:,
         tos_uri:,
         policy_uri:,
-        token_endpoint_auth_method:,
         lti_tool_configuration:,
         scopes:
       }.compact)
@@ -63,6 +55,11 @@ module Lti::IMS
     end
     let(:developer_key) { DeveloperKey.create }
 
+    it "is soft_deleted when destroy is called" do
+      registration.destroy
+      expect(registration.reload.workflow_state).to eq("deleted")
+    end
+
     describe "associations" do
       subject { Lti::IMS::Registration.new }
       it { is_expected.to belong_to(:lti_registration).class_name("Lti::Registration") }
@@ -76,64 +73,6 @@ module Lti::IMS
         it { is_expected.to be true }
       end
 
-      context "application_type" do
-        context "is \"web\"" do
-          it { is_expected.to be true }
-        end
-
-        context "is not \"web\"" do
-          let(:application_type) { "native" }
-
-          it { is_expected.to be false }
-        end
-
-        context "is not included" do
-          let(:application_type) { nil }
-
-          it { is_expected.to be false }
-        end
-      end
-
-      context "grant_types" do
-        context "includes other types" do
-          let(:grant_types) { %i[client_credentials implicit foo bar] }
-
-          it { is_expected.to be true }
-        end
-
-        context "does not include implicit" do
-          let(:grant_types) { [:client_credentials, :foo] }
-
-          it { is_expected.to be false }
-        end
-
-        context "does not include client_credentials" do
-          let(:grant_types) { [:implicit, :foo] }
-
-          it { is_expected.to be false }
-        end
-      end
-
-      context "response_types" do
-        context "includes other types" do
-          let(:response_types) { %i[id_token foo bar] }
-
-          it { is_expected.to be true }
-        end
-
-        context "is not included" do
-          let(:response_types) { nil }
-
-          it { is_expected.to be false }
-        end
-
-        context "does not include id_token" do
-          let(:response_types) { [:foo, :bar] }
-
-          it { is_expected.to be false }
-        end
-      end
-
       context "redirect_uris" do
         context "includes valid uris" do
           let(:redirect_uris) { ["https://example.com", "https://example.com/foo"] }
@@ -196,14 +135,6 @@ module Lti::IMS
         end
       end
 
-      context "token_endpoint_auth_method" do
-        context "is not \"private_key_jwt\"" do
-          let(:token_endpoint_auth_method) { "asdf" }
-
-          it { is_expected.to be false }
-        end
-      end
-
       context "logo_uri" do
         context "is not a valid uri" do
           let(:logo_uri) { "asdf" }