Skip to content

Commit 5dcf89b

Browse files
rivernaterivernate
committed
use the extracted lti security code
fixes PLAT-1570 test plan: enable the disable_lti_post_only feature flag install the test tool create a launch with a query param launch to newly created launch endpoint it should launch correctly regression test lti launches in canvas Change-Id: I624ede3fd579a8ad7a2e6bcaa340e0b9490b9357 Reviewed-on: https://gerrit.instructure.com/81756 Tested-by: Jenkins Reviewed-by: Brad Horrocks <bhorrocks@instructure.com> QA-Review: August Thornton <august@instructure.com> Product-Review: Nathan Mills <nathanm@instructure.com>
1 parent 7a058b7 commit 5dcf89b

5 files changed

Lines changed: 34 additions & 186 deletions

File tree

‎app/controllers/external_tools_controller.rb‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -467,8 +467,13 @@ def content_item_selection(tool, placement, message_type, opts = {})
467467

468468
lti_launch = @tool.settings['post_only'] ? Lti::Launch.new(post_only: true) : Lti::Launch.new
469469
lti_launch.resource_url = opts[:launch_url] || tool.extension_setting(placement, :url)
470-
lti_launch.params = LtiOutbound::ToolLaunch.generate_params(params, lti_launch.resource_url, tool.consumer_key, tool.shared_secret,
471-
disable_lti_post_only: @context.root_account.feature_enabled?(:disable_lti_post_only))
470+
lti_launch.params = Lti::Security.signed_post_params(
471+
params,
472+
lti_launch.resource_url,
473+
tool.consumer_key,
474+
tool.shared_secret,
475+
@context.root_account.feature_enabled?(:disable_lti_post_only)
476+
)
472477
lti_launch.link_text = tool.label_for(placement.to_sym)
473478
lti_launch.analytics_id = tool.tool_id
474479

@@ -545,8 +550,13 @@ def content_item_selection_request(tool, placement, opts = {})
545550

546551
lti_launch = @tool.settings['post_only'] ? Lti::Launch.new(post_only: true) : Lti::Launch.new
547552
lti_launch.resource_url = opts[:launch_url]|| tool.extension_setting(placement, :url)
548-
lti_launch.params = LtiOutbound::ToolLaunch.generate_params(params, lti_launch.resource_url, tool.consumer_key, tool.shared_secret,
549-
disable_lti_post_only: @context.root_account.feature_enabled?(:disable_lti_post_only))
553+
lti_launch.params = Lti::Security.signed_post_params(
554+
params,
555+
lti_launch.resource_url,
556+
tool.consumer_key,
557+
tool.shared_secret,
558+
@context.root_account.feature_enabled?(:disable_lti_post_only)
559+
)
550560
lti_launch.link_text = tool.label_for(placement.to_sym, I18n.locale)
551561
lti_launch.analytics_id = tool.tool_id
552562

‎app/models/lti/lti_outbound_adapter.rb‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,21 @@ def prepare_tool_launch(return_url, variable_expander, opts = {})
5151
user: lti_user,
5252
tool: lti_tool,
5353
account: lti_account,
54-
variable_expander: variable_expander,
55-
disable_lti_post_only: @root_account.feature_enabled?(:disable_lti_post_only)
54+
variable_expander: variable_expander
5655
}
5756
)
5857
self
5958
end
6059

6160
def generate_post_payload
6261
raise('Called generate_post_payload before calling prepare_tool_launch') unless @tool_launch
63-
@tool_launch.generate(@overrides)
62+
hash = @tool_launch.generate(@overrides)
63+
Lti::Security.signed_post_params(
64+
hash,
65+
@tool_launch.url,
66+
@tool.consumer_key,
67+
@tool.shared_secret,
68+
@root_account.feature_enabled?(:disable_lti_post_only))
6469
end
6570

6671
def generate_post_payload_for_assignment(assignment, outcome_service_url, legacy_outcome_service_url, lti_turnitin_outcomes_placement_url)

‎gems/lti_outbound/lib/lti_outbound/tool_launch.rb‎

Lines changed: 1 addition & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ def initialize(options)
3939
@consumer_instance = context.consumer_instance || raise('Consumer instance required for generating LTI content')
4040

4141
@variable_expander = options[:variable_expander] || raise('VariableExpander is required for generating LTI content')
42-
@post_only = options[:disable_lti_post_only]
4342

4443
@hash = {}
4544
end
@@ -127,7 +126,7 @@ def generate(overrides={})
127126
hash['oauth_callback'] = 'about:blank'
128127

129128
@variable_expander.expand_variables!(hash)
130-
self.class.generate_params(hash, url, tool.consumer_key, tool.shared_secret, disable_lti_post_only: @post_only)
129+
hash
131130
end
132131

133132
private
@@ -163,54 +162,5 @@ def set_resource_type_keys
163162
end
164163
end
165164

166-
def self.generate_params(params, url, key, secret, feature_flags = {})
167-
uri = URI.parse(url)
168-
169-
if uri.port == uri.default_port
170-
host = uri.host
171-
else
172-
host = "#{uri.host}:#{uri.port}"
173-
end
174-
175-
consumer = OAuth::Consumer.new(key, secret, {
176-
:site => "#{uri.scheme}://#{host}",
177-
:signature_method => 'HMAC-SHA1'
178-
})
179-
180-
path = uri.path
181-
path = '/' if path.empty?
182-
unless feature_flags[:disable_lti_post_only]
183-
if uri.query && uri.query != ''
184-
CGI.parse(uri.query).each do |query_key, query_values|
185-
unless params[query_key]
186-
params[query_key] = query_values.first
187-
end
188-
end
189-
end
190-
end
191-
options = {
192-
:scheme => 'body',
193-
:timestamp => @timestamp,
194-
:nonce => @nonce
195-
}
196-
197-
request = consumer.create_signed_request(:post, path, nil, options, stringify_hash(params))
198-
199-
# the request is made by a html form in the user's browser, so we
200-
# want to revert the escapage and return the hash of post parameters ready
201-
# for embedding in a html view
202-
hash = {}
203-
request.body.split(/&/).each do |param|
204-
key, val = param.split(/=/).map{|v| CGI.unescape(v) }
205-
hash[key] = val
206-
end
207-
hash
208-
end
209-
210-
def self.stringify_hash(hash)
211-
hash.dup.tap do |new_hash|
212-
new_hash.keys.each { |k| new_hash[k.to_s] = new_hash.delete(k) unless k.is_a?(String) }
213-
end
214-
end
215165
end
216166
end

‎gems/lti_outbound/spec/lti_outbound/tool_launch_spec.rb‎

Lines changed: 5 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@
141141
expect(hash['lis_person_name_family']).to eq 'last_name'
142142
expect(hash['lis_person_name_given']).to eq 'first_name'
143143
expect(hash['lis_person_sourcedid']).to eq '$Person.sourcedId'
144-
expect(hash['launch_presentation_locale']).to eq 'en' #was I18n.default_locale.to_s
144+
expect(hash['launch_presentation_locale']).to eq :en #was I18n.default_locale.to_s
145145
expect(hash['launch_presentation_document_target']).to eq 'iframe'
146146
expect(hash['launch_presentation_return_url']).to eq 'http://www.google.com'
147147
expect(hash['tool_consumer_instance_guid']).to eq 'root_account_lti_guid'
@@ -196,7 +196,7 @@
196196
I18n.stub(:localizer).and_return(-> { :es })
197197
hash = tool_launch.generate
198198

199-
expect(hash['launch_presentation_locale']).to eq 'es'
199+
expect(hash['launch_presentation_locale']).to eq :es
200200
end
201201

202202
it 'adds account info in launch data for account navigation' do
@@ -230,19 +230,6 @@
230230
expect(hash['tool_consumer_instance_guid']).to eq 'root_account_lti_guid' #was hash['tool_consumer_instance_guid']).to eq sub_account.root_account.lti_guid
231231
end
232232

233-
it 'includes URI query parameters' do
234-
hash = LtiOutbound::ToolLaunch.new(:url => 'http://www.yahoo.com?paramater_a=value_a&parameter_b=value_b',
235-
:tool => tool,
236-
:user => user,
237-
:account => account,
238-
:context => course,
239-
:link_code => '123456',
240-
:return_url => 'http://www.google.com',
241-
:variable_expander => variable_expander).generate
242-
expect(hash['paramater_a']).to eq 'value_a'
243-
expect(hash['parameter_b']).to eq 'value_b'
244-
end
245-
246233
it 'does not allow overwriting other parameters from the URI query string' do
247234
hash = LtiOutbound::ToolLaunch.new(:url => 'http://www.yahoo.com?user_id=ATTEMPT_TO_SET_DATA&oauth_callback=ATTEMPT_TO_SET_DATA',
248235
:tool => tool,
@@ -269,7 +256,7 @@
269256
expect(hash['custom_bob']).to eql('bob')
270257
expect(hash['custom_fred']).to eql('fred')
271258
expect(hash['custom_john']).to eql('john')
272-
expect(hash['custom___taa____']).to eql('123')
259+
expect(hash['custom___taa____']).to eql(123)
273260
expect(hash).to_not have_key '@$TAA$#$#'
274261
expect(hash).to_not have_key 'john'
275262
end
@@ -321,8 +308,8 @@
321308
:return_url => 'http://www.yahoo.com',
322309
:resource_type => 'editor_button',
323310
:variable_expander => variable_expander).generate
324-
expect(hash['launch_presentation_width']).to eq '1000'
325-
expect(hash['launch_presentation_height']).to eq '300'
311+
expect(hash['launch_presentation_width']).to eq 1000
312+
expect(hash['launch_presentation_height']).to eq 300
326313
end
327314

328315
it 'does not copy query params to POST body if disable_lti_post_only feature flag is set' do
@@ -394,109 +381,4 @@
394381
end
395382
end
396383

397-
#TODO: do not test private methods
398-
describe '.generate_params' do
399-
def explicit_signature_settings(timestamp, nonce)
400-
LtiOutbound::ToolLaunch.instance_variable_set(:'@timestamp', timestamp)
401-
LtiOutbound::ToolLaunch.instance_variable_set(:'@nonce', nonce)
402-
end
403-
404-
it 'generate a correct signature' do
405-
explicit_signature_settings('1251600739', 'c8350c0e47782d16d2fa48b2090c1d8f')
406-
407-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
408-
:resource_link_id => '120988f929-274612',
409-
:user_id => '292832126',
410-
:roles => 'Instructor',
411-
:lis_person_name_full => 'Jane Q. Public',
412-
:lis_person_contact_email_primary => 'user@school.edu',
413-
:lis_person_sourced_id => 'school.edu:user',
414-
:context_id => '456434513',
415-
:context_title => 'Design of Personal Environments',
416-
:context_label => 'SI182',
417-
:lti_version => 'LTI-1p0',
418-
:lti_message_type => 'basic-lti-launch-request',
419-
:tool_consumer_instance_guid => 'lmsng.school.edu',
420-
:tool_consumer_instance_description => 'University of School (LMSng)',
421-
:lti_submit => 'Launch Endpoint with LTI Data'
422-
}, 'http://dr-chuck.com/ims/php-simple/tool.php', '12345', 'secret')
423-
424-
expect(hash['oauth_signature']).to eql('l1ZTsn1HjGXzqeaTQMPbjrqvjLU=')
425-
end
426-
427-
it 'generate a correct signature with URL query parameters' do
428-
explicit_signature_settings('1251600739', 'c8350c0e47782d16d2fa48b2090c1d8f')
429-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
430-
:resource_link_id => '120988f929-274612',
431-
:user_id => '292832126',
432-
:roles => 'Instructor',
433-
:lis_person_name_full => 'Jane Q. Public',
434-
:lis_person_contact_email_primary => 'user@school.edu',
435-
:lis_person_sourced_id => 'school.edu:user',
436-
:context_id => '456434513',
437-
:context_title => 'Design of Personal Environments',
438-
:context_label => 'SI182',
439-
:lti_version => 'LTI-1p0',
440-
:lti_message_type => 'basic-lti-launch-request',
441-
:tool_consumer_instance_guid => 'lmsng.school.edu',
442-
:tool_consumer_instance_description => 'University of School (LMSng)',
443-
:lti_submit => 'Launch Endpoint with LTI Data'
444-
}, 'http://dr-chuck.com/ims/php-simple/tool.php?a=1&b=2&c=3%20%26a', '12345', 'secret')
445-
expect(hash['oauth_signature']).to eql('k/+aMdax1Jm5kuGF6DG/ptN5VfY=')
446-
expect(hash['c']).to eq '3 &a'
447-
end
448-
449-
it 'generate a correct signature with a non-standard port' do
450-
#signatures generated using http://oauth.googlecode.com/svn/code/javascript/example/signature.html
451-
explicit_signature_settings('1251600739', 'c8350c0e47782d16d2fa48b2090c1d8f')
452-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
453-
}, 'http://dr-chuck.com:123/ims/php-simple/tool.php', '12345', 'secret')
454-
expect(hash['oauth_signature']).to eql('ghEdPHwN4iJmsM3Nr4AndDx2Kx8=')
455-
456-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
457-
}, 'http://dr-chuck.com/ims/php-simple/tool.php', '12345', 'secret')
458-
expect(hash['oauth_signature']).to eql('WoSpvCr2HEsLzao6Do0eukxwAsk=')
459-
460-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
461-
}, 'http://dr-chuck.com:80/ims/php-simple/tool.php', '12345', 'secret')
462-
expect(hash['oauth_signature']).to eql('WoSpvCr2HEsLzao6Do0eukxwAsk=')
463-
464-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
465-
}, 'http://dr-chuck.com:443/ims/php-simple/tool.php', '12345', 'secret')
466-
expect(hash['oauth_signature']).to eql('KqAV7eIS/+iWIDpvCyDfY8ZpmT4=')
467-
468-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
469-
}, 'https://dr-chuck.com/ims/php-simple/tool.php', '12345', 'secret')
470-
expect(hash['oauth_signature']).to eql('wFRB/1ZXi/91dop6GwahfboWPvQ=')
471-
472-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
473-
}, 'https://dr-chuck.com:443/ims/php-simple/tool.php', '12345', 'secret')
474-
expect(hash['oauth_signature']).to eql('wFRB/1ZXi/91dop6GwahfboWPvQ=')
475-
476-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
477-
}, 'https://dr-chuck.com:80/ims/php-simple/tool.php', '12345', 'secret')
478-
expect(hash['oauth_signature']).to eql('X8Aq2HXSHnr6u/6z/G9zI5aDoR0=')
479-
end
480-
481-
it 'does not copy query params to POST body if disable_lti_post_only feature flag is set' do
482-
hash = LtiOutbound::ToolLaunch.send(:generate_params, {
483-
:resource_link_id => '120988f929-274612',
484-
:user_id => '292832126',
485-
:roles => 'Instructor',
486-
:lis_person_name_full => 'Jane Q. Public',
487-
:lis_person_contact_email_primary => 'user@school.edu',
488-
:lis_person_sourced_id => 'school.edu:user',
489-
:context_id => '456434513',
490-
:context_title => 'Design of Personal Environments',
491-
:context_label => 'SI182',
492-
:lti_version => 'LTI-1p0',
493-
:lti_message_type => 'basic-lti-launch-request',
494-
:tool_consumer_instance_guid => 'lmsng.school.edu',
495-
:tool_consumer_instance_description => 'University of School (LMSng)',
496-
:lti_submit => 'Launch Endpoint with LTI Data'
497-
}, 'http://www.instructure.com?first=weston&last=dransfield', 'key', 'secret',
498-
disable_lti_post_only: true)
499-
expect(hash.key?('first')).to eq false
500-
end
501-
end
502384
end

‎spec/models/lti/lti_outbound_adapter_spec.rb‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -190,11 +190,12 @@
190190

191191
describe "#generate_post_payload" do
192192
it "calls generate on the tool launch" do
193-
tool_launch = mock('tool launch', generate: {})
193+
tool_launch = mock('tool launch')
194+
tool_launch.expects(generate: {})
195+
tool_launch.stubs(url: "http://example.com/launch")
194196
LtiOutbound::ToolLaunch.stubs(:new).returns(tool_launch)
195197
adapter.prepare_tool_launch(return_url, variable_expander)
196-
197-
expect(adapter.generate_post_payload).to eq({})
198+
adapter.generate_post_payload
198199
end
199200

200201
it "raises a not prepared error if the tool launch has not been prepared" do
@@ -206,7 +207,7 @@
206207
let(:outcome_service_url) { '/outcome/service' }
207208
let(:legacy_outcome_service_url) { '/legacy/service' }
208209
let(:lti_turnitin_outcomes_placement_url) { 'turnitin/outcomes/placement' }
209-
let(:tool_launch) { stub('tool launch', generate: {}) }
210+
let(:tool_launch) { stub('tool launch', generate: {}, url: "http://example.com/launch") }
210211

211212
before(:each) do
212213
LtiOutbound::ToolLaunch.stubs(:new).returns(tool_launch)
@@ -244,7 +245,7 @@
244245

245246
describe "#generate_post_payload_for_homework_submission" do
246247
it "creates an lti_assignment" do
247-
tool_launch = mock('tool launch', generate: {})
248+
tool_launch = mock('tool launch', generate: {}, url: "http://example.com/launch")
248249
LtiOutbound::ToolLaunch.stubs(:new).returns(tool_launch)
249250
adapter.prepare_tool_launch(return_url, variable_expander)
250251

0 commit comments

Comments
 (0)