Merge branch '12910-uploader-pers-snippet' into 'master'

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 = "<i class=\"fa fa-spinner fa-spin div-dropzone-icon\"></i>";
     uploadProgress = $("<div class=\"div-dropzone-progress\"></div>");
     btnAlert = "<button type=\"button\"" + alertAttr + ">&times;</button>";
-    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("<div class=\"div-dropzone\"></div>");
@@ -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",

app/controllers/concerns/uploads_actions.rb

+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

app/controllers/projects/uploads_controller.rb

 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

app/controllers/uploads_controller.rb

 class UploadsController < ApplicationController
-  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
+  include UploadsActions
 
-    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
+  skip_before_action :authenticate_user!
+  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

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

app/services/projects/upload_service.rb → app/services/upload_service.rb

-module Projects
-  class UploadService < BaseService
-    def initialize(project, file)
-      @project, @file = project, file
-    end
+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
+  def execute
+    return nil unless @file && @file.size <= max_attachment_size
 
-      uploader = FileUploader.new(@project)
-      uploader.store!(@file)
+    uploader = @uploader_class.new(@model)
+    uploader.store!(@file)
 
-      uploader.to_h
-    end
+    uploader.to_h
+  end
 
-    private
+  private
 
-    def max_attachment_size
-      current_application_settings.max_attachment_size.megabytes.to_i
-    end
+  def max_attachment_size
+    current_application_settings.max_attachment_size.megabytes.to_i
   end
 end

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

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

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

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

app/uploaders/personal_file_uploader.rb

+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

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

changelogs/unreleased/12910-uploader-pers-snippet.yml

+---
+title: Support uploaders for personal snippets comments
+merge_request:
+author:

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.

lib/api/projects.rb

@@ -385,7 +385,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

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

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!

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) }

spec/policies/personal_snippet_policy_spec.rb

+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

spec/services/projects/upload_service_spec.rb → spec/services/upload_service_spec.rb

 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

spec/uploaders/personal_file_uploader_spec.rb

+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