summaryrefslogtreecommitdiff
path: root/app
diff options
context:
space:
mode:
authorAlexander Randa <randa.alex@gmail.com>2017-07-20 15:12:06 +0000
committerRémy Coutable <remy@rymai.me>2017-07-20 15:12:06 +0000
commite0ab5618a0998175df9f90c95ebd35d7afa01db7 (patch)
tree91cfff5c3c9f6a118d69df5e2816b7461ce2ccc5 /app
parent020b6a0be06614815d96854084f3dcafeefcf0b7 (diff)
downloadgitlab-ce-e0ab5618a0998175df9f90c95ebd35d7afa01db7.tar.gz
Wrong data type when testing webhooks
Diffstat (limited to 'app')
-rw-r--r--app/controllers/admin/hook_logs_controller.rb4
-rw-r--r--app/controllers/admin/hooks_controller.rb15
-rw-r--r--app/controllers/concerns/hooks_execution.rb13
-rw-r--r--app/controllers/projects/hook_logs_controller.rb4
-rw-r--r--app/controllers/projects/hooks_controller.rb12
-rw-r--r--app/helpers/hooks_helper.rb17
-rw-r--r--app/models/hooks/project_hook.rb25
-rw-r--r--app/models/hooks/service_hook.rb1
-rw-r--r--app/models/hooks/system_hook.rb10
-rw-r--r--app/models/hooks/web_hook.rb14
-rw-r--r--app/services/system_hooks_service.rb2
-rw-r--r--app/services/test_hook_service.rb6
-rw-r--r--app/services/test_hooks/base_service.rb41
-rw-r--r--app/services/test_hooks/project_service.rb63
-rw-r--r--app/services/test_hooks/system_service.rb48
-rw-r--r--app/services/web_hook_service.rb11
-rw-r--r--app/services/wiki_pages/base_service.rb16
-rw-r--r--app/views/admin/hooks/edit.html.haml2
-rw-r--r--app/views/admin/hooks/index.html.haml8
-rw-r--r--app/views/projects/hooks/edit.html.haml3
-rw-r--r--app/views/projects/settings/integrations/_project_hook.html.haml14
-rw-r--r--app/views/shared/web_hooks/_test_button.html.haml12
22 files changed, 253 insertions, 88 deletions
diff --git a/app/controllers/admin/hook_logs_controller.rb b/app/controllers/admin/hook_logs_controller.rb
index aa069b89563..3017f96c26f 100644
--- a/app/controllers/admin/hook_logs_controller.rb
+++ b/app/controllers/admin/hook_logs_controller.rb
@@ -10,9 +10,9 @@ class Admin::HookLogsController < Admin::ApplicationController
end
def retry
- status, message = hook.execute(hook_log.request_data, hook_log.trigger)
+ result = hook.execute(hook_log.request_data, hook_log.trigger)
- set_hook_execution_notice(status, message)
+ set_hook_execution_notice(result)
redirect_to edit_admin_hook_path(@hook)
end
diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb
index 054c3500b35..77e3c95d197 100644
--- a/app/controllers/admin/hooks_controller.rb
+++ b/app/controllers/admin/hooks_controller.rb
@@ -38,9 +38,9 @@ class Admin::HooksController < Admin::ApplicationController
end
def test
- status, message = hook.execute(sample_hook_data, 'system_hooks')
+ result = TestHooks::SystemService.new(hook, current_user, params[:trigger]).execute
- set_hook_execution_notice(status, message)
+ set_hook_execution_notice(result)
redirect_back_or_default
end
@@ -66,15 +66,4 @@ class Admin::HooksController < Admin::ApplicationController
:url
)
end
-
- def sample_hook_data
- {
- event_name: "project_create",
- name: "Ruby",
- path: "ruby",
- project_id: 1,
- owner_name: "Someone",
- owner_email: "example@gitlabhq.com"
- }
- end
end
diff --git a/app/controllers/concerns/hooks_execution.rb b/app/controllers/concerns/hooks_execution.rb
index 846cd60518f..a22e46b4860 100644
--- a/app/controllers/concerns/hooks_execution.rb
+++ b/app/controllers/concerns/hooks_execution.rb
@@ -3,11 +3,14 @@ module HooksExecution
private
- def set_hook_execution_notice(status, message)
- if status && status >= 200 && status < 400
- flash[:notice] = "Hook executed successfully: HTTP #{status}"
- elsif status
- flash[:alert] = "Hook executed successfully but returned HTTP #{status} #{message}"
+ def set_hook_execution_notice(result)
+ http_status = result[:http_status]
+ message = result[:message]
+
+ if http_status && http_status >= 200 && http_status < 400
+ flash[:notice] = "Hook executed successfully: HTTP #{http_status}"
+ elsif http_status
+ flash[:alert] = "Hook executed successfully but returned HTTP #{http_status} #{message}"
else
flash[:alert] = "Hook execution failed: #{message}"
end
diff --git a/app/controllers/projects/hook_logs_controller.rb b/app/controllers/projects/hook_logs_controller.rb
index b9c4b29580a..745e89fc843 100644
--- a/app/controllers/projects/hook_logs_controller.rb
+++ b/app/controllers/projects/hook_logs_controller.rb
@@ -14,9 +14,9 @@ class Projects::HookLogsController < Projects::ApplicationController
end
def retry
- status, message = hook.execute(hook_log.request_data, hook_log.trigger)
+ result = hook.execute(hook_log.request_data, hook_log.trigger)
- set_hook_execution_notice(status, message)
+ set_hook_execution_notice(result)
redirect_to edit_project_hook_path(@project, @hook)
end
diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index 18895c3f0f3..85d35900c71 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -9,6 +9,10 @@ class Projects::HooksController < Projects::ApplicationController
layout "project_settings"
+ def index
+ redirect_to project_settings_integrations_path(@project)
+ end
+
def create
@hook = @project.hooks.new(hook_params)
@hook.save
@@ -33,13 +37,9 @@ class Projects::HooksController < Projects::ApplicationController
end
def test
- if !@project.empty_repo?
- status, message = TestHookService.new.execute(hook, current_user)
+ result = TestHooks::ProjectService.new(hook, current_user, params[:trigger]).execute
- set_hook_execution_notice(status, message)
- else
- flash[:alert] = 'Hook execution failed. Ensure the project has commits.'
- end
+ set_hook_execution_notice(result)
redirect_back_or_default(default: { action: 'index' })
end
diff --git a/app/helpers/hooks_helper.rb b/app/helpers/hooks_helper.rb
new file mode 100644
index 00000000000..551b9cca6b1
--- /dev/null
+++ b/app/helpers/hooks_helper.rb
@@ -0,0 +1,17 @@
+module HooksHelper
+ def link_to_test_hook(hook, trigger)
+ path = case hook
+ when ProjectHook
+ project = hook.project
+ test_project_hook_path(project, hook, trigger: trigger)
+ when SystemHook
+ test_admin_hook_path(hook, trigger: trigger)
+ end
+
+ trigger_human_name = trigger.to_s.tr('_', ' ').camelize
+
+ link_to path, rel: 'nofollow' do
+ content_tag(:span, trigger_human_name)
+ end
+ end
+end
diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb
index ee6165fd32d..a8c424a6614 100644
--- a/app/models/hooks/project_hook.rb
+++ b/app/models/hooks/project_hook.rb
@@ -1,11 +1,20 @@
class ProjectHook < WebHook
- belongs_to :project
+ TRIGGERS = {
+ push_hooks: :push_events,
+ tag_push_hooks: :tag_push_events,
+ issue_hooks: :issues_events,
+ confidential_issue_hooks: :confidential_issues_events,
+ note_hooks: :note_events,
+ merge_request_hooks: :merge_requests_events,
+ job_hooks: :job_events,
+ pipeline_hooks: :pipeline_events,
+ wiki_page_hooks: :wiki_page_events
+ }.freeze
+
+ TRIGGERS.each do |trigger, event|
+ scope trigger, -> { where(event => true) }
+ end
- scope :issue_hooks, -> { where(issues_events: true) }
- scope :confidential_issue_hooks, -> { where(confidential_issues_events: true) }
- scope :note_hooks, -> { where(note_events: true) }
- scope :merge_request_hooks, -> { where(merge_requests_events: true) }
- scope :job_hooks, -> { where(job_events: true) }
- scope :pipeline_hooks, -> { where(pipeline_events: true) }
- scope :wiki_page_hooks, -> { where(wiki_page_events: true) }
+ belongs_to :project
+ validates :project, presence: true
end
diff --git a/app/models/hooks/service_hook.rb b/app/models/hooks/service_hook.rb
index 40e43c27f91..aef11514945 100644
--- a/app/models/hooks/service_hook.rb
+++ b/app/models/hooks/service_hook.rb
@@ -1,5 +1,6 @@
class ServiceHook < WebHook
belongs_to :service
+ validates :service, presence: true
def execute(data)
WebHookService.new(self, data, 'service_hook').execute
diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb
index 1584235ab00..180c479c41b 100644
--- a/app/models/hooks/system_hook.rb
+++ b/app/models/hooks/system_hook.rb
@@ -1,5 +1,13 @@
class SystemHook < WebHook
- scope :repository_update_hooks, -> { where(repository_update_events: true) }
+ TRIGGERS = {
+ repository_update_hooks: :repository_update_events,
+ push_hooks: :push_events,
+ tag_push_hooks: :tag_push_events
+ }.freeze
+
+ TRIGGERS.each do |trigger, event|
+ scope trigger, -> { where(event => true) }
+ end
default_value_for :push_events, false
default_value_for :repository_update_events, true
diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index 7a9f8997959..5a70e114f56 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -1,22 +1,8 @@
class WebHook < ActiveRecord::Base
include Sortable
- default_value_for :push_events, true
- default_value_for :issues_events, false
- default_value_for :confidential_issues_events, false
- default_value_for :note_events, false
- default_value_for :merge_requests_events, false
- default_value_for :tag_push_events, false
- default_value_for :job_events, false
- default_value_for :pipeline_events, false
- default_value_for :repository_update_events, false
- default_value_for :enable_ssl_verification, true
-
has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
- scope :push_hooks, -> { where(push_events: true) }
- scope :tag_push_hooks, -> { where(tag_push_events: true) }
-
validates :url, presence: true, url: true
def execute(data, hook_name)
diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb
index ed476fc9d0c..bd58a54592f 100644
--- a/app/services/system_hooks_service.rb
+++ b/app/services/system_hooks_service.rb
@@ -4,7 +4,7 @@ class SystemHooksService
end
def execute_hooks(data, hooks_scope = :all)
- SystemHook.send(hooks_scope).each do |hook|
+ SystemHook.public_send(hooks_scope).find_each do |hook|
hook.async_execute(data, 'system_hooks')
end
end
diff --git a/app/services/test_hook_service.rb b/app/services/test_hook_service.rb
deleted file mode 100644
index 280c81f7d2d..00000000000
--- a/app/services/test_hook_service.rb
+++ /dev/null
@@ -1,6 +0,0 @@
-class TestHookService
- def execute(hook, current_user)
- data = Gitlab::DataBuilder::Push.build_sample(hook.project, current_user)
- hook.execute(data, 'push_hooks')
- end
-end
diff --git a/app/services/test_hooks/base_service.rb b/app/services/test_hooks/base_service.rb
new file mode 100644
index 00000000000..74ba814afff
--- /dev/null
+++ b/app/services/test_hooks/base_service.rb
@@ -0,0 +1,41 @@
+module TestHooks
+ class BaseService
+ attr_accessor :hook, :current_user, :trigger
+
+ def initialize(hook, current_user, trigger)
+ @hook = hook
+ @current_user = current_user
+ @trigger = trigger
+ end
+
+ def execute
+ trigger_data_method = "#{trigger}_data"
+
+ if !self.respond_to?(trigger_data_method, true) ||
+ !hook.class::TRIGGERS.value?(trigger.to_sym)
+
+ return error('Testing not available for this hook')
+ end
+
+ error_message = catch(:validation_error) do
+ sample_data = self.__send__(trigger_data_method)
+
+ return hook.execute(sample_data, trigger)
+ end
+
+ error(error_message)
+ end
+
+ private
+
+ def error(message, http_status = nil)
+ result = {
+ message: message,
+ status: :error
+ }
+
+ result[:http_status] = http_status if http_status
+ result
+ end
+ end
+end
diff --git a/app/services/test_hooks/project_service.rb b/app/services/test_hooks/project_service.rb
new file mode 100644
index 00000000000..01d5d774cd5
--- /dev/null
+++ b/app/services/test_hooks/project_service.rb
@@ -0,0 +1,63 @@
+module TestHooks
+ class ProjectService < TestHooks::BaseService
+ private
+
+ def project
+ @project ||= hook.project
+ end
+
+ def push_events_data
+ throw(:validation_error, 'Ensure the project has at least one commit.') if project.empty_repo?
+
+ Gitlab::DataBuilder::Push.build_sample(project, current_user)
+ end
+
+ alias_method :tag_push_events_data, :push_events_data
+
+ def note_events_data
+ note = project.notes.first
+ throw(:validation_error, 'Ensure the project has notes.') unless note.present?
+
+ Gitlab::DataBuilder::Note.build(note, current_user)
+ end
+
+ def issues_events_data
+ issue = project.issues.first
+ throw(:validation_error, 'Ensure the project has issues.') unless issue.present?
+
+ issue.to_hook_data(current_user)
+ end
+
+ alias_method :confidential_issues_events_data, :issues_events_data
+
+ def merge_requests_events_data
+ merge_request = project.merge_requests.first
+ throw(:validation_error, 'Ensure the project has merge requests.') unless merge_request.present?
+
+ merge_request.to_hook_data(current_user)
+ end
+
+ def job_events_data
+ build = project.builds.first
+ throw(:validation_error, 'Ensure the project has CI jobs.') unless build.present?
+
+ Gitlab::DataBuilder::Build.build(build)
+ end
+
+ def pipeline_events_data
+ pipeline = project.pipelines.first
+ throw(:validation_error, 'Ensure the project has CI pipelines.') unless pipeline.present?
+
+ Gitlab::DataBuilder::Pipeline.build(pipeline)
+ end
+
+ def wiki_page_events_data
+ page = project.wiki.pages.first
+ if !project.wiki_enabled? || page.blank?
+ throw(:validation_error, 'Ensure the wiki is enabled and has pages.')
+ end
+
+ Gitlab::DataBuilder::WikiPage.build(page, current_user, 'create')
+ end
+ end
+end
diff --git a/app/services/test_hooks/system_service.rb b/app/services/test_hooks/system_service.rb
new file mode 100644
index 00000000000..76c3c19bd74
--- /dev/null
+++ b/app/services/test_hooks/system_service.rb
@@ -0,0 +1,48 @@
+module TestHooks
+ class SystemService < TestHooks::BaseService
+ private
+
+ def project
+ @project ||= begin
+ project = Project.first
+
+ throw(:validation_error, 'Ensure that at least one project exists.') unless project
+
+ project
+ end
+ end
+
+ def push_events_data
+ if project.empty_repo?
+ throw(:validation_error, "Ensure project \"#{project.human_name}\" has commits.")
+ end
+
+ Gitlab::DataBuilder::Push.build_sample(project, current_user)
+ end
+
+ def tag_push_events_data
+ if project.repository.tags.empty?
+ throw(:validation_error, "Ensure project \"#{project.human_name}\" has tags.")
+ end
+
+ Gitlab::DataBuilder::Push.build_sample(project, current_user)
+ end
+
+ def repository_update_events_data
+ commit = project.commit
+ ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{project.default_branch}"
+
+ unless commit
+ throw(:validation_error, "Ensure project \"#{project.human_name}\" has commits.")
+ end
+
+ change = Gitlab::DataBuilder::Repository.single_change(
+ commit.parent_id || Gitlab::Git::BLANK_SHA,
+ commit.id,
+ ref
+ )
+
+ Gitlab::DataBuilder::Repository.update(project, current_user, [change], [ref])
+ end
+ end
+end
diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index 4241b912d5b..a5110a23cad 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -39,7 +39,11 @@ class WebHookService
execution_duration: Time.now - start_time
)
- [response.code, response.to_s]
+ {
+ status: :success,
+ http_status: response.code,
+ message: response.to_s
+ }
rescue SocketError, OpenSSL::SSL::SSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, Net::OpenTimeout => e
log_execution(
trigger: hook_name,
@@ -52,7 +56,10 @@ class WebHookService
Rails.logger.error("WebHook Error => #{e}")
- [nil, e.to_s]
+ {
+ status: :error,
+ message: e.to_s
+ }
end
def async_execute
diff --git a/app/services/wiki_pages/base_service.rb b/app/services/wiki_pages/base_service.rb
index 14317ea65c8..260c04a8b94 100644
--- a/app/services/wiki_pages/base_service.rb
+++ b/app/services/wiki_pages/base_service.rb
@@ -1,23 +1,9 @@
module WikiPages
class BaseService < ::BaseService
- def hook_data(page, action)
- hook_data = {
- object_kind: page.class.name.underscore,
- user: current_user.hook_attrs,
- project: @project.hook_attrs,
- wiki: @project.wiki.hook_attrs,
- object_attributes: page.hook_attrs
- }
-
- page_url = Gitlab::UrlBuilder.build(page)
- hook_data[:object_attributes].merge!(url: page_url, action: action)
- hook_data
- end
-
private
def execute_hooks(page, action = 'create')
- page_data = hook_data(page, action)
+ page_data = Gitlab::DataBuilder::WikiPage.build(page, current_user, action)
@project.execute_hooks(page_data, :wiki_page_hooks)
@project.execute_services(page_data, :wiki_page_hooks)
end
diff --git a/app/views/admin/hooks/edit.html.haml b/app/views/admin/hooks/edit.html.haml
index 0e35a1905bf..665e8c7e74f 100644
--- a/app/views/admin/hooks/edit.html.haml
+++ b/app/views/admin/hooks/edit.html.haml
@@ -12,7 +12,7 @@
= render partial: 'form', locals: { form: f, hook: @hook }
.form-actions
= f.submit 'Save changes', class: 'btn btn-create'
- = link_to 'Test hook', test_admin_hook_path(@hook), class: 'btn btn-default'
+ = render 'shared/web_hooks/test_button', triggers: SystemHook::TRIGGERS, hook: @hook
= link_to 'Remove', admin_hook_path(@hook), method: :delete, class: 'btn btn-remove pull-right', data: { confirm: 'Are you sure?' }
%hr
diff --git a/app/views/admin/hooks/index.html.haml b/app/views/admin/hooks/index.html.haml
index e92b8bc39f4..fed6002528d 100644
--- a/app/views/admin/hooks/index.html.haml
+++ b/app/views/admin/hooks/index.html.haml
@@ -22,12 +22,12 @@
- @hooks.each do |hook|
%li
.controls
- = link_to 'Test hook', test_admin_hook_path(hook), class: 'btn btn-sm'
+ = render 'shared/web_hooks/test_button', triggers: SystemHook::TRIGGERS, hook: hook, button_class: 'btn-small'
= link_to 'Edit', edit_admin_hook_path(hook), class: 'btn btn-sm'
= link_to 'Remove', admin_hook_path(hook), data: { confirm: 'Are you sure?' }, method: :delete, class: 'btn btn-remove btn-sm'
.monospace= hook.url
%div
- - %w(repository_update_events push_events tag_push_events issues_events note_events merge_requests_events job_events).each do |trigger|
- - if hook.send(trigger)
- %span.label.label-gray= trigger.titleize
+ - SystemHook::TRIGGERS.each_value do |event|
+ - if hook.public_send(event)
+ %span.label.label-gray= event.to_s.titleize
%span.label.label-gray SSL Verification: #{hook.enable_ssl_verification ? 'enabled' : 'disabled'}
diff --git a/app/views/projects/hooks/edit.html.haml b/app/views/projects/hooks/edit.html.haml
index 4944e0c8041..c8c17d2d828 100644
--- a/app/views/projects/hooks/edit.html.haml
+++ b/app/views/projects/hooks/edit.html.haml
@@ -13,9 +13,10 @@
= render partial: 'shared/web_hooks/form', locals: { form: f, hook: @hook }
= f.submit 'Save changes', class: 'btn btn-create'
- = link_to 'Test hook', test_project_hook_path(@project, @hook), class: 'btn btn-default'
+ = render 'shared/web_hooks/test_button', triggers: ProjectHook::TRIGGERS, hook: @hook
= link_to 'Remove', project_hook_path(@project, @hook), method: :delete, class: 'btn btn-remove pull-right', data: { confirm: 'Are you sure?' }
%hr
= render partial: 'projects/hook_logs/index', locals: { hook: @hook, hook_logs: @hook_logs, project: @project }
+
diff --git a/app/views/projects/settings/integrations/_project_hook.html.haml b/app/views/projects/settings/integrations/_project_hook.html.haml
index 00700e286c3..d5792e95f5a 100644
--- a/app/views/projects/settings/integrations/_project_hook.html.haml
+++ b/app/views/projects/settings/integrations/_project_hook.html.haml
@@ -3,14 +3,14 @@
.col-md-8.col-lg-7
%strong.light-header= hook.url
%div
- - %w(push_events tag_push_events issues_events confidential_issues_events note_events merge_requests_events job_events pipeline_events wiki_page_events).each do |trigger|
- - if hook.send(trigger)
- %span.label.label-gray.deploy-project-label= trigger.titleize
+ - ProjectHook::TRIGGERS.each_value do |event|
+ - if hook.public_send(event)
+ %span.label.label-gray.deploy-project-label= event.to_s.titleize
.col-md-4.col-lg-5.text-right-lg.prepend-top-5
%span.append-right-10.inline
- SSL Verification: #{hook.enable_ssl_verification ? "enabled" : "disabled"}
- = link_to "Edit", edit_project_hook_path(@project, hook), class: "btn btn-sm"
- = link_to "Test", test_project_hook_path(@project, hook), class: "btn btn-sm"
- = link_to project_hook_path(@project, hook), data: { confirm: 'Are you sure?'}, method: :delete, class: "btn btn-transparent" do
+ SSL Verification: #{hook.enable_ssl_verification ? 'enabled' : 'disabled'}
+ = link_to 'Edit', edit_project_hook_path(@project, hook), class: 'btn btn-sm'
+ = render 'shared/web_hooks/test_button', triggers: ProjectHook::TRIGGERS, hook: hook, button_class: 'btn-small'
+ = link_to project_hook_path(@project, hook), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-transparent' do
%span.sr-only Remove
= icon('trash')
diff --git a/app/views/shared/web_hooks/_test_button.html.haml b/app/views/shared/web_hooks/_test_button.html.haml
new file mode 100644
index 00000000000..cf1d5e061c6
--- /dev/null
+++ b/app/views/shared/web_hooks/_test_button.html.haml
@@ -0,0 +1,12 @@
+- triggers = local_assigns.fetch(:triggers)
+- button_class = local_assigns.fetch(:button_class, '')
+- hook = local_assigns.fetch(:hook)
+
+.hook-test-button.dropdown.inline
+ %button.btn{ 'data-toggle' => 'dropdown', class: button_class }
+ Test
+ = icon('caret-down')
+ %ul.dropdown-menu.dropdown-menu-align-right{ role: 'menu' }
+ - triggers.each_value do |event|
+ %li
+ = link_to_test_hook(hook, event)