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

better validation and error handling for lti launches 

fixes CNVS-28799

test plan:
- commons share links should still work
- try generating an invalid share link (with ids that don't exist)
- it should give an appropriate error
- basic lti launches and api lti launches should still work

Change-Id: I6731f4a578b3075507e4052874f7e5d38f470892
Reviewed-on: https://gerrit.instructure.com/77393
Reviewed-by: Nathan Mills <nathanm@instructure.com>
QA-Review: Benjamin Christian Nelson <bcnelson@instructure.com>
Tested-by: Jenkins
Product-Review: Simon Williams <simon@instructure.com>
This commit is contained in:
Simon Williams 2016-04-19 11:48:06 -06:00
parent 73cc71e15d
commit 70fd83f3c9
4 changed files with 147 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
app/controllers/external_tools_controller.rb
View File

@ -311,6 +311,8 @@ class ExternalToolsController < ApplicationController
@show_embedded_chat = false if @tool.tool_id == 'chat'
@lti_launch = lti_launch(@tool, placement)
return unless @lti_launch
render Lti::AppUtil.display_template(@tool.display_type(placement), display_override: params[:display])
end
end
@ -359,6 +361,8 @@ class ExternalToolsController < ApplicationController
return unless find_tool(params[:external_tool_id], selection_type)
@lti_launch = lti_launch(@tool, selection_type)
return unless @lti_launch
render Lti::AppUtil.display_template('borderless')
end
@ -382,12 +386,25 @@ class ExternalToolsController < ApplicationController
case message_type
when 'ContentItemSelectionResponse', 'ContentItemSelection'
#ContentItemSelectionResponse is deprecated, use ContentItemSelection instead
content_item_selection(tool, selection_type, create_content_item_response, message_type, url)
content_item_selection(tool, selection_type, message_type, url)
when 'ContentItemSelectionRequest'
content_item_selection_request(tool, selection_type, url)
else
basic_lti_launch_request(tool, selection_type, url)
end
rescue Lti::UnauthorizedError
render_unauthorized_action
nil
rescue Lti::UnsupportedExportTypeError, Lti::InvalidMediaTypeError
respond_to do |format|
err = t('There was an error generating the tool launch')
format.html do
flash[:error] = err
redirect_to named_context_url(@context, :context_url)
end
format.json { render :json => { error: err } }
end
nil
end
protected :lti_launch
@ -411,7 +428,18 @@ class ExternalToolsController < ApplicationController
end
protected :basic_lti_launch_request
def content_item_selection(tool, placement, content_item_response, message_type, url = nil)
def content_item_selection(tool, placement, message_type, url = nil)
media_types = params.select do |param|
Lti::ContentItemResponse::MEDIA_TYPES.include?(param.to_sym)
end
content_item_response = Lti::ContentItemResponse.new(
@context,
self,
@current_user,
media_types,
params["export_type"]
)
params = default_lti_params.merge(
{
#required params
@ -436,19 +464,6 @@ class ExternalToolsController < ApplicationController
end
protected :content_item_selection
def create_content_item_response
media_types = params.select { |param| Lti::ContentItemResponse::MEDIA_TYPES.include? param.to_sym }
begin
Lti::ContentItemResponse.new(@context, self, @current_user, media_types, params["export_type"])
rescue Lti::UnauthorizedError
render_unauthorized_action
rescue Lti::UnsupportedExportTypeError
#Legacy API behavior does nothing if the export type is unsupported
end
end
protected :create_content_item_response
# Do an official content-item request as specified: http://www.imsglobal.org/LTI/services/ltiCIv1p0pd/ltiCIv1p0pd.html
def content_item_selection_request(tool, placement, url = nil)
extra_params = {}
@ -757,6 +772,8 @@ class ExternalToolsController < ApplicationController
raise ActiveRecord::RecordNotFound if tool.nil?
launch = lti_launch(tool)
return unless launch
params = launch.params.reject {|p| p.starts_with?('oauth_')}
params[:consumer_key] = tool.consumer_key
params[:iat] = Time.zone.now.to_i

62
lib/lti/content_item_response.rb
View File

@ -20,6 +20,11 @@
# Likewise, all the methods added will be available for all controllers.
module Lti
class UnauthorizedError < StandardError; end
class UnsupportedExportTypeError < StandardError; end
class UnsupportedMessageTypeError < StandardError; end
class InvalidMediaTypeError < StandardError; end
class ContentItemResponse
MEDIA_TYPES = [:assignments, :discussion_topics, :modules, :module_items, :pages, :quizzes, :files]
@ -27,11 +32,12 @@ module Lti
def initialize(context, controller, current_user, media_types, export_type)
@context = context
@media_types = media_types.with_indifferent_access
@canvas_media_type = @media_types.keys.size == 1 ? @media_types.keys.first.to_s.singularize : 'course'
@current_user = current_user
@controller = controller #for url generation
@current_user = current_user
@media_types = media_types.with_indifferent_access
@export_type = export_type || 'common_cartridge' #legacy API behavior defaults to common cartridge
raise Lti::InvalidMediaTypeError unless media_types_valid?
raise Lti::UnsupportedExportTypeError unless SUPPORTED_EXPORT_TYPES.include? @export_type
end
@ -47,7 +53,7 @@ module Lti
def media_type
unless @media_type
if @canvas_media_type == 'module_item'
if canvas_media_type == 'module_item'
case tag.content
when Assignment
@media_type = 'assignment'
@ -59,7 +65,7 @@ module Lti
@media_type = 'page'
end
else
@media_type = @canvas_media_type
@media_type = canvas_media_type
end
end
@media_type
@ -92,7 +98,7 @@ module Lti
end
def title
@title ||= case @canvas_media_type
@title ||= case canvas_media_type
when 'file'
file.display_name
when 'assignment'
@ -134,6 +140,38 @@ module Lti
private
def canvas_media_type
@canvas_media_type ||= if @media_types.keys.size == 1
@media_types.keys.first.to_s.singularize
else
'course'
end
end
def media_types_valid?
@media_types.each do |type, ids|
scope = case type
when 'files'
Attachment
when 'assignments'
@context.assignments
when 'discussion_topics'
@context.discussion_topics
when 'modules'
@context.context_modules
when 'pages'
@context.wiki.wiki_pages
when 'module_items'
@context.context_module_tags
when 'quizzes'
@context.quizzes
end
return false if scope.where(:id => ids).count != ids.count
end
true
end
##
# This message type is deprecated, please use content_item_selection_json
##
@ -170,14 +208,4 @@ module Lti
end
end
class UnauthorizedError < StandardError
end
class UnsupportedExportTypeError < StandardError
end
class UnsupportedMessageTypeError < StandardError
end
end
end

18
spec/controllers/external_tools_controller_spec.rb
View File

@ -148,7 +148,7 @@ describe ExternalToolsController do
expect(lti_launch.params['resource_link_id']).to eq opaque_id(@course)
end
it "returns 404 if the tool is not found" do
it "returns flash error if the tool is not found" do
user_session(@teacher)
get :show, :account_id => @course.account.id, id: 0
expect(response).to be_redirect
@ -340,19 +340,27 @@ describe ExternalToolsController do
it "sends content item json for selected content" do
user_session(@teacher)
get :show, :course_id => @course.id, id: @tool.id, :pages => [1,6], :assignments => [6]
page = @course.wiki.wiki_pages.create!(title: 'a page')
assignment = @course.assignments.create!(name: 'an assignment')
get :show, :course_id => @course.id, id: @tool.id, :pages => [page.id], :assignments => [assignment.id]
placement = JSON.parse(assigns[:lti_launch].params['content_items'])['@graph'].first
migration_url = placement['placementOf']['@id']
params = migration_url.split('?').last.split('&')
expect(migration_url).to start_with api_v1_course_content_exports_url(@course)
expect(params).to include 'export_type=common_cartridge'
expect(params).to include "select%5Bpages%5D%5B%5D=1"
expect(params).to include "select%5Bpages%5D%5B%5D=6"
expect(params).to include "select%5Bassignments%5D%5B%5D=6"
expect(params).to include "select%5Bpages%5D%5B%5D=#{page.id}"
expect(params).to include "select%5Bassignments%5D%5B%5D=#{assignment.id}"
expect(placement['placementOf']['mediaType']).to eq 'application/vnd.instructure.api.content-exports.course'
expect(placement['placementOf']['title']).to eq 'a course'
end
it "returns flash error if invalid id params are passed in" do
user_session(@teacher)
get :show, :course_id => @course.id, id: @tool.id, :pages => [0]
expect(response).to be_redirect
expect(flash[:error]).to match(/error generating the tool launch/)
end
end
context 'ContentItemSelectionRequest' do

85
spec/lib/lti/content_item_response_spec.rb
View File

@ -20,7 +20,10 @@ require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
describe Lti::ContentItemResponse do
let(:context) { course_with_teacher(:active_all => true); @course }
let_once(:context) { course(active_all: true) }
let_once(:teacher) { course_with_teacher(course: context, active_all: true).user }
let_once(:assign1) { context.assignments.create!(name: "A1") }
let_once(:assign2) { context.assignments.create!(name: "A2") }
let(:controller) do
controller_mock = mock('controller')
controller_mock.stubs(:api_v1_course_content_exports_url).returns('api_export_url')
@ -28,25 +31,51 @@ describe Lti::ContentItemResponse do
controller_mock
end
def subject(media_types)
described_class.new(context, controller, teacher, media_types, 'common_cartridge')
end
describe '#initialize' do
it 'raises an error if an invalid id is passed in' do
expect { subject({ assignments: [0] }) }.to(
raise_error(Lti::InvalidMediaTypeError )
)
end
it 'raises an error if on an invalid export type' do
expect {
described_class.new(
context,
controller,
teacher,
{ "assignments" => [assign1.id] },
'blah'
)
}.to raise_error(Lti::UnsupportedExportTypeError)
end
end
describe '#query_params' do
it 'return correct query params' do
content_item_response = described_class.new(context, controller, @user, {assignments: [1, 2], modules: [3, 4]}, 'common_cartridge')
expect(content_item_response.query_params).to eq({"export_type" => "common_cartridge", "select" => {"assignments" => [1, 2], "modules" => [3, 4]}})
content_item_response = subject({assignments: [assign1.id, assign2.id]})
expect(content_item_response.query_params).to eq({"export_type" => "common_cartridge", "select" => {"assignments" => [assign1.id, assign2.id]}})
end
it 'does not return the select object if there are no media types' do
content_item_response = described_class.new(context, controller, @user, {}, 'common_cartridge')
content_item_response = subject({})
expect(content_item_response.query_params.keys).not_to include 'select'
end
end
describe 'media_type' do
describe '#media_type' do
it 'uses the canvas_media_type when it is not a module item' do
content_item_response = described_class.new(context, controller, @user, {assignments: [1, 2]}, 'common_cartridge')
content_item_response = subject({assignments: [assign1.id, assign2.id]})
expect(content_item_response.media_type).to eq 'assignment'
end
it 'returns canvas if more than one canvas media is passed in' do
content_item_response = described_class.new(context, controller, @user, {assignments: [1, 2], modules: [3, 4]}, 'common_cartridge')
topic = context.discussion_topics.create!(:title => "blah")
content_item_response = subject({assignments: [assign1.id, assign2.id], discussion_topics: [topic.id]})
expect(content_item_response.media_type).to eq 'course'
end
@ -55,28 +84,28 @@ describe Lti::ContentItemResponse do
context_module = context.context_modules.create!(name: 'a module')
assignment = context.assignments.create!(name: 'an assignment')
tag = context_module.add_item(:id => assignment.id, :type => 'assignment')
content_item_response = described_class.new(context, controller, @user, {module_items: [tag.id]}, 'common_cartridge')
content_item_response = subject({module_items: [tag.id]})
expect(content_item_response.media_type).to eq 'assignment'
end
it 'sets the media_type to "quiz"' do
context_module = context.context_modules.create!(name: 'a module')
quiz = context.quizzes.create!(title: 'a quiz')
tag = context_module.add_item(:id => quiz.id, :type => 'quiz')
content_item_response = described_class.new(context, controller, @user, {module_items: [tag.id]}, 'common_cartridge')
content_item_response = subject({module_items: [tag.id]})
expect(content_item_response.media_type).to eq 'quiz'
end
it 'sets the media_type to "page"' do
context_module = context.context_modules.create!(name: 'a module')
page = context.wiki.wiki_pages.create!(title: 'a page')
tag = context_module.add_item(:id => page.id, :type => 'page')
content_item_response = described_class.new(context, controller, @user, {module_items: [tag.id]}, 'common_cartridge')
content_item_response = subject({module_items: [tag.id]})
expect(content_item_response.media_type).to eq 'page'
end
it 'sets the media_type to "discussion_topic"' do
context_module = context.context_modules.create!(name: 'a module')
topic = context.discussion_topics.create!(:title => "blah")
tag = context_module.add_item(:id => topic.id, :type => 'discussion_topic')
content_item_response = described_class.new(context, controller, @user, {module_items: [tag.id]}, 'common_cartridge')
content_item_response = subject({module_items: [tag.id]})
expect(content_item_response.media_type).to eq 'discussion_topic'
end
end
@ -87,7 +116,7 @@ describe Lti::ContentItemResponse do
context_module = context.context_modules.create!(name: 'a module')
assignment = context.assignments.create!(name: 'an assignment')
tag = context_module.add_item(:id => assignment.id, :type => 'assignment')
content_item_response = described_class.new(context, controller, @user, {module_items: [tag.id]}, 'common_cartridge')
content_item_response = subject({module_items: [tag.id]})
expect(content_item_response.tag).to eq tag
end
end
@ -95,7 +124,7 @@ describe Lti::ContentItemResponse do
describe '#file' do
it 'returns a file' do
file = attachment_model(context: context)
content_item_response = described_class.new(context, controller, @user, {files: [file.id]}, 'common_cartridge')
content_item_response = subject({files: [file.id]})
expect(content_item_response.file).to eq file
end
end
@ -103,31 +132,31 @@ describe Lti::ContentItemResponse do
describe '#title' do
it 'gets the title for a file' do
file = attachment_model(context: context)
content_item_response = described_class.new(context, controller, @user, {files: [file.id]}, 'common_cartridge')
content_item_response = subject({files: [file.id]})
expect(content_item_response.title).to eq 'unknown.loser'
end
it 'gets the title for a assignment' do
assignment = context.assignments.create!(name: 'an assignment')
content_item_response = described_class.new(context, controller, @user, {assignments: [assignment.id]}, 'common_cartridge')
content_item_response = subject({assignments: [assignment.id]})
expect(content_item_response.title).to eq 'an assignment'
end
it 'gets the title for a discussion_topic' do
topic = context.discussion_topics.create!(:title => "blah")
content_item_response = described_class.new(context, controller, @user, {discussion_topics: [topic.id]}, 'common_cartridge')
content_item_response = subject({discussion_topics: [topic.id]})
expect(content_item_response.title).to eq 'blah'
end
it 'gets the title for a module' do
context_module = context.context_modules.create!(name: 'a module')
content_item_response = described_class.new(context, controller, @user, {modules: [context_module.id]}, 'common_cartridge')
content_item_response = subject({modules: [context_module.id]})
expect(content_item_response.title).to eq 'a module'
end
it 'gets the title for a page' do
page = context.wiki.wiki_pages.create!(title: 'a page')
content_item_response = described_class.new(context, controller, @user, {pages: [page.id]}, 'common_cartridge')
content_item_response = subject({pages: [page.id]})
expect(content_item_response.title).to eq 'a page'
end
@ -135,20 +164,20 @@ describe Lti::ContentItemResponse do
context_module = context.context_modules.create!(name: 'a module')
topic = context.discussion_topics.create!(:title => "blah")
tag = context_module.add_item(:id => topic.id, :type => 'discussion_topic')
content_item_response = described_class.new(context, controller, @user, {module_items: [tag.id]}, 'common_cartridge')
content_item_response = subject({module_items: [tag.id]})
expect(content_item_response.title).to eq 'blah'
end
it 'gets the title for a quiz' do
quiz = context.quizzes.create!(title: 'a quiz')
content_item_response = described_class.new(context, controller, @user, {quizzes: [quiz.id]}, 'common_cartridge')
content_item_response = subject({quizzes: [quiz.id]})
expect(content_item_response.title).to eq 'a quiz'
end
it 'gets the title for a course' do
context_module = context.context_modules.create!(name: 'a module')
quiz = context.quizzes.create!(title: 'a quiz')
content_item_response = described_class.new(context, controller, @user, {modules: [context_module.id], quizzes: [quiz.id]}, 'common_cartridge')
content_item_response = subject({modules: [context_module.id], quizzes: [quiz.id]})
expect(content_item_response.title).to eq 'Unnamed Course'
end
@ -157,13 +186,13 @@ describe Lti::ContentItemResponse do
describe '#content_type' do
it 'gets the files content_type' do
file = attachment_model(context: context)
content_item_response = described_class.new(context, controller, @user, {files: [file.id]}, 'common_cartridge')
content_item_response = subject({files: [file.id]})
expect(content_item_response.content_type).to eq 'application/loser'
end
it 'gets the content_type for non files' do
quiz = context.quizzes.create!(title: 'a quiz')
content_item_response = described_class.new(context, controller, @user, {quizzes: [quiz.id]}, 'common_cartridge')
content_item_response = subject({quizzes: [quiz.id]})
expect(content_item_response.content_type).to eq 'application/vnd.instructure.api.content-exports.quiz'
end
end
@ -171,13 +200,13 @@ describe Lti::ContentItemResponse do
describe '#url' do
it 'gets the id for a file' do
file = attachment_model(context: context)
content_item_response = described_class.new(context, controller, @user, {files: [file.id]}, 'common_cartridge')
content_item_response = subject({files: [file.id]})
expect(content_item_response.url).to eq 'file_download_url'
end
it 'gets the id for non files' do
quiz = context.quizzes.create!(title: 'a quiz')
content_item_response = described_class.new(context, controller, @user, {quizzes: [quiz.id]}, 'common_cartridge')
content_item_response = subject({quizzes: [quiz.id]})
expect(content_item_response.url).to include 'api_export_url'
end
end
@ -185,7 +214,7 @@ describe Lti::ContentItemResponse do
describe '#as_json' do
it 'generates the json for ContentItemSelectionResponse' do
quiz = context.quizzes.create!(title: 'a quiz')
content_item_response = described_class.new(context, controller, @user, {quizzes: [quiz.id]}, 'common_cartridge')
content_item_response = subject({quizzes: [quiz.id]})
json = content_item_response.as_json(lti_message_type: 'ContentItemSelectionResponse')
expect(json['@context']).to eq "http://purl.imsglobal.org/ctx/lti/v1/ContentItemPlacement"
expect(json['@graph'].first['@type']).to eq "ContentItemPlacement"
@ -197,7 +226,7 @@ describe Lti::ContentItemResponse do
it 'generates the json for ContentItemSelection' do
quiz = context.quizzes.create!(title: 'a quiz')
content_item_response = described_class.new(context, controller, @user, {quizzes: [quiz.id]}, 'common_cartridge')
content_item_response = subject({quizzes: [quiz.id]})
json = content_item_response.as_json(lti_message_type: 'ContentItemSelection')
expect(json['@context']).to eq "http://purl.imsglobal.org/ctx/lti/v1/ContentItem"
expect(json['@graph'].first['@type']).to eq "FileItem"
@ -209,4 +238,4 @@ describe Lti::ContentItemResponse do
end
end
end