From 9bf8480b4a0d3ea6e284c4bd8bf26243f3f3f6f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20Rosen=C3=B6gger?= <123haynes@gmail.com> Date: Sat, 14 Feb 2015 16:04:45 +0100 Subject: [PATCH 01/16] 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 | 1 + .../javascripts/dropzone_input.js.coffee | 21 +++-- app/controllers/files_controller.rb | 29 ++++++- app/controllers/projects_controller.rb | 18 ++--- app/services/projects/file_service.rb | 55 +++++++++++++ app/services/projects/image_service.rb | 39 --------- app/uploaders/file_uploader.rb | 19 ++++- app/views/projects/_issuable_form.html.haml | 2 +- app/views/projects/issues/_form.html.haml | 2 +- .../projects/merge_requests/_form.html.haml | 2 +- .../merge_requests/_new_submit.html.haml | 5 +- app/views/projects/milestones/_form.html.haml | 4 +- app/views/projects/notes/_edit_form.html.haml | 2 +- app/views/projects/notes/_form.html.haml | 6 +- app/views/projects/wikis/_form.html.haml | 5 +- config/routes.rb | 5 +- spec/controllers/projects_controller_spec.rb | 48 ++++++----- spec/services/projects/file_service_spec.rb | 81 +++++++++++++++++++ spec/services/projects/image_service_spec.rb | 62 -------------- 19 files changed, 247 insertions(+), 159 deletions(-) create mode 100644 app/services/projects/file_service.rb delete mode 100644 app/services/projects/image_service.rb create mode 100644 spec/services/projects/file_service_spec.rb delete mode 100644 spec/services/projects/image_service_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 05d1e7bdb40..9e50178d8b7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,6 @@ 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 diff --git a/app/assets/javascripts/dropzone_input.js.coffee b/app/assets/javascripts/dropzone_input.js.coffee index d98d5482937..bed8471b39a 100644 --- a/app/assets/javascripts/dropzone_input.js.coffee +++ b/app/assets/javascripts/dropzone_input.js.coffee @@ -9,7 +9,7 @@ class @DropzoneInput iconPicture = "" iconSpinner = "" btnAlert = "" - 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 "
" @@ -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 diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb index 15523cbc2e7..a86340dd9bb 100644 --- a/app/controllers/files_controller.rb +++ b/app/controllers/files_controller.rb @@ -1,5 +1,5 @@ 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 diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 462ab3d4749..b430278903a 100644 --- a/app/controllers/projects_controller.rb +++ b/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 diff --git a/app/services/projects/file_service.rb b/app/services/projects/file_service.rb new file mode 100644 index 00000000000..8c149bf53a1 --- /dev/null +++ b/app/services/projects/file_service.rb @@ -0,0 +1,55 @@ +module Projects + 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("#{Rails.root}/uploads", upload_path, accepted_files) + file = @params['markdown_file'] + + 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' => filename, + 'url' => uploader.secure_url, + 'is_image' => image?(file) + } + else + link = nil + end + end + + protected + + def accepted_files + # insert accepted mime types here (e.g %w(jpg jpeg gif png)) + nil + end + + def accepted_images + %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?(file) + accepted_files.map { |format| image.content_type.include? format }.any? + end + end +end diff --git a/app/services/projects/image_service.rb b/app/services/projects/image_service.rb deleted file mode 100644 index 7ca7e82c4a3..00000000000 --- a/app/services/projects/image_service.rb +++ /dev/null @@ -1,39 +0,0 @@ -module Projects - class ImageService < 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'] - - if image && correct_mime_type?(image) - alt = image.original_filename - uploader.store!(image) - link = { - 'alt' => File.basename(alt, '.*'), - 'url' => File.join(@root_url, uploader.url) - } - else - link = nil - end - end - - protected - - 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) - end - - def correct_mime_type?(image) - accepted_images.map{ |format| image.content_type.include? format }.any? - end - end -end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 0fa987c93f6..ac7bd5b27ec 100644 --- a/app/uploaders/file_uploader.rb +++ b/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 diff --git a/app/views/projects/_issuable_form.html.haml b/app/views/projects/_issuable_form.html.haml index 5a57673b584..18897b055aa 100644 --- a/app/views/projects/_issuable_form.html.haml +++ b/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 diff --git a/app/views/projects/issues/_form.html.haml b/app/views/projects/issues/_form.html.haml index 2a7b44955cd..975980bd6bd 100644 --- a/app/views/projects/issues/_form.html.haml +++ b/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}"; diff --git a/app/views/projects/merge_requests/_form.html.haml b/app/views/projects/merge_requests/_form.html.haml index d52e64666a0..28c4734e14f 100644 --- a/app/views/projects/merge_requests/_form.html.haml +++ b/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}"; diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index bca3e45bf19..0653b30fcca 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/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' }); + diff --git a/app/views/projects/milestones/_form.html.haml b/app/views/projects/milestones/_form.html.haml index b3b170d7114..5fbb668570c 100644 --- a/app/views/projects/milestones/_form.html.haml +++ b/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}"; diff --git a/app/views/projects/notes/_edit_form.html.haml b/app/views/projects/notes/_edit_form.html.haml index cdc76f5d96f..4ba59078318 100644 --- a/app/views/projects/notes/_edit_form.html.haml +++ b/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 diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index 1a4e06289f8..fe3dab569f4 100644 --- a/app/views/projects/notes/_form.html.haml +++ b/app/views/projects/notes/_form.html.haml @@ -1,4 +1,4 @@ -= 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}"; diff --git a/app/views/projects/wikis/_form.html.haml b/app/views/projects/wikis/_form.html.haml index 84731e43e95..0afee138c8b 100644 --- a/app/views/projects/wikis/_form.html.haml +++ b/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}"; + diff --git a/config/routes.rb b/config/routes.rb index 65786d83566..bd659ddeabd 100644 --- a/config/routes.rb +++ b/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 diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index ef786ccd324..039751a41e8 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/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) - end - end - - context "with valid file" do - before do - post :upload_image, id: project.to_param, markdown_img: jpg, 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\":\"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 non-image file' do + before do + post :upload_file, id: project.to_param, markdown_file: txt, format: :json + end + + 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 diff --git a/spec/services/projects/file_service_spec.rb b/spec/services/projects/file_service_spec.rb new file mode 100644 index 00000000000..38ab4a467bb --- /dev/null +++ b/spec/services/projects/file_service_spec.rb @@ -0,0 +1,81 @@ +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 diff --git a/spec/services/projects/image_service_spec.rb b/spec/services/projects/image_service_spec.rb deleted file mode 100644 index 23c4e227ae3..00000000000 --- a/spec/services/projects/image_service_spec.rb +++ /dev/null @@ -1,62 +0,0 @@ -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 From ca504a77fe994da893cd0632de5e0e7ea5b729fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20Rosen=C3=B6gger?= <123haynes@gmail.com> Date: Sat, 14 Feb 2015 16:45:22 +0100 Subject: [PATCH 02/16] Fix tests --- spec/services/projects/file_service_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/services/projects/file_service_spec.rb b/spec/services/projects/file_service_spec.rb index 38ab4a467bb..e2d6766735e 100644 --- a/spec/services/projects/file_service_spec.rb +++ b/spec/services/projects/file_service_spec.rb @@ -20,7 +20,7 @@ describe Projects::FileService do 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("/files/#{@project.path_with_namespace}") } it { expect(@link_to_file['url']).to match('banana_sample.gif') } end @@ -38,7 +38,7 @@ describe Projects::FileService do 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("/files/#{@project.path_with_namespace}") } it { expect(@link_to_file['url']).to match('dk.png') } end @@ -53,7 +53,7 @@ describe Projects::FileService do 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("/files/#{@project.path_with_namespace}") } it { expect(@link_to_file['url']).to match('rails_sample.jpg') } end @@ -70,7 +70,7 @@ describe Projects::FileService do 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("/files/#{@project.path_with_namespace}") } it { expect(@link_to_file['url']).to match('doc_sample.txt') } end end From 9729cc584f5758395960416f308a9c45f698cdee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20Rosen=C3=B6gger?= <123haynes@gmail.com> Date: Sat, 14 Feb 2015 19:52:45 +0100 Subject: [PATCH 03/16] implement Project::UploadsController --- app/controllers/files_controller.rb | 29 ++----------------- .../projects/uploads_controller.rb | 16 ++++++++++ app/uploaders/file_uploader.rb | 4 ++- config/routes.rb | 5 ++-- spec/services/projects/file_service_spec.rb | 8 ++--- 5 files changed, 28 insertions(+), 34 deletions(-) create mode 100644 app/controllers/projects/uploads_controller.rb diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb index a86340dd9bb..15523cbc2e7 100644 --- a/app/controllers/files_controller.rb +++ b/app/controllers/files_controller.rb @@ -1,5 +1,5 @@ class FilesController < ApplicationController - def download_notes + def download note = Note.find(params[:id]) uploader = note.attachment @@ -14,32 +14,7 @@ class FilesController < ApplicationController not_found! end else - not_found! + redirect_to uploader.url 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 diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb new file mode 100644 index 00000000000..1c9fb1c86fb --- /dev/null +++ b/app/controllers/projects/uploads_controller.rb @@ -0,0 +1,16 @@ +class Projects::UploadsController < Projects::ApplicationController + layout 'project' + + before_filter :project + + def show + folder_id = params[:folder_id] + filename = params[:filename] + + uploader = FileUploader.new("#{Rails.root}/uploads","#{@project.path_with_namespace}/#{folder_id}") + uploader.retrieve_from_store!(filename) + + disposition = uploader.image? ? 'inline' : 'attachment' + send_file uploader.file.path, disposition: disposition + end +end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index ac7bd5b27ec..51ae8040e52 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -40,7 +40,9 @@ class FileUploader < CarrierWave::Uploader::Base end def secure_url - Gitlab.config.gitlab.relative_url_root + "/files/#{@path}/#{@filename}" + path_array = @path.split('/') + path = File.join(path_array[0],path_array[1],'uploads',path_array[2]) + Gitlab.config.gitlab.relative_url_root + "/#{path}/#{@filename}" end def image? diff --git a/config/routes.rb b/config/routes.rb index bd659ddeabd..d29ad8db639 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -93,8 +93,7 @@ Gitlab::Application.routes.draw do # # Attachments serving # - 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: /.+/ } + get 'files/:type/:id/:filename' => 'files#download', constraints: { id: /\d+/, type: /[a-z]+/, filename: /.+/ } # # Admin Area @@ -257,6 +256,8 @@ Gitlab::Application.routes.draw do end end + get '/uploads/:folder_id/:filename' => 'uploads#show', constraints: { filename: /.+/ } + get '/compare/:from...:to' => 'compare#show', :as => 'compare', :constraints => { from: /.+/, to: /.+/ } diff --git a/spec/services/projects/file_service_spec.rb b/spec/services/projects/file_service_spec.rb index e2d6766735e..7bbe5b575c9 100644 --- a/spec/services/projects/file_service_spec.rb +++ b/spec/services/projects/file_service_spec.rb @@ -20,7 +20,7 @@ describe Projects::FileService do 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("/files/#{@project.path_with_namespace}") } + it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") } it { expect(@link_to_file['url']).to match('banana_sample.gif') } end @@ -38,7 +38,7 @@ describe Projects::FileService do 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("/files/#{@project.path_with_namespace}") } + it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") } it { expect(@link_to_file['url']).to match('dk.png') } end @@ -53,7 +53,7 @@ describe Projects::FileService do 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("/files/#{@project.path_with_namespace}") } + it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") } it { expect(@link_to_file['url']).to match('rails_sample.jpg') } end @@ -70,7 +70,7 @@ describe Projects::FileService do 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("/files/#{@project.path_with_namespace}") } + it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") } it { expect(@link_to_file['url']).to match('doc_sample.txt') } end end From 192e7306626e073a5e6fb3b41d69f0e3fddb0821 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20Rosen=C3=B6gger?= <123haynes@gmail.com> Date: Sun, 15 Feb 2015 18:48:32 +0100 Subject: [PATCH 04/16] Fix tests --- spec/controllers/projects_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 039751a41e8..2d52e3fd913 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -30,7 +30,7 @@ describe ProjectsController do 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 "\"url\":\"/#{project.path_with_namespace}/uploads" expect(response.body).to match '\"is_image\":true' end end @@ -42,7 +42,7 @@ describe ProjectsController do 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 "\"url\":\"/#{project.path_with_namespace}/uploads" expect(response.body).to match '\"is_image\":false' end end From d2ebdf664b42d4fac6b2e060ef79aa9fe0b0e72d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 16 Feb 2015 19:58:40 +0100 Subject: [PATCH 05/16] Refactor. --- .gitignore | 1 + Gemfile | 1 + Gemfile.lock | 6 ++ .../javascripts/dropzone_input.js.coffee | 26 ++++----- app/controllers/files_controller.rb | 17 +++--- .../projects/uploads_controller.rb | 39 ++++++++++--- app/controllers/projects_controller.rb | 17 ------ app/services/projects/file_service.rb | 55 ------------------- app/services/projects/upload_service.rb | 22 ++++++++ app/uploaders/attachment_uploader.rb | 2 +- app/uploaders/file_uploader.rb | 38 ++++--------- app/views/projects/issues/_form.html.haml | 2 +- .../projects/merge_requests/_form.html.haml | 2 +- .../merge_requests/_new_submit.html.haml | 2 +- app/views/projects/milestones/_form.html.haml | 2 +- app/views/projects/notes/_form.html.haml | 2 +- app/views/projects/wikis/_form.html.haml | 2 +- config/routes.rb | 7 ++- db/schema.rb | 1 - .../projects/uploads_controller_spec.rb | 49 +++++++++++++++++ spec/controllers/projects_controller_spec.rb | 45 +-------------- ...service_spec.rb => upload_service_spec.rb} | 20 +++---- 22 files changed, 164 insertions(+), 194 deletions(-) delete mode 100644 app/services/projects/file_service.rb create mode 100644 app/services/projects/upload_service.rb create mode 100644 spec/controllers/projects/uploads_controller_spec.rb rename spec/services/projects/{file_service_spec.rb => upload_service_spec.rb} (80%) diff --git a/.gitignore b/.gitignore index 7a7b5c93936..89fe301ee8f 100644 --- a/.gitignore +++ b/.gitignore @@ -36,6 +36,7 @@ nohup.out public/assets/ public/uploads.* public/uploads/ +uploads/ rails_best_practices_output.html tags tmp/ diff --git a/Gemfile b/Gemfile index c3d8299e944..3f0eae8ef42 100644 --- a/Gemfile +++ b/Gemfile @@ -205,6 +205,7 @@ group :development do gem "letter_opener" gem 'quiet_assets', '~> 1.0.1' gem 'rack-mini-profiler', require: false + gem "byebug" # Better errors handler gem 'better_errors' diff --git a/Gemfile.lock b/Gemfile.lock index 3283da40f8d..2f5ebf80b12 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -61,6 +61,9 @@ GEM sass (~> 3.2) browser (0.7.2) builder (3.2.2) + byebug (3.2.0) + columnize (~> 0.8) + debugger-linecache (~> 1.2) cal-heatmap-rails (0.0.1) capybara (2.2.1) mime-types (>= 1.16) @@ -88,6 +91,7 @@ GEM coffee-script-source (1.6.3) colored (1.2) colorize (0.5.8) + columnize (0.9.0) connection_pool (2.1.0) coveralls (0.7.0) multi_json (~> 1.3) @@ -103,6 +107,7 @@ GEM daemons (1.1.9) database_cleaner (1.3.0) debug_inspector (0.0.2) + debugger-linecache (1.2.0) default_value_for (3.0.0) activerecord (>= 3.2.0, < 5.0) descendants_tracker (0.0.3) @@ -646,6 +651,7 @@ DEPENDENCIES binding_of_caller bootstrap-sass (~> 3.0) browser + byebug cal-heatmap-rails (~> 0.0.1) capybara (~> 2.2.1) carrierwave diff --git a/app/assets/javascripts/dropzone_input.js.coffee b/app/assets/javascripts/dropzone_input.js.coffee index bed8471b39a..2d9b496b132 100644 --- a/app/assets/javascripts/dropzone_input.js.coffee +++ b/app/assets/javascripts/dropzone_input.js.coffee @@ -9,7 +9,7 @@ class @DropzoneInput iconPicture = "" iconSpinner = "" btnAlert = "" - project_file_path_upload = window.project_file_path_upload or null + project_uploads_path = window.project_uploads_path or null form_textarea = $(form).find("textarea.markdown-area") form_textarea.wrap "
" @@ -72,10 +72,10 @@ class @DropzoneInput form.find(".md-preview-holder").hide() dropzone = form_dropzone.dropzone( - url: project_file_path_upload + url: project_uploads_path dictDefaultMessage: "" clickable: true - paramName: "markdown_file" + paramName: "file" maxFilesize: 10 uploadMultiple: false headers: @@ -131,10 +131,9 @@ class @DropzoneInput child = $(dropzone[0]).children("textarea") - formatLink = (str) -> - text = "[" + str.alt + "](" + str.url + ")" - if str.is_image is true - text = "!" + text + formatLink = (link) -> + text = "[#{link.alt}](#{link.url})" + text = "!#{text}" if link.is_image text handlePaste = (event) -> @@ -179,9 +178,9 @@ class @DropzoneInput uploadFile = (item, filename) -> formData = new FormData() - formData.append "markdown_file", item, filename + formData.append "file", item, filename $.ajax - url: project_file_path_upload + url: project_uploads_path type: "POST" data: formData dataType: "json" @@ -235,8 +234,7 @@ class @DropzoneInput $(@).closest('.gfm-form').find('.div-dropzone').click() return - formatLink: (str) -> - text = "[" + str.alt + "](" + str.url + ")" - if str.is_image is true - text = "!" + text - text + formatLink: (link) -> + text = "[#{link.alt}](#{link.url})" + text = "!#{text}" if link.is_image + text \ No newline at end of file diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb index 15523cbc2e7..267239b7b84 100644 --- a/app/controllers/files_controller.rb +++ b/app/controllers/files_controller.rb @@ -3,18 +3,21 @@ class FilesController < ApplicationController note = Note.find(params[:id]) uploader = note.attachment - if uploader.file_storage? - if can?(current_user, :read_project, note.project) - # Replace old notes location in /public with the new one in / and send the file + if can?(current_user, :read_project, note.project) + if uploader.file_storage? path = uploader.file.path.gsub("#{Rails.root}/public", Rails.root.to_s) - disposition = uploader.image? ? 'inline' : 'attachment' - send_file path, disposition: disposition + if File.exist?(path) + disposition = uploader.image? ? 'inline' : 'attachment' + send_file path, disposition: disposition + else + not_found! + end else - not_found! + redirect_to uploader.url end else - redirect_to uploader.url + not_found! end end end diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index 1c9fb1c86fb..355163ac879 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -3,14 +3,37 @@ class Projects::UploadsController < Projects::ApplicationController before_filter :project - def show - folder_id = params[:folder_id] - filename = params[:filename] - - uploader = FileUploader.new("#{Rails.root}/uploads","#{@project.path_with_namespace}/#{folder_id}") - uploader.retrieve_from_store!(filename) + def create + link_to_file = ::Projects::UploadService.new(repository, params[:file]). + execute - disposition = uploader.image? ? 'inline' : 'attachment' - send_file uploader.file.path, disposition: disposition + respond_to do |format| + if link_to_file + format.json do + render json: { link: link_to_file } + end + else + format.json do + render json: 'Invalid file.', status: :unprocessable_entity + end + end + end + end + + def show + uploader = FileUploader.new(project, params[:secret]) + + if uploader.file_storage? + uploader.retrieve_from_store!(params[:filename]) + + if uploader.file.exists? + disposition = uploader.image? ? 'inline' : 'attachment' + send_file uploader.file.path, disposition: disposition + else + not_found! + end + else + redirect_to uploader.url + end end end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index b430278903a..9be66b6b9f9 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -134,19 +134,6 @@ class ProjectsController < ApplicationController end end - def upload_file - link_to_file = ::Projects::FileService.new(repository, params, root_url). - execute - - respond_to do |format| - if link_to_file - format.json { render json: { link: link_to_file } } - else - format.json { render json: 'Invalid file.', status: :unprocessable_entity } - end - end - end - def toggle_star current_user.toggle_star(@project) @project.reload @@ -159,10 +146,6 @@ class ProjectsController < ApplicationController private - def invalid_file(error) - render json: { message: error.message }, status: :internal_server_error - end - def set_title @title = 'New Project' end diff --git a/app/services/projects/file_service.rb b/app/services/projects/file_service.rb deleted file mode 100644 index 8c149bf53a1..00000000000 --- a/app/services/projects/file_service.rb +++ /dev/null @@ -1,55 +0,0 @@ -module Projects - 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("#{Rails.root}/uploads", upload_path, accepted_files) - file = @params['markdown_file'] - - 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' => filename, - 'url' => uploader.secure_url, - 'is_image' => image?(file) - } - else - link = nil - end - end - - protected - - def accepted_files - # insert accepted mime types here (e.g %w(jpg jpeg gif png)) - nil - end - - def accepted_images - %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?(file) - accepted_files.map { |format| image.content_type.include? format }.any? - end - end -end diff --git a/app/services/projects/upload_service.rb b/app/services/projects/upload_service.rb new file mode 100644 index 00000000000..a186c97628f --- /dev/null +++ b/app/services/projects/upload_service.rb @@ -0,0 +1,22 @@ +module Projects + class UploadService < BaseService + def initialize(project, file) + @project, @file = project, file + end + + def execute + return nil unless @file + + uploader = FileUploader.new(@project) + uploader.store!(@file) + + filename = uploader.image? ? uploader.file.basename : uploader.file.filename + + { + 'alt' => filename, + 'url' => uploader.secure_url, + 'is_image' => uploader.image? + } + end + end +end diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb index 22742d287a4..58dc6e90c1e 100644 --- a/app/uploaders/attachment_uploader.rb +++ b/app/uploaders/attachment_uploader.rb @@ -21,7 +21,7 @@ class AttachmentUploader < CarrierWave::Uploader::Base end def secure_url - Gitlab.config.gitlab.relative_url_root + "/files/#{model.class.to_s.underscore}/#{model.id}/#{file.filename}" + File.join(Gitlab.config.gitlab.relative_url_root, "files", model.class.to_s.underscore, model.id.to_s, file.filename) end def file_storage? diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 51ae8040e52..c040f6bbe92 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -2,47 +2,33 @@ class FileUploader < CarrierWave::Uploader::Base storage :file - def initialize(base_dir, path = '', allowed_extensions = nil) - @base_dir = base_dir - @path = path - @allowed_extensions = allowed_extensions + def initialize(project, secret = self.class.generate_secret) + @project = project + @secret = secret end def base_dir - @base_dir + "#{Rails.root}/uploads" end def store_dir - File.join(@base_dir, @path) + File.join(base_dir, @project.path_with_namespace, @secret) end def cache_dir - File.join(@base_dir, 'tmp', @path) + File.join(base_dir, 'tmp', @project.path_with_namespace, @secret) end - def extension_white_list - @allowed_extensions || super - end - - def store!(file) - @filename = self.class.generate_filename(file) - super - end - - def self.generate_filename(file) - original_filename = File.basename(file.original_filename, '.*') - extension = File.extname(file.original_filename) - new_filename = Digest::MD5.hexdigest(original_filename) + extension - end - - def self.generate_dir + def self.generate_secret SecureRandom.hex(5) end def secure_url - path_array = @path.split('/') - path = File.join(path_array[0],path_array[1],'uploads',path_array[2]) - Gitlab.config.gitlab.relative_url_root + "/#{path}/#{@filename}" + File.join(Gitlab.config.gitlab.relative_url_root, @project.path_with_namespace, "uploads", @secret, file.filename) + end + + def file_storage? + self.class.storage == CarrierWave::Storage::File end def image? diff --git a/app/views/projects/issues/_form.html.haml b/app/views/projects/issues/_form.html.haml index 975980bd6bd..afeeed6edf4 100644 --- a/app/views/projects/issues/_form.html.haml +++ b/app/views/projects/issues/_form.html.haml @@ -11,4 +11,4 @@ e.preventDefault(); }); - window.project_file_path_upload = "#{upload_file_project_path @project}"; + window.project_uploads_path = "#{project_uploads_path @project}"; diff --git a/app/views/projects/merge_requests/_form.html.haml b/app/views/projects/merge_requests/_form.html.haml index 28c4734e14f..c1a05e4586f 100644 --- a/app/views/projects/merge_requests/_form.html.haml +++ b/app/views/projects/merge_requests/_form.html.haml @@ -9,4 +9,4 @@ e.preventDefault(); }); - window.project_file_path_upload = "#{upload_file_project_path @project}"; + window.project_uploads_path = "#{project_uploads_path @project}"; diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index 0653b30fcca..4cf2a05b1a3 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -113,7 +113,7 @@ e.preventDefault(); }); - window.project_file_path_upload = "#{upload_file_project_path @project}"; + window.project_uploads_path = "#{project_uploads_path @project}"; :javascript var merge_request diff --git a/app/views/projects/milestones/_form.html.haml b/app/views/projects/milestones/_form.html.haml index 5fbb668570c..dbcd23eee05 100644 --- a/app/views/projects/milestones/_form.html.haml +++ b/app/views/projects/milestones/_form.html.haml @@ -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_file_path_upload = "#{upload_file_project_path @project}"; + window.project_uploads_path = "#{project_uploads_path @project}"; diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index fe3dab569f4..9f9efc782d5 100644 --- a/app/views/projects/notes/_form.html.haml +++ b/app/views/projects/notes/_form.html.haml @@ -29,4 +29,4 @@ = f.file_field :attachment, class: "js-note-attachment-input hidden" :javascript - window.project_file_path_upload = "#{upload_file_project_path @project}"; + window.project_uploads_path = "#{project_uploads_path @project}"; diff --git a/app/views/projects/wikis/_form.html.haml b/app/views/projects/wikis/_form.html.haml index 0afee138c8b..b1579878ed1 100644 --- a/app/views/projects/wikis/_form.html.haml +++ b/app/views/projects/wikis/_form.html.haml @@ -43,6 +43,6 @@ = link_to "Cancel", project_wiki_path(@project, :home), class: "btn btn-cancel" :javascript - window.project_file_path_upload = "#{upload_file_project_path @project}"; + window.project_uploads_path = "#{project_uploads_path @project}"; diff --git a/config/routes.rb b/config/routes.rb index d29ad8db639..f0a7cf1e8a6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -220,7 +220,6 @@ Gitlab::Application.routes.draw do put :transfer post :archive post :unarchive - post :upload_file post :toggle_star post :markdown_preview get :autocomplete_sources @@ -256,7 +255,11 @@ Gitlab::Application.routes.draw do end end - get '/uploads/:folder_id/:filename' => 'uploads#show', constraints: { filename: /.+/ } + resources :uploads, only: [:create] do + collection do + get ":secret/:filename", action: :show, constraints: { filename: /.+/ } + end + end get '/compare/:from...:to' => 'compare#show', :as => 'compare', :constraints => { from: /.+/, to: /.+/ } diff --git a/db/schema.rb b/db/schema.rb index e11a068c9c5..be3d35a431d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -26,7 +26,6 @@ ActiveRecord::Schema.define(version: 20150213121042) do t.datetime "updated_at" t.string "home_page_url" t.integer "default_branch_protection", default: 2 - t.boolean "twitter_sharing_enabled", default: true end create_table "broadcast_messages", force: true do |t| diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb new file mode 100644 index 00000000000..8c99b5ca528 --- /dev/null +++ b/spec/controllers/projects/uploads_controller_spec.rb @@ -0,0 +1,49 @@ +require('spec_helper') + +describe Projects::UploadsController do + let(:project) { create(:project) } + let(:user) { create(:user) } + 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 #create' do + before do + sign_in(user) + project.team << [user, :developer] + end + + context "without params['file']" do + it 'returns an error' do + post :create, project_id: project.to_param, format: :json + expect(response.status).to eq(422) + end + end + + context 'with valid image' do + before do + post :create, + project_id: project.to_param, + file: jpg, + format: :json + end + + 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\":\"/#{project.path_with_namespace}/uploads" + expect(response.body).to match '\"is_image\":true' + end + end + + context 'with valid non-image file' do + before do + post :create, project_id: project.to_param, file: txt, format: :json + end + + 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\":\"/#{project.path_with_namespace}/uploads" + expect(response.body).to match '\"is_image\":false' + end + end + end +end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 2d52e3fd913..9be4c2e505c 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -4,50 +4,7 @@ describe ProjectsController do let(:project) { create(:project) } let(:public_project) { create(:project, :public) } let(:user) { create(:user) } - 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_file' do - before do - sign_in(user) - project.team << [user, :developer] - end - - 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 valid image' do - before do - post :upload_file, - id: project.to_param, - markdown_file: jpg, - format: :json - end - - 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\":\"/#{project.path_with_namespace}/uploads" - expect(response.body).to match '\"is_image\":true' - end - end - - context 'with valid non-image file' do - before do - post :upload_file, id: project.to_param, markdown_file: txt, format: :json - end - - 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\":\"/#{project.path_with_namespace}/uploads" - 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 sign_in(user) diff --git a/spec/services/projects/file_service_spec.rb b/spec/services/projects/upload_service_spec.rb similarity index 80% rename from spec/services/projects/file_service_spec.rb rename to spec/services/projects/upload_service_spec.rb index 7bbe5b575c9..fc34b456482 100644 --- a/spec/services/projects/file_service_spec.rb +++ b/spec/services/projects/upload_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Projects::FileService do +describe Projects::UploadService do describe 'File service' do before do @user = create :user @@ -10,9 +10,7 @@ describe Projects::FileService do 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/') + @link_to_file = upload_file(@project.repository, gif) end it { expect(@link_to_file).to have_key('alt') } @@ -28,9 +26,7 @@ describe Projects::FileService 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/') + @link_to_file = upload_file(@project.repository, png) end it { expect(@link_to_file).to have_key('alt') } @@ -45,7 +41,7 @@ describe Projects::FileService do 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/') + @link_to_file = upload_file(@project.repository, jpg) end it { expect(@link_to_file).to have_key('alt') } @@ -60,9 +56,7 @@ describe Projects::FileService do 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/') + @link_to_file = upload_file(@project.repository, txt) end it { expect(@link_to_file).to have_key('alt') } @@ -75,7 +69,7 @@ describe Projects::FileService do end end - def upload_file(repository, params, root_url) - Projects::FileService.new(repository, params, root_url).execute + def upload_file(repository, file) + Projects::UploadService.new(repository, file).execute end end From ab401a6132411294cee03cf4e0902ec75c2c42dc Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 16 Feb 2015 22:08:44 +0100 Subject: [PATCH 06/16] Remove note attachment file selector. --- app/assets/javascripts/notes.js.coffee | 13 ------------- app/views/projects/notes/_edit_form.html.haml | 10 +--------- app/views/projects/notes/_form.html.haml | 8 -------- 3 files changed, 1 insertion(+), 30 deletions(-) diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee index 1c090bd06dc..90e6fd6d154 100644 --- a/app/assets/javascripts/notes.js.coffee +++ b/app/assets/javascripts/notes.js.coffee @@ -39,9 +39,6 @@ class @Notes # reset main target form after submit $(document).on "ajax:complete", ".js-main-target-form", @resetMainTargetForm - # attachment button - $(document).on "click", ".js-choose-note-attachment-button", @chooseNoteAttachment - # update the file name when an attachment is selected $(document).on "change", ".js-note-attachment-input", @updateFormAttachment @@ -73,7 +70,6 @@ class @Notes $(document).off "click", ".js-note-delete" $(document).off "click", ".js-note-attachment-delete" $(document).off "ajax:complete", ".js-main-target-form" - $(document).off "click", ".js-choose-note-attachment-button" $(document).off "click", ".js-discussion-reply-button" $(document).off "click", ".js-add-diff-note-button" $(document).off "visibilitychange" @@ -173,15 +169,6 @@ class @Notes form.find(".js-note-text").data("autosave").reset() - ### - Called when clicking the "Choose File" button. - - Opens the file selection dialog. - ### - chooseNoteAttachment: -> - form = $(this).closest("form") - form.find(".js-note-attachment-input").click() - ### Shows the main form and does some setup on it. diff --git a/app/views/projects/notes/_edit_form.html.haml b/app/views/projects/notes/_edit_form.html.haml index 4ba59078318..ca097f3d55a 100644 --- a/app/views/projects/notes/_edit_form.html.haml +++ b/app/views/projects/notes/_edit_form.html.haml @@ -11,12 +11,4 @@ .note-form-actions .buttons = f.submit 'Save Comment', class: "btn btn-primary btn-save btn-grouped js-comment-button" - = link_to 'Cancel', "#", class: "btn btn-cancel note-edit-cancel" - - .note-form-option.hidden-xs - %a.choose-btn.btn.js-choose-note-attachment-button - %i.fa.fa-paperclip - %span Choose File ... -   - %span.file_name.js-attachment-filename - = f.file_field :attachment, class: "js-note-attachment-input hidden" + = link_to 'Cancel', "#", class: "btn btn-cancel note-edit-cancel" \ No newline at end of file diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index 9f9efc782d5..8b331ef819f 100644 --- a/app/views/projects/notes/_form.html.haml +++ b/app/views/projects/notes/_form.html.haml @@ -20,13 +20,5 @@ = yield(:note_actions) %a.btn.grouped.js-close-discussion-note-form Cancel - .note-form-option.hidden-xs - %a.choose-btn.btn.js-choose-note-attachment-button - %i.fa.fa-paperclip - %span Choose File ... -   - %span.file_name.js-attachment-filename - = f.file_field :attachment, class: "js-note-attachment-input hidden" - :javascript window.project_uploads_path = "#{project_uploads_path @project}"; From ab65be7a2f5d46a681d882f4f90d1f0438c64b02 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 16 Feb 2015 22:08:55 +0100 Subject: [PATCH 07/16] Use longer upload secret. --- app/uploaders/file_uploader.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index c040f6bbe92..bdfbfc668bd 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -20,7 +20,7 @@ class FileUploader < CarrierWave::Uploader::Base end def self.generate_secret - SecureRandom.hex(5) + SecureRandom.hex end def secure_url From 99fb4d387fbcddc56292b788a84b1e62d27765dd Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 16 Feb 2015 22:09:17 +0100 Subject: [PATCH 08/16] Add paperclip icon to links to uploads in notes. --- app/assets/stylesheets/sections/notes.scss | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss index 5494845eb8c..40adc8b3ba7 100644 --- a/app/assets/stylesheets/sections/notes.scss +++ b/app/assets/stylesheets/sections/notes.scss @@ -66,6 +66,22 @@ ul.notes { overflow: auto; word-wrap: break-word; @include md-typography; + + a[href*="/uploads/"] { + &:before { + margin-right: 4px; + + font: normal normal normal 14px/1 FontAwesome; + font-size: inherit; + text-rendering: auto; + -webkit-font-smoothing: antialiased; + content: "\f0c6"; + } + + &:hover:before { + text-decoration: none; + } + } } } .note-header { From 4036fb167ddce47db8ad4d885000b2c9db96595d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 16 Feb 2015 22:09:52 +0100 Subject: [PATCH 09/16] Change textarea upload hover icon from picture to paperclip. --- app/assets/javascripts/dropzone_input.js.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/dropzone_input.js.coffee b/app/assets/javascripts/dropzone_input.js.coffee index 2d9b496b132..06e9f0001ae 100644 --- a/app/assets/javascripts/dropzone_input.js.coffee +++ b/app/assets/javascripts/dropzone_input.js.coffee @@ -6,7 +6,7 @@ class @DropzoneInput divHover = "
" divSpinner = "
" divAlert = "
" - iconPicture = "" + iconPaperclip = "" iconSpinner = "" btnAlert = "" project_uploads_path = window.project_uploads_path or null @@ -19,7 +19,7 @@ class @DropzoneInput form_dropzone = $(form).find('.div-dropzone') form_dropzone.parent().addClass "div-dropzone-wrapper" form_dropzone.append divHover - $(".div-dropzone-hover").append iconPicture + $(".div-dropzone-hover").append iconPaperclip form_dropzone.append divSpinner $(".div-dropzone-spinner").append iconSpinner $(".div-dropzone-spinner").css From 65b125a5035cb021aeb81e168fd4ae1ad6c74c11 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 18 Feb 2015 08:16:42 +0100 Subject: [PATCH 10/16] Update schema. --- db/schema.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/schema.rb b/db/schema.rb index be3d35a431d..e11a068c9c5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -26,6 +26,7 @@ ActiveRecord::Schema.define(version: 20150213121042) do t.datetime "updated_at" t.string "home_page_url" t.integer "default_branch_protection", default: 2 + t.boolean "twitter_sharing_enabled", default: true end create_table "broadcast_messages", force: true do |t| From c801df81fb48272b670b7448e3898a98cdb8b742 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 20 Feb 2015 14:39:35 +0100 Subject: [PATCH 11/16] Satisfy Rubocop. --- app/controllers/projects/uploads_controller.rb | 2 +- config/initializers/static_files.rb | 2 +- config/routes.rb | 4 ++-- lib/gitlab/middleware/static.rb | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index b922b56418a..2b4da35bc7f 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -16,4 +16,4 @@ class Projects::UploadsController < Projects::ApplicationController not_found! end end -end \ No newline at end of file +end diff --git a/config/initializers/static_files.rb b/config/initializers/static_files.rb index e04c29cee4a..2a6eaec0cc4 100644 --- a/config/initializers/static_files.rb +++ b/config/initializers/static_files.rb @@ -10,4 +10,4 @@ begin rescue # If ActionDispatch::Static wasn't loaded onto the stack (like in production), # an exception is raised. -end \ No newline at end of file +end diff --git a/config/routes.rb b/config/routes.rb index 0e7f7d893d4..ca56c2d268e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -76,11 +76,11 @@ Gitlab::Application.routes.draw do scope path: :uploads do # Note attachments and User/Group/Project avatars get ":model/:mounted_as/:id/:filename", to: "uploads#show", - constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /.+/ } + constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /.+/ } # Project markdown uploads get ":id/:secret/:filename", to: "projects/uploads#show", - constraints: { id: /[a-zA-Z.0-9_\-]+\/[a-zA-Z.0-9_\-]+/, filename: /.+/ } + constraints: { id: /[a-zA-Z.0-9_\-]+\/[a-zA-Z.0-9_\-]+/, filename: /.+/ } end # diff --git a/lib/gitlab/middleware/static.rb b/lib/gitlab/middleware/static.rb index b92319c95d4..85ffa8aca68 100644 --- a/lib/gitlab/middleware/static.rb +++ b/lib/gitlab/middleware/static.rb @@ -10,4 +10,4 @@ module Gitlab end end end -end \ No newline at end of file +end From 4ef6ffaad3e9b7a29b438722e5e101de78521ec7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 20 Feb 2015 15:19:50 +0100 Subject: [PATCH 12/16] Split up AttachmentUploader. --- app/models/group.rb | 2 +- app/models/project.rb | 2 +- app/models/user.rb | 2 +- app/uploaders/attachment_uploader.rb | 10 -------- app/uploaders/avatar_uploader.rb | 32 ++++++++++++++++++++++++ app/views/events/event/_note.html.haml | 6 ++--- app/views/projects/notes/_note.html.haml | 6 ++--- features/steps/groups.rb | 2 +- features/steps/profile/profile.rb | 2 +- features/steps/project/project.rb | 2 +- 10 files changed, 44 insertions(+), 22 deletions(-) create mode 100644 app/uploaders/avatar_uploader.rb diff --git a/app/models/group.rb b/app/models/group.rb index d6ec0be6081..da9621a2a1a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -23,7 +23,7 @@ class Group < Namespace validate :avatar_type, if: ->(user) { user.avatar_changed? } validates :avatar, file_size: { maximum: 200.kilobytes.to_i } - mount_uploader :avatar, AttachmentUploader + mount_uploader :avatar, AvatarUploader after_create :post_create_hook after_destroy :post_destroy_hook diff --git a/app/models/project.rb b/app/models/project.rb index 56e1aa29040..e2c7f76eb09 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -138,7 +138,7 @@ class Project < ActiveRecord::Base if: ->(project) { project.avatar && project.avatar_changed? } validates :avatar, file_size: { maximum: 200.kilobytes.to_i } - mount_uploader :avatar, AttachmentUploader + mount_uploader :avatar, AvatarUploader # Scopes scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) } diff --git a/app/models/user.rb b/app/models/user.rb index 21ccc76978e..a723b1289b6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -177,7 +177,7 @@ class User < ActiveRecord::Base end end - mount_uploader :avatar, AttachmentUploader + mount_uploader :avatar, AvatarUplaoder # Scopes scope :admins, -> { where(admin: true) } diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb index b122b6c8658..a9691bee46e 100644 --- a/app/uploaders/attachment_uploader.rb +++ b/app/uploaders/attachment_uploader.rb @@ -3,8 +3,6 @@ class AttachmentUploader < CarrierWave::Uploader::Base storage :file - after :store, :reset_events_cache - def store_dir "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" end @@ -22,15 +20,7 @@ class AttachmentUploader < CarrierWave::Uploader::Base false end - def secure_url - Gitlab.config.gitlab.relative_url_root + "/files/#{model.class.to_s.underscore}/#{model.id}/#{file.filename}" - end - def file_storage? self.class.storage == CarrierWave::Storage::File end - - def reset_events_cache(file) - model.reset_events_cache if model.is_a?(User) - end end diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb new file mode 100644 index 00000000000..7cad044555b --- /dev/null +++ b/app/uploaders/avatar_uploader.rb @@ -0,0 +1,32 @@ +# encoding: utf-8 + +class AvatarUploader < CarrierWave::Uploader::Base + storage :file + + after :store, :reset_events_cache + + def store_dir + "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" + 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 + + def file_storage? + self.class.storage == CarrierWave::Storage::File + end + + def reset_events_cache(file) + model.reset_events_cache if model.is_a?(User) + end +end diff --git a/app/views/events/event/_note.html.haml b/app/views/events/event/_note.html.haml index 0acb8538778..4ef18c09060 100644 --- a/app/views/events/event/_note.html.haml +++ b/app/views/events/event/_note.html.haml @@ -18,9 +18,9 @@ - note = event.target - if note.attachment.url - if note.attachment.image? - = link_to note.attachment.secure_url, target: '_blank' do - = image_tag note.attachment.secure_url, class: 'note-image-attach' + = link_to note.attachment.url, target: '_blank' do + = image_tag note.attachment.url, class: 'note-image-attach' - else - = link_to note.attachment.secure_url, target: "_blank", class: 'note-file-attach' do + = link_to note.attachment.url, target: "_blank", class: 'note-file-attach' do %i.fa.fa-paperclip = note.attachment_identifier diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 88c7b7ccf1a..cfeba00d271 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -57,10 +57,10 @@ - if note.attachment.url .note-attachment - if note.attachment.image? - = link_to note.attachment.secure_url, target: '_blank' do - = image_tag note.attachment.secure_url, class: 'note-image-attach' + = link_to note.attachment.url, target: '_blank' do + = image_tag note.attachment.url, class: 'note-image-attach' .attachment - = link_to note.attachment.secure_url, target: "_blank" do + = link_to note.attachment.url, target: "_blank" do %i.fa.fa-paperclip = note.attachment_identifier = link_to delete_attachment_project_note_path(@project, note), diff --git a/features/steps/groups.rb b/features/steps/groups.rb index 610e7fd3a48..0a9b4ccba53 100644 --- a/features/steps/groups.rb +++ b/features/steps/groups.rb @@ -110,7 +110,7 @@ class Spinach::Features::Groups < Spinach::FeatureSteps end step 'I should see new group "Owned" avatar' do - Group.find_by(name: "Owned").avatar.should be_instance_of AttachmentUploader + Group.find_by(name: "Owned").avatar.should be_instance_of AvatarUploader Group.find_by(name: "Owned").avatar.url.should == "/uploads/group/avatar/#{ Group.find_by(name:"Owned").id }/gitlab_logo.png" end diff --git a/features/steps/profile/profile.rb b/features/steps/profile/profile.rb index a907b0b7dcf..4efd2176782 100644 --- a/features/steps/profile/profile.rb +++ b/features/steps/profile/profile.rb @@ -29,7 +29,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps end step 'I should see new avatar' do - @user.avatar.should be_instance_of AttachmentUploader + @user.avatar.should be_instance_of AvatarUploader @user.avatar.url.should == "/uploads/user/avatar/#{ @user.id }/gitlab_logo.png" end diff --git a/features/steps/project/project.rb b/features/steps/project/project.rb index 033d45e0253..d39c8e7d2db 100644 --- a/features/steps/project/project.rb +++ b/features/steps/project/project.rb @@ -35,7 +35,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps end step 'I should see new project avatar' do - @project.avatar.should be_instance_of AttachmentUploader + @project.avatar.should be_instance_of AvatarUploader url = @project.avatar.url url.should == "/uploads/project/avatar/#{ @project.id }/gitlab_logo.png" end From 7f1adc3d9cdc5c3f1c0fcbf6c72d89b8ee062af5 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 20 Feb 2015 15:56:12 +0100 Subject: [PATCH 13/16] Fix URL to uploaded file. --- app/controllers/projects/uploads_controller.rb | 2 +- app/services/projects/upload_service.rb | 3 +-- app/uploaders/file_uploader.rb | 4 ++++ config/routes.rb | 6 +++--- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index 53b92d8643d..9020e86c44e 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -4,7 +4,7 @@ class Projects::UploadsController < Projects::ApplicationController before_filter :project def create - link_to_file = ::Projects::UploadService.new(repository, params[:file]). + link_to_file = ::Projects::UploadService.new(project, params[:file]). execute respond_to do |format| diff --git a/app/services/projects/upload_service.rb b/app/services/projects/upload_service.rb index b2466b52ad9..a186c97628f 100644 --- a/app/services/projects/upload_service.rb +++ b/app/services/projects/upload_service.rb @@ -1,6 +1,5 @@ module Projects class UploadService < BaseService - include Rails.application.routes.url_helpers def initialize(project, file) @project, @file = project, file end @@ -15,7 +14,7 @@ module Projects { 'alt' => filename, - 'url' => project_upload_url(@project, secret: uploader.secret, filename: uploader.file.filename), + 'url' => uploader.secure_url, 'is_image' => uploader.image? } end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 36a28f93c49..f9673abbfe8 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -25,6 +25,10 @@ class FileUploader < CarrierWave::Uploader::Base SecureRandom.hex end + def secure_url + File.join(Gitlab.config.gitlab.url, @project.path_with_namespace, "uploads", @secret, file.filename) + end + def file_storage? self.class.storage == CarrierWave::Storage::File end diff --git a/config/routes.rb b/config/routes.rb index 498716b12e0..b6f58acf1a6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -79,8 +79,8 @@ Gitlab::Application.routes.draw do constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /.+/ } # Project markdown uploads - get ":id/:secret/:filename", to: "projects/uploads#show", - constraints: { id: /[a-zA-Z.0-9_\-]+\/[a-zA-Z.0-9_\-]+/, filename: /.+/ } + get ":project_id/:secret/:filename", to: "projects/uploads#show", + constraints: { project_id: /[a-zA-Z.0-9_\-]+\/[a-zA-Z.0-9_\-]+/, filename: /.+/ } end # @@ -264,7 +264,7 @@ Gitlab::Application.routes.draw do resources :uploads, only: [:create] do collection do - get ":secret/:filename", action: :show, constraints: { filename: /.+/ } + get ":secret/:filename", action: :show, as: :show, constraints: { filename: /.+/ } end end From 2570e4df79fa09d3c4abc1d0ec82c67a322b249e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 20 Feb 2015 16:55:38 +0100 Subject: [PATCH 14/16] Fix specs. --- config/routes.rb | 10 ++++++---- spec/controllers/projects/uploads_controller_spec.rb | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index b6f58acf1a6..3d826bf5599 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -75,12 +75,14 @@ Gitlab::Application.routes.draw do scope path: :uploads do # Note attachments and User/Group/Project avatars - get ":model/:mounted_as/:id/:filename", to: "uploads#show", - constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /.+/ } + get ":model/:mounted_as/:id/:filename", + to: "uploads#show", + constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /.+/ } # Project markdown uploads - get ":project_id/:secret/:filename", to: "projects/uploads#show", - constraints: { project_id: /[a-zA-Z.0-9_\-]+\/[a-zA-Z.0-9_\-]+/, filename: /.+/ } + get ":project_id/:secret/:filename", + to: "projects/uploads#show", + constraints: { project_id: /[a-zA-Z.0-9_\-]+\/[a-zA-Z.0-9_\-]+/, filename: /.+/ } end # diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb index 8c99b5ca528..28d313b7e98 100644 --- a/spec/controllers/projects/uploads_controller_spec.rb +++ b/spec/controllers/projects/uploads_controller_spec.rb @@ -29,7 +29,7 @@ describe Projects::UploadsController do 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\":\"/#{project.path_with_namespace}/uploads" + expect(response.body).to match "\"url\":\"http://localhost/#{project.path_with_namespace}/uploads" expect(response.body).to match '\"is_image\":true' end end @@ -41,7 +41,7 @@ describe Projects::UploadsController do 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\":\"/#{project.path_with_namespace}/uploads" + expect(response.body).to match "\"url\":\"http://localhost/#{project.path_with_namespace}/uploads" expect(response.body).to match '\"is_image\":false' end end From 769d1ce64bdb7c9fd3981c65bc25dd14eb176c1e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 24 Feb 2015 16:10:55 +0100 Subject: [PATCH 15/16] Fix spec. --- spec/controllers/projects/uploads_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb index 8774ee0a841..029f48b2d7a 100644 --- a/spec/controllers/projects/uploads_controller_spec.rb +++ b/spec/controllers/projects/uploads_controller_spec.rb @@ -25,7 +25,7 @@ describe Projects::UploadsController do context 'with valid image' do before do post :create, - namespace_id: project.namespace.to_param + namespace_id: project.namespace.to_param, project_id: project.to_param, file: jpg, format: :json From 43f1ab9c12630e85861003c424da43314c2768a8 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 25 Feb 2015 23:23:49 +0100 Subject: [PATCH 16/16] Move CHANGELOG entry to 7.9. --- CHANGELOG | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index dce52b7c700..05413e02a9c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -10,6 +10,7 @@ v 7.9.0 (unreleased) - Save web edit in new branch - Fix ordering of imported but unchanged projects (Marco Wessel) - Mobile UI improvements: make aside content expandable + - Generalize image upload in drag and drop in markdown to all files (Hannes Rosenögger) v 7.8.1 - Fix run of custom post receive hooks @@ -20,8 +21,6 @@ v 7.8.1 v 7.8.0 - Fix access control and protection against XSS for note attachments and other uploads. - - 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