diff --git a/app/jobs/regular/emit_web_hook_event.rb b/app/jobs/regular/emit_web_hook_event.rb index 4095f3b9d7e..9d91f121c45 100644 --- a/app/jobs/regular/emit_web_hook_event.rb +++ b/app/jobs/regular/emit_web_hook_event.rb @@ -41,27 +41,21 @@ module Jobs def send_webhook! uri = URI(web_hook.payload_url.strip) - - conn = Excon.new( - uri.to_s, - ssl_verify_peer: web_hook.verify_certificate, - retry_limit: 0 - ) + conn = Excon.new(uri.to_s, ssl_verify_peer: web_hook.verify_certificate, retry_limit: 0) web_hook_body = build_webhook_body web_hook_event = create_webhook_event(web_hook_body) web_hook_headers = build_webhook_headers(uri, web_hook_body, web_hook_event) - - response = nil + web_hook_response = nil begin now = Time.zone.now - response = conn.post(headers: web_hook_headers, body: web_hook_body) + web_hook_response = conn.post(headers: web_hook_headers, body: web_hook_body) web_hook_event.update!( headers: MultiJson.dump(web_hook_headers), - status: response.status, - response_headers: MultiJson.dump(response.headers), - response_body: response.body, + status: web_hook_response.status, + response_headers: MultiJson.dump(web_hook_response.headers), + response_body: web_hook_response.body, duration: ((Time.zone.now - now) * 1000).to_i ) rescue => e @@ -74,7 +68,23 @@ module Jobs end publish_webhook_event(web_hook_event) - retry_web_hook if response&.status != 200 + process_webhook_response(web_hook_response) + end + + def process_webhook_response(web_hook_response) + return if web_hook_response&.status.blank? + + case web_hook_response.status + when 200..299 + return + when 404, 410 + if @retry_count >= MAX_RETRY_COUNT + web_hook.update_attribute(:active, false) + StaffActionLogger.new(Discourse.system_user).log_web_hook_deactivate(web_hook, web_hook_response.status) + end + else + retry_web_hook + end end def retry_web_hook diff --git a/app/models/user_history.rb b/app/models/user_history.rb index af847f1dfe5..2c37510d75d 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -91,7 +91,8 @@ class UserHistory < ActiveRecord::Base web_hook_destroy: 72, embeddable_host_create: 73, embeddable_host_update: 74, - embeddable_host_destroy: 75 + embeddable_host_destroy: 75, + web_hook_deactivate: 76 ) end @@ -159,6 +160,7 @@ class UserHistory < ActiveRecord::Base :web_hook_create, :web_hook_update, :web_hook_destroy, + :web_hook_deactivate, :embeddable_host_create, :embeddable_host_update, :embeddable_host_destroy diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index 5b047a17e9b..dae6589cd3b 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -599,6 +599,19 @@ class StaffActionLogger )) end + def log_web_hook_deactivate(web_hook, response_http_status, opts = {}) + context = [ + "webhook_id: #{web_hook.id}", + "webhook_response_status: #{response_http_status}" + ] + + UserHistory.create!(params.merge( + action: UserHistory.actions[:web_hook_deactivate], + context: context, + details: I18n.t('staff_action_logs.webhook_deactivation_reason', status: response_http_status) + )) + end + def log_embeddable_host(embeddable_host, action, opts = {}) old_values, new_values = get_changes(opts[:changes]) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index e4af071525e..7d9f5cb77dd 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3744,6 +3744,7 @@ en: web_hook_create: "webhook create" web_hook_update: "webhook update" web_hook_destroy: "webhook destroy" + web_hook_deactivate: "webhook deactivate" embeddable_host_create: "embeddable host create" embeddable_host_update: "embeddable host update" embeddable_host_destroy: "embeddable host destroy" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 24e0147cee5..0ff20bd1f6a 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -4400,6 +4400,7 @@ en: unknown: "unknown" user_merged: "%{username} was merged into this account" user_delete_self: "Deleted by self from %{url}" + webhook_deactivation_reason: "Your webhook has been automatically deactivated. We received multiple '%{status}' HTTP status failure responses." reviewables: missing_version: "You must supply a version parameter" diff --git a/spec/jobs/emit_web_hook_event_spec.rb b/spec/jobs/emit_web_hook_event_spec.rb index 95cc8d4b909..505de48d9d1 100644 --- a/spec/jobs/emit_web_hook_event_spec.rb +++ b/spec/jobs/emit_web_hook_event_spec.rb @@ -40,54 +40,88 @@ describe Jobs::EmitWebHookEvent do context 'when the web hook is failed' do before do SiteSetting.retry_web_hook_events = true - - stub_request(:post, post_hook.payload_url) - .to_return(body: 'Invalid Access', status: 403) end - it 'retry if site setting is enabled' do - expect do - subject.execute( - web_hook_id: post_hook.id, - event_type: described_class::PING_EVENT - ) - end.to change { Jobs::EmitWebHookEvent.jobs.size }.by(1) - end + context 'when the webhook has failed for 404 or 410' do + before do + stub_request(:post, post_hook.payload_url).to_return(body: 'Invalid Access', status: response_status) + end - it 'does not retry for more than maximum allowed times' do - expect do + let(:response_status) { 410 } + + it 'disables the webhook' do + expect do + subject.execute( + web_hook_id: post_hook.id, + event_type: described_class::PING_EVENT, + retry_count: described_class::MAX_RETRY_COUNT + ) + end.to change { post_hook.reload.active }.to(false) + end + + it 'logs webhook deactivation reason' do subject.execute( web_hook_id: post_hook.id, event_type: described_class::PING_EVENT, retry_count: described_class::MAX_RETRY_COUNT ) - end.to_not change { Jobs::EmitWebHookEvent.jobs.size } + user_history = UserHistory.find_by(action: UserHistory.actions[:web_hook_deactivate], acting_user: Discourse.system_user) + expect(user_history).to be_present + expect(user_history.context).to eq([ + "webhook_id: #{post_hook.id}", + "webhook_response_status: #{response_status}" + ].to_s) + end end - it 'does not retry if site setting is disabled' do - SiteSetting.retry_web_hook_events = false + context 'when the webhook has failed' do + before do + stub_request(:post, post_hook.payload_url).to_return(body: 'Invalid Access', status: 403) + end - expect do + it 'retry if site setting is enabled' do + expect do + subject.execute( + web_hook_id: post_hook.id, + event_type: described_class::PING_EVENT + ) + end.to change { Jobs::EmitWebHookEvent.jobs.size }.by(1) + end + + it 'does not retry for more than maximum allowed times' do + expect do + subject.execute( + web_hook_id: post_hook.id, + event_type: described_class::PING_EVENT, + retry_count: described_class::MAX_RETRY_COUNT + ) + end.to_not change { Jobs::EmitWebHookEvent.jobs.size } + end + + it 'does not retry if site setting is disabled' do + SiteSetting.retry_web_hook_events = false + + expect do + subject.execute( + web_hook_id: post_hook.id, + event_type: described_class::PING_EVENT + ) + end.to change { Jobs::EmitWebHookEvent.jobs.size }.by(0) + end + + it 'properly logs error on rescue' do + stub_request(:post, post_hook.payload_url).to_raise("connection error") subject.execute( web_hook_id: post_hook.id, event_type: described_class::PING_EVENT ) - end.to change { Jobs::EmitWebHookEvent.jobs.size }.by(0) + + event = WebHookEvent.last + expect(event.payload).to eq(MultiJson.dump(ping: 'OK')) + expect(event.status).to eq(-1) + expect(MultiJson.load(event.response_headers)['error']).to eq('connection error') + end end - - it 'properly logs error on rescue' do - stub_request(:post, post_hook.payload_url).to_raise("connection error") - subject.execute( - web_hook_id: post_hook.id, - event_type: described_class::PING_EVENT - ) - - event = WebHookEvent.last - expect(event.payload).to eq(MultiJson.dump(ping: 'OK')) - expect(event.status).to eq(-1) - expect(MultiJson.load(event.response_headers)['error']).to eq('connection error') - end - end it 'does not raise an error for a ping event without payload' do