From 07356866b2ce85f4d724c96f14e129fbe6a56963 Mon Sep 17 00:00:00 2001
From: Duana Saskia <starkcoffee@users.noreply.github.com>
Date: Wed, 6 Jun 2018 23:08:29 +0200
Subject: Refactor tests for executing project hooks

So that they test the negative case of hooks that don't have the
specified hook scope
---
 spec/models/project_spec.rb | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 076de06cf99..2ec030daa67 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3670,21 +3670,30 @@ describe Project do
   end
 
   describe '#execute_hooks' do
+    let(:data) { { ref: 'refs/heads/master', data: 'data' } }
     it 'executes the projects hooks with the specified scope' do
-      hook1 = create(:project_hook, merge_requests_events: true, tag_push_events: false)
-      hook2 = create(:project_hook, merge_requests_events: false, tag_push_events: true)
-      project = create(:project, hooks: [hook1, hook2])
+      hook = create(:project_hook, merge_requests_events: false, tag_push_events: true)
+      project = create(:project, hooks: [hook])
 
       expect_any_instance_of(ProjectHook).to receive(:async_execute).once
 
-      project.execute_hooks({}, :tag_push_hooks)
+      project.execute_hooks(data, :tag_push_hooks)
+    end
+
+    it 'does not execute project hooks that dont match the specified scope' do
+      hook = create(:project_hook, merge_requests_events: true, tag_push_events: false)
+      project = create(:project, hooks: [hook])
+
+      expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
+
+      project.execute_hooks(data, :tag_push_hooks)
     end
 
     it 'executes the system hooks with the specified scope' do
-      expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with({ data: 'data' }, :merge_request_hooks)
+      expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(data, :merge_request_hooks)
 
       project = build(:project)
-      project.execute_hooks({ data: 'data' }, :merge_request_hooks)
+      project.execute_hooks(data, :merge_request_hooks)
     end
 
     it 'executes the system hooks when inside a transaction' do
@@ -3699,7 +3708,7 @@ describe Project do
       # actually get to the `after_commit` hook that queues these jobs.
       expect do
         project.transaction do
-          project.execute_hooks({ data: 'data' }, :merge_request_hooks)
+          project.execute_hooks(data, :merge_request_hooks)
         end
       end.not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError
     end
-- 
cgit v1.2.1


From ece6a1ea6ecffdbde5ff7d663f1ad1eb74f78aa6 Mon Sep 17 00:00:00 2001
From: Duana Saskia <starkcoffee@users.noreply.github.com>
Date: Thu, 7 Jun 2018 09:35:17 +0200
Subject: Filter project hooks by branch

Allow specificying a branch filter for a project hook and only trigger
a project hook if either the branch filter is blank or the branch matches.
Only supported for push_events for now.
---
 app/controllers/projects/hooks_controller.rb       |  1 +
 app/models/hooks/active_hook_filter.rb             | 38 ++++++++++++
 app/models/hooks/web_hook.rb                       |  1 +
 app/models/project.rb                              |  3 +-
 app/validators/branch_filter_validator.rb          | 34 +++++++++++
 app/views/shared/web_hooks/_form.html.haml         |  1 +
 .../unreleased/filter-web-hooks-by-branch.yml      |  5 ++
 ...8_add_push_events_branch_filter_to_web_hooks.rb | 12 ++++
 db/schema.rb                                       |  1 +
 doc/api/projects.md                                |  3 +
 doc/user/project/integrations/webhooks.md          |  4 +-
 lib/api/entities.rb                                |  1 +
 lib/api/project_hooks.rb                           |  3 +
 .../projects/settings/integration_settings_spec.rb |  1 +
 .../gitlab/import_export/safe_model_attributes.yml |  1 +
 spec/models/hooks/active_hook_filter_spec.rb       | 67 ++++++++++++++++++++++
 spec/models/hooks/web_hook_spec.rb                 | 20 +++++++
 spec/models/project_spec.rb                        | 17 +++++-
 spec/requests/api/project_hooks_spec.rb            | 14 ++++-
 spec/validators/branch_filter_validator_spec.rb    | 42 ++++++++++++++
 20 files changed, 261 insertions(+), 8 deletions(-)
 create mode 100644 app/models/hooks/active_hook_filter.rb
 create mode 100644 app/validators/branch_filter_validator.rb
 create mode 100644 changelogs/unreleased/filter-web-hooks-by-branch.yml
 create mode 100644 db/migrate/20180607071808_add_push_events_branch_filter_to_web_hooks.rb
 create mode 100644 spec/models/hooks/active_hook_filter_spec.rb
 create mode 100644 spec/validators/branch_filter_validator_spec.rb

diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index 2da2aad9b33..bbf8c7d5cbc 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -66,6 +66,7 @@ class Projects::HooksController < Projects::ApplicationController
       :enable_ssl_verification,
       :token,
       :url,
+      :push_events_branch_filter,
       *ProjectHook.triggers.values
     )
   end
diff --git a/app/models/hooks/active_hook_filter.rb b/app/models/hooks/active_hook_filter.rb
new file mode 100644
index 00000000000..611af971163
--- /dev/null
+++ b/app/models/hooks/active_hook_filter.rb
@@ -0,0 +1,38 @@
+class ActiveHookFilter
+  def initialize(hook)
+    @hook = hook
+  end
+
+  def matches?(hooks_scope, data)
+    return true if hooks_scope != :push_hooks
+    return true if @hook.push_events_branch_filter.blank?
+
+    branch_name = Gitlab::Git.branch_name(data[:ref])
+    exact_match?(branch_name) || wildcard_match?(branch_name)
+  end
+
+  private
+
+  def exact_match?(branch_name)
+    @hook.push_events_branch_filter == branch_name
+  end
+
+  def wildcard_match?(branch_name)
+    return false unless wildcard?
+
+    wildcard_regex === branch_name
+  end
+
+  def wildcard_regex
+    @wildcard_regex ||= begin
+      name = @hook.push_events_branch_filter.gsub('*', 'STAR_DONT_ESCAPE')
+      quoted_name = Regexp.quote(name)
+      regex_string = quoted_name.gsub('STAR_DONT_ESCAPE', '.*?')
+      /\A#{regex_string}\z/
+    end
+  end
+
+  def wildcard?
+    @hook.push_events_branch_filter && @hook.push_events_branch_filter.include?('*')
+  end
+end
diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index f18aadefa5c..20f15c15277 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -9,6 +9,7 @@ class WebHook < ActiveRecord::Base
                                                 allow_local_network: lambda(&:allow_local_requests?) }
 
   validates :token, format: { without: /\n/ }
+  validates :push_events_branch_filter, branch_filter: true
 
   def execute(data, hook_name)
     WebHookService.new(self, data, hook_name).execute
diff --git a/app/models/project.rb b/app/models/project.rb
index 7735f23cb9e..72a53e1a50d 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1163,10 +1163,9 @@ class Project < ActiveRecord::Base
 
   def execute_hooks(data, hooks_scope = :push_hooks)
     run_after_commit_or_now do
-      hooks.hooks_for(hooks_scope).each do |hook|
+      hooks.hooks_for(hooks_scope).select {|hook| ActiveHookFilter.new(hook).matches?(hooks_scope, data)}.each do |hook|
         hook.async_execute(data, hooks_scope.to_s)
       end
-
       SystemHooksService.new.execute_hooks(data, hooks_scope)
     end
   end
diff --git a/app/validators/branch_filter_validator.rb b/app/validators/branch_filter_validator.rb
new file mode 100644
index 00000000000..0965be3d101
--- /dev/null
+++ b/app/validators/branch_filter_validator.rb
@@ -0,0 +1,34 @@
+# BranchFilterValidator
+#
+# Custom validator for branch names. Squishes whitespace and ignores empty
+# string.  This only checks that a string is a valid git branch name. It does
+# not check whether a branch already exists.
+#
+# Example:
+#
+#   class Webhook < ActiveRecord::Base
+#     validates :push_events_branch_filter, branch_name: true
+#   end
+#
+class BranchFilterValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    value.squish! unless value.nil?
+
+    if value.present?
+      value_without_wildcards = value.tr('*', 'x')
+      unless Gitlab::GitRefValidator.validate(value_without_wildcards)
+        record.errors[attribute] << "is not a valid branch name"
+      end
+
+      unless value.length <= 4000
+        record.errors[attribute] << "is longer than the allowed length of 4000 characters."
+      end
+    end
+  end
+
+  private
+
+  def contains_wildcard?(value)
+    value.include?('*')
+  end
+end
diff --git a/app/views/shared/web_hooks/_form.html.haml b/app/views/shared/web_hooks/_form.html.haml
index 07ebb8680d2..9c5b9593bba 100644
--- a/app/views/shared/web_hooks/_form.html.haml
+++ b/app/views/shared/web_hooks/_form.html.haml
@@ -17,6 +17,7 @@
         %strong Push events
       %p.light.ml-1
         This URL will be triggered by a push to the repository
+        = form.text_field :push_events_branch_filter, class: 'form-control', placeholder: 'Branch name or wildcard pattern to trigger on (leave blank for all)'
     %li
       = form.check_box :tag_push_events, class: 'form-check-input'
       = form.label :tag_push_events, class: 'list-label form-check-label ml-1' do
diff --git a/changelogs/unreleased/filter-web-hooks-by-branch.yml b/changelogs/unreleased/filter-web-hooks-by-branch.yml
new file mode 100644
index 00000000000..7bd2c191d7f
--- /dev/null
+++ b/changelogs/unreleased/filter-web-hooks-by-branch.yml
@@ -0,0 +1,5 @@
+---
+title: Add branch filter to project webhooks
+merge_request: 20338
+author: Duana Saskia
+type: added
diff --git a/db/migrate/20180607071808_add_push_events_branch_filter_to_web_hooks.rb b/db/migrate/20180607071808_add_push_events_branch_filter_to_web_hooks.rb
new file mode 100644
index 00000000000..6a69460e611
--- /dev/null
+++ b/db/migrate/20180607071808_add_push_events_branch_filter_to_web_hooks.rb
@@ -0,0 +1,12 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddPushEventsBranchFilterToWebHooks < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :web_hooks, :push_events_branch_filter, :text
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index f1d8f4df3b7..16ae2565c93 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -2239,6 +2239,7 @@ ActiveRecord::Schema.define(version: 20180807153545) do
     t.boolean "repository_update_events", default: false, null: false
     t.boolean "job_events", default: false, null: false
     t.boolean "confidential_note_events"
+    t.text "push_events_branch_filter"
   end
 
   add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
diff --git a/doc/api/projects.md b/doc/api/projects.md
index bda4164ee92..372138e72cc 100644
--- a/doc/api/projects.md
+++ b/doc/api/projects.md
@@ -1334,6 +1334,7 @@ GET /projects/:id/hooks/:hook_id
   "url": "http://example.com/hook",
   "project_id": 3,
   "push_events": true,
+  "push_events_branch_filter": "",
   "issues_events": true,
   "confidential_issues_events": true,
   "merge_requests_events": true,
@@ -1360,6 +1361,7 @@ POST /projects/:id/hooks
 | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
 | `url` | string | yes | The hook URL |
 | `push_events` | boolean | no | Trigger hook on push events |
+| `push_events_branch_filter` | string | no | Trigger hook on push events for matching branches only |
 | `issues_events` | boolean | no | Trigger hook on issues events |
 | `confidential_issues_events` | boolean | no | Trigger hook on confidential issues events |
 | `merge_requests_events` | boolean | no | Trigger hook on merge requests events |
@@ -1385,6 +1387,7 @@ PUT /projects/:id/hooks/:hook_id
 | `hook_id` | integer | yes | The ID of the project hook |
 | `url` | string | yes | The hook URL |
 | `push_events` | boolean | no | Trigger hook on push events |
+| `push_events_branch_filter` | string | no | Trigger hook on push events for matching branches only |
 | `issues_events` | boolean | no | Trigger hook on issues events |
 | `confidential_issues_events` | boolean | no | Trigger hook on confidential issues events |
 | `merge_requests_events` | boolean | no | Trigger hook on merge requests events |
diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md
index 77fa517b5b1..2e7be878da7 100644
--- a/doc/user/project/integrations/webhooks.md
+++ b/doc/user/project/integrations/webhooks.md
@@ -65,8 +65,8 @@ Below are described the supported events.
 
 Triggered when you push to the repository except when pushing tags.
 
-> **Note:** When more than 20 commits are pushed at once, the `commits` web hook
-  attribute will only contain the first 20 for performance reasons. Loading
+> **Note:** When more than 20 commits are pushed at once, the `commits` web hook 
+  attribute will only contain the first 20 for performance reasons. Loading 
   detailed commit data is expensive. Note that despite only 20 commits being
   present in the `commits` attribute, the `total_commits_count` attribute will
   contain the actual total.
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 27f28e1df93..13a43a052a5 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -83,6 +83,7 @@ module API
       expose :project_id, :issues_events, :confidential_issues_events
       expose :note_events, :confidential_note_events, :pipeline_events, :wiki_page_events
       expose :job_events
+      expose :push_events_branch_filter
     end
 
     class SharedGroup < Grape::Entity
diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb
index 4760a1c08d7..0fb454bc22e 100644
--- a/lib/api/project_hooks.rb
+++ b/lib/api/project_hooks.rb
@@ -20,6 +20,7 @@ module API
         optional :wiki_page_events, type: Boolean, desc: "Trigger hook on wiki events"
         optional :enable_ssl_verification, type: Boolean, desc: "Do SSL verification when triggering the hook"
         optional :token, type: String, desc: "Secret token to validate received payloads; this will not be returned in the response"
+        optional :push_events_branch_filter, type: String, desc: "Trigger hook on specified branch only"
       end
     end
 
@@ -63,6 +64,7 @@ module API
           present hook, with: Entities::ProjectHook
         else
           error!("Invalid url given", 422) if hook.errors[:url].present?
+          error!("Invalid branch filter given", 422) if hook.errors[:push_events_branch_filter].present?
 
           not_found!("Project hook #{hook.errors.messages}")
         end
@@ -84,6 +86,7 @@ module API
           present hook, with: Entities::ProjectHook
         else
           error!("Invalid url given", 422) if hook.errors[:url].present?
+          error!("Invalid branch filter given", 422) if hook.errors[:push_events_branch_filter].present?
 
           not_found!("Project hook #{hook.errors.messages}")
         end
diff --git a/spec/features/projects/settings/integration_settings_spec.rb b/spec/features/projects/settings/integration_settings_spec.rb
index 8745ff72df0..32959969f54 100644
--- a/spec/features/projects/settings/integration_settings_spec.rb
+++ b/spec/features/projects/settings/integration_settings_spec.rb
@@ -51,6 +51,7 @@ describe 'Projects > Settings > Integration settings' do
 
         fill_in 'hook_url', with: url
         check 'Tag push events'
+        fill_in 'hook_push_events_branch_filter', with: 'master'
         check 'Enable SSL verification'
         check 'Job events'
 
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 0a1e3eb83d3..579f175c4a8 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -416,6 +416,7 @@ ProjectHook:
 - type
 - service_id
 - push_events
+- push_events_branch_filter
 - issues_events
 - merge_requests_events
 - tag_push_events
diff --git a/spec/models/hooks/active_hook_filter_spec.rb b/spec/models/hooks/active_hook_filter_spec.rb
new file mode 100644
index 00000000000..c97003eb542
--- /dev/null
+++ b/spec/models/hooks/active_hook_filter_spec.rb
@@ -0,0 +1,67 @@
+require 'spec_helper'
+
+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)
+      end
+
+      context 'branch filter is specified' do
+        let(:branch_filter) { 'master' }
+        it 'returns true if branch matches' do
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+        end
+
+        it 'returns false if branch does not match' do
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to eq(false)
+        end
+
+        it 'returns false if ref is nil' do
+          expect(filter.matches?(:push_hooks, {})).to eq(false)
+        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 eq(true)
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch/something' })).to eq(true)
+          end
+
+          it 'returns false if branch does not match' do
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(false)
+          end
+        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 eq(true)
+        end
+      end
+
+      context 'branch filter is empty string' do
+        let(:branch_filter) { '' }
+
+        it 'acts like branch is not specified' do
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+        end
+      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: '')
+      end
+
+      it 'returns true as branch filters are not yet supported for these' do
+        expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to eq(true)
+      end
+    end
+  end
+end
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index ea6d6e53ef5..a4181631f01 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -35,6 +35,26 @@ describe WebHook do
 
       it { is_expected.not_to allow_values("foo\nbar", "foo\r\nbar").for(:token) }
     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
+
+        expect(hook.push_events_branch_filter).to eq('branch')
+      end
+
+      it 'stores whitespace only as empty' do
+        hook.push_events_branch_filter = ' '
+        hook.save
+
+        expect(hook.push_events_branch_filter).to eq('')
+      end
+    end
   end
 
   describe 'execute' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2ec030daa67..cea7fa31b16 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3671,7 +3671,10 @@ describe Project do
 
   describe '#execute_hooks' do
     let(:data) { { ref: 'refs/heads/master', data: 'data' } }
-    it 'executes the projects hooks with the specified scope' do
+    it 'executes active projects hooks with the specified scope' do
+      expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
+        .with(:tag_push_hooks, data)
+        .and_return(true)
       hook = create(:project_hook, merge_requests_events: false, tag_push_events: true)
       project = create(:project, hooks: [hook])
 
@@ -3689,6 +3692,18 @@ describe Project do
       project.execute_hooks(data, :tag_push_hooks)
     end
 
+    it 'does not execute project hooks which are not active' do
+      expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
+        .with(:tag_push_hooks, data)
+        .and_return(false)
+      hook = create(:project_hook, tag_push_events: true)
+      project = create(:project, hooks: [hook])
+
+      expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
+
+      project.execute_hooks(data, :tag_push_hooks)
+    end
+
     it 'executes the system hooks with the specified scope' do
       expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(data, :merge_request_hooks)
 
diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb
index bc45a63d9f1..a0816322b10 100644
--- a/spec/requests/api/project_hooks_spec.rb
+++ b/spec/requests/api/project_hooks_spec.rb
@@ -9,7 +9,8 @@ describe API::ProjectHooks, 'ProjectHooks' do
            :all_events_enabled,
            project: project,
            url: 'http://example.com',
-           enable_ssl_verification: true)
+           enable_ssl_verification: true,
+           push_events_branch_filter: 'master')
   end
 
   before do
@@ -38,6 +39,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
         expect(json_response.first['pipeline_events']).to eq(true)
         expect(json_response.first['wiki_page_events']).to eq(true)
         expect(json_response.first['enable_ssl_verification']).to eq(true)
+        expect(json_response.first['push_events_branch_filter']).to eq('master')
       end
     end
 
@@ -95,7 +97,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
       expect do
         post api("/projects/#{project.id}/hooks", user),
           url: "http://example.com", issues_events: true, confidential_issues_events: true, wiki_page_events: true,
-          job_events: true
+          job_events: true, push_events_branch_filter: 'some-feature-branch'
       end.to change {project.hooks.count}.by(1)
 
       expect(response).to have_gitlab_http_status(201)
@@ -111,6 +113,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
       expect(json_response['pipeline_events']).to eq(false)
       expect(json_response['wiki_page_events']).to eq(true)
       expect(json_response['enable_ssl_verification']).to eq(true)
+      expect(json_response['push_events_branch_filter']).to eq('some-feature-branch')
       expect(json_response).not_to include('token')
     end
 
@@ -137,7 +140,12 @@ describe API::ProjectHooks, 'ProjectHooks' do
     end
 
     it "returns a 422 error if url not valid" do
-      post api("/projects/#{project.id}/hooks", user), "url" => "ftp://example.com"
+      post api("/projects/#{project.id}/hooks", user), url: "ftp://example.com"
+      expect(response).to have_gitlab_http_status(422)
+    end
+
+    it "returns a 422 error if branch filter is not valid" do
+      post api("/projects/#{project.id}/hooks", user), url: "http://example.com", push_events_branch_filter: '~badbranchname/'
       expect(response).to have_gitlab_http_status(422)
     end
   end
diff --git a/spec/validators/branch_filter_validator_spec.rb b/spec/validators/branch_filter_validator_spec.rb
new file mode 100644
index 00000000000..3be54827431
--- /dev/null
+++ b/spec/validators/branch_filter_validator_spec.rb
@@ -0,0 +1,42 @@
+require 'spec_helper'
+
+describe BranchFilterValidator do
+  let(:validator) { described_class.new(attributes: [:push_events_branch_filter]) }
+  let(:hook) { build(:project_hook) }
+
+  describe '#validates_each' do
+    it 'allows valid branch names' do
+      validator.validate_each(hook, :push_events_branch_filter, "good_branch_name")
+      validator.validate_each(hook, :push_events_branch_filter, "another/good_branch_name")
+      expect(hook.errors.empty?).to be true
+    end
+
+    it 'disallows bad branch names' do
+      validator.validate_each(hook, :push_events_branch_filter, "bad branch~name")
+      expect(hook.errors[:push_events_branch_filter].empty?).to be false
+    end
+
+    it 'allows wildcards' do
+      validator.validate_each(hook, :push_events_branch_filter, "features/*")
+      validator.validate_each(hook, :push_events_branch_filter, "features/*/bla")
+      validator.validate_each(hook, :push_events_branch_filter, "*-stable")
+      expect(hook.errors.empty?).to be true
+    end
+
+    it 'gets rid of whitespace' do
+      filter = ' master '
+      validator.validate_each(hook, :push_events_branch_filter, filter)
+
+      expect(filter).to eq 'master'
+    end
+
+    # Branch names can be quite long but in practice aren't over 255 so 4000 should
+    # be enough space for a list of branch names but we can increase if needed.
+    it 'limits length to 4000 chars' do
+      filter = 'a' * 4001
+      validator.validate_each(hook, :push_events_branch_filter, filter)
+
+      expect(hook.errors[:push_events_branch_filter].empty?).to be false
+    end
+  end
+end
-- 
cgit v1.2.1


From c322976032e45f02b60701ebf244a8a876063078 Mon Sep 17 00:00:00 2001
From: Duana Saskia <starkcoffee@users.noreply.github.com>
Date: Wed, 25 Jul 2018 13:01:10 +0200
Subject: Refactor ProtectedRefMatcher to be more generic

---
 app/models/concerns/protected_ref.rb   | 10 ++++--
 app/models/hooks/active_hook_filter.rb | 28 ++---------------
 app/models/protected_ref_matcher.rb    | 56 ----------------------------------
 app/models/ref_matcher.rb              | 46 ++++++++++++++++++++++++++++
 4 files changed, 56 insertions(+), 84 deletions(-)
 delete mode 100644 app/models/protected_ref_matcher.rb
 create mode 100644 app/models/ref_matcher.rb

diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb
index e62e680af6e..af387c99f3d 100644
--- a/app/models/concerns/protected_ref.rb
+++ b/app/models/concerns/protected_ref.rb
@@ -50,14 +50,20 @@ module ProtectedRef
         .map(&:"#{action}_access_levels").flatten
     end
 
+    # Returns all protected refs that match the given ref name.
+    # This checks all records from the scope built up so far, and does
+    # _not_ return a relation.
+    #
+    # This method optionally takes in a list of `protected_refs` to search
+    # through, to avoid calling out to the database.
     def matching(ref_name, protected_refs: nil)
-      ProtectedRefMatcher.matching(self, ref_name, protected_refs: protected_refs)
+      (protected_refs || self.all).select { |protected_ref| protected_ref.matches?(ref_name) }
     end
   end
 
   private
 
   def ref_matcher
-    @ref_matcher ||= ProtectedRefMatcher.new(self)
+    @ref_matcher ||= RefMatcher.new(self.name)
   end
 end
diff --git a/app/models/hooks/active_hook_filter.rb b/app/models/hooks/active_hook_filter.rb
index 611af971163..ea046bea368 100644
--- a/app/models/hooks/active_hook_filter.rb
+++ b/app/models/hooks/active_hook_filter.rb
@@ -1,6 +1,7 @@
 class ActiveHookFilter
   def initialize(hook)
     @hook = hook
+    @push_events_filter_matcher = RefMatcher.new(@hook.push_events_branch_filter)
   end
 
   def matches?(hooks_scope, data)
@@ -8,31 +9,6 @@ class ActiveHookFilter
     return true if @hook.push_events_branch_filter.blank?
 
     branch_name = Gitlab::Git.branch_name(data[:ref])
-    exact_match?(branch_name) || wildcard_match?(branch_name)
-  end
-
-  private
-
-  def exact_match?(branch_name)
-    @hook.push_events_branch_filter == branch_name
-  end
-
-  def wildcard_match?(branch_name)
-    return false unless wildcard?
-
-    wildcard_regex === branch_name
-  end
-
-  def wildcard_regex
-    @wildcard_regex ||= begin
-      name = @hook.push_events_branch_filter.gsub('*', 'STAR_DONT_ESCAPE')
-      quoted_name = Regexp.quote(name)
-      regex_string = quoted_name.gsub('STAR_DONT_ESCAPE', '.*?')
-      /\A#{regex_string}\z/
-    end
-  end
-
-  def wildcard?
-    @hook.push_events_branch_filter && @hook.push_events_branch_filter.include?('*')
+    @push_events_filter_matcher.matches?(branch_name)
   end
 end
diff --git a/app/models/protected_ref_matcher.rb b/app/models/protected_ref_matcher.rb
deleted file mode 100644
index bfa9180ac93..00000000000
--- a/app/models/protected_ref_matcher.rb
+++ /dev/null
@@ -1,56 +0,0 @@
-# frozen_string_literal: true
-
-class ProtectedRefMatcher
-  def initialize(protected_ref)
-    @protected_ref = protected_ref
-  end
-
-  # Returns all protected refs that match the given ref name.
-  # This checks all records from the scope built up so far, and does
-  # _not_ return a relation.
-  #
-  # This method optionally takes in a list of `protected_refs` to search
-  # through, to avoid calling out to the database.
-  def self.matching(type, ref_name, protected_refs: nil)
-    (protected_refs || type.all).select { |protected_ref| protected_ref.matches?(ref_name) }
-  end
-
-  # Returns all branches/tags (among the given list of refs [`Gitlab::Git::Branch`])
-  # that match the current protected ref.
-  def matching(refs)
-    refs.select { |ref| @protected_ref.matches?(ref.name) }
-  end
-
-  # Checks if the protected ref matches the given ref name.
-  def matches?(ref_name)
-    return false if @protected_ref.name.blank?
-
-    exact_match?(ref_name) || wildcard_match?(ref_name)
-  end
-
-  # Checks if this protected ref contains a wildcard
-  def wildcard?
-    @protected_ref.name && @protected_ref.name.include?('*')
-  end
-
-  protected
-
-  def exact_match?(ref_name)
-    @protected_ref.name == ref_name
-  end
-
-  def wildcard_match?(ref_name)
-    return false unless wildcard?
-
-    wildcard_regex === ref_name
-  end
-
-  def wildcard_regex
-    @wildcard_regex ||= begin
-      name = @protected_ref.name.gsub('*', 'STAR_DONT_ESCAPE')
-      quoted_name = Regexp.quote(name)
-      regex_string = quoted_name.gsub('STAR_DONT_ESCAPE', '.*?')
-      /\A#{regex_string}\z/
-    end
-  end
-end
diff --git a/app/models/ref_matcher.rb b/app/models/ref_matcher.rb
new file mode 100644
index 00000000000..fa7d2c0f06c
--- /dev/null
+++ b/app/models/ref_matcher.rb
@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+class RefMatcher
+  def initialize(ref_name_or_pattern)
+    @ref_name_or_pattern = ref_name_or_pattern
+  end
+
+  # Returns all branches/tags (among the given list of refs [`Gitlab::Git::Branch`])
+  # that match the current protected ref.
+  def matching(refs)
+    refs.select { |ref| matches?(ref.name) }
+  end
+
+  # Checks if the protected ref matches the given ref name.
+  def matches?(ref_name)
+    return false if @ref_name_or_pattern.blank?
+
+    exact_match?(ref_name) || wildcard_match?(ref_name)
+  end
+
+  # Checks if this protected ref contains a wildcard
+  def wildcard?
+    @ref_name_or_pattern && @ref_name_or_pattern.include?('*')
+  end
+
+  protected
+
+  def exact_match?(ref_name)
+    @ref_name_or_pattern == ref_name
+  end
+
+  def wildcard_match?(ref_name)
+    return false unless wildcard?
+
+    wildcard_regex === ref_name
+  end
+
+  def wildcard_regex
+    @wildcard_regex ||= begin
+      name = @ref_name_or_pattern.gsub('*', 'STAR_DONT_ESCAPE')
+      quoted_name = Regexp.quote(name)
+      regex_string = quoted_name.gsub('STAR_DONT_ESCAPE', '.*?')
+      /\A#{regex_string}\z/
+    end
+  end
+end
-- 
cgit v1.2.1


From 9d742e61a79dcc85589598259e2fdac030b7ac00 Mon Sep 17 00:00:00 2001
From: Duana Saskia <starkcoffee@users.noreply.github.com>
Date: Sun, 2 Sep 2018 01:55:06 +1000
Subject: Refactor: move active hook filter to TriggerableHooks

---
 app/models/concerns/triggerable_hooks.rb       |  6 ++++++
 app/models/project.rb                          |  2 +-
 app/validators/branch_filter_validator.rb      |  1 +
 spec/models/concerns/triggerable_hooks_spec.rb | 24 ++++++++++++++++++++++++
 spec/models/hooks/active_hook_filter_spec.rb   | 19 ++++++++++---------
 spec/models/project_spec.rb                    | 24 ++++++++++++------------
 6 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/app/models/concerns/triggerable_hooks.rb b/app/models/concerns/triggerable_hooks.rb
index f55ab2fcaf3..9f2e8b420bc 100644
--- a/app/models/concerns/triggerable_hooks.rb
+++ b/app/models/concerns/triggerable_hooks.rb
@@ -28,6 +28,12 @@ module TriggerableHooks
       public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend
     end
 
+    def select_active(hooks_scope, data)
+      select do |hook|
+        ActiveHookFilter.new(hook).matches?(hooks_scope, data)
+      end
+    end
+
     private
 
     def triggerable_hooks(hooks)
diff --git a/app/models/project.rb b/app/models/project.rb
index 72a53e1a50d..728bbffd671 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1163,7 +1163,7 @@ class Project < ActiveRecord::Base
 
   def execute_hooks(data, hooks_scope = :push_hooks)
     run_after_commit_or_now do
-      hooks.hooks_for(hooks_scope).select {|hook| ActiveHookFilter.new(hook).matches?(hooks_scope, data)}.each do |hook|
+      hooks.hooks_for(hooks_scope).select_active(hooks_scope, data).each do |hook|
         hook.async_execute(data, hooks_scope.to_s)
       end
       SystemHooksService.new.execute_hooks(data, hooks_scope)
diff --git a/app/validators/branch_filter_validator.rb b/app/validators/branch_filter_validator.rb
index 0965be3d101..ef482aaaa63 100644
--- a/app/validators/branch_filter_validator.rb
+++ b/app/validators/branch_filter_validator.rb
@@ -16,6 +16,7 @@ class BranchFilterValidator < ActiveModel::EachValidator
 
     if value.present?
       value_without_wildcards = value.tr('*', 'x')
+
       unless Gitlab::GitRefValidator.validate(value_without_wildcards)
         record.errors[attribute] << "is not a valid branch name"
       end
diff --git a/spec/models/concerns/triggerable_hooks_spec.rb b/spec/models/concerns/triggerable_hooks_spec.rb
index 621d2d38eae..265abd6bd72 100644
--- a/spec/models/concerns/triggerable_hooks_spec.rb
+++ b/spec/models/concerns/triggerable_hooks_spec.rb
@@ -40,4 +40,28 @@ RSpec.describe TriggerableHooks do
       end
     end
   end
+
+  describe '.select_active' do
+    it 'returns hooks that match the active filter' do
+      TestableHook.create!(url: 'http://example1.com', push_events: true)
+      TestableHook.create!(url: 'http://example2.com', push_events: true)
+      filter1 = double(:filter1)
+      filter2 = double(:filter2)
+      allow(ActiveHookFilter).to receive(:new).exactly(2).times.and_return(filter1, filter2)
+      expect(filter1).to receive(:matches?).and_return(true)
+      expect(filter2).to receive(:matches?).and_return(false)
+
+      hooks = TestableHook.push_hooks.order_id_asc
+      expect(hooks.select_active(:push_hooks, {})).to eq [hooks.first]
+    end
+
+    it 'returns empty list if no hooks match the active filter' do
+      TestableHook.create!(url: 'http://example1.com', push_events: true)
+      filter = double(:filter)
+      allow(ActiveHookFilter).to receive(:new).and_return(filter)
+      expect(filter).to receive(:matches?).and_return(false)
+
+      expect(TestableHook.push_hooks.select_active(:push_hooks, {})).to eq []
+    end
+  end
 end
diff --git a/spec/models/hooks/active_hook_filter_spec.rb b/spec/models/hooks/active_hook_filter_spec.rb
index c97003eb542..df7edda2213 100644
--- a/spec/models/hooks/active_hook_filter_spec.rb
+++ b/spec/models/hooks/active_hook_filter_spec.rb
@@ -11,28 +11,29 @@ describe ActiveHookFilter do
 
       context 'branch filter is specified' do
         let(:branch_filter) { 'master' }
+
         it 'returns true if branch matches' do
-          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
         end
 
         it 'returns false if branch does not match' do
-          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to eq(false)
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to be false
         end
 
         it 'returns false if ref is nil' do
-          expect(filter.matches?(:push_hooks, {})).to eq(false)
+          expect(filter.matches?(:push_hooks, {})).to be false
         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 eq(true)
-            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch/something' })).to eq(true)
+            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 eq(false)
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be false
           end
         end
       end
@@ -41,7 +42,7 @@ describe ActiveHookFilter do
         let(:branch_filter) { nil }
 
         it 'returns true' do
-          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
         end
       end
 
@@ -49,7 +50,7 @@ describe ActiveHookFilter do
         let(:branch_filter) { '' }
 
         it 'acts like branch is not specified' do
-          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
         end
       end
     end
@@ -60,7 +61,7 @@ describe ActiveHookFilter do
       end
 
       it 'returns true as branch filters are not yet supported for these' do
-        expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to eq(true)
+        expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to be true
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index cea7fa31b16..d3dc95548aa 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3672,36 +3672,36 @@ describe Project do
   describe '#execute_hooks' do
     let(:data) { { ref: 'refs/heads/master', data: 'data' } }
     it 'executes active projects hooks with the specified scope' do
-      expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
-        .with(:tag_push_hooks, data)
-        .and_return(true)
-      hook = create(:project_hook, merge_requests_events: false, tag_push_events: true)
+      hook = create(:project_hook, merge_requests_events: false, push_events: true)
+      expect(ProjectHook).to receive(:select_active)
+        .with(:push_hooks, data)
+        .and_return([hook])
       project = create(:project, hooks: [hook])
 
       expect_any_instance_of(ProjectHook).to receive(:async_execute).once
 
-      project.execute_hooks(data, :tag_push_hooks)
+      project.execute_hooks(data, :push_hooks)
     end
 
     it 'does not execute project hooks that dont match the specified scope' do
-      hook = create(:project_hook, merge_requests_events: true, tag_push_events: false)
+      hook = create(:project_hook, merge_requests_events: true, push_events: false)
       project = create(:project, hooks: [hook])
 
       expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
 
-      project.execute_hooks(data, :tag_push_hooks)
+      project.execute_hooks(data, :push_hooks)
     end
 
     it 'does not execute project hooks which are not active' do
-      expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
-        .with(:tag_push_hooks, data)
-        .and_return(false)
-      hook = create(:project_hook, tag_push_events: true)
+      hook = create(:project_hook, push_events: true)
+      expect(ProjectHook).to receive(:select_active)
+        .with(:push_hooks, data)
+        .and_return([])
       project = create(:project, hooks: [hook])
 
       expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
 
-      project.execute_hooks(data, :tag_push_hooks)
+      project.execute_hooks(data, :push_hooks)
     end
 
     it 'executes the system hooks with the specified scope' do
-- 
cgit v1.2.1