Merge pull request #1177 from ZogStriP/attachments

Attachments

app/assets/javascripts/discourse/components/click_track.js

@@ -85,7 +85,7 @@ Discourse.ClickTrack = {
     }
 
     // If we're on the same site, use the router and track via AJAX
-    if ((href.indexOf(Discourse.URL.origin()) === 0) && (!href.match(/\.(png|gif|jpg|jpeg)$/i))) {
+    if ((href.indexOf(Discourse.URL.origin()) === 0) && !href.match(/\/uploads\//i)) {
       Discourse.ajax("/clicks/track", {
         data: {
           url: href,
@@ -109,5 +109,3 @@ Discourse.ClickTrack = {
     return false;
   }
 };
-
-

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

@@ -212,6 +212,30 @@ Discourse.Utilities = {
     if (!extensions) return false;
     var regexp = new RegExp("\\.(" + extensions.replace(/\./g, "") + ")$", "i");
     return file && file.name ? file.name.match(regexp) : false;
+  },
+
+  /**
+    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 (this.isAnImage(upload.original_filename)) {
+      return '<img src="' + upload.url + '" width="' + upload.width + '" height="' + upload.height + '">';
+    } else {
+      return '<a class="attachment" href="' + upload.url + '">' + upload.original_filename + '</a>';
+    }
+  },
+
+  /**
+    Check whether the path is refering to an image
+
+    @method isAnImage
+    @param {String} path The path
+  **/
+  isAnImage: function(path) {
+    return path && path.match(/\.(png|jpg|jpeg|gif|bmp|tif)$/i);
   }
 
 };

app/assets/javascripts/discourse/views/composer_view.js

@@ -285,9 +285,9 @@ Discourse.ComposerView = Discourse.View.extend({
 
     // done
     $uploadTarget.on('fileuploaddone', function (e, data) {
-      var upload = data.result;
+      var markdown = Discourse.Utilities.getUploadMarkdown(data.result);
-      var html = "<img src=\"" + upload.url + "\" width=\"" + upload.width + "\" height=\"" + upload.height + "\">";
+      // appends a space at the end of the inserted markdown
-      composerView.addMarkdown(html);
+      composerView.addMarkdown(markdown + " ");
       composerView.set('isUploading', false);
     });
 

app/assets/javascripts/external/Markdown.Converter.js

@@ -1275,7 +1275,7 @@ else
             //  autolink anything like <http://example.com>
 
             var replacer = function (wholematch, m1) {
-                m1encoded = m1.replace(/\_\_/, '%5F%5F');
+                var m1encoded = m1.replace(/\_\_/, '%5F%5F');
                 return "<a href=\"" + m1encoded + "\">" + pluginHooks.plainLinkText(m1) + "</a>";
             }
             text = text.replace(/<((https?|ftp):[^'">\s]+)>/gi, replacer);

app/assets/stylesheets/application/topic.css.scss

@@ -175,6 +175,16 @@
     }
   }
 
+  a.attachment:before {
+    display: inline-block;
+    margin-right: 4px;
+    font-family: "FontAwesome";
+    content: "\f019";
+  }
+  .attachment + .size {
+    margin: 0 5px;
+  }
+
   // When we are quoting something
   aside.quote {
     border-left: 5px solid #d7d7d7;

app/controllers/uploads_controller.rb

@@ -4,8 +4,7 @@ class UploadsController < ApplicationController
   def create
     file = params[:file] || params[:files].first
 
-    # only supports images for now
+    return render status: 415, json: failed_json unless SiteSetting.authorized_file?(file)
-    return render status: 415, json: failed_json unless file.content_type =~ /^image\/.+/
 
     upload = Upload.create_for(current_user.id, file)
 

app/models/optimized_image.rb

@@ -49,19 +49,15 @@ class OptimizedImage < ActiveRecord::Base
   end
 
   def url
-    "#{Upload.base_url}/#{optimized_path}/#{filename}"
+    "#{LocalStore.base_url}/#{optimized_path}/#{filename}"
   end
 
   def path
-    "#{path_root}/#{optimized_path}/#{filename}"
+    "#{LocalStore.base_path}/#{optimized_path}/#{filename}"
-  end
-
-  def path_root
-    @path_root ||= "#{Rails.root}/public"
   end
 
   def optimized_path
-    "uploads/#{RailsMultisite::ConnectionManagement.current_db}/_optimized/#{sha1[0..2]}/#{sha1[3..5]}"
+    "_optimized/#{sha1[0..2]}/#{sha1[3..5]}"
   end
 
   def filename

app/models/site_setting.rb

@@ -274,6 +274,23 @@ class SiteSetting < ActiveRecord::Base
     top_menu_items.map { |item| item.name }.select{ |item| list.include?(item) }.first
   end
 
+  def self.authorized_file?(file)
+    file.original_filename =~ /\.(#{authorized_extensions.tr(". ", "")})$/i
+  end
+
+  def self.images
+    @images ||= ["jpg", "jpeg", "png", "gif", "tif", "tiff", "bmp"]
+  end
+
+  def self.authorized_image?(file)
+    authorized_images = authorized_extensions
+                          .tr(". ", "")
+                          .split("|")
+                          .select { |extension| images.include?(extension) }
+                          .join("|")
+    file.original_filename =~ /\.(#{authorized_images})$/i
+  end
+
 end
 
 # == Schema Information

app/models/upload.rb

@@ -1,9 +1,9 @@
 require 'digest/sha1'
 require 'image_sizer'
-require 's3'
-require 'local_store'
 require 'tempfile'
 require 'pathname'
+require_dependency 's3'
+require_dependency 'local_store'
 
 class Upload < ActiveRecord::Base
   belongs_to :user
@@ -48,24 +48,25 @@ class Upload < ActiveRecord::Base
     sha1 = Digest::SHA1.file(file.tempfile).hexdigest
     # check if the file has already been uploaded
     unless upload = Upload.where(sha1: sha1).first
-      # retrieve image info
-      image_info = FastImage.new(file.tempfile, raise_on_failure: true)
-      # compute image aspect ratio
-      width, height = ImageSizer.resize(*image_info.size)
       # create a db record (so we can use the id)
       upload = Upload.create!({
         user_id: user_id,
         original_filename: file.original_filename,
         filesize: File.size(file.tempfile),
         sha1: sha1,
-        width: width,
-        height: height,
         url: ""
       })
-      # make sure we're at the beginning of the file (FastImage is moving the pointer)
+      # deal with width & heights for images
-      file.rewind
+      if SiteSetting.authorized_image?(file)
+        # retrieve image info
+        image_info = FastImage.new(file.tempfile, raise_on_failure: true)
+        # compute image aspect ratio
+        upload.width, upload.height = ImageSizer.resize(*image_info.size)
+        # make sure we're at the beginning of the file (FastImage is moving the pointer)
+        file.rewind
+      end
       # store the file and update its url
-      upload.url = Upload.store_file(file, sha1, image_info, upload.id)
+      upload.url = Upload.store_file(file, sha1, upload.id)
       # save the url
       upload.save
     end
@@ -73,9 +74,9 @@ class Upload < ActiveRecord::Base
     upload
   end
 
-  def self.store_file(file, sha1, image_info, upload_id)
+  def self.store_file(file, sha1, upload_id)
-    return S3.store_file(file, sha1, image_info, upload_id) if SiteSetting.enable_s3_uploads?
+    return S3.store_file(file, sha1, upload_id) if SiteSetting.enable_s3_uploads?
-    return LocalStore.store_file(file, sha1, image_info, upload_id)
+    return LocalStore.store_file(file, sha1, upload_id)
   end
 
   def self.remove_file(url)
@@ -92,29 +93,15 @@ class Upload < ActiveRecord::Base
   end
 
   def self.is_local?(url)
-    url.start_with?(base_url)
+    !SiteSetting.enable_s3_uploads? && url.start_with?(LocalStore.base_url)
   end
 
   def self.is_on_s3?(url)
     SiteSetting.enable_s3_uploads? && url.start_with?(S3.base_url)
   end
 
-  def self.base_url
-    asset_host.present? ? asset_host : Discourse.base_url_no_prefix
-  end
-
-  def self.asset_host
-    ActionController::Base.asset_host
-  end
-
   def self.get_from_url(url)
-    if has_been_uploaded?(url)
+    Upload.where(url: url).first if has_been_uploaded?(url)
-      if m = LocalStore.uploaded_regex.match(url)
-        Upload.where(id: m[:upload_id]).first
-      elsif is_on_s3?(url)
-        Upload.where(url: url).first
-      end
-    end
   end
 
 end

app/serializers/upload_serializer.rb

@@ -1,5 +1,5 @@
 class UploadSerializer < ApplicationSerializer
 
-  attributes :url, :filesize, :original_filename, :width, :height
+  attributes :url, :original_filename, :width, :height
 
 end

config/nginx.sample.conf

@@ -21,8 +21,8 @@ server {
   location / {
     root /home/discourse/discourse/public;
 
-    ## optional image anti-hotlinking rules
+    ## optional upload anti-hotlinking rules
-    #location ~ \.(jpe?g|png|gif)$ {
+    #location ~ ^/uploads/ {
     #  valid_referers none blocked mysite.com *.mysite.com;
     #  if ($invalid_referer) {
     #    return 403;
@@ -35,7 +35,7 @@ server {
       add_header ETag "";
     }
 
-    location ~ ^/assets/ {
+    location ~ ^/(assets|uploads)/ {
       expires 1y;
       add_header Cache-Control public;
       add_header ETag "";

lib/cooked_post_processor.rb

@@ -16,10 +16,26 @@ class CookedPostProcessor
   end
 
   def post_process
+    post_process_attachments
     post_process_images
     post_process_oneboxes
   end
 
+  def post_process_attachments
+    attachments.each do |attachment|
+      href = attachment['href']
+      attachment['href'] = relative_to_absolute(href)
+      if upload = Upload.get_from_url(href)
+        # update reverse index
+        associate_to_post(upload)
+        # append the size
+        append_human_size!(attachment, upload)
+      end
+      # mark as dirty
+      @dirty = true
+    end
+  end
+
   def post_process_images
     images = extract_images
     return if images.blank?
@@ -33,7 +49,7 @@ class CookedPostProcessor
         # make sure the img has proper width and height attributes
         update_dimensions!(img)
         # retrieve the associated upload, if any
-        if upload = Upload.get_from_url(img['src'])
+        if upload = Upload.get_from_url(src)
           # update reverse index
           associate_to_post(upload)
         end
@@ -209,6 +225,22 @@ class CookedPostProcessor
   rescue URI::InvalidURIError
   end
 
+  def attachments
+    if SiteSetting.enable_s3_uploads?
+      @doc.css("a[href^=\"#{S3.base_url}\"]")
+    else
+      # local uploads are identified using a relative uri
+      @doc.css("a[href^=\"#{LocalStore.directory}\"]")
+    end
+  end
+
+  def append_human_size!(attachment, upload)
+    size = Nokogiri::XML::Node.new("span", @doc)
+    size["class"] = "size"
+    size.content = "(#{number_to_human_size(upload.filesize)})"
+    attachment.add_next_sibling(size)
+  end
+
   def dirty?
     @dirty
   end

lib/local_store.rb

@@ -1,18 +1,21 @@
 module LocalStore
 
-  def self.store_file(file, sha1, image_info, upload_id)
+  def self.store_file(file, sha1, upload_id)
-    clean_name = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0,16] + ".#{image_info.type}"
+    unique_sha1 = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0,16]
+    extension = File.extname(file.original_filename)
+    clean_name = "#{unique_sha1}#{extension}"
     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}"
+    Discourse::base_uri + "#{url_root}/#{clean_name}"
   end
 
   def self.remove_file(url)
@@ -24,4 +27,21 @@ module LocalStore
     /\/uploads\/#{RailsMultisite::ConnectionManagement.current_db}\/(?<upload_id>\d+)\/[0-9a-f]{16}\.(png|jpg|jpeg|gif|tif|tiff|bmp)/
   end
 
+  def self.base_url
+    url = asset_host.present? ? asset_host : Discourse.base_url_no_prefix
+    "#{url}#{directory}"
+  end
+
+  def self.base_path
+    "#{Rails.root}/public#{directory}"
+  end
+
+  def self.directory
+    "/uploads/#{RailsMultisite::ConnectionManagement.current_db}"
+  end
+
+  def self.asset_host
+    ActionController::Base.asset_host
+  end
+
 end

lib/s3.rb

@@ -1,11 +1,11 @@
 module S3
 
-  def self.store_file(file, sha1, image_info, upload_id)
+  def self.store_file(file, sha1, upload_id)
     S3.check_missing_site_settings
 
     directory = S3.get_or_create_directory(SiteSetting.s3_upload_bucket)
-
+    extension = File.extname(file.original_filename)
-    remote_filename = "#{upload_id}#{sha1}.#{image_info.type}"
+    remote_filename = "#{upload_id}#{sha1}#{extension}"
 
     # if this fails, it will throw an exception
     file = S3.upload(file, remote_filename, directory)

spec/components/cooked_post_processor_spec.rb

@@ -9,7 +9,8 @@ describe CookedPostProcessor do
     let(:cpp) { CookedPostProcessor.new(post) }
     let(:post_process) { sequence("post_process") }
 
-    it "works on images before oneboxes" do
+    it "post process in sequence" do
+      cpp.expects(:post_process_attachments).in_sequence(post_process)
       cpp.expects(:post_process_images).in_sequence(post_process)
       cpp.expects(:post_process_oneboxes).in_sequence(post_process)
       cpp.post_process
@@ -17,6 +18,35 @@ describe CookedPostProcessor do
 
   end
 
+  context "post_process_attachments" do
+
+    context "with attachment" do
+
+      let(:upload) { Fabricate(:upload) }
+      let(:post) { Fabricate(:post_with_an_attachment) }
+      let(:cpp) { CookedPostProcessor.new(post) }
+
+      # all in one test to speed things up
+      it "works" do
+        Upload.expects(:get_from_url).returns(upload)
+        cpp.post_process_attachments
+        # ensures absolute urls on attachment
+        cpp.html.should =~ /#{LocalStore.base_url}/
+        # ensure name is present
+        cpp.html.should =~ /archive.zip/
+        # ensure size is present
+        cpp.html.should =~ /<span class=\"size\">\(1.21 KB\)<\/span>/
+        # dirty
+        cpp.should be_dirty
+        # keeps the reverse index up to date
+        post.uploads.reload
+        post.uploads.count.should == 1
+      end
+
+    end
+
+  end
+
   context "post_process_images" do
 
     context "with images in quotes and oneboxes" do
@@ -48,7 +78,7 @@ describe CookedPostProcessor do
         Upload.expects(:get_from_url).returns(upload).twice
         cpp.post_process_images
         # ensures absolute urls on uploaded images
-        cpp.html.should =~ /#{Discourse.base_url_no_prefix}/
+        cpp.html.should =~ /#{LocalStore.base_url}/
         # dirty
         cpp.should be_dirty
         # keeps the reverse index up to date
@@ -58,7 +88,7 @@ describe CookedPostProcessor do
 
     end
 
-    context "width sized images" do
+    context "with sized images" do
 
       let(:post) { build(:post_with_image_url) }
       let(:cpp) { CookedPostProcessor.new(post, image_sizes: {'http://foo.bar/image.png' => {'width' => 111, 'height' => 222}}) }

spec/components/local_store_spec.rb

@@ -22,7 +22,7 @@ describe LocalStore 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))
       #
-      LocalStore.store_file(file, "", image_info, 1).should == '/uploads/default/1/253dc8edf9d4ada1.png'
+      LocalStore.store_file(file, "", 1).should == '/uploads/default/1/253dc8edf9d4ada1.png'
     end
 
   end

spec/components/s3_spec.rb

@@ -24,7 +24,7 @@ describe S3 do
     end
 
     it 'returns the url of the S3 upload if successful' do
-      S3.store_file(file, "SHA", image_info, 1).should == '//s3_upload_bucket.s3.amazonaws.com/1SHA.png'
+      S3.store_file(file, "SHA", 1).should == '//s3_upload_bucket.s3.amazonaws.com/1SHA.png'
     end
 
     after(:each) do

spec/controllers/uploads_controller_spec.rb

@@ -41,20 +41,39 @@ describe UploadsController do
       let(:files) { [ logo_dev, logo ] }
 
       context 'with a file' do
-        it 'is succesful' do
+
+        it 'is successful' do
           xhr :post, :create, file: logo
           response.should be_success
         end
 
-        it 'supports only images' do
+        context 'when authorized' do
-          xhr :post, :create, file: text_file
+
-          response.status.should eq 415
+          before { SiteSetting.stubs(:authorized_extensions).returns(".txt") }
+
+          it 'is successful' do
+            xhr :post, :create, file: text_file
+            response.status.should eq 200
+          end
+
         end
+
+        context 'when not authorized' do
+
+          before { SiteSetting.stubs(:authorized_extensions).returns(".png") }
+
+          it 'rejects the upload' do
+            xhr :post, :create, file: text_file
+            response.status.should eq 415
+          end
+
+        end
+
       end
 
       context 'with some files' do
 
-        it 'is succesful' do
+        it 'is successful' do
           xhr :post, :create, files: files
           response.should be_success
         end

spec/fabricators/post_fabricator.rb

@@ -50,6 +50,10 @@ Fabricator(:post_with_uploaded_images, from: :post) do
 '
 end
 
+Fabricator(:post_with_an_attachment, from: :post) do
+  cooked '<a class="attachment" href="/uploads/default/186/66b3ed1503efc936.zip">archive.zip</a>'
+end
+
 Fabricator(:post_with_unsized_images, from: :post) do
   cooked '
 <img src="http://foo.bar/image.png">

spec/fabricators/upload_fabricator.rb

@@ -6,3 +6,10 @@ Fabricator(:upload) do
   height 200
   url  "/uploads/default/1/1234567890123456.jpg"
 end
+
+Fabricator(:attachment, from: :upload) do
+  user
+  original_filename "archive.zip"
+  filesize 1234
+  url  "/uploads/default/186/66b3ed1503efc936.zip"
+end

spec/models/upload_spec.rb

@@ -62,13 +62,13 @@ describe Upload do
 
     it "identifies internal or relatives urls" do
       Discourse.expects(:base_url_no_prefix).returns("http://discuss.site.com")
-      Upload.has_been_uploaded?("http://discuss.site.com/upload/1234/42/0123456789ABCDEF.jpg").should == true
+      Upload.has_been_uploaded?("http://discuss.site.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
       Upload.has_been_uploaded?("/upload/42/0123456789ABCDEF.jpg").should == true
     end
 
     it "identifies internal urls when using a CDN" do
       ActionController::Base.expects(:asset_host).returns("http://my.cdn.com").twice
-      Upload.has_been_uploaded?("http://my.cdn.com/upload/1234/42/0123456789ABCDEF.jpg").should == true
+      Upload.has_been_uploaded?("http://my.cdn.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
     end
 
     it "identifies S3 uploads" do
@@ -78,7 +78,7 @@ describe Upload do
     end
 
     it "identifies external urls" do
-      Upload.has_been_uploaded?("http://domain.com/upload/1234/42/0123456789ABCDEF.jpg").should == false
+      Upload.has_been_uploaded?("http://domain.com/uploads/default/42/0123456789ABCDEF.jpg").should == false
       Upload.has_been_uploaded?("//bucket.s3.amazonaws.com/1337.png").should == false
     end
 

test/javascripts/components/click_track_test.js

@@ -20,6 +20,7 @@ module("Discourse.ClickTrack", {
       '      <a id="inside-onebox-forced" class="track-link" href="http://www.google.com">google.com<span class="badge">1</span></a>',
       '    </div>',
       '    <a id="same-site" href="http://discuss.domain.com">forum</a>',
+      '    <a class="attachment" href="http://discuss.domain.com/uploads/default/1234/1532357280.txt">log.txt</a>',
       '  </article>',
       '</div>'].join("\n"));
   },
@@ -156,6 +157,13 @@ test("tracks via AJAX if we're on the same site", function() {
   ok(Discourse.URL.routeTo.calledOnce);
 });
 
+test("does not track via AJAX for attachments", function() {
+  this.stub(Discourse.URL, "routeTo");
+  this.stub(Discourse.URL, "origin").returns("http://discuss.domain.com");
+
+  ok(!track(generateClickEventOn('.attachment')));
+  ok(Discourse.URL.redirectTo.calledOnce);
+});
 
 test("tracks custom urls when opening in another window", function() {
   var clickEvent = generateClickEventOn('a');

test/javascripts/components/utilities_test.js

@@ -7,7 +7,6 @@ test("emailValid", function() {
   ok(utils.emailValid('bob@EXAMPLE.com'), "allows upper case in the email domain");
 });
 
-
 var validUpload = utils.validateFilesForUpload;
 
 test("validateFilesForUpload", function() {
@@ -51,9 +50,9 @@ test("prevents files that are too big from being uploaded", function() {
 });
 
 var dummyBlob = function() {
-  window.BlobBuilder = window.BlobBuilder || window.WebKitBlobBuilder || window.MozBlobBuilder || window.MSBlobBuilder;
+  var BlobBuilder = window.BlobBuilder || window.WebKitBlobBuilder || window.MozBlobBuilder || window.MSBlobBuilder;
-  if (window.BlobBuilder) {
+  if (BlobBuilder) {
-    var bb = new window.BlobBuilder();
+    var bb = new BlobBuilder();
     bb.append([1]);
     return bb.getBlob("image/png");
   } else {
@@ -85,3 +84,28 @@ test("isAuthorizedUpload", function() {
   ok(!isAuthorized("image.txt"));
   ok(!isAuthorized(""));
 });
+
+var getUploadMarkdown = function(filename) {
+  return utils.getUploadMarkdown({
+    original_filename: filename,
+    width: 100,
+    height: 200,
+    url: "/upload/123/abcdef.ext"
+  });
+};
+
+test("getUploadMarkdown", function() {
+  ok(getUploadMarkdown("lolcat.gif") === '<img src="/upload/123/abcdef.ext" width="100" height="200">');
+  ok(getUploadMarkdown("important.txt") === '<a class="attachment" href="/upload/123/abcdef.ext">important.txt</a>');
+});
+
+test("isAnImage", function() {
+  _.each(["png", "jpg", "jpeg", "bmp", "gif", "tif"], function(extension) {
+    var image = "image." + extension;
+    ok(utils.isAnImage(image));
+    ok(utils.isAnImage("http://foo.bar/path/to/" + image));
+  });
+  ok(!utils.isAnImage("file.txt"));
+  ok(!utils.isAnImage("http://foo.bar/path/to/file.txt"));
+  ok(!utils.isAnImage(""));
+});