diff --git a/app/jsx/developer_keys/ManualConfigurationForm/RequiredValues.js b/app/jsx/developer_keys/ManualConfigurationForm/RequiredValues.js index 9f34a98d8a5..7b708fe2b9d 100644 --- a/app/jsx/developer_keys/ManualConfigurationForm/RequiredValues.js +++ b/app/jsx/developer_keys/ManualConfigurationForm/RequiredValues.js @@ -92,6 +92,11 @@ export default class RequiredValues extends React.Component { this.setState(state => ({toolConfiguration: {...state.toolConfiguration, public_jwk: value}})) } + handlePublicJwkUrlChange = e => { + const value = e.target.value; + this.setState(state => ({toolConfiguration: {...state.toolConfiguration, public_jwk_url: value}})) + } + render() { const { toolConfiguration } = this.state; const { showMessages } = this.props @@ -156,6 +161,13 @@ export default class RequiredValues extends React.Component { onChange={this.handlePublicJwkChange} messages={showMessages && !toolConfiguration.public_jwk ? validationMessage : []} /> +
diff --git a/app/jsx/developer_keys/NewKeyModal.js b/app/jsx/developer_keys/NewKeyModal.js index 4a66935e997..c288ddbe0b5 100644 --- a/app/jsx/developer_keys/NewKeyModal.js +++ b/app/jsx/developer_keys/NewKeyModal.js @@ -63,7 +63,8 @@ export default class DeveloperKeyModal extends React.Component { get toolConfiguration () { const public_jwk = this.developerKey.public_jwk ? { public_jwk: this.developerKey.public_jwk } : {} - return {...(this.developerKey.tool_configuration || {}), ...this.state.toolConfiguration, ...public_jwk} + const public_jwk_url = this.developerKey.public_jwk_url ? { public_jwk_url: this.developerKey.public_jwk_url } : {} + return {...(this.developerKey.tool_configuration || {}), ...this.state.toolConfiguration, ...public_jwk, ...public_jwk_url} } get isLtiKey() { diff --git a/app/models/developer_key.rb b/app/models/developer_key.rb index ab2f2320bf5..d8af487642d 100644 --- a/app/models/developer_key.rb +++ b/app/models/developer_key.rb @@ -34,11 +34,11 @@ class DeveloperKey < ActiveRecord::Base has_one :tool_configuration, class_name: 'Lti::ToolConfiguration', dependent: :destroy, inverse_of: :developer_key serialize :scopes, Array + before_validation :normalize_public_jwk_url before_validation :validate_scopes! before_create :generate_api_key before_create :set_auto_expire_tokens before_create :set_visible - before_create :infer_key_type before_save :nullify_empty_icon_url before_save :protect_default_key before_save :set_require_scopes @@ -50,6 +50,7 @@ class DeveloperKey < ActiveRecord::Base validates_as_url :redirect_uri, :oidc_initiation_url, :public_jwk_url, allowed_schemes: nil validate :validate_redirect_uris validate :validate_public_jwk + validate :validate_lti_fields attr_reader :private_jwk @@ -297,8 +298,14 @@ class DeveloperKey < ActiveRecord::Base private - def infer_key_type - self.is_lti_key = self.public_jwk.present? || self.public_jwk_url.present? + def validate_lti_fields + return unless self.is_lti_key? + return if self.public_jwk.present? || self.public_jwk_url.present? + errors.add(:lti_key, "must have public jwk or public jwk url") + end + + def normalize_public_jwk_url + self.public_jwk_url = nil if self.public_jwk_url.blank? end def manage_external_tools(enqueue_args, method, affected_account) diff --git a/app/models/lti/tool_configuration.rb b/app/models/lti/tool_configuration.rb index 8a5b69caa46..2aac4039297 100644 --- a/app/models/lti/tool_configuration.rb +++ b/app/models/lti/tool_configuration.rb @@ -22,7 +22,6 @@ module Lti belongs_to :developer_key - before_validation :store_configuration_from_url, only: :create before_save :normalize_configuration after_update :update_external_tools!, if: :update_external_tools? @@ -55,35 +54,53 @@ module Lti end def self.create_tool_config_and_key!(account, tool_configuration_params) - self.transaction do - dk = DeveloperKey.create!(account: (account.site_admin? ? nil : account)) - settings = tool_configuration_params[:settings]&.try(:to_unsafe_hash) || tool_configuration_params[:settings] + settings = if tool_configuration_params[:settings_url].present? && tool_configuration_params[:settings].blank? + retrieve_and_extract_configuration(tool_configuration_params[:settings_url]) + elsif tool_configuration_params[:settings].present? + tool_configuration_params[:settings]&.try(:to_unsafe_hash) || tool_configuration_params[:settings] + end - if settings.present? - self.create!( - developer_key: dk, - configuration: settings.deep_merge( - 'custom_fields' => ContextExternalTool.find_custom_fields_from_string(tool_configuration_params[:custom_fields]) - ), - disabled_placements: tool_configuration_params[:disabled_placements] - ) - else - # Creating config via URL - t = self.create!( - developer_key: dk, - configuration_url: tool_configuration_params[:settings_url], - disabled_placements: tool_configuration_params[:disabled_placements] - ) - t.update! configuration: t.configuration.deep_merge( + raise_error(:configuration, "Configuration must be present") if settings.blank? + self.transaction do + dk = DeveloperKey.create!( + account: (account.site_admin? ? nil : account), + is_lti_key: true, + public_jwk_url: settings[:public_jwk_url], + public_jwk: settings[:public_jwk] + ) + self.create!( + developer_key: dk, + configuration: settings.deep_merge( 'custom_fields' => ContextExternalTool.find_custom_fields_from_string(tool_configuration_params[:custom_fields]) - ) - t - end + ), + configuration_url: tool_configuration_params[:settings_url], + disabled_placements: tool_configuration_params[:disabled_placements] + ) end end private + def self.retrieve_and_extract_configuration(url) + + response = CC::Importer::BLTIConverter.new.fetch(url) + + raise_error(:configuration_url, 'Content type must be "application/json"') unless response['content-type'].include? 'application/json' + raise_error(:configuration_url, response.message) unless response.is_a? Net::HTTPSuccess + + JSON.parse(response.body).with_indifferent_access + rescue Timeout::Error + raise_error(:configuration_url, 'Could not retrieve settings, the server response timed out.') + end + private_class_method :retrieve_and_extract_configuration + + def self.raise_error(type, message) + tool_config_obj = self.new + tool_config_obj.errors.add(type, message) + raise ActiveRecord::RecordInvalid, tool_config_obj + end + private_class_method :raise_error + def update_external_tools? saved_change_to_settings? end @@ -132,22 +149,6 @@ module Lti extension end - def store_configuration_from_url - return if configuration_url.blank? || configuration.present? - - response = CC::Importer::BLTIConverter.new.fetch(configuration_url) - - errors.add(:configuration_url, 'Content type must be "application/json"') unless response['content-type'].include? 'application/json' - return if errors[:configuration_url].present? - - errors.add(:configuration_url, response.message) unless response.is_a? Net::HTTPSuccess - return if errors[:configuration_url].present? - - self.settings = JSON.parse(response.body) - rescue Timeout::Error - errors.add(:configuration_url, 'Could not retrieve settings, the server response timed out.') - end - def normalize_configuration self.configuration = JSON.parse(configuration) if configuration.is_a? String end diff --git a/lib/schemas/lti/tool_configuration.rb b/lib/schemas/lti/tool_configuration.rb index a782a45a1d0..c2f16767840 100644 --- a/lib/schemas/lti/tool_configuration.rb +++ b/lib/schemas/lti/tool_configuration.rb @@ -26,13 +26,18 @@ module Schemas::Lti # to be reenabled after scopes bug fix # "scopes", "target_link_uri", - "oidc_initiation_url", - "public_jwk" + "oidc_initiation_url" ].freeze, "properties" => { "title" => { "type" => "string" }.freeze, + "public_jwk_url" => { + "type" => "string" + }.freeze, + "is_lti_key" => { + "type" => "boolean" + }.freeze, "description" => { "type" => "string" }.freeze, diff --git a/spec/controllers/lti/tool_configurations_api_controller_spec.rb b/spec/controllers/lti/tool_configurations_api_controller_spec.rb index 942fc1fe892..f17c5738eb8 100644 --- a/spec/controllers/lti/tool_configurations_api_controller_spec.rb +++ b/spec/controllers/lti/tool_configurations_api_controller_spec.rb @@ -41,7 +41,8 @@ RSpec.describe Lti::ToolConfigurationsApiController, type: :controller do test_cluster_only: true, scopes: ['https://purl.imsglobal.org/spec/lti-ags/scope/lineitem'], require_scopes: true, - redirect_uris: "http://www.test.com\r\nhttp://www.anothertest.com" + redirect_uris: "http://www.test.com\r\nhttp://www.anothertest.com", + public_jwk_url: "https://www.test.com" } end let(:new_url) { 'https://www.new-url.com/test' } diff --git a/spec/models/developer_key_spec.rb b/spec/models/developer_key_spec.rb index 9ce8bc3b476..cbd501ab2c6 100644 --- a/spec/models/developer_key_spec.rb +++ b/spec/models/developer_key_spec.rb @@ -47,33 +47,18 @@ describe DeveloperKey do key_hash['kty'] = key_hash['kty'].to_s key_hash end - let(:developer_key_with_jwk) { DeveloperKey.create!(public_jwk: public_jwk) } - let(:developer_key_with_url) { DeveloperKey.create!(public_jwk_url: "https://hello.world.com") } - let(:developer_key_with_key_and_url) { DeveloperKey.create!(public_jwk: public_jwk, public_jwk_url: "https://hello.world.com") } - let(:developer_key_none) { DeveloperKey.create! } + let(:public_jwk_url) { "https://hello.world.com" } - context 'when public jwk is present' do - it 'is_lti_key should return true' do - expect(developer_key_with_jwk.is_lti_key).to eq true - end + it 'throws error if public jwk and public jwk are absent' do + expect { DeveloperKey.create!(is_lti_key: true) }.to raise_error ActiveRecord::RecordInvalid end - context 'when public jwk url is present' do - it 'is_lti_key should return true' do - expect(developer_key_with_url.is_lti_key).to eq true - end + it 'validates when public jwk is present' do + expect { DeveloperKey.create!(is_lti_key: true, public_jwk: public_jwk) }.to_not raise_error end - context 'when public jwk url and public jwk is not present' do - it 'is_lti_key should return false' do - expect(developer_key_none.is_lti_key).to eq false - end - end - - context 'when public jwk url and public jwk is present' do - it 'is_lti_key should return true' do - expect(developer_key_with_key_and_url.is_lti_key).to eq true - end + it 'validates when public jwk url is present' do + expect { DeveloperKey.create!(is_lti_key: true, public_jwk_url: public_jwk_url) }.to_not raise_error end end diff --git a/spec/models/lti/tool_configuration_spec.rb b/spec/models/lti/tool_configuration_spec.rb index 87b5ced5743..c7218988e6e 100644 --- a/spec/models/lti/tool_configuration_spec.rb +++ b/spec/models/lti/tool_configuration_spec.rb @@ -133,97 +133,6 @@ module Lti end end - describe 'before_validation' do - subject do - tool_configuration.validate - tool_configuration - end - - context 'when "settings_url" is present' do - let(:url) { 'https://www.mytool.com/config/json' } - let(:stubbed_response) do - double( - body: settings.to_json, - '[]' => 'application/json;', - is_a?: true - ) - end - - before do - tool_configuration.settings = nil - tool_configuration.settings_url = url - tool_configuration.developer_key = developer_key - allow_any_instance_of(Net::HTTP).to receive(:request).and_return(stubbed_response) - end - - it 'fetches JSON from the URL' do - expect(subject.settings['target_link_uri']).to eq settings['target_link_uri'] - end - - context 'when a timeout occurs' do - before { allow_any_instance_of(Net::HTTP).to receive(:request).and_raise(Timeout::Error) } - - it 'raises exception if timeout occurs' do - expect(subject.errors[:configuration_url]).to include 'Could not retrieve settings, the server response timed out.' - end - end - - context 'when the response is not a success' do - let(:stubbed_response) { double() } - - before do - allow(stubbed_response).to receive(:is_a?).with(Net::HTTPSuccess).and_return false - allow(stubbed_response).to receive('[]').and_return('application/json') - allow_any_instance_of(Net::HTTP).to receive(:request).and_return(stubbed_response) - end - - context 'when the response is "not found"' do - before do - allow(stubbed_response).to receive(:message).and_return('Not found') - allow(stubbed_response).to receive(:code).and_return('404') - end - - it 'adds a "not found error to the model' do - expect(subject.errors[:configuration_url]).to include 'Not found' - end - end - - context 'when the response is "unauthorized"' do - before do - allow(stubbed_response).to receive(:message).and_return('Unauthorized') - allow(stubbed_response).to receive(:code).and_return('401') - end - - it 'adds a "unauthorized error to the model' do - expect(subject.errors[:configuration_url]).to include 'Unauthorized' - end - end - - context 'when the response is "internal server error"' do - before do - allow(stubbed_response).to receive(:message).and_return('Internal server error') - allow(stubbed_response).to receive(:code).and_return('500') - end - - it 'adds a "internal server error to the model' do - expect(subject.errors[:configuration_url]).to include 'Internal server error' - end - end - - context 'when the response is not JSON' do - before do - allow(stubbed_response).to receive('[]').and_return('text/html') - allow(stubbed_response).to receive(:is_a?).with(Net::HTTPSuccess).and_return true - end - - it 'adds an error to the model' do - expect(subject.errors[:configuration_url]).to include 'Content type must be "application/json"' - end - end - end - end - end - describe 'after_update' do subject { tool_configuration.update!(changes) } @@ -425,7 +334,7 @@ module Lti let_once(:account) { Account.create! } let(:params) do { - settings: settings + settings: settings.with_indifferent_access } end let(:tool_configuration) { described_class.create_tool_config_and_key!(account, params) } @@ -456,6 +365,92 @@ module Lti expect(DeveloperKey.where(account: account).count).to eq 0 end end + + context 'when settings_url is present' do + let(:params) do + { + settings_url: url + } + end + let(:url) { 'https://www.mytool.com/config/json' } + let(:stubbed_response) do + double( + body: settings.to_json, + '[]' => 'application/json;', + is_a?: true + ) + end + + before do + allow_any_instance_of(Net::HTTP).to receive(:request).and_return(stubbed_response) + end + + it 'fetches JSON from the URL' do + expect(tool_configuration.settings['target_link_uri']).to eq settings['target_link_uri'] + end + + context 'when a timeout occurs' do + before { allow_any_instance_of(Net::HTTP).to receive(:request).and_raise(Timeout::Error) } + + it 'raises exception if timeout occurs' do + expect{ tool_configuration }.to raise_error /Could not retrieve settings, the server response timed out./ + end + end + + context 'when the response is not a success' do + let(:stubbed_response) { double() } + + before do + allow(stubbed_response).to receive(:is_a?).with(Net::HTTPSuccess).and_return false + allow(stubbed_response).to receive('[]').and_return('application/json') + allow_any_instance_of(Net::HTTP).to receive(:request).and_return(stubbed_response) + end + + context 'when the response is "not found"' do + before do + allow(stubbed_response).to receive(:message).and_return('Not found') + allow(stubbed_response).to receive(:code).and_return('404') + end + + it 'adds a "not found error to the model' do + expect{ tool_configuration }.to raise_error /Not found/ + end + end + + context 'when the response is "unauthorized"' do + before do + allow(stubbed_response).to receive(:message).and_return('Unauthorized') + allow(stubbed_response).to receive(:code).and_return('401') + end + + it 'adds a "unauthorized error to the model' do + expect{ tool_configuration }.to raise_error /Unauthorized/ + end + end + + context 'when the response is "internal server error"' do + before do + allow(stubbed_response).to receive(:message).and_return('Internal server error') + allow(stubbed_response).to receive(:code).and_return('500') + end + + it 'adds a "internal server error to the model' do + expect{ tool_configuration }.to raise_error /Internal server error/ + end + end + + context 'when the response is not JSON' do + before do + allow(stubbed_response).to receive('[]').and_return('text/html') + allow(stubbed_response).to receive(:is_a?).with(Net::HTTPSuccess).and_return true + end + + it 'adds an error to the model' do + expect{ tool_configuration }.to raise_error /Content type must be \"application\/json\"/ + end + end + end + end end end end