diff options
Diffstat (limited to 'spec/services/web_hook_service_spec.rb')
| -rw-r--r-- | spec/services/web_hook_service_spec.rb | 244 |
1 files changed, 225 insertions, 19 deletions
diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 2fe72ab31c2..b3fd4e33640 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -5,8 +5,9 @@ require 'spec_helper' RSpec.describe WebHookService do include StubRequests - let(:project) { create(:project) } - let(:project_hook) { create(:project_hook) } + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:project_hook) { create(:project_hook, project: project) } + let(:headers) do { 'Content-Type' => 'application/json', @@ -21,6 +22,10 @@ RSpec.describe WebHookService do let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } + around do |example| + travel_to(Time.current) { example.run } + end + describe '#initialize' do before do stub_application_setting(setting_name => setting) @@ -56,12 +61,8 @@ RSpec.describe WebHookService do end describe '#execute' do - before do - project.hooks << [project_hook] - end - context 'when token is defined' do - let(:project_hook) { create(:project_hook, :token) } + let_it_be(:project_hook) { create(:project_hook, :token) } it 'POSTs to the webhook URL' do stub_full_request(project_hook.url, method: :post) @@ -85,8 +86,8 @@ RSpec.describe WebHookService do end context 'when auth credentials are present' do - let(:url) {'https://example.org'} - let(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') } + let_it_be(:url) {'https://example.org'} + let_it_be(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') } it 'uses the credentials' do stub_full_request(url, method: :post) @@ -100,8 +101,8 @@ RSpec.describe WebHookService do end context 'when auth credentials are partial present' do - let(:url) {'https://example.org'} - let(:project_hook) { create(:project_hook, url: 'https://demo@example.org/') } + let_it_be(:url) {'https://example.org'} + let_it_be(:project_hook) { create(:project_hook, url: 'https://demo@example.org/') } it 'uses the credentials anyways' do stub_full_request(url, method: :post) @@ -120,10 +121,21 @@ RSpec.describe WebHookService do expect { service_instance.execute }.to raise_error(StandardError) end + it 'does not execute disabled hooks' do + project_hook.update!(recent_failures: 4) + + expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' }) + end + it 'handles exceptions' do - exceptions = [SocketError, OpenSSL::SSL::SSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, Errno::EHOSTUNREACH, Net::OpenTimeout, Net::ReadTimeout, Gitlab::HTTP::BlockedUrlError, Gitlab::HTTP::RedirectionTooDeep] + exceptions = [ + SocketError, OpenSSL::SSL::SSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, + Errno::EHOSTUNREACH, Net::OpenTimeout, Net::ReadTimeout, + Gitlab::HTTP::BlockedUrlError, Gitlab::HTTP::RedirectionTooDeep + ] exceptions.each do |exception_class| exception = exception_class.new('Exception message') + project_hook.enable! stub_full_request(project_hook.url, method: :post).to_raise(exception) expect(service_instance.execute).to eq({ status: :error, message: exception.to_s }) @@ -132,7 +144,7 @@ RSpec.describe WebHookService do end context 'when url is not encoded' do - let(:project_hook) { create(:project_hook, url: 'http://server.com/my path/') } + let_it_be(:project_hook) { create(:project_hook, url: 'http://server.com/my path/') } it 'handles exceptions' do expect(service_instance.execute).to eq(status: :error, message: 'bad URI(is not URI?): "http://server.com/my path/"') @@ -166,10 +178,11 @@ RSpec.describe WebHookService do context 'with success' do before do stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') - service_instance.execute end it 'log successful execution' do + service_instance.execute + expect(hook_log.trigger).to eq('push_hooks') expect(hook_log.url).to eq(project_hook.url) expect(hook_log.request_headers).to eq(headers) @@ -178,15 +191,81 @@ RSpec.describe WebHookService do expect(hook_log.execution_duration).to be > 0 expect(hook_log.internal_error_message).to be_nil end + + it 'does not increment the failure count' do + expect { service_instance.execute }.not_to change(project_hook, :recent_failures) + end + + it 'does not change the disabled_until attribute' do + expect { service_instance.execute }.not_to change(project_hook, :disabled_until) + end + + context 'when the hook had previously failed' do + before do + project_hook.update!(recent_failures: 2) + end + + it 'resets the failure count' do + expect { service_instance.execute }.to change(project_hook, :recent_failures).to(0) + end + end + end + + context 'with bad request' do + before do + stub_full_request(project_hook.url, method: :post).to_return(status: 400, body: 'Bad request') + end + + it 'logs failed execution' do + service_instance.execute + + expect(hook_log).to have_attributes( + trigger: eq('push_hooks'), + url: eq(project_hook.url), + request_headers: eq(headers), + response_body: eq('Bad request'), + response_status: eq('400'), + execution_duration: be > 0, + internal_error_message: be_nil + ) + end + + it 'increments the failure count' do + expect { service_instance.execute }.to change(project_hook, :recent_failures).by(1) + end + + it 'does not change the disabled_until attribute' do + expect { service_instance.execute }.not_to change(project_hook, :disabled_until) + end + + it 'does not allow the failure count to overflow' do + project_hook.update!(recent_failures: 32767) + + expect { service_instance.execute }.not_to change(project_hook, :recent_failures) + end + + context 'when the web_hooks_disable_failed FF is disabled' do + before do + # Hook will only be executed if the flag is disabled. + stub_feature_flags(web_hooks_disable_failed: false) + end + + it 'does not allow the failure count to overflow' do + project_hook.update!(recent_failures: 32767) + + expect { service_instance.execute }.not_to change(project_hook, :recent_failures) + end + end end context 'with exception' do before do stub_full_request(project_hook.url, method: :post).to_raise(SocketError.new('Some HTTP Post error')) - service_instance.execute end it 'log failed execution' do + service_instance.execute + expect(hook_log.trigger).to eq('push_hooks') expect(hook_log.url).to eq(project_hook.url) expect(hook_log.request_headers).to eq(headers) @@ -195,6 +274,47 @@ RSpec.describe WebHookService do expect(hook_log.execution_duration).to be > 0 expect(hook_log.internal_error_message).to eq('Some HTTP Post error') end + + it 'does not increment the failure count' do + expect { service_instance.execute }.not_to change(project_hook, :recent_failures) + end + + it 'sets the disabled_until attribute' do + expect { service_instance.execute } + .to change(project_hook, :disabled_until).to(project_hook.next_backoff.from_now) + end + + it 'increases the backoff count' do + expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1) + end + + context 'when the previous cool-off was near the maximum' do + before do + project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 8) + end + + it 'sets the disabled_until attribute' do + expect { service_instance.execute }.to change(project_hook, :disabled_until).to(1.day.from_now) + end + + it 'sets the last_backoff attribute' do + expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1) + end + end + + context 'when we have backed-off many many times' do + before do + project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 365) + end + + it 'sets the disabled_until attribute' do + expect { service_instance.execute }.to change(project_hook, :disabled_until).to(1.day.from_now) + end + + it 'sets the last_backoff attribute' do + expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1) + end + end end context 'with unsafe response body' do @@ -217,12 +337,98 @@ RSpec.describe WebHookService do end describe '#async_execute' do - let(:system_hook) { create(:system_hook) } + def expect_to_perform_worker(hook) + expect(WebHookWorker).to receive(:perform_async).with(hook.id, data, 'push_hooks') + end + + def expect_to_rate_limit(hook, threshold:, throttled: false) + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:web_hook_calls, scope: [hook], threshold: threshold) + .and_return(throttled) + end + + context 'when rate limiting is not configured' do + it 'queues a worker without tracking the call' do + expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) + expect_to_perform_worker(project_hook) + + service_instance.async_execute + end + end + + context 'when rate limiting is configured' do + let_it_be(:threshold) { 3 } + let_it_be(:plan_limits) { create(:plan_limits, :default_plan, web_hook_calls: threshold) } + + it 'queues a worker and tracks the call' do + expect_to_rate_limit(project_hook, threshold: threshold) + expect_to_perform_worker(project_hook) + + service_instance.async_execute + end + + context 'when the hook is throttled (via mock)' do + before do + expect_to_rate_limit(project_hook, threshold: threshold, throttled: true) + end + + it 'does not queue a worker and logs an error' do + expect(WebHookWorker).not_to receive(:perform_async) - it 'enqueue WebHookWorker' do - expect(WebHookWorker).to receive(:perform_async).with(project_hook.id, data, 'push_hooks') + payload = { + message: 'Webhook rate limit exceeded', + hook_id: project_hook.id, + hook_type: 'ProjectHook', + hook_name: 'push_hooks' + } - described_class.new(project_hook, data, 'push_hooks').async_execute + expect(Gitlab::AuthLogger).to receive(:error).with(payload) + expect(Gitlab::AppLogger).to receive(:error).with(payload) + + service_instance.async_execute + end + end + + context 'when the hook is throttled (via Redis)', :clean_gitlab_redis_cache do + before do + # Set a high interval to avoid intermittent failures in CI + allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits).and_return( + web_hook_calls: { interval: 1.day } + ) + + expect_to_perform_worker(project_hook).exactly(threshold).times + + threshold.times { service_instance.async_execute } + end + + it 'stops queueing workers and logs errors' do + expect(Gitlab::AuthLogger).to receive(:error).twice + expect(Gitlab::AppLogger).to receive(:error).twice + + 2.times { service_instance.async_execute } + end + + it 'still queues workers for other hooks' do + other_hook = create(:project_hook) + + expect_to_perform_worker(other_hook) + + described_class.new(other_hook, data, :push_hooks).async_execute + end + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(web_hooks_rate_limit: false) + end + + it 'queues a worker without tracking the call' do + expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) + expect_to_perform_worker(project_hook) + + service_instance.async_execute + end + end end end end |