Merge branch 'log_service_web_hooks' into 'master'

Log service web hooks

See merge request gitlab-org/gitlab!20976

app/controllers/projects/hook_logs_controller.rb

@@ -16,15 +16,17 @@ class Projects::HookLogsController < Projects::ApplicationController
   end
 
   def retry
-    result = hook.execute(hook_log.request_data, hook_log.trigger)
-
-    set_hook_execution_notice(result)
-
+    execute_hook
     redirect_to edit_project_hook_path(@project, @hook)
   end
 
   private
 
+  def execute_hook
+    result = hook.execute(hook_log.request_data, hook_log.trigger)
+    set_hook_execution_notice(result)
+  end
+
   def hook
     @hook ||= @project.hooks.find(params[:hook_id])
   end

app/controllers/projects/service_hook_logs_controller.rb

+# frozen_string_literal: true
+
+class Projects::ServiceHookLogsController < Projects::HookLogsController
+  before_action :service, only: [:show, :retry]
+
+  def retry
+    execute_hook
+    redirect_to edit_project_service_path(@project, @service)
+  end
+
+  private
+
+  def hook
+    @hook ||= service.service_hook
+  end
+
+  def service
+    @service ||= @project.find_or_initialize_service(params[:service_id])
+  end
+end

app/controllers/projects/services_controller.rb

@@ -7,6 +7,7 @@ class Projects::ServicesController < Projects::ApplicationController
   before_action :authorize_admin_project!
   before_action :ensure_service_enabled
   before_action :service
+  before_action :web_hook_logs, only: [:edit, :update]
 
   respond_to :html
 
@@ -77,6 +78,12 @@ class Projects::ServicesController < Projects::ApplicationController
     @service ||= @project.find_or_initialize_service(params[:id])
   end
 
+  def web_hook_logs
+    return unless @service.service_hook.present?
+
+    @web_hook_logs ||= @service.service_hook.web_hook_logs.recent.page(params[:page])
+  end
+
   def ensure_service_enabled
     render_404 unless service
   end

app/models/concerns/safe_url.rb

+# frozen_string_literal: true
+
+module SafeUrl
+  extend ActiveSupport::Concern
+
+  def safe_url(usernames_whitelist: [])
+    return if url.nil?
+
+    uri = URI.parse(url)
+    uri.password = '*****' if uri.password
+    uri.user = '*****' if uri.user && !usernames_whitelist.include?(uri.user)
+    uri.to_s
+  rescue URI::Error
+  end
+end

app/models/hooks/project_hook.rb

@@ -2,6 +2,7 @@
 
 class ProjectHook < WebHook
   include TriggerableHooks
+  include Presentable
 
   triggerable_hooks [
     :push_hooks,

app/models/hooks/service_hook.rb

 # frozen_string_literal: true
 
 class ServiceHook < WebHook
+  include Presentable
+
   belongs_to :service
   validates :service, presence: true
 

app/models/hooks/web_hook_log.rb

 # frozen_string_literal: true
 
 class WebHookLog < ApplicationRecord
+  include SafeUrl
+  include Presentable
+
   belongs_to :web_hook
 
   serialize :request_headers, Hash # rubocop:disable Cop/ActiveRecordSerialize
@@ -9,6 +12,8 @@ class WebHookLog < ApplicationRecord
 
   validates :web_hook, presence: true
 
+  before_save :obfuscate_basic_auth
+
   def self.recent
     where('created_at >= ?', 2.days.ago.beginning_of_day)
       .order(created_at: :desc)
@@ -17,4 +22,10 @@ class WebHookLog < ApplicationRecord
   def success?
     response_status =~ /^2/
   end
+
+  private
+
+  def obfuscate_basic_auth
+    self.url = safe_url
+  end
 end

app/models/remote_mirror.rb

@@ -3,6 +3,7 @@
 class RemoteMirror < ApplicationRecord
   include AfterCommitQueue
   include MirrorAuthentication
+  include SafeUrl
 
   MAX_FIRST_RUNTIME = 3.hours
   MAX_INCREMENTAL_RUNTIME = 1.hour
@@ -194,13 +195,7 @@ class RemoteMirror < ApplicationRecord
   end
 
   def safe_url
-    return if url.nil?
-
-    result = URI.parse(url)
-    result.password = '*****' if result.password
-    result.user = '*****' if result.user && result.user != 'git' # tokens or other data may be saved as user
-    result.to_s
-  rescue URI::Error
+    super(usernames_whitelist: %w[git])
   end
 
   def ensure_remote!

app/presenters/hooks/project_hook_presenter.rb

+# frozen_string_literal: true
+
+class ProjectHookPresenter < Gitlab::View::Presenter::Delegated
+  presents :project_hook
+
+  def logs_details_path(log)
+    project_hook_hook_log_path(project, self, log)
+  end
+
+  def logs_retry_path(log)
+    retry_project_hook_hook_log_path(project, self, log)
+  end
+end

app/presenters/hooks/service_hook_presenter.rb

+# frozen_string_literal: true
+
+class ServiceHookPresenter < Gitlab::View::Presenter::Delegated
+  presents :service_hook
+
+  def logs_details_path(log)
+    project_service_hook_log_path(service.project, service, log)
+  end
+
+  def logs_retry_path(log)
+    retry_project_service_hook_log_path(service.project, service, log)
+  end
+end

app/presenters/web_hook_log_presenter.rb

+# frozen_string_literal: true
+
+class WebHookLogPresenter < Gitlab::View::Presenter::Delegated
+  presents :web_hook_log
+
+  def details_path
+    web_hook.present.logs_details_path(self)
+  end
+
+  def retry_path
+    web_hook.present.logs_retry_path(self)
+  end
+end

app/services/web_hook_service.rb

@@ -92,9 +92,6 @@ class WebHookService
   end
 
   def log_execution(trigger:, url:, request_data:, response:, execution_duration:, error_message: nil)
-    # logging for ServiceHook's is not available
-    return if hook.is_a?(ServiceHook)
-
     WebHookLog.create(
       web_hook: hook,
       trigger: trigger,

app/views/projects/hook_logs/_index.html.haml

@@ -28,7 +28,7 @@
             %td.light
               = time_ago_with_tooltip(hook_log.created_at)
             %td
-              = link_to 'View details', project_hook_hook_log_path(project, hook, hook_log)
+              = link_to 'View details', hook_log.present.details_path
 
       = paginate hook_logs, theme: 'gitlab'
 

app/views/projects/hook_logs/show.html.haml

@@ -3,7 +3,6 @@
     %h4.prepend-top-0
       Request details
   .col-lg-9
-
-    = link_to 'Resend Request', retry_project_hook_hook_log_path(@project, @hook, @hook_log), method: :post, class: "btn btn-default float-right prepend-left-10"
+    = link_to 'Resend Request', @hook_log.present.retry_path, method: :post, class: "btn btn-default float-right prepend-left-10"
 
     = render partial: 'shared/hook_logs/content', locals: { hook_log: @hook_log }

app/views/projects/services/edit.html.haml

 - breadcrumb_title @service.title
 - page_title @service.title, s_("ProjectService|Services")
 - add_to_breadcrumbs(s_("ProjectService|Settings"), edit_project_path(@project))
-- add_to_breadcrumbs(s_("ProjectService|Integrations"), namespace_project_settings_integrations_path)
+- add_to_breadcrumbs(s_("ProjectService|Integrations"), project_settings_integrations_path(@project))
 
 = render 'deprecated_message' if @service.deprecation_message
 
 = render 'form'
+- if @web_hook_logs
+  = render partial: 'projects/hook_logs/index', locals: { hook: @service.service_hook, hook_logs: @web_hook_logs, project: @project }

changelogs/unreleased/log_service_web_hooks.yml

+---
+title: Added WebHookLogs for ServiceHooks
+merge_request: 20976
+author:
+type: added

config/routes/project.rb

@@ -159,6 +159,12 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           member do
             put :test
           end
+
+          resources :hook_logs, only: [:show], controller: :service_hook_logs do
+            member do
+              post :retry
+            end
+          end
         end
 
         resources :boards, only: [:index, :show, :create, :update, :destroy], constraints: { id: /\d+/ } do

spec/controllers/projects/service_hook_logs_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::ServiceHookLogsController do
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user) }
+  let(:service) { create(:drone_ci_service, project: project) }
+  let(:log) { create(:web_hook_log, web_hook: service.service_hook) }
+  let(:log_params) do
+    {
+      namespace_id: project.namespace,
+      project_id: project,
+      service_id: service.to_param,
+      id: log.id
+    }
+  end
+
+  before do
+    sign_in(user)
+    project.add_maintainer(user)
+  end
+
+  describe 'GET #show' do
+    subject { get :show, params: log_params }
+
+    it do
+      expect(response).to be_successful
+    end
+  end
+
+  describe 'POST #retry' do
+    subject { post :retry, params: log_params }
+
+    it 'executes the hook and redirects to the service form' do
+      expect_any_instance_of(ServiceHook).to receive(:execute)
+      expect_any_instance_of(described_class).to receive(:set_hook_execution_notice)
+      expect(subject).to redirect_to(edit_project_service_path(project, service))
+    end
+  end
+end

spec/factories/services.rb

@@ -44,6 +44,13 @@ FactoryBot.define do
     end
   end
 
+  factory :drone_ci_service do
+    project
+    active { true }
+    drone_url { 'https://bamboo.example.com' }
+    token { 'test' }
+  end
+
   factory :jira_service do
     project
     active { true }

spec/models/concerns/safe_url_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe SafeUrl do
+  describe '#safe_url' do
+    class TestClass
+      include SafeUrl
+
+      attr_reader :url
+
+      def initialize(url)
+        @url = url
+      end
+    end
+
+    let(:test_class) { TestClass.new(url) }
+    let(:url) { 'http://example.com' }
+
+    subject { test_class.safe_url }
+
+    it { is_expected.to eq(url) }
+
+    context 'when URL contains credentials' do
+      let(:url) { 'http://foo:bar@example.com' }
+
+      it { is_expected.to eq('http://*****:*****@example.com')}
+
+      context 'when username is whitelisted' do
+        subject { test_class.safe_url(usernames_whitelist: usernames_whitelist) }
+
+        let(:usernames_whitelist) { %w[foo] }
+
+        it 'does expect the whitelisted username not to be masked' do
+          is_expected.to eq('http://foo:*****@example.com')
+        end
+      end
+    end
+
+    context 'when URL is empty' do
+      let(:url) { nil }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'when URI raises an error' do
+      let(:url) { 123 }
+
+      it { is_expected.to be_nil }
+    end
+  end
+end

spec/models/hooks/web_hook_log_spec.rb

@@ -29,6 +29,25 @@ describe WebHookLog do
     end
   end
 
+  describe '#save' do
+    let(:web_hook_log) { build(:web_hook_log, url: url) }
+    let(:url) { 'http://example.com' }
+
+    subject { web_hook_log.save! }
+
+    it { is_expected.to eq(true) }
+
+    context 'with basic auth credentials' do
+      let(:url) { 'http://test:123@example.com'}
+
+      it 'obfuscates the basic auth credentials' do
+        subject
+
+        expect(web_hook_log.url).to eq('http://*****:*****@example.com')
+      end
+    end
+  end
+
   describe '#success?' do
     let(:web_hook_log) { build(:web_hook_log, response_status: status) }
 

spec/presenters/hooks/project_hook_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ProjectHookPresenter do
+  let(:web_hook_log) { create(:web_hook_log) }
+  let(:project) { web_hook_log.web_hook.project }
+  let(:web_hook) { web_hook_log.web_hook }
+
+  describe '#logs_details_path' do
+    subject { web_hook.present.logs_details_path(web_hook_log) }
+
+    let(:expected_path) do
+      "/#{project.namespace.path}/#{project.name}/hooks/#{web_hook.id}/hook_logs/#{web_hook_log.id}"
+    end
+
+    it { is_expected.to eq(expected_path) }
+  end
+
+  describe '#logs_retry_path' do
+    subject { web_hook.present.logs_details_path(web_hook_log) }
+
+    let(:expected_path) do
+      "/#{project.namespace.path}/#{project.name}/hooks/#{web_hook.id}/hook_logs/#{web_hook_log.id}"
+    end
+
+    it { is_expected.to eq(expected_path) }
+  end
+end

spec/presenters/hooks/service_hook_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ServiceHookPresenter do
+  let(:web_hook_log) { create(:web_hook_log, web_hook: service_hook) }
+  let(:service_hook) { create(:service_hook, service: service) }
+  let(:service) { create(:drone_ci_service, project: project) }
+  let(:project) { create(:project) }
+
+  describe '#logs_details_path' do
+    subject { service_hook.present.logs_details_path(web_hook_log) }
+
+    let(:expected_path) do
+      "/#{project.namespace.path}/#{project.name}/-/services/#{service.to_param}/hook_logs/#{web_hook_log.id}"
+    end
+
+    it { is_expected.to eq(expected_path) }
+  end
+
+  describe '#logs_retry_path' do
+    subject { service_hook.present.logs_retry_path(web_hook_log) }
+
+    let(:expected_path) do
+      "/#{project.namespace.path}/#{project.name}/-/services/#{service.to_param}/hook_logs/#{web_hook_log.id}/retry"
+    end
+
+    it { is_expected.to eq(expected_path) }
+  end
+end

spec/presenters/web_hook_log_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe WebHookLogPresenter do
+  include Gitlab::Routing.url_helpers
+
+  describe '#details_path' do
+    let(:web_hook_log) { create(:web_hook_log, web_hook: web_hook) }
+    let(:project) { create(:project) }
+
+    subject { web_hook_log.present.details_path }
+
+    context 'project hook' do
+      let(:web_hook) { create(:project_hook, project: project) }
+
+      it { is_expected.to eq(project_hook_hook_log_path(project, web_hook, web_hook_log)) }
+    end
+
+    context 'service hook' do
+      let(:web_hook) { create(:service_hook, service: service) }
+      let(:service) { create(:drone_ci_service, project: project) }
+
+      it { is_expected.to eq(project_service_hook_log_path(project, service, web_hook_log)) }
+    end
+  end
+
+  describe '#retry_path' do
+    let(:web_hook_log) { create(:web_hook_log, web_hook: web_hook) }
+    let(:project) { create(:project) }
+
+    subject { web_hook_log.present.retry_path }
+
+    context 'project hook' do
+      let(:web_hook) { create(:project_hook, project: project) }
+
+      it { is_expected.to eq(retry_project_hook_hook_log_path(project, web_hook, web_hook_log)) }
+    end
+
+    context 'service hook' do
+      let(:web_hook) { create(:service_hook, service: service) }
+      let(:service) { create(:drone_ci_service, project: project) }
+
+      it { is_expected.to eq(retry_project_service_hook_log_path(project, service, web_hook_log)) }
+    end
+  end
+end

spec/services/web_hook_service_spec.rb

@@ -203,17 +203,6 @@ describe WebHookService do
           expect(hook_log.internal_error_message).to be_nil
         end
       end
-
-      context 'should not log ServiceHooks' do
-        let(:service_hook) { create(:service_hook) }
-        let(:service_instance) { described_class.new(service_hook, data, 'service_hook') }
-
-        before do
-          stub_full_request(service_hook.url, method: :post).to_return(status: 200, body: 'Success')
-        end
-
-        it { expect { service_instance.execute }.not_to change(WebHookLog, :count) }
-      end
     end
   end
 

spec/views/projects/services/edit.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'projects/services/edit' do
+  let(:service) { create(:drone_ci_service, project: project) }
+  let(:project) { create(:project) }
+
+  before do
+    assign :project, project
+    assign :service, service
+  end
+
+  it do
+    render
+
+    expect(rendered).not_to have_text('Recent Deliveries')
+  end
+
+  context 'service using WebHooks' do
+    before do
+      assign(:web_hook_logs, [])
+    end
+
+    it do
+      render
+
+      expect(rendered).to have_text('Recent Deliveries')
+    end
+  end
+end