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 <hudson@instructure.com>
Reviewed-by: Brian Whitmer <brian@instructure.com>

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'

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

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

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

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
 

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 = %{
 <?xml version = "1.0" encoding = "UTF-8"?>
 <imsx_POXEnvelopeRequest xmlns = "http://www.imsglobal.org/lis/oms1p0/pox">
@@ -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 = %{
 <?xml version = "1.0" encoding = "UTF-8"?>
 <imsx_POXEnvelopeRequest xmlns = "http://www.imsglobal.org/lis/oms1p0/pox">
@@ -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 = %{
 <?xml version = "1.0" encoding = "UTF-8"?>
 <imsx_POXEnvelopeRequest xmlns = "http://www.imsglobal.org/lis/oms1p0/pox">
@@ -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

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

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