add success include param for upload preflight

refs CORE-20 test plan: - test file upload api with success_include[]=avatar on the preflight request - the final response on success should include the avatar property - test with local storage, s3, and InstFS Change-Id: I974197944d0f84ad0b89a628ab8604f50cdec45e Reviewed-on: https://gerrit.instructure.com/144456 Reviewed-by: Ryan Shaw <[email protected]> Reviewed-by: Jacob Fugal <[email protected]> Tested-by: Jenkins QA-Review: Tucker McKnight <[email protected]> Product-Review: Brent Burgoyne <[email protected]>
jiniaoxu · Apr 4, 2018 · ce3379a · ce3379a
1 parent e1b9a42
commit ce3379a
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 8 deletions.
diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb
@@ -747,7 +747,8 @@ def create_pending
           size: params[:attachment][:size],
           parent_folder_id: params[:attachment][:folder_id],
           on_duplicate: params[:attachment][:on_duplicate],
-          no_redirect: params[:no_redirect]
+          no_redirect: params[:no_redirect],
+          success_include: params[:success_include]
         })
     end
   end
@@ -763,7 +764,11 @@ def api_create
     @attachment.uploaded_data = params[:file] || params[:attachment] && params[:attachment][:uploaded_data]
     if @attachment.save
       # for consistency with the s3 upload client flow, we redirect to the success url here to finish up
-      redirect_to api_v1_files_create_success_url(@attachment, :uuid => @attachment.uuid, :on_duplicate => params[:on_duplicate], :quota_exemption => params[:quota_exemption])
+      redirect_to api_v1_files_create_success_url(@attachment,
+        uuid: @attachment.uuid,
+        on_duplicate: params[:on_duplicate],
+        quota_exemption: params[:quota_exemption],
+        include: params[:success_include])
     else
       head :bad_request
     end

diff --git a/doc/api/file_uploads.md b/doc/api/file_uploads.md
@@ -38,6 +38,7 @@ Arguments:
   <dt>parent_folder_path</dt> <dd>The path of the folder to store the file in. The path separator is the forward slash `/`, never a back slash. The folder will be created if it does not already exist. This parameter only applies to file uploads in a context that has folders, such as a user, a course, or a group. If this and parent_folder_id are sent an error will be returned. If neither is given, a default folder will be used.</dd>
   <dt>folder</dt> <dd>[deprecated] Use parent_folder_path instead.</dd>
   <dt>on_duplicate</dt> <dd>How to handle duplicate filenames. If `overwrite`, then this file upload will overwrite any other file in the folder with the same name. If `rename`, then this file will be renamed if another file in the folder exists with the given name. If no parameter is given, the default is `overwrite`. This doesn't apply to file uploads in a context that doesn't have folders.</dd>
+  <dt>success_include[]</dt> <dd>An array of additional information to include in the upload success response. See <a href="files.html#method.files.api_show">Files API</a> for more information.</dd>
 </dl>
 
 Example Request:

diff --git a/lib/api/v1/attachment.rb b/lib/api/v1/attachment.rb
@@ -236,7 +236,8 @@ def api_attachment_preflight(context, request, opts = {})
         quota_exempt: !opts[:check_quota],
         capture_url: api_v1_files_capture_url,
         target_url: params[:url],
-        progress_json: progress_json
+        progress_json: progress_json,
+        include_param: params[:success_include]
       )
     else
       @attachment = Attachment.new
@@ -273,12 +274,14 @@ def api_attachment_preflight(context, request, opts = {})
           @current_pseudonym,
           api_v1_files_create_url(
             on_duplicate: on_duplicate,
-            quota_exemption: quota_exemption),
+            quota_exemption: quota_exemption,
+            success_include: params[:success_include]),
           api_v1_files_create_success_url(
             @attachment,
             uuid: @attachment.uuid,
             on_duplicate: on_duplicate,
-            quota_exemption: quota_exemption),
+            quota_exemption: quota_exemption,
+            include: params[:success_include]),
           ssl: request.ssl?,
           file_param: opts[:file_param],
           no_redirect: params[:no_redirect])

diff --git a/lib/inst_fs.rb b/lib/inst_fs.rb
@@ -66,7 +66,7 @@ def jwt_secret
       Base64.decode64(setting("secret"))
     end
 
-    def upload_preflight_json(context:, user:, acting_as:, folder:, filename:, content_type:, quota_exempt:, on_duplicate:, capture_url:, target_url: nil, progress_json: nil)
+    def upload_preflight_json(context:, user:, acting_as:, folder:, filename:, content_type:, quota_exempt:, on_duplicate:, capture_url:, target_url: nil, progress_json: nil, include_param: nil)
       raise ArgumentError unless !!target_url == !!progress_json # these params must both be present or both absent
 
       token = upload_jwt(user, acting_as, capture_url,
@@ -77,7 +77,8 @@ def upload_preflight_json(context:, user:, acting_as:, folder:, filename:, conte
         root_account_id: context.respond_to?(:root_account) && context.root_account.global_id.to_s,
         quota_exempt: !!quota_exempt,
         on_duplicate: on_duplicate,
-        progress_id: progress_json && progress_json[:id])
+        progress_id: progress_json && progress_json[:id],
+        include: include_param)
 
       upload_params = {
         filename: filename,

diff --git a/spec/apis/v1/files_controller_api_spec.rb b/spec/apis/v1/files_controller_api_spec.rb
@@ -47,6 +47,61 @@ def api_get_json
     course_with_teacher(:active_all => true, :user => user_with_pseudonym)
   end
 
+  describe 'create file' do
+    def call_course_create_file
+      api_call(:post, "/api/v1/courses/#{@course.id}/files", {
+        controller: 'courses',
+        action: 'create_file',
+        course_id: @course.id,
+        format: 'json',
+        name: 'test_file.png',
+        size: '12345',
+        content_type: 'image/png',
+        success_include: ['avatar'],
+        no_redirect: 'true'
+      })
+    end
+
+    it 'includes success_include param in file create url' do
+      local_storage!
+      json = call_course_create_file
+      query = Rack::Utils.parse_nested_query(URI(json['upload_url']).query)
+      expect(query['success_include']).to include('avatar')
+    end
+
+    it 'includes include param in create success url' do
+      s3_storage!
+      json = call_course_create_file
+      query = Rack::Utils.parse_nested_query(URI(json['upload_params']['success_url']).query)
+      expect(query['include']).to include('avatar')
+    end
+
+    it 'includes include capture param in inst_fs token' do
+      secret = 'secret'
+      allow(InstFS).to receive(:enabled?).and_return true
+      allow(InstFS).to receive(:jwt_secret).and_return(secret)
+      json = call_course_create_file
+      query = Rack::Utils.parse_nested_query(URI(json['upload_url']).query)
+      payload = Canvas::Security.decode_jwt(query['token'], [secret])
+      expect(payload['capture_params']['include']).to include('avatar')
+    end
+  end
+
+  describe 'api_create' do
+    it 'includes success_include as include when redirecting' do
+      local_storage!
+      a = attachment_model(workflow_state: :unattached)
+      params = a.ajax_upload_params(@pseudonym, '/url', '/s3')[:upload_params]
+      raw_api_call(:post, "/files_api", params.merge({
+        controller: 'files',
+        action: 'api_create',
+        success_include: ['avatar'],
+      }))
+      query = Rack::Utils.parse_nested_query(URI(response.headers['Location']).query)
+      expect(query['include']).to include('avatar')
+    end
+  end
+
   describe "api_create_success" do
     before :once do
       @attachment = Attachment.new

diff --git a/spec/lib/inst_fs_spec.rb b/spec/lib/inst_fs_spec.rb
@@ -144,6 +144,7 @@
     let(:content_type) { 'text/plain' }
     let(:quota_exempt) { true }
     let(:on_duplicate) { 'rename' }
+    let(:include_param) { ['avatar'] }
     let(:capture_url) { 'http://canvas.host/api/v1/files/capture' }
 
     let(:default_args) do
@@ -156,7 +157,8 @@
         content_type: content_type,
         quota_exempt: quota_exempt,
         on_duplicate: on_duplicate,
-        capture_url: capture_url
+        capture_url: capture_url,
+        include_param: include_param
       }
     end
 
@@ -229,6 +231,10 @@
         it "include the on_duplicate method" do
           expect(capture_params['on_duplicate']).to eq on_duplicate
         end
+
+        it "include the inlcude options" do
+          expect(capture_params['include']).to eq include_param
+        end
       end
     end