Allow testing of disabled hooks

Ignore executable status if the webhook is executed directly, as it
is when testing a hook.

This is essential for dealing with disabled hooks. If they cannot be
run, then they cannot be re-enabled. For this reason we also run the
log-execution (which handles re-enablement) inline during tests.

Changelog: fixed

app/models/hooks/web_hook.rb

@@ -50,8 +50,9 @@ class WebHook < ApplicationRecord
   end
 
   # rubocop: disable CodeReuse/ServiceClass
-  def execute(data, hook_name)
-    WebHookService.new(self, data, hook_name).execute if executable?
+  def execute(data, hook_name, force: false)
+    # hook.executable? is checked in WebHookService#execute
+    WebHookService.new(self, data, hook_name, force: force).execute
   end
   # rubocop: enable CodeReuse/ServiceClass
 

app/services/test_hooks/base_service.rb

@@ -18,7 +18,7 @@ module TestHooks
       return error('Testing not available for this hook') if trigger_key.nil? || data.blank?
       return error(data[:error]) if data[:error].present?
 
-      hook.execute(data, trigger_key)
+      hook.execute(data, trigger_key, force: true)
     end
   end
 end

app/services/web_hook_service.rb

@@ -34,11 +34,12 @@ class WebHookService
     hook_name.to_s.singularize.titleize
   end
 
-  def initialize(hook, data, hook_name, uniqueness_token = nil)
+  def initialize(hook, data, hook_name, uniqueness_token = nil, force: false)
     @hook = hook
     @data = data
     @hook_name = hook_name.to_s
     @uniqueness_token = uniqueness_token
+    @force = force
     @request_options = {
       timeout: Gitlab.config.gitlab.webhook_timeout,
       use_read_total_timeout: true,
@@ -46,8 +47,12 @@ class WebHookService
     }
   end
 
+  def disabled?
+    !@force && !hook.executable?
+  end
+
   def execute
-    return { status: :error, message: 'Hook disabled' } unless hook.executable?
+    return { status: :error, message: 'Hook disabled' } if disabled?
 
     if recursion_blocked?
       log_recursion_blocked
@@ -148,8 +153,12 @@ class WebHookService
       internal_error_message: error_message
     }
 
-    ::WebHooks::LogExecutionWorker
-      .perform_async(hook.id, log_data, category, uniqueness_token)
+    if @force # executed as part of test - run log-execution inline.
+      ::WebHooks::LogExecutionService.new(hook: hook, log_data: log_data, response_category: category).execute
+    else
+      ::WebHooks::LogExecutionWorker
+        .perform_async(hook.id, log_data, category, uniqueness_token)
+    end
   end
 
   def response_category(response)

spec/models/hooks/service_hook_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe ServiceHook do
     let(:data) { { key: 'value' } }
 
     it '#execute' do
-      expect(WebHookService).to receive(:new).with(hook, data, 'service_hook').and_call_original
+      expect(WebHookService).to receive(:new).with(hook, data, 'service_hook', force: false).and_call_original
       expect_any_instance_of(WebHookService).to receive(:execute)
 
       hook.execute(data)

spec/models/hooks/system_hook_spec.rb

@@ -168,17 +168,17 @@ RSpec.describe SystemHook do
     let(:data) { { key: 'value' } }
     let(:hook_name) { 'system_hook' }
 
-    before do
-      expect(WebHookService).to receive(:new).with(hook, data, hook_name).and_call_original
-    end
-
     it '#execute' do
+      expect(WebHookService).to receive(:new).with(hook, data, hook_name, force: false).and_call_original
+
       expect_any_instance_of(WebHookService).to receive(:execute)
 
       hook.execute(data, hook_name)
     end
 
     it '#async_execute' do
+      expect(WebHookService).to receive(:new).with(hook, data, hook_name).and_call_original
+
       expect_any_instance_of(WebHookService).to receive(:async_execute)
 
       hook.async_execute(data, hook_name)

spec/models/hooks/web_hook_spec.rb

@@ -100,12 +100,18 @@ RSpec.describe WebHook do
       hook.execute(data, hook_name)
     end
 
-    it 'does not execute non-executable hooks' do
-      hook.update!(disabled_until: 1.day.from_now)
+    it 'passes force: false to the web hook service by default' do
+      expect(WebHookService)
+        .to receive(:new).with(hook, data, hook_name, force: false).and_return(double(execute: :done))
 
-      expect(WebHookService).not_to receive(:new)
+      expect(hook.execute(data, hook_name)).to eq :done
+    end
 
-      hook.execute(data, hook_name)
+    it 'passes force: true to the web hook service if required' do
+      expect(WebHookService)
+        .to receive(:new).with(hook, data, hook_name, force: true).and_return(double(execute: :forced))
+
+      expect(hook.execute(data, hook_name, force: true)).to eq :forced
     end
 
     it '#async_execute' do

spec/services/test_hooks/project_service_spec.rb

@@ -37,7 +37,7 @@ RSpec.describe TestHooks::ProjectService do
       it 'executes hook' do
         allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data)
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -49,7 +49,7 @@ RSpec.describe TestHooks::ProjectService do
       it 'executes hook' do
         allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data)
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -69,7 +69,7 @@ RSpec.describe TestHooks::ProjectService do
         allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data)
         allow_next(NotesFinder).to receive(:execute).and_return(Note.all)
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -86,7 +86,7 @@ RSpec.describe TestHooks::ProjectService do
         allow(issue).to receive(:to_hook_data).and_return(sample_data)
         allow_next(IssuesFinder).to receive(:execute).and_return([issue])
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -119,7 +119,7 @@ RSpec.describe TestHooks::ProjectService do
         allow(merge_request).to receive(:to_hook_data).and_return(sample_data)
         allow_next(MergeRequestsFinder).to receive(:execute).and_return([merge_request])
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -138,7 +138,7 @@ RSpec.describe TestHooks::ProjectService do
         allow(Gitlab::DataBuilder::Build).to receive(:build).and_return(sample_data)
         allow_next(Ci::JobsFinder).to receive(:execute).and_return([ci_job])
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -157,7 +157,7 @@ RSpec.describe TestHooks::ProjectService do
         allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data)
         allow_next(Ci::PipelinesFinder).to receive(:execute).and_return([pipeline])
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -184,7 +184,7 @@ RSpec.describe TestHooks::ProjectService do
         create(:wiki_page, wiki: project.wiki)
         allow(Gitlab::DataBuilder::WikiPage).to receive(:build).and_return(sample_data)
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -203,7 +203,7 @@ RSpec.describe TestHooks::ProjectService do
         allow(release).to receive(:to_hook_data).and_return(sample_data)
         allow_next(ReleasesFinder).to receive(:execute).and_return([release])
 
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end

spec/services/test_hooks/system_service_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe TestHooks::SystemService do
       it 'executes hook' do
         expect(Gitlab::DataBuilder::Push).to receive(:sample_data).and_call_original
 
-        expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -45,7 +45,7 @@ RSpec.describe TestHooks::SystemService do
         allow(project.repository).to receive(:tags).and_return(['tag'])
         expect(Gitlab::DataBuilder::Push).to receive(:sample_data).and_call_original
 
-        expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -57,7 +57,7 @@ RSpec.describe TestHooks::SystemService do
       it 'executes hook' do
         expect(Gitlab::DataBuilder::Repository).to receive(:sample_data).and_call_original
 
-        expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Repository::SAMPLE_DATA, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Repository::SAMPLE_DATA, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end
@@ -76,7 +76,7 @@ RSpec.describe TestHooks::SystemService do
       it 'executes hook' do
         expect(MergeRequest).to receive(:of_projects).and_return([merge_request])
         expect(merge_request).to receive(:to_hook_data).and_return(sample_data)
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
     end

spec/services/web_hook_service_spec.rb

@@ -52,6 +52,25 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
     end
   end
 
+  describe '#disabled?' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { described_class.new(hook, data, :push_hooks, force: forced) }
+
+    let(:hook) { double(executable?: executable, allow_local_requests?: false) }
+
+    where(:forced, :executable, :disabled) do
+      false | true | false
+      false | false | true
+      true | true | false
+      true | false | false
+    end
+
+    with_them do
+      it { is_expected.to have_attributes(disabled?: disabled) }
+    end
+  end
+
   describe '#execute' do
     let!(:uuid) { SecureRandom.uuid }
     let(:headers) do
@@ -129,7 +148,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
     end
 
     it 'does not execute disabled hooks' do
-      project_hook.update!(recent_failures: 4)
+      allow(service_instance).to receive(:disabled?).and_return(true)
 
       expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' })
     end
@@ -251,6 +270,20 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
           stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success')
         end
 
+        context 'when forced' do
+          let(:service_instance) { described_class.new(project_hook, data, :push_hooks, force: true) }
+
+          it 'logs execution inline' do
+            expect(::WebHooks::LogExecutionWorker).not_to receive(:perform_async)
+            expect(::WebHooks::LogExecutionService)
+              .to receive(:new)
+              .with(hook: project_hook, log_data: Hash, response_category: :ok)
+              .and_return(double(execute: nil))
+
+            run_service
+          end
+        end
+
         it 'log successful execution' do
           run_service