ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Add manual edit for public jwk url 

refs PLAT-4493

Test Plan:
-create a developer key and validate the public jwk url field
	is present
-save developer key
-edit developer key and validate public jwk url field is present
-save edit and validate edits were saved to developer key

Change-Id: I9019d116ad9995931757439f4c3d63b3d67a3a5f
Reviewed-on: https://gerrit.instructure.com/197713
Tested-by: Jenkins
Reviewed-by: Marc Phillips <mphillips@instructure.com>
QA-Review: Marc Phillips <mphillips@instructure.com>
Product-Review: Jesse Poulos <jpoulos@instructure.com>
This commit is contained in:
Drake Harper 2019-06-14 11:49:13 -06:00
parent b3d4e32f0c
commit e0d184126a
8 changed files with 167 additions and 160 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
app/jsx/developer_keys/ManualConfigurationForm/RequiredValues.js
View File

@ -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 : []}
/>
<TextInput
name="public_jwk_url"
value={toolConfiguration.public_jwk_url}
label={I18n.t("* Public JWK URL")}
onChange={this.handlePublicJwkUrlChange}
messages={showMessages && !toolConfiguration.public_jwk_url ? validationMessage : []}
/>
<PresentationContent>
<hr />
</PresentationContent>

3
app/jsx/developer_keys/NewKeyModal.js
View File

@ -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() {

13
app/models/developer_key.rb
View File

@ -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)

79
app/models/lti/tool_configuration.rb
View File

@ -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

9
lib/schemas/lti/tool_configuration.rb
View File

@ -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,

3
spec/controllers/lti/tool_configurations_api_controller_spec.rb
View File

@ -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' }

29
spec/models/developer_key_spec.rb
View File

@ -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

179
spec/models/lti/tool_configuration_spec.rb
View File

@ -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