Merge branch 'configurable-attachment-size' into 'master'

Support configurable attachment size in Application Settings page

### What does this MR do?

This MR provides the ability to configure the maximum size of an attachment inside a note. A parameter has been added to the Application Settings page.

### Are there points in the code the reviewer needs to double check?

What should be done with the legacy note attachment validation? I added code to make the validation work with the configurable setting. I could see an issue where an admin lowers the limit from 10 megabytes to 5 megabytes, which could cause an existing model to be invalid.

### Why was this MR needed?

We often have attachments that exceed 10 MB, and it would be nice to be able to override the defaults.

### What are the relevant issue numbers / [Feature requests](http://feedback.gitlab.com/)?

See Issue #1258

### Screenshots

Before:

![Screen_Shot_2015-03-29_at_3.06.53_PM](https://gitlab.com/gitlab-org/gitlab-ce/uploads/6013a1dbc8cf61a63e93744149937fa0/Screen_Shot_2015-03-29_at_3.06.53_PM.png)

After:

![Screen_Shot_2015-03-29_at_3.12.34_PM](https://gitlab.com/gitlab-org/gitlab-ce/uploads/f3518af7e8653ba40f0a3579456da6ad/Screen_Shot_2015-03-29_at_3.12.34_PM.png)

See merge request !407

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.10.0 (unreleased)
+  - Fix bug where error messages from Dropzone would not be displayed on the issues page (Stan Hu)
   - Fix broken side-by-side diff view on merge request page (Stan Hu)
   - Set Application controller default URL options to ensure all url_for calls are consistent (Stan Hu)
   - Allow HTML tags in Markdown input
   - Fix code unfold not working on Compare commits page (Stan Hu)
   - Fix dots in Wiki slugs causing errors (Stan Hu)
+  - Make maximum attachment size configurable via Application Settings (Stan Hu)
   - Update poltergeist to version 1.6.0 to support PhantomJS 2.0 (Zeger-Jan van de Weg)
   - Fix cross references when usernames, milestones, or project names contain underscores (Stan Hu)
   - Disable reference creation for comments surrounded by code/preformatted blocks (Stan Hu)

app/assets/javascripts/dropzone_input.js.coffee

@@ -10,6 +10,7 @@ class @DropzoneInput
     iconSpinner = "<i class=\"fa fa-spinner fa-spin div-dropzone-icon\"></i>"
     btnAlert = "<button type=\"button\"" + alertAttr + ">&times;</button>"
     project_uploads_path = window.project_uploads_path or null
+    max_file_size = gon.max_file_size or 10
 
     form_textarea = $(form).find("textarea.markdown-area")
     form_textarea.wrap "<div class=\"div-dropzone\"></div>"
@@ -76,7 +77,7 @@ class @DropzoneInput
       dictDefaultMessage: ""
       clickable: true
       paramName: "file"
-      maxFilesize: 10
+      maxFilesize: max_file_size
       uploadMultiple: false
       headers:
         "X-CSRF-Token": $("meta[name=\"csrf-token\"]").attr("content")
@@ -108,9 +109,10 @@ class @DropzoneInput
         return
 
       error: (temp, errorMessage) ->
-        checkIfMsgExists = $(".error-alert").children().length
+        errorAlert = $(form).find('.error-alert')
+        checkIfMsgExists = errorAlert.children().length
         if checkIfMsgExists is 0
-          $(".error-alert").append divAlert
+          errorAlert.append divAlert
           $(".div-dropzone-alert").append btnAlert + errorMessage
         return
 
@@ -221,9 +223,10 @@ class @DropzoneInput
         "display": "none"
 
     showError = (message) ->
-      checkIfMsgExists = $(".error-alert").children().length
+      errorAlert = $(form).find('.error-alert')
+      checkIfMsgExists = errorAlert.children().length
       if checkIfMsgExists is 0
-        $(".error-alert").append divAlert
+        errorAlert.append divAlert
         $(".div-dropzone-alert").append btnAlert + message
 
     closeAlertMessage = ->

app/controllers/admin/application_settings_controller.rb

@@ -38,6 +38,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :twitter_sharing_enabled,
       :sign_in_text,
       :home_page_url,
+      :max_attachment_size,
       restricted_visibility_levels: []
     )
   end

app/controllers/application_controller.rb

@@ -203,6 +203,7 @@ class ApplicationController < ActionController::Base
     gon.api_version = API::API.version
     gon.relative_url_root = Gitlab.config.gitlab.relative_url_root
     gon.default_avatar_url = URI::join(Gitlab.config.gitlab.url, ActionController::Base.helpers.image_path('no_avatar.png')).to_s
+    gon.max_file_size = current_application_settings.max_attachment_size;
 
     if current_user
       gon.current_user_id = current_user.id

app/models/application_setting.rb

@@ -16,6 +16,7 @@
 #  default_branch_protection    :integer          default(2)
 #  twitter_sharing_enabled      :boolean          default(TRUE)
 #  restricted_visibility_levels :text
+#  max_attachment_size          :integer          default(10)
 #
 
 class ApplicationSetting < ActiveRecord::Base
@@ -49,7 +50,8 @@ class ApplicationSetting < ActiveRecord::Base
       twitter_sharing_enabled: Settings.gitlab['twitter_sharing_enabled'],
       gravatar_enabled: Settings.gravatar['enabled'],
       sign_in_text: Settings.extra['sign_in_text'],
-      restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels']
+      restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'],
+      max_attachment_size: Settings.gitlab['max_attachment_size']
     )
   end
 

app/models/note.rb

@@ -22,6 +22,7 @@ require 'file_size_validator'
 
 class Note < ActiveRecord::Base
   include Mentionable
+  include Gitlab::CurrentSettings
 
   default_value_for :system, false
 
@@ -36,7 +37,8 @@ class Note < ActiveRecord::Base
 
   validates :note, :project, presence: true
   validates :line_code, format: { with: /\A[a-z0-9]+_\d+_\d+\Z/ }, allow_blank: true
-  validates :attachment, file_size: { maximum: 10.megabytes.to_i }
+  # Attachments are deprecated and are handled by Markdown uploader
+  validates :attachment, file_size: { maximum: :max_attachment_size }
 
   validates :noteable_id, presence: true, if: ->(n) { n.noteable_type.present? && n.noteable_type != 'Commit' }
   validates :commit_id, presence: true, if: ->(n) { n.noteable_type == 'Commit' }
@@ -321,6 +323,10 @@ class Note < ActiveRecord::Base
     end
   end
 
+  def max_attachment_size
+    current_application_settings.max_attachment_size.megabytes.to_i
+  end
+
   def commit_author
     @commit_author ||=
       project.team.users.find_by(email: noteable.author_email) ||

app/services/projects/upload_service.rb

@@ -5,7 +5,7 @@ module Projects
     end
 
     def execute
-      return nil unless @file
+      return nil unless @file and @file.size <= max_attachment_size
 
       uploader = FileUploader.new(@project)
       uploader.store!(@file)
@@ -18,5 +18,11 @@ module Projects
         'is_image'  => uploader.image?
       }
     end
+
+    private
+
+    def max_attachment_size
+      current_application_settings.max_attachment_size.megabytes.to_i
+    end
   end
 end

app/views/admin/application_settings/_form.html.haml

@@ -60,5 +60,10 @@
       .col-sm-10
         = f.text_area :sign_in_text, class: 'form-control', rows: 4
         .help-block Markdown enabled
+    .form-group
+      = f.label :max_attachment_size, 'Maximum attachment size (MB)', class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.number_field :max_attachment_size, class: 'form-control'
+
   .form-actions
     = f.submit 'Save', class: 'btn btn-primary'

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

@@ -12,7 +12,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 files by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector', tabindex: -1 }.
-
+    .error-alert
 
   .note-form-actions
     .buttons

config/initializers/1_settings.rb

@@ -119,6 +119,7 @@ Settings.gitlab['username_changing_enabled'] = true if Settings.gitlab['username
 Settings.gitlab['issue_closing_pattern'] = '((?:[Cc]los(?:e[sd]?|ing)|[Ff]ix(?:e[sd]|ing)?|[Rr]esolv(?:e[sd]?|ing)) +(?:(?:issues? +)?#\d+(?:(?:, *| +and +)?))+)' if Settings.gitlab['issue_closing_pattern'].nil?
 Settings.gitlab['default_projects_features'] ||= {}
 Settings.gitlab['webhook_timeout'] ||= 10
+Settings.gitlab['max_attachment_size'] ||= 10
 Settings.gitlab.default_projects_features['issues']         = true if Settings.gitlab.default_projects_features['issues'].nil?
 Settings.gitlab.default_projects_features['merge_requests'] = true if Settings.gitlab.default_projects_features['merge_requests'].nil?
 Settings.gitlab.default_projects_features['wiki']           = true if Settings.gitlab.default_projects_features['wiki'].nil?

db/migrate/20150328132231_add_max_attachment_size_to_application_settings.rb

+class AddMaxAttachmentSizeToApplicationSettings < ActiveRecord::Migration
+  def change
+    add_column :application_settings, :max_attachment_size, :integer, default: 10, null: false
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150324155957) do
+ActiveRecord::Schema.define(version: 20150328132231) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -28,6 +28,7 @@ ActiveRecord::Schema.define(version: 20150324155957) do
     t.integer  "default_branch_protection",    default: 2
     t.boolean  "twitter_sharing_enabled",      default: true
     t.text     "restricted_visibility_levels"
+    t.integer  "max_attachment_size",          default: 10,   null: false
   end
 
   create_table "broadcast_messages", force: true do |t|

features/project/issues/issues.feature

@@ -42,6 +42,7 @@ Feature: Project Issues
     Given I visit issue page "Release 0.4"
     And I leave a comment like "XML attached"
     Then I should see comment "XML attached"
+    And I should see an error alert section within the comment form
 
   @javascript
   Scenario: I search issue

features/steps/project/issues/issues.rb

@@ -204,6 +204,12 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
     end
   end
 
+  step 'I should see an error alert section within the comment form' do
+    within(".js-main-target-form") do
+      find(".error-alert")
+    end
+  end
+
   step 'The code block should be unchanged' do
     page.should have_content("```\nCommand [1]: /usr/local/bin/git , see [text](doc/text)\n```")
   end

lib/file_size_validator.rb

@@ -25,8 +25,8 @@ class FileSizeValidator < ActiveModel::EachValidator
     keys.each do |key|
       value = options[key]
 
-      unless value.is_a?(Integer) && value >= 0
-        raise ArgumentError, ":#{key} must be a nonnegative Integer"
+      unless (value.is_a?(Integer) && value >= 0) || value.is_a?(Symbol)
+        raise ArgumentError, ":#{key} must be a nonnegative Integer or symbol"
       end
     end
   end
@@ -39,6 +39,14 @@ class FileSizeValidator < ActiveModel::EachValidator
     CHECKS.each do |key, validity_check|
       next unless check_value = options[key]
 
+      check_value =
+        case check_value
+        when Integer
+          check_value
+        when Symbol
+          record.send(check_value)
+        end
+
       value ||= [] if key == :maximum
 
       value_size = value.size

lib/gitlab/current_settings.rb

@@ -20,7 +20,8 @@ module Gitlab
         signin_enabled: Settings.gitlab['signin_enabled'],
         gravatar_enabled: Settings.gravatar['enabled'],
         sign_in_text: Settings.extra['sign_in_text'],
-        restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels']
+        restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'],
+        max_attachment_size: Settings.gitlab['max_attachment_size']
       )
     end
   end

spec/lib/file_size_validator_spec.rb

+require 'spec_helper'
+
+describe 'Gitlab::FileSizeValidatorSpec' do
+  let(:validator) { FileSizeValidator.new(options) }
+  let(:attachment) { AttachmentUploader.new }
+  let(:note) { create(:note) }
+
+  describe 'options uses an integer' do
+    let(:options) { { maximum: 10, attributes: { attachment: attachment } } }
+
+    it 'attachment exceeds maximum limit' do
+        allow(attachment).to receive(:size) { 100 }
+        validator.validate_each(note, :attachment, attachment)
+        expect(note.errors).to have_key(:attachment)
+    end
+
+    it 'attachment under maximum limit' do
+      allow(attachment).to receive(:size) { 1 }
+      validator.validate_each(note, :attachment, attachment)
+      expect(note.errors).not_to have_key(:attachment)
+    end
+  end
+
+  describe 'options uses a symbol' do
+    let(:options) { { maximum: :test,
+                      attributes: { attachment: attachment } } }
+    before do
+      allow(note).to receive(:test) { 10 }
+    end
+
+    it 'attachment exceeds maximum limit' do
+      allow(attachment).to receive(:size) { 100 }
+      validator.validate_each(note, :attachment, attachment)
+      expect(note.errors).to have_key(:attachment)
+    end
+
+    it 'attachment under maximum limit' do
+      allow(attachment).to receive(:size) { 1 }
+      validator.validate_each(note, :attachment, attachment)
+      expect(note.errors).not_to have_key(:attachment)
+    end
+  end
+end

spec/services/projects/upload_service_spec.rb

@@ -67,6 +67,16 @@ describe Projects::UploadService do
       it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file['url']).to match('doc_sample.txt') }
     end
+
+    context 'for too large a file' do
+      before do
+        txt = fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain')
+        allow(txt).to receive(:size) { 1000.megabytes.to_i }
+        @link_to_file = upload_file(@project.repository, txt)
+      end
+
+      it { expect(@link_to_file).to eq(nil) }
+    end
   end
 
   def upload_file(repository, file)