REFACTOR: upload workflow creation into UploadCreator

- Automatically convert large-ish PNG/BMP to JPEG
- Updated fast_image to latest version

Gemfile

@@ -61,8 +61,7 @@ gem 'fast_xs'
 
 gem 'fast_xor'
 
-# while we sort out https://github.com/sdsykes/fastimage/pull/46
+gem 'fastimage', '2.1.0'
-gem 'discourse_fastimage', '2.0.3', require: 'fastimage'
 gem 'aws-sdk', require: false
 gem 'excon', require: false
 gem 'unf', require: false

Gemfile.lock

@@ -78,7 +78,6 @@ GEM
     diff-lcs (1.3)
     discourse-qunit-rails (0.0.9)
       railties
-    discourse_fastimage (2.0.3)
     discourse_image_optim (0.24.4)
       exifr (~> 1.2, >= 1.2.2)
       fspath (~> 3.0)
@@ -114,6 +113,7 @@ GEM
       rake
       rake-compiler
     fast_xs (0.8.0)
+    fastimage (2.1.0)
     ffi (1.9.18)
     flamegraph (0.9.5)
     foreman (0.82.0)
@@ -402,7 +402,6 @@ DEPENDENCIES
   byebug
   certified
   discourse-qunit-rails
-  discourse_fastimage (= 2.0.3)
   discourse_image_optim
   email_reply_trimmer (= 0.1.6)
   ember-handlebars-template (= 0.7.5)
@@ -415,6 +414,7 @@ DEPENDENCIES
   fast_blank
   fast_xor
   fast_xs
+  fastimage (= 2.1.0)
   flamegraph
   foreman
   gc_tracer

app/controllers/admin/emojis_controller.rb

@@ -1,3 +1,5 @@
+require_dependency 'upload_creator'
+
 class Admin::EmojisController < Admin::AdminController
 
   def index
@@ -14,13 +16,11 @@ class Admin::EmojisController < Admin::AdminController
                  .gsub(/_{2,}/, '_')
                  .downcase
 
-      upload = Upload.create_for(
+      upload = UploadCreator.new(
-        current_user.id,
         file.tempfile,
         file.original_filename,
-        File.size(file.tempfile.path),
+        type: 'custom_emoji'
-        image_type: 'custom_emoji'
+      ).create_for(current_user.id)
-      )
 
       data =
         if upload.persisted?

app/controllers/admin/themes_controller.rb

@@ -1,3 +1,5 @@
+require_dependency 'upload_creator'
+
 class Admin::ThemesController < Admin::AdminController
 
   skip_before_filter :check_xhr, only: [:show, :preview]
@@ -5,27 +7,23 @@ class Admin::ThemesController < Admin::AdminController
   def preview
     @theme = Theme.find(params[:id])
 
-    redirect_to path("/"), flash: {preview_theme_key: @theme.key}
+    redirect_to path("/"), flash: { preview_theme_key: @theme.key }
   end
 
   def upload_asset
     path = params[:file].path
     File.open(path) do |file|
-      upload = Upload.create_for(current_user.id,
+      filename = params[:file]&.original_filename || File.basename(path)
-                                 file,
+      upload = UploadCreator.new(file, filename, for_theme: true).create_for(current_user.id)
-                                 params[:file]&.original_filename || File.basename(path),
-                                 File.size(path),
-                                 for_theme: true)
       if upload.errors.count > 0
         render json: upload.errors, status: :unprocessable_entity
       else
-        render json: {upload_id: upload.id}, status: :created
+        render json: { upload_id: upload.id }, status: :created
       end
     end
   end
 
   def import
-
     @theme = nil
     if params[:theme]
       json = JSON::parse(params[:theme].read)
@@ -48,7 +46,6 @@ class Admin::ThemesController < Admin::AdminController
     else
       render json: @theme.errors, status: :unprocessable_entity
     end
-
   end
 
   def index
@@ -206,7 +203,6 @@ class Admin::ThemesController < Admin::AdminController
     end
 
     def set_fields
-
       return unless fields = theme_params[:theme_fields]
 
       fields.each do |field|

app/controllers/uploads_controller.rb

@@ -1,3 +1,5 @@
+require_dependency 'upload_creator'
+
 class UploadsController < ApplicationController
   before_filter :ensure_logged_in, except: [:show]
   skip_before_filter :preload_json, :check_xhr, :redirect_to_login_if_required, only: [:show]
@@ -5,26 +7,25 @@ class UploadsController < ApplicationController
   def create
     type = params.require(:type)
 
-    raise Discourse::InvalidAccess.new unless type =~ /^[a-z\-\_]{1,100}$/
+    raise Discourse::InvalidAccess.new unless Upload::UPLOAD_TYPES.include?(type)
 
-    file = params[:file] || params[:files].try(:first)
+    if type == "avatar" && (SiteSetting.sso_overrides_avatar || !SiteSetting.allow_uploaded_avatars)
-    url = params[:url]
+      return render json: failed_json, status: 422
-    client_id = params[:client_id]
-    synchronous = (current_user.staff? || is_api?) && params[:synchronous]
-
-    if type == "avatar"
-      if SiteSetting.sso_overrides_avatar || !SiteSetting.allow_uploaded_avatars
-        return render json: failed_json, status: 422
-      end
     end
 
-    if synchronous
+    url  = params[:url]
-      data = create_upload(type, file, url)
+    file = params[:file] || params[:files]&.first
+
+    if params[:synchronous] && (current_user.staff? || is_api?)
+      data = create_upload(file, url, type)
       render json: data.as_json
     else
       Scheduler::Defer.later("Create Upload") do
-        data = create_upload(type, file, url)
+        begin
-        MessageBus.publish("/uploads/#{type}", data.as_json, client_ids: [client_id])
+          data = create_upload(file, url, type)
+        ensure
+          MessageBus.publish("/uploads/#{type}", (data || {}).as_json, client_ids: [params[:client_id]])
+        end
       end
       render json: success_json
     end
@@ -58,86 +59,31 @@ class UploadsController < ApplicationController
     render nothing: true, status: 404
   end
 
-  def create_upload(type, file, url)
+  def create_upload(file, url, type)
-    begin
+    if file.nil?
-      maximum_upload_size = [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes
+      if url.present? && is_api?
-
+        maximum_upload_size = [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes
-      # ensure we have a file
+        tempfile = FileHelper.download(url, maximum_upload_size, "discourse-upload-#{type}") rescue nil
-      if file.nil?
+        filename = File.basename(URI.parse(url).path)
-        # API can provide a URL
-        if url.present? && is_api?
-          tempfile = FileHelper.download(url, maximum_upload_size, "discourse-upload-#{type}") rescue nil
-          filename = File.basename(URI.parse(url).path)
-        end
-      else
-        tempfile = file.tempfile
-        filename = file.original_filename
-        content_type = file.content_type
       end
-
-      return { errors: I18n.t("upload.file_missing") } if tempfile.nil?
-
-      # convert pasted images to HQ jpegs
-      if filename == "image.png" && SiteSetting.convert_pasted_images_to_hq_jpg
-        jpeg_path = "#{File.dirname(tempfile.path)}/image.jpg"
-        OptimizedImage.ensure_safe_paths!(tempfile.path, jpeg_path)
-
-        Discourse::Utils.execute_command('convert', tempfile.path, '-quality', SiteSetting.convert_pasted_images_quality.to_s, jpeg_path)
-        # only change the format of the image when JPG is at least 5% smaller
-        if File.size(jpeg_path) < File.size(tempfile.path) * 0.95
-          filename = "image.jpg"
-          content_type = "image/jpeg"
-          tempfile = File.open(jpeg_path)
-        else
-          File.delete(jpeg_path) rescue nil
-        end
-      end
-
-      # allow users to upload large images that will be automatically reduced to allowed size
-      max_image_size_kb = SiteSetting.max_image_size_kb.kilobytes
-      if max_image_size_kb > 0 && FileHelper.is_image?(filename)
-        if File.size(tempfile.path) >= max_image_size_kb && Upload.should_optimize?(tempfile.path)
-          attempt = 2
-          allow_animation = type == "avatar" ? SiteSetting.allow_animated_avatars : SiteSetting.allow_animated_thumbnails
-          while attempt > 0
-            downsized_size = File.size(tempfile.path)
-            break if downsized_size < max_image_size_kb
-            image_info = FastImage.new(tempfile.path) rescue nil
-            w, h = *(image_info.try(:size) || [0, 0])
-            break if w == 0 || h == 0
-            downsize_ratio = best_downsize_ratio(downsized_size, max_image_size_kb)
-            dimensions = "#{(w * downsize_ratio).floor}x#{(h * downsize_ratio).floor}"
-            OptimizedImage.downsize(tempfile.path, tempfile.path, dimensions, filename: filename, allow_animation: allow_animation)
-            attempt -= 1
-          end
-        end
-      end
-
-      upload = Upload.create_for(current_user.id, tempfile, filename, File.size(tempfile.path), content_type: content_type, image_type: type)
-
-      if upload.errors.empty? && current_user.admin?
-        retain_hours = params[:retain_hours].to_i
-        upload.update_columns(retain_hours: retain_hours) if retain_hours > 0
-      end
-
-      if upload.errors.empty? && FileHelper.is_image?(filename)
-        Jobs.enqueue(:create_thumbnails, upload_id: upload.id, type: type, user_id: params[:user_id])
-      end
-
-      upload.errors.empty? ? upload : { errors: upload.errors.values.flatten }
-    ensure
-      tempfile.try(:close!) rescue nil
-    end
-  end
-
-  def best_downsize_ratio(downsized_size, max_image_size)
-    if downsized_size / 9 > max_image_size
-      0.3
-    elsif downsized_size / 3 > max_image_size
-      0.6
     else
-      0.8
+      tempfile = file.tempfile
+      filename = file.original_filename
+      content_type = file.content_type
     end
+
+    return { errors: [I18n.t("upload.file_missing")] } if tempfile.nil?
+
+    upload = UploadCreator.new(tempfile, filename, type: type, content_type: content_type).create_for(current_user.id)
+
+    if upload.errors.empty? && current_user.admin?
+      retain_hours = params[:retain_hours].to_i
+      upload.update_columns(retain_hours: retain_hours) if retain_hours > 0
+    end
+
+    upload.errors.empty? ? upload : { errors: upload.errors.values.flatten }
+  ensure
+    tempfile&.close! rescue nil
   end
 
 end

app/jobs/onceoff/migrate_custom_emojis.rb

@@ -1,3 +1,5 @@
+require_dependency 'upload_creator'
+
 module Jobs
   class MigrateCustomEmojis < Jobs::Onceoff
     def execute_onceoff(args)
@@ -7,13 +9,11 @@ module Jobs
         name = File.basename(path, File.extname(path))
 
         File.open(path) do |file|
-          upload = Upload.create_for(
+          upload = UploadCreator.new(
-            Discourse.system_user.id,
             file,
             File.basename(path),
-            file.size,
+            type: 'custom_emoji'
-            image_type: 'custom_emoji'
+          ).create_for(Discourse.system_user.id)
-          )
 
           if upload.persisted?
             custom_emoji = CustomEmoji.new(name: name, upload: upload)

app/jobs/regular/create_avatar_thumbnails.rb

@@ -0,0 +1,20 @@
+module Jobs
+
+  class CreateAvatarThumbnails < Jobs::Base
+
+    def execute(args)
+      upload_id = args[:upload_id]
+
+      raise Discourse::InvalidParameters.new(:upload_id) if upload_id.blank?
+
+      upload = Upload.find(upload_id)
+      user   = User.find(args[:user_id] || upload.user_id)
+
+      Discourse.avatar_sizes.each do |size|
+        OptimizedImage.create_for(upload, size, size, filename: upload.original_filename, allow_animation: SiteSetting.allow_animated_avatars)
+      end
+    end
+
+  end
+
+end

app/jobs/regular/create_thumbnails.rb

@@ -1,37 +0,0 @@
-module Jobs
-
-  class CreateThumbnails < Jobs::Base
-
-    def execute(args)
-      type = args[:type]
-      upload_id = args[:upload_id]
-
-      raise Discourse::InvalidParameters.new(:type) if type.blank?
-      raise Discourse::InvalidParameters.new(:upload_id) if upload_id.blank?
-
-      # only need to generate thumbnails for avatars
-      return if type != "avatar"
-
-      upload = Upload.find(upload_id)
-
-      user_id = args[:user_id] || upload.user_id
-      user = User.find(user_id)
-
-      self.send("create_thumbnails_for_#{type}", upload, user)
-    end
-
-    def create_thumbnails_for_avatar(upload, user)
-      Discourse.avatar_sizes.each do |size|
-        OptimizedImage.create_for(
-          upload,
-          size,
-          size,
-          filename: upload.original_filename,
-          allow_animation: SiteSetting.allow_animated_avatars
-        )
-      end
-    end
-
-  end
-
-end

app/jobs/regular/pull_hotlinked_images.rb

@@ -1,5 +1,6 @@
 require_dependency 'url_helper'
 require_dependency 'file_helper'
+require_dependency 'upload_creator'
 
 module Jobs
 
@@ -41,7 +42,7 @@ module Jobs
               if hotlinked
                 if File.size(hotlinked.path) <= @max_size
                   filename = File.basename(URI.parse(src).path)
-                  upload = Upload.create_for(post.user_id, hotlinked, filename, File.size(hotlinked.path), { origin: src })
+                  upload = UploadCreator.new(hotlinked, filename, origin: src).create_for(post.user_id)
                   downloaded_urls[src] = upload.url
                 else
                   Rails.logger.info("Failed to pull hotlinked image for post: #{post_id}: #{src} - Image is bigger than #{@max_size}")

app/models/optimized_image.rb

@@ -178,7 +178,7 @@ class OptimizedImage < ActiveRecord::Base
       #{from}[0]
       -gravity center
       -background transparent
-      -resize #{dimensions}#{!!opts[:force_aspect_ratio] ? "\\!" : "\\>"}
+      -resize #{dimensions}
       -profile #{File.join(Rails.root, 'vendor', 'data', 'RT_sRGB.icm')}
       #{to}
     }

app/models/remote_theme.rb

@@ -1,4 +1,5 @@
 require_dependency 'git_importer'
+require_dependency 'upload_creator'
 
 class RemoteTheme < ActiveRecord::Base
 
@@ -50,7 +51,7 @@ class RemoteTheme < ActiveRecord::Base
 
     theme_info["assets"]&.each do |name, relative_path|
       if path = importer.real_path(relative_path)
-        upload = Upload.create_for(theme.user_id, File.open(path), File.basename(relative_path), File.size(path), for_theme: true)
+        upload = UploadCreator.new(File.open(path), File.basename(relative_path), for_theme: true).create_for(theme.user_id)
         theme.set_field(target: :common, name: name, type: :theme_upload_var, upload_id: upload.id)
       end
     end

app/models/upload.rb

@@ -1,5 +1,4 @@
 require "digest/sha1"
-require_dependency "image_sizer"
 require_dependency "file_helper"
 require_dependency "url_helper"
 require_dependency "db_helper"
@@ -22,6 +21,9 @@ class Upload < ActiveRecord::Base
 
   validates_with ::Validators::UploadValidator
 
+  CROPPED_TYPES ||= %w{avatar card_background custom_emoji profile_background}
+  UPLOAD_TYPES  ||= CROPPED_TYPES + %w{composer}
+
   def thumbnail(width = self.width, height = self.height)
     optimized_images.find_by(width: width, height: height)
   end
@@ -57,198 +59,10 @@ class Upload < ActiveRecord::Base
     File.extname(original_filename)
   end
 
-  # list of image types that will be cropped
-  CROPPED_IMAGE_TYPES ||= %w{
-    avatar
-    profile_background
-    card_background
-    custom_emoji
-  }
-
-  WHITELISTED_SVG_ELEMENTS ||= %w{
-    circle
-    clippath
-    defs
-    ellipse
-    g
-    line
-    linearGradient
-    path
-    polygon
-    polyline
-    radialGradient
-    rect
-    stop
-    svg
-    text
-    textpath
-    tref
-    tspan
-    use
-  }
-
   def self.generate_digest(path)
     Digest::SHA1.file(path).hexdigest
   end
 
-  def self.svg_whitelist_xpath
-    @@svg_whitelist_xpath ||= "//*[#{WHITELISTED_SVG_ELEMENTS.map { |e| "name()!='#{e}'" }.join(" and ") }]"
-  end
-
-  # options
-  #   - content_type
-  #   - origin (url)
-  #   - image_type ("avatar", "profile_background", "card_background", "custom_emoji")
-  #   - is_attachment_for_group_message (boolean)
-  #   - for_theme (boolean)
-  def self.create_for(user_id, file, filename, filesize, options = {})
-    upload = Upload.new
-
-    DistributedMutex.synchronize("upload_#{user_id}_#{filename}") do
-      # do some work on images
-      if FileHelper.is_image?(filename) && is_actual_image?(file)
-        # retrieve image info
-        w, h = FastImage.size(file) || [0, 0]
-
-        if filename[/\.svg$/i]
-          # whitelist svg elements
-          doc = Nokogiri::XML(file)
-          doc.xpath(svg_whitelist_xpath).remove
-          File.write(file.path, doc.to_s)
-          file.rewind
-        else
-          if w * h >= SiteSetting.max_image_megapixels * 1_000_000
-            upload.errors.add(:base, I18n.t("upload.images.larger_than_x_megapixels", max_image_megapixels: SiteSetting.max_image_megapixels))
-            return upload
-          end
-
-          # fix orientation first
-          fix_image_orientation(file.path) if should_optimize?(file.path, [w, h])
-        end
-
-        # default size
-        width, height = ImageSizer.resize(w, h)
-
-        # make sure we're at the beginning of the file (both FastImage and Nokogiri move the pointer)
-        file.rewind
-
-        # crop images depending on their type
-        if CROPPED_IMAGE_TYPES.include?(options[:image_type])
-          allow_animation = SiteSetting.allow_animated_thumbnails
-          max_pixel_ratio = Discourse::PIXEL_RATIOS.max
-
-          case options[:image_type]
-          when "avatar"
-            allow_animation = SiteSetting.allow_animated_avatars
-            width = height = Discourse.avatar_sizes.max
-            OptimizedImage.resize(file.path, file.path, width, height, filename: filename, allow_animation: allow_animation)
-          when "profile_background"
-            max_width = 850 * max_pixel_ratio
-            width, height = ImageSizer.resize(w, h, max_width: max_width, max_height: max_width)
-            OptimizedImage.downsize(file.path, file.path, "#{width}x#{height}", filename: filename, allow_animation: allow_animation)
-          when "card_background"
-            max_width = 590 * max_pixel_ratio
-            width, height = ImageSizer.resize(w, h, max_width: max_width, max_height: max_width)
-            OptimizedImage.downsize(file.path, file.path, "#{width}x#{height}", filename: filename, allow_animation: allow_animation)
-          when "custom_emoji"
-            OptimizedImage.downsize(file.path, file.path, "100x100", filename: filename, allow_animation: allow_animation)
-          end
-        end
-
-        # optimize image (except GIFs, SVGs and large PNGs)
-        if should_optimize?(file.path, [w, h])
-          begin
-            ImageOptim.new.optimize_image!(file.path)
-          rescue ImageOptim::Worker::TimeoutExceeded
-            # Don't optimize if it takes too long
-            Rails.logger.warn("ImageOptim timed out while optimizing #{filename}")
-          end
-          # update the file size
-          filesize = File.size(file.path)
-        end
-      end
-
-      # compute the sha of the file
-      sha1 = Upload.generate_digest(file)
-
-      # do we already have that upload?
-      upload = find_by(sha1: sha1)
-
-      # make sure the previous upload has not failed
-      if upload && (upload.url.blank? || is_dimensionless_image?(filename, upload.width, upload.height))
-        upload.destroy
-        upload = nil
-      end
-
-      # return the previous upload if any
-      return upload unless upload.nil?
-
-      # create the upload otherwise
-      upload = Upload.new
-      upload.user_id           = user_id
-      upload.original_filename = filename
-      upload.filesize          = filesize
-      upload.sha1              = sha1
-      upload.url               = ""
-      upload.width             = width
-      upload.height            = height
-      upload.origin            = options[:origin][0...1000] if options[:origin]
-
-      if options[:is_attachment_for_group_message]
-        upload.is_attachment_for_group_message = true
-      end
-
-      if options[:for_theme]
-        upload.for_theme = true
-      end
-
-      if is_dimensionless_image?(filename, upload.width, upload.height)
-        upload.errors.add(:base, I18n.t("upload.images.size_not_found"))
-        return upload
-      end
-
-      return upload unless upload.save
-
-      # store the file and update its url
-      File.open(file.path) do |f|
-        url = Discourse.store.store_upload(f, upload, options[:content_type])
-        if url.present?
-          upload.url = url
-          upload.save
-        else
-          upload.errors.add(:url, I18n.t("upload.store_failure", { upload_id: upload.id, user_id: user_id }))
-        end
-      end
-
-      upload
-    end
-  end
-
-  def self.is_actual_image?(file)
-    # due to ImageMagick CVE-2016–3714, use FastImage to check the magic bytes
-    # cf. https://meta.discourse.org/t/imagemagick-cve-2016-3714/43624
-    FastImage.size(file, raise_on_failure: true)
-  rescue
-    false
-  end
-
-  LARGE_PNG_SIZE ||= 3.megabytes
-
-  def self.should_optimize?(path, dimensions = nil)
-    # don't optimize GIFs or SVGs
-    return false if path =~ /\.(gif|svg)$/i
-    return true  if path !~ /\.png$/i
-
-    dimensions ||= (FastImage.size(path) || [0, 0])
-    w, h = dimensions
-    # don't optimize large PNGs
-    w > 0 && h > 0 && w * h < LARGE_PNG_SIZE
-  end
-
-  def self.is_dimensionless_image?(filename, width, height)
-    FileHelper.is_image?(filename) && (width.blank? || width == 0 || height.blank? || height == 0)
-  end
-
   def self.get_from_url(url)
     return if url.blank?
     # we store relative urls, so we need to remove any host/cdn
@@ -263,10 +77,6 @@ class Upload < ActiveRecord::Base
     Upload.find_by(url: url)
   end
 
-  def self.fix_image_orientation(path)
-    Discourse::Utils.execute_command('convert', path, '-auto-orient', path)
-  end
-
   def self.migrate_to_new_scheme(limit=nil)
     problems = []
 

app/models/user_avatar.rb

@@ -1,4 +1,5 @@
 require_dependency 'letter_avatar'
+require_dependency 'upload_creator'
 
 class UserAvatar < ActiveRecord::Base
   belongs_to :user
@@ -20,7 +21,7 @@ class UserAvatar < ActiveRecord::Base
         max = Discourse.avatar_sizes.max
         gravatar_url = "http://www.gravatar.com/avatar/#{email_hash}.png?s=#{max}&d=404"
         tempfile = FileHelper.download(gravatar_url, SiteSetting.max_image_size_kb.kilobytes, "gravatar")
-        upload = Upload.create_for(user_id, tempfile, 'gravatar.png', File.size(tempfile.path), origin: gravatar_url, image_type: "avatar")
+        upload = UploadCreator.new(tempfile, 'gravatar.png', origin: gravatar_url, type: "avatar").create_for(user_id)
 
         if gravatar_upload_id != upload.id
           gravatar_upload.try(:destroy!) rescue nil
@@ -65,7 +66,7 @@ class UserAvatar < ActiveRecord::Base
     ext = FastImage.type(tempfile).to_s
     tempfile.rewind
 
-    upload = Upload.create_for(user.id, tempfile, "external-avatar." + ext, File.size(tempfile.path), origin: avatar_url, image_type: "avatar")
+    upload = UploadCreator.new(tempfile, "external-avatar." + ext, origin: avatar_url, type: "avatar").create_for(user.id)
 
     user.create_user_avatar unless user.user_avatar
 

config/locales/server.en.yml

@@ -1179,8 +1179,7 @@ en:
 
     allow_all_attachments_for_group_messages: "Allow all email attachments for group messages."
 
-    convert_pasted_images_to_hq_jpg: "Convert pasted images to high-quality JPG files."
+    png_to_jpg_quality: "Quality of the converted JPG file (1 is lowest quality, 99 is best quality, 100 to disable)."
-    convert_pasted_images_quality: "Quality of the converted JPG file (1 is lowest quality, 100 is best quality)."
 
     enable_flash_video_onebox: "Enable embedding of swf and flv (Adobe Flash) links in oneboxes. WARNING: may introduce security risks."
 
@@ -2721,12 +2720,15 @@ en:
 
   deleted: 'deleted'
 
+  image: "image"
+
   upload:
     edit_reason: "downloaded local copies of images"
     unauthorized: "Sorry, the file you are trying to upload is not authorized (authorized extensions: %{authorized_extensions})."
     pasted_image_filename: "Pasted image"
     store_failure: "Failed to store upload #%{upload_id} for user #%{user_id}."
     file_missing: "Sorry, you must provide a file to upload."
+    empty: "Sorry, but the file you provided is empty."
     attachments:
       too_large: "Sorry, the file you are trying to upload is too big (maximum size is %{max_size_kb}KB)."
     images:

config/site_settings.yml

@@ -794,8 +794,7 @@ files:
     regex: '^((https?:)?\/)?\/.+[^\/]'
     shadowed_by_global: true
   allow_all_attachments_for_group_messages: false
-  convert_pasted_images_to_hq_jpg: true
+  png_to_jpg_quality:
-  convert_pasted_images_quality:
     default: 95
     min: 1
     max: 100

lib/email/receiver.rb

@@ -2,6 +2,7 @@ require "digest"
 require_dependency "new_post_manager"
 require_dependency "post_action_creator"
 require_dependency "html_to_markdown"
+require_dependency "upload_creator"
 
 module Email
 
@@ -570,7 +571,7 @@ module Email
           File.open(tmp.path, "w+b") { |f| f.write attachment.body.decoded }
           # create the upload for the user
           opts = { is_attachment_for_group_message: options[:is_group_message] }
-          upload = Upload.create_for(options[:user].id, tmp, attachment.filename, tmp.size, opts)
+          upload = UploadCreator.new(tmp, attachment.filename, opts).create_for(options[:user].id)
           if upload && upload.errors.empty?
             # try to inline images
             if attachment.content_type.start_with?("image/")

lib/tasks/uploads.rake

@@ -143,7 +143,7 @@ def migrate_from_s3
           if filename = guess_filename(url, post.raw)
             puts "FILENAME: #{filename}"
             file = FileHelper.download("http:#{url}", 20.megabytes, "from_s3", true)
-            if upload = Upload.create_for(post.user_id || -1, file, filename, File.size(file))
+            if upload = UploadCreator.new(file, filename, File.size(file)).create_for(post.user_id || -1)
               post.raw = post.raw.gsub(/(https?:)?#{Regexp.escape(url)}/, upload.url)
               post.save
               post.rebake!
@@ -433,7 +433,7 @@ def recover_from_tombstone
 
             if File.exists?(tombstone_path)
               File.open(tombstone_path) do |file|
-                new_upload = Upload.create_for(Discourse::SYSTEM_USER_ID, file, File.basename(url), File.size(file))
+                new_upload = UploadCreator.new(file, File.basename(url), File.size(file)).create_for(Discourse::SYSTEM_USER_ID)
 
                 if new_upload.persisted?
                   printf "Restored into #{new_upload.url}\n"

lib/upload_creator.rb

@@ -0,0 +1,255 @@
+require "fastimage"
+require_dependency "image_sizer"
+
+class UploadCreator
+
+  TYPES_CONVERTED_TO_JPEG ||= %i{bmp png}
+
+  WHITELISTED_SVG_ELEMENTS ||= %w{
+    circle clippath defs ellipse g line linearGradient path polygon polyline
+    radialGradient rect stop svg text textpath tref tspan use
+  }
+
+  # Available options
+  #  - type (string)
+  #  - content_type (string)
+  #  - origin (string)
+  #  - is_attachment_for_group_message (boolean)
+  #  - for_theme (boolean)
+  def initialize(file, filename, opts = {})
+    @upload = Upload.new
+    @file = file
+    @filename = filename
+    @opts = opts
+  end
+
+  def create_for(user_id)
+    if filesize <= 0
+      @upload.errors.add(:base, I18n.t("upload.empty"))
+      return @upload
+    end
+
+    DistributedMutex.synchronize("upload_#{user_id}_#{@filename}") do
+      if FileHelper.is_image?(@filename)
+        extract_image_info!
+        return @upload if @upload.errors.present?
+
+        if @filename[/\.svg$/i]
+          whitelist_svg!
+        else
+          convert_to_jpeg! if should_convert_to_jpeg?
+          downsize!        if should_downsize?
+
+          return @upload   if is_still_too_big?
+
+          fix_orientation! if should_fix_orientation?
+          crop!            if should_crop?
+          optimize!        if should_optimize?
+        end
+      end
+
+      # compute the sha of the file
+      sha1 = Upload.generate_digest(@file)
+
+      # do we already have that upload?
+      @upload = Upload.find_by(sha1: sha1)
+
+      # make sure the previous upload has not failed
+      if @upload && @upload.url.blank?
+        @upload.destroy
+        @upload = nil
+      end
+
+      # return the previous upload if any
+      return @upload unless @upload.nil?
+
+      # create the upload otherwise
+      @upload = Upload.new
+      @upload.user_id           = user_id
+      @upload.original_filename = @filename
+      @upload.filesize          = filesize
+      @upload.sha1              = sha1
+      @upload.url               = ""
+      @upload.origin            = @opts[:origin][0...1000] if @opts[:origin]
+
+      if FileHelper.is_image?(@filename)
+        @upload.width, @upload.height = ImageSizer.resize(*@image_info.size)
+      end
+
+      if @opts[:is_attachment_for_group_message]
+        @upload.is_attachment_for_group_message = true
+      end
+
+      if @opts[:for_theme]
+        @upload.for_theme = true
+      end
+
+      return @upload unless @upload.save
+
+      # store the file and update its url
+      File.open(@file.path) do |f|
+        url = Discourse.store.store_upload(f, @upload, @opts[:content_type])
+        if url.present?
+          @upload.url = url
+          @upload.save
+        else
+          @upload.errors.add(:url, I18n.t("upload.store_failure", upload_id: @upload.id, user_id: user_id))
+        end
+      end
+
+      if @upload.errors.empty? && FileHelper.is_image?(@filename) && @opts[:type] == "avatar"
+        Jobs.enqueue(:create_avatar_thumbnails, upload_id: @upload.id, user_id: user_id)
+      end
+
+      @upload
+    end
+  ensure
+    @file.close! rescue nil
+  end
+
+  def extract_image_info!
+    @image_info = FastImage.new(@file) rescue nil
+    @file.rewind
+
+    if @image_info.nil?
+      @upload.errors.add(:base, I18n.t("upload.images.not_supported_or_corrupted"))
+    elsif filesize <= 0
+      @upload.errors.add(:base, I18n.t("upload.empty"))
+    elsif pixels == 0
+      @upload.errors.add(:base, I18n.t("upload.images.size_not_found"))
+    end
+  end
+
+  def should_convert_to_jpeg?
+    TYPES_CONVERTED_TO_JPEG.include?(@image_info.type) &&
+    @image_info.size.min > 720 &&
+    SiteSetting.png_to_jpg_quality < 100
+  end
+
+  def convert_to_jpeg!
+    jpeg_tempfile = Tempfile.new(["image", ".jpg"])
+
+    OptimizedImage.ensure_safe_paths!(@file.path, jpeg_tempfile.path)
+    Discourse::Utils.execute_command('convert', @file.path, '-quality', SiteSetting.png_to_jpg_quality.to_s, jpeg_tempfile.path)
+
+    # keep the JPEG if it's at least 15% smaller
+    if File.size(jpeg_tempfile.path) < filesize * 0.85
+      @image_info = FastImage.new(jpeg_tempfile)
+      @file = jpeg_tempfile
+      @filename = (File.basename(@filename, ".*").presence || I18n.t("image").presence || "image") + ".jpg"
+      @opts[:content_type] = "image/jpeg"
+    else
+      jpeg_tempfile.close! rescue nil
+    end
+  end
+
+  def should_downsize?
+    max_image_size > 0 && filesize >= max_image_size
+  end
+
+  def downsize!
+    3.times do
+      original_size = filesize
+      downsized_pixels = [pixels, max_image_pixels].min / 2
+      OptimizedImage.downsize(@file.path, @file.path, "#{downsized_pixels}@", filename: @filename, allow_animation: allow_animation)
+      extract_image_info!
+      return if filesize >= original_size || pixels == 0 || !should_downsize?
+    end
+  end
+
+  def is_still_too_big?
+    if max_image_pixels > 0 && pixels >= max_image_pixels
+      @upload.errors.add(:base, I18n.t("upload.images.larger_than_x_megapixels", max_image_megapixels: SiteSetting.max_image_megapixels))
+      true
+    elsif max_image_size > 0 && filesize >= max_image_size
+      @upload.errors.add(:base, I18n.t("upload.images.too_large", max_size_kb: SiteSetting.max_image_size_kb))
+      true
+    else
+      false
+    end
+  end
+
+  def whitelist_svg!
+    doc = Nokogiri::XML(@file)
+    doc.xpath(svg_whitelist_xpath).remove
+    File.write(@file.path, doc.to_s)
+    @file.rewind
+  end
+
+  def should_crop?
+    Upload::CROPPED_TYPES.include?(@opts[:type])
+  end
+
+  def crop!
+    max_pixel_ratio = Discourse::PIXEL_RATIOS.max
+
+    case @opts[:type]
+    when "avatar"
+      width = height = Discourse.avatar_sizes.max
+      OptimizedImage.resize(@file.path, @file.path, width, height, filename: @filename, allow_animation: allow_animation)
+    when "profile_background"
+      max_width = 850 * max_pixel_ratio
+      width, height = ImageSizer.resize(@image_info.size[0], @image_info.size[1], max_width: max_width, max_height: max_width)
+      OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\\>", filename: @filename, allow_animation: allow_animation)
+    when "card_background"
+      max_width = 590 * max_pixel_ratio
+      width, height = ImageSizer.resize(@image_info.size[0], @image_info.size[1], max_width: max_width, max_height: max_width)
+      OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\\>", filename: @filename, allow_animation: allow_animation)
+    when "custom_emoji"
+      OptimizedImage.downsize(@file.path, @file.path, "100x100\\>", filename: @filename, allow_animation: allow_animation)
+    end
+  end
+
+  def should_fix_orientation?
+    # orientation is between 1 and 8, 1 being the default
+    # cf. http://www.daveperrett.com/articles/2012/07/28/exif-orientation-handling-is-a-ghetto/
+    @image_info.orientation.to_i > 1
+  end
+
+  def fix_orientation!
+    OptimizedImage.ensure_safe_paths!(@file.path)
+    Discourse::Utils.execute_command('convert', @file.path, '-auto-orient', @file.path)
+  end
+
+  def should_optimize?
+    # GIF is too slow (plus, we'll soon be converting them to MP4)
+    # Optimizing SVG is useless
+    return false if @file.path =~ /\.(gif|svg)$/i
+    # Safeguard for large PNGs
+    return pixels < 2_000_000 if @file.path =~ /\.png/i
+    # Everything else is fine!
+    true
+  end
+
+  def optimize!
+    OptimizedImage.ensure_safe_paths!(@file.path)
+    ImageOptim.new.optimize_image!(@file.path)
+  rescue ImageOptim::Worker::TimeoutExceeded
+    Rails.logger.warn("ImageOptim timed out while optimizing #{@filename}")
+  end
+
+  def filesize
+    File.size?(@file.path).to_i
+  end
+
+  def max_image_size
+    @@max_image_size ||= SiteSetting.max_image_size_kb.kilobytes
+  end
+
+  def max_image_pixels
+    @@max_image_pixels ||= SiteSetting.max_image_megapixels * 1_000_000
+  end
+
+  def pixels
+    @image_info.size&.reduce(:*).to_i
+  end
+
+  def allow_animation
+    @@allow_animation ||= @opts[:type] == "avatar" ? SiteSetting.allow_animated_avatars : SiteSetting.allow_animated_thumbnails
+  end
+
+  def svg_whitelist_xpath
+    @@svg_whitelist_xpath ||= "//*[#{WHITELISTED_SVG_ELEMENTS.map { |e| "name()!='#{e}'" }.join(" and ") }]"
+  end
+
+end

script/import_scripts/base/uploader.rb

@@ -15,7 +15,7 @@ module ImportScripts
       src.close
       tmp.rewind
 
-      Upload.create_for(user_id, tmp, source_filename, tmp.size)
+      UploadCreator.new(tmp, source_filename).create_for(user_id)
     rescue => e
       Rails.logger.error("Failed to create upload: #{e}")
       nil

script/import_scripts/lithium.rb

@@ -144,7 +144,7 @@ class ImportScripts::Lithium < ImportScripts::Base
     file.write(picture["filedata"].encode("ASCII-8BIT").force_encoding("UTF-8"))
     file.rewind
 
-    upload = Upload.create_for(imported_user.id, file, picture["filename"], file.size)
+    upload = UploadCreator.new(file, picture["filename"]).create_for(imported_user.id)
 
     return if !upload.persisted?
 
@@ -173,7 +173,7 @@ class ImportScripts::Lithium < ImportScripts::Base
     file.write(background["filedata"].encode("ASCII-8BIT").force_encoding("UTF-8"))
     file.rewind
 
-    upload = Upload.create_for(imported_user.id, file, background["filename"], file.size)
+    upload = UploadCreator.new(file, background["filename"]).create_for(imported_user.id)
 
     return if !upload.persisted?
 
@@ -807,7 +807,7 @@ SQL
 
           if image
             File.open(image) do |file|
-              upload = Upload.create_for(user_id, file, "image." + (image =~ /.png$/ ? "png": "jpg"), File.size(image))
+              upload = UploadCreator.new(file, "image." + (image.ends_with?(".png") ? "png" : "jpg")).create_for(user_id)
               l["src"] = upload.url
             end
           else

script/import_scripts/mbox.rb

@@ -439,7 +439,7 @@ p    end
             # read attachment
             File.open(tmp.path, "w+b") { |f| f.write attachment.body.decoded }
             # create the upload for the user
-            upload = Upload.create_for(user_id_from_imported_user_id(from_email) || Discourse::SYSTEM_USER_ID, tmp, attachment.filename, tmp.size )
+            upload = UploadCreator.new(tmp, attachment.filename).create_for(user_id_from_imported_user_id(from_email) || Discourse::SYSTEM_USER_ID)
             if upload && upload.errors.empty?
               raw << "\n\n#{receiver.attachment_markdown(upload)}\n\n"
             end
@@ -530,7 +530,7 @@ p    end
             # read attachment
             File.open(tmp.path, "w+b") { |f| f.write attachment.body.decoded }
             # create the upload for the user
-            upload = Upload.create_for(user_id_from_imported_user_id(from_email) || Discourse::SYSTEM_USER_ID, tmp, attachment.filename, tmp.size )
+            upload = UploadCreator.new(tmp, attachment.filename).create_for(user_id_from_imported_user_id(from_email) || Discourse::SYSTEM_USER_ID)
             if upload && upload.errors.empty?
               raw << "\n\n#{receiver.attachment_markdown(upload)}\n\n"
             end

script/import_scripts/sfn.rb

@@ -101,7 +101,7 @@ class ImportScripts::Sfn < ImportScripts::Base
             avatar.write(user["avatar"].encode("ASCII-8BIT").force_encoding("UTF-8"))
             avatar.rewind
 
-            upload = Upload.create_for(newuser.id, avatar, "avatar.jpg", avatar.size)
+            upload = UploadCreator.new(avatar, "avatar.jpg").create_for(newuser.id)
             if upload.persisted?
               newuser.create_user_avatar
               newuser.user_avatar.update(custom_upload_id: upload.id)

script/import_scripts/vbulletin.rb

@@ -201,7 +201,7 @@ EOM
     file.write(picture["filedata"].encode("ASCII-8BIT").force_encoding("UTF-8"))
     file.rewind
 
-    upload = Upload.create_for(imported_user.id, file, picture["filename"], file.size)
+    upload = UploadCreator.new(file, picture["filename"]).create_for(imported_user.id)
 
     return if !upload.persisted?
 
@@ -231,7 +231,7 @@ EOM
     file.write(background["filedata"].encode("ASCII-8BIT").force_encoding("UTF-8"))
     file.rewind
 
-    upload = Upload.create_for(imported_user.id, file, background["filename"], file.size)
+    upload = UploadCreator.new(file, background["filename"]).create_for(imported_user.id)
 
     return if !upload.persisted?
 

script/import_scripts/vbulletin5.rb

@@ -123,7 +123,7 @@ class ImportScripts::VBulletin < ImportScripts::Base
       file = Tempfile.new("profile-picture")
       file.write(picture["filedata"].encode("ASCII-8BIT").force_encoding("UTF-8"))
       file.rewind
-      upload = Upload.create_for(imported_user.id, file, picture["filename"], file.size)
+      upload = UploadCreator.new(file, picture["filename"]).create_for(imported_user.id)
     else
       filename = File.join(AVATAR_DIR, picture['filename'])
       unless File.exists?(filename)
@@ -160,7 +160,7 @@ class ImportScripts::VBulletin < ImportScripts::Base
     file.write(background["filedata"].encode("ASCII-8BIT").force_encoding("UTF-8"))
     file.rewind
 
-    upload = Upload.create_for(imported_user.id, file, background["filename"], file.size)
+    upload = UploadCreator.new(file, background["filename"]).create_for(imported_user.id)
 
     return if !upload.persisted?
 

spec/controllers/uploads_controller_spec.rb

@@ -48,7 +48,7 @@ describe UploadsController do
       end
 
       it 'is successful with an image' do
-        Jobs.expects(:enqueue).with(:create_thumbnails, anything)
+        Jobs.expects(:enqueue).with(:create_avatar_thumbnails, anything)
 
         message = MessageBus.track_publish do
           xhr :post, :create, file: logo, type: "avatar"
@@ -78,7 +78,7 @@ describe UploadsController do
         SiteSetting.authorized_extensions = "*"
         controller.stubs(:is_api?).returns(true)
 
-        Jobs.expects(:enqueue).with(:create_thumbnails, anything)
+        Jobs.expects(:enqueue).with(:create_avatar_thumbnails, anything)
 
         stub_request(:get, "http://example.com/image.png").to_return(body: File.read('spec/fixtures/images/logo.png'))
 
@@ -92,7 +92,7 @@ describe UploadsController do
 
       it 'correctly sets retain_hours for admins' do
         log_in :admin
-        Jobs.expects(:enqueue).with(:create_thumbnails, anything)
+        Jobs.expects(:enqueue).with(:create_avatar_thumbnails, anything).never
 
         message = MessageBus.track_publish do
           xhr :post, :create, file: logo, retain_hours: 100, type: "profile_background"
@@ -110,7 +110,7 @@ describe UploadsController do
         end.first
 
         expect(response.status).to eq 200
-        expect(message.data["errors"]).to eq(I18n.t("upload.file_missing"))
+        expect(message.data["errors"]).to contain_exactly(I18n.t("upload.file_missing"))
       end
 
       it 'properly returns errors' do
@@ -139,7 +139,7 @@ describe UploadsController do
       end
 
       it 'returns an error when it could not determine the dimensions of an image' do
-        Jobs.expects(:enqueue).with(:create_thumbnails, anything).never
+        Jobs.expects(:enqueue).with(:create_avatar_thumbnails, anything).never
 
         message = MessageBus.track_publish do
           xhr :post, :create, file: fake_jpg, type: "composer"
@@ -148,8 +148,7 @@ describe UploadsController do
         expect(response.status).to eq 200
 
         expect(message.channel).to eq("/uploads/composer")
-        expect(message.data["errors"]).to be
+        expect(message.data["errors"]).to contain_exactly(I18n.t("upload.images.size_not_found"))
-        expect(message.data["errors"][0]).to eq(I18n.t("upload.images.size_not_found"))
       end
 
     end

spec/models/theme_spec.rb

@@ -171,7 +171,7 @@ HTML
 
     it 'can handle uploads based of ThemeField' do
       theme = Theme.new(name: 'theme', user_id: -1)
-      upload = Upload.create_for(-1, image, "logo.png", File.size(image))
+      upload = UploadCreator.new(image, "logo.png").create_for(-1)
       theme.set_field(target: :common, name: :logo, upload_id: upload.id, type: :theme_upload_var)
       theme.set_field(target: :common, name: :scss, value: 'body {background-image: url($logo)}')
       theme.save!

spec/models/upload_spec.rb

@@ -46,91 +46,6 @@ describe Upload do
 
   end
 
-  context "#create_for" do
-
-    before do
-      Upload.stubs(:fix_image_orientation)
-      ImageOptim.any_instance.stubs(:optimize_image!)
-    end
-
-    it "does not create another upload if it already exists" do
-      Upload.expects(:find_by).with(sha1: image_sha1).returns(upload)
-      Upload.expects(:save).never
-      expect(Upload.create_for(user_id, image, image_filename, image_filesize)).to eq(upload)
-    end
-
-    it "ensures images isn't huge before processing it" do
-      Upload.expects(:fix_image_orientation).never
-      upload = Upload.create_for(user_id, huge_image, huge_image_filename, huge_image_filesize)
-      expect(upload.errors.size).to be > 0
-    end
-
-    it "fix image orientation" do
-      Upload.expects(:fix_image_orientation).with(image.path)
-      Upload.create_for(user_id, image, image_filename, image_filesize)
-    end
-
-    it "computes width & height for images" do
-      ImageSizer.expects(:resize)
-      Upload.create_for(user_id, image, image_filename, image_filesize)
-    end
-
-    it "does not compute width & height for non-image" do
-      FastImage.any_instance.expects(:size).never
-      upload = Upload.create_for(user_id, attachment, attachment_filename, attachment_filesize)
-      expect(upload.errors.size).to be > 0
-    end
-
-    it "generates an error when the image is too large" do
-      SiteSetting.stubs(:max_image_size_kb).returns(1)
-      upload = Upload.create_for(user_id, image, image_filename, image_filesize)
-      expect(upload.errors.size).to be > 0
-    end
-
-    it "generates an error when the attachment is too large" do
-      SiteSetting.stubs(:max_attachment_size_kb).returns(1)
-      upload = Upload.create_for(user_id, attachment, attachment_filename, attachment_filesize)
-      expect(upload.errors.size).to be > 0
-    end
-
-    it "saves proper information" do
-      store = {}
-      Discourse.expects(:store).returns(store)
-      store.expects(:store_upload).returns(url)
-
-      upload = Upload.create_for(user_id, image, image_filename, image_filesize)
-
-      expect(upload.user_id).to eq(user_id)
-      expect(upload.original_filename).to eq(image_filename)
-      expect(upload.filesize).to eq(image_filesize)
-      expect(upload.width).to eq(244)
-      expect(upload.height).to eq(66)
-      expect(upload.url).to eq(url)
-    end
-
-    context "when svg is authorized" do
-
-      before { SiteSetting.stubs(:authorized_extensions).returns("svg") }
-
-      it "consider SVG as an image" do
-        store = {}
-        Discourse.expects(:store).returns(store)
-        store.expects(:store_upload).returns(url)
-
-        upload = Upload.create_for(user_id, image_svg, image_svg_filename, image_svg_filesize)
-
-        expect(upload.user_id).to eq(user_id)
-        expect(upload.original_filename).to eq(image_svg_filename)
-        expect(upload.filesize).to eq(image_svg_filesize)
-        expect(upload.width).to eq(100)
-        expect(upload.height).to eq(50)
-        expect(upload.url).to eq(url)
-      end
-
-    end
-
-  end
-
   context ".get_from_url" do
     let(:url) { "/uploads/default/original/3X/1/0/10f73034616a796dfd70177dc54b6def44c4ba6f.png" }
     let(:upload) { Fabricate(:upload, url: url) }