diff options
Diffstat (limited to 'spec/models/hooks')
-rw-r--r-- | spec/models/hooks/active_hook_filter_spec.rb | 113 | ||||
-rw-r--r-- | spec/models/hooks/project_hook_spec.rb | 20 | ||||
-rw-r--r-- | spec/models/hooks/system_hook_spec.rb | 11 | ||||
-rw-r--r-- | spec/models/hooks/web_hook_log_spec.rb | 19 | ||||
-rw-r--r-- | spec/models/hooks/web_hook_spec.rb | 142 |
5 files changed, 228 insertions, 77 deletions
diff --git a/spec/models/hooks/active_hook_filter_spec.rb b/spec/models/hooks/active_hook_filter_spec.rb index 1f693ce9fde..47c0fbdb106 100644 --- a/spec/models/hooks/active_hook_filter_spec.rb +++ b/spec/models/hooks/active_hook_filter_spec.rb @@ -6,65 +6,102 @@ RSpec.describe ActiveHookFilter do subject(:filter) { described_class.new(hook) } describe '#matches?' do - context 'for push event hooks' do + using RSpec::Parameterized::TableSyntax + + context 'for various types of branch_filter' do let(:hook) do - create(:project_hook, push_events: true, push_events_branch_filter: branch_filter) + build(:project_hook, push_events: true, issues_events: true) end - context 'branch filter is specified' do - let(:branch_filter) { 'master' } + where(:branch_filter_strategy, :branch_filter, :ref, :expected_matches?) do + 'all_branches' | 'master' | 'refs/heads/master' | true + 'all_branches' | '' | 'refs/heads/master' | true + 'all_branches' | nil | 'refs/heads/master' | true + 'all_branches' | '.*' | 'refs/heads/master' | true + 'wildcard' | 'master' | 'refs/heads/master' | true + 'wildcard' | 'master' | 'refs/heads/my_branch' | false + 'wildcard' | 'features/*' | 'refs/heads/features/my-branch' | true + 'wildcard' | 'features/*' | 'refs/heads/features/my-branch/something' | true + 'wildcard' | 'features/*' | 'refs/heads/master' | false + 'wildcard' | nil | 'refs/heads/master' | true + 'wildcard' | '' | 'refs/heads/master' | true + 'regex' | 'master' | 'refs/heads/master' | true + 'regex' | 'master' | 'refs/heads/my_branch' | false + 'regex' | 'features/*' | 'refs/heads/xxxx/features/my-branch' | true + 'regex' | 'features/*' | 'refs/heads/features/' | true + 'regex' | 'features/*' | 'refs/heads/features' | true + 'regex' | 'features/.*' | 'refs/heads/features/my-branch' | true + 'regex' | 'features/.*' | 'refs/heads/features/my-branch/something' | true + 'regex' | 'features/.*' | 'refs/heads/master' | false + 'regex' | '(feature|dev)' | 'refs/heads/feature' | true + 'regex' | '(feature|dev)' | 'refs/heads/dev' | true + 'regex' | '(feature|dev)' | 'refs/heads/master' | false + 'regex' | nil | 'refs/heads/master' | true + 'regex' | '' | 'refs/heads/master' | true + end - it 'returns true if branch matches' do - expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true + with_them do + before do + hook.assign_attributes( + push_events_branch_filter: branch_filter, + branch_filter_strategy: branch_filter_strategy + ) end - it 'returns false if branch does not match' do - expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to be false - end + it { expect(filter.matches?(:push_hooks, { ref: ref })).to be expected_matches? } + it { expect(filter.matches?(:issues_events, { ref: ref })).to be true } + end - it 'returns false if ref is nil' do - expect(filter.matches?(:push_hooks, {})).to be false + context 'when the branch filter is a invalid regex' do + let(:hook) do + build( + :project_hook, + push_events: true, + push_events_branch_filter: 'master', + branch_filter_strategy: 'regex' + ) end - context 'branch filter contains wildcard' do - let(:branch_filter) { 'features/*' } - - it 'returns true if branch matches' do - expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch' })).to be true - expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch/something' })).to be true - end - - it 'returns false if branch does not match' do - expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be false - end + before do + allow(hook).to receive(:push_events_branch_filter).and_return("invalid-regex[") end - end - context 'branch filter is not specified' do - let(:branch_filter) { nil } - - it 'returns true' do - expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true - end + it { expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be false } end - context 'branch filter is empty string' do - let(:branch_filter) { '' } + context 'when the branch filter is not properly set to nil' do + let(:hook) do + build( + :project_hook, + push_events: true, + branch_filter_strategy: 'all_branches' + ) + end - it 'acts like branch is not specified' do - expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true + before do + allow(hook).to receive(:push_events_branch_filter).and_return("master") end + + it { expect(filter.matches?(:push_hooks, { ref: 'refs/heads/feature1' })).to be true } end end - context 'for non-push-events hooks' do - let(:hook) do - create(:project_hook, issues_events: true, push_events: false, push_events_branch_filter: '') + context 'when feature flag is disabled' do + before do + stub_feature_flags(enhanced_webhook_support_regex: false) end - it 'returns true as branch filters are not yet supported for these' do - expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to be true + let(:hook) do + build( + :project_hook, + push_events: true, + push_events_branch_filter: '(master)', + branch_filter_strategy: 'regex' + ) end + + it { expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be false } + it { expect(filter.matches?(:push_hooks, { ref: 'refs/heads/(master)' })).to be true } end end end diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index 923a6f92424..3d8c377ab21 100644 --- a/spec/models/hooks/project_hook_spec.rb +++ b/spec/models/hooks/project_hook_spec.rb @@ -12,14 +12,14 @@ RSpec.describe ProjectHook do end it_behaves_like 'includes Limitable concern' do - subject { build(:project_hook, project: create(:project)) } + subject { build(:project_hook) } end describe '.for_projects' do it 'finds related project hooks' do - hook_a = create(:project_hook) - hook_b = create(:project_hook) - hook_c = create(:project_hook) + hook_a = create(:project_hook, project: build(:project)) + hook_b = create(:project_hook, project: build(:project)) + hook_c = create(:project_hook, project: build(:project)) expect(described_class.for_projects([hook_a.project, hook_b.project])) .to contain_exactly(hook_a, hook_b) @@ -30,16 +30,18 @@ RSpec.describe ProjectHook do describe '.push_hooks' do it 'returns hooks for push events only' do - hook = create(:project_hook, push_events: true) - create(:project_hook, push_events: false) + project = build(:project) + hook = create(:project_hook, project: project, push_events: true) + create(:project_hook, project: project, push_events: false) expect(described_class.push_hooks).to eq([hook]) end end describe '.tag_push_hooks' do it 'returns hooks for tag push events only' do - hook = create(:project_hook, tag_push_events: true) - create(:project_hook, tag_push_events: false) + project = build(:project) + hook = create(:project_hook, project: project, tag_push_events: true) + create(:project_hook, project: project, tag_push_events: false) expect(described_class.tag_push_hooks).to eq([hook]) end end @@ -65,7 +67,7 @@ RSpec.describe ProjectHook do end describe '#update_last_failure', :clean_gitlab_redis_shared_state do - let_it_be(:hook) { create(:project_hook) } + let(:hook) { build(:project_hook) } it 'is a method of this class' do expect { hook.update_last_failure }.not_to raise_error diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb index f4786083b75..ba94730b1dd 100644 --- a/spec/models/hooks/system_hook_spec.rb +++ b/spec/models/hooks/system_hook_spec.rb @@ -4,7 +4,7 @@ require "spec_helper" RSpec.describe SystemHook do context 'default attributes' do - let(:system_hook) { build(:system_hook) } + let(:system_hook) { described_class.new } it 'sets defined default parameters' do attrs = { @@ -32,10 +32,10 @@ RSpec.describe SystemHook do end describe "execute", :sidekiq_might_not_need_inline do - let(:system_hook) { create(:system_hook) } - let(:user) { create(:user) } - let(:project) { create(:project, namespace: user.namespace) } - let(:group) { create(:group) } + let_it_be(:system_hook) { create(:system_hook) } + let_it_be(:user) { create(:user) } + let(:project) { build(:project, namespace: user.namespace) } + let(:group) { build(:group) } let(:params) do { name: 'John Doe', username: 'jduser', email: 'jg@example.com', password: User.random_password } end @@ -145,6 +145,7 @@ RSpec.describe SystemHook do end it 'group member update hook' do + group = create(:group) group.add_guest(user) group.add_maintainer(user) diff --git a/spec/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb index 3441dfda7d6..fafca144cae 100644 --- a/spec/models/hooks/web_hook_log_spec.rb +++ b/spec/models/hooks/web_hook_log_spec.rb @@ -12,7 +12,7 @@ RSpec.describe WebHookLog do it { is_expected.to validate_presence_of(:web_hook) } describe '.recent' do - let(:hook) { create(:project_hook) } + let(:hook) { build(:project_hook) } it 'does not return web hook logs that are too old' do create(:web_hook_log, web_hook: hook, created_at: 10.days.ago) @@ -30,8 +30,10 @@ RSpec.describe WebHookLog do end describe '#save' do + let(:hook) { build(:project_hook) } + context 'with basic auth credentials' do - let(:web_hook_log) { build(:web_hook_log, url: 'http://test:123@example.com') } + let(:web_hook_log) { build(:web_hook_log, web_hook: hook, url: 'http://test:123@example.com') } subject { web_hook_log.save! } @@ -45,9 +47,9 @@ RSpec.describe WebHookLog do end context "with users' emails" do - let(:author) { create(:user) } - let(:user) { create(:user) } - let(:web_hook_log) { create(:web_hook_log, request_data: data) } + let(:author) { build(:user) } + let(:user) { build(:user) } + let(:web_hook_log) { create(:web_hook_log, web_hook: hook, request_data: data) } let(:data) do { user: { @@ -93,11 +95,12 @@ RSpec.describe WebHookLog do end describe '.delete_batch_for' do - let(:hook) { create(:project_hook) } + let_it_be(:hook) { build(:project_hook) } + let_it_be(:hook2) { build(:project_hook) } - before do + before_all do create_list(:web_hook_log, 3, web_hook: hook) - create_list(:web_hook_log, 3) + create_list(:web_hook_log, 3, web_hook: hook2) end subject { described_class.delete_batch_for(hook, batch_size: batch_size) } diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index da8c10b67a6..db854670cc3 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -34,6 +34,11 @@ RSpec.describe WebHook do it { is_expected.to allow_value({ 'x' => ('a' * 100) }).for(:url_variables) } it { is_expected.to allow_value({ 'foo' => 'bar', 'bar' => 'baz' }).for(:url_variables) } it { is_expected.to allow_value((1..20).to_h { ["k#{_1}", 'value'] }).for(:url_variables) } + it { is_expected.to allow_value({ 'MY-TOKEN' => 'bar' }).for(:url_variables) } + it { is_expected.to allow_value({ 'my_secr3t-token' => 'bar' }).for(:url_variables) } + it { is_expected.to allow_value({ 'x-y-z' => 'bar' }).for(:url_variables) } + it { is_expected.to allow_value({ 'x_y_z' => 'bar' }).for(:url_variables) } + it { is_expected.to allow_value({ 'f.o.o' => 'bar' }).for(:url_variables) } it { is_expected.not_to allow_value([]).for(:url_variables) } it { is_expected.not_to allow_value({ 'foo' => 1 }).for(:url_variables) } @@ -45,6 +50,10 @@ RSpec.describe WebHook do it { is_expected.not_to allow_value({ '' => 'foo' }).for(:url_variables) } it { is_expected.not_to allow_value({ '1foo' => 'foo' }).for(:url_variables) } it { is_expected.not_to allow_value((1..21).to_h { ["k#{_1}", 'value'] }).for(:url_variables) } + it { is_expected.not_to allow_value({ 'MY--TOKEN' => 'foo' }).for(:url_variables) } + it { is_expected.not_to allow_value({ 'MY__SECRET' => 'foo' }).for(:url_variables) } + it { is_expected.not_to allow_value({ 'x-_y' => 'foo' }).for(:url_variables) } + it { is_expected.not_to allow_value({ 'x..y' => 'foo' }).for(:url_variables) } end describe 'url' do @@ -83,7 +92,7 @@ RSpec.describe WebHook do subject { hook } before do - hook.url_variables = { 'one' => 'a', 'two' => 'b' } + hook.url_variables = { 'one' => 'a', 'two' => 'b', 'url' => 'http://example.com' } end it { is_expected.to allow_value('http://example.com').for(:url) } @@ -92,6 +101,8 @@ RSpec.describe WebHook do it { is_expected.to allow_value('http://example.com/{two}').for(:url) } it { is_expected.to allow_value('http://user:s3cret@example.com/{two}').for(:url) } it { is_expected.to allow_value('http://{one}:{two}@example.com').for(:url) } + it { is_expected.to allow_value('http://{one}').for(:url) } + it { is_expected.to allow_value('{url}').for(:url) } it { is_expected.not_to allow_value('http://example.com/{one}/{two}/{three}').for(:url) } it { is_expected.not_to allow_value('http://example.com/{foo}').for(:url) } @@ -113,24 +124,81 @@ RSpec.describe WebHook do end describe 'push_events_branch_filter' do - it { is_expected.to allow_values("good_branch_name", "another/good-branch_name").for(:push_events_branch_filter) } - it { is_expected.to allow_values("").for(:push_events_branch_filter) } - it { is_expected.not_to allow_values("bad branch name", "bad~branchname").for(:push_events_branch_filter) } - - it 'gets rid of whitespace' do - hook.push_events_branch_filter = ' branch ' - hook.save! + before do + subject.branch_filter_strategy = strategy + end + + context 'with "all branches" strategy' do + let(:strategy) { 'all_branches' } + + it { + is_expected.to allow_values( + "good_branch_name", + "another/good-branch_name", + "good branch name", + "good~branchname", + "good_branchname(", + "good_branchname[", + "" + ).for(:push_events_branch_filter) + } + end + + context 'with "wildcard" strategy' do + let(:strategy) { 'wildcard' } + + it { + is_expected.to allow_values( + "good_branch_name", + "another/good-branch_name", + "good_branch_name(", + "" + ).for(:push_events_branch_filter) + } + + it { + is_expected.not_to allow_values( + "bad branch name", + "bad~branchname", + "bad_branch_name[" + ).for(:push_events_branch_filter) + } + + it 'gets rid of whitespace' do + hook.push_events_branch_filter = ' branch ' + hook.save! + + expect(hook.push_events_branch_filter).to eq('branch') + end - expect(hook.push_events_branch_filter).to eq('branch') + it 'stores whitespace only as empty' do + hook.push_events_branch_filter = ' ' + hook.save! + expect(hook.push_events_branch_filter).to eq('') + end end - it 'stores whitespace only as empty' do - hook.push_events_branch_filter = ' ' - hook.save! + context 'with "regex" strategy' do + let(:strategy) { 'regex' } - expect(hook.push_events_branch_filter).to eq('') + it { + is_expected.to allow_values( + "good_branch_name", + "another/good-branch_name", + "good branch name", + "good~branch~name", + "" + ).for(:push_events_branch_filter) + } + + it { is_expected.not_to allow_values("bad_branch_name(", "bad_branch_name[").for(:push_events_branch_filter) } end end + + it "only consider these branch filter strategies are valid" do + expected_valid_types = %w[all_branches regex wildcard] + expect(described_class.branch_filter_strategies.keys).to contain_exactly(*expected_valid_types) + end end describe 'encrypted attributes' do @@ -141,7 +209,7 @@ RSpec.describe WebHook do describe '.web_hooks_disable_failed?' do it 'returns true when feature is enabled for parent' do - second_hook = build(:project_hook, project: create(:project)) + second_hook = build(:project_hook) stub_feature_flags(web_hooks_disable_failed: [false, second_hook.project]) expect(described_class.web_hooks_disable_failed?(hook)).to eq(false) @@ -242,7 +310,7 @@ RSpec.describe WebHook do end describe '#executable?' do - let(:web_hook) { create(:project_hook, project: project) } + let_it_be_with_reload(:web_hook) { create(:project_hook, project: project) } where(:recent_failures, :not_until, :executable) do [ @@ -389,7 +457,7 @@ RSpec.describe WebHook do end end - describe 'backoff!' do + 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) @@ -400,6 +468,26 @@ RSpec.describe WebHook do end end + context 'when the recent failure value is the max value of a smallint' do + before do + hook.update!(recent_failures: 32767, disabled_until: 1.hour.ago) + end + + it 'reduces to MAX_FAILURES' do + expect { hook.backoff! }.to change(hook, :recent_failures).to(described_class::MAX_FAILURES) + end + end + + context 'when the recent failure value is MAX_FAILURES' do + before do + hook.update!(recent_failures: described_class::MAX_FAILURES, disabled_until: 1.hour.ago) + end + + it 'does not change recent_failures' do + expect { hook.backoff! }.not_to change(hook, :recent_failures) + end + end + context 'when we have exhausted the grace period' do before do hook.update!(recent_failures: described_class::FAILURE_THRESHOLD) @@ -459,11 +547,21 @@ RSpec.describe WebHook do end end - describe 'failed!' do + describe '#failed!' do it 'increments the failure count' do expect { hook.failed! }.to change(hook, :recent_failures).by(1) end + context 'when the recent failure value is the max value of a smallint' do + before do + hook.update!(recent_failures: 32767) + end + + it 'does not change recent_failures' do + expect { hook.failed! }.not_to change(hook, :recent_failures) + end + end + it 'does not update the hook if the the failure count exceeds the maximum value' do hook.recent_failures = described_class::MAX_FAILURES @@ -670,4 +768,14 @@ RSpec.describe WebHook 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 |