From 8abf920d1f55e9117dd3b05d81ee9ebf7721f2bd Mon Sep 17 00:00:00 2001
From: George Koltsov <gkoltsov@gitlab.com>
Date: Tue, 30 Jul 2019 11:53:23 +0100
Subject: 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                    |   3 +-
 app/models/hooks/web_hook.rb                       |   4 +-
 app/validators/system_hook_url_validator.rb        |  16 +-----
 ...koltsov-55474-outbound-setting-system-hooks.yml |   2 +-
 ..._from_hooks_and_services_application_setting.rb |   4 +-
 ...ooks_and_services_application_setting_rename.rb |  15 ++++++
 db/schema.rb                                       |   2 +-
 doc/security/img/outbound_requests_section.png     | Bin 7314 -> 0 bytes
 .../img/outbound_requests_section_v12_2.png        | Bin 0 -> 21108 bytes
 doc/security/img/outbound_requests_section_v2.png  | Bin 21108 -> 0 bytes
 doc/security/webhooks.md                           |   9 ++--
 lib/gitlab/octokit/middleware.rb                   |   2 +-
 spec/lib/gitlab/octokit/middleware_spec.rb         |   8 +--
 spec/services/web_hook_service_spec.rb             |  10 ++--
 .../shared_examples/url_validator_examples.rb      |  57 +++++++++++++++++++--
 spec/validators/public_url_validator_spec.rb       |  24 +--------
 spec/validators/system_hook_url_validator_spec.rb  |  52 +------------------
 17 files changed, 94 insertions(+), 114 deletions(-)
 create mode 100644 db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb
 delete mode 100644 doc/security/img/outbound_requests_section.png
 create mode 100644 doc/security/img/outbound_requests_section_v12_2.png
 delete mode 100644 doc/security/img/outbound_requests_section_v2.png

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
Binary files a/doc/security/img/outbound_requests_section.png and /dev/null differ
diff --git a/doc/security/img/outbound_requests_section_v12_2.png b/doc/security/img/outbound_requests_section_v12_2.png
new file mode 100644
index 00000000000..4fd3c7d9fce
Binary files /dev/null and b/doc/security/img/outbound_requests_section_v12_2.png differ
diff --git a/doc/security/img/outbound_requests_section_v2.png b/doc/security/img/outbound_requests_section_v2.png
deleted file mode 100644
index 4fd3c7d9fce..00000000000
Binary files a/doc/security/img/outbound_requests_section_v2.png and /dev/null differ
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
-- 
cgit v1.2.1