diff options
Diffstat (limited to 'spec/models/hooks')
| -rw-r--r-- | spec/models/hooks/project_hook_spec.rb | 120 | ||||
| -rw-r--r-- | spec/models/hooks/service_hook_spec.rb | 38 | ||||
| -rw-r--r-- | spec/models/hooks/system_hook_spec.rb | 12 | ||||
| -rw-r--r-- | spec/models/hooks/web_hook_log_spec.rb | 31 | ||||
| -rw-r--r-- | spec/models/hooks/web_hook_spec.rb | 402 |
5 files changed, 245 insertions, 358 deletions
diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index 3d8c377ab21..c3484c4a42c 100644 --- a/spec/models/hooks/project_hook_spec.rb +++ b/spec/models/hooks/project_hook_spec.rb @@ -2,7 +2,19 @@ require 'spec_helper' -RSpec.describe ProjectHook do +RSpec.describe ProjectHook, feature_category: :integrations do + include_examples 'a hook that gets automatically disabled on failure' do + let_it_be(:project) { create(:project) } + + let(:hook) { build(:project_hook, project: project) } + let(:hook_factory) { :project_hook } + let(:default_factory_arguments) { { project: project } } + + def find_hooks + project.hooks + end + end + describe 'associations' do it { is_expected.to belong_to :project } end @@ -67,17 +79,91 @@ RSpec.describe ProjectHook do end describe '#update_last_failure', :clean_gitlab_redis_shared_state do - let(:hook) { build(:project_hook) } + let_it_be(:hook) { create(:project_hook) } + + def last_failure + Gitlab::Redis::SharedState.with do |redis| + redis.get(hook.project.last_failure_redis_key) + end + end + + def any_failed? + Gitlab::Redis::SharedState.with do |redis| + Gitlab::Utils.to_boolean(redis.get(hook.project.web_hook_failure_redis_key)) + end + end it 'is a method of this class' do expect { hook.update_last_failure }.not_to raise_error end context 'when the hook is executable' do - it 'does not update the state' do - expect(Gitlab::Redis::SharedState).not_to receive(:with) + let(:redis_key) { hook.project.web_hook_failure_redis_key } + + def redis_value + any_failed? + end + + context 'when the state was previously failing' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set(redis_key, true) + end + end + + it 'does update the state' do + expect { hook.update_last_failure }.to change { redis_value }.to(false) + end + + context 'when there is another failing sibling hook' do + before do + create(:project_hook, :permanently_disabled, project: hook.project) + end + + it 'does not update the state' do + expect { hook.update_last_failure }.not_to change { redis_value }.from(true) + end + + it 'caches the current value' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:set).with(redis_key, 'true', ex: 1.hour).and_call_original + end + + hook.update_last_failure + end + end + end + + context 'when the state was previously unknown' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.del(redis_key) + end + end + + it 'does not update the state' do + expect { hook.update_last_failure }.not_to change { redis_value }.from(nil) + end + end + + context 'when the state was previously not failing' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set(redis_key, false) + end + end - hook.update_last_failure + it 'does not update the state' do + expect { hook.update_last_failure }.not_to change { redis_value }.from(false) + end + + it 'does not cache the current value' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).not_to receive(:set) + end + + hook.update_last_failure + end end end @@ -86,28 +172,34 @@ RSpec.describe ProjectHook do allow(hook).to receive(:executable?).and_return(false) end - def last_failure - Gitlab::Redis::SharedState.with do |redis| - redis.get("web_hooks:last_failure:project-#{hook.project.id}") - end - end - context 'there is no prior value', :freeze_time do - it 'updates the state' do + it 'updates last_failure' do expect { hook.update_last_failure }.to change { last_failure }.to(Time.current) end + + it 'updates any_failed?' do + expect { hook.update_last_failure }.to change { any_failed? }.to(true) + end end - context 'there is a prior value, from before now' do + context 'when there is a prior last_failure, from before now' do it 'updates the state' do the_future = 1.minute.from_now - hook.update_last_failure travel_to(the_future) do expect { hook.update_last_failure }.to change { last_failure }.to(the_future.iso8601) end end + + it 'does not change the failing state' do + the_future = 1.minute.from_now + hook.update_last_failure + + travel_to(the_future) do + expect { hook.update_last_failure }.not_to change { any_failed? }.from(true) + end + end end context 'there is a prior value, from after now' do diff --git a/spec/models/hooks/service_hook_spec.rb b/spec/models/hooks/service_hook_spec.rb index 2ece04c7158..e52af4a32b0 100644 --- a/spec/models/hooks/service_hook_spec.rb +++ b/spec/models/hooks/service_hook_spec.rb @@ -2,7 +2,17 @@ require 'spec_helper' -RSpec.describe ServiceHook do +RSpec.describe ServiceHook, feature_category: :integrations do + it_behaves_like 'a hook that does not get automatically disabled on failure' do + let(:hook) { create(:service_hook) } + let(:hook_factory) { :service_hook } + let(:default_factory_arguments) { {} } + + def find_hooks + described_class.all + end + end + describe 'associations' do it { is_expected.to belong_to :integration } end @@ -11,32 +21,6 @@ RSpec.describe ServiceHook do it { is_expected.to validate_presence_of(:integration) } end - describe 'executable?' do - let!(:hooks) do - [ - [0, Time.current], - [0, 1.minute.from_now], - [1, 1.minute.from_now], - [3, 1.minute.from_now], - [4, nil], - [4, 1.day.ago], - [4, 1.minute.from_now], - [0, nil], - [0, 1.day.ago], - [1, nil], - [1, 1.day.ago], - [3, nil], - [3, 1.day.ago] - ].map do |(recent_failures, disabled_until)| - create(:service_hook, recent_failures: recent_failures, disabled_until: disabled_until) - end - end - - it 'is always true' do - expect(hooks).to all(be_executable) - end - end - describe 'execute' do let(:hook) { build(:service_hook) } let(:data) { { key: 'value' } } diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb index ba94730b1dd..edb307148b6 100644 --- a/spec/models/hooks/system_hook_spec.rb +++ b/spec/models/hooks/system_hook_spec.rb @@ -2,7 +2,17 @@ require "spec_helper" -RSpec.describe SystemHook do +RSpec.describe SystemHook, feature_category: :integrations do + it_behaves_like 'a hook that does not get automatically disabled on failure' do + let(:hook) { create(:system_hook) } + let(:hook_factory) { :system_hook } + let(:default_factory_arguments) { {} } + + def find_hooks + described_class.all + end + end + context 'default attributes' do let(:system_hook) { described_class.new } diff --git a/spec/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb index 2f0bfbd4fed..5be2b2d3bb0 100644 --- a/spec/models/hooks/web_hook_log_spec.rb +++ b/spec/models/hooks/web_hook_log_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe WebHookLog do +RSpec.describe WebHookLog, feature_category: :integrations do it { is_expected.to belong_to(:web_hook) } it { is_expected.to serialize(:request_headers).as(Hash) } @@ -94,6 +94,35 @@ RSpec.describe WebHookLog do end end + describe 'before_save' do + describe '#set_url_hash' do + let(:web_hook_log) { build(:web_hook_log, interpolated_url: interpolated_url) } + + subject(:save_web_hook_log) { web_hook_log.save! } + + context 'when interpolated_url is nil' do + let(:interpolated_url) { nil } + + it { expect { save_web_hook_log }.not_to change { web_hook_log.url_hash } } + end + + context 'when interpolated_url has a blank value' do + let(:interpolated_url) { ' ' } + + it { expect { save_web_hook_log }.not_to change { web_hook_log.url_hash } } + end + + context 'when interpolated_url has a value' do + let(:interpolated_url) { 'example@gitlab.com' } + let(:expected_value) { Gitlab::CryptoHelper.sha256(interpolated_url) } + + it 'assigns correct digest value' do + expect { save_web_hook_log }.to change { web_hook_log.url_hash }.from(nil).to(expected_value) + end + end + end + end + describe '.delete_batch_for' do let_it_be(:hook) { build(:project_hook) } let_it_be(:hook2) { build(:project_hook) } diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 75ff917c036..72958a54e10 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -258,7 +258,7 @@ RSpec.describe WebHook, feature_category: :integrations do end describe 'encrypted attributes' do - subject { described_class.encrypted_attributes.keys } + subject { described_class.attr_encrypted_attributes.keys } it { is_expected.to contain_exactly(:token, :url, :url_variables) } end @@ -311,88 +311,6 @@ RSpec.describe WebHook, feature_category: :integrations do end end - describe '.executable/.disabled' do - let!(:not_executable) do - [ - [0, Time.current], - [0, 1.minute.from_now], - [1, 1.minute.from_now], - [3, 1.minute.from_now], - [4, nil], - [4, 1.day.ago], - [4, 1.minute.from_now] - ].map do |(recent_failures, disabled_until)| - create(:project_hook, project: project, recent_failures: recent_failures, disabled_until: disabled_until) - end - end - - let!(:executables) do - [ - [0, nil], - [0, 1.day.ago], - [1, nil], - [1, 1.day.ago], - [3, nil], - [3, 1.day.ago] - ].map do |(recent_failures, disabled_until)| - create(:project_hook, project: project, recent_failures: recent_failures, disabled_until: disabled_until) - end - end - - it 'finds the correct set of project hooks' do - expect(described_class.where(project_id: project.id).executable).to match_array executables - expect(described_class.where(project_id: project.id).disabled).to match_array not_executable - end - end - - describe '#executable?' do - let_it_be_with_reload(:web_hook) { create(:project_hook, project: project) } - - where(:recent_failures, :not_until, :executable) do - [ - [0, :not_set, true], - [0, :past, true], - [0, :future, true], - [0, :now, true], - [1, :not_set, true], - [1, :past, true], - [1, :future, true], - [3, :not_set, true], - [3, :past, true], - [3, :future, true], - [4, :not_set, false], - [4, :past, true], # expired suspension - [4, :now, false], # active suspension - [4, :future, false] # active suspension - ] - end - - with_them do - # Phasing means we cannot put these values in the where block, - # which is not subject to the frozen time context. - let(:disabled_until) do - case not_until - when :not_set - nil - when :past - 1.minute.ago - when :future - 1.minute.from_now - when :now - Time.current - end - end - - before do - web_hook.update!(recent_failures: recent_failures, disabled_until: disabled_until) - end - - it 'has the correct state' do - expect(web_hook.executable?).to eq(executable) - end - end - end - describe '#next_backoff' do context 'when there was no last backoff' do before do @@ -435,50 +353,112 @@ RSpec.describe WebHook, feature_category: :integrations do end end - shared_examples 'is tolerant of invalid records' do - specify do - hook.url = nil + describe '#rate_limited?' do + it 'is false when hook has not been rate limited' do + expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| + expect(rate_limiter).to receive(:rate_limited?).and_return(false) + end - expect(hook).to be_invalid - run_expectation + expect(hook).not_to be_rate_limited + end + + it 'is true when hook has been rate limited' do + expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| + expect(rate_limiter).to receive(:rate_limited?).and_return(true) + end + + expect(hook).to be_rate_limited end end - describe '#enable!' do - it 'makes a hook executable if it was marked as failed' do - hook.recent_failures = 1000 + describe '#rate_limit' do + it 'returns the hook rate limit' do + expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| + expect(rate_limiter).to receive(:limit).and_return(10) + end - expect { hook.enable! }.to change(hook, :executable?).from(false).to(true) + expect(hook.rate_limit).to eq(10) end + end - it 'makes a hook executable if it is currently backed off' do - hook.recent_failures = 1000 - hook.disabled_until = 1.hour.from_now + describe '#to_json' do + it 'does not error' do + expect { hook.to_json }.not_to raise_error + end - expect { hook.enable! }.to change(hook, :executable?).from(false).to(true) + it 'does not contain binary attributes' do + expect(hook.to_json).not_to include('encrypted_url_variables') end + end - it 'does not update hooks unless necessary' do - sql_count = ActiveRecord::QueryRecorder.new { hook.enable! }.count + describe '#interpolated_url' do + subject(:hook) { build(:project_hook, project: project) } - expect(sql_count).to eq(0) + context 'when the hook URL does not contain variables' do + before do + hook.url = 'http://example.com' + end + + it { is_expected.to have_attributes(interpolated_url: hook.url) } + end + + it 'is not vulnerable to malicious input' do + hook.url = 'something%{%<foo>2147483628G}' + hook.url_variables = { 'foo' => '1234567890.12345678' } + + expect(hook).to have_attributes(interpolated_url: hook.url) end - include_examples 'is tolerant of invalid records' do - def run_expectation - hook.recent_failures = 1000 + context 'when the hook URL contains variables' do + before do + hook.url = 'http://example.com/{path}/resource?token={token}' + hook.url_variables = { 'path' => 'abc', 'token' => 'xyz' } + end + + it { is_expected.to have_attributes(interpolated_url: 'http://example.com/abc/resource?token=xyz') } + + context 'when a variable is missing' do + before do + hook.url_variables = { 'path' => 'present' } + end + + it 'raises an error' do + # We expect validations to prevent this entirely - this is not user-error + expect { hook.interpolated_url } + .to raise_error(described_class::InterpolationError, include('Missing key token')) + end + end + + context 'when the URL appears to include percent formatting' do + before do + hook.url = 'http://example.com/%{path}/resource?token=%{token}' + end - expect { hook.enable! }.to change(hook, :executable?).from(false).to(true) + it 'succeeds, interpolates correctly' do + expect(hook.interpolated_url).to eq 'http://example.com/%abc/resource?token=%xyz' + end end end end + describe '#update_last_failure' do + it 'is a method of this class' do + expect { described_class.new(project: project).update_last_failure }.not_to raise_error + end + end + + describe '#masked_token' do + it { expect(hook.masked_token).to be_nil } + + context 'with a token' do + let(:hook) { build(:project_hook, :token, project: project) } + + it { expect(hook.masked_token).to eq described_class::SECRET_MASK } + end + end + describe '#backoff!' do context 'when we have not backed off before' do - it 'does not disable the hook' do - expect { hook.backoff! }.not_to change(hook, :executable?).from(true) - end - it 'increments the recent_failures count' do expect { hook.backoff! }.to change(hook, :recent_failures).by(1) end @@ -517,20 +497,6 @@ RSpec.describe WebHook, feature_category: :integrations do expect { hook.backoff! }.to change(hook, :backoff_count).by(1) end - context 'when the hook is permanently disabled' do - before do - allow(hook).to receive(:permanently_disabled?).and_return(true) - end - - it 'does not set disabled_until' do - expect { hook.backoff! }.not_to change(hook, :disabled_until) - end - - it 'does not increment the backoff count' do - expect { hook.backoff! }.not_to change(hook, :backoff_count) - end - end - context 'when we have backed off MAX_FAILURES times' do before do stub_const("#{described_class}::MAX_FAILURES", 5) @@ -554,12 +520,6 @@ RSpec.describe WebHook, feature_category: :integrations do end end end - - include_examples 'is tolerant of invalid records' do - def run_expectation - expect { hook.backoff! }.to change(hook, :backoff_count).by(1) - end - end end end @@ -585,193 +545,5 @@ RSpec.describe WebHook, feature_category: :integrations do expect(sql_count).to eq(0) end - - include_examples 'is tolerant of invalid records' do - def run_expectation - expect { hook.failed! }.to change(hook, :recent_failures).by(1) - end - end - end - - describe '#disable!' do - it 'disables a hook' do - expect { hook.disable! }.to change(hook, :executable?).from(true).to(false) - end - - include_examples 'is tolerant of invalid records' do - def run_expectation - expect { hook.disable! }.to change(hook, :executable?).from(true).to(false) - end - end - end - - describe '#temporarily_disabled?' do - it 'is false when not temporarily disabled' do - expect(hook).not_to be_temporarily_disabled - end - - it 'allows FAILURE_THRESHOLD initial failures before we back-off' do - described_class::FAILURE_THRESHOLD.times do - hook.backoff! - expect(hook).not_to be_temporarily_disabled - end - - hook.backoff! - expect(hook).to be_temporarily_disabled - end - - context 'when hook has been told to back off' do - before do - hook.update!(recent_failures: described_class::FAILURE_THRESHOLD) - hook.backoff! - end - - it 'is true' do - expect(hook).to be_temporarily_disabled - end - end - end - - describe '#permanently_disabled?' do - it 'is false when not disabled' do - expect(hook).not_to be_permanently_disabled - end - - context 'when hook has been disabled' do - before do - hook.disable! - end - - it 'is true' do - expect(hook).to be_permanently_disabled - end - end - end - - describe '#rate_limited?' do - it 'is false when hook has not been rate limited' do - expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| - expect(rate_limiter).to receive(:rate_limited?).and_return(false) - end - - expect(hook).not_to be_rate_limited - end - - it 'is true when hook has been rate limited' do - expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| - expect(rate_limiter).to receive(:rate_limited?).and_return(true) - end - - expect(hook).to be_rate_limited - end - end - - describe '#rate_limit' do - it 'returns the hook rate limit' do - expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| - expect(rate_limiter).to receive(:limit).and_return(10) - end - - expect(hook.rate_limit).to eq(10) - end - end - - describe '#alert_status' do - subject(:status) { hook.alert_status } - - it { is_expected.to eq :executable } - - context 'when hook has been disabled' do - before do - hook.disable! - end - - it { is_expected.to eq :disabled } - end - - context 'when hook has been backed off' do - before do - hook.update!(recent_failures: described_class::FAILURE_THRESHOLD + 1) - hook.disabled_until = 1.hour.from_now - end - - it { is_expected.to eq :temporarily_disabled } - end - end - - describe '#to_json' do - it 'does not error' do - expect { hook.to_json }.not_to raise_error - end - - it 'does not contain binary attributes' do - expect(hook.to_json).not_to include('encrypted_url_variables') - end - end - - describe '#interpolated_url' do - subject(:hook) { build(:project_hook, project: project) } - - context 'when the hook URL does not contain variables' do - before do - hook.url = 'http://example.com' - end - - it { is_expected.to have_attributes(interpolated_url: hook.url) } - end - - it 'is not vulnerable to malicious input' do - hook.url = 'something%{%<foo>2147483628G}' - hook.url_variables = { 'foo' => '1234567890.12345678' } - - expect(hook).to have_attributes(interpolated_url: hook.url) - end - - context 'when the hook URL contains variables' do - before do - hook.url = 'http://example.com/{path}/resource?token={token}' - hook.url_variables = { 'path' => 'abc', 'token' => 'xyz' } - end - - it { is_expected.to have_attributes(interpolated_url: 'http://example.com/abc/resource?token=xyz') } - - context 'when a variable is missing' do - before do - hook.url_variables = { 'path' => 'present' } - end - - it 'raises an error' do - # We expect validations to prevent this entirely - this is not user-error - expect { hook.interpolated_url } - .to raise_error(described_class::InterpolationError, include('Missing key token')) - end - end - - context 'when the URL appears to include percent formatting' do - before do - hook.url = 'http://example.com/%{path}/resource?token=%{token}' - end - - it 'succeeds, interpolates correctly' do - expect(hook.interpolated_url).to eq 'http://example.com/%abc/resource?token=%xyz' - end - end - end - end - - describe '#update_last_failure' do - it 'is a method of this class' do - expect { described_class.new.update_last_failure }.not_to raise_error - end - end - - describe '#masked_token' do - it { expect(hook.masked_token).to be_nil } - - context 'with a token' do - let(:hook) { build(:project_hook, :token, project: project) } - - it { expect(hook.masked_token).to eq described_class::SECRET_MASK } - end end end |