From b29eb063833b13efc46b8e7fd34286a34815f608 Mon Sep 17 00:00:00 2001 From: Jacob Fugal Date: Mon, 27 Nov 2017 14:10:55 -0700 Subject: [PATCH] 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 QA-Review: Collin Parrish Product-Review: Jacob Fugal --- app/controllers/brand_configs_controller.rb | 2 +- app/controllers/eportfolios_controller.rb | 4 +- app/controllers/file_previews_controller.rb | 31 +--------- app/controllers/files_controller.rb | 8 +-- app/controllers/gradebooks_controller.rb | 5 +- .../quizzes/quiz_submissions_controller.rb | 4 +- app/controllers/submissions_controller.rb | 4 +- app/helpers/attachment_helper.rb | 4 +- app/models/attachment.rb | 44 ++++++++++----- app/models/canvadoc.rb | 2 +- app/models/content_export.rb | 2 +- app/models/course.rb | 2 +- app/models/crocodoc_document.rb | 2 +- app/models/epub_export.rb | 4 +- app/models/kaltura_media_file_handler.rb | 2 +- app/views/gradebooks/gradebook.html.erb | 2 +- lib/canvadocs/session.rb | 2 +- lib/google_docs_preview.rb | 56 +++++++++++++++++++ lib/inst_fs.rb | 2 +- public/javascripts/instructure.js | 2 +- spec/controllers/files_controller_spec.rb | 12 ++-- spec/lib/inst_fs_spec.rb | 15 +++++ spec/models/attachment_spec.rb | 22 ++++---- spec/models/canvadoc_spec.rb | 2 +- spec/models/content_export_spec.rb | 2 +- spec/models/course_spec.rb | 4 +- 26 files changed, 155 insertions(+), 86 deletions(-) create mode 100644 lib/google_docs_preview.rb diff --git a/app/controllers/brand_configs_controller.rb b/app/controllers/brand_configs_controller.rb index 43ba9b8bb6eca..8385a48fc4b36 100644 --- 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 index ccca33e96f84c..9e015086536ad 100644 --- 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 index 5c4d87db92785..a980e49b530e4 100644 --- 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 index 3d966cddafb3f..02dfc78a24eb7 100644 --- 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 index 45580b0dfd8b2..df0ae04fad9b0 100644 --- 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 index f4a78d345aad4..2ba62ae2d1eab 100644 --- 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 index 83f292071123b..d25aade1f1c7d 100644 --- 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 index 49261160fa736..bd531e6d5ffe7 100644 --- 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 index 11978a5e14220..2651258fd2ba4 100644 --- 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 index 4084982f3e6f0..a0a852bd0a0dc 100644 --- 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 index f59ae1fa0bacd..60412d66c7c38 100644 --- 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 index bc55b665c74be..48e74eb682658 100644 --- 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 index 8ac0997549f44..66b3d9344731c 100644 --- 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 index 06531ec5ea30d..fac17dfaba920 100644 --- 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 index 96fe8de9473be..ba4c6589d24fe 100644 --- 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 index 837f62162c6da..e1f440447804c 100644 --- a/app/views/gradebooks/gradebook.html.erb +++ b/app/views/gradebooks/gradebook.html.erb @@ -115,7 +115,7 @@