diff --git a/app/assets/javascripts/dropzone_input.js b/app/assets/javascripts/dropzone_input.js index b70d242269d..b3a76fbb43e 100644 --- a/app/assets/javascripts/dropzone_input.js +++ b/app/assets/javascripts/dropzone_input.js @@ -5,7 +5,7 @@ require('./preview_markdown'); window.DropzoneInput = (function() { function DropzoneInput(form) { - var $mdArea, alertAttr, alertClass, appendToTextArea, btnAlert, child, closeAlertMessage, closeSpinner, divAlert, divHover, divSpinner, dropzone, form_dropzone, form_textarea, getFilename, handlePaste, iconPaperclip, iconSpinner, insertToTextArea, isImage, max_file_size, pasteText, project_uploads_path, showError, showSpinner, uploadFile, uploadProgress; + var $mdArea, alertAttr, alertClass, appendToTextArea, btnAlert, child, closeAlertMessage, closeSpinner, divAlert, divHover, divSpinner, dropzone, form_dropzone, form_textarea, getFilename, handlePaste, iconPaperclip, iconSpinner, insertToTextArea, isImage, max_file_size, pasteText, uploads_path, showError, showSpinner, uploadFile, uploadProgress; Dropzone.autoDiscover = false; alertClass = "alert alert-danger alert-dismissable div-dropzone-alert"; alertAttr = "class=\"close\" data-dismiss=\"alert\"" + "aria-hidden=\"true\""; @@ -16,7 +16,7 @@ window.DropzoneInput = (function() { iconSpinner = ""; uploadProgress = $("
"); btnAlert = ""; - project_uploads_path = window.project_uploads_path || null; + uploads_path = window.uploads_path || null; max_file_size = gon.max_file_size || 10; form_textarea = $(form).find(".js-gfm-input"); form_textarea.wrap(""); @@ -39,10 +39,10 @@ window.DropzoneInput = (function() { "display": "none" }); - if (!project_uploads_path) return; + if (!uploads_path) return; dropzone = form_dropzone.dropzone({ - url: project_uploads_path, + url: uploads_path, dictDefaultMessage: "", clickable: true, paramName: "file", @@ -159,7 +159,7 @@ window.DropzoneInput = (function() { formData = new FormData(); formData.append("file", item, filename); return $.ajax({ - url: project_uploads_path, + url: uploads_path, type: "POST", data: formData, dataType: "json", diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb new file mode 100644 index 00000000000..dec2e27335a --- /dev/null +++ b/app/controllers/concerns/uploads_actions.rb @@ -0,0 +1,27 @@ +module UploadsActions + def create + link_to_file = UploadService.new(model, params[:file], uploader_class).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 + return render_404 unless uploader.exists? + + disposition = uploader.image_or_video? ? 'inline' : 'attachment' + + expires_in 0.seconds, must_revalidate: true, private: true + + 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 index 61686499bd3..6966a7c5fee 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -1,33 +1,11 @@ class Projects::UploadsController < Projects::ApplicationController + include UploadsActions + skip_before_action :project, :repository, if: -> { action_name == 'show' && image_or_video? } before_action :authorize_upload_file!, only: [:create] - def create - link_to_file = ::Projects::UploadService.new(project, 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 - return render_404 if uploader.nil? || !uploader.file.exists? - - disposition = uploader.image_or_video? ? 'inline' : 'attachment' - send_file uploader.file.path, disposition: disposition - end - private def uploader @@ -52,4 +30,10 @@ class Projects::UploadsController < Projects::ApplicationController def image_or_video? uploader && uploader.file.exists? && uploader.image_or_video? end + + def uploader_class + FileUploader + end + + alias_method :model, :project end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index f1bfd574f04..21a964fb391 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -1,50 +1,43 @@ class UploadsController < ApplicationController + include UploadsActions + skip_before_action :authenticate_user! - before_action :find_model, :authorize_access! - - def show - uploader = @model.send(upload_mount) - - unless uploader.file_storage? - return redirect_to uploader.url - end - - unless uploader.file && uploader.file.exists? - return render_404 - end - - disposition = uploader.image? ? 'inline' : 'attachment' - - expires_in 0.seconds, must_revalidate: true, private: true - send_file uploader.file.path, disposition: disposition - end + before_action :find_model + before_action :authorize_access!, only: [:show] + before_action :authorize_create_access!, only: [:create] private def find_model - unless upload_model && upload_mount - return render_404 - end + return render_404 unless upload_model && upload_mount @model = upload_model.find(params[:id]) end def authorize_access! authorized = - case @model - when Project - can?(current_user, :read_project, @model) - when Group - can?(current_user, :read_group, @model) + case model when Note - can?(current_user, :read_project, @model.project) - else - # No authentication required for user avatars. + can?(current_user, :read_project, model.project) + when User true + else + permission = "read_#{model.class.to_s.underscore}".to_sym + + can?(current_user, permission, model) end - return if authorized + render_unauthorized unless authorized + end + def authorize_create_access! + # for now we support only personal snippets comments + authorized = can?(current_user, :comment_personal_snippet, model) + + render_unauthorized unless authorized + end + + def render_unauthorized if current_user render_404 else @@ -58,17 +51,44 @@ class UploadsController < ApplicationController "project" => Project, "note" => Note, "group" => Group, - "appearance" => Appearance + "appearance" => Appearance, + "personal_snippet" => PersonalSnippet } upload_models[params[:model]] end def upload_mount + return true unless params[:mounted_as] + upload_mounts = %w(avatar attachment file logo header_logo) if upload_mounts.include?(params[:mounted_as]) params[:mounted_as] end end + + def uploader + return @uploader if defined?(@uploader) + + if model.is_a?(PersonalSnippet) + @uploader = PersonalFileUploader.new(model, params[:secret]) + + @uploader.retrieve_from_store!(params[:filename]) + else + @uploader = @model.send(upload_mount) + + redirect_to @uploader.url unless @uploader.file_storage? + end + + @uploader + end + + def uploader_class + PersonalFileUploader + end + + def model + @model ||= find_model + end end diff --git a/app/policies/personal_snippet_policy.rb b/app/policies/personal_snippet_policy.rb index d3913986cd8..e1e5336da8c 100644 --- a/app/policies/personal_snippet_policy.rb +++ b/app/policies/personal_snippet_policy.rb @@ -3,11 +3,16 @@ class PersonalSnippetPolicy < BasePolicy can! :read_personal_snippet if @subject.public? return unless @user + if @subject.public? + can! :comment_personal_snippet + end + if @subject.author == @user can! :read_personal_snippet can! :update_personal_snippet can! :destroy_personal_snippet can! :admin_personal_snippet + can! :comment_personal_snippet end unless @user.external? @@ -16,6 +21,7 @@ class PersonalSnippetPolicy < BasePolicy if @subject.internal? && !@user.external? can! :read_personal_snippet + can! :comment_personal_snippet end end end diff --git a/app/services/projects/upload_service.rb b/app/services/projects/upload_service.rb deleted file mode 100644 index be34d4fa9b8..00000000000 --- a/app/services/projects/upload_service.rb +++ /dev/null @@ -1,22 +0,0 @@ -module Projects - class UploadService < BaseService - def initialize(project, file) - @project, @file = project, file - end - - def execute - return nil unless @file && @file.size <= max_attachment_size - - uploader = FileUploader.new(@project) - uploader.store!(@file) - - uploader.to_h - end - - private - - def max_attachment_size - current_application_settings.max_attachment_size.megabytes.to_i - end - end -end diff --git a/app/services/upload_service.rb b/app/services/upload_service.rb new file mode 100644 index 00000000000..6c5b2baff41 --- /dev/null +++ b/app/services/upload_service.rb @@ -0,0 +1,20 @@ +class UploadService + def initialize(model, file, uploader_class = FileUploader) + @model, @file, @uploader_class = model, file, uploader_class + end + + def execute + return nil unless @file && @file.size <= max_attachment_size + + uploader = @uploader_class.new(@model) + uploader.store!(@file) + + uploader.to_h + end + + private + + def max_attachment_size + current_application_settings.max_attachment_size.megabytes.to_i + end +end diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index e84944ed411..3e36ec91205 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -30,8 +30,4 @@ class ArtifactUploader < GitlabUploader def filename file.try(:filename) end - - def exists? - file.try(:exists?) - end end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index d2783ce5b2f..7e94218c23d 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -26,11 +26,11 @@ class FileUploader < GitlabUploader File.join(CarrierWave.root, base_dir, model.path_with_namespace) end - attr_accessor :project + attr_accessor :model attr_reader :secret - def initialize(project, secret = nil) - @project = project + def initialize(model, secret = nil) + @model = model @secret = secret || generate_secret end @@ -38,10 +38,6 @@ class FileUploader < GitlabUploader File.join(dynamic_path_segment, @secret) end - def model - project - end - def relative_path self.file.path.sub("#{dynamic_path_segment}/", '') end diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index d662ba6820c..e0a6c9b4067 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -33,4 +33,8 @@ class GitlabUploader < CarrierWave::Uploader::Base def relative_path self.file.path.sub("#{root}/", '') end + + def exists? + file.try(:exists?) + end end diff --git a/app/uploaders/lfs_object_uploader.rb b/app/uploaders/lfs_object_uploader.rb index faab539b8e0..95a891111e1 100644 --- a/app/uploaders/lfs_object_uploader.rb +++ b/app/uploaders/lfs_object_uploader.rb @@ -9,10 +9,6 @@ class LfsObjectUploader < GitlabUploader "#{Gitlab.config.lfs.storage_path}/tmp/cache" end - def exists? - file.try(:exists?) - end - def filename model.oid[4..-1] end diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb new file mode 100644 index 00000000000..969b0a20d38 --- /dev/null +++ b/app/uploaders/personal_file_uploader.rb @@ -0,0 +1,15 @@ +class PersonalFileUploader < FileUploader + def self.dynamic_path_segment(model) + File.join(CarrierWave.root, model_path(model)) + end + + private + + def secure_url + File.join(self.class.model_path(model), secret, file.filename) + end + + def self.model_path(model) + File.join("/#{base_dir}", model.class.to_s.underscore, model.id.to_s) + end +end diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index f5e7ea7710d..e9e06e5c8e3 100644 --- a/app/views/layouts/project.html.haml +++ b/app/views/layouts/project.html.haml @@ -11,7 +11,7 @@ - preview_markdown_path = preview_markdown_namespace_project_path(project.namespace, project) - if current_user :javascript - window.project_uploads_path = "#{namespace_project_uploads_path project.namespace,project}"; + window.uploads_path = "#{namespace_project_uploads_path project.namespace,project}"; window.preview_markdown_path = "#{preview_markdown_path}"; - content_for :header_content do diff --git a/changelogs/unreleased/12910-uploader-pers-snippet.yml b/changelogs/unreleased/12910-uploader-pers-snippet.yml new file mode 100644 index 00000000000..1c163632fc6 --- /dev/null +++ b/changelogs/unreleased/12910-uploader-pers-snippet.yml @@ -0,0 +1,4 @@ +--- +title: Support uploaders for personal snippets comments +merge_request: +author: diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb index 2b22148a134..b315186b178 100644 --- a/config/routes/uploads.rb +++ b/config/routes/uploads.rb @@ -4,6 +4,11 @@ scope path: :uploads do to: "uploads#show", constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /[^\/]+/ } + # show uploads for models, snippets (notes) available for now + get ':model/:id/:secret/:filename', + to: 'uploads#show', + constraints: { model: /personal_snippet/, id: /\d+/, filename: /[^\/]+/ } + # Appearance get ":model/:mounted_as/:id/:filename", to: "uploads#show", @@ -13,6 +18,12 @@ scope path: :uploads do get ":namespace_id/:project_id/:secret/:filename", to: "projects/uploads#show", constraints: { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: /[^\/]+/ } + + # create uploads for models, snippets (notes) available for now + post ':model/:id/', + to: 'uploads#create', + constraints: { model: /personal_snippet/, id: /\d+/ }, + as: 'upload' end # Redirect old note attachments path to new uploads path. diff --git a/lib/api/projects.rb b/lib/api/projects.rb index db4b31b55bc..ca6c3083649 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -381,7 +381,7 @@ module API requires :file, type: File, desc: 'The file to be uploaded' end post ":id/uploads" do - ::Projects::UploadService.new(user_project, params[:file]).execute + UploadService.new(user_project, params[:file]).execute end desc 'Get the users list of a project' do diff --git a/lib/api/v3/projects.rb b/lib/api/v3/projects.rb index ba9748ada59..06cc704afc6 100644 --- a/lib/api/v3/projects.rb +++ b/lib/api/v3/projects.rb @@ -452,7 +452,7 @@ module API requires :file, type: File, desc: 'The file to be uploaded' end post ":id/uploads" do - ::Projects::UploadService.new(user_project, params[:file]).execute + UploadService.new(user_project, params[:file]).execute end desc 'Get the users list of a project' do diff --git a/lib/gitlab/email/attachment_uploader.rb b/lib/gitlab/email/attachment_uploader.rb index 32cece8316b..83440ae227d 100644 --- a/lib/gitlab/email/attachment_uploader.rb +++ b/lib/gitlab/email/attachment_uploader.rb @@ -21,7 +21,7 @@ module Gitlab content_type: attachment.content_type } - link = ::Projects::UploadService.new(project, file).execute + link = UploadService.new(project, file).execute attachments << link if link ensure tmp.close! diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index f67d26da0ac..7dedfe160a6 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -8,6 +8,93 @@ end describe UploadsController do let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) } + describe 'POST create' do + let(:model) { 'personal_snippet' } + let(:snippet) { create(:personal_snippet, :public) } + 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') } + + context 'when a user does not have permissions to upload a file' do + it "returns 401 when the user is not logged in" do + post :create, model: model, id: snippet.id, format: :json + + expect(response).to have_http_status(401) + end + + it "returns 404 when user can't comment on a snippet" do + private_snippet = create(:personal_snippet, :private) + + sign_in(user) + post :create, model: model, id: private_snippet.id, format: :json + + expect(response).to have_http_status(404) + end + end + + context 'when a user is logged in' do + before do + sign_in(user) + end + + it "returns an error without file" do + post :create, model: model, id: snippet.id, format: :json + + expect(response).to have_http_status(422) + end + + it "returns an error with invalid model" do + expect { post :create, model: 'invalid', id: snippet.id, format: :json } + .to raise_error(ActionController::UrlGenerationError) + end + + it "returns 404 status when object not found" do + post :create, model: model, id: 9999, format: :json + + expect(response).to have_http_status(404) + end + + context 'with valid image' do + before do + post :create, model: 'personal_snippet', id: snippet.id, 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\":\"/uploads" + end + + it 'creates a corresponding Upload record' do + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq snippet + end + end + end + + context 'with valid non-image file' do + before do + post :create, model: 'personal_snippet', id: snippet.id, 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\":\"/uploads" + end + + it 'creates a corresponding Upload record' do + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq snippet + end + end + end + end + end + describe "GET show" do context 'Content-Disposition security measures' do let(:project) { create(:empty_project, :public) } diff --git a/spec/policies/personal_snippet_policy_spec.rb b/spec/policies/personal_snippet_policy_spec.rb new file mode 100644 index 00000000000..58aa1145c9e --- /dev/null +++ b/spec/policies/personal_snippet_policy_spec.rb @@ -0,0 +1,141 @@ +require 'spec_helper' + +describe PersonalSnippetPolicy, models: true do + let(:regular_user) { create(:user) } + let(:external_user) { create(:user, :external) } + let(:admin_user) { create(:user, :admin) } + + let(:author_permissions) do + [ + :update_personal_snippet, + :admin_personal_snippet, + :destroy_personal_snippet + ] + end + + def permissions(user) + described_class.abilities(user, snippet).to_set + end + + context 'public snippet' do + let(:snippet) { create(:personal_snippet, :public) } + + context 'no user' do + subject { permissions(nil) } + + it do + is_expected.to include(:read_personal_snippet) + is_expected.not_to include(:comment_personal_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'regular user' do + subject { permissions(regular_user) } + + it do + is_expected.to include(:read_personal_snippet) + is_expected.to include(:comment_personal_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'author' do + subject { permissions(snippet.author) } + + it do + is_expected.to include(:read_personal_snippet) + is_expected.to include(:comment_personal_snippet) + is_expected.to include(*author_permissions) + end + end + end + + context 'internal snippet' do + let(:snippet) { create(:personal_snippet, :internal) } + + context 'no user' do + subject { permissions(nil) } + + it do + is_expected.not_to include(:read_personal_snippet) + is_expected.not_to include(:comment_personal_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'regular user' do + subject { permissions(regular_user) } + + it do + is_expected.to include(:read_personal_snippet) + is_expected.to include(:comment_personal_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'external user' do + subject { permissions(external_user) } + + it do + is_expected.not_to include(:read_personal_snippet) + is_expected.not_to include(:comment_personal_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'snippet author' do + subject { permissions(snippet.author) } + + it do + is_expected.to include(:read_personal_snippet) + is_expected.to include(:comment_personal_snippet) + is_expected.to include(*author_permissions) + end + end + end + + context 'private snippet' do + let(:snippet) { create(:project_snippet, :private) } + + context 'no user' do + subject { permissions(nil) } + + it do + is_expected.not_to include(:read_personal_snippet) + is_expected.not_to include(:comment_personal_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'regular user' do + subject { permissions(regular_user) } + + it do + is_expected.not_to include(:read_personal_snippet) + is_expected.not_to include(:comment_personal_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'external user' do + subject { permissions(external_user) } + + it do + is_expected.not_to include(:read_personal_snippet) + is_expected.not_to include(:comment_personal_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'snippet author' do + subject { permissions(snippet.author) } + + it do + is_expected.to include(:read_personal_snippet) + is_expected.to include(:comment_personal_snippet) + is_expected.to include(*author_permissions) + end + end + end +end diff --git a/spec/services/projects/upload_service_spec.rb b/spec/services/upload_service_spec.rb similarity index 95% rename from spec/services/projects/upload_service_spec.rb rename to spec/services/upload_service_spec.rb index d2cefa46bfa..95ba28dbecd 100644 --- a/spec/services/projects/upload_service_spec.rb +++ b/spec/services/upload_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Projects::UploadService, services: true do +describe UploadService, services: true do describe 'File service' do before do @user = create(:user) @@ -68,6 +68,6 @@ describe Projects::UploadService, services: true do end def upload_file(project, file) - Projects::UploadService.new(project, file).execute + described_class.new(project, file, FileUploader).execute end end diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb new file mode 100644 index 00000000000..fb92f2ae3ab --- /dev/null +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe PersonalFileUploader do + let(:uploader) { described_class.new(build_stubbed(:empty_project)) } + let(:snippet) { create(:personal_snippet) } + + describe '.absolute_path' do + it 'returns the correct absolute path by building it dynamically' do + upload = double(model: snippet, path: 'secret/foo.jpg') + + dynamic_segment = "personal_snippet/#{snippet.id}" + + expect(described_class.absolute_path(upload)).to end_with("#{dynamic_segment}/secret/foo.jpg") + end + end + + describe '#to_h' do + it 'returns the hass' do + uploader = described_class.new(snippet, 'secret') + + allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name')) + expected_url = "/uploads/personal_snippet/#{snippet.id}/secret/file_name" + + expect(uploader.to_h).to eq( + alt: 'file_name', + url: expected_url, + markdown: "[file_name](#{expected_url})" + ) + end + end +end