Commit ece6a1ea authored by Duana Saskia's avatar Duana Saskia

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.
parent 07356866
...@@ -66,6 +66,7 @@ class Projects::HooksController < Projects::ApplicationController ...@@ -66,6 +66,7 @@ class Projects::HooksController < Projects::ApplicationController
:enable_ssl_verification, :enable_ssl_verification,
:token, :token,
:url, :url,
:push_events_branch_filter,
*ProjectHook.triggers.values *ProjectHook.triggers.values
) )
end end
......
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
...@@ -9,6 +9,7 @@ class WebHook < ActiveRecord::Base ...@@ -9,6 +9,7 @@ class WebHook < ActiveRecord::Base
allow_local_network: lambda(&:allow_local_requests?) } allow_local_network: lambda(&:allow_local_requests?) }
validates :token, format: { without: /\n/ } validates :token, format: { without: /\n/ }
validates :push_events_branch_filter, branch_filter: true
def execute(data, hook_name) def execute(data, hook_name)
WebHookService.new(self, data, hook_name).execute WebHookService.new(self, data, hook_name).execute
......
...@@ -1163,10 +1163,9 @@ class Project < ActiveRecord::Base ...@@ -1163,10 +1163,9 @@ class Project < ActiveRecord::Base
def execute_hooks(data, hooks_scope = :push_hooks) def execute_hooks(data, hooks_scope = :push_hooks)
run_after_commit_or_now do 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) hook.async_execute(data, hooks_scope.to_s)
end end
SystemHooksService.new.execute_hooks(data, hooks_scope) SystemHooksService.new.execute_hooks(data, hooks_scope)
end end
end end
......
# 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
...@@ -17,6 +17,7 @@ ...@@ -17,6 +17,7 @@
%strong Push events %strong Push events
%p.light.ml-1 %p.light.ml-1
This URL will be triggered by a push to the repository 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 %li
= form.check_box :tag_push_events, class: 'form-check-input' = form.check_box :tag_push_events, class: 'form-check-input'
= form.label :tag_push_events, class: 'list-label form-check-label ml-1' do = form.label :tag_push_events, class: 'list-label form-check-label ml-1' do
......
---
title: Add branch filter to project webhooks
merge_request: 20338
author: Duana Saskia
type: added
# 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
...@@ -2239,6 +2239,7 @@ ActiveRecord::Schema.define(version: 20180807153545) do ...@@ -2239,6 +2239,7 @@ ActiveRecord::Schema.define(version: 20180807153545) do
t.boolean "repository_update_events", default: false, null: false t.boolean "repository_update_events", default: false, null: false
t.boolean "job_events", default: false, null: false t.boolean "job_events", default: false, null: false
t.boolean "confidential_note_events" t.boolean "confidential_note_events"
t.text "push_events_branch_filter"
end end
add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
......
...@@ -1334,6 +1334,7 @@ GET /projects/:id/hooks/:hook_id ...@@ -1334,6 +1334,7 @@ GET /projects/:id/hooks/:hook_id
"url": "http://example.com/hook", "url": "http://example.com/hook",
"project_id": 3, "project_id": 3,
"push_events": true, "push_events": true,
"push_events_branch_filter": "",
"issues_events": true, "issues_events": true,
"confidential_issues_events": true, "confidential_issues_events": true,
"merge_requests_events": true, "merge_requests_events": true,
...@@ -1360,6 +1361,7 @@ POST /projects/:id/hooks ...@@ -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) | | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
| `url` | string | yes | The hook URL | | `url` | string | yes | The hook URL |
| `push_events` | boolean | no | Trigger hook on push events | | `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 | | `issues_events` | boolean | no | Trigger hook on issues events |
| `confidential_issues_events` | boolean | no | Trigger hook on confidential issues events | | `confidential_issues_events` | boolean | no | Trigger hook on confidential issues events |
| `merge_requests_events` | boolean | no | Trigger hook on merge requests events | | `merge_requests_events` | boolean | no | Trigger hook on merge requests events |
...@@ -1385,6 +1387,7 @@ PUT /projects/:id/hooks/:hook_id ...@@ -1385,6 +1387,7 @@ PUT /projects/:id/hooks/:hook_id
| `hook_id` | integer | yes | The ID of the project hook | | `hook_id` | integer | yes | The ID of the project hook |
| `url` | string | yes | The hook URL | | `url` | string | yes | The hook URL |
| `push_events` | boolean | no | Trigger hook on push events | | `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 | | `issues_events` | boolean | no | Trigger hook on issues events |
| `confidential_issues_events` | boolean | no | Trigger hook on confidential issues events | | `confidential_issues_events` | boolean | no | Trigger hook on confidential issues events |
| `merge_requests_events` | boolean | no | Trigger hook on merge requests events | | `merge_requests_events` | boolean | no | Trigger hook on merge requests events |
......
...@@ -83,6 +83,7 @@ module API ...@@ -83,6 +83,7 @@ module API
expose :project_id, :issues_events, :confidential_issues_events expose :project_id, :issues_events, :confidential_issues_events
expose :note_events, :confidential_note_events, :pipeline_events, :wiki_page_events expose :note_events, :confidential_note_events, :pipeline_events, :wiki_page_events
expose :job_events expose :job_events
expose :push_events_branch_filter
end end
class SharedGroup < Grape::Entity class SharedGroup < Grape::Entity
......
...@@ -20,6 +20,7 @@ module API ...@@ -20,6 +20,7 @@ module API
optional :wiki_page_events, type: Boolean, desc: "Trigger hook on wiki events" 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 :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 :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
end end
...@@ -63,6 +64,7 @@ module API ...@@ -63,6 +64,7 @@ module API
present hook, with: Entities::ProjectHook present hook, with: Entities::ProjectHook
else else
error!("Invalid url given", 422) if hook.errors[:url].present? 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}") not_found!("Project hook #{hook.errors.messages}")
end end
...@@ -84,6 +86,7 @@ module API ...@@ -84,6 +86,7 @@ module API
present hook, with: Entities::ProjectHook present hook, with: Entities::ProjectHook
else else
error!("Invalid url given", 422) if hook.errors[:url].present? 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}") not_found!("Project hook #{hook.errors.messages}")
end end
......
...@@ -51,6 +51,7 @@ describe 'Projects > Settings > Integration settings' do ...@@ -51,6 +51,7 @@ describe 'Projects > Settings > Integration settings' do
fill_in 'hook_url', with: url fill_in 'hook_url', with: url
check 'Tag push events' check 'Tag push events'
fill_in 'hook_push_events_branch_filter', with: 'master'
check 'Enable SSL verification' check 'Enable SSL verification'
check 'Job events' check 'Job events'
......
...@@ -416,6 +416,7 @@ ProjectHook: ...@@ -416,6 +416,7 @@ ProjectHook:
- type - type
- service_id - service_id
- push_events - push_events
- push_events_branch_filter
- issues_events - issues_events
- merge_requests_events - merge_requests_events
- tag_push_events - tag_push_events
......
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
...@@ -35,6 +35,26 @@ describe WebHook do ...@@ -35,6 +35,26 @@ describe WebHook do
it { is_expected.not_to allow_values("foo\nbar", "foo\r\nbar").for(:token) } it { is_expected.not_to allow_values("foo\nbar", "foo\r\nbar").for(:token) }
end 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 end
describe 'execute' do describe 'execute' do
......
...@@ -3671,7 +3671,10 @@ describe Project do ...@@ -3671,7 +3671,10 @@ describe Project do
describe '#execute_hooks' do describe '#execute_hooks' do
let(:data) { { ref: 'refs/heads/master', data: 'data' } } 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) hook = create(:project_hook, merge_requests_events: false, tag_push_events: true)
project = create(:project, hooks: [hook]) project = create(:project, hooks: [hook])
...@@ -3689,6 +3692,18 @@ describe Project do ...@@ -3689,6 +3692,18 @@ describe Project do
project.execute_hooks(data, :tag_push_hooks) project.execute_hooks(data, :tag_push_hooks)
end 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 it 'executes the system hooks with the specified scope' do
expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(data, :merge_request_hooks) expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(data, :merge_request_hooks)
......
...@@ -9,7 +9,8 @@ describe API::ProjectHooks, 'ProjectHooks' do ...@@ -9,7 +9,8 @@ describe API::ProjectHooks, 'ProjectHooks' do
:all_events_enabled, :all_events_enabled,
project: project, project: project,
url: 'http://example.com', url: 'http://example.com',
enable_ssl_verification: true) enable_ssl_verification: true,
push_events_branch_filter: 'master')
end end
before do before do
...@@ -38,6 +39,7 @@ describe API::ProjectHooks, 'ProjectHooks' do ...@@ -38,6 +39,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
expect(json_response.first['pipeline_events']).to eq(true) expect(json_response.first['pipeline_events']).to eq(true)
expect(json_response.first['wiki_page_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['enable_ssl_verification']).to eq(true)
expect(json_response.first['push_events_branch_filter']).to eq('master')
end end
end end
...@@ -95,7 +97,7 @@ describe API::ProjectHooks, 'ProjectHooks' do ...@@ -95,7 +97,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
expect do expect do
post api("/projects/#{project.id}/hooks", user), post api("/projects/#{project.id}/hooks", user),
url: "http://example.com", issues_events: true, confidential_issues_events: true, wiki_page_events: true, 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) end.to change {project.hooks.count}.by(1)
expect(response).to have_gitlab_http_status(201) expect(response).to have_gitlab_http_status(201)
...@@ -111,6 +113,7 @@ describe API::ProjectHooks, 'ProjectHooks' do ...@@ -111,6 +113,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
expect(json_response['pipeline_events']).to eq(false) expect(json_response['pipeline_events']).to eq(false)
expect(json_response['wiki_page_events']).to eq(true) expect(json_response['wiki_page_events']).to eq(true)
expect(json_response['enable_ssl_verification']).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') expect(json_response).not_to include('token')
end end
...@@ -137,7 +140,12 @@ describe API::ProjectHooks, 'ProjectHooks' do ...@@ -137,7 +140,12 @@ describe API::ProjectHooks, 'ProjectHooks' do
end end
it "returns a 422 error if url not valid" do 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) expect(response).to have_gitlab_http_status(422)
end end
end end
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment