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.

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?

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

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

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

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

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

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"

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
 

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

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'

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
 

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') }
-
-  subject { validator.validate(badge) }
-
   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) }
+
     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

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

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