From 48b4ed41f5b70e7f068931c071d1d4aae72a4d64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 3 Jul 2020 19:16:54 +0200 Subject: [PATCH] FIX: uploading an existing image as a site setting The previous fix (f43c0a5d857d34) wasn't working for images that were already uploaded. The "metadata" (eg. 'for_*' and 'secure' attributes) were not added to existing uploads. Also used 'Upload.get_from_url' is the admin/site_setting controller to properly retrieve an upload from its URL. Fixed the Upload::URL_REGEX to use the \h (hexadecimal) for the SHA Follow-up-to: f43c0a5d857d34 --- .../admin/site_settings_controller.rb | 2 +- app/models/upload.rb | 2 +- lib/upload_creator.rb | 21 +++++++++++-------- spec/requests/uploads_controller_spec.rb | 19 +++++++---------- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index c143e04dca7..e56c7faf019 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -17,7 +17,7 @@ class Admin::SiteSettingsController < Admin::AdminController raise_access_hidden_setting(id) if SiteSetting.type_supervisor.get_type(id) == :upload - value = Upload.find_by(url: value) || '' + value = Upload.get_from_url(value) || "" end update_existing_users = params[:updateExistingUsers].present? diff --git a/app/models/upload.rb b/app/models/upload.rb index 77ecd05b5cb..25b589c376b 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -8,7 +8,7 @@ class Upload < ActiveRecord::Base SHA1_LENGTH = 40 SEEDED_ID_THRESHOLD = 0 - URL_REGEX ||= /(\/original\/\dX[\/\.\w]*\/([a-zA-Z0-9]+)[\.\w]*)/ + URL_REGEX ||= /(\/original\/\dX[\/\.\w]*\/(\h+)[\.\w]*)/ SECURE_MEDIA_ROUTE = "secure-media-uploads" belongs_to :user diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index caaacb0a748..797328f19c6 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -73,7 +73,6 @@ class UploadCreator # between uploads instead of the sha1, and to get around various # access/permission issues for uploads if !SiteSetting.secure_media - # do we already have that upload? @upload = Upload.find_by(sha1: sha1) @@ -85,6 +84,7 @@ class UploadCreator # return the previous upload if any if @upload + add_metadata! UserUpload.find_or_create_by!(user_id: user_id, upload_id: @upload.id) if user_id return @upload end @@ -121,14 +121,7 @@ class UploadCreator @upload.width, @upload.height = @image_info.size end - @upload.for_private_message = true if @opts[:for_private_message] - @upload.for_group_message = true if @opts[:for_group_message] - @upload.for_theme = true if @opts[:for_theme] - @upload.for_export = true if @opts[:for_export] - @upload.for_site_setting = true if @opts[:for_site_setting] - @upload.for_gravatar = true if @opts[:for_gravatar] - @upload.secure = UploadSecurity.new(@upload, @opts).should_be_secure? - + add_metadata! return @upload unless @upload.save # store the file and update its url @@ -375,4 +368,14 @@ class UploadCreator @@svg_whitelist_xpath ||= "//*[#{WHITELISTED_SVG_ELEMENTS.map { |e| "name()!='#{e}'" }.join(" and ") }]" end + def add_metadata! + @upload.for_private_message = true if @opts[:for_private_message] + @upload.for_group_message = true if @opts[:for_group_message] + @upload.for_theme = true if @opts[:for_theme] + @upload.for_export = true if @opts[:for_export] + @upload.for_site_setting = true if @opts[:for_site_setting] + @upload.for_gravatar = true if @opts[:for_gravatar] + @upload.secure = UploadSecurity.new(@upload, @opts).should_be_secure? + end + end diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb index 69687b6c0f7..4c3f7dcb07e 100644 --- a/spec/requests/uploads_controller_spec.rb +++ b/spec/requests/uploads_controller_spec.rb @@ -17,18 +17,11 @@ describe UploadsController do end let(:logo_file) { file_from_fixtures("logo.png") } - let(:logo) do - Rack::Test::UploadedFile.new(logo_file) - end let(:logo_filename) { File.basename(logo_file) } - let(:fake_jpg) do - Rack::Test::UploadedFile.new(file_from_fixtures("fake.jpg")) - end - - let(:text_file) do - Rack::Test::UploadedFile.new(File.new("#{Rails.root}/LICENSE.txt")) - end + let(:logo) { Rack::Test::UploadedFile.new(logo_file) } + let(:fake_jpg) { Rack::Test::UploadedFile.new(file_from_fixtures("fake.jpg")) } + let(:text_file) { Rack::Test::UploadedFile.new(File.new("#{Rails.root}/LICENSE.txt")) } it 'expects a type' do post "/uploads.json", params: { file: logo } @@ -44,9 +37,13 @@ describe UploadsController do it 'returns "raw" url for site settings' do set_cdn_url "https://awesome.com" + + upload = UploadCreator.new(logo_file, "logo.png").create_for(-1) + logo = Rack::Test::UploadedFile.new(file_from_fixtures("logo.png")) + post "/uploads.json", params: { file: logo, type: "site_setting", for_site_setting: "true" } expect(response.status).to eq 200 - expect(response.parsed_body["url"]).to start_with("/uploads/default/") + expect(response.parsed_body["url"]).to eq(upload.url) end it 'returns cdn url' do