Generalize the image upload in markdown

This commit generalizes the image upload via drag and drop so it supports all files.
It also adds access control for these files.

CHANGELOG

 v 7.8.0 (unreleased)
   - Fix broken access control for note attachments (Hannes Rosenögger)
+  - Generalize image upload in drag and drop in markdown to all files (Hannes Rosenögger)
   - Replace highlight.js with rouge-fork rugments (Stefan Tatschner)
   - Make project search case insensitive (Hannes Rosenögger)
   - Include issue/mr participants in list of recipients for reassign/close/reopen emails

app/assets/javascripts/dropzone_input.js.coffee

@@ -9,7 +9,7 @@ class @DropzoneInput
     iconPicture = "<i class=\"fa fa-picture-o div-dropzone-icon\"></i>"
     iconSpinner = "<i class=\"fa fa-spinner fa-spin div-dropzone-icon\"></i>"
     btnAlert = "<button type=\"button\"" + alertAttr + ">&times;</button>"
-    project_image_path_upload = window.project_image_path_upload or null
+    project_file_path_upload = window.project_file_path_upload or null
 
     form_textarea = $(form).find("textarea.markdown-area")
     form_textarea.wrap "<div class=\"div-dropzone\"></div>"
@@ -72,13 +72,12 @@ class @DropzoneInput
       form.find(".md-preview-holder").hide()
 
     dropzone = form_dropzone.dropzone(
-      url: project_image_path_upload
+      url: project_file_path_upload
       dictDefaultMessage: ""
       clickable: true
-      paramName: "markdown_img"
+      paramName: "markdown_file"
       maxFilesize: 10
       uploadMultiple: false
-      acceptedFiles: "image/jpg,image/jpeg,image/gif,image/png"
       headers:
         "X-CSRF-Token": $("meta[name=\"csrf-token\"]").attr("content")
 
@@ -133,7 +132,10 @@ class @DropzoneInput
     child = $(dropzone[0]).children("textarea")
 
     formatLink = (str) ->
-      "![" + str.alt + "](" + str.url + ")"
+      text = "[" + str.alt + "](" + str.url + ")"
+      if str.is_image is true
+        text = "!" + text
+      text
 
     handlePaste = (event) ->
       pasteEvent = event.originalEvent
@@ -177,9 +179,9 @@ class @DropzoneInput
 
     uploadFile = (item, filename) ->
       formData = new FormData()
-      formData.append "markdown_img", item, filename
+      formData.append "markdown_file", item, filename
       $.ajax
-        url: project_image_path_upload
+        url: project_file_path_upload
         type: "POST"
         data: formData
         dataType: "json"
@@ -234,4 +236,7 @@ class @DropzoneInput
       return
 
   formatLink: (str) ->
-    "![" + str.alt + "](" + str.url + ")"
+    text = "[" + str.alt + "](" + str.url + ")"
+    if str.is_image is true
+      text = "!" + text
+    text

app/controllers/files_controller.rb

 class FilesController < ApplicationController
-  def download
+  def download_notes
     note = Note.find(params[:id])
     uploader = note.attachment
 
@@ -14,7 +14,32 @@ class FilesController < ApplicationController
         not_found!
       end
     else
-      redirect_to uploader.url
+      not_found!
+    end
+  end
+
+  def download_files
+    namespace_id = params[:namespace]
+    project_id = params[:project]
+    folder_id = params[:folder_id]
+    filename = params[:filename]
+    project_with_namespace="#{namespace_id}/#{project_id}"
+    filename_with_id="#{folder_id}/#{filename}"
+    
+    project = Project.find_with_namespace(project_with_namespace)
+
+    uploader = FileUploader.new("#{Rails.root}/uploads","#{project_with_namespace}/#{folder_id}")
+    uploader.retrieve_from_store!(filename)
+
+    if can?(current_user, :read_project, project)
+      download(uploader)
+    else
+      not_found!
     end
   end
+
+  def download(uploader)
+    disposition = uploader.image? ? 'inline' : 'attachment'
+    send_file uploader.file.path, disposition: disposition
+  end
 end

app/controllers/projects_controller.rb

@@ -134,12 +134,13 @@ class ProjectsController < ApplicationController
     end
   end
 
-  def upload_image
-    link_to_image = ::Projects::ImageService.new(repository, params, root_url).execute
+  def upload_file
+    link_to_file = ::Projects::FileService.new(repository, params, root_url).
+      execute
 
     respond_to do |format|
-      if link_to_image
-        format.json { render json: { link: link_to_image } }
+      if link_to_file
+        format.json { render json: { link: link_to_file } }
       else
         format.json { render json: 'Invalid file.', status: :unprocessable_entity }
       end
@@ -158,13 +159,8 @@ class ProjectsController < ApplicationController
 
   private
 
-  def upload_path
-    base_dir = FileUploader.generate_dir
-    File.join(repository.path_with_namespace, base_dir)
-  end
-
-  def accepted_images
-    %w(png jpg jpeg gif)
+  def invalid_file(error)
+    render json: { message: error.message }, status: :internal_server_error
   end
 
   def set_title

app/services/projects/image_service.rb → app/services/projects/file_service.rb

 module Projects
-  class ImageService < BaseService
+  class FileService < BaseService
     include Rails.application.routes.url_helpers
     def initialize(repository, params, root_url)
       @repository, @params, @root_url = repository, params.dup, root_url
     end
 
     def execute
-      uploader = FileUploader.new('uploads', upload_path, accepted_images)
-      image = @params['markdown_img']
+      uploader = FileUploader.new("#{Rails.root}/uploads", upload_path, accepted_files)
+      file = @params['markdown_file']
 
-      if image && correct_mime_type?(image)
-        alt = image.original_filename
-        uploader.store!(image)
+      if file
+        alt = file.original_filename
+        uploader.store!(file)
+        filename = nil
+        if image?(file)
+          filename=File.basename(alt, '.*')
+        else
+          filename=File.basename(alt)
+        end
         link = {
-                 'alt' => File.basename(alt, '.*'),
-                 'url' => File.join(@root_url, uploader.url)
+          'alt' => filename,
+          'url' => uploader.secure_url,
+          'is_image' => image?(file)
         }
       else
         link = nil
@@ -23,17 +30,26 @@ module Projects
 
     protected
 
-    def upload_path
-      base_dir = FileUploader.generate_dir
-      File.join(@repository.path_with_namespace, base_dir)
+    def accepted_files
+      # insert accepted mime types here (e.g %w(jpg jpeg gif png))
+      nil
     end
 
     def accepted_images
-      %w(png jpg jpeg gif)
+      %w(jpg jpeg gif png)
+    end
+
+    def image?(file)
+      accepted_images.map { |format| file.content_type.include? format }.any?
+    end
+
+    def upload_path
+      base_dir = FileUploader.generate_dir
+      File.join(@repository.path_with_namespace, base_dir)
     end
 
-    def correct_mime_type?(image)
-      accepted_images.map{ |format| image.content_type.include? format }.any?
+    def correct_mime_type?(file)
+      accepted_files.map { |format| image.content_type.include? format }.any?
     end
   end
 end

app/uploaders/file_uploader.rb

@@ -21,7 +21,7 @@ class FileUploader < CarrierWave::Uploader::Base
   end
 
   def extension_white_list
-    @allowed_extensions
+    @allowed_extensions || super
   end
 
   def store!(file)
@@ -38,4 +38,21 @@ class FileUploader < CarrierWave::Uploader::Base
   def self.generate_dir
     SecureRandom.hex(5)
   end
+
+  def secure_url
+    Gitlab.config.gitlab.relative_url_root + "/files/#{@path}/#{@filename}"
+  end
+
+  def image?
+    img_ext = %w(png jpg jpeg gif bmp tiff)
+    if file.respond_to?(:extension)
+      img_ext.include?(file.extension.downcase)
+    else
+      # Not all CarrierWave storages respond to :extension
+      ext = file.path.split('.').last.downcase
+      img_ext.include?(ext)
+    end
+  rescue
+    false
+  end
 end

app/views/projects/_issuable_form.html.haml

@@ -23,7 +23,7 @@
           Parsed with
           #{link_to 'GitLab Flavored Markdown', help_page_path('markdown', 'markdown'), target: '_blank'}.
         .pull-right
-          Attach images (JPG, PNG, GIF) by dragging & dropping
+          Attach files by dragging & dropping
           or #{link_to 'selecting them', '#', class: 'markdown-selector' }.
 
       .clearfix

app/views/projects/issues/_form.html.haml

@@ -11,4 +11,4 @@
     e.preventDefault();
   });
 
-  window.project_image_path_upload = "#{upload_image_project_path @project}";
+  window.project_file_path_upload = "#{upload_file_project_path @project}";

app/views/projects/merge_requests/_form.html.haml

@@ -9,4 +9,4 @@
     e.preventDefault();
   });
 
-  window.project_image_path_upload = "#{upload_image_project_path @project}";
+  window.project_file_path_upload = "#{upload_file_project_path @project}";

app/views/projects/merge_requests/_new_submit.html.haml

@@ -27,7 +27,7 @@
               Parsed with
               #{link_to 'Gitlab Flavored Markdown', help_page_path('markdown', 'markdown'), target: '_blank'}.
             .pull-right
-              Attach images (JPG, PNG, GIF) by dragging & dropping
+              Attach files by dragging & dropping
               or #{link_to 'selecting them', '#', class: 'markdown-selector'}.
 
           .clearfix
@@ -113,10 +113,11 @@
     e.preventDefault();
   });
 
-  window.project_image_path_upload = "#{upload_image_project_path @project}";
+  window.project_file_path_upload = "#{upload_file_project_path @project}";
 
 :javascript
   var merge_request
   merge_request = new MergeRequest({
     action: 'commits'
   });
+

app/views/projects/milestones/_form.html.haml

@@ -25,7 +25,7 @@
             = render 'projects/zen', f: f, attr: :description, classes: 'description form-control'
             .hint
               .pull-left Milestones are parsed with #{link_to "GitLab Flavored Markdown", help_page_path("markdown", "markdown"), target: '_blank'}.
-              .pull-left Attach images (JPG, PNG, GIF) by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector' }.
+              .pull-left Attach files by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector' }.
           .clearfix
           .error-alert
     .col-md-6
@@ -51,4 +51,4 @@
     onSelect: function(dateText, inst) { $("#milestone_due_date").val(dateText) }
   }).datepicker("setDate", $.datepicker.parseDate('yy-mm-dd', $('#milestone_due_date').val()));
 
-  window.project_image_path_upload = "#{upload_image_project_path @project}";
+  window.project_file_path_upload = "#{upload_file_project_path @project}";

app/views/projects/notes/_edit_form.html.haml

@@ -6,7 +6,7 @@
 
       .comment-hints.clearfix
         .pull-left Comments are parsed with #{link_to "GitLab Flavored Markdown", help_page_path("markdown", "markdown"),{ target: '_blank', tabindex: -1 }}
-        .pull-right Attach images (JPG, PNG, GIF) by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector', tabindex: -1 }.
+        .pull-right Attach files by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector', tabindex: -1 }.
 
     .note-form-actions
       .buttons

app/views/projects/notes/_form.html.haml

-= form_for [@project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new_note js-new-note-form common-note-form gfm-form" }, authenticity_token: true do |f|
+= form_for [@project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new_note js-new-note-form common-note-form" }, authenticity_token: true do |f|
   = note_target_fields
   = f.hidden_field :commit_id
   = f.hidden_field :line_code
@@ -11,7 +11,7 @@
 
     .comment-hints.clearfix
       .pull-left Comments are parsed with #{link_to "GitLab Flavored Markdown", help_page_path("markdown", "markdown"),{ target: '_blank', tabindex: -1 }}
-      .pull-right Attach images (JPG, PNG, GIF) by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector', tabindex: -1 }.
+      .pull-right Attach files by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector', tabindex: -1 }.
 
 
   .note-form-actions
@@ -29,4 +29,4 @@
       = f.file_field :attachment, class: "js-note-attachment-input hidden"
 
 :javascript
-  window.project_image_path_upload = "#{upload_image_project_path @project}";
+  window.project_file_path_upload = "#{upload_file_project_path @project}";

app/views/projects/wikis/_form.html.haml

@@ -26,7 +26,7 @@
         = render 'projects/zen', f: f, attr: :content, classes: 'description form-control'
         .col-sm-12.hint
           .pull-left Wiki content is parsed with #{link_to "GitLab Flavored Markdown", help_page_path("markdown", "markdown"), target: '_blank'}
-          .pull-right Attach images (JPG, PNG, GIF) by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector' }.
+          .pull-right Attach files by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector' }.
 
       .clearfix
       .error-alert
@@ -43,5 +43,6 @@
       = link_to "Cancel", project_wiki_path(@project, :home), class: "btn btn-cancel"
 
 :javascript
-  window.project_image_path_upload = "#{upload_image_project_path @project}";
+  window.project_file_path_upload = "#{upload_file_project_path @project}";
+
 

config/routes.rb

@@ -93,7 +93,8 @@ Gitlab::Application.routes.draw do
   #
   # Attachments serving
   #
-  get 'files/:type/:id/:filename' => 'files#download', constraints: { id: /\d+/, type: /[a-z]+/, filename:  /.+/ }
+  get 'files/:type/:id/:filename' => 'files#download_notes', constraints: { id: /\d+/, type: /[a-z]+/, filename:  /.+/ }
+  get 'files/:namespace/:project/:folder_id/:filename' => 'files#download_files', constraints: { namespace: /[^\/]+/, project: /[a-zA-Z.\/0-9_\-]+/, filename:  /.+/ }
 
   #
   # Admin Area
@@ -220,7 +221,7 @@ Gitlab::Application.routes.draw do
       put :transfer
       post :archive
       post :unarchive
-      post :upload_image
+      post :upload_file
       post :toggle_star
       post :markdown_preview
       get :autocomplete_sources

spec/controllers/projects_controller_spec.rb

@@ -7,43 +7,49 @@ describe ProjectsController do
   let(:jpg)     { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
   let(:txt)     { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
 
-  describe "POST #upload_image" do
+  describe 'POST #upload_file' do
     before do
       sign_in(user)
       project.team << [user, :developer]
     end
 
-    context "without params['markdown_img']" do
-      it "returns an error" do
-        post :upload_image, id: project.to_param, format: :json
+    context "without params['markdown_file']" do
+      it 'returns an error' do
+        post :upload_file, id: project.to_param, format: :json
         expect(response.status).to eq(422)
       end
     end
 
-    context "with invalid file" do
+    context 'with valid image' do
       before do
-        post :upload_image, id: project.to_param, markdown_img: txt, format: :json
+        post :upload_file,
+          id: project.to_param,
+          markdown_file: jpg,
+          format: :json
       end
 
-      it "returns an error" do
-        expect(response.status).to eq(422)
+      it 'returns a content with original filename, new link, and correct type.' do
+        expect(response.body).to match '\"alt\":\"rails_sample\"'
+        expect(response.body).to match "\"url\":\"http://test.host/uploads/#{project.path_with_namespace}"
+        expect(response.body).to match '\"is_image\":true'
       end
     end
 
-    context "with valid file" do
+    context 'with valid non-image file' do
       before do
-        post :upload_image, id: project.to_param, markdown_img: jpg, format: :json
+        post :upload_file, id: project.to_param, markdown_file: txt, format: :json
       end
 
-      it "returns a content with original filename and new link." do
-        expect(response.body).to match "\"alt\":\"rails_sample\""
+      it 'returns a content with original filename, new link, and correct type.' do
+        expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
         expect(response.body).to match "\"url\":\"http://test.host/uploads/#{project.path_with_namespace}"
+        expect(response.body).to match '\"is_image\":false'
       end
     end
   end
 
-  describe "POST #toggle_star" do
-    it "toggles star if user is signed in" do
+  describe 'POST #toggle_star' do
+    it 'toggles star if user is signed in' do
       sign_in(user)
       expect(user.starred?(public_project)).to be_falsey
       post :toggle_star, id: public_project.to_param
@@ -52,7 +58,7 @@ describe ProjectsController do
       expect(user.starred?(public_project)).to be_falsey
     end
 
-    it "does nothing if user is not signed in" do
+    it 'does nothing if user is not signed in' do
       post :toggle_star, id: public_project.to_param
       expect(user.starred?(public_project)).to be_falsey
       post :toggle_star, id: public_project.to_param

spec/services/projects/file_service_spec.rb

+require 'spec_helper'
+
+describe Projects::FileService do
+  describe 'File service' do
+    before do
+      @user = create :user
+      @project = create :project, creator_id: @user.id, namespace: @user.namespace
+    end
+
+    context 'for valid gif file' do
+      before do
+        gif = fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif')
+        @link_to_file = upload_file(@project.repository,
+          { 'markdown_file' => gif },
+          'http://test.example/')
+      end
+
+      it { expect(@link_to_file).to have_key('alt') }
+      it { expect(@link_to_file).to have_key('url') }
+      it { expect(@link_to_file).to have_key('is_image') }
+      it { expect(@link_to_file).to have_value('banana_sample') }
+      it { expect(@link_to_file['is_image']).to equal(true) }
+      it { expect(@link_to_file['url']).to match("http://test.example/uploads/#{@project.path_with_namespace}") }
+      it { expect(@link_to_file['url']).to match('banana_sample.gif') }
+    end
+
+     context 'for valid png file' do
+      before do
+        png = fixture_file_upload(Rails.root + 'spec/fixtures/dk.png',
+          'image/png')
+        @link_to_file = upload_file(@project.repository,
+          { 'markdown_file' => png },
+          'http://test.example/')
+      end
+
+      it { expect(@link_to_file).to have_key('alt') }
+      it { expect(@link_to_file).to have_key('url') }
+      it { expect(@link_to_file).to have_value('dk') }
+      it { expect(@link_to_file).to have_key('is_image') }
+      it { expect(@link_to_file['is_image']).to equal(true) }
+      it { expect(@link_to_file['url']).to match("http://test.example/uploads/#{@project.path_with_namespace}") }
+      it { expect(@link_to_file['url']).to match('dk.png') }
+    end
+
+     context 'for valid jpg file' do
+      before do
+        jpg = fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg')
+        @link_to_file = upload_file(@project.repository, { 'markdown_file' => jpg }, 'http://test.example/')
+      end
+
+      it { expect(@link_to_file).to have_key('alt') }
+      it { expect(@link_to_file).to have_key('url') }
+      it { expect(@link_to_file).to have_key('is_image') }
+      it { expect(@link_to_file).to have_value('rails_sample') }
+      it { expect(@link_to_file['is_image']).to equal(true) }
+      it { expect(@link_to_file['url']).to match("http://test.example/uploads/#{@project.path_with_namespace}") }
+      it { expect(@link_to_file['url']).to match('rails_sample.jpg') }
+    end
+
+    context 'for txt file' do
+      before do
+        txt = fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain')
+        @link_to_file = upload_file(@project.repository,
+          { 'markdown_file' => txt },
+          'http://test.example/')
+      end
+
+      it { expect(@link_to_file).to have_key('alt') }
+      it { expect(@link_to_file).to have_key('url') }
+      it { expect(@link_to_file).to have_key('is_image') }
+      it { expect(@link_to_file).to have_value('doc_sample.txt') }
+      it { expect(@link_to_file['is_image']).to equal(false) }
+      it { expect(@link_to_file['url']).to match("http://test.example/uploads/#{@project.path_with_namespace}") }
+      it { expect(@link_to_file['url']).to match('doc_sample.txt') }
+    end
+  end
+
+  def upload_file(repository, params, root_url)
+    Projects::FileService.new(repository, params, root_url).execute
+  end
+end

spec/services/projects/image_service_spec.rb

-require 'spec_helper'
-
-describe Projects::ImageService do
-  describe 'Image service' do
-    before do
-      @user = create :user
-      @project = create :project, creator_id: @user.id, namespace: @user.namespace
-    end
-
-    context 'for valid gif file' do
-      before do
-        gif = fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif')
-        @link_to_image = upload_image(@project.repository, { 'markdown_img' => gif }, "http://test.example/")
-      end
-
-      it { expect(@link_to_image).to have_key("alt") }
-      it { expect(@link_to_image).to have_key("url") }
-      it { expect(@link_to_image).to have_value("banana_sample") }
-      it { expect(@link_to_image["url"]).to match("http://test.example/uploads/#{@project.path_with_namespace}") }
-      it { expect(@link_to_image["url"]).to match("banana_sample.gif") }
-    end
-
-     context 'for valid png file' do
-      before do
-        png = fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/png')
-        @link_to_image = upload_image(@project.repository, { 'markdown_img' => png }, "http://test.example/")
-      end
-
-      it { expect(@link_to_image).to have_key("alt") }
-      it { expect(@link_to_image).to have_key("url") }
-      it { expect(@link_to_image).to have_value("dk") }
-      it { expect(@link_to_image["url"]).to match("http://test.example/uploads/#{@project.path_with_namespace}") }
-      it { expect(@link_to_image["url"]).to match("dk.png") }
-    end
-
-     context 'for valid jpg file' do
-      before do
-        jpg = fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg')
-        @link_to_image = upload_image(@project.repository, { 'markdown_img' => jpg }, "http://test.example/")
-      end
-
-      it { expect(@link_to_image).to have_key("alt") }
-      it { expect(@link_to_image).to have_key("url") }
-      it { expect(@link_to_image).to have_value("rails_sample") }
-      it { expect(@link_to_image["url"]).to match("http://test.example/uploads/#{@project.path_with_namespace}") }
-      it { expect(@link_to_image["url"]).to match("rails_sample.jpg") }
-    end
-
-    context 'for txt file' do
-      before do
-        txt = fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain')
-        @link_to_image = upload_image(@project.repository, { 'markdown_img' => txt }, "http://test.example/")
-      end
-
-      it { expect(@link_to_image).to be_nil }
-    end
-  end
-
-  def upload_image(repository, params, root_url)
-    Projects::ImageService.new(repository, params, root_url).execute
-  end
-end