diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f334441baaf..8f3b4f11d5a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -826,7 +826,7 @@ class ApplicationController < ActionController::Base @return_url = named_context_url(@context, :context_external_tool_finished_url, @tool.id, :include_host => true) @launch = BasicLTI::ToolLaunch.new(:url => @resource_url, :tool => @tool, :user => @current_user, :context => @context, :link_code => @opaque_id, :return_url => @return_url) if @tag.context.is_a?(Assignment) && @context.students.include?(@current_user) - @launch.for_assignment!(@tag.context, lti_grade_passback_api_url(@context, @tag.context, @current_user)) + @launch.for_assignment!(@tag.context, lti_grade_passback_api_url(@tool)) end @tool_settings = @launch.generate render :template => 'external_tools/tool_show' diff --git a/app/controllers/lti_api_controller.rb b/app/controllers/lti_api_controller.rb index ebbdf336674..735273d5af4 100644 --- a/app/controllers/lti_api_controller.rb +++ b/app/controllers/lti_api_controller.rb @@ -19,39 +19,29 @@ require 'oauth/request_proxy/action_controller_request' class LtiApiController < ApplicationController - - class Unauthorized < Exception; end - def grade_passback - require_context - # load the external tool to grab the key and secret - @assignment = @context.assignments.find(params[:assignment_id]) - @user = @context.students.find(params[:id]) - tag = @assignment.external_tool_tag - @tool = ContextExternalTool.find_external_tool(tag.url, @context) if tag + @tool = ContextExternalTool.find(params[:tool_id]) - raise(Unauthorized, "LTI tool not found") unless @tool - - # verify the request oauth signature + # verify the request oauth signature, timestamp and nonce begin @signature = OAuth::Signature.build(request, :consumer_secret => @tool.shared_secret) @signature.verify() or raise OAuth::Unauthorized rescue OAuth::Signature::UnknownSignatureMethod, OAuth::Unauthorized - raise Unauthorized, "Invalid authorization header" + raise BasicLTI::BasicOutcomes::Unauthorized, "Invalid authorization header" end timestamp = Time.zone.at(@signature.request.timestamp.to_i) # 90 minutes is suggested by the LTI spec allowed_delta = Setting.get_cached('oauth.allowed_timestamp_delta', 90.minutes.to_s).to_i if timestamp < allowed_delta.ago || timestamp > allowed_delta.from_now - raise Unauthorized, "Timestamp too old, request has expired" + raise BasicLTI::BasicOutcomes::Unauthorized, "Timestamp too old or too far in the future, request has expired" end nonce = @signature.request.nonce unless Canvas::Redis.lock("nonce:#{@tool.asset_string}:#{nonce}", allowed_delta) - raise Unauthorized, "Duplicate nonce detected" + raise BasicLTI::BasicOutcomes::Unauthorized, "Duplicate nonce detected" end if request.content_type != "application/xml" @@ -60,10 +50,10 @@ class LtiApiController < ApplicationController xml = Nokogiri::XML.parse(request.body) - lti_response = BasicLTI::BasicOutcomes.process_request(@tool, @context, @assignment, @user, xml) + lti_response = BasicLTI::BasicOutcomes.process_request(@tool, xml) render :text => lti_response.to_xml, :content_type => 'application/xml' - rescue Unauthorized => e + rescue BasicLTI::BasicOutcomes::Unauthorized => e render :text => e.to_s, :content_type => 'application/xml', :status => 401 end end diff --git a/config/routes.rb b/config/routes.rb index 665d3e12954..f2ad05f6c77 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -740,7 +740,7 @@ ActionController::Routing::Routes.draw do |map| map.oauth2_token 'login/oauth2/token',:controller => 'pseudonym_sessions', :action => 'oauth2_token', :conditions => { :method => :post } ApiRouteSet.route(map, "/api/lti/v1") do |lti| - lti.post "courses/:course_id/assignments/:assignment_id/submissions/:id", :controller => :lti_api, :action => :grade_passback, :path_name => "lti_grade_passback_api" + lti.post "tools/:tool_id/grade_passback", :controller => :lti_api, :action => :grade_passback, :path_name => "lti_grade_passback_api" end map.resources :equation_images, :only => :show diff --git a/lib/basic_lti.rb b/lib/basic_lti.rb index 0853ea02140..6cc89a61901 100644 --- a/lib/basic_lti.rb +++ b/lib/basic_lti.rb @@ -66,7 +66,7 @@ module BasicLTI end def for_assignment!(assignment, outcome_service_url) - hash['lis_result_sourcedid'] = BasicLTI::BasicOutcomes.result_source_id(tool, context, assignment, user) + hash['lis_result_sourcedid'] = BasicLTI::BasicOutcomes.encode_source_id(tool, context, assignment, user) hash['lis_outcome_service_url'] = outcome_service_url if tool.public? hash['custom_canvas_assignment_id'] = assignment.id diff --git a/lib/basic_lti/basic_outcomes.rb b/lib/basic_lti/basic_outcomes.rb index ecc4acfdd68..ee47ba95a0c 100644 --- a/lib/basic_lti/basic_outcomes.rb +++ b/lib/basic_lti/basic_outcomes.rb @@ -1,16 +1,38 @@ module BasicLTI::BasicOutcomes + class Unauthorized < Exception; end + # this is the lis_result_sourcedid field in the launch, and the # sourcedGUID/sourcedId in BLTI basic outcome requests. # it's a secure signature of the (tool, course, assignment, user). Combined with # the pre-determined shared secret that the tool signs requests with, this # ensures that only this launch of the tool can modify the score. - def self.result_source_id(tool, course, assignment, user) - Canvas::Security.hmac_sha1("#{tool.id}-#{course.id}-#{assignment.id}-#{user.id}") + def self.encode_source_id(tool, course, assignment, user) + payload = [tool.id, course.id, assignment.id, user.id].join('-') + "#{payload}-#{Canvas::Security.hmac_sha1(payload)}" end - def self.process_request(tool, course, assignment, user, xml) + SOURCE_ID_REGEX = %r{^(\d+)-(\d+)-(\d+)-(\d+)-(\w+)$} + + def self.decode_source_id(sourceid) + md = sourceid.match(SOURCE_ID_REGEX) + return false unless md + new_encoding = [md[1], md[2], md[3], md[4]].join('-') + return false unless Canvas::Security.hmac_sha1(new_encoding) == md[5] + tool = ContextExternalTool.find(md[1]) + course = Course.find(md[2]) + assignment = course.assignments.active.find(md[3]) + user = course.students.active.find(md[4]) + tag = assignment.external_tool_tag + if !tag || tool != ContextExternalTool.find_external_tool(tag.url, course) + return false # assignment settings have changed, this tool is no longer active + end + return course, assignment, user + end + + def self.process_request(tool, xml) res = LtiResponse.new(xml) - unless self.handle_request(tool, course, assignment, user, xml, res) + + unless self.handle_request(tool, xml, res) res.code_major = 'unsupported' end return res @@ -18,12 +40,14 @@ module BasicLTI::BasicOutcomes protected - def self.handle_request(tool, course, assignment, user, xml, res) + def self.handle_request(tool, xml, res) # verify the lis_result_sourcedid param, which will be a canvas-signed # tuple of (course, assignment, user) to ensure that only this launch of # the tool is attempting to modify this data. source_id = xml.at_css('imsx_POXBody sourcedGUID > sourcedId').try(:content) - unless source_id && source_id == BasicLTI::BasicOutcomes.result_source_id(tool, course, assignment, user) + course, assignment, user = self.decode_source_id(source_id) if source_id + + unless course && assignment && user return false end diff --git a/spec/apis/lti/grade_passback_spec.rb b/spec/apis/lti/grade_passback_spec.rb index 42e71c9d2f0..88b5c4a3166 100644 --- a/spec/apis/lti/grade_passback_spec.rb +++ b/spec/apis/lti/grade_passback_spec.rb @@ -31,7 +31,7 @@ describe LtiApiController, :type => :integration do end def make_call(opts = {}) - opts['path'] ||= "/api/lti/v1/courses/#{@course.id}/assignments/#{@assignment.id}/submissions/#{@student.id}" + opts['path'] ||= "/api/lti/v1/tools/#{@tool.id}/grade_passback" opts['key'] ||= @tool.consumer_key opts['secret'] ||= @tool.shared_secret opts['content-type'] ||= 'application/xml' @@ -60,7 +60,7 @@ describe LtiApiController, :type => :integration do end def replace_result(score, sourceid = nil) - sourceid ||= BasicLTI::BasicOutcomes.result_source_id(@tool, @course, @assignment, @student) + sourceid ||= BasicLTI::BasicOutcomes.encode_source_id(@tool, @course, @assignment, @student) body = %{ @@ -90,7 +90,7 @@ describe LtiApiController, :type => :integration do end def read_result(sourceid = nil) - sourceid ||= BasicLTI::BasicOutcomes.result_source_id(@tool, @course, @assignment, @student) + sourceid ||= BasicLTI::BasicOutcomes.encode_source_id(@tool, @course, @assignment, @student) body = %{ @@ -114,7 +114,7 @@ describe LtiApiController, :type => :integration do end def delete_result(sourceid = nil) - sourceid ||= BasicLTI::BasicOutcomes.result_source_id(@tool, @course, @assignment, @student) + sourceid ||= BasicLTI::BasicOutcomes.encode_source_id(@tool, @course, @assignment, @student) body = %{ @@ -259,7 +259,7 @@ describe LtiApiController, :type => :integration do tag.content_type = 'ContextExternalTool' tag.save! make_call('body' => replace_result('0.5')) - response.status.should == "401 Unauthorized" + check_failure end it "should be unsupported if the assignment switched to a new tool with the same shared secret" do @@ -277,7 +277,7 @@ describe LtiApiController, :type => :integration do @assignment.update_attributes(:submission_types => 'online_upload') @assignment.external_tool_tag.destroy! make_call('body' => replace_result('0.5')) - response.status.should == "401 Unauthorized" + check_failure end it "should verify the sourcedid is correct for this tool launch" do diff --git a/spec/integration/external_tools_spec.rb b/spec/integration/external_tools_spec.rb index 1d169c35a1f..4b2abc2d524 100644 --- a/spec/integration/external_tools_spec.rb +++ b/spec/integration/external_tools_spec.rb @@ -53,8 +53,8 @@ describe "External Tools" do get "/courses/#{@course.id}/assignments/#{@assignment.id}" response.should be_success doc = Nokogiri::HTML.parse(response.body) - doc.at_css('form#tool_form input#lis_result_sourcedid')['value'].should == BasicLTI::BasicOutcomes.result_source_id(@tool, @course, @assignment, @user) - doc.at_css('form#tool_form input#lis_outcome_service_url')['value'].should == lti_grade_passback_api_url(@course, @assignment, @user) + doc.at_css('form#tool_form input#lis_result_sourcedid')['value'].should == BasicLTI::BasicOutcomes.encode_source_id(@tool, @course, @assignment, @user) + doc.at_css('form#tool_form input#lis_outcome_service_url')['value'].should == lti_grade_passback_api_url(@tool) end it "should not include outcome service params when viewing as teacher" do diff --git a/spec/lib/basic_lti_spec.rb b/spec/lib/basic_lti_spec.rb index fcdd9fdeba6..c40126b2087 100644 --- a/spec/lib/basic_lti_spec.rb +++ b/spec/lib/basic_lti_spec.rb @@ -194,7 +194,7 @@ describe BasicLTI do assignment_model(:submission_types => "external_tool", :course => @course) launch.for_assignment!(@assignment, "/my/test/url") hash = launch.generate - hash['lis_result_sourcedid'].should == BasicLTI::BasicOutcomes.result_source_id(@tool, @course, @assignment, @user) + hash['lis_result_sourcedid'].should == BasicLTI::BasicOutcomes.encode_source_id(@tool, @course, @assignment, @user) hash['lis_outcome_service_url'].should == "/my/test/url" end end