Browse Source

Fixes #5060 - Webhook custom payload not support complex object attribute values.

Co-authored-by: Tobias Schäfer <ts@zammad.com>
Co-authored-by: Dominik Klein <dk@zammad.com>
Dominik Klein 9 months ago
parent
commit
b02083fad9

+ 33 - 8
app/jobs/trigger_webhook_job/custom_payload/parser.rb

@@ -7,6 +7,12 @@ module TriggerWebhookJob::CustomPayload::Parser
 
   private
 
+  STRING_LIKE_CLASSES = %w[
+    String
+    ActiveSupport::TimeWithZone
+    ActiveSupport::Duration
+  ].freeze
+
   # This module validates the scanned replacement variables.
   def parse(variables, tracks)
     mappings = {}
@@ -29,19 +35,38 @@ module TriggerWebhookJob::CustomPayload::Parser
   # payload is valid JSON.
   def replace(record, mappings)
     mappings.each do |variable, value|
-      record.gsub!("\#{#{variable}}", value
-      .to_s
-      .gsub(%r{"}, '\"')
-      .gsub(%r{\n}, '\n')
-      .gsub(%r{\r}, '\r')
-      .gsub(%r{\t}, '\t')
-      .gsub(%r{\f}, '\f')
-      .gsub(%r{\v}, '\v'))
+      escaped_variable = Regexp.escape(variable)
+      pattern = %r{("\#\{#{escaped_variable}\}"|\#\{#{escaped_variable}\})}
+
+      is_string_like = value.class.to_s.in?(STRING_LIKE_CLASSES)
+
+      record.gsub!(pattern) do |match|
+        if match.start_with?('"')
+          escaped_value = escape_replace_value(value, is_string_like:)
+          is_string_like ? "\"#{escaped_value}\"" : escaped_value
+        else
+          escape_replace_value(value, is_string_like: true)
+        end
+      end
     end
 
     record
   end
 
+  def escape_replace_value(value, is_string_like: false)
+    if is_string_like
+      value.to_s
+        .gsub(%r{"}, '\"')
+        .gsub(%r{\n}, '\n')
+        .gsub(%r{\r}, '\r')
+        .gsub(%r{\t}, '\t')
+        .gsub(%r{\f}, '\f')
+        .gsub(%r{\v}, '\v')
+    else
+      value.to_json
+    end
+  end
+
   # Scan the custom payload for replacement variables.
   def scan(record)
     placeholders = record.scan(%r{(#\{[a-z_.?!]+\})}).flatten.uniq

+ 23 - 0
app/jobs/trigger_webhook_job/custom_payload/validator.rb

@@ -11,6 +11,8 @@ module TriggerWebhookJob::CustomPayload::Validator
     Integer
     String
     Float
+    FalseClass
+    TrueClass
   ].freeze
 
   ALLOWED_RAILS_CLASSES = %w[
@@ -18,6 +20,11 @@ module TriggerWebhookJob::CustomPayload::Validator
     ActiveSupport::Duration
   ].freeze
 
+  ALLOWED_CONTAINER_CLASSES = %w[
+    Hash
+    Array
+  ].freeze
+
   ALLOWED_DEFAULT_CLASSES = ALLOWED_SIMPLE_CLASSES + ALLOWED_RAILS_CLASSES
 
   # This method executes the replacement variables and executes on any error,
@@ -47,11 +54,27 @@ module TriggerWebhookJob::CustomPayload::Validator
 
   # Final value must be one of the above described classes.
   def validate_value!(value, display)
+    return validate_container_values(value) if value.class.to_s.in?(ALLOWED_CONTAINER_CLASSES)
     return "\#{#{display} / no such method}" if !value.class.to_s.in?(ALLOWED_DEFAULT_CLASSES)
 
     value
   end
 
+  def validate_container_values(container)
+    case container.class.to_s
+    when 'Array'
+      container.each_with_index do |value, index|
+        container[index] = value.class.to_s.in?(ALLOWED_DEFAULT_CLASSES) ? value : 'no such item'
+      end
+    when 'Hash'
+      container.each do |key, value|
+        container[key] = value.class.to_s.in?(ALLOWED_DEFAULT_CLASSES) ? value : 'no such item'
+      end
+    end
+
+    container
+  end
+
   # Any top level object must be provided by the tracks hash (ticket, article,
   # notification by default, any further information is related to the webhook
   # content).

+ 89 - 6
spec/jobs/trigger_webhook_job/custom_payload_spec.rb

@@ -113,7 +113,7 @@ RSpec.describe TriggerWebhookJob::CustomPayload do
 
     context 'when the placeholder contains valid object and method' do
       let(:record) { { 'ticket.id' => '#{ticket.id}' }.to_json }
-      let(:json_data) { { 'ticket.id' => ticket.id.to_s } }
+      let(:json_data) { { 'ticket.id' => ticket.id } }
 
       it 'returns the determined value' do
         expect(generate).to eq(json_data)
@@ -159,10 +159,20 @@ RSpec.describe TriggerWebhookJob::CustomPayload do
     end
 
     context 'when the placeholder contains multiple attributes' do
-      let(:record) { { 'my_field' => '#{ticket.id} // #{ticket.group.name}' }.to_json }
+      let(:record) do
+        {
+          'my_field'  => 'Test #{ticket.id} // #{ticket.group.name} Test',
+          'my_field2' => '#{ticket.id} // #{ticket.group.name} Test',
+          'my_field3' => '#{ticket.id}',
+          'my_field4' => '#{ticket.group.name}',
+        }.to_json
+      end
       let(:json_data) do
         {
-          'my_field' => "#{ticket.id} // #{ticket.group.name}",
+          'my_field'  => "Test #{ticket.id} // #{ticket.group.name} Test",
+          'my_field2' => "#{ticket.id} // #{ticket.group.name} Test",
+          'my_field3' => ticket.id,
+          'my_field4' => ticket.group.name.to_s,
         }
       end
 
@@ -184,7 +194,8 @@ RSpec.describe TriggerWebhookJob::CustomPayload do
               'created_at'  => '#{article.created_at}',
               'subject'     => '#{article.subject}',
               'body'        => '#{article.body}',
-              'attachments' => '#{article.attachments}'
+              'attachments' => '#{article.attachments}',
+              'internal'    => '#{article.internal}',
             }
           }
         }.to_json
@@ -193,15 +204,16 @@ RSpec.describe TriggerWebhookJob::CustomPayload do
         {
           'current_user' => '#{current_user / no such object}',
           'ticket'       => {
-            'id'      => ticket.id.to_s,
+            'id'      => ticket.id,
             'owner'   => ticket.owner.fullname.to_s,
             'group'   => ticket.group.name.to_s,
             'article' => {
-              'id'          => article.id.to_s,
+              'id'          => article.id,
               'created_at'  => article.created_at.to_s,
               'subject'     => article.subject.to_s,
               'body'        => article.body.to_s,
               'attachments' => '#{article.attachments / no such method}',
+              'internal'    => article.internal
             }
           }
         }
@@ -209,6 +221,7 @@ RSpec.describe TriggerWebhookJob::CustomPayload do
 
       it 'returns a valid JSON payload' do
         expect(generate).to eq(json_data)
+
       end
     end
 
@@ -222,6 +235,76 @@ RSpec.describe TriggerWebhookJob::CustomPayload do
       end
     end
 
+    context 'when object attributes are used in the placeholder', db_strategy: :reset do
+      let(:ticket) { create(:ticket, object_manager_attribute_name => object_manager_attribute_value) }
+
+      before do
+        create_object_manager_attribute
+        ObjectManager::Attribute.migration_execute
+      end
+
+      shared_examples 'check different usage' do
+        context 'when used in string context' do
+          let(:record) do
+            {
+              "ticket.#{object_manager_attribute_name}" => "Test \#{ticket.#{object_manager_attribute_name}}"
+            }.to_json
+          end
+          let(:json_data) do
+            {
+              "ticket.#{object_manager_attribute_name}" => "Test #{object_manager_attribute_value}"
+            }
+          end
+
+          it 'returns the determined value' do
+            expect(generate).to eq(json_data)
+          end
+        end
+
+        context 'when used in direct context' do
+          let(:record) do
+            {
+              "ticket.#{object_manager_attribute_name}" => "\#{ticket.#{object_manager_attribute_name}}"
+            }.to_json
+          end
+          let(:json_data) do
+            {
+              "ticket.#{object_manager_attribute_name}" => object_manager_attribute_value
+            }
+          end
+
+          it 'returns the determined value' do
+            expect(generate).to eq(json_data)
+          end
+        end
+      end
+
+      context 'when multiselect is used inside the ticket' do
+        let(:object_manager_attribute_name)  { 'multiselect' }
+        let(:object_manager_attribute_value) { %w[key_1 key_3] }
+        let(:create_object_manager_attribute) do
+          create(:object_manager_attribute_multiselect, name: object_manager_attribute_name)
+        end
+
+        include_examples 'check different usage'
+      end
+
+      context 'when external data source is used inside the ticket', db_adapter: :postgresql do
+        let(:object_manager_attribute_name)  { 'autocompletion_ajax_external_data_source' }
+        let(:object_manager_attribute_value) do
+          {
+            'value' => 123,
+            'label' => 'Example',
+          }
+        end
+        let(:create_object_manager_attribute) do
+          create(:object_manager_attribute_autocompletion_ajax_external_data_source, name: object_manager_attribute_name)
+        end
+
+        include_examples 'check different usage'
+      end
+    end
+
     describe "when the placeholder contains object 'notification'" do
       let(:record) do
         {