Skip to content

Protect against concurrent duplicated webhooks #286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
waiting-for-dev merged 1 commit into from
Apr 18, 2023
Merged

Protect against concurrent duplicated webhooks #286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 32 additions & 28 deletions app/subscribers/solidus_stripe/webhook/payment_intent_subscriber.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,11 @@ class PaymentIntentSubscriber
# @see SolidusStripe::RefundsSynchronizer
def capture_payment(event)
payment = extract_payment_from_event(event)
return if payment.completed?
payment.with_lock do
break false if payment.completed?

event.data.object.to_hash => {
amount: stripe_amount,
amount_received: stripe_amount_received,
currency:
}
if stripe_amount == stripe_amount_received
complete_payment(payment)
else
complete_payment(payment)
sync_refunds(event)
end
end && sync_refunds(event)
end

# Fails a payment.
Expand All @@ -47,14 +39,17 @@ def capture_payment(event)
# @param event [SolidusStripe::Webhook::Event]
def fail_payment(event)
payment = extract_payment_from_event(event)
return if payment.failed?

payment.failure!.tap do
SolidusStripe::LogEntries.payment_log(
payment,
success: false,
message: "Payment was marked as failed after payment_intent.failed webhook"
)
payment.with_lock do
break if payment.failed?

payment.failure!.tap do
SolidusStripe::LogEntries.payment_log(
payment,
success: false,
message: "Payment was marked as failed after payment_intent.failed webhook"
)
end
end
end

Expand All @@ -66,15 +61,18 @@ def fail_payment(event)
# @param event [SolidusStripe::Webhook::Event]
def void_payment(event)
payment = extract_payment_from_event(event)
return if payment.void?

reason = event.data.object.cancellation_reason
payment.void!.tap do
SolidusStripe::LogEntries.payment_log(
payment,
success: true,
message: "Payment was voided after payment_intent.voided webhook (#{reason})"
)

payment.with_lock do
break if payment.void?

payment.void!.tap do
SolidusStripe::LogEntries.payment_log(
payment,
success: true,
message: "Payment was voided after payment_intent.voided webhook (#{reason})"
)
end
end
end

Expand All @@ -96,9 +94,15 @@ def complete_payment(payment)
end

def sync_refunds(event)
payment_method = event.spree_payment_method
payment_intent_id = event.data.object.id
event.data.object.to_hash => {
id: payment_intent_id,
amount: stripe_amount,
amount_received: stripe_amount_received,
currency:
}
return if stripe_amount == stripe_amount_received

payment_method = event.spree_payment_method
RefundsSynchronizer
.new(payment_method)
.call(payment_intent_id)
Expand Down
2 changes: 1 addition & 1 deletion spec/subscribers/solidus_stripe/webhook/payment_intent_subscriber_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@

it "does nothing if the payment is already voided" do
payment_method = create(:stripe_payment_method)
stripe_payment_intent = Stripe::PaymentIntent.construct_from(id: "pi_123")
stripe_payment_intent = Stripe::PaymentIntent.construct_from(id: "pi_123", cancellation_reason: "duplicate")

@elia elia Apr 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious about this change, what's the story behind it?

@rainerdema rainerdema Apr 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elia, I think this is from a comment that I left some time ago 😅 #250 (comment)

@waiting-for-dev waiting-for-dev Apr 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cancellation reason from the event is used to build the log entry message. We don't use it when the payment is already voided.

Before this PR, we skipped fetching it when the payment was already voided. However, we're now fetching it before acquiring the lock (where the voided check needs to happen) only to hold the lock for the minimum required time.

payment = create(:payment,
payment_method: payment_method,
response_code: stripe_payment_intent.id,
Expand Down