Refactor SystemHookUrlValidator and specs

Simplify SystemHookUrlValidator to inherit from PublicUrlValidator
Refactor specs to move out shared examples to be used in both
system hooks and public url validators.

16 files changed, 94 insertions, 114 deletions

diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb
index b83913c845f..3d54d17e787 100644
--- a/app/models/hooks/system_hook.rb
+++ b/app/models/hooks/system_hook.rb
@@ -14,8 +14,7 @@ class SystemHook < WebHook
   default_value_for :repository_update_events, true
   default_value_for :merge_requests_events, false
 
-  validates :url, presence: true, public_url: false, system_hook_url: { allow_localhost: lambda(&:allow_local_requests?),
-                                                                        allow_local_network: lambda(&:allow_local_requests?) }
+  validates :url, system_hook_url: true
 
   # Allow urls pointing localhost and the local network
   def allow_local_requests?
diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index 3203bb024ab..16fc7fdbd48 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -15,8 +15,8 @@ class WebHook < ApplicationRecord
 
   has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
-  validates :url, presence: true, public_url: { allow_localhost: lambda(&:allow_local_requests?),
-                                                allow_local_network: lambda(&:allow_local_requests?) }
+  validates :url, presence: true
+  validates :url, public_url: true, unless: ->(hook) { hook.is_a?(SystemHook) }
 
   validates :token, format: { without: /\n/ }
   validates :push_events_branch_filter, branch_filter: true
diff --git a/app/validators/system_hook_url_validator.rb b/app/validators/system_hook_url_validator.rb
index e482828685d..f4253006dad 100644
--- a/app/validators/system_hook_url_validator.rb
+++ b/app/validators/system_hook_url_validator.rb
@@ -7,23 +7,11 @@
 # ApplicationSetting.allow_local_requests_from_system_hooks
 #
 # Example:
-#
 #   class SystemHook < WebHook
-#     validates :url, system_hook_url: { allow_localhost: true, allow_local_network: true }
+#     validates :url, system_hook_url: true
 #   end
 #
-class SystemHookUrlValidator < AddressableUrlValidator
-  DEFAULT_OPTIONS = {
-    allow_localhost: false,
-    allow_local_network: false
-  }.freeze
-
-  def initialize(options)
-    options.reverse_merge!(DEFAULT_OPTIONS)
-
-    super(options)
-  end
-
+class SystemHookUrlValidator < PublicUrlValidator
   def self.allow_setting_local_requests?
     ApplicationSetting.current&.allow_local_requests_from_system_hooks?
   end
diff --git a/changelogs/unreleased/georgekoltsov-55474-outbound-setting-system-hooks.yml b/changelogs/unreleased/georgekoltsov-55474-outbound-setting-system-hooks.yml
index 46b4a408a27..fb1acb1e9f5 100644
--- a/changelogs/unreleased/georgekoltsov-55474-outbound-setting-system-hooks.yml
+++ b/changelogs/unreleased/georgekoltsov-55474-outbound-setting-system-hooks.yml
@@ -2,4 +2,4 @@
 title: Add new outbound network requests application setting for system hooks
 merge_request: 31177
 author:
-type: changed
+type: added
diff --git a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb
index e3e8514427c..ac65e8d745c 100644
--- a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb
+++ b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb
@@ -8,10 +8,10 @@ class RenameAllowLocalRequestsFromHooksAndServicesApplicationSetting < ActiveRec
   disable_ddl_transaction!
 
   def up
-    rename_column :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services
+    rename_column_concurrently :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services
   end
 
   def down
-    rename_column :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services
+    cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services
   end
 end
diff --git a/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb
new file mode 100644
index 00000000000..397d3bce60a
--- /dev/null
+++ b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class CleanupAllowLocalRequestsFromHooksAndServicesApplicationSettingRename < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services
+  end
+
+  def down
+    rename_column_concurrently :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index e2ae13174cd..c88fe55277e 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -183,7 +183,6 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do
     t.string "external_authorization_service_default_label"
     t.boolean "pages_domain_verification_enabled", default: true, null: false
     t.string "user_default_internal_regex"
-    t.boolean "allow_local_requests_from_web_hooks_and_services", default: false, null: false
     t.float "external_authorization_service_timeout", default: 0.5
     t.text "external_auth_client_cert"
     t.text "encrypted_external_auth_client_key"
@@ -230,6 +229,7 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do
     t.string "grafana_url", default: "/-/grafana", null: false
     t.string "outbound_local_requests_whitelist", limit: 255, default: [], null: false, array: true
     t.integer "raw_blob_request_limit", default: 300, null: false
+    t.boolean "allow_local_requests_from_web_hooks_and_services", default: false, null: false
     t.boolean "allow_local_requests_from_system_hooks", default: true, null: false
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
diff --git a/doc/security/img/outbound_requests_section.png b/doc/security/img/outbound_requests_section.png
deleted file mode 100644
index f7783f34cdd..00000000000
--- a/doc/security/img/outbound_requests_section.png
+++ /dev/null
diff --git a/doc/security/img/outbound_requests_section_v2.png b/doc/security/img/outbound_requests_section_v12_2.png
index 4fd3c7d9fce..4fd3c7d9fce 100644
--- a/doc/security/img/outbound_requests_section_v2.png
+++ b/doc/security/img/outbound_requests_section_v12_2.png
diff --git a/doc/security/webhooks.md b/doc/security/webhooks.md
index 401df02e5c3..7ece9407ac0 100644
--- a/doc/security/webhooks.md
+++ b/doc/security/webhooks.md
@@ -38,11 +38,12 @@ local network from web hooks and services"* in the *"Outbound requests"* section
 inside the Admin area under **Settings**
 (`/admin/application_settings/network`):
 
-![Outbound requests admin settings](img/outbound_requests_section_v2.png)
+![Outbound requests admin settings](img/outbound_requests_section_v12_2.png)
 
->**Note:**
-*System hooks* are enabled to make requests to local network by default since they are set up by admins. 
-However, it can be turned off by disabling *"Allow requests to the local network from system hooks"* option.
+NOTE: **Note:**
+*System hooks* are enabled to make requests to local network by default since they are
+set up by administrators. However, you can turn this off by disabling the
+**Allow requests to the local network from system hooks** option.
 
 <!-- ## Troubleshooting
 
diff --git a/lib/gitlab/octokit/middleware.rb b/lib/gitlab/octokit/middleware.rb
index 2f762957d1b..2dd7d08a58b 100644
--- a/lib/gitlab/octokit/middleware.rb
+++ b/lib/gitlab/octokit/middleware.rb
@@ -16,7 +16,7 @@ module Gitlab
       private
 
       def allow_local_requests?
-        Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services?
+        Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?
       end
     end
   end
diff --git a/spec/lib/gitlab/octokit/middleware_spec.rb b/spec/lib/gitlab/octokit/middleware_spec.rb
index 7f2b523f5b7..43f6d13f7ba 100644
--- a/spec/lib/gitlab/octokit/middleware_spec.rb
+++ b/spec/lib/gitlab/octokit/middleware_spec.rb
@@ -30,7 +30,7 @@ describe Gitlab::Octokit::Middleware do
 
       context 'when localhost requests are not allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: false)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
         end
 
         it_behaves_like 'Local URL'
@@ -38,7 +38,7 @@ describe Gitlab::Octokit::Middleware do
 
       context 'when localhost requests are allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: true)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
         end
 
         it_behaves_like 'Public URL'
@@ -50,7 +50,7 @@ describe Gitlab::Octokit::Middleware do
 
       context 'when local network requests are not allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: false)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
         end
 
         it_behaves_like 'Local URL'
@@ -58,7 +58,7 @@ describe Gitlab::Octokit::Middleware do
 
       context 'when local network requests are allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: true)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
         end
 
         it_behaves_like 'Public URL'
diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb
index 115599bd1a4..50167a2e059 100644
--- a/spec/services/web_hook_service_spec.rb
+++ b/spec/services/web_hook_service_spec.rb
@@ -23,14 +23,14 @@ describe WebHookService do
       stub_application_setting(setting_name => setting)
     end
 
-    shared_examples_for 'respecting outbound network setting' do
-      context 'local requests are allowed' do
+    shared_examples_for 'respects outbound network setting' do
+      context 'when local requests are allowed' do
         let(:setting) { true }
 
         it { expect(hook.request_options[:allow_local_requests]).to be_truthy }
       end
 
-      context 'local requests are not allowed' do
+      context 'when local requests are not allowed' do
         let(:setting) { false }
 
         it { expect(hook.request_options[:allow_local_requests]).to be_falsey }
@@ -41,14 +41,14 @@ describe WebHookService do
       let(:setting_name) { :allow_local_requests_from_system_hooks }
       let(:hook) { described_class.new(build(:system_hook), data, :system_hook) }
 
-      include_examples 'respecting outbound network setting'
+      include_examples 'respects outbound network setting'
     end
 
     context 'when ProjectHook' do
       let(:setting_name) { :allow_local_requests_from_web_hooks_and_services }
       let(:hook) { described_class.new(build(:project_hook), data, :project_hook) }
 
-      include_examples 'respecting outbound network setting'
+      include_examples 'respects outbound network setting'
     end
   end
 
diff --git a/spec/support/shared_examples/url_validator_examples.rb b/spec/support/shared_examples/url_validator_examples.rb
index 16fceddb605..c5a775fefb6 100644
--- a/spec/support/shared_examples/url_validator_examples.rb
+++ b/spec/support/shared_examples/url_validator_examples.rb
@@ -1,12 +1,12 @@
 # frozen_string_literal: true
 
 RSpec.shared_examples 'url validator examples' do |schemes|
-  let(:validator) { described_class.new(attributes: [:link_url], **options) }
-  let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
+  describe '#validate' do
+    let(:validator) { described_class.new(attributes: [:link_url], **options) }
+    let(:badge) { build(:badge, link_url: 'http://www.example.com') }
 
-  subject { validator.validate(badge) }
+    subject { validator.validate(badge) }
 
-  describe '#validate' do
     context 'with no options' do
       let(:options) { {} }
 
@@ -42,3 +42,52 @@ RSpec.shared_examples 'url validator examples' do |schemes|
     end
   end
 end
+
+RSpec.shared_examples 'public url validator examples' do |setting|
+  let(:validator) { described_class.new(attributes: [:link_url]) }
+  let(:badge) { build(:badge, link_url: 'http://www.example.com') }
+
+  subject { validator.validate(badge) }
+
+  context 'by default' do
+    it 'blocks urls pointing to localhost' do
+      badge.link_url = 'https://127.0.0.1'
+
+      subject
+
+      expect(badge.errors).to be_present
+    end
+
+    it 'blocks urls pointing to the local network' do
+      badge.link_url = 'https://192.168.1.1'
+
+      subject
+
+      expect(badge.errors).to be_present
+    end
+  end
+
+  context 'when local requests are allowed' do
+    let!(:settings) { create(:application_setting) }
+
+    before do
+      stub_application_setting(setting)
+    end
+
+    it 'does not block urls pointing to localhost' do
+      badge.link_url = 'https://127.0.0.1'
+
+      subject
+
+      expect(badge.errors).not_to be_present
+    end
+
+    it 'does not block urls pointing to the local network' do
+      badge.link_url = 'https://192.168.1.1'
+
+      subject
+
+      expect(badge.errors).not_to be_present
+    end
+  end
+end
diff --git a/spec/validators/public_url_validator_spec.rb b/spec/validators/public_url_validator_spec.rb
index f6364fb1dd5..3cbf1002730 100644
--- a/spec/validators/public_url_validator_spec.rb
+++ b/spec/validators/public_url_validator_spec.rb
@@ -2,27 +2,5 @@ require 'spec_helper'
 
 describe PublicUrlValidator do
   include_examples 'url validator examples', AddressableUrlValidator::DEFAULT_OPTIONS[:schemes]
-
-  context 'by default' do
-    let(:validator) { described_class.new(attributes: [:link_url]) }
-    let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
-
-    subject { validator.validate(badge) }
-
-    it 'blocks urls pointing to localhost' do
-      badge.link_url = 'https://127.0.0.1'
-
-      subject
-
-      expect(badge.errors).to be_present
-    end
-
-    it 'blocks urls pointing to the local network' do
-      badge.link_url = 'https://192.168.1.1'
-
-      subject
-
-      expect(badge.errors).to be_present
-    end
-  end
+  include_examples 'public url validator examples', allow_local_requests_from_web_hooks_and_services: true
 end
diff --git a/spec/validators/system_hook_url_validator_spec.rb b/spec/validators/system_hook_url_validator_spec.rb
index 78e95db2b47..02384bbd1ce 100644
--- a/spec/validators/system_hook_url_validator_spec.rb
+++ b/spec/validators/system_hook_url_validator_spec.rb
@@ -4,55 +4,5 @@ require 'spec_helper'
 
 describe SystemHookUrlValidator do
   include_examples 'url validator examples', AddressableUrlValidator::DEFAULT_OPTIONS[:schemes]
-
-  context 'by default' do
-    let(:validator) { described_class.new(attributes: [:link_url]) }
-    let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
-
-    subject { validator.validate(badge) }
-
-    it 'blocks urls pointing to localhost' do
-      badge.link_url = 'https://127.0.0.1'
-
-      subject
-
-      expect(badge.errors).to be_present
-    end
-
-    it 'blocks urls pointing to the local network' do
-      badge.link_url = 'https://192.168.1.1'
-
-      subject
-
-      expect(badge.errors).to be_present
-    end
-  end
-
-  context 'when local requests are allowed' do
-    let(:validator) { described_class.new(attributes: [:link_url]) }
-    let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
-    let!(:settings) { create(:application_setting) }
-
-    subject { validator.validate(badge) }
-
-    before do
-      stub_application_setting(allow_local_requests_from_system_hooks: true)
-    end
-
-    it 'does not block urls pointing to localhost' do
-      badge.link_url = 'https://127.0.0.1'
-
-      subject
-
-      expect(badge.errors).not_to be_present
-    end
-
-    it 'does not block urls pointing to the local network' do
-      badge.link_url = 'https://192.168.1.1'
-
-      subject
-
-      expect(badge.errors).not_to be_present
-    end
-  end
+  include_examples 'public url validator examples', allow_local_requests_from_system_hooks: true
 end