Commit af67a682 authored by Andy Soiron's avatar Andy Soiron Committed by Kamil Trzciński

Load webhook logs only if service has a hook

Some tests failed because we tried to render webhook
logs for services that do not use webhooks
parent 63870e78
......@@ -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
......
# 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
......@@ -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
......
# 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
......@@ -2,6 +2,7 @@
class ProjectHook < WebHook
include TriggerableHooks
include Presentable
triggerable_hooks [
:push_hooks,
......
# frozen_string_literal: true
class ServiceHook < WebHook
include Presentable
belongs_to :service
validates :service, presence: true
......
# 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
......@@ -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!
......
# 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
# 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
# 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
......@@ -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,
......
......@@ -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'
......
......@@ -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 }
- 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 }
---
title: Added WebHookLogs for ServiceHooks
merge_request: 20976
author:
type: added
......@@ -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
......
# 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
......@@ -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 }
......
# 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
......@@ -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) }
......
# 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
# 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
# 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
......@@ -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
......
# 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
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