Add latest changes from gitlab-org/gitlab@master

2 files changed, 145 insertions, 51 deletions

diff --git a/spec/models/hooks/active_hook_filter_spec.rb b/spec/models/hooks/active_hook_filter_spec.rb
index 1f693ce9fde..38e4d9d3a72 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
-      let(:hook) do
-        create(:project_hook, push_events: true, push_events_branch_filter: branch_filter)
+    using RSpec::Parameterized::TableSyntax
+
+    context 'for various types of branch_filter' do
+      let_it_be_with_reload(:hook) do
+        create(: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_it_be(:hook) do
+          create(
+            :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_it_be(:hook) do
+          create(
+            :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_it_be(:hook) do
+        create(
+          :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/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index da8c10b67a6..dd5bf8b99dd 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -113,24 +113,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' }
+
+        it {
+          is_expected.to allow_values(
+            "good_branch_name",
+            "another/good-branch_name",
+            "good branch name",
+            "good~branch~name",
+            ""
+          ).for(:push_events_branch_filter)
+        }
 
-        expect(hook.push_events_branch_filter).to eq('')
+        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