From 7aec3da9458f91505fa44b7907beee9b6d5837dc Mon Sep 17 00:00:00 2001 From: Nathan Mills Date: Tue, 14 Mar 2017 06:32:23 -0600 Subject: [PATCH] lti2 submission attachment download endpoint fixes PLAT-2354 test plan: -create a submission with an attachment on an assignment that has the originality detection tool associated -hit the lti2 endpoint to get the submission json -from the submission json get the attachment url -use the lti2 credentials with the attachment url to download the attachment -it should download the attachment -try the following, they should all return 401: -try to download the attachment without the lti2 credentials -try to download the attachment using a canvas session -try to monkey with the url to download a different attachment you shouldn't have access to, i.e. on a different assignment Change-Id: Ib38bbdbe9a1a649826a6bc98dd0d19b71a32635e Reviewed-on: https://gerrit.instructure.com/104989 Tested-by: Jenkins Reviewed-by: Michael Brewer-Davis Reviewed-by: Andrew Butterfield QA-Review: August Thornton Product-Review: Nathan Mills --- app/controllers/files_controller.rb | 39 +------ .../lti/ims/authorization_controller.rb | 4 +- .../lti/submissions_api_controller.rb | 34 +++++- app/helpers/attachment_helper.rb | 36 ++++++ config/routes.rb | 1 + lib/lti/oauth2/access_token.rb | 2 +- spec/apis/lti/ims/authorization_api_spec.rb | 7 ++ spec/apis/lti/lti2_api_spec_helper.rb | 2 + spec/apis/lti/submissions_api_spec.rb | 103 ++++++++++++------ spec/lib/lti/oauth2/access_token_spec.rb | 5 + 10 files changed, 163 insertions(+), 70 deletions(-) diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb index d1ae3900e69..487de0c8b93 100644 --- a/app/controllers/files_controller.rb +++ b/app/controllers/files_controller.rb @@ -590,18 +590,6 @@ class FilesController < ApplicationController show end - # checks if for the current root account there's a 'files' domain - # defined and tried to use that. This way any files that we stream through - # a canvas URL are at least on a separate subdomain and the javascript - # won't be able to access or update data with AJAX requests. - def safer_domain_available? - if !@files_domain && request.host_with_port != HostUrl.file_host(@domain_root_account, request.host_with_port) - @safer_domain_host = HostUrl.file_host_with_shard(@domain_root_account, request.host_with_port) - end - !!@safer_domain_host - end - protected :safer_domain_available? - def attachment_content @attachment = @context.attachments.active.find(params[:file_id]) if authorized_action(@attachment, @current_user, :update) @@ -647,30 +635,15 @@ class FilesController < ApplicationController user ||= api_find(User, params[:user_id]) if params[:user_id].present? attachment.context_module_action(user, :read) if user && !params[:preview] log_asset_access(@attachment, "files", "files") unless params[:preview] - set_cache_header(attachment) - if safer_domain_available? - redirect_to safe_domain_file_url(attachment, @safer_domain_host, params[:verifier], !inline) - elsif Attachment.local_storage? - @headers = false if @files_domain - send_file(attachment.full_filename, :type => attachment.content_type_with_encoding, :disposition => (inline ? 'inline' : 'attachment'), :filename => attachment.display_name) - elsif redirect_to_s3 - redirect_to(inline ? attachment.inline_url : attachment.download_url) - else - send_file_headers!( :length=> attachment.s3object.content_length, :filename=>attachment.filename, :disposition => 'inline', :type => attachment.content_type_with_encoding) - render :status => 200, :text => attachment.s3object.get.body.read - end + render_or_redirect_to_stored_file( + attachment: attachment, + verifier: params[:verifier], + inline: inline, + redirect_to_s3: redirect_to_s3 + ) end protected :send_stored_file - def set_cache_header(attachment) - unless attachment.content_type.match(/\Atext/) || attachment.extension == '.html' || attachment.extension == '.htm' - cancel_cache_buster - #set cache to expoire in 1 day, max-age take seconds, and Expires takes a date - response.headers["Cache-Control"] = "private, max-age=86400" - response.headers["Expires"] = 1.day.from_now.httpdate - end - end - def create_pending @context = Context.find_by_asset_string(params[:attachment][:context_code]) @asset = Context.find_asset_by_asset_string(params[:attachment][:asset_string], @context) if params[:attachment][:asset_string] diff --git a/app/controllers/lti/ims/authorization_controller.rb b/app/controllers/lti/ims/authorization_controller.rb index 31a161f36c6..87c12ba29f8 100644 --- a/app/controllers/lti/ims/authorization_controller.rb +++ b/app/controllers/lti/ims/authorization_controller.rb @@ -100,8 +100,10 @@ module Lti code: code ) jwt_validator.validate! + file_host, _ = HostUrl.file_host_with_shard(@domain_root_account || Account.default, request.host_with_port) + aud = [request.host, request.protocol + file_host] render json: { - access_token: Lti::Oauth2::AccessToken.create_jwt(aud: request.host, sub: jwt_validator.sub, reg_key: code).to_s, + access_token: Lti::Oauth2::AccessToken.create_jwt(aud: aud, sub: jwt_validator.sub, reg_key: code).to_s, token_type: 'bearer', expires_in: Setting.get('lti.oauth2.access_token.expiration', 1.hour.to_s) } diff --git a/app/controllers/lti/submissions_api_controller.rb b/app/controllers/lti/submissions_api_controller.rb index 68abd69b816..92a874b8174 100644 --- a/app/controllers/lti/submissions_api_controller.rb +++ b/app/controllers/lti/submissions_api_controller.rb @@ -109,6 +109,7 @@ module Lti class SubmissionsApiController < ApplicationController include Lti::Ims::AccessTokenHelper include Api::V1::Submission + include AttachmentHelper SUBMISSION_SERVICE = 'vnd.Canvas.submission' SUBMISSION_HISTORY_SERVICE = 'vnd.Canvas.submission.history' @@ -151,8 +152,31 @@ module Lti render json: submissions.map { |s| api_json(s) } end + def attachment + attachment = Attachment.find(params[:attachment_id]) + render_unauthorized and return unless attachment_for_submission?(attachment) + render_or_redirect_to_stored_file( + attachment: attachment, redirect_to_s3: true) + end + + + def attachment_url(attachment_id) + account = @domain_root_account || Account.default + host, shard = HostUrl.file_host_with_shard(account, request.host_with_port) + res = "#{request.protocol}#{host}" + shard.activate do + res + lti_submission_attachment_download_path(params[:assignment_id], params[:submission_id], attachment_id) + end + end + private + def attachment_for_submission?(attachment) + submissions = Submission.bulk_load_versioned_attachments(submission.submission_history + [submission]) + attachments = submissions.map { |s| s.versioned_attachments }.flatten + attachments.include?(attachment) + end + def submission @_submission ||= Submission.find(params[:submission_id]) end @@ -165,14 +189,20 @@ module Lti def api_json(submission) submission_attributes = %w(id body url submitted_at assignment_id user_id submission_type workflow_state attempt attachments) - attachment_attributes = %w(id display_name filename content-type url size created_at updated_at) sub_hash = filtered_json(model: submission, whitelist: submission_attributes) sub_hash[:user_id] = Lti::Asset.opaque_identifier_for(User.find(sub_hash[:user_id])) attachments = submission.versioned_attachments - sub_hash[:attachments] = attachments.map { |a| filtered_json(model: a, whitelist: attachment_attributes) } + sub_hash[:attachments] = attachments.map { |a| attachment_json(a) } sub_hash end + def attachment_json(attachment) + attachment_attributes = %w(id display_name filename content-type size created_at updated_at) + attach = filtered_json(model: attachment, whitelist: attachment_attributes) + attach[:url] = attachment_url(attachment.id) + attach + end + def filtered_json(model:, whitelist:) model.as_json(include_root: false).select { |k, _| whitelist.include?(k) } end diff --git a/app/helpers/attachment_helper.rb b/app/helpers/attachment_helper.rb index 6feeab8d63d..5f24aa02284 100644 --- a/app/helpers/attachment_helper.rb +++ b/app/helpers/attachment_helper.rb @@ -56,4 +56,40 @@ module AttachmentHelper crocodoc_session_url: attachment.crocodoc_url(@current_user), } end + + def render_or_redirect_to_stored_file(attachment:, verifier: nil, inline: false, redirect_to_s3: false) + set_cache_header(attachment) + if safer_domain_available? + redirect_to safe_domain_file_url(attachment, @safer_domain_host, verifier, !inline) + elsif Attachment.local_storage? + @headers = false if @files_domain + send_file(attachment.full_filename, :type => attachment.content_type_with_encoding, :disposition => (inline ? 'inline' : 'attachment'), :filename => attachment.display_name) + elsif redirect_to_s3 + redirect_to(inline ? attachment.inline_url : attachment.download_url) + else + send_file_headers!( :length=> attachment.s3object.content_length, :filename=>attachment.filename, :disposition => 'inline', :type => attachment.content_type_with_encoding) + render :status => 200, :text => attachment.s3object.get.body.read + end + end + + # checks if for the current root account there's a 'files' domain + # defined and tried to use that. This way any files that we stream through + # a canvas URL are at least on a separate subdomain and the javascript + # won't be able to access or update data with AJAX requests. + def safer_domain_available? + if !@files_domain && request.host_with_port != HostUrl.file_host(@domain_root_account, request.host_with_port) + @safer_domain_host = HostUrl.file_host_with_shard(@domain_root_account, request.host_with_port) + end + !!@safer_domain_host + end + + def set_cache_header(attachment) + unless attachment.content_type.match(/\Atext/) || attachment.extension == '.html' || attachment.extension == '.htm' + cancel_cache_buster + #set cache to expoire in 1 day, max-age take seconds, and Expires takes a date + response.headers["Cache-Control"] = "private, max-age=86400" + response.headers["Expires"] = 1.day.from_now.httpdate + end + end + end diff --git a/config/routes.rb b/config/routes.rb index 8aabd6702a7..7a67da6488f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1980,6 +1980,7 @@ CanvasRails::Application.routes.draw do scope(controller: 'lti/submissions_api') do get "assignments/:assignment_id/submissions/:submission_id", action: :show get "assignments/:assignment_id/submissions/:submission_id/history", action: :history + get "assignments/:assignment_id/submissions/:submission_id/attachment/:attachment_id", action: :attachment, as: :lti_submission_attachment_download end # Originality Report Service diff --git a/lib/lti/oauth2/access_token.rb b/lib/lti/oauth2/access_token.rb index 15c32df2bad..abbf69dfde7 100644 --- a/lib/lti/oauth2/access_token.rb +++ b/lib/lti/oauth2/access_token.rb @@ -31,7 +31,7 @@ module Lti decoded_jwt = Canvas::Security.decode_jwt(jwt) check_required_assertions(decoded_jwt.keys) raise InvalidTokenError, 'invalid iss' if decoded_jwt['iss'] != ISS - raise InvalidTokenError, 'invalid aud' if decoded_jwt[:aud] != aud + raise InvalidTokenError, 'invalid aud' unless [*decoded_jwt[:aud]].include?(aud) raise InvalidTokenError, 'iat must be in the past' unless Time.zone.at(decoded_jwt['iat']) < Time.zone.now true rescue InvalidTokenError diff --git a/spec/apis/lti/ims/authorization_api_spec.rb b/spec/apis/lti/ims/authorization_api_spec.rb index 3437336e450..fb519f5af63 100644 --- a/spec/apis/lti/ims/authorization_api_spec.rb +++ b/spec/apis/lti/ims/authorization_api_spec.rb @@ -100,6 +100,13 @@ module Lti expect(response.body).to eq({error: 'invalid_grant'}.to_json) end + it "adds the file_host and the request host to the aud" do + post auth_endpoint, params + file_host, _ = HostUrl.file_host_with_shard(@domain_root_account || Account.default, request.host_with_port) + jwt = JSON::JWT.decode(JSON.parse(response.body)["access_token"], :skip_verification) + expect(jwt["aud"]).to match_array [request.host, request.protocol + file_host] + end + context "developer credentials" do let(:raw_jwt) do diff --git a/spec/apis/lti/lti2_api_spec_helper.rb b/spec/apis/lti/lti2_api_spec_helper.rb index f59e1559e96..29233ebfc96 100644 --- a/spec/apis/lti/lti2_api_spec_helper.rb +++ b/spec/apis/lti/lti2_api_spec_helper.rb @@ -10,6 +10,8 @@ RSpec.shared_context "lti2_api_spec_helper", :shared_context => :metadata do end let(:access_token) do aud = host rescue (@request || request).host + file_host, _ = HostUrl.file_host_with_shard(account) + aud = [aud, file_host] Lti::Oauth2::AccessToken.create_jwt(aud: aud, sub: tool_proxy.guid) end let(:request_headers) { {Authorization: "Bearer #{access_token}"} } diff --git a/spec/apis/lti/submissions_api_spec.rb b/spec/apis/lti/submissions_api_spec.rb index 71690aeba48..877049ded6e 100644 --- a/spec/apis/lti/submissions_api_spec.rb +++ b/spec/apis/lti/submissions_api_spec.rb @@ -10,21 +10,34 @@ module Lti let(:service_name) { SubmissionsApiController::SUBMISSION_SERVICE } let(:submission) do - @assignment.submit_homework(@user, submission_type: 'online_upload', - attachments: [@attachment]) + assignment.submit_homework(student, submission_type: 'online_upload', + attachments: [attachment]) end + let(:mock_file) do + stub_file_data('myfile.txt', nil, "plain/txt") + end + + let(:attachment) do + student.attachments.create! uploaded_data: dummy_io, filename: 'doc.doc', display_name: 'doc.doc', context: student + end + + let(:assignment) do + a = course.assignments.new(:title => "some assignment") + a.workflow_state = "published" + a.tool_settings_tool = message_handler + a.save! + a + end + + let(:student) { course_with_student(active_all: true, course: course); @user } + + before do - course_with_teacher(active_all: true, course: course) - course_with_student(active_all: true, course: course) + mock_sub_helper = instance_double("Lti::AssignmentSubscriptionsHelper", create_subscription: "123") + allow(Lti::AssignmentSubscriptionsHelper).to receive(:new).and_return(mock_sub_helper) tool_proxy.raw_data['enabled_capability'] << ResourcePlacement::SIMILARITY_DETECTION_LTI2 tool_proxy.save! - @assignment = course.assignments.new(:title => "some assignment") - @assignment.workflow_state = "published" - AssignmentConfigurationToolLookup.any_instance.stubs(:create_subscription).returns true - @assignment.tool_settings_tool = message_handler - @assignment.save - @attachment = attachment_model(filename: "submission.doc", context: @user) end RSpec.shared_examples "authorization" do @@ -41,8 +54,8 @@ module Lti end it "returns a 401 if the tool is not associated with the assignment" do - @assignment.tool_settings_tool = [] - @assignment.save! + assignment.tool_settings_tool = [] + assignment.save! get endpoint, {}, request_headers expect(response.code).to eq '401' end @@ -60,8 +73,9 @@ module Lti end describe "#show" do - let(:endpoint) { "/api/lti/assignments/#{@assignment.id}/submissions/#{submission.id}" } + let(:endpoint) { "/api/lti/assignments/#{assignment.id}/submissions/#{submission.id}" } include_examples "authorization" + it "returns a submission json object" do now = Time.now.utc Timecop.freeze(now) do @@ -72,18 +86,19 @@ module Lti "body" => nil, "url" => nil, "submitted_at" => now.iso8601, - "assignment_id" => @assignment.id, - "user_id" => Lti::Asset.opaque_identifier_for(@user), + "assignment_id" => assignment.id, + "user_id" => Lti::Asset.opaque_identifier_for(student), "submission_type" => "online_upload", "workflow_state" => "submitted", "attempt" => 1, "attachments" => [ { - "id" => @attachment.id, - "size" => 100, - "filename" => "submission.doc", - "display_name" => "submission.doc", + "id" => attachment.id, + "size" => attachment.size, + "url" => controller.attachment_url(attachment.id), + "filename" => attachment.filename, + "display_name" => attachment.display_name, "created_at" => now.iso8601, "updated_at" => now.iso8601 } @@ -98,7 +113,7 @@ module Lti describe "#history" do - let(:endpoint) { "/api/lti/assignments/#{@assignment.id}/submissions/#{submission.id}/history" } + let(:endpoint) { "/api/lti/assignments/#{assignment.id}/submissions/#{submission.id}/history" } include_examples "authorization" it "returns the submission history as an array of JSON objects" do now = Time.now.utc @@ -111,18 +126,19 @@ module Lti "body" => nil, "url" => nil, "submitted_at" => now.iso8601, - "assignment_id" => @assignment.id, - "user_id" => Lti::Asset.opaque_identifier_for(@user), + "assignment_id" => assignment.id, + "user_id" => Lti::Asset.opaque_identifier_for(student), "submission_type" => "online_upload", "workflow_state" => "submitted", "attempt" => 1, "attachments" => [ { - "id" => @attachment.id, - "size" => 100, - "filename" => "submission.doc", - "display_name" => "submission.doc", + "id" => attachment.id, + "size" => attachment.size, + "url" => controller.attachment_url(attachment.id), + "filename" => attachment.filename, + "display_name" => attachment.display_name, "created_at" => now.iso8601, "updated_at" => now.iso8601 } @@ -134,27 +150,48 @@ module Lti end it "sends back versioned attachments" do - attachments = [attachment_model(filename: "submission-a.doc", :context => @user)] + attachments = [attachment_model(filename: "submission-a.doc", :context => student)] Timecop.freeze(10.second.ago) do - @assignment.submit_homework(@user, submission_type: 'online_upload', - attachments: [attachments[0]]) + assignment.submit_homework(student, submission_type: 'online_upload', + attachments: [attachments[0]]) end - attachments << attachment_model(filename: "submission-b.doc", :context => @user) + attachments << attachment_model(filename: "submission-b.doc", :context => student) Timecop.freeze(5.second.ago) do - @assignment.submit_homework @user, attachments: [attachments[1]] + assignment.submit_homework student, attachments: [attachments[1]] end - attachments << attachment_model(filename: "submission-c.doc", :context => @user) + attachments << attachment_model(filename: "submission-c.doc", :context => student) Timecop.freeze(1.second.ago) do - @assignment.submit_homework @user, attachments: [attachments[2]] + assignment.submit_homework student, attachments: [attachments[2]] end get endpoint, {}, request_headers json = JSON.parse(response.body) expect(json[0]["attachments"].first["id"]).to_not equal json[1]["attachments"].first["id"] end + end + describe "#attachment" do + + let(:endpoint) { "/api/lti/assignments/#{assignment.id}/submissions/#{submission.id}/attachment/#{attachment.id}" } + include_examples 'authorization' + + it "allows a user to download a file" do + get "/api/lti/assignments/#{assignment.id}/submissions/#{submission.id}", {}, request_headers + json = JSON.parse(response.body) + url = json["attachments"].first["url"] + get url, {}, request_headers + expect(response.content_type.to_s).to eq attachment.content_type + end + + it "returns a 401 if the attachment isn't associated to the assignment" do + get "/api/lti/assignments/#{assignment.id}/submissions/#{submission.id}", {}, request_headers + attachment1 = Attachment.create!(context: Account.create!, filename: "test.txt", content_type: "text/plain") + endpoint = "/api/lti/assignments/#{assignment.id}/submissions/#{submission.id}/attachment/#{attachment1.id}" + get controller.attachment_url(attachment1.id), {}, request_headers + expect(response.code).to eq "401" + end end diff --git a/spec/lib/lti/oauth2/access_token_spec.rb b/spec/lib/lti/oauth2/access_token_spec.rb index cdb38ceca66..17ce5e645ae 100644 --- a/spec/lib/lti/oauth2/access_token_spec.rb +++ b/spec/lib/lti/oauth2/access_token_spec.rb @@ -123,6 +123,11 @@ module Lti expect{ access_token.validate! }.to raise_error InvalidTokenError, 'invalid aud' end + it "handles an array for aud" do + body[:aud] = [aud, 'file_host'] + expect{ access_token.validate!}.to_not raise_error + end + it "raises an InvalidTokenError if the 'iat' is in the future" do body[:iat] = 1.hour.from_now expect{ access_token.validate! }.to raise_error InvalidTokenError, 'iat must be in the past'