From ca88877bc89b486e64f5c9fddb4fd09e2e0f5afd Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Tue, 10 Nov 2020 11:31:13 +0100 Subject: [PATCH] Fixes #290, Closes #3185 - Webhooks. --- .../_ui_element/ticket_perform_action.coffee | 8 +- .../ticket_perform_action/article.jst.eco | 2 +- .../notification.jst.eco | 2 +- .../ticket_perform_action/webhook.jst.eco | 24 +++++ app/jobs/trigger_webhook_job.rb | 73 +++++++++++++++ .../trigger_webhook_job/record_payload.rb | 10 +++ .../record_payload/base.rb | 56 ++++++++++++ .../record_payload/ticket.rb | 3 + .../record_payload/ticket/article.rb | 28 ++++++ app/jobs/trigger_webhook_job/request_error.rb | 2 + .../concerns/checks_perform_validation.rb | 7 +- app/models/ticket.rb | 3 +- .../record_payload/base_example.rb | 31 +++++++ .../record_payload/ticket/article_spec.rb | 45 ++++++++++ .../record_payload/ticket_spec.rb | 6 ++ .../record_payload_spec.rb | 43 +++++++++ spec/jobs/trigger_webhook_job_spec.rb | 89 +++++++++++++++++++ spec/models/ticket_spec.rb | 16 ++++ 18 files changed, 441 insertions(+), 7 deletions(-) create mode 100644 app/assets/javascripts/app/views/generic/ticket_perform_action/webhook.jst.eco create mode 100644 app/jobs/trigger_webhook_job.rb create mode 100644 app/jobs/trigger_webhook_job/record_payload.rb create mode 100644 app/jobs/trigger_webhook_job/record_payload/base.rb create mode 100644 app/jobs/trigger_webhook_job/record_payload/ticket.rb create mode 100644 app/jobs/trigger_webhook_job/record_payload/ticket/article.rb create mode 100644 app/jobs/trigger_webhook_job/request_error.rb create mode 100644 spec/jobs/trigger_webhook_job/record_payload/base_example.rb create mode 100644 spec/jobs/trigger_webhook_job/record_payload/ticket/article_spec.rb create mode 100644 spec/jobs/trigger_webhook_job/record_payload/ticket_spec.rb create mode 100644 spec/jobs/trigger_webhook_job/record_payload_spec.rb create mode 100644 spec/jobs/trigger_webhook_job_spec.rb diff --git a/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee b/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee index c5175c989..a46e6de40 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee +++ b/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee @@ -23,6 +23,7 @@ class App.UiElement.ticket_perform_action if groupKey is 'notification' elements["#{groupKey}.email"] = { name: 'email', display: 'Email' } elements["#{groupKey}.sms"] = { name: 'sms', display: 'SMS' } + elements["#{groupKey}.webhook"] = { name: 'webhook', display: 'Webhook' } else if groupKey is 'article' elements["#{groupKey}.note"] = { name: 'note', display: 'Note' } else @@ -395,12 +396,17 @@ class App.UiElement.ticket_perform_action selectionRecipient = columnSelectRecipient.element() - notificationElement = $( App.view('generic/ticket_perform_action/notification')( + elementTemplate = 'notification' + if notificationType is 'webhook' + elementTemplate = 'webhook' + + notificationElement = $( App.view("generic/ticket_perform_action/#{elementTemplate}")( attribute: attribute name: name notificationType: notificationType meta: meta || {} )) + notificationElement.find('.js-recipient select').replaceWith(selectionRecipient) visibilitySelection = App.UiElement.select.render( diff --git a/app/assets/javascripts/app/views/generic/ticket_perform_action/article.jst.eco b/app/assets/javascripts/app/views/generic/ticket_perform_action/article.jst.eco index c8662a06a..b673408c5 100644 --- a/app/assets/javascripts/app/views/generic/ticket_perform_action/article.jst.eco +++ b/app/assets/javascripts/app/views/generic/ticket_perform_action/article.jst.eco @@ -9,7 +9,7 @@
- +
diff --git a/app/assets/javascripts/app/views/generic/ticket_perform_action/notification.jst.eco b/app/assets/javascripts/app/views/generic/ticket_perform_action/notification.jst.eco index 5550fc97d..9f912dba7 100644 --- a/app/assets/javascripts/app/views/generic/ticket_perform_action/notification.jst.eco +++ b/app/assets/javascripts/app/views/generic/ticket_perform_action/notification.jst.eco @@ -17,7 +17,7 @@
-
+
<% end %>
diff --git a/app/assets/javascripts/app/views/generic/ticket_perform_action/webhook.jst.eco b/app/assets/javascripts/app/views/generic/ticket_perform_action/webhook.jst.eco new file mode 100644 index 000000000..9cb3d179f --- /dev/null +++ b/app/assets/javascripts/app/views/generic/ticket_perform_action/webhook.jst.eco @@ -0,0 +1,24 @@ +
+
+ +
+
+ +
+
+
+
+ +
+
+ +
+
+
+
+ +
+
+ checked<% end %>> +
+
diff --git a/app/jobs/trigger_webhook_job.rb b/app/jobs/trigger_webhook_job.rb new file mode 100644 index 000000000..6857cad2c --- /dev/null +++ b/app/jobs/trigger_webhook_job.rb @@ -0,0 +1,73 @@ +class TriggerWebhookJob < ApplicationJob + + USER_ATTRIBUTE_BLACKLIST = %w[ + last_login + login_failed + password + preferences + group_ids + groups + authorization_ids + authorizations + ].freeze + + attr_reader :ticket, :trigger, :article + + retry_on TriggerWebhookJob::RequestError, attempts: 5, wait: lambda { |executions| + executions * 10.seconds + } + + def perform(trigger, ticket, article) + @trigger = trigger + @ticket = ticket + @article = article + + return if request.success? + + raise TriggerWebhookJob::RequestError + end + + private + + def request + UserAgent.post( + config['endpoint'], + payload, + { + json: true, + jsonParseDisable: true, + open_timeout: 4, + read_timeout: 30, + total_timeout: 60, + headers: headers, + signature_token: config['token'], + verify_ssl: verify_ssl?, + log: { + facility: 'webhook', + }, + }, + ) + end + + def config + @config ||= trigger.perform['notification.webhook'] + end + + def verify_ssl? + config.fetch('verify_ssl', false).present? + end + + def headers + { + 'X-Zammad-Trigger' => trigger.name, + 'X-Zammad-Delivery' => job_id + } + end + + def payload + { + ticket: TriggerWebhookJob::RecordPayload.generate(ticket), + article: TriggerWebhookJob::RecordPayload.generate(article), + } + end +end diff --git a/app/jobs/trigger_webhook_job/record_payload.rb b/app/jobs/trigger_webhook_job/record_payload.rb new file mode 100644 index 000000000..5e59ed3c0 --- /dev/null +++ b/app/jobs/trigger_webhook_job/record_payload.rb @@ -0,0 +1,10 @@ +class TriggerWebhookJob::RecordPayload + + def self.generate(record) + return {} if record.blank? + + backend = "TriggerWebhookJob::RecordPayload::#{record.class.name}".constantize + generator = backend.new(record) + generator.generate + end +end diff --git a/app/jobs/trigger_webhook_job/record_payload/base.rb b/app/jobs/trigger_webhook_job/record_payload/base.rb new file mode 100644 index 000000000..2583a9ee1 --- /dev/null +++ b/app/jobs/trigger_webhook_job/record_payload/base.rb @@ -0,0 +1,56 @@ +class TriggerWebhookJob::RecordPayload::Base + + USER_ATTRIBUTE_BLACKLIST = %w[ + last_login + login_failed + password + preferences + group_ids + groups + authorization_ids + authorizations + ].freeze + + attr_reader :record + + def initialize(record) + @record = record + end + + def generate + reflect_on_associations.each_with_object(record_attributes) do |association, result| + result[association.name.to_s] = resolved_association(association) + end + end + + def resolved_association(association) + id = record_attributes["#{association.name}_id"] + return {} if id.blank? + + associated_record = association.klass.lookup(id: id) + associated_record_attributes(associated_record) + end + + def record_attributes + @record_attributes ||= attributes_with_association_names(record) + end + + def reflect_on_associations + record.class.reflect_on_all_associations.select do |association| + self.class.const_get(:ASSOCIATIONS).include?(association.name) + end + end + + def associated_record_attributes(record) + return {} if record.blank? + + attributes = attributes_with_association_names(record) + return attributes if !record.instance_of?(::User) + + attributes.except(*USER_ATTRIBUTE_BLACKLIST) + end + + def attributes_with_association_names(record) + record.attributes_with_association_names.sort.to_h + end +end diff --git a/app/jobs/trigger_webhook_job/record_payload/ticket.rb b/app/jobs/trigger_webhook_job/record_payload/ticket.rb new file mode 100644 index 000000000..17043ad88 --- /dev/null +++ b/app/jobs/trigger_webhook_job/record_payload/ticket.rb @@ -0,0 +1,3 @@ +class TriggerWebhookJob::RecordPayload::Ticket < TriggerWebhookJob::RecordPayload::Base + ASSOCIATIONS = %i[owner customer created_by updated_by organization priority group].freeze +end diff --git a/app/jobs/trigger_webhook_job/record_payload/ticket/article.rb b/app/jobs/trigger_webhook_job/record_payload/ticket/article.rb new file mode 100644 index 000000000..eaaa29764 --- /dev/null +++ b/app/jobs/trigger_webhook_job/record_payload/ticket/article.rb @@ -0,0 +1,28 @@ +class TriggerWebhookJob::RecordPayload::Ticket::Article < TriggerWebhookJob::RecordPayload::Base + + ASSOCIATIONS = %i[created_by updated_by].freeze + + def generate + result = add_attachments_url(super) + add_accounted_time(result) + end + + def add_accounted_time(result) + result['accounted_time'] = record.ticket_time_accounting&.time_unit.to_i + result + end + + def add_attachments_url(result) + return result if result['attachments'].blank? + + result['attachments'].each do |attachment| + attachment['url'] = format(attachment_url_template, result['ticket_id'], result['id'], attachment['id']) + end + + result + end + + def attachment_url_template + @attachment_url_template ||= "#{Setting.get('http_type')}://#{Setting.get('fqdn')}#{Rails.configuration.api_path}/ticket_attachment/%s/%s/%s" + end +end diff --git a/app/jobs/trigger_webhook_job/request_error.rb b/app/jobs/trigger_webhook_job/request_error.rb new file mode 100644 index 000000000..e19c80ad7 --- /dev/null +++ b/app/jobs/trigger_webhook_job/request_error.rb @@ -0,0 +1,2 @@ +class TriggerWebhookJob::RequestError < StandardError +end diff --git a/app/models/concerns/checks_perform_validation.rb b/app/models/concerns/checks_perform_validation.rb index 7aedd4a85..8cdb4487d 100644 --- a/app/models/concerns/checks_perform_validation.rb +++ b/app/models/concerns/checks_perform_validation.rb @@ -12,9 +12,10 @@ module ChecksPerformValidation validate_perform = Marshal.load(Marshal.dump(perform)) check_present = { - 'article.note' => %w[body subject internal], - 'notification.email' => %w[body recipient subject], - 'notification.sms' => %w[body recipient], + 'article.note' => %w[body subject internal], + 'notification.email' => %w[body recipient subject], + 'notification.sms' => %w[body recipient], + 'notification.webhook' => %w[endpoint], } check_present.each do |key, values| diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 41526f321..3fbd1089b 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -1051,6 +1051,8 @@ perform changes on ticket next when 'notification.email' send_email_notification(value, article, perform_origin) + when 'notification.webhook' + TriggerWebhookJob.perform_later(performable, self, article) end end @@ -1786,6 +1788,5 @@ result updated_by_id: 1, created_by_id: 1, ) - end end diff --git a/spec/jobs/trigger_webhook_job/record_payload/base_example.rb b/spec/jobs/trigger_webhook_job/record_payload/base_example.rb new file mode 100644 index 000000000..36ce5d77e --- /dev/null +++ b/spec/jobs/trigger_webhook_job/record_payload/base_example.rb @@ -0,0 +1,31 @@ +RSpec.shared_examples 'TriggerWebhookJob::RecordPayload backend' do |factory| + + describe 'const USER_ATTRIBUTE_BLACKLIST' do + + subject(:blacklist) { described_class.const_get(:USER_ATTRIBUTE_BLACKLIST) } + + it 'contains sensitive attributes' do + expect(blacklist).to include('password') + end + end + + describe '#generate' do + subject(:generate) { described_class.new(record).generate } + let(:resolved_associations) { described_class.const_get(:ASSOCIATIONS).map(&:to_s) } + let(:record) { build(factory) } + + it 'includes attributes with association names' do + expect(generate).to include(record.attributes_with_association_names.except(*resolved_associations)) + end + + it 'resolves defined associations' do + resolved_associations.each do |association| + expect(generate[association]).to be_a(Hash) + end + end + + it 'does not contain blacklisted User attributes' do + expect(generate['created_by']).not_to have_key('password') + end + end +end diff --git a/spec/jobs/trigger_webhook_job/record_payload/ticket/article_spec.rb b/spec/jobs/trigger_webhook_job/record_payload/ticket/article_spec.rb new file mode 100644 index 000000000..0907a5efa --- /dev/null +++ b/spec/jobs/trigger_webhook_job/record_payload/ticket/article_spec.rb @@ -0,0 +1,45 @@ +require 'rails_helper' +require 'jobs/trigger_webhook_job/record_payload/base_example' + +RSpec.describe TriggerWebhookJob::RecordPayload::Ticket::Article do + it_behaves_like 'TriggerWebhookJob::RecordPayload backend', :'ticket/article' + + describe '#generate' do + subject(:generate) { described_class.new(record).generate } + + let(:resolved_associations) { described_class.const_get(:ASSOCIATIONS).map(&:to_s) } + let(:record) { create(:'ticket/article') } + + it "adds 'accounted_time' key" do + expect(generate['accounted_time']).to be_zero + end + + context 'when time accounting entry is present' do + let!(:entry) { create(:ticket_time_accounting, ticket_id: record.ticket.id, ticket_article_id: record.id) } + + it "stores value as 'accounted_time' key" do + expect(generate['accounted_time']).to eq(entry.time_unit) + end + end + + context 'when Article has stored attachments' do + + before do + Store.add( + object: record.class.name, + o_id: record.id, + data: 'some content', + filename: 'some_file.txt', + preferences: { + 'Content-Type' => 'text/plain', + }, + created_by_id: 1, + ) + end + + it 'adds URLs to attachments' do + expect(generate['attachments'].first['url']).to include(Setting.get('fqdn')) + end + end + end +end diff --git a/spec/jobs/trigger_webhook_job/record_payload/ticket_spec.rb b/spec/jobs/trigger_webhook_job/record_payload/ticket_spec.rb new file mode 100644 index 000000000..073081f5e --- /dev/null +++ b/spec/jobs/trigger_webhook_job/record_payload/ticket_spec.rb @@ -0,0 +1,6 @@ +require 'rails_helper' +require 'jobs/trigger_webhook_job/record_payload/base_example' + +RSpec.describe TriggerWebhookJob::RecordPayload::Ticket do + it_behaves_like 'TriggerWebhookJob::RecordPayload backend', :ticket +end diff --git a/spec/jobs/trigger_webhook_job/record_payload_spec.rb b/spec/jobs/trigger_webhook_job/record_payload_spec.rb new file mode 100644 index 000000000..35f9d3b77 --- /dev/null +++ b/spec/jobs/trigger_webhook_job/record_payload_spec.rb @@ -0,0 +1,43 @@ +require 'rails_helper' + +RSpec.describe TriggerWebhookJob::RecordPayload do + + describe '.generate' do + + subject(:generate) { described_class.generate(record) } + + context 'when generator backend exists' do + + let(:record) { build(:ticket) } + let(:backend) { TriggerWebhookJob::RecordPayload::Ticket } + + it 'initializes backend instance and sends generate' do + instance = double() + allow(instance).to receive(:generate) + allow(backend).to receive(:new).and_return(instance) + + generate + + expect(instance).to have_received(:generate) + end + end + + context 'when given record is nil' do + + let(:record) { nil } + + it 'returns an empty hash' do + expect(generate).to eq({}) + end + end + + context 'when given record is not supported' do + + let(:record) { build(:sla) } + + it 'raises an exception' do + expect { generate }.to raise_exception(NameError) + end + end + end +end diff --git a/spec/jobs/trigger_webhook_job_spec.rb b/spec/jobs/trigger_webhook_job_spec.rb new file mode 100644 index 000000000..0cc79d4e0 --- /dev/null +++ b/spec/jobs/trigger_webhook_job_spec.rb @@ -0,0 +1,89 @@ +require 'rails_helper' + +RSpec.describe TriggerWebhookJob, type: :job do + describe '#perform' do + subject(:perform) { described_class.perform_now(trigger, ticket, article) } + + let(:payload_ticket) { TriggerWebhookJob::RecordPayload.generate(ticket) } + let(:payload_article) { TriggerWebhookJob::RecordPayload.generate(article) } + + let!(:ticket) { create(:ticket) } + let!(:article) { create(:'ticket/article') } + + let(:trigger) do + create(:trigger, + perform: { + 'notification.webhook' => { + endpoint: endpoint, + token: token + } + }) + end + + let(:endpoint) { 'http://api.example.com/webhook' } + let(:token) { 's3cr3t-t0k3n' } + + let(:response_status) { 200 } + let(:payload) do + { + ticket: payload_ticket, + article: payload_article, + } + end + + let(:headers) do + { + 'Content-Type' => 'application/json', + 'User-Agent' => 'Zammad User Agent', + 'X-Zammad-Trigger' => trigger.name, + } + end + + let(:response_body) do + {}.to_json + end + + before do + stub_request(:post, endpoint).to_return(status: response_status, body: response_body) + + perform + end + + context 'with trigger token configured' do + it 'includes X-Hub-Signature header' do + expect(WebMock).to have_requested(:post, endpoint) + .with( body: payload, headers: headers ) + .with { |req| req.headers['X-Zammad-Delivery'].is_a?(String) } + .with { |req| req.headers['X-Hub-Signature'].is_a?(String) } + end + end + + context 'without trigger token configured' do + let(:token) { nil } + + it "doesn't include X-Hub-Signature header" do + expect(WebMock).to have_requested(:post, endpoint) + .with( body: payload, headers: headers ) + .with { |req| req.headers['X-Zammad-Delivery'].is_a?(String) } + .with { |req| !req.headers.key?('X-Hub-Signature') } + end + end + + context 'when response is not JSON' do + + let(:response_body) { 'Thanks!' } + + it 'succeeds anyway' do + expect(described_class).not_to have_been_enqueued + end + end + + context "when request doesn't succeed" do + let(:response_status) { 404 } + + it 'enqueues job again' do + expect(described_class).to have_been_enqueued + end + end + end +end diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index a60d34ac1..e47e8a718 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -498,6 +498,22 @@ RSpec.describe Ticket, type: :model do include_examples 'verify log visibility status' end end + + context 'with a "notification.webhook" trigger', performs_jobs: true do + let(:trigger) do + create(:trigger, + perform: { + 'notification.webhook' => { + endpoint: 'http://api.example.com/webhook', + token: '53CR3t' + } + }) + end + + it 'schedules the webhooks notification job' do + expect { ticket.perform_changes(trigger, 'trigger', {}, 1) }.to have_enqueued_job(TriggerWebhookJob).with(trigger, ticket, nil) + end + end end describe '#subject_build' do