mirror of https://github.com/docusealco/docuseal
CP-12376 extend webhooks partnerships (#55)
* add partnership_id to webhook_urls - add migration to make account_id OR partnership_id required, you can use either, but can and must use at least one - add PARTNERSHIP_EVENTS constant to constrain webhook firing to just template events * extract duplicated webhook retry logic for webhook jobs - 11 webhook jobs all used the same retry logic (except one file that had 12 max retries instead of 10. - remove and replace duplicated code - add retry logic for partnership templates * refactor WebhookUrls to support partnerships - add for_template method to support account/partnership templates. - for_account_id will still work for submissions - update controllers with new method - I'm not great with Arel, so I refactored since I wanted account/partnership to use a shared method. * a automatic webhook creation for new partnerships * fix rubocop violations * update spec to expect raised error instead of empty array * fix rubocop/rspec for HTTP requests in test * remove after commit partnership webhook temporarily The immediately following PR will add this `after_commit` back. We only really need the upcoming template.preferences_updated webhook event that will be in the next PR, so even though it's unlikely anyone will be testing this at the Partnership level right now, better to just remove it for the time being for a cleaner PR. * validate incoming events against WebhookUrl::EVENTS constant * safety against SQL injection This method does not accept user input, but adding this just to be safe. * add form.changes_requested to events * since we added the events constant checker, we need to make sure this event is part of the constant listpull/608/head
Ryan Arakawa
1 month ago
committed by
GitHub
parent
3570b7c0b5
commit
41e18f9484
@ -0,0 +1,14 @@
|
||||
class AddPartnershipIdToWebhookUrls < ActiveRecord::Migration[8.0]
|
||||
def change
|
||||
# Make account_id nullable since webhooks can now belong to either account or partnership
|
||||
change_column_null :webhook_urls, :account_id, true
|
||||
|
||||
# Add partnership_id as optional reference
|
||||
add_reference :webhook_urls, :partnership, null: true, foreign_key: true
|
||||
|
||||
# Add check constraint to ensure exactly one of account_id or partnership_id is set
|
||||
add_check_constraint :webhook_urls,
|
||||
'(account_id IS NOT NULL AND partnership_id IS NULL) OR (account_id IS NULL AND partnership_id IS NOT NULL)',
|
||||
name: 'webhook_urls_owner_check'
|
||||
end
|
||||
end
|
||||
@ -0,0 +1,37 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
# Shared logic for determining if webhook requests should be retried
|
||||
# Used across all Send*WebhookRequestJob classes
|
||||
module WebhookRetryLogic
|
||||
module_function
|
||||
|
||||
MAX_ATTEMPTS = 10
|
||||
|
||||
# Determines if a failed webhook request should be retried
|
||||
#
|
||||
# @param response [HTTP::Response, nil] The HTTP response from the webhook request
|
||||
# @param attempt [Integer] Current retry attempt number
|
||||
# @param record [Template, Submission, Submitter] The record triggering the webhook
|
||||
# @return [Boolean] true if the webhook should be retried
|
||||
def should_retry?(response:, attempt:, record:)
|
||||
return false unless response.nil? || response.status.to_i >= 400
|
||||
return false if attempt > MAX_ATTEMPTS
|
||||
return true unless Docuseal.multitenant?
|
||||
|
||||
eligible_for_retries?(record)
|
||||
end
|
||||
|
||||
# Checks if a record is eligible for webhook retries in multitenant mode
|
||||
# @param record [Template, Submission, Submitter] The record to check
|
||||
# @return [Boolean] true if eligible for retries
|
||||
def eligible_for_retries?(record)
|
||||
case record
|
||||
when Template
|
||||
record.partnership_id.present? || record.account&.account_configs&.exists?(key: :plan)
|
||||
when Submission, Submitter
|
||||
record.account.account_configs.exists?(key: :plan)
|
||||
else
|
||||
false
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -0,0 +1,45 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
# Simple response object for testing
|
||||
FakeResponse = Struct.new(:status)
|
||||
|
||||
RSpec.describe WebhookRetryLogic do
|
||||
let(:account) { create(:account) }
|
||||
let(:user) { create(:user, account: account) }
|
||||
let(:template) { create(:template, account: account, author: user) }
|
||||
|
||||
describe '.should_retry?' do
|
||||
context 'with successful response' do
|
||||
it 'does not retry' do
|
||||
response = FakeResponse.new(200)
|
||||
result = described_class.should_retry?(response: response, attempt: 1, record: template)
|
||||
expect(result).to be false
|
||||
end
|
||||
end
|
||||
|
||||
context 'with failed response' do
|
||||
it 'retries on 4xx errors within attempt limit' do
|
||||
response = FakeResponse.new(400)
|
||||
result = described_class.should_retry?(response: response, attempt: 5, record: template)
|
||||
expect(result).to be true
|
||||
end
|
||||
|
||||
it 'retries on 5xx errors within attempt limit' do
|
||||
response = FakeResponse.new(500)
|
||||
result = described_class.should_retry?(response: response, attempt: 5, record: template)
|
||||
expect(result).to be true
|
||||
end
|
||||
|
||||
it 'retries on nil response within attempt limit' do
|
||||
result = described_class.should_retry?(response: nil, attempt: 5, record: template)
|
||||
expect(result).to be true
|
||||
end
|
||||
|
||||
it 'does not retry when max attempts exceeded' do
|
||||
response = FakeResponse.new(500)
|
||||
result = described_class.should_retry?(response: response, attempt: 11, record: template)
|
||||
expect(result).to be false
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -0,0 +1,157 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
RSpec.describe WebhookUrls do
|
||||
describe '.for_template' do
|
||||
let(:user) { create(:user) }
|
||||
|
||||
context 'with a partnership template' do
|
||||
let(:partnership) { create(:partnership) }
|
||||
let(:template) { create(:template, partnership: partnership, account: nil, author: user) }
|
||||
let!(:partnership_webhook) do
|
||||
create(:webhook_url,
|
||||
partnership: partnership,
|
||||
account: nil,
|
||||
events: ['template.created'],
|
||||
url: 'https://partnership.example.com/webhook')
|
||||
end
|
||||
|
||||
it 'returns partnership webhooks' do
|
||||
webhooks = described_class.for_template(template, 'template.created')
|
||||
expect(webhooks).to include(partnership_webhook)
|
||||
end
|
||||
|
||||
it 'does not return account webhooks' do
|
||||
account_webhook = create(:webhook_url, account: create(:account), events: ['template.created'])
|
||||
webhooks = described_class.for_template(template, 'template.created')
|
||||
expect(webhooks).not_to include(account_webhook)
|
||||
end
|
||||
|
||||
it 'filters by event type' do
|
||||
non_matching_webhook = create(:webhook_url,
|
||||
partnership: partnership,
|
||||
account: nil,
|
||||
events: ['template.updated'])
|
||||
|
||||
webhooks = described_class.for_template(template, 'template.created')
|
||||
expect(webhooks).to include(partnership_webhook)
|
||||
expect(webhooks).not_to include(non_matching_webhook)
|
||||
end
|
||||
end
|
||||
|
||||
context 'with an account template' do
|
||||
let(:account) { create(:account) }
|
||||
let(:account_user) { create(:user, account: account) }
|
||||
let(:template) { create(:template, account: account, partnership: nil, author: account_user) }
|
||||
let!(:account_webhook) do
|
||||
create(:webhook_url,
|
||||
account: account,
|
||||
partnership: nil,
|
||||
events: ['template.created'])
|
||||
end
|
||||
|
||||
it 'returns account webhooks' do
|
||||
webhooks = described_class.for_template(template, 'template.created')
|
||||
expect(webhooks).to include(account_webhook)
|
||||
end
|
||||
|
||||
it 'does not return partnership webhooks' do
|
||||
partnership_webhook = create(:webhook_url,
|
||||
partnership: create(:partnership),
|
||||
account: nil,
|
||||
events: ['template.created'])
|
||||
webhooks = described_class.for_template(template, 'template.created')
|
||||
expect(webhooks).not_to include(partnership_webhook)
|
||||
end
|
||||
end
|
||||
|
||||
context 'with a template that has neither account nor partnership' do
|
||||
let(:template) { build(:template, account: nil, partnership: nil, author: user) }
|
||||
|
||||
it 'raises an ArgumentError' do
|
||||
expect do
|
||||
described_class.for_template(template, 'template.created')
|
||||
end.to raise_error(ArgumentError, 'Template must have either account_id or partnership_id')
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '.for_partnership_id' do
|
||||
let(:partnership) { create(:partnership) }
|
||||
let!(:webhook) do
|
||||
create(:webhook_url,
|
||||
partnership: partnership,
|
||||
account: nil,
|
||||
events: ['template.created', 'template.updated'])
|
||||
end
|
||||
let!(:webhook_update_only) do
|
||||
create(:webhook_url,
|
||||
partnership: partnership,
|
||||
account: nil,
|
||||
events: ['template.updated'])
|
||||
end
|
||||
|
||||
it 'returns webhooks matching the event' do
|
||||
webhooks = described_class.for_partnership_id(partnership.id, 'template.created')
|
||||
expect(webhooks).to include(webhook)
|
||||
expect(webhooks).not_to include(webhook_update_only)
|
||||
end
|
||||
|
||||
it 'returns webhooks matching any of multiple events' do
|
||||
webhooks = described_class.for_partnership_id(partnership.id, ['template.created', 'template.updated'])
|
||||
expect(webhooks).to include(webhook, webhook_update_only)
|
||||
end
|
||||
|
||||
it 'does not return webhooks from other partnerships' do
|
||||
other_partnership = create(:partnership)
|
||||
other_webhook = create(:webhook_url,
|
||||
partnership: other_partnership,
|
||||
account: nil,
|
||||
events: ['template.created'])
|
||||
|
||||
webhooks = described_class.for_partnership_id(partnership.id, 'template.created')
|
||||
expect(webhooks).not_to include(other_webhook)
|
||||
end
|
||||
|
||||
it 'handles single event as string' do
|
||||
webhooks = described_class.for_partnership_id(partnership.id, 'template.updated')
|
||||
expect(webhooks).to include(webhook, webhook_update_only)
|
||||
end
|
||||
end
|
||||
|
||||
describe '.for_account_id' do
|
||||
let(:account) { create(:account) }
|
||||
let!(:webhook) do
|
||||
create(:webhook_url,
|
||||
account: account,
|
||||
partnership: nil,
|
||||
events: ['template.created'])
|
||||
end
|
||||
|
||||
it 'returns webhooks for the account' do
|
||||
webhooks = described_class.for_account_id(account.id, 'template.created')
|
||||
expect(webhooks).to include(webhook)
|
||||
end
|
||||
|
||||
it 'does not return webhooks from other accounts' do
|
||||
other_account = create(:account)
|
||||
other_webhook = create(:webhook_url,
|
||||
account: other_account,
|
||||
partnership: nil,
|
||||
events: ['template.created'])
|
||||
|
||||
webhooks = described_class.for_account_id(account.id, 'template.created')
|
||||
expect(webhooks).not_to include(other_webhook)
|
||||
end
|
||||
|
||||
it 'filters by event type' do
|
||||
non_matching = create(:webhook_url,
|
||||
account: account,
|
||||
partnership: nil,
|
||||
events: ['template.updated'])
|
||||
|
||||
webhooks = described_class.for_account_id(account.id, 'template.created')
|
||||
expect(webhooks).to include(webhook)
|
||||
expect(webhooks).not_to include(non_matching)
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -0,0 +1,94 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
# == Schema Information
|
||||
#
|
||||
# Table name: webhook_urls
|
||||
#
|
||||
# id :bigint not null, primary key
|
||||
# events :text not null
|
||||
# secret :text not null
|
||||
# sha1 :string not null
|
||||
# url :text not null
|
||||
# created_at :datetime not null
|
||||
# updated_at :datetime not null
|
||||
# account_id :integer
|
||||
# partnership_id :bigint
|
||||
#
|
||||
# Indexes
|
||||
#
|
||||
# index_webhook_urls_on_account_id (account_id)
|
||||
# index_webhook_urls_on_partnership_id (partnership_id)
|
||||
# index_webhook_urls_on_sha1 (sha1)
|
||||
#
|
||||
# Foreign Keys
|
||||
#
|
||||
# fk_rails_... (account_id => accounts.id)
|
||||
# fk_rails_... (partnership_id => partnerships.id)
|
||||
#
|
||||
describe WebhookUrl do
|
||||
describe 'validations' do
|
||||
context 'with owner presence' do
|
||||
it 'is valid with account_id and no partnership_id' do
|
||||
webhook = build(:webhook_url, account: create(:account), partnership: nil)
|
||||
expect(webhook).to be_valid
|
||||
end
|
||||
|
||||
it 'is valid with partnership_id and no account_id' do
|
||||
# Disable webhook creation callback by removing env vars
|
||||
stub_const('ENV', ENV.to_hash.except('CAREERPLUG_WEBHOOK_URL', 'CAREERPLUG_WEBHOOK_SECRET'))
|
||||
|
||||
partnership = create(:partnership)
|
||||
webhook = build(:webhook_url,
|
||||
account: nil,
|
||||
partnership: partnership,
|
||||
events: WebhookUrl::PARTNERSHIP_EVENTS)
|
||||
expect(webhook).to be_valid
|
||||
end
|
||||
|
||||
it 'is invalid with both account_id and partnership_id' do
|
||||
stub_const('ENV', ENV.to_hash.except('CAREERPLUG_WEBHOOK_URL', 'CAREERPLUG_WEBHOOK_SECRET'))
|
||||
|
||||
webhook = build(:webhook_url, account: create(:account), partnership: create(:partnership))
|
||||
expect(webhook).not_to be_valid
|
||||
expect(webhook.errors[:base]).to include('Must have either account_id or partnership_id, but not both')
|
||||
end
|
||||
|
||||
it 'is invalid with neither account_id nor partnership_id' do
|
||||
webhook = build(:webhook_url, account: nil, partnership: nil)
|
||||
expect(webhook).not_to be_valid
|
||||
expect(webhook.errors[:base]).to include('Must have either account_id or partnership_id, but not both')
|
||||
end
|
||||
end
|
||||
|
||||
context 'with partnership events constraint' do
|
||||
it 'only includes template.* events in PARTNERSHIP_EVENTS' do
|
||||
expect(WebhookUrl::PARTNERSHIP_EVENTS).to all(start_with('template.'))
|
||||
end
|
||||
|
||||
it 'PARTNERSHIP_EVENTS is a subset of EVENTS' do
|
||||
expect(WebhookUrl::PARTNERSHIP_EVENTS).to all(be_in(WebhookUrl::EVENTS))
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe 'callbacks' do
|
||||
describe '#set_sha1' do
|
||||
it 'sets sha1 based on url' do
|
||||
webhook = build(:webhook_url, url: 'https://example.com/webhook')
|
||||
webhook.valid?
|
||||
expect(webhook.sha1).to eq(Digest::SHA1.hexdigest('https://example.com/webhook'))
|
||||
end
|
||||
|
||||
it 'updates sha1 when url changes' do
|
||||
webhook = create(:webhook_url, url: 'https://example.com/webhook')
|
||||
original_sha1 = webhook.sha1
|
||||
|
||||
webhook.url = 'https://example.com/new-webhook'
|
||||
webhook.valid?
|
||||
|
||||
expect(webhook.sha1).not_to eq(original_sha1)
|
||||
expect(webhook.sha1).to eq(Digest::SHA1.hexdigest('https://example.com/new-webhook'))
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
Loading…
Reference in new issue