FIX: crop avatars on the server instead of the client

FIX: support for dots in S3 bucket names

app/assets/javascripts/discourse/components/avatar-uploader.js.es6

@@ -10,24 +10,12 @@ export default Em.Component.extend(UploadMixin, {
   }.property("uploading"),
 
   uploadDone(upload) {
-    // in order to be as much responsive as possible, we're cheating a bit here
+    this.setProperties({
-    // indeed, the server gives us back the url to the file we've just uploaded
+      imageIsNotASquare: upload.width !== upload.height,
-    // often, this file is not a square, so we need to crop it properly
+      uploadedAvatarTemplate: upload.url,
-    // this will also capture the first frame of animated avatars when they're not allowed
+      custom_avatar_upload_id: upload.id,
-    Discourse.Utilities.cropAvatar(upload.url).then(avatarTemplate => {
-      // display a warning whenever the image is not a square
-      this.set("imageIsNotASquare", upload.width !== upload.height);
-
-      // set the avatar template to update the image on the client
-      this.set("uploadedAvatarTemplate", avatarTemplate);
-
-      // indicates the users is using an uploaded avatar (must happen after cropping, otherwise
-      //  we will attempt to load an invalid avatar and cache a redirect to old one, uploadedAvatarTemplate
-      //  trumps over custom_avatar_upload_id)
-      this.set("custom_avatar_upload_id", upload.id);
-
-      // the upload is now done
-      this.sendAction("done");
     });
+
+    this.sendAction("done");
   }
 });

app/assets/javascripts/discourse/lib/utilities.js

@@ -1,10 +1,3 @@
-/**
-  General utility functions
-
-  @class Utilities
-  @namespace Discourse
-  @module Discourse
-**/
 Discourse.Utilities = {
 
   translateSize: function(size) {
@@ -195,22 +188,10 @@ Discourse.Utilities = {
     return true;
   },
 
-
-  /**
-    Determine whether all file extensions are authorized.
-
-    @method authorizesAllExtensions
-  **/
   authorizesAllExtensions: function() {
     return Discourse.SiteSettings.authorized_extensions.indexOf("*") >= 0;
   },
 
-  /**
-    Check the extension of the file against the list of authorized extensions
-
-    @method isAuthorizedUpload
-    @param {File} file The file we want to upload
-  **/
   isAuthorizedUpload: function(file) {
     if (file && file.name) {
       var extensions = _.chain(Discourse.SiteSettings.authorized_extensions.split("|"))
@@ -222,11 +203,6 @@ Discourse.Utilities = {
     return false;
   },
 
-  /**
-    List the authorized extension for display
-
-    @method authorizedExtensions
-  **/
   authorizedExtensions: function() {
     return _.chain(Discourse.SiteSettings.authorized_extensions.split("|"))
             .reject(function(extension) { return extension.indexOf("*") >= 0; })
@@ -235,12 +211,6 @@ Discourse.Utilities = {
             .join(", ");
   },
 
-  /**
-    Get the markdown template for an upload (either an image or an attachment)
-
-    @method getUploadMarkdown
-    @param {Upload} upload The upload we want the markdown from
-  **/
   getUploadMarkdown: function(upload) {
     if (Discourse.Utilities.isAnImage(upload.original_filename)) {
       return '<img src="' + upload.url + '" width="' + upload.width + '" height="' + upload.height + '">';
@@ -249,12 +219,6 @@ Discourse.Utilities = {
     }
   },
 
-  /**
-    Check whether the path is refering to an image
-
-    @method isAnImage
-    @param {String} path The path
-  **/
   isAnImage: function(path) {
     return (/\.(png|jpe?g|gif|bmp|tiff?|svg|webp)$/i).test(path);
   },
@@ -264,11 +228,6 @@ Discourse.Utilities = {
            (/(png|jpe?g|gif|bmp|tiff?|svg|webp)/i).test(Discourse.Utilities.authorizedExtensions());
   },
 
-  /**
-    Determines whether we allow attachments or not
-
-    @method allowsAttachments
-  **/
   allowsAttachments: function() {
     return Discourse.Utilities.authorizesAllExtensions() ||
            !(/((png|jpe?g|gif|bmp|tiff?|svg|webp)(,\s)?)+$/i).test(Discourse.Utilities.authorizedExtensions());
@@ -304,55 +263,6 @@ Discourse.Utilities = {
     bootbox.alert(I18n.t('post.errors.upload'));
   },
 
-  /**
-    Crop an image to be used as avatar.
-    Simulate the "centered square thumbnail" generation done server-side.
-    Uses only the first frame of animated gifs when they are disabled.
-
-    @method cropAvatar
-    @param {String} url The url of the avatar
-    @returns {Promise} a promise that will eventually be the cropped avatar.
-  **/
-  cropAvatar: function(url) {
-    var extension = /\.(\w{2,})$/.exec(url)[1];
-    if (Discourse.SiteSettings.allow_animated_avatars && extension === "gif") {
-      // can't crop animated gifs... let the browser stretch the gif
-      return Ember.RSVP.resolve(url);
-    } else {
-      return new Ember.RSVP.Promise(function(resolve) {
-        var image = document.createElement("img");
-        image.crossOrigin = 'Anonymous';
-        // this event will be fired as soon as the image is loaded
-        image.onload = function(e) {
-          var img = e.target;
-          // computes the dimension & position (x, y) of the largest square we can fit in the image
-          var width = img.width, height = img.height, dimension, center, x, y;
-          if (width <= height) {
-            dimension = width;
-            center = height / 2;
-            x = 0;
-            y = center - (dimension / 2);
-          } else {
-            dimension = height;
-            center = width / 2;
-            x = center - (dimension / 2);
-            y = 0;
-          }
-          // set the size of the canvas to the maximum available size for avatars (browser will take care of downsizing the image)
-          var canvas = document.createElement("canvas");
-          var size = Discourse.Utilities.getRawSize(Discourse.Utilities.translateSize("huge"));
-          canvas.height = canvas.width = size;
-          // draw the image into the canvas
-          canvas.getContext("2d").drawImage(img, x, y, dimension, dimension, 0, 0, size, size);
-          // retrieve the image from the canvas
-          resolve(canvas.toDataURL("image/" + extension));
-        };
-        // launch the onload event
-        image.src = url;
-      });
-    }
-  },
-
   defaultHomepage: function() {
     // the homepage is the first item of the 'top_menu' site setting
     return Discourse.SiteSettings.top_menu.split("|")[0].split(",")[0];

app/assets/javascripts/discourse/mixins/upload.js.es6

@@ -6,10 +6,6 @@ export default Em.Mixin.create({
     Em.warn("You should implement `uploadDone`");
   },
 
-  deleteDone() {
-    Em.warn("You should implement `deleteDone`");
-  },
-
   _initialize: function() {
     const $upload = this.$(),
           csrf = Discourse.Session.currentProp("csrfToken"),

app/controllers/uploads_controller.rb

@@ -8,32 +8,42 @@ class UploadsController < ApplicationController
     url = params[:url]
 
     Scheduler::Defer.later("Create Upload") do
-      # API can provide a URL
+      begin
-      if file.nil? && url.present? && is_api?
+        # API can provide a URL
-        tempfile = FileHelper.download(url, SiteSetting.max_image_size_kb.kilobytes, "discourse-upload-#{type}") rescue nil
+        if file.nil? && url.present? && is_api?
-        filename = File.basename(URI.parse(file).path)
+          tempfile = FileHelper.download(url, SiteSetting.max_image_size_kb.kilobytes, "discourse-upload-#{type}") rescue nil
-      else
+          filename = File.basename(URI.parse(file).path)
-        tempfile = file.tempfile
+        else
-        filename = file.original_filename
+          tempfile = file.tempfile
-        content_type = file.content_type
+          filename = file.original_filename
+          content_type = file.content_type
+        end
+
+        # when we're dealing with an avatar, crop it to its maximum size
+        if type == "avatar" && FileHelper.is_image?(filename)
+          max = Discourse.avatar_sizes.max
+          OptimizedImage.resize(tempfile.path, tempfile.path, max, max, allow_animation: SiteSetting.allow_animated_avatars)
+        end
+
+        upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_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)
+        end
+
+        data = upload.errors.empty? ? upload : { errors: upload.errors.values.flatten }
+
+        MessageBus.publish("/uploads/#{type}", data.as_json, user_ids: [current_user.id])
+      rescue => e
+        pp e
+      ensure
+        tempfile.try(:close!) rescue nil
       end
-
-      upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_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)
-      end
-
-      data = upload.errors.empty? ? upload : { errors: upload.errors.values.flatten }
-
-      MessageBus.publish("/uploads/#{type}", data.as_json, user_ids: [current_user.id])
-
-      tempfile.try(:close!) rescue nil
     end
 
     # HACK FOR IE9 to prevent the "download dialog"

app/controllers/user_avatars_controller.rb

@@ -49,19 +49,14 @@ class UserAvatarsController < ApplicationController
   protected
 
   def show_in_site(hostname)
-    size = params[:size].to_i
-
-
     username = params[:username].to_s
     return render_dot unless user = User.find_by(username_lower: username.downcase)
 
     version = params[:version].to_i
     return render_dot unless version > 0 && user_avatar = user.user_avatar
 
-    # some sanity checks
+    size = params[:size].to_i
-    if size < 8 || size > 500
+    return render_dot if size < 8 || size > 500
-      return render_dot
-    end
 
     if !Discourse.avatar_sizes.include?(size) && Discourse.store.external?
       closest = Discourse.avatar_sizes.to_a.min{|a,b| (size-a).abs <=> (size-b).abs}

config/site_settings.yml

@@ -531,7 +531,7 @@ files:
     enum: 'S3RegionSiteSetting'
   s3_upload_bucket:
     default: ''
-    regex: '^[^A-Z._]+$'
+    regex: '^[^A-Z_]+$'
   s3_cdn_url:
     default: ''
     regex: '^https?:\/\/.+[^\/]$'
@@ -770,7 +770,7 @@ backups:
   s3_backup_bucket:
     client: false
     default: ''
-    regex: "^[^\\.]*$"
+    regex: "^[^A-Z_]+$"
 
 uncategorized:
   version_checks:

lib/discourse.rb

@@ -82,9 +82,7 @@ module Discourse
     # Don't cache until we can get a notification from site settings to expire cache
     set = Set.new(SiteSetting.avatar_sizes.split("|").map(&:to_i))
     # add retinas which are 2x dpi
-    set.to_a.each do |size|
+    set.to_a.each { |size| set << size * 2 }
-      set << size*2
-    end
     set
   end
 

lib/s3_helper.rb

@@ -49,14 +49,12 @@ class S3Helper
   private
 
     def s3_resource
-      opts = {}
+      opts = { region: SiteSetting.s3_region }
 
-      opts = {
+      unless SiteSetting.s3_use_iam_profile
-        access_key_id: SiteSetting.s3_access_key_id,
+        opts[:access_key_id] = SiteSetting.s3_access_key_id
-        secret_access_key: SiteSetting.s3_secret_access_key
+        opts[:secret_access_key] = SiteSetting.s3_secret_access_key
-      } unless SiteSetting.s3_use_iam_profile
+      end
-
-      opts[:region] = SiteSetting.s3_region unless SiteSetting.s3_region.blank?
 
       Aws::S3::Resource.new(opts)
     end

test/javascripts/lib/utilities-test.js.es6

@@ -169,16 +169,3 @@ test("defaultHomepage", function() {
   Discourse.SiteSettings.top_menu = "latest|top|hot";
   equal(utils.defaultHomepage(), "latest", "default homepage is the first item in the top_menu site setting");
 });
-
-module("Discourse.Utilities.cropAvatar with animated avatars", {
-  setup: function() { Discourse.SiteSettings.allow_animated_avatars = true; }
-});
-
-asyncTestDiscourse("cropAvatar", function() {
-  expect(1);
-
-  utils.cropAvatar("/path/to/avatar.gif").then(function(avatarTemplate) {
-    equal(avatarTemplate, "/path/to/avatar.gif", "returns the url to the gif when animated gif are enabled");
-    start();
-  });
-});