Commit 1b956a6e authored by Alex Kalderimis's avatar Alex Kalderimis

Always pass data payloads as a hash for hooks

Web hook payloads must be plain hashes of data - they are serialized
when sending to sidekiq and when storing in the database.

To support this, we ensure that the payload is in
fact a hash in the execution service.

This adds a regression test
for https://gitlab.com/gitlab-org/gitlab/-/issues/353567, and simplifies
log_execution, since serveral of the arguments to this method are
fixed.

Changelog: fixed
parent c59e63c0
...@@ -36,7 +36,7 @@ class WebHookService ...@@ -36,7 +36,7 @@ class WebHookService
def initialize(hook, data, hook_name, uniqueness_token = nil, force: false) def initialize(hook, data, hook_name, uniqueness_token = nil, force: false)
@hook = hook @hook = hook
@data = data @data = data.to_h
@hook_name = hook_name.to_s @hook_name = hook_name.to_s
@uniqueness_token = uniqueness_token @uniqueness_token = uniqueness_token
@force = force @force = force
...@@ -70,9 +70,6 @@ class WebHookService ...@@ -70,9 +70,6 @@ class WebHookService
end end
log_execution( log_execution(
trigger: hook_name,
url: hook.url,
request_data: data,
response: response, response: response,
execution_duration: Gitlab::Metrics::System.monotonic_time - start_time execution_duration: Gitlab::Metrics::System.monotonic_time - start_time
) )
...@@ -86,9 +83,6 @@ class WebHookService ...@@ -86,9 +83,6 @@ class WebHookService
Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError => e Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError => e
execution_duration = Gitlab::Metrics::System.monotonic_time - start_time execution_duration = Gitlab::Metrics::System.monotonic_time - start_time
log_execution( log_execution(
trigger: hook_name,
url: hook.url,
request_data: data,
response: InternalErrorResponse.new, response: InternalErrorResponse.new,
execution_duration: execution_duration, execution_duration: execution_duration,
error_message: e.to_s error_message: e.to_s
...@@ -139,14 +133,14 @@ class WebHookService ...@@ -139,14 +133,14 @@ class WebHookService
make_request(post_url, basic_auth) make_request(post_url, basic_auth)
end end
def log_execution(trigger:, url:, request_data:, response:, execution_duration:, error_message: nil) def log_execution(response:, execution_duration:, error_message: nil)
category = response_category(response) category = response_category(response)
log_data = { log_data = {
trigger: trigger, trigger: hook_name,
url: url, url: hook.url,
execution_duration: execution_duration, execution_duration: execution_duration,
request_headers: build_headers, request_headers: build_headers,
request_data: request_data, request_data: data,
response_headers: format_response_headers(response), response_headers: format_response_headers(response),
response_body: safe_response_body(response), response_body: safe_response_body(response),
response_status: response.code, response_status: response.code,
......
...@@ -107,6 +107,21 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state ...@@ -107,6 +107,21 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state
).once ).once
end end
context 'when the data is a Gitlab::DataBuilder::Pipeline' do
let(:pipeline) { create(:ci_pipeline, project: project) }
let(:data) { ::Gitlab::DataBuilder::Pipeline.new(pipeline) }
it 'can log the request payload' do
stub_full_request(project_hook.url, method: :post)
# we call this with force to ensure that the logs are written inline,
# which tests that we can serialize the data to the DB correctly.
service = described_class.new(project_hook, data, :push_hooks, force: true)
expect { service.execute }.to change(::WebHookLog, :count).by(1)
end
end
context 'when auth credentials are present' do context 'when auth credentials are present' do
let_it_be(:url) {'https://example.org'} let_it_be(:url) {'https://example.org'}
let_it_be(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') } let_it_be(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') }
......
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