Merge pull request #16468 from opf/fix/webhooks-error

Also log errors on errors without response

modules/webhooks/app/components/webhooks/outgoing/deliveries/row_component.rb

@@ -2,7 +2,7 @@ module ::Webhooks
   module Outgoing
     module Deliveries
       class RowComponent < ::RowComponent
-        property :id, :description, :event_name, :response_code
+        property :id, :description, :event_name
 
         def log
           model
@@ -12,6 +12,14 @@ def time
           model.updated_at.to_s # Force ISO8601
         end
 
+        def response_code
+          if log.response_code <= 0
+            I18n.t(:label_none_parentheses)
+          else
+            log.response_code
+          end
+        end
+
         def response_body
           render ResponseComponent.new(log)
         end

modules/webhooks/app/models/webhooks/log.rb

@@ -9,7 +9,7 @@ class Log < ApplicationRecord
     serialize :response_headers, type: Hash
     serialize :request_headers, type: Hash
 
-    validates :request_headers, presence: true
+    validates :request_headers, presence: true, allow_blank: true
     validates :request_body, presence: true
 
     def self.newest(limit: 10)

modules/webhooks/app/services/webhooks/outgoing/request_webhook_service.rb

@@ -0,0 +1,60 @@
+module Webhooks
+  module Outgoing
+    class RequestWebhookService
+      include ::OpenProjectErrorHelper
+
+      attr_reader :current_user, :event_name, :webhook
+
+      def initialize(webhook, event_name:, current_user:)
+        @current_user = current_user
+        @webhook = webhook
+        @event_name = event_name
+      end
+
+      def call!(body:, headers:)
+        begin
+          response = Faraday.post(
+            webhook.url,
+            body,
+            headers
+          )
+        rescue Faraday::Error => e
+          response = e.response
+          exception = e
+        rescue StandardError => e
+          op_handle_error(e.message, reference: :webhook_job)
+          exception = e
+        end
+
+        log!(body:, headers:, response:, exception:)
+
+        # We want to re-raise timeout exceptions
+        # but log the request beforehand
+        raise exception if exception.is_a?(Faraday::TimeoutError)
+      end
+
+      def log!(body:, headers:, response:, exception:)
+        log = ::Webhooks::Log.new(
+          webhook:,
+          event_name:,
+          url: webhook.url,
+          request_headers: headers,
+          request_body: body,
+          **response_attributes(response:, exception:)
+        )
+
+        unless log.save
+          OpenProject.logger.error("Failed to save webhook log: #{log.errors.full_messages.join('. ')}")
+        end
+      end
+
+      def response_attributes(response:, exception:)
+        {
+          response_code: response&.status || -1,
+          response_headers: response&.headers&.to_h&.transform_keys { |k| k.underscore.to_sym },
+          response_body: response&.body || exception&.message
+        }
+      end
+    end
+  end
+end

modules/webhooks/app/workers/represented_webhook_job.rb

@@ -27,8 +27,6 @@
 #++
 
 class RepresentedWebhookJob < WebhookJob
-  include ::OpenProjectErrorHelper
-
   attr_reader :resource
 
   def perform(webhook_id, resource, event_name)
@@ -39,49 +37,22 @@ def perform(webhook_id, resource, event_name)
 
     body = request_body
     headers = request_headers
-    exception = nil
-    response = nil
 
-    if signature = request_signature(body)
+    if (signature = request_signature(body))
       headers["X-OP-Signature"] = signature
     end
 
-    begin
-      response = Faraday.post(
-        webhook.url,
-        request_body,
-        headers
-      )
-    rescue Faraday::Error => e
-      response = e.response
-      exception = e
-    rescue StandardError => e
-      op_handle_error(e.message, reference: :webhook_job)
-      exception = e
-    end
-
-    ::Webhooks::Log.create(
-      webhook:,
-      event_name:,
-      url: webhook.url,
-      request_headers: headers,
-      request_body: body,
-      response_code: response&.status,
-      response_headers: response&.headers&.to_h&.transform_keys { |k| k.underscore.to_sym },
-      response_body: response&.body || exception&.message
-    )
-
-    # We want to re-raise timeout exceptions
-    # but log the request beforehand
-    raise exception if exception.is_a?(Faraday::TimeoutError)
+    ::Webhooks::Outgoing::RequestWebhookService
+      .new(webhook, event_name:, current_user: User.system)
+      .call!(body:, headers:)
   end
 
   def accepted_in_project?
     webhook.enabled_for_project?(resource.project_id)
   end
 
   def request_signature(request_body)
-    if secret = webhook.secret.presence
+    if (secret = webhook.secret.presence)
       "sha1=#{OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), secret, request_body)}"
     end
   end

modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb

@@ -0,0 +1,52 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+require "spec_helper"
+
+RSpec.describe Webhooks::Outgoing::RequestWebhookService, type: :model do
+  let(:user) { build_stubbed(:user) }
+  let(:instance) { described_class.new(webhook, event_name: :created, current_user: user) }
+
+  shared_let(:webhook) { create(:webhook, all_projects: true, url: "https://example.net/test/42", secret: nil) }
+
+  subject { instance.call!(body: "body", headers: {}) }
+
+  describe "#call!" do
+    context "when request_url fails with SSL errors" do
+      it "still logs the exception" do
+        allow(Faraday)
+          .to receive(:post)
+          .with(webhook.url, *any_args)
+          .and_raise(Faraday::SSLError, "SSL error")
+
+        expect { subject }
+          .to change(Webhooks::Log, :count).by(1)
+      end
+    end
+  end
+end