diff --git a/lib/active_merchant/billing/gateways/stripe_ach_setup_intents.rb b/lib/active_merchant/billing/gateways/stripe_ach_setup_intents.rb index 6de9df0f2b7..f22ec2aac9c 100644 --- a/lib/active_merchant/billing/gateways/stripe_ach_setup_intents.rb +++ b/lib/active_merchant/billing/gateways/stripe_ach_setup_intents.rb @@ -80,19 +80,26 @@ def void(identification, options = {}) commit(:post, "refunds", { payment_intent: identification }, options) end - # Detaches the customer's us_bank_account PaymentMethod(s). The legacy StripeGateway#unstore - # deletes a card source (customers/{id}/cards/{id}), which does not apply to PaymentMethod-based - # us_bank_account profiles. + # Detaches the one us_bank_account PaymentMethod this profile owns. The legacy + # StripeGateway#unstore deletes a card source (customers/{id}/cards/{id}), which does not apply + # to PaymentMethod-based us_bank_account profiles. One Stripe customer can back several payment + # profiles, so detaching every listed method would remove instruments belonging to profiles the + # caller never asked about. def unstore(identification, options = {}, _deprecated_options = {}) - customer_id = identification.to_s.split("|").first - payment_methods = list_us_bank_account_payment_methods(customer_id) - return Response.new(true, "No us_bank_account payment method to detach") if payment_methods.empty? - - MultiResponse.run(:first) do |r| - payment_methods.each do |payment_method| - r.process { commit(:post, "payment_methods/#{CGI.escape(payment_method["id"])}/detach", {}, options) } - end + customer_id, instrument_id = identification.to_s.split("|") + listed = list_us_bank_account_payment_methods(customer_id).map { |pm| pm["id"] } + # Nothing listed, or a named instrument no longer listed: the detach already happened. + already_detached = listed.empty? || (instrument_id && !listed.include?(instrument_id)) + return Response.new(true, "No us_bank_account payment method to detach") if already_detached + + payment_method_id = instrument_id || + (listed.size == 1 ? listed.first : customer_default_payment_method(customer_id)) + unless listed.include?(payment_method_id) + raise StripeCustomerManyPaymentMethodWithoutDefault, + "Customer has more than one us_bank_account payment method and none identifies this profile." end + + commit(:post, "payment_methods/#{payment_method_id}/detach", {}, options) end private @@ -192,25 +199,22 @@ def customer_acceptance(options) # back to the single listed us_bank_account PM. Raises on ambiguity or absence rather than # silently charging the wrong method. def us_bank_account_payment_method_for_customer(customer) - # Only modern PaymentMethods (pm_*) are valid as a PaymentIntent payment_method. Stripe can - # list legacy bank-account sources here with their original ba_* id; those must not be used. - payment_method_ids = list_us_bank_account_payment_methods(customer) - .map { |pm| pm["id"] } - .select { |id| id.to_s.start_with?("pm_") } + # Not filtered by id prefix: a legacy bank account keeps its ba_* id, and PaymentIntent takes it. + ids = list_us_bank_account_payment_methods(customer).map { |pm| pm["id"] } - return payment_method_ids.first if payment_method_ids.size == 1 + raise RuntimeError, "Customer has no us_bank_account payment method." if ids.empty? + return ids.first if ids.size == 1 - if payment_method_ids.size > 1 - # Disambiguate by the customer's default payment method, but only when the default is - # itself one of the us_bank_account methods (it could be a card). - default = customer_default_payment_method(customer) - return default if default && payment_method_ids.include?(default) + # The default is only usable when it is itself one of the us_bank_account methods (it could be a card). + default = customer_default_payment_method(customer) + return default if ids.include?(default) - raise StripeCustomerManyPaymentMethodWithoutDefault, - "Customer has more than one us_bank_account payment method but no default one." - end + # A single modern PM beside a legacy ba_* charged fine before the pm_* filter went away; keep it working. + modern = ids.grep(/\Apm_/) + return modern.first if modern.size == 1 - raise RuntimeError, "Customer has no us_bank_account payment method." + raise StripeCustomerManyPaymentMethodWithoutDefault, + "Customer has more than one us_bank_account payment method but no default one." end def list_us_bank_account_payment_methods(customer) diff --git a/test/unit/gateways/stripe_ach_setup_intents_test.rb b/test/unit/gateways/stripe_ach_setup_intents_test.rb index 9916754d2d6..239ac050c90 100644 --- a/test/unit/gateways/stripe_ach_setup_intents_test.rb +++ b/test/unit/gateways/stripe_ach_setup_intents_test.rb @@ -59,13 +59,9 @@ def test_store_rejects_non_bank_account_payment end def test_store_creates_us_bank_account_pm_and_setup_intent_on_new_customer - stub_pm = stub_request_for("payment_methods", successful_payment_method_response) - stub_customer = stub_request_for("customers", successful_new_customer_response) - stub_setup_intent = stub_request_for("setup_intents", successful_setup_intent_response) - - expect_ssl_for("/v1/payment_methods", stub_pm) - expect_ssl_for("/v1/customers", stub_customer) - expect_ssl_for("/v1/setup_intents", stub_setup_intent) + expect_ssl_for("/v1/payment_methods", successful_payment_method_response) + expect_ssl_for("/v1/customers", successful_new_customer_response) + expect_ssl_for("/v1/setup_intents", successful_setup_intent_response) response = @gateway.store(@check, @options) @@ -83,23 +79,15 @@ def test_store_creates_payment_method_with_us_bank_account_fields_and_mapped_hol post.include?("us_bank_account[account_holder_type]=individual") && # personal -> individual post.include?("us_bank_account[account_type]=checking") end.returns(successful_payment_method_response) - @gateway.stubs(:ssl_request).with do |_method, endpoint, _post, _headers| - endpoint.start_with?("https://api.stripe.com/v1/customers") - end.returns(successful_new_customer_response) - @gateway.stubs(:ssl_request).with do |_method, endpoint, _post, _headers| - endpoint.start_with?("https://api.stripe.com/v1/setup_intents") - end.returns(successful_setup_intent_response) + stub_ssl_for("/v1/customers", successful_new_customer_response) + stub_ssl_for("/v1/setup_intents", successful_setup_intent_response) assert_success @gateway.store(@check, @options) end def test_store_uses_online_mandate_for_browser_channel - @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/payment_methods") - end.returns(successful_payment_method_response) - @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/customers") - end.returns(successful_new_customer_response) + stub_ssl_for("/v1/payment_methods", successful_payment_method_response) + stub_ssl_for("/v1/customers", successful_new_customer_response) @gateway.expects(:ssl_request).with do |_m, endpoint, post, _h| endpoint.start_with?("https://api.stripe.com/v1/setup_intents") && post.include?("mandate_data[customer_acceptance][type]=online") && @@ -111,16 +99,10 @@ def test_store_uses_online_mandate_for_browser_channel end def test_store_uses_offline_mandate_for_api_channel - @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/payment_methods") - end.returns(successful_payment_method_response) - @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/customers") - end.returns(successful_new_customer_response) - @gateway.expects(:ssl_request).with do |_m, endpoint, post, _h| - endpoint.start_with?("https://api.stripe.com/v1/setup_intents") && - post.include?("mandate_data[customer_acceptance][type]=offline") - end.returns(successful_setup_intent_response) + stub_ssl_for("/v1/payment_methods", successful_payment_method_response) + stub_ssl_for("/v1/customers", successful_new_customer_response) + expect_ssl_for("/v1/setup_intents", successful_setup_intent_response, + "mandate_data[customer_acceptance][type]=offline") assert_success @gateway.store(@check, @options.merge(channel: "api")) end @@ -128,9 +110,7 @@ def test_store_uses_offline_mandate_for_api_channel # --- purchase ------------------------------------------------------------------------------ def test_purchase_creates_off_session_payment_intent_with_resolved_bank_pm - @gateway.expects(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer") - end.returns(payment_methods_list_with_single_bank_account) + expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123")) @gateway.expects(:ssl_request).with do |_m, endpoint, post, _h| endpoint.start_with?("https://api.stripe.com/v1/payment_intents") && post.include?("off_session=true") && @@ -146,26 +126,47 @@ def test_purchase_creates_off_session_payment_intent_with_resolved_bank_pm end def test_purchase_treats_processing_payment_intent_as_success - @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer") - end.returns(payment_methods_list_with_single_bank_account) - @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/payment_intents") - end.returns(successful_payment_intent_response("processing")) + stub_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123")) + stub_ssl_for("/v1/payment_intents", successful_payment_intent_response("processing")) assert_success @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH")) end def test_purchase_raises_when_customer_has_no_us_bank_account_pm - @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer") - end.returns(empty_payment_methods_list) + stub_ssl_for("payment_methods?customer", payment_methods_list) assert_raises(RuntimeError) do @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH")) end end + def test_purchase_uses_a_legacy_bank_account_id_as_the_payment_method + expect_ssl_for("payment_methods?customer", payment_methods_list("ba_legacy123")) + expect_ssl_for("/v1/payment_intents", successful_payment_intent_response("processing"), + "payment_method=ba_legacy123") + + assert_success @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH")) + end + + # Documents why Stripe::AchRoutingState keeps a customer with several legacy instruments on the + # legacy path: here there is nothing to disambiguate them with, so the charge cannot be made at all. + def test_purchase_raises_when_several_instruments_have_no_usable_default + stub_ssl_for("payment_methods?customer", payment_methods_list("ba_legacy123", "ba_legacy999")) + stub_ssl_for("/v1/customers/", customer_without_default_payment_method) + + assert_raises(ActiveMerchant::Billing::StripeCustomerManyPaymentMethodWithoutDefault) do + @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH")) + end + end + + def test_purchase_prefers_the_single_modern_payment_method_over_a_legacy_sibling + stub_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123", "ba_legacy123")) + stub_ssl_for("/v1/customers/", customer_without_default_payment_method) + expect_ssl_for("/v1/payment_intents", successful_payment_intent_response("processing"), "payment_method=pm_bank123") + + assert_success @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH")) + end + # --- refund -------------------------------------------------------------------------------- def test_refund_refunds_by_payment_intent @@ -183,36 +184,62 @@ def test_refund_refunds_by_payment_intent # --- unstore (detach) ---------------------------------------------------------------------- def test_unstore_detaches_us_bank_account_payment_methods - @gateway.expects(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer") - end.returns(payment_methods_list_with_single_bank_account) - @gateway.expects(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint == "https://api.stripe.com/v1/payment_methods/pm_bank123/detach" - end.returns(successful_detach_response) + expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123")) + expect_ssl_for("payment_methods/pm_bank123/detach", successful_detach_response) assert_success @gateway.unstore("cus_ACH") end def test_unstore_is_a_noop_when_no_bank_account_pm_exists - @gateway.expects(:ssl_request).with do |_m, endpoint, _p, _h| - endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer") - end.returns(empty_payment_methods_list) + expect_ssl_for("payment_methods?customer", payment_methods_list) assert_success @gateway.unstore("cus_ACH") end - private + def test_unstore_detaches_only_the_payment_method_named_in_the_identification + expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123", "pm_bank999")) + expect_ssl_for("payment_methods/pm_bank999/detach", successful_detach_response) + + assert_success @gateway.unstore("cus_ACH|pm_bank999") + end + + def test_unstore_detaches_a_legacy_bank_account_id + expect_ssl_for("payment_methods?customer", payment_methods_list("ba_legacy123")) + expect_ssl_for("payment_methods/ba_legacy123/detach", successful_detach_response) + + assert_success @gateway.unstore("cus_ACH|ba_legacy123") + end + + def test_unstore_is_a_noop_when_the_named_instrument_is_already_detached + expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123")) + + response = @gateway.unstore("cus_ACH|ba_gone123") - def stub_request_for(_endpoint, response) - response + assert_success response + assert_equal "No us_bank_account payment method to detach", response.message + end + + def test_unstore_raises_rather_than_detaching_every_method_when_the_profile_is_ambiguous + expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123", "pm_bank999")) + expect_ssl_for("/v1/customers/cus_ACH", customer_without_default_payment_method) + + assert_raises(StripeCustomerManyPaymentMethodWithoutDefault) do + @gateway.unstore("cus_ACH") + end end - def expect_ssl_for(path_fragment, response) - @gateway.expects(:ssl_request).with do |_method, endpoint, _post, _headers| - endpoint.include?(path_fragment) + private + + def expect_ssl_for(path_fragment, response, post_fragment = nil, mode: :expects) + @gateway.send(mode, :ssl_request).with do |_method, endpoint, post, _headers| + endpoint.include?(path_fragment) && (post_fragment.nil? || post.to_s.include?(post_fragment)) end.returns(response) end + def stub_ssl_for(path_fragment, response, post_fragment = nil) + expect_ssl_for(path_fragment, response, post_fragment, mode: :stubs) + end + # --- fixtures ------------------------------------------------------------------------------ def successful_payment_method_response @@ -239,11 +266,12 @@ def successful_detach_response %({"id": "pm_bank123", "object": "payment_method", "customer": null, "livemode": false}) end - def payment_methods_list_with_single_bank_account - %({"object": "list", "data": [{"id": "pm_bank123", "type": "us_bank_account"}], "livemode": false}) + def payment_methods_list(*ids) + data = ids.map { |id| %({"id": "#{id}", "type": "us_bank_account"}) }.join(", ") + %({"object": "list", "data": [#{data}], "livemode": false}) end - def empty_payment_methods_list - %({"object": "list", "data": [], "livemode": false}) + def customer_without_default_payment_method + %({"id": "cus_ACH", "object": "customer", "invoice_settings": {"default_payment_method": null}, "livemode": false}) end end