Merge pull request #956 from ZogStriP/fix-image-upload-to-s3

fix image uploads on s3/imgur

app/controllers/uploads_controller.rb

@@ -4,10 +4,14 @@ class UploadsController < ApplicationController
   def create
     requires_parameter(:topic_id)
     file = params[:file] || params[:files].first
+    
     # only supports images for now
     return render status: 415, json: failed_json unless file.content_type =~ /^image\/.+/
+    
     upload = Upload.create_for(current_user.id, file, params[:topic_id])
+    
     render_serialized(upload, UploadSerializer, root: false)
+  
   rescue FastImage::ImageFetchFailure
     render status: 422, text: I18n.t("upload.image.fetch_failure")
   rescue FastImage::UnknownImageType
@@ -15,4 +19,5 @@ class UploadsController < ApplicationController
   rescue FastImage::SizeNotFound
     render status: 422, text: I18n.t("upload.image.size_not_found")
   end
+
 end

app/models/site_setting.rb

@@ -73,8 +73,7 @@ class SiteSetting < ActiveRecord::Base
   setting(:crawl_images, !Rails.env.test?)
   setting(:enable_imgur, false)
   setting(:imgur_client_id, '')
-  setting(:imgur_client_secret, '')
-  setting(:imgur_endpoint, "http://api.imgur.com/3/image.json")
+  setting(:imgur_endpoint, "https://api.imgur.com/3/image.json")
   setting(:max_image_width, 690)
   client_setting(:category_featured_topics, 6)
   setting(:topics_per_page, 30)
@@ -165,7 +164,7 @@ class SiteSetting < ActiveRecord::Base
   setting(:enable_s3_uploads, false)
   setting(:s3_access_key_id, '')
   setting(:s3_secret_access_key, '')
-  setting(:s3_region, 'us-west-1')
+  setting(:s3_region, '')
   setting(:s3_upload_bucket, '')
 
   setting(:default_trust_level, 0)

app/models/upload.rb

@@ -1,4 +1,8 @@
 require 'digest/sha1'
+require 'image_sizer'
+require 'imgur'
+require 's3'
+require 'local_store'
 
 class Upload < ActiveRecord::Base
   belongs_to :user
@@ -7,96 +11,39 @@ class Upload < ActiveRecord::Base
   validates_presence_of :filesize
   validates_presence_of :original_filename
 
-  # Create an upload given a user, file and topic
   def self.create_for(user_id, file, topic_id)
-    return create_on_imgur(user_id, file, topic_id) if SiteSetting.enable_imgur?
-    return create_on_s3(user_id, file, topic_id) if SiteSetting.enable_s3_uploads?
-    return create_locally(user_id, file, topic_id)
-  end
+    # retrieve image info
+    image_info = FastImage.new(file.tempfile, raise_on_failure: true)
+    # compute image aspect ratio
+    width, height = ImageSizer.resize(*image_info.size)
 
-  # Store uploads on imgur
-  def self.create_on_imgur(user_id, file, topic_id)
-    @imgur_loaded = require 'imgur' unless @imgur_loaded
-
-    info = Imgur.upload_file(file)
-
-    Upload.create!({
-      user_id: user_id,
-      topic_id: topic_id,
-      original_filename: file.original_filename
-    }.merge!(info))
-  end
-
-  # Store uploads on s3
-  def self.create_on_s3(user_id, file, topic_id)
-    @fog_loaded = require 'fog' unless @fog_loaded
-
-    tempfile = file.tempfile
-
-    upload = Upload.new(user_id: user_id,
-                        topic_id: topic_id,
-                        filesize: File.size(tempfile),
-                        original_filename: file.original_filename)
-
-    image_info = FastImage.new(tempfile, raise_on_failure: true)
-    blob = file.read
-    sha1 = Digest::SHA1.hexdigest(blob)
-    remote_filename = "#{sha1}.#{image_info.type}"
-
-    fog = Fog::Storage.new(
-      aws_access_key_id: SiteSetting.s3_access_key_id,
-      aws_secret_access_key: SiteSetting.s3_secret_access_key,
-      region: SiteSetting.s3_region,
-      provider: 'AWS'
-    )
-
-    directory = fog.directories.create(key: SiteSetting.s3_upload_bucket)
-
-    file = directory.files.create(
-      key: remote_filename,
-      body: tempfile,
-      public: true,
-      content_type: file.content_type
-    )
-    
-    upload.width, upload.height = ImageSizer.resize(*image_info.size)
-    upload.url = "//#{SiteSetting.s3_upload_bucket}.s3-#{SiteSetting.s3_region}.amazonaws.com/#{remote_filename}"
-
-    upload.save
-
-    upload
-  end
-
-  def self.create_locally(user_id, file, topic_id)
     upload = Upload.create!({
       user_id: user_id,
       topic_id: topic_id,
-      url: "",
+      original_filename: file.original_filename,
       filesize: File.size(file.tempfile),
-      original_filename: file.original_filename
+      width: width,
+      height: height,
+      url: ""
     })
 
-    # populate the rest of the info
-    clean_name = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0,16]
-    image_info = FastImage.new(file.tempfile, raise_on_failure: true)
-    clean_name += ".#{image_info.type}"
-    url_root = "/uploads/#{RailsMultisite::ConnectionManagement.current_db}/#{upload.id}"
-    path = "#{Rails.root}/public#{url_root}"
+    # make sure we're at the beginning of the file (FastImage is moving the pointer)
+    file.rewind
 
-    FileUtils.mkdir_p path
-    # not using cause mv, cause permissions are no good on move
-    File.open("#{path}/#{clean_name}", "wb") do |f|
-      f.write File.read(file.tempfile)
-    end
-
-    upload.width, upload.height = ImageSizer.resize(*image_info.size)
-    upload.url = Discourse::base_uri + "#{url_root}/#{clean_name}"
+    # store the file and update its url
+    upload.url = Upload.store_file(file, image_info, upload.id)
 
     upload.save
 
     upload
   end
 
+  def self.store_file(file, image_info, upload_id)
+    return Imgur.store_file(file, image_info, upload_id) if SiteSetting.enable_imgur?
+    return S3.store_file(file, image_info, upload_id)    if SiteSetting.enable_s3_uploads?
+    return LocalStore.store_file(file, image_info, upload_id)
+  end
+
 end
 
 # == Schema Information
@@ -119,4 +66,3 @@ end
 #  index_uploads_on_forum_thread_id  (topic_id)
 #  index_uploads_on_user_id          (user_id)
 #
-

lib/discourse.rb

@@ -14,6 +14,9 @@ module Discourse
   # When something they want is not found
   class NotFound < Exception; end
 
+  # When a setting is missing
+  class SiteSettingMissing < Exception; end
+
   def self.cache
     @cache ||= Cache.new
   end

lib/imgur.rb

@@ -1,27 +1,25 @@
 require 'rest_client'
-require 'image_size'
 
 module Imgur
 
-  def self.upload_file(file)
+  def self.store_file(file, image_info, upload_id)
+    raise Discourse::SiteSettingMissing.new("imgur_endpoint")   if SiteSetting.imgur_endpoint.blank?
+    raise Discourse::SiteSettingMissing.new("imgur_client_id")  if SiteSetting.imgur_client_id.blank?
+
+    @imgur_loaded = require 'imgur' unless @imgur_loaded
 
     blob = file.read
-    response = RestClient.post(SiteSetting.imgur_endpoint, { image: Base64.encode64(blob) }, { 'Authorization' => "Client-ID #{SiteSetting.imgur_client_id}" })
+
+    response = RestClient.post(
+      SiteSetting.imgur_endpoint, 
+      { image: Base64.encode64(blob) }, 
+      { 'Authorization' => "Client-ID #{SiteSetting.imgur_client_id}" }
+    )
 
     json = JSON.parse(response.body)['data'] rescue nil
 
     return nil if json.blank?
-
-    # Resize the image
-    image_info = FastImage.new(file, raise_on_failure: true)
-    width, height = ImageSizer.resize(*image_info.size)
-
-    {
-      url: json['link'],
-      filesize: File.size(file),
-      width: width,
-      height: height
-    }
+    return json['link']
   end
 
 end

lib/local_store.rb

@@ -0,0 +1,18 @@
+module LocalStore
+
+  def self.store_file(file, image_info, upload_id)
+    clean_name = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0,16] + ".#{image_info.type}"
+    url_root = "/uploads/#{RailsMultisite::ConnectionManagement.current_db}/#{upload_id}"
+    path = "#{Rails.root}/public#{url_root}"
+
+    FileUtils.mkdir_p path
+    # not using cause mv, cause permissions are no good on move
+    File.open("#{path}/#{clean_name}", "wb") do |f|
+      f.write File.read(file.tempfile)
+    end
+
+    # url
+    return Discourse::base_uri + "#{url_root}/#{clean_name}"
+  end
+
+end

lib/s3.rb

@@ -0,0 +1,50 @@
+module S3
+
+  def self.store_file(file, image_info, upload_id)
+    raise Discourse::SiteSettingMissing.new("s3_upload_bucket")     if SiteSetting.s3_upload_bucket.blank?
+    raise Discourse::SiteSettingMissing.new("s3_access_key_id")     if SiteSetting.s3_access_key_id.blank?
+    raise Discourse::SiteSettingMissing.new("s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank?
+
+    @fog_loaded = require 'fog' unless @fog_loaded
+
+    blob = file.read
+    sha1 = Digest::SHA1.hexdigest(blob)
+    remote_filename = "#{upload_id}#{sha1}.#{image_info.type}"
+
+    options = S3.generate_options
+    directory = S3.get_or_create_directory(SiteSetting.s3_upload_bucket, options)
+    # if this fails, it will throw an exception
+    file = S3.upload(file, remote_filename, directory)
+
+    return "//#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/#{remote_filename}"
+  end
+
+  def self.generate_options
+    options = {
+      provider: 'AWS',
+      aws_access_key_id: SiteSetting.s3_access_key_id,
+      aws_secret_access_key: SiteSetting.s3_secret_access_key
+    }
+    options[:region] = SiteSetting.s3_region unless SiteSetting.s3_region.empty?
+
+    options
+  end
+
+  def self.get_or_create_directory(name, options)
+    fog = Fog::Storage.new(options)
+    directory = fog.directories.get(name)
+    directory = fog.directories.create(key: name) unless directory
+
+    directory
+  end
+
+  def self.upload(file, name, directory)
+    directory.files.create(
+      key: name,
+      public: true,
+      body: file.tempfile,
+      content_type: file.content_type
+    )
+  end
+
+end

spec/components/imgur_spec.rb

@@ -3,12 +3,18 @@ require 'imgur'
 
 describe Imgur do
 
-  describe "upload_file" do
+  describe "store_file" do
 
     let(:file) { Rails.root.join('app', 'assets', 'images', 'logo.png') }
+    let(:image_info) { FastImage.new(file) }
     let(:params) { [SiteSetting.imgur_endpoint, { image: Base64.encode64(file.read) }, { 'Authorization' => "Client-ID #{SiteSetting.imgur_client_id}" }] }
 
-    it 'returns JSON of the Imgur upload if successful' do
+    before(:each) do 
+      SiteSetting.stubs(:imgur_endpoint).returns("imgur_endpoint")
+      SiteSetting.stubs(:imgur_client_id).returns("imgur_client_id")
+    end
+
+    it 'returns the url of the Imgur upload if successful' do
       json = {
         data: {
           id: 'fake',
@@ -21,20 +27,9 @@ describe Imgur do
 
       response = mock
       response.expects(:body).returns(json)
-
-      image_info = {
-        url: 'http://imgur.com/fake.png',
-        filesize: File.size(file)
-      }
-
       RestClient.expects(:post).with(*params).returns(response)
-      result = Imgur.upload_file(file)
 
-      # Not testing what width/height actually are because ImageSizer is already tested
-      result[:url].should eq(image_info[:url])
-      result[:filesize].should eq(image_info[:filesize])
-      result[:width].should_not be_nil
-      result[:height].should_not be_nil
+      Imgur.store_file(file, image_info, 1).should == 'http://imgur.com/fake.png'
     end
 
     it 'returns nil if the request fails' do
@@ -47,7 +42,7 @@ describe Imgur do
       response.expects(:body).returns(json)
       RestClient.expects(:post).with(*params).returns(response)
 
-      Imgur.upload_file(file).should be_nil
+      Imgur.store_file(file, image_info, 1).should be_nil
     end
 
   end

spec/components/local_store_spec.rb

@@ -0,0 +1,30 @@
+require 'spec_helper'
+require 'local_store'
+
+describe LocalStore do
+
+  describe "store_file" do
+
+    let(:file) do
+      ActionDispatch::Http::UploadedFile.new({
+        filename: 'logo.png',
+        content_type: 'image/png',
+        tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
+      })
+    end
+
+    let(:image_info) { FastImage.new(file) }
+
+    it 'returns the url of the S3 upload if successful' do
+      # prevent the tests from creating directories & files...
+      FileUtils.stubs(:mkdir_p)
+      File.stubs(:open)
+      # The Time needs to be frozen as it is used to generate a clean & unique name
+      Time.stubs(:now).returns(Time.utc(2013, 2, 17, 12, 0, 0, 0))
+      # 
+      LocalStore.store_file(file, image_info, 1).should == '/uploads/default/1/253dc8edf9d4ada1.png'
+    end
+
+  end
+
+end

spec/components/s3_spec.rb

@@ -0,0 +1,36 @@
+require 'spec_helper'
+require 'fog'
+require 's3'
+
+describe S3 do
+
+  describe "store_file" do
+
+    let(:file) do
+      ActionDispatch::Http::UploadedFile.new({
+        filename: 'logo.png',
+        content_type: 'image/png',
+        tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
+      })
+    end
+
+    let(:image_info) { FastImage.new(file) }
+
+    before(:each) do 
+      SiteSetting.stubs(:s3_upload_bucket).returns("s3_upload_bucket")
+      SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id")
+      SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key")
+      Fog.mock!
+    end
+
+    it 'returns the url of the S3 upload if successful' do
+      S3.store_file(file, image_info, 1).should == '//s3_upload_bucket.s3.amazonaws.com/1e8b1353813a7d091231f9a27f03566f123463fc1.png'
+    end
+
+    after(:each) do
+      Fog.unmock!
+    end
+
+  end
+
+end

spec/models/upload_spec.rb

@@ -1,6 +1,4 @@
 require 'spec_helper'
-require 'fog'
-require 'imgur'
 
 describe Upload do
 
@@ -23,95 +21,45 @@ describe Upload do
       })
     end
 
-    it "uses imgur when it is enabled" do
-      SiteSetting.stubs(:enable_imgur?).returns(true)
-      Upload.expects(:create_on_imgur).with(user_id, logo, topic_id)
-      Upload.create_for(user_id, logo, topic_id)
-    end
+    let(:upload) { Upload.create_for(user_id, logo, topic_id) }
 
-    it "uses s3 when it is enabled" do
-      SiteSetting.stubs(:enable_s3_uploads?).returns(true)
-      Upload.expects(:create_on_s3).with(user_id, logo, topic_id)
-      Upload.create_for(user_id, logo, topic_id)
-    end
-
-    it "uses local storage otherwise" do
-      Upload.expects(:create_locally).with(user_id, logo, topic_id)
-      Upload.create_for(user_id, logo, topic_id)
-    end
+    let(:url) { "http://domain.com" }
 
     shared_examples_for "upload" do
       it "is valid" do
+        upload.user_id.should == user_id
+        upload.topic_id.should == topic_id
         upload.original_filename.should == logo.original_filename
-        upload.filesize.should == logo.size
+        upload.filesize.should == File.size(logo.tempfile)
         upload.width.should == 244
         upload.height.should == 66
+        upload.url.should == url
       end
     end
 
-    context 'imgur' do
-
+    context "imgur" do
       before(:each) do
-        # Stub out Imgur entirely as it already is tested.
-        Imgur.stubs(:upload_file).returns({
-          url: "imgurlink",
-          filesize: logo.size,
-          width: 244,
-          height: 66
-        })
+        SiteSetting.stubs(:enable_imgur?).returns(true)
+        Imgur.stubs(:store_file).returns(url)
       end
 
-      let(:upload) { Upload.create_on_imgur(user_id, logo, topic_id) }
-
       it_behaves_like "upload"
 
-      it "works" do
-        upload.url.should == "imgurlink"
-      end
-
     end
 
-    context 's3' do
-
+    context "s3" do
       before(:each) do 
-        SiteSetting.stubs(:s3_upload_bucket).returns("bucket")
-        Fog.mock!
+        SiteSetting.stubs(:enable_s3_uploads?).returns(true)
+        S3.stubs(:store_file).returns(url)
       end
 
-      let(:upload) { Upload.create_on_s3(user_id, logo, topic_id) }
-
       it_behaves_like "upload"
 
-      it "works" do
-        upload.url.should == "//bucket.s3-us-west-1.amazonaws.com/e8b1353813a7d091231f9a27f03566f123463fc1.png"
-      end
-
-      after(:each) do
-        Fog.unmock!
-      end
-
     end
 
-    context 'local' do
-
-      before(:each) do
-        # prevent the tests from creating directories & files...
-        FileUtils.stubs(:mkdir_p)
-        File.stubs(:open)
-      end
-
-      let(:upload) do
-        # The Time needs to be frozen as it is used to generate a clean & unique name
-        Time.stubs(:now).returns(Time.utc(2013, 2, 17, 12, 0, 0, 0))
-        Upload.create_locally(user_id, logo, topic_id)
-      end
-
+    context "locally" do
+      before(:each) { LocalStore.stubs(:store_file).returns(url) }
       it_behaves_like "upload"
-
-      it "works" do
-        upload.url.should match /\/uploads\/default\/[\d]+\/253dc8edf9d4ada1.png/
-      end
-
     end
 
   end