Build out a URI Adapter to allow uploading an avatar via a url

Currently only really accessible via the API. The UriAdapter creates a tempfile from a url and gives a ActionDispatch::HTTP::UploadedFile back to the controller to process as normal. This will help a lot in being able to transfer avatar urls from another app without monkey patching a lot of discourse code.
2013-10-18 10:33:19 -04:00 · 2013-10-18 10:33:19 -04:00 · cbef844a57
parent e527cbf884
commit cbef844a57
4 changed files with 221 additions and 35 deletions
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@ -302,14 +302,30 @@ class UsersController < ApplicationController

    file = params[:file] || params[:files].first

-    unless SiteSetting.authorized_image?(file)
+    # Only allow url uploading for API users
+    # TODO: Does not protect from huge uploads
+    # https://github.com/discourse/discourse/pull/1512
+    if file.is_a?(String) && is_api?
+      adapted   = ::UriAdapter.new(file)
+      file      = adapted.build_uploaded_file
+      filesize  = adapted.file_size
+    elsif file.is_a?(String)
      return render status: 422, text: I18n.t("upload.images.unknown_image_type")
    end

    # check the file size (note: this might also be done in the web server)
-    filesize = File.size(file.tempfile)
+    filesize ||= File.size(file.tempfile)
    max_size_kb = SiteSetting.max_image_size_kb * 1024
-    return render status: 413, text: I18n.t("upload.images.too_large", max_size_kb: max_size_kb) if filesize > max_size_kb
+
+    if filesize > max_size_kb
+      return render status: 413,
+                    text: I18n.t("upload.images.too_large",
+                                  max_size_kb: max_size_kb)
+    end
+
+    unless SiteSetting.authorized_image?(file)
+      return render status: 422, text: I18n.t("upload.images.unknown_image_type")
+    end

    upload = Upload.create_for(user.id, file, filesize)

@ -326,6 +342,8 @@ class UsersController < ApplicationController
      height: upload.height,
    }

+  rescue Discourse::InvalidParameters
+    render status: 422, text: I18n.t("upload.images.unknown_image_type")
  rescue FastImage::ImageFetchFailure
    render status: 422, text: I18n.t("upload.images.fetch_failure")
  rescue FastImage::UnknownImageType
--- a/app/services/uri_adapter.rb
+++ b/app/services/uri_adapter.rb
@ -0,0 +1,64 @@
+# For converting urls to files
+class UriAdapter
+
+  attr_reader :target, :content, :tempfile, :original_filename
+
+  def initialize(target)
+    raise Discourse::InvalidParameters unless target =~ /^https?:\/\//
+
+    @target = URI(target)
+    @original_filename = ::File.basename(@target.path)
+    @content = download_content
+    @tempfile = TempfileFactory.new.generate(@original_filename)
+  end
+
+  def download_content
+    open(target)
+  end
+
+  def copy_to_tempfile(src)
+    while data = src.read(16*1024)
+      tempfile.write(data)
+    end
+    src.close
+    tempfile.rewind
+    tempfile
+  end
+
+  def file_size
+    content.size
+  end
+
+  def build_uploaded_file
+    return if (SiteSetting.max_image_size_kb * 1024) < file_size
+
+    copy_to_tempfile(content)
+    content_type = content.content_type if content.respond_to?(:content_type)
+    content_type ||= "text/html"
+
+    ActionDispatch::Http::UploadedFile.new( tempfile: tempfile,
+                                            filename: original_filename,
+                                            type: content_type
+                                            )
+  end
+end
+
+# From https://github.com/thoughtbot/paperclip/blob/master/lib/paperclip/tempfile_factory.rb
+class TempfileFactory
+  ILLEGAL_FILENAME_CHARACTERS = /^~/
+
+  def generate(name)
+    @name = name
+    file = Tempfile.new([basename, extension])
+    file.binmode
+    file
+  end
+
+  def extension
+    File.extname(@name)
+  end
+
+  def basename
+    File.basename(@name, extension).gsub(ILLEGAL_FILENAME_CHARACTERS, '_')
+  end
+end
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@ -954,42 +954,95 @@ describe UsersController do
        })
      end

-      it 'raises an error when you don\'t have permission to upload an avatar' do
-        Guardian.any_instance.expects(:can_edit?).with(user).returns(false)
-        xhr :post, :upload_avatar, username: user.username
-        response.should be_forbidden
+      describe "with uploaded file" do
+
+        it 'raises an error when you don\'t have permission to upload an avatar' do
+          Guardian.any_instance.expects(:can_edit?).with(user).returns(false)
+          xhr :post, :upload_avatar, username: user.username
+          response.should be_forbidden
+        end
+
+        it 'rejects large images' do
+          SiteSetting.stubs(:max_image_size_kb).returns(1)
+          xhr :post, :upload_avatar, username: user.username, file: avatar
+          response.status.should eq 413
+        end
+
+        it 'rejects unauthorized images' do
+          SiteSetting.stubs(:authorized_image?).returns(false)
+          xhr :post, :upload_avatar, username: user.username, file: avatar
+          response.status.should eq 422
+        end
+
+        it 'is successful' do
+          upload = Fabricate(:upload)
+          Upload.expects(:create_for).returns(upload)
+          # enqueues the avatar generator job
+          Jobs.expects(:enqueue).with(:generate_avatars, { user_id: user.id, upload_id: upload.id })
+          xhr :post, :upload_avatar, username: user.username, file: avatar
+          user.reload
+          # erase the previous template
+          user.uploaded_avatar_template.should == nil
+          # link to the right upload
+          user.uploaded_avatar.id.should == upload.id
+          # automatically set "use_uploaded_avatar"
+          user.use_uploaded_avatar.should == true
+          # returns the url, width and height of the uploaded image
+          json = JSON.parse(response.body)
+          json['url'].should == "/uploads/default/1/1234567890123456.jpg"
+          json['width'].should == 100
+          json['height'].should == 200
+        end
      end

-      it 'rejects large images' do
-        SiteSetting.stubs(:max_image_size_kb).returns(1)
-        xhr :post, :upload_avatar, username: user.username, file: avatar
-        response.status.should eq 413
-      end
+      describe "with url" do
+        let(:avatar_url) { "http://cdn.discourse.org/assets/logo.png" }

-      it 'rejects unauthorized images' do
-        SiteSetting.stubs(:authorized_image?).returns(false)
-        xhr :post, :upload_avatar, username: user.username, file: avatar
-        response.status.should eq 422
-      end
+        before :each do
+          UsersController.any_instance.stubs(:is_api?).returns(true)
+        end
+
+        describe "correct urls" do
+          before :each do
+            UriAdapter.any_instance.stubs(:open).returns StringIO.new(fixture_file("images/logo.png"))
+          end
+
+          it 'rejects large images' do
+            SiteSetting.stubs(:max_image_size_kb).returns(1)
+            xhr :post, :upload_avatar, username: user.username, file: avatar_url
+            response.status.should eq 413
+          end
+
+          it 'rejects unauthorized images' do
+            SiteSetting.stubs(:authorized_image?).returns(false)
+            xhr :post, :upload_avatar, username: user.username, file: avatar_url
+            response.status.should eq 422
+          end
+
+          it 'is successful' do
+            upload = Fabricate(:upload)
+            Upload.expects(:create_for).returns(upload)
+            # enqueues the avatar generator job
+            Jobs.expects(:enqueue).with(:generate_avatars, { user_id: user.id, upload_id: upload.id })
+            xhr :post, :upload_avatar, username: user.username, file: avatar_url
+            user.reload
+            user.uploaded_avatar_template.should == nil
+            user.uploaded_avatar.id.should == upload.id
+            user.use_uploaded_avatar.should == true
+
+            # returns the url, width and height of the uploaded image
+            json = JSON.parse(response.body)
+            json['url'].should == "/uploads/default/1/1234567890123456.jpg"
+            json['width'].should == 100
+            json['height'].should == 200
+          end
+        end
+
+        it "should handle malformed urls" do
+          xhr :post, :upload_avatar, username: user.username, file: "foobar"
+          response.status.should eq 422
+        end

-      it 'is successful' do
-        upload = Fabricate(:upload)
-        Upload.expects(:create_for).returns(upload)
-        # enqueues the avatar generator job
-        Jobs.expects(:enqueue).with(:generate_avatars, { user_id: user.id, upload_id: upload.id })
-        xhr :post, :upload_avatar, username: user.username, file: avatar
-        user.reload
-        # erase the previous template
-        user.uploaded_avatar_template.should == nil
-        # link to the right upload
-        user.uploaded_avatar.id.should == upload.id
-        # automatically set "use_uploaded_avatar"
-        user.use_uploaded_avatar.should == true
-        # returns the url, width and height of the uploaded image
-        json = JSON.parse(response.body)
-        json['url'].should == "/uploads/default/1/1234567890123456.jpg"
-        json['width'].should == 100
-        json['height'].should == 200
      end

    end
--- a/spec/services/uri_adapter_spec.rb
+++ b/spec/services/uri_adapter_spec.rb
@ -0,0 +1,51 @@
+require 'spec_helper'
+
+describe UriAdapter do
+  let(:target) { "http://cdn.discourse.org/assets/logo.png" }
+  let(:response) { StringIO.new(fixture_file("images/logo.png")) }
+
+  before :each do
+    response.stubs(:content_type).returns("image/png")
+    UriAdapter.any_instance.stubs(:open).returns(response)
+  end
+
+  subject { UriAdapter.new(target) }
+
+  describe "#initialize" do
+
+    it "has a target" do
+      subject.target.should be_instance_of(URI::HTTP)
+    end
+
+    it "has content" do
+      subject.content.should == response
+    end
+
+    it "has an original_filename" do
+      subject.original_filename.should == "logo.png"
+    end
+
+    it "has a tempfile" do
+      subject.tempfile.should be_instance_of Tempfile
+    end
+
+  end
+
+  describe "#copy_to_tempfile" do
+    it "does not allow files bigger then max_image_size_kb" do
+      SiteSetting.stubs(:max_image_size_kb).returns(1)
+      subject.build_uploaded_file.should == nil
+    end
+  end
+
+  describe "#build_uploaded_file" do
+    it "returns an uploaded file" do
+      file = subject.build_uploaded_file
+      file.should be_instance_of(ActionDispatch::Http::UploadedFile)
+      file.content_type.should == "image/png"
+      file.original_filename.should == "logo.png"
+      file.tempfile.should be_instance_of Tempfile
+    end
+  end
+
+end