Merge branch '65096-fix-validations' into 'master'

Change validations to allow blank but not allow nil

See merge request gitlab-org/gitlab-ce!31133

8 files changed, 36 insertions, 29 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index a769a8f07fd..9dbcef8abaa 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -42,9 +42,9 @@ class ApplicationSetting < ApplicationRecord
   validates :uuid, presence: true
 
   validates :outbound_local_requests_whitelist,
-            length: { maximum: 1_000, message: N_('is too long (maximum is 1000 entries)') }
-
-  validates :outbound_local_requests_whitelist, qualified_domain_array: true, allow_blank: true
+            length: { maximum: 1_000, message: N_('is too long (maximum is 1000 entries)') },
+            allow_nil: false,
+            qualified_domain_array: true
 
   validates :session_expire_delay,
             presence: true,
diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb
index 1e612bd0e78..6efd07a6008 100644
--- a/app/models/application_setting_implementation.rb
+++ b/app/models/application_setting_implementation.rb
@@ -163,6 +163,8 @@ module ApplicationSettingImplementation
 
   def outbound_local_requests_whitelist_arrays
     strong_memoize(:outbound_local_requests_whitelist_arrays) do
+      next [[], []] unless self.outbound_local_requests_whitelist
+
       ip_whitelist = []
       domain_whitelist = []
 
@@ -284,6 +286,8 @@ module ApplicationSettingImplementation
   end
 
   def domain_strings_to_array(values)
+    return [] unless values
+
     values
       .split(DOMAIN_LIST_SEPARATOR)
       .reject(&:empty?)
diff --git a/app/validators/qualified_domain_array_validator.rb b/app/validators/qualified_domain_array_validator.rb
index 986c146a9db..c3a79d21ac0 100644
--- a/app/validators/qualified_domain_array_validator.rb
+++ b/app/validators/qualified_domain_array_validator.rb
@@ -23,9 +23,9 @@ class QualifiedDomainArrayValidator < ActiveModel::EachValidator
   private
 
   def validate_value_present(record, attribute, value)
-    return unless value.blank?
+    return unless value.nil?
 
-    record.errors.add(attribute, _('entries cannot be blank'))
+    record.errors.add(attribute, _('entries cannot be nil'))
   end
 
   def validate_host_length(record, attribute, value)
diff --git a/app/views/admin/application_settings/_outbound.html.haml b/app/views/admin/application_settings/_outbound.html.haml
index e58bb526c11..4fecdb59e1d 100644
--- a/app/views/admin/application_settings/_outbound.html.haml
+++ b/app/views/admin/application_settings/_outbound.html.haml
@@ -13,7 +13,7 @@
         = _('Whitelist to allow requests to the local network from hooks and services')
       = f.text_area :outbound_local_requests_whitelist_raw, placeholder: "example.com, 192.168.1.1", class: 'form-control', rows: 8
       %span.form-text.text-muted
-        = _('Requests to these domain(s)/address(es) on the local network will be allowed when local requests from hooks and services are disabled. IP ranges such as 1:0:0:0:0:0:0:0/124 or 127.0.0.0/28 are supported. Domain wildcards are not supported currently. Use comma, semicolon, or newline to separate multiple entries. The whitelist can hold a maximum of 4000 entries. Domains should use IDNA encoding. Ex: domain.com, 192.168.1.1, 127.0.0.0/28.')
+        = _('Requests to these domain(s)/address(es) on the local network will be allowed when local requests from hooks and services are not allowed. IP ranges such as 1:0:0:0:0:0:0:0/124 or 127.0.0.0/28 are supported. Domain wildcards are not supported currently. Use comma, semicolon, or newline to separate multiple entries. The whitelist can hold a maximum of 1000 entries. Domains should use IDNA encoding. Ex: example.com, 192.168.1.1, 127.0.0.0/28, xn--itlab-j1a.com.')
 
     .form-group
       .form-check
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index feae99fb95a..878ee5bc344 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -9238,7 +9238,7 @@ msgstr ""
 msgid "Requests Profiles"
 msgstr ""
 
-msgid "Requests to these domain(s)/address(es) on the local network will be allowed when local requests from hooks and services are disabled. IP ranges such as 1:0:0:0:0:0:0:0/124 or 127.0.0.0/28 are supported. Domain wildcards are not supported currently. Use comma, semicolon, or newline to separate multiple entries. The whitelist can hold a maximum of 4000 entries. Domains should use IDNA encoding. Ex: domain.com, 192.168.1.1, 127.0.0.0/28."
+msgid "Requests to these domain(s)/address(es) on the local network will be allowed when local requests from hooks and services are not allowed. IP ranges such as 1:0:0:0:0:0:0:0/124 or 127.0.0.0/28 are supported. Domain wildcards are not supported currently. Use comma, semicolon, or newline to separate multiple entries. The whitelist can hold a maximum of 1000 entries. Domains should use IDNA encoding. Ex: example.com, 192.168.1.1, 127.0.0.0/28, xn--itlab-j1a.com."
 msgstr ""
 
 msgid "Require all users in this group to setup Two-factor authentication"
@@ -13137,10 +13137,10 @@ msgstr ""
 msgid "encrypted: needs to be a :required, :optional or :migrating!"
 msgstr ""
 
-msgid "entries cannot be blank"
+msgid "entries cannot be larger than 255 characters"
 msgstr ""
 
-msgid "entries cannot be larger than 255 characters"
+msgid "entries cannot be nil"
 msgstr ""
 
 msgid "entries cannot contain HTML tags"
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index bd87bbd8d68..db80b85360f 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -45,7 +45,7 @@ describe ApplicationSetting do
     it { is_expected.to allow_value(['xn--itlab-j1a.com']).for(:outbound_local_requests_whitelist) }
     it { is_expected.not_to allow_value(['<h1></h1>']).for(:outbound_local_requests_whitelist) }
     it { is_expected.to allow_value(['gitlab.com']).for(:outbound_local_requests_whitelist) }
-    it { is_expected.to allow_value(nil).for(:outbound_local_requests_whitelist) }
+    it { is_expected.not_to allow_value(nil).for(:outbound_local_requests_whitelist) }
     it { is_expected.to allow_value([]).for(:outbound_local_requests_whitelist) }
 
     context "when user accepted let's encrypt terms of service" do
diff --git a/spec/support/shared_examples/application_setting_examples.rb b/spec/support/shared_examples/application_setting_examples.rb
index 2c600785ad3..0e6a8059376 100644
--- a/spec/support/shared_examples/application_setting_examples.rb
+++ b/spec/support/shared_examples/application_setting_examples.rb
@@ -40,6 +40,11 @@ RSpec.shared_examples 'string of domains' do |attribute|
     setting.method("#{attribute}_raw=").call("example;34543:garbage:fdh5654;")
     expect(setting.method(attribute).call).to contain_exactly('example', '34543:garbage:fdh5654')
   end
+
+  it 'does not raise error with nil' do
+    setting.method("#{attribute}_raw=").call(nil)
+    expect(setting.method(attribute).call).to eq([])
+  end
 end
 
 RSpec.shared_examples 'application settings examples' do
@@ -75,6 +80,12 @@ RSpec.shared_examples 'application settings examples' do
 
       expect(setting.outbound_local_requests_whitelist_arrays).to contain_exactly(ip_whitelist, domain_whitelist)
     end
+
+    it 'does not raise error with nil' do
+      setting.outbound_local_requests_whitelist = nil
+
+      expect(setting.outbound_local_requests_whitelist_arrays).to contain_exactly([], [])
+    end
   end
 
   describe 'usage ping settings' do
diff --git a/spec/validators/qualified_domain_array_validator_spec.rb b/spec/validators/qualified_domain_array_validator_spec.rb
index a96b00bfd1d..6beb4c67f6f 100644
--- a/spec/validators/qualified_domain_array_validator_spec.rb
+++ b/spec/validators/qualified_domain_array_validator_spec.rb
@@ -19,20 +19,19 @@ describe QualifiedDomainArrayValidator do
 
   subject { validator.validate(record) }
 
-  shared_examples 'cannot be blank' do
-    it 'returns error when attribute is blank' do
-      record.domain_array = []
+  shared_examples 'can be nil' do
+    it 'allows when attribute is nil' do
+      record.domain_array = nil
 
       subject
 
-      expect(record.errors).to be_present
-      expect(record.errors.first[1]).to eq 'entries cannot be blank'
+      expect(record.errors).to be_empty
     end
   end
 
-  shared_examples 'can be nil' do
-    it 'allows when attribute is nil' do
-      record.domain_array = nil
+  shared_examples 'can be blank' do
+    it 'allows when attribute is blank' do
+      record.domain_array = []
 
       subject
 
@@ -43,7 +42,7 @@ describe QualifiedDomainArrayValidator do
   describe 'validations' do
     let(:validator) { described_class.new(attributes: [:domain_array]) }
 
-    it_behaves_like 'cannot be blank'
+    it_behaves_like 'can be blank'
 
     it 'returns error when attribute is nil' do
       record.domain_array = nil
@@ -51,6 +50,7 @@ describe QualifiedDomainArrayValidator do
       subject
 
       expect(record.errors).to be_present
+      expect(record.errors.first[1]).to eq('entries cannot be nil')
     end
 
     it 'allows when domain is valid' do
@@ -91,21 +91,13 @@ describe QualifiedDomainArrayValidator do
     let(:validator) { described_class.new(attributes: [:domain_array], allow_nil: true) }
 
     it_behaves_like 'can be nil'
-
-    it_behaves_like 'cannot be blank'
+    it_behaves_like 'can be blank'
   end
 
   context 'when allow_blank is set to true' do
     let(:validator) { described_class.new(attributes: [:domain_array], allow_blank: true) }
 
     it_behaves_like 'can be nil'
-
-    it 'allows when attribute is blank' do
-      record.domain_array = []
-
-      subject
-
-      expect(record.errors).to be_empty
-    end
+    it_behaves_like 'can be blank'
   end
 end