Commit 0957161c authored by charlie ablett's avatar charlie ablett Committed by Alex Kalderimis

Apply reviewer comments

- remove _raw from places
- Add tiny test helper method
- Downcase upon storage and comparison
parent ab04004c
...@@ -105,6 +105,6 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -105,6 +105,6 @@ class Projects::NotesController < Projects::ApplicationController
end end
def rate_limit_users_allowlist def rate_limit_users_allowlist
Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist_raw Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist
end end
end end
...@@ -65,7 +65,7 @@ module Mutations ...@@ -65,7 +65,7 @@ module Mutations
def rate_limit_throttled? def rate_limit_throttled?
rate_limiter = ::Gitlab::ApplicationRateLimiter rate_limiter = ::Gitlab::ApplicationRateLimiter
allowlist = Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist_raw allowlist = Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist
rate_limiter.throttled?(:notes_create, scope: [current_user], users_allowlist: allowlist) rate_limiter.throttled?(:notes_create, scope: [current_user], users_allowlist: allowlist)
end end
......
...@@ -329,7 +329,7 @@ module ApplicationSettingsHelper ...@@ -329,7 +329,7 @@ module ApplicationSettingsHelper
:email_restrictions, :email_restrictions,
:issues_create_limit, :issues_create_limit,
:notes_create_limit, :notes_create_limit,
:notes_create_limit_allowlist, :notes_create_limit_allowlist_raw,
:raw_blob_request_limit, :raw_blob_request_limit,
:project_import_limit, :project_import_limit,
:project_export_limit, :project_export_limit,
......
...@@ -276,7 +276,7 @@ module ApplicationSettingImplementation ...@@ -276,7 +276,7 @@ module ApplicationSettingImplementation
end end
def notes_create_limit_allowlist_raw=(values) def notes_create_limit_allowlist_raw=(values)
self.notes_create_limit_allowlist = strings_to_array(values) self.notes_create_limit_allowlist = strings_to_array(values).map(&:downcase)
end end
def asset_proxy_allowlist=(values) def asset_proxy_allowlist=(values)
......
...@@ -74,7 +74,7 @@ module API ...@@ -74,7 +74,7 @@ module API
end end
post ":id/#{noteables_str}/:noteable_id/notes", feature_category: feature_category do post ":id/#{noteables_str}/:noteable_id/notes", feature_category: feature_category do
allowlist = allowlist =
Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist_raw Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist
check_rate_limit! :notes_create, [current_user], allowlist check_rate_limit! :notes_create, [current_user], allowlist
noteable = find_noteable(noteable_type, params[:noteable_id]) noteable = find_noteable(noteable_type, params[:noteable_id])
......
...@@ -150,7 +150,7 @@ module Gitlab ...@@ -150,7 +150,7 @@ module Gitlab
scoped_user = [options[:scope]].flatten.find { |s| s.is_a?(User) } scoped_user = [options[:scope]].flatten.find { |s| s.is_a?(User) }
return unless scoped_user return unless scoped_user
scoped_user.username.downcase.in?(options[:users_allowlist].map(&:downcase)) scoped_user.username.downcase.in?(options[:users_allowlist])
end end
end end
end end
......
...@@ -763,8 +763,9 @@ RSpec.describe Projects::NotesController do ...@@ -763,8 +763,9 @@ RSpec.describe Projects::NotesController do
4.times { create! } 4.times { create! }
end end
it 'allows user in allow-list to create notes' do it 'allows user in allow-list to create notes, even if the case is different' do
stub_application_setting(notes_create_limit_allowlist_raw: ["#{user.username}"]) user.update_attribute(:username, user.username.titleize)
stub_application_setting(notes_create_limit_allowlist: ["#{user.username.downcase}"])
3.times { create! } 3.times { create! }
create! create!
......
...@@ -120,8 +120,12 @@ RSpec.describe ApplicationSetting do ...@@ -120,8 +120,12 @@ RSpec.describe ApplicationSetting do
it { is_expected.not_to allow_value(5.5).for(:notes_create_limit) } it { is_expected.not_to allow_value(5.5).for(:notes_create_limit) }
it { is_expected.not_to allow_value(-2).for(:notes_create_limit) } it { is_expected.not_to allow_value(-2).for(:notes_create_limit) }
it { is_expected.to allow_value(['username'] * 100).for(:notes_create_limit_allowlist) } def many_usernames(num = 100)
it { is_expected.not_to allow_value(['username'] * 101).for(:notes_create_limit_allowlist) } Array.new(num) { |i| "username#{i}" }
end
it { is_expected.to allow_value(many_usernames(100)).for(:notes_create_limit_allowlist) }
it { is_expected.not_to allow_value(many_usernames(101)).for(:notes_create_limit_allowlist) }
it { is_expected.not_to allow_value(nil).for(:notes_create_limit_allowlist) } it { is_expected.not_to allow_value(nil).for(:notes_create_limit_allowlist) }
it { is_expected.to allow_value([]).for(:notes_create_limit_allowlist) } it { is_expected.to allow_value([]).for(:notes_create_limit_allowlist) }
......
...@@ -77,7 +77,7 @@ RSpec.shared_examples 'a Note mutation when there are rate limit validation erro ...@@ -77,7 +77,7 @@ RSpec.shared_examples 'a Note mutation when there are rate limit validation erro
context 'when the user is in the allowlist' do context 'when the user is in the allowlist' do
before do before do
stub_application_setting(notes_create_limit_allowlist_raw: ["#{current_user.username}"]) stub_application_setting(notes_create_limit_allowlist: ["#{current_user.username}"])
end end
it_behaves_like 'a Note mutation that creates a Note' it_behaves_like 'a Note mutation that creates a Note'
......
...@@ -295,7 +295,7 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name| ...@@ -295,7 +295,7 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
end end
it 'allows user in allow-list to create notes' do it 'allows user in allow-list to create notes' do
stub_application_setting(notes_create_limit_allowlist_raw: ["#{user.username}"]) stub_application_setting(notes_create_limit_allowlist: ["#{user.username}"])
subject subject
expect(response).to have_gitlab_http_status(:created) expect(response).to have_gitlab_http_status(:created)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment