user real user when available in instfs JWTs

refs RECNVS-264 but still pass the @current_user for access and upload JWTs, so that it can go in a separate claim if its different test-plan: - smoke test an inst-fs upload. should still work Change-Id: I4be9cd4049c83848e69aae37500ae9f4b96392b4 Reviewed-on: https://gerrit.instructure.com/142334 Tested-by: Jenkins Reviewed-by: Michael Jasper <[email protected]> QA-Review: Jonathan Featherstone <[email protected]> Product-Review: Jacob Fugal <[email protected]>
ralacas · Mar 6, 2018 · e601437 · e601437
1 parent 09badef
commit e601437
Show file tree

Hide file tree

Showing 12 changed files with 69 additions and 42 deletions.
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
@@ -369,6 +369,7 @@ def reject!(cause, status=:bad_request)
   def logged_in_user
     @real_current_user || @current_user
   end
+  helper_method :logged_in_user
 
   def not_fake_student_user
     @current_user && @current_user.fake_student? ? logged_in_user : @current_user
@@ -1911,8 +1912,8 @@ def destroy_session
   end
 
   def logout_current_user
-    @current_user.try(:stamp_logout_time!)
-    InstFS.logout(@current_user)
+    logged_in_user.try(:stamp_logout_time!)
+    InstFS.logout(logged_in_user)
     destroy_session
   end
 

diff --git a/app/controllers/eportfolios_controller.rb b/app/controllers/eportfolios_controller.rb
@@ -174,13 +174,14 @@ def export
       else
         respond_to do |format|
           if @attachment.zipped?
-            if Attachment.s3_storage?
-              format.html { redirect_to @attachment.inline_url_for_user(@current_user) }
-              format.zip { redirect_to @attachment.inline_url_for_user(@current_user) }
-            else
+            if @attachment.stored_locally?
               cancel_cache_buster
               format.html { send_file(@attachment.full_filename, :type => @attachment.content_type_with_encoding, :disposition => 'inline') }
               format.zip { send_file(@attachment.full_filename, :type => @attachment.content_type_with_encoding, :disposition => 'inline') }
+            else
+              inline_url = @attachment.inline_url_for_user(logged_in_user, @current_user)
+              format.html { redirect_to inline_url }
+              format.zip { redirect_to inline_url }
             end
             format.json { render :json => @attachment.as_json(:methods => :readable_size) }
           else

diff --git a/app/controllers/gradebooks_controller.rb b/app/controllers/gradebooks_controller.rb
@@ -371,7 +371,7 @@ def old_gradebook_env
         ),
         export_gradebook_csv_url: course_gradebook_csv_url,
         gradebook_csv_progress: @last_exported_gradebook_csv.try(:progress),
-        attachment_url: last_exported_attachment && last_exported_attachment.download_url_for_user(@current_user),
+        attachment_url: last_exported_attachment&.download_url_for_user(logged_in_user, @current_user),
         attachment: last_exported_attachment,
         sis_app_url: Setting.get('sis_app_url', nil),
         sis_app_token: Setting.get('sis_app_token', nil),

diff --git a/app/controllers/quizzes/quiz_submissions_controller.rb b/app/controllers/quizzes/quiz_submissions_controller.rb
@@ -199,10 +199,7 @@ def generate_submission_zip(quiz, context)
 
     respond_to do |format|
       if attachment.zipped?
-        if Attachment.s3_storage?
-          format.html { redirect_to attachment.inline_url_for_user(@current_user) }
-          format.zip { redirect_to attachment.inline_url_for_user(@current_user) }
-        else
+        if attachment.stored_locally?
           cancel_cache_buster
 
           format.html do
@@ -218,6 +215,10 @@ def generate_submission_zip(quiz, context)
               :disposition => 'inline'
             })
           end
+        else
+          inline_url = attachment.inline_url_for_user(logged_in_user, @current_user)
+          format.html { redirect_to inline_url }
+          format.zip { redirect_to inline_url }
         end
 
         format.any(:json, :jsonapi) { render :json => attachment.as_json(:methods => :readable_size) }

diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb
@@ -667,10 +667,7 @@ def generate_submission_zip(assignment, context)
 
     respond_to do |format|
       if attachment.zipped?
-        if Attachment.s3_storage?
-          format.html { redirect_to attachment.inline_url_for_user(@current_user) }
-          format.zip { redirect_to attachment.inline_url_for_user(@current_user) }
-        else
+        if attachment.stored_locally?
           cancel_cache_buster
 
           format.html do
@@ -686,6 +683,10 @@ def generate_submission_zip(assignment, context)
               :disposition => 'inline'
             })
           end
+        else
+          inline_url = attachment.inline_url_for_user(logged_in_user, @current_user)
+          format.html { redirect_to inline_url }
+          format.zip { redirect_to inline_url }
         end
         format.json { render :json => attachment.as_json(:methods => :readable_size) }
       else

diff --git a/app/helpers/attachment_helper.rb b/app/helpers/attachment_helper.rb
@@ -72,9 +72,9 @@ def render_or_redirect_to_stored_file(attachment:, verifier: nil, inline: false)
       send_file_headers!( :length=> attachment.s3object.content_length, :filename=>attachment.filename, :disposition => 'inline', :type => attachment.content_type_with_encoding)
       render body: attachment.s3object.get.body.read
     elsif inline
-      redirect_to attachment.inline_url_for_user(@current_user)
+      redirect_to attachment.inline_url_for_user(logged_in_user, @current_user)
     else
-      redirect_to attachment.download_url_for_user(@current_user)
+      redirect_to attachment.download_url_for_user(logged_in_user, @current_user)
     end
   end
 

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
@@ -774,35 +774,32 @@ def authenticated_url(**options)
     else
       # attachment_fu doesn't like the extra option when building s3 urls
       options.delete(:user)
+      options.delete(:acting_as)
       should_download = options.delete(:download)
       disposition = should_download ? "attachment" : "inline"
       options[:response_content_disposition] = "#{disposition}; #{disposition_filename}"
       self.authenticated_s3_url(**options)
     end
   end
 
-  def authenticated_url_for_user(user, **options)
-    authenticated_url(options.merge(user: user))
+  def download_url_for_user(user, acting_as, ttl = url_ttl)
+    authenticated_url(user: user, acting_as: acting_as, expires_in: ttl, download: true)
   end
 
-  def download_url_for_user(user, ttl = url_ttl)
-    authenticated_url_for_user(user, expires_in: ttl, download: true)
-  end
-
-  def inline_url_for_user(user, ttl = url_ttl)
-    authenticated_url_for_user(user, expires_in: ttl, download: false)
+  def inline_url_for_user(user, acting_as, ttl = url_ttl)
+    authenticated_url(user: user, acting_as: acting_as, expires_in: ttl, download: false)
   end
 
   def public_url(**options)
-    authenticated_url_for_user(nil, options)
+    authenticated_url(options.merge(user: nil))
   end
 
   def public_inline_url(ttl = url_ttl)
-    inline_url_for_user(nil, ttl)
+    authenticated_url(user: nil, expires_in: ttl, download: false)
   end
 
   def public_download_url(ttl = url_ttl)
-    download_url_for_user(nil, ttl)
+    authenticated_url(user: nil, expires_in: ttl, download: true)
   end
 
   def url_ttl

diff --git a/app/views/gradebooks/gradebook.html.erb b/app/views/gradebooks/gradebook.html.erb
@@ -115,7 +115,7 @@
           <ul style="display: none;" id="csv_export_options" class="export_dropdown ui-kyle-menu">
             <li class="generate_new_csv"><a href="#"><%=t('CSV File') %></a></li>
             <% if @last_exported_gradebook_csv.present? %>
-              <li><a href="<%= @last_exported_gradebook_csv.attachment.download_url_for_user(@current_user) %>" class="open_in_a_new_tab">
+              <li><a href="<%= @last_exported_gradebook_csv.attachment.download_url_for_user(logged_in_user, @current_user) %>" class="open_in_a_new_tab">
                 <%= t('Previous CSV File (%{time})', time: datetime_string(@last_exported_gradebook_csv.attachment.updated_at)) %>
               </a></li>
             <% else %>

diff --git a/app/views/layouts/_foot.html.erb b/app/views/layouts/_foot.html.erb
@@ -1,4 +1,4 @@
-<%= InstFS.login_pixel(@current_user, session, @domain_root_account) %>
+<%= InstFS.login_pixel(logged_in_user, session, @domain_root_account) %>
 
 <script>
   INST = <%= raw(inst_env.to_json) %>;

diff --git a/lib/api/v1/attachment.rb b/lib/api/v1/attachment.rb
@@ -228,7 +228,8 @@ def api_attachment_preflight(context, request, opts = {})
       else
         json = InstFS.upload_preflight_json(
           context: context,
-          user: @current_user,
+          user: logged_in_user,
+          acting_as: @current_user,
           domain_root_account: @domain_root_account,
           folder: folder,
           filename: infer_upload_filename(params),

diff --git a/lib/inst_fs.rb b/lib/inst_fs.rb
@@ -61,12 +61,12 @@ def jwt_secret
       Base64.decode64(setting("secret"))
     end
 
-    def upload_preflight_json(context:, user:, folder:, filename:, content_type:, quota_exempt:, on_duplicate:, capture_url:, domain_root_account:)
-      token = upload_jwt(user, capture_url, domain_root_account.domain,
+    def upload_preflight_json(context:, user:, acting_as:, folder:, filename:, content_type:, quota_exempt:, on_duplicate:, capture_url:, domain_root_account:)
+      token = upload_jwt(user, acting_as, capture_url, domain_root_account.domain,
         context_type: context.class.to_s,
         context_id: context.global_id.to_s,
-        user_id: user.global_id.to_s,
-        folder_id: folder && folder.global_id.to_s,
+        user_id: acting_as.global_id.to_s,
+        folder_id: folder&.global_id.to_s,
         root_account_id: context.respond_to?(:root_account) && context.root_account.global_id.to_s,
         quota_exempt: !!quota_exempt,
         on_duplicate: on_duplicate)
@@ -132,24 +132,32 @@ def upload_url(token=nil)
     def access_jwt(attachment, options={})
       expires_in = Setting.get('instfs.access_jwt.expiration_hours', '24').to_i.hours
       expires_in = options[:expires_in] || expires_in
-      Canvas::Security.create_jwt({
+      claims = {
         iat: Time.now.utc.to_i,
         user_id: options[:user]&.global_id&.to_s,
         resource: attachment.instfs_uuid,
         host: Attachment.domain_namespace_account.domain,
-      }, expires_in.from_now, self.jwt_secret)
+      }
+      if options[:acting_as] && options[:acting_as] != options[:user]
+        claims[:acting_as_user_id] = options[:acting_as].global_id.to_s
+      end
+      Canvas::Security.create_jwt(claims, expires_in.from_now, self.jwt_secret)
     end
 
-    def upload_jwt(user, capture_url, host, capture_params)
+    def upload_jwt(user, acting_as, capture_url, host, capture_params)
       expires_in = Setting.get('instfs.upload_jwt.expiration_minutes', '10').to_i.minutes
-      Canvas::Security.create_jwt({
+      claims = {
         iat: Time.now.utc.to_i,
         user_id: user.global_id.to_s,
         resource: upload_url,
         capture_url: capture_url,
         host: host,
         capture_params: capture_params
-      }, expires_in.from_now, self.jwt_secret)
+      }
+      unless acting_as == user
+        claims[:acting_as_user_id] = acting_as.global_id.to_s
+      end
+      Canvas::Security.create_jwt(claims, expires_in.from_now, self.jwt_secret)
     end
 
     def direct_upload_jwt(host)

diff --git a/spec/lib/inst_fs_spec.rb b/spec/lib/inst_fs_spec.rb
@@ -81,6 +81,16 @@
       expect(claims[:user_id]).to eql(user.global_id.to_s)
     end
 
+    it "includes distinct global acting_as_user_id claim in the token if acting_as provided" do
+      user1 = user_model
+      user2 = user_model
+      url = InstFS.authenticated_url(@attachment, user: user1, acting_as: user2)
+      token = url.split(/token=/).last
+      claims = Canvas::Security.decode_jwt(token, [ @secret ])
+      expect(claims[:user_id]).to eql(user1.global_id.to_s)
+      expect(claims[:acting_as_user_id]).to eql(user2.global_id.to_s)
+    end
+
     it "includes omits user_id claim in the token if no user provided" do
       url = InstFS.authenticated_url(@attachment)
       token = url.split(/token=/).last
@@ -129,6 +139,7 @@
   context "upload_preflight_json" do
     let(:context) { instance_double("Course", id: 1, global_id: 101, root_account: Account.default) }
     let(:user) { instance_double("User", id: 2, global_id: 102) }
+    let(:acting_as) { instance_double("User", id: 4, global_id: 104) }
     let(:folder) { instance_double("Folder", id: 3, global_id: 103) }
     let(:filename) { 'test.txt' }
     let(:content_type) { 'text/plain' }
@@ -140,6 +151,7 @@
       InstFS.upload_preflight_json(
         context: context,
         user: user,
+        acting_as: acting_as,
         folder: folder,
         filename: filename,
         content_type: content_type,
@@ -175,6 +187,11 @@
         Canvas::Security.decode_jwt(token, [ @secret ])
       end
 
+      it "embeds the user_id and acting_as_user_id in the token" do
+        expect(jwt['user_id']).to eq user.global_id.to_s
+        expect(jwt['acting_as_user_id']).to eq acting_as.global_id.to_s
+      end
+
       it "embeds a capture_url in the token" do
         expect(jwt['capture_url']).to eq capture_url
       end
@@ -191,8 +208,8 @@
           expect(capture_params['context_id']).to eq context.global_id.to_s
         end
 
-        it "include the user" do
-          expect(capture_params['user_id']).to eq user.global_id.to_s
+        it "include the acting_as user" do
+          expect(capture_params['user_id']).to eq acting_as.global_id.to_s
         end
 
         it "include the folder" do