diff options
22 files changed, 163 insertions, 193 deletions
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/ @@ -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 = "<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 + ">×</button>" - 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 "<div class=\"div-dropzone\"></div>" @@ -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 create + link_to_file = ::Projects::UploadService.new(repository, params[:file]). + execute + + 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 - 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) + uploader = FileUploader.new(project, params[:secret]) + + if uploader.file_storage? + uploader.retrieve_from_store!(params[:filename]) - disposition = uploader.image? ? 'inline' : 'attachment' - send_file uploader.file.path, disposition: disposition + 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 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 |