diff options
20 files changed, 261 insertions, 8 deletions
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 |