FIX: Ensure show_short URLs handle secure uploads using multisite (#9212)

Meta report: https://meta.discourse.org/t/short-url-secure-uploads-s3/144224
* if the show_short route is hit for an upload that is
  secure, we redirect to the secure presigned URL. however
  this was not taking into account multisite so the db name
  was left off the path which broke the presigned URL
* we now use the correct url_for method if we know the
  upload (like in the show_short case) which takes into
  account multisite
This commit is contained in:
Martin Brennan 2020-03-16 11:54:14 +10:00 committed by GitHub
parent d4595fbf29
commit a6e9057609
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 22 additions and 4 deletions

View File

@ -102,7 +102,9 @@ class UploadsController < ApplicationController
sha1 = Upload.sha1_from_base62_encoded(params[:base62]) sha1 = Upload.sha1_from_base62_encoded(params[:base62])
if upload = Upload.find_by(sha1: sha1) if upload = Upload.find_by(sha1: sha1)
return handle_secure_upload_request(upload, Discourse.store.get_path_for_upload(upload)) if upload.secure? && SiteSetting.secure_media? if upload.secure? && SiteSetting.secure_media?
return handle_secure_upload_request(upload)
end
if Discourse.store.internal? if Discourse.store.internal?
send_file_local_upload(upload) send_file_local_upload(upload)
@ -128,7 +130,6 @@ class UploadsController < ApplicationController
upload = Upload.find_by(sha1: sha1) upload = Upload.find_by(sha1: sha1)
return render_404 if upload.blank? return render_404 if upload.blank?
signed_secure_url = Discourse.store.signed_url_for_path(path_with_ext)
return handle_secure_upload_request(upload, path_with_ext) if SiteSetting.secure_media? return handle_secure_upload_request(upload, path_with_ext) if SiteSetting.secure_media?
# we don't want to 404 here if secure media gets disabled # we don't want to 404 here if secure media gets disabled
@ -138,14 +139,21 @@ class UploadsController < ApplicationController
# if the upload is still secure, that means the ACL is probably still # if the upload is still secure, that means the ACL is probably still
# private, so we don't want to go to the CDN url just yet otherwise we # private, so we don't want to go to the CDN url just yet otherwise we
# will get a 403. if the upload is not secure we assume the ACL is public # will get a 403. if the upload is not secure we assume the ACL is public
signed_secure_url = Discourse.store.signed_url_for_path(path_with_ext)
redirect_to upload.secure? ? signed_secure_url : Discourse.store.cdn_url(upload.url) redirect_to upload.secure? ? signed_secure_url : Discourse.store.cdn_url(upload.url)
end end
def handle_secure_upload_request(upload, path_with_ext) def handle_secure_upload_request(upload, path_with_ext = nil)
if upload.access_control_post_id.present? if upload.access_control_post_id.present?
raise Discourse::InvalidAccess if !guardian.can_see?(upload.access_control_post) raise Discourse::InvalidAccess if !guardian.can_see?(upload.access_control_post)
end end
# url_for figures out the full URL, handling multisite DBs,
# and will return a presigned URL for the upload
if path_with_ext.blank?
return redirect_to Discourse.store.url_for(upload)
end
redirect_to Discourse.store.signed_url_for_path(path_with_ext) redirect_to Discourse.store.signed_url_for_path(path_with_ext)
end end

View File

@ -398,9 +398,19 @@ describe UploadsController do
upload.update(access_control_post: post) upload.update(access_control_post: post)
get upload.short_path get upload.short_path
expect(response.code).to eq("403") expect(response.code).to eq("403")
end end
context "when running on a multisite connection" do
before do
Rails.configuration.multisite = true
end
it "redirects to the signed_url_for_path with the multisite DB name in the url" do
freeze_time
get upload.short_path
expect(response.body).to include(RailsMultisite::ConnectionManagement.current_db)
end
end
end end
end end
end end