From dfd256f29ee817b5ffc563bb554a02d26ae44502 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 20 Mar 2015 05:11:12 -0700 Subject: Support configurable attachment size via Application Settings Fix bug where error messages from Dropzone would not be displayed on the issues page Closes #1258 --- CHANGELOG | 2 + app/assets/javascripts/dropzone_input.js.coffee | 15 +++++--- .../admin/application_settings_controller.rb | 1 + app/controllers/application_controller.rb | 1 + app/models/application_setting.rb | 4 +- app/models/note.rb | 10 ++++- app/services/projects/upload_service.rb | 8 +++- .../admin/application_settings/_form.html.haml | 5 +++ app/views/projects/notes/_form.html.haml | 2 +- config/initializers/1_settings.rb | 1 + ..._max_attachment_size_to_application_settings.rb | 5 +++ db/schema.rb | 3 +- features/project/issues/issues.feature | 1 + features/steps/project/issues/issues.rb | 6 +++ lib/file_size_validator.rb | 12 +++++- lib/gitlab/current_settings.rb | 3 +- spec/lib/file_size_validator_spec.rb | 43 ++++++++++++++++++++++ spec/services/projects/upload_service_spec.rb | 10 +++++ 18 files changed, 117 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20150328132231_add_max_attachment_size_to_application_settings.rb create mode 100644 spec/lib/file_size_validator_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 7cca0835afa..57d0bba1cfe 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,11 +1,13 @@ 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) diff --git a/app/assets/javascripts/dropzone_input.js.coffee b/app/assets/javascripts/dropzone_input.js.coffee index 06e9f0001ae..fca2a290e2d 100644 --- a/app/assets/javascripts/dropzone_input.js.coffee +++ b/app/assets/javascripts/dropzone_input.js.coffee @@ -10,6 +10,7 @@ class @DropzoneInput iconSpinner = "" btnAlert = "" 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 "
" @@ -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 = -> @@ -237,4 +240,4 @@ class @DropzoneInput formatLink: (link) -> text = "[#{link.alt}](#{link.url})" text = "!#{text}" if link.is_image - text \ No newline at end of file + text diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 9a5685877f8..b5fda196bf0 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/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 diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2809f90c0d5..80e983b5314 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 1c87db613ae..6e98c4c2f02 100644 --- a/app/models/application_setting.rb +++ b/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 diff --git a/app/models/note.rb b/app/models/note.rb index e86160e7cd9..fdab4517df3 100644 --- a/app/models/note.rb +++ b/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) || @@ -451,7 +457,7 @@ class Note < ActiveRecord::Base prev_match_line = line else prev_lines << line - + break if generate_line_code(line) == self.line_code prev_lines.shift if prev_lines.length >= max_number_of_lines diff --git a/app/services/projects/upload_service.rb b/app/services/projects/upload_service.rb index a186c97628f..992a7a7a1dc 100644 --- a/app/services/projects/upload_service.rb +++ b/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 diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index edfcccfcf4c..4f3565c67eb 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/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' diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index be96c302143..2ada6cb6700 100644 --- a/app/views/projects/notes/_form.html.haml +++ b/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 diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 15c1ae9466f..4c37fbf7cab 100644 --- a/config/initializers/1_settings.rb +++ b/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? diff --git a/db/migrate/20150328132231_add_max_attachment_size_to_application_settings.rb b/db/migrate/20150328132231_add_max_attachment_size_to_application_settings.rb new file mode 100644 index 00000000000..1d161674a9a --- /dev/null +++ b/db/migrate/20150328132231_add_max_attachment_size_to_application_settings.rb @@ -0,0 +1,5 @@ +class AddMaxAttachmentSizeToApplicationSettings < ActiveRecord::Migration + def change + add_column :application_settings, :max_attachment_size, :integer, default: 10, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 4a445ae5832..14e32a7946e 100644 --- a/db/schema.rb +++ b/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| diff --git a/features/project/issues/issues.feature b/features/project/issues/issues.feature index b9031f6f32b..af01c7058ea 100644 --- a/features/project/issues/issues.feature +++ b/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 diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index e8ca3f7c176..e2834d51a27 100644 --- a/features/steps/project/issues/issues.rb +++ b/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 diff --git a/lib/file_size_validator.rb b/lib/file_size_validator.rb index 42970c1be59..2eae55e534b 100644 --- a/lib/file_size_validator.rb +++ b/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 diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 0ebebfa09c4..d8f696d247b 100644 --- a/lib/gitlab/current_settings.rb +++ b/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 diff --git a/spec/lib/file_size_validator_spec.rb b/spec/lib/file_size_validator_spec.rb new file mode 100644 index 00000000000..5c89c854714 --- /dev/null +++ b/spec/lib/file_size_validator_spec.rb @@ -0,0 +1,43 @@ +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 diff --git a/spec/services/projects/upload_service_spec.rb b/spec/services/projects/upload_service_spec.rb index fc34b456482..e5c47015a03 100644 --- a/spec/services/projects/upload_service_spec.rb +++ b/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) -- cgit v1.2.3