involve user in generating non-public links

fixes RECNVS-12 and make public links explicit. note that for non-inst-fs file storage, the user parameter to the existing authenticated_url method is unused. so for non-inst-fs, the following sets of methods are equivalent: * authenticated_url_for_user(*) == public_url == authenticated_url * download_url_for_user(*) == public_download_url (was download_url) * inline_url_for_user(*) == public_inline_url (was inline_url) the choice of `public_...` over `..._for_user` methods in the refactoring should thus be a no-op except when inst-fs is enabled. with inst-fs enabled, the `public_...` methods produce URLs usable by any user (including those not logged in!); this matches non-inst-fs behavior. the `..._for_user` methods produce URLs usable only by the user for whom they were generated, and should be preferred where public access is not necessary. after this refactor, make public links for google doc previews short lived and consolidate some code around google doc preview links. test-plan: - enable inst-fs [per-user inst-fs JWTs] - have a student and a teacher in a course with an assignment - as the teacher upload an image to the course files, then add the image to the course syllabus - as the student, attempt to view the course syllabus. should see the image in the course syllabus (prior to this commit would fail) - copy the inst-fs URL the student's browser was redirected to when viewing the file - as the teacher attempt to access the image directly using the student's inst-fs URL; should fail [public inst-fs JWTs] - as the teacher, upload a course card image (example of public file) - as the teacher, view the course card image and copy the inst-fs URL redirected to - as the student, attempt to access the course card image directly using the copied inst-fs URL; should succeed [google docs preview] - disable canvadocs on the account if enabled - as the teacher, upload a PDF to the course files - find the PDF in the course files and preview it - preview should be displayed via embedded google docs iframe - preview should succeed Change-Id: I8384cbb89f1522022e2f06579e6381de5ed0076c Reviewed-on: https://gerrit.instructure.com/133889 Tested-by: Jenkins Reviewed-by: Andrew Huff <[email protected]> QA-Review: Collin Parrish <[email protected]> Product-Review: Jacob Fugal <[email protected]>
ralacas · Jan 9, 2018 · b29eb06 · b29eb06
1 parent 15a850a
commit b29eb06
Show file tree

Hide file tree

Showing 26 changed files with 155 additions and 86 deletions.
diff --git a/app/controllers/brand_configs_controller.rb b/app/controllers/brand_configs_controller.rb
@@ -226,7 +226,7 @@ def upload_file(file)
     if Attachment.s3_storage?
       attachment.s3_url
     else
-      attachment.authenticated_url
+      attachment.public_url
     end
   end
 end
diff --git a/app/controllers/eportfolios_controller.rb b/app/controllers/eportfolios_controller.rb
@@ -175,8 +175,8 @@ def export
         respond_to do |format|
           if @attachment.zipped?
             if Attachment.s3_storage?
-              format.html { redirect_to @attachment.inline_url }
-              format.zip { redirect_to @attachment.inline_url }
+              format.html { redirect_to @attachment.inline_url_for_user(@current_user) }
+              format.zip { redirect_to @attachment.inline_url_for_user(@current_user) }
             else
               cancel_cache_buster
               format.html { send_file(@attachment.full_filename, :type => @attachment.content_type_with_encoding, :disposition => 'inline') }

diff --git a/app/controllers/file_previews_controller.rb b/app/controllers/file_previews_controller.rb
@@ -20,32 +20,6 @@ class FilePreviewsController < ApplicationController
 
   before_action :get_context
 
-  GOOGLE_PREVIEWABLE_TYPES = %w{
-   application/vnd.openxmlformats-officedocument.wordprocessingml.template
-   application/vnd.oasis.opendocument.spreadsheet
-   application/vnd.sun.xml.writer
-   application/excel
-   application/vnd.openxmlformats-officedocument.spreadsheetml.sheet
-   text/rtf
-   text/plain
-   application/vnd.openxmlformats-officedocument.spreadsheetml.template
-   application/vnd.sun.xml.impress
-   application/vnd.sun.xml.calc
-   application/vnd.ms-excel
-   application/msword
-   application/mspowerpoint
-   application/rtf
-   application/vnd.oasis.opendocument.presentation
-   application/vnd.oasis.opendocument.text
-   application/vnd.openxmlformats-officedocument.presentationml.template
-   application/vnd.openxmlformats-officedocument.presentationml.slideshow
-   application/vnd.openxmlformats-officedocument.presentationml.presentation
-   application/vnd.openxmlformats-officedocument.wordprocessingml.document
-   application/postscript
-   application/pdf
-   application/vnd.ms-powerpoint
-  }.freeze
-
   # renders (or redirects to) appropriate content for the file, such as
   # canvadocs, crocodoc, inline image, etc.
   def show
@@ -74,8 +48,9 @@ def show
       elsif url = @file.canvadoc_url(@current_user)
         redirect_to url and return
       # google docs
-      elsif service_enabled?(:google_docs_previews) && GOOGLE_PREVIEWABLE_TYPES.include?(@file.content_type)
-        redirect_to('//docs.google.com/viewer?' + { embedded: true, url: @file.authenticated_url }.to_query) and return
+      elsif GoogleDocsPreview.previewable?(@domain_root_account, @file)
+        url = GoogleDocsPreview.url_for(@file)
+        redirect_to('//docs.google.com/viewer?' + { embedded: true, url: url }.to_query) and return
       # images
       elsif @file.content_type =~ %r{\Aimage/}
         return render template: 'file_previews/img_preview', layout: false

diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb
@@ -398,7 +398,7 @@ def public_url
     # verify that the requested attachment belongs to the submission
     return render_unauthorized_action if @submission && !@submission.includes_attachment?(@attachment)
     if @submission ? authorized_action(@submission, @current_user, :read) : authorized_action(@attachment, @current_user, :download)
-      render :json  => { :public_url => @attachment.authenticated_url(:secure => request.ssl?) }
+      render :json  => { :public_url => @attachment.public_url(:secure => request.ssl?) }
     end
   end
 
@@ -537,11 +537,11 @@ def render_attachment(attachment)
           # some form.
           if @current_user &&
              (attachment.canvadocable? ||
-              (service_enabled?(:google_docs_previews) && attachment.authenticated_url))
+              GoogleDocsPreview.previewable?(@domain_root_account, attachment))
             attachment.context_module_action(@current_user, :read)
           end
-          if url = service_enabled?(:google_docs_previews) && attachment.authenticated_url
-            json[:attachment][:authenticated_url] = url
+          if GoogleDocsPreview.previewable?(@domain_root_account, attachment)
+            json[:attachment][:public_url] = GoogleDocsPreview.url_for(attachment)
           end
 
           json_include = if @attachment.context.is_a?(User) || @attachment.context.is_a?(Course)

diff --git a/app/controllers/gradebooks_controller.rb b/app/controllers/gradebooks_controller.rb
@@ -303,6 +303,7 @@ def old_gradebook_env
                    Setting.get('gradebook2.many_submissions_chunk_size', '10').to_i
                  end
     js_env STUDENT_CONTEXT_CARDS_ENABLED: @domain_root_account.feature_enabled?(:student_context_cards)
+    last_exported_attachment = @last_exported_gradebook_csv.try(:attachment)
     {
       GRADEBOOK_OPTIONS: {
         api_max_per_page: per_page,
@@ -380,8 +381,8 @@ 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_gradebook_csv.try(:attachment).try(:download_url),
-        attachment: @last_exported_gradebook_csv.try(:attachment),
+        attachment_url: last_exported_attachment && last_exported_attachment.download_url_for_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),
         list_students_by_sortable_name_enabled: @context.list_students_by_sortable_name?,

diff --git a/app/controllers/quizzes/quiz_submissions_controller.rb b/app/controllers/quizzes/quiz_submissions_controller.rb
@@ -200,8 +200,8 @@ def generate_submission_zip(quiz, context)
     respond_to do |format|
       if attachment.zipped?
         if Attachment.s3_storage?
-          format.html { redirect_to attachment.inline_url }
-          format.zip { redirect_to attachment.inline_url }
+          format.html { redirect_to attachment.inline_url_for_user(@current_user) }
+          format.zip { redirect_to attachment.inline_url_for_user(@current_user) }
         else
           cancel_cache_buster
 

diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb
@@ -667,8 +667,8 @@ def generate_submission_zip(assignment, context)
     respond_to do |format|
       if attachment.zipped?
         if Attachment.s3_storage?
-          format.html { redirect_to attachment.inline_url }
-          format.zip { redirect_to attachment.inline_url }
+          format.html { redirect_to attachment.inline_url_for_user(@current_user) }
+          format.zip { redirect_to attachment.inline_url_for_user(@current_user) }
         else
           cancel_cache_buster
 

diff --git a/app/helpers/attachment_helper.rb b/app/helpers/attachment_helper.rb
@@ -71,8 +71,10 @@ def render_or_redirect_to_stored_file(attachment:, verifier: nil, inline: false)
     elsif inline && attachment.can_be_proxied?
       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)
     else
-      redirect_to(inline ? attachment.inline_url : attachment.download_url)
+      redirect_to attachment.download_url_for_user(@current_user)
     end
   end
 

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
@@ -779,6 +779,37 @@ def authenticated_url(**options)
     end
   end
 
+  def authenticated_url_for_user(user, **options)
+    authenticated_url(options.merge(user: user))
+  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)
+  end
+
+  def public_url(**options)
+    authenticated_url_for_user(nil, options)
+  end
+
+  def public_inline_url(ttl = url_ttl)
+    inline_url_for_user(nil, ttl)
+  end
+
+  def public_download_url(ttl = url_ttl)
+    download_url_for_user(nil, ttl)
+  end
+
+  def url_ttl
+    settings = root_account_id && Account.find_cached(root_account_id)&.settings
+    setting = settings && settings[:s3_url_ttl_seconds]
+    setting ||= Setting.get('attachment_url_ttl', 1.hour.to_s)
+    setting.to_i.seconds
+  end
+
   def stored_locally?
     # if the file exists in inst-fs, it won't be in local storage even if
     # that's what Canvas otherwise thinks it's configured for
@@ -967,19 +998,6 @@ def readable_size
     h.number_to_human_size(self.size) rescue "size unknown"
   end
 
-  def download_url(ttl = url_ttl)
-    authenticated_url(expires_in: ttl, download: true)
-  end
-
-  def inline_url(ttl = url_ttl)
-    authenticated_url(expires_in: ttl, download: false)
-  end
-
-  def url_ttl
-    default = Setting.get('attachment_url_ttl', 1.hour.to_s).to_i.seconds
-    (root_account_id && Account.find_cached(root_account_id)&.settings[:s3_url_ttl_seconds]&.to_i&.seconds) || default
-  end
-
   def disposition_filename
     ascii_filename = Iconv.conv("ASCII//TRANSLIT//IGNORE", "UTF-8", display_name)
 

diff --git a/app/models/canvadoc.rb b/app/models/canvadoc.rb
@@ -27,7 +27,7 @@ class Canvadoc < ActiveRecord::Base
   def upload(opts = {})
     return if document_id.present?
 
-    url = attachment.authenticated_url(expires_in: 7.days)
+    url = attachment.public_url(expires_in: 7.days)
 
     opts.delete(:annotatable) unless Canvadocs.annotations_supported?
 

diff --git a/app/models/content_export.rb b/app/models/content_export.rb
@@ -213,7 +213,7 @@ def export_quizzes2
           export_type: QUIZZES2
         )
         self.settings[:quizzes2][:qti_export] = {}
-        self.settings[:quizzes2][:qti_export][:url] = self.attachment.download_url
+        self.settings[:quizzes2][:qti_export][:url] = self.attachment.public_download_url
         self.progress = 100
         mark_exported
       end

diff --git a/app/models/course.rb b/app/models/course.rb
@@ -417,7 +417,7 @@ def valid_course_image_url?(image_url)
   def image
     if self.image_id.present?
       self.shard.activate do
-        self.attachments.active.where(id: self.image_id).first.download_url rescue nil
+        self.attachments.active.where(id: self.image_id).take&.public_download_url
       end
     elsif self.image_url
       self.image_url

diff --git a/app/models/crocodoc_document.rb b/app/models/crocodoc_document.rb
@@ -39,7 +39,7 @@ class CrocodocDocument < ActiveRecord::Base
   def upload
     return if uuid.present?
 
-    url = attachment.authenticated_url(expires_in: 1.day)
+    url = attachment.public_url(expires_in: 1.day)
 
     begin
       response = Canvas.timeout_protection("crocodoc_upload", raise_on_timeout: true) do

diff --git a/app/models/epub_export.rb b/app/models/epub_export.rb
@@ -72,7 +72,9 @@ def update_progress_from_content_export!(val)
     create_job_progress(completion: 0, tag: self.class.to_s.underscore)
   end
 
-  delegate :download_url, to: :attachment, allow_nil: true
+  delegate :public_download_url, to: :attachment, allow_nil: true
+  delegate :download_url_for_user, to: :attachment, allow_nil: true
+  delegate :downloadable?, to: :attachment, allow_nil: true
   delegate :completion, :running?, to: :job_progress, allow_nil: true
 
   scope :running, -> { where(workflow_state: ['created', 'exporting', 'exported', 'generating']) }

diff --git a/app/models/kaltura_media_file_handler.rb b/app/models/kaltura_media_file_handler.rb
@@ -27,7 +27,7 @@ def add_media_files(attachments, wait_for_completion)
     attachments.select{|a| !a.media_object }.each do |attachment|
       files << {
                   :name       => attachment.display_name,
-                  :url        => attachment.download_url,
+                  :url        => attachment.public_download_url,
                   :media_type => (attachment.content_type || "").match(/\Avideo/) ? 'video' : 'audio',
                   :partner_data  => build_partner_data(attachment)
                }

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 %>" class="open_in_a_new_tab">
+              <li><a href="<%= @last_exported_gradebook_csv.attachment.download_url_for_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/lib/canvadocs/session.rb b/lib/canvadocs/session.rb
@@ -7,7 +7,7 @@ def canvadocs_session_url(opts = {})
       enable_annotations = opts.delete(:enable_annotations)
       moderated_grading_whitelist = opts.delete(:moderated_grading_whitelist)
       opts.merge! canvadoc_permissions_for_user(user, enable_annotations, moderated_grading_whitelist)
-      opts[:url] = attachment.authenticated_url(expires_in: 7.days)
+      opts[:url] = attachment.public_url(expires_in: 7.days)
       opts[:locale] = I18n.locale || I18n.default_locale
 
       Canvas.timeout_protection("canvadocs", raise_on_timeout: true) do

diff --git a/lib/google_docs_preview.rb b/lib/google_docs_preview.rb
@@ -0,0 +1,56 @@
+#
+# Copyright (C) 2017 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+module GoogleDocsPreview
+  PREVIEWABLE_TYPES = %w{
+    application/vnd.openxmlformats-officedocument.wordprocessingml.template
+    application/vnd.oasis.opendocument.spreadsheet
+    application/vnd.sun.xml.writer
+    application/excel
+    application/vnd.openxmlformats-officedocument.spreadsheetml.sheet
+    text/rtf
+    text/plain
+    application/vnd.openxmlformats-officedocument.spreadsheetml.template
+    application/vnd.sun.xml.impress
+    application/vnd.sun.xml.calc
+    application/vnd.ms-excel
+    application/msword
+    application/mspowerpoint
+    application/rtf
+    application/vnd.oasis.opendocument.presentation
+    application/vnd.oasis.opendocument.text
+    application/vnd.openxmlformats-officedocument.presentationml.template
+    application/vnd.openxmlformats-officedocument.presentationml.slideshow
+    application/vnd.openxmlformats-officedocument.presentationml.presentation
+    application/vnd.openxmlformats-officedocument.wordprocessingml.document
+    application/postscript
+    application/pdf
+    application/vnd.ms-powerpoint
+  }.freeze
+
+  def self.previewable?(account, attachment)
+    account&.service_enabled?(:google_docs_previews) &&
+    PREVIEWABLE_TYPES.include?(attachment.content_type) &&
+    attachment.downloadable?
+  end
+
+  def self.url_for(attachment)
+    expires_in = Setting.get('google_docs_previews.link_duration_minutes', '5').to_i.minutes
+    attachment.public_url(expires_in: expires_in)
+  end
+end
diff --git a/lib/inst_fs.rb b/lib/inst_fs.rb
@@ -105,7 +105,7 @@ def access_jwt(attachment, options={})
       expires_in = options[:expires_in] || expires_in
       Canvas::Security.create_jwt({
         iat: Time.now.utc.to_i,
-        user_id: attachment.global_user_id.to_s,
+        user_id: options[:user]&.global_id&.to_s,
         resource: attachment.instfs_uuid
       }, expires_in.from_now, self.jwt_secret)
     end

diff --git a/public/javascripts/instructure.js b/public/javascripts/instructure.js
@@ -356,7 +356,7 @@ function handleYoutubeLink () {
               .loadDocPreview({
                 canvadoc_session_url: attachment.canvadoc_session_url,
                 mimeType: attachment.content_type,
-                public_url: attachment.authenticated_url,
+                public_url: attachment.public_url,
                 attachment_preview_processing: attachment.workflow_state == 'pending_upload' || attachment.workflow_state == 'processing'
               })
               .prepend(