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

use canvashttp to pull external tool config from url 

test plan:
* should always receive the same error message
 when trying to create an external tool by config url
 whether using "localhost" or a non-existent url

closes #PLAT-4964

Change-Id: I3beea4eec4526ad9bab8feac11ee07534c67d9a1
Reviewed-on: https://gerrit.instructure.com/210754
Tested-by: Jenkins
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
QA-Review: Jeremy Putnam <jeremyp@instructure.com>
Product-Review: James Williams <jamesw@instructure.com>
This commit is contained in:
James Williams 2019-09-24 08:50:17 -06:00
parent cb3bb7600f
commit 7224326a7d
8 changed files with 32 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
app/models/context_external_tool.rb
View File

@ -299,6 +299,7 @@ class ContextExternalTool < ActiveRecord::Base
def process_extended_configuration
return unless (config_type == 'by_url' && config_url) || (config_type == 'by_xml' && config_xml)
tool_hash = nil
@config_errors = []
begin
converter = CC::Importer::BLTIConverter.new
if config_type == 'by_url'
@ -310,8 +311,6 @@ class ContextExternalTool < ActiveRecord::Base
tool_hash = {:error => e.message}
end
@config_errors = []
error_field = config_type == 'by_xml' ? 'config_xml' : 'config_url'
converter = CC::Importer::BLTIConverter.new
tool_hash = if config_type == 'by_url'
@ -331,7 +330,7 @@ class ContextExternalTool < ActiveRecord::Base
self.name = real_name unless real_name.blank?
rescue CC::Importer::BLTIConverter::CCImportError => e
@config_errors << [error_field, e.message]
rescue URI::Error
rescue URI::Error, CanvasHttp::Error
@config_errors << [:config_url, "Invalid URL"]
rescue ActiveRecord::RecordInvalid => e
@config_errors += Array(e.record.errors)

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

@ -82,8 +82,7 @@ module Lti
private
def self.retrieve_and_extract_configuration(url)
response = CC::Importer::BLTIConverter.new.fetch(url)
response = CanvasHttp.get(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
@ -113,8 +112,8 @@ module Lti
if configuration['public_jwk'].blank? && configuration['public_jwk_url'].blank?
errors.add(:lti_key, "tool configuration must have public jwk or public jwk url")
end
if configuration['public_jwk'].present?
jwk_schema_errors = Schemas::Lti::PublicJwk.simple_validation_errors(configuration['public_jwk'])
if configuration['public_jwk'].present?
jwk_schema_errors = Schemas::Lti::PublicJwk.simple_validation_errors(configuration['public_jwk'])
errors.add(:configuration, jwk_schema_errors) if jwk_schema_errors.present?
end
schema_errors = Schemas::Lti::ToolConfiguration.simple_validation_errors(configuration.compact)

4
gems/canvas_http/lib/canvas_http.rb
View File

@ -29,8 +29,8 @@ module CanvasHttp
@code = code
end
end
class RelativeUriError < ArgumentError; end
class InsecureUriError < ArgumentError; end
class RelativeUriError < CanvasHttp::Error; end
class InsecureUriError < CanvasHttp::Error; end
def self.put(*args, &block)
CanvasHttp.request(Net::HTTP::Put, *args, &block)

21
lib/cc/importer/blti_converter.rb
View File

@ -136,28 +136,9 @@ module CC::Importer
end
end
def fetch(url, limit = 10)
# You should choose better exception.
raise ArgumentError, 'HTTP redirect too deep' if limit == 0
uri = URI.parse(url)
http = Net::HTTP.new(uri.host, uri.port)
http.use_ssl = uri.scheme == 'https'
http.verify_mode = OpenSSL::SSL::VERIFY_NONE
request = Net::HTTP::Get.new(uri.request_uri)
response = http.request(request)
case response
when Net::HTTPRedirection then fetch(response['location'], limit - 1)
else
response
end
end
def retrieve_and_convert_blti_url(url)
begin
response = fetch(url)
response = CanvasHttp.get(url, redirect_limit: 10)
config_xml = response.body
convert_blti_xml(config_xml)
rescue Timeout::Error

2
lib/custom_validations.rb
View File

@ -27,7 +27,7 @@ module CustomValidations
value, uri = CanvasHttp.validate_url(value, allowed_schemes: allowed_schemes)
record.send("#{attr}=", value)
rescue URI::Error, ArgumentError
rescue CanvasHttp::Error, URI::Error, ArgumentError
record.errors.add attr, 'is not a valid URL'
end
end

19
spec/controllers/external_tools_controller_spec.rb
View File

@ -1037,7 +1037,7 @@ describe ExternalToolsController do
let(:params) { raise "Override in specs" }
before do
allow_any_instance_of(Net::HTTP).to receive(:request).and_return(xml_response)
allow(CanvasHttp).to receive(:get).and_return(xml_response)
ContextExternalTool.create!(
context: @course,
name: 'first tool',
@ -1323,7 +1323,7 @@ describe ExternalToolsController do
</cartridge_basiclti_link>
XML
obj = OpenStruct.new({:body => xml})
allow_any_instance_of(Net::HTTP).to receive(:request).and_return(obj)
allow(CanvasHttp).to receive(:get).and_return(obj)
post 'create', params: {:course_id => @course.id, :external_tool => {:name => "tool name", :url => "http://example.com", :consumer_key => "key", :shared_secret => "secret", :config_type => "by_url", :config_url => "http://config.example.com"}}, :format => "json"
expect(response).to be_successful
@ -1338,7 +1338,7 @@ describe ExternalToolsController do
end
it "should fail gracefully on invalid URL retrieval or timeouts" do
allow_any_instance_of(Net::HTTP).to receive(:request).and_raise(Timeout::Error)
allow(CanvasHttp).to receive(:get).and_raise(Timeout::Error)
user_session(@teacher)
xml = "bob"
post 'create', params: {:course_id => @course.id, :external_tool => {:name => "tool name", :url => "http://example.com", :consumer_key => "key", :shared_secret => "secret", :config_type => "by_url", :config_url => "http://config.example.com"}}, :format => "json"
@ -1348,6 +1348,19 @@ describe ExternalToolsController do
expect(json['errors']['config_url'][0]['message']).to eq I18n.t(:retrieve_timeout, 'could not retrieve configuration, the server response timed out')
end
it "should fail gracefully trying to retrieve from localhost" do
expect(CanvasHttp).to receive(:insecure_host?).with("localhost").and_return(true)
user_session(@teacher)
xml = "bob"
post 'create', params: {:course_id => @course.id, :external_tool => {:name => "tool name", :url => "http://example.com",
:consumer_key => "key", :shared_secret => "secret", :config_type => "by_url",
:config_url => "http://localhost:9001"}}, :format => "json"
expect(response).not_to be_successful
expect(assigns[:tool]).to be_new_record
json = json_parse(response.body)
expect(json['errors']['config_url'][0]['message']).to eq "Invalid URL"
end
context "navigation tabs caching" do
it "shouldn't clear the navigation tabs cache for non navigtaion tools" do
enable_cache do

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

@ -118,7 +118,7 @@ RSpec.describe Lti::ToolConfigurationsApiController, type: :controller do
context 'when the request does not time out' do
before do
allow_any_instance_of(Net::HTTP).to receive(:request).and_return(ok_response)
allow(CanvasHttp).to receive(:get).and_return(ok_response)
end
it 'uses the tool configuration JSON from the settings_url' do
@ -146,7 +146,7 @@ RSpec.describe Lti::ToolConfigurationsApiController, type: :controller do
context 'when the request times out' do
before do
allow_any_instance_of(Net::HTTP).to receive(:request).and_raise(Timeout::Error)
allow(CanvasHttp).to receive(:get).and_raise(Timeout::Error)
end
it { is_expected.to have_http_status :unprocessable_entity }
@ -165,7 +165,7 @@ RSpec.describe Lti::ToolConfigurationsApiController, type: :controller do
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)
allow(CanvasHttp).to receive(:get).and_return(stubbed_response)
end
context 'when the response is "not found"' do
@ -301,7 +301,7 @@ RSpec.describe Lti::ToolConfigurationsApiController, type: :controller do
it { is_expected.to be_nil }
end
context 'when both the public jwk and public jwk url are missing' do
context 'when both the public jwk and public jwk url are missing' do
let(:public_jwk) { nil }
let(:settings) do
s = super()

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

@ -427,7 +427,7 @@ module Lti
end
before do
allow_any_instance_of(Net::HTTP).to receive(:request).and_return(stubbed_response)
allow(CanvasHttp).to receive(:get).and_return(stubbed_response)
end
it 'fetches JSON from the URL' do
@ -435,7 +435,7 @@ module Lti
end
context 'when a timeout occurs' do
before { allow_any_instance_of(Net::HTTP).to receive(:request).and_raise(Timeout::Error) }
before { allow(CanvasHttp).to receive(:get).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./
@ -448,7 +448,7 @@ module Lti
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)
allow(CanvasHttp).to receive(:get).and_return(stubbed_response)
end
context 'when the response is "not found"' do