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 <mbd@instructure.com>
Reviewed-by: Andrew Butterfield <abutterfield@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Product-Review: Nathan Mills <nathanm@instructure.com>

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]

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)
         }

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

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

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

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

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

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}"} }

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
 

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'