From 2147584daf7313d17cccbf0bc39e3f1bb2aa4992 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 6 May 2021 12:52:36 +0200 Subject: [PATCH] Validate intents against Stripe and display errors Now the existing validation is redundant. It's Stripe's API who does that now. It's up to them to decide what's a valid intent. --- app/controllers/spree/orders_controller.rb | 5 ++- app/services/process_payment_intent.rb | 42 ++++++++++++++--- .../spree/orders_controller_spec.rb | 38 ++++++++++++++++ spec/services/process_payment_intent_spec.rb | 45 ++++++++++++++++--- 4 files changed, 118 insertions(+), 12 deletions(-) diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb index d4bcf65f39..fa6212de7c 100644 --- a/app/controllers/spree/orders_controller.rb +++ b/app/controllers/spree/orders_controller.rb @@ -28,7 +28,10 @@ module Spree @order = Spree::Order.find_by!(number: params[:id]) if params.key?("payment_intent") - ProcessPaymentIntent.new(params["payment_intent"], @order).call! + result = ProcessPaymentIntent.new(params["payment_intent"], @order).call! + unless result.ok? + flash[:error] = "The payment could not be processed. #{result.error}" + end @order.reload end end diff --git a/app/services/process_payment_intent.rb b/app/services/process_payment_intent.rb index d3142462f7..bff7f40170 100644 --- a/app/services/process_payment_intent.rb +++ b/app/services/process_payment_intent.rb @@ -3,12 +3,26 @@ # When directing a customer to Stripe to authorize the payment, we specify a # redirect_url that Stripe should return them to. When checking out, it's # /checkout; for admin payments and subscription payemnts it's the order url. +# # This class confirms that the payment intent matches what's in our database, # marks the payment as complete, and removes the cvv_response_message field, # which we use to indicate that authorization is required. It also completes the # Order, if appropriate. class ProcessPaymentIntent + class Result + attr_reader :error + + def initialize(ok:, error: "") + @ok = ok + @error = error + end + + def ok? + @ok + end + end + def initialize(payment_intent, order) @payment_intent = payment_intent @order = order @@ -16,11 +30,17 @@ class ProcessPaymentIntent end def call! - return unless valid? + validate_intent! + return Result.new(ok: false) unless valid? + + mark_as_processed - last_payment.update_attribute(:cvv_response_message, nil) OrderWorkflow.new(@order).next last_payment.complete! if last_payment.can_complete? + + Result.new(ok: true) + rescue Stripe::StripeError => e + Result.new(ok: false, error: e.message) end private @@ -28,14 +48,26 @@ class ProcessPaymentIntent attr_reader :order, :payment_intent, :last_payment def valid? - order.present? && valid_intent_string? && matches_last_payment? + order.present? && matches_last_payment? end - def valid_intent_string? - payment_intent&.starts_with?("pi_") + def validate_intent! + Stripe::PaymentIntentValidator.new.call(payment_intent, stripe_account_id) end def matches_last_payment? last_payment&.state == "pending" && last_payment&.response_code == payment_intent end + + def mark_as_processed + last_payment.update_attribute(:cvv_response_message, nil) + end + + def stripe_account_id + StripeAccount.find_by(enterprise_id: preferred_enterprise_id).stripe_user_id + end + + def preferred_enterprise_id + last_payment.payment_method.preferred_enterprise_id + end end diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 36f5b4f386..fc7a0b3279 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -83,8 +83,10 @@ describe Spree::OrdersController, type: :controller do describe "confirming a payment intent" do let(:customer) { create(:customer) } let(:order) { create(:order, customer: customer, distributor: customer.enterprise) } + let(:payment_method) { create(:stripe_sca_payment_method) } let!(:payment) { create( :payment, + payment_method: payment_method, cvv_response_message: "https://stripe.com/redirect", response_code: "pi_123", order: order, @@ -101,8 +103,16 @@ describe Spree::OrdersController, type: :controller do context "with a valid payment intent" do let(:payment_intent) { "pi_123" } + before do + allow_any_instance_of(Stripe::PaymentIntentValidator) + .to receive(:call) + .with(payment_intent, anything) + .and_return(payment_intent) + end + it "completes the payment" do get :show, params: { id: order.number, payment_intent: payment_intent } + expect(response).to be_success payment.reload expect(payment.cvv_response_message).to be nil @@ -112,9 +122,37 @@ describe Spree::OrdersController, type: :controller do context "with an invalid payment intent" do let(:payment_intent) { "invalid" } + let(:result) { instance_double(ProcessPaymentIntent::Result) } + + before do + allow_any_instance_of(Stripe::PaymentIntentValidator) + .to receive(:call) + .with(payment_intent, anything) + .and_return(result) + end it "does not complete the payment" do get :show, params: { id: order.number, payment_intent: payment_intent } + + expect(response).to be_success + payment.reload + expect(payment.cvv_response_message).to eq("https://stripe.com/redirect") + expect(payment.state).to eq("pending") + end + end + + context "with an invalid last payment" do + let(:payment_intent) { "valid" } + let(:finder) { instance_double(OrderPaymentFinder, last_payment: payment) } + + before do + allow(payment).to receive(:response_code).and_return("invalid") + allow(OrderPaymentFinder).to receive(:new).with(order).and_return(finder) + end + + it "does not complete the payment" do + get :show, params: { id: order.number, payment_intent: payment_intent } + expect(response).to be_success payment.reload expect(payment.cvv_response_message).to eq("https://stripe.com/redirect") diff --git a/spec/services/process_payment_intent_spec.rb b/spec/services/process_payment_intent_spec.rb index c93082da2a..2166ccb619 100644 --- a/spec/services/process_payment_intent_spec.rb +++ b/spec/services/process_payment_intent_spec.rb @@ -8,17 +8,36 @@ describe ProcessPaymentIntent do describe "processing a payment intent" do let(:customer) { create(:customer) } let(:order) { create(:order, customer: customer, distributor: customer.enterprise, state: "payment") } + let(:payment_method) { create(:stripe_sca_payment_method) } let!(:payment) { create( :payment, + payment_method: payment_method, cvv_response_message: "https://stripe.com/redirect", response_code: "pi_123", order: order, state: "pending") } + let(:validator) { instance_double(Stripe::PaymentIntentValidator) } + + before do + allow(Stripe::PaymentIntentValidator).to receive(:new).and_return(validator) + end context "an invalid intent" do - let(:invalid_intent) { "invalid" } - let(:service) { ProcessPaymentIntent.new(invalid_intent, order) } + let(:intent) { "invalid" } + let(:service) { ProcessPaymentIntent.new(intent, order) } + + before do + allow(validator) + .to receive(:call).with(intent, anything).and_raise(Stripe::StripeError, "error message") + end + + it "returns the error message" do + result = service.call! + + expect(result.ok?).to eq(false) + expect(result.error).to eq("error message") + end it "does not complete the payment" do service.call! @@ -27,11 +46,17 @@ describe ProcessPaymentIntent do end context "a valid intent" do - let(:valid_intent) { "pi_123" } - let(:service) { ProcessPaymentIntent.new(valid_intent, order) } + let(:intent) { "pi_123" } + let(:service) { ProcessPaymentIntent.new(intent, order) } before do allow(order).to receive(:deliver_order_confirmation_email) + allow(validator).to receive(:call).with(intent, anything).and_return(intent) + end + + it "validates the intent" do + service.call! + expect(validator).to have_received(:call) end it "completes the payment" do @@ -49,11 +74,19 @@ describe ProcessPaymentIntent do end context "payment is in a failed state" do - let(:invalid_intent) { "invalid" } - let(:service) { ProcessPaymentIntent.new(invalid_intent, order) } + let(:intent) { "valid" } + let(:service) { ProcessPaymentIntent.new(intent, order) } before do payment.update_attribute(:state, "failed") + allow(validator).to receive(:call).with(intent, anything).and_return(intent) + end + + it "does not return any error message" do + result = service.call! + + expect(result.ok?).to eq(false) + expect(result.error).to eq("") end it "does not complete the payment" do