From 0e72be8bcfc080e6189e0c33af20797ab34e1fb9 Mon Sep 17 00:00:00 2001 From: Brian Palmer Date: Fri, 4 Nov 2011 09:29:29 -0600 Subject: [PATCH] change to a more generic lti outcome service url scheme refs #5892 according to the spec, this url shouldn't change for each user/assignment, so we've refactored so that only the tool id is in the url. This involved some refactoring to include the user, course and assignment in the sourcedid, rather than just the signature of those values, and using those values to look up the objects. As a consequence we now return "unsupported" rather than a 401 error if the sourcedid is incorrect, which I think is more correct according to the spec anyway. Change-Id: I612d48cbd63b527d9f9209b858ebeca28ebe9202 Reviewed-on: https://gerrit.instructure.com/6699 Tested-by: Hudson Reviewed-by: Brian Whitmer --- app/controllers/application_controller.rb | 2 +- app/controllers/lti_api_controller.rb | 24 +++++---------- config/routes.rb | 2 +- lib/basic_lti.rb | 2 +- lib/basic_lti/basic_outcomes.rb | 36 +++++++++++++++++++---- spec/apis/lti/grade_passback_spec.rb | 12 ++++---- spec/integration/external_tools_spec.rb | 4 +-- spec/lib/basic_lti_spec.rb | 2 +- 8 files changed, 49 insertions(+), 35 deletions(-) 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