From 6d51ece69c8ea02b5a4d9ce3185710bee28680f6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Mar 2021 12:14:47 +0000 Subject: [PATCH 1/4] Tidy up payment method params --- .../spree/admin/payment_methods_controller.rb | 45 +++++++++++-------- .../admin/payment_methods_controller_spec.rb | 41 ++++++++++++++++- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb index 5086ad64cd..aaee7fb306 100644 --- a/app/controllers/spree/admin/payment_methods_controller.rb +++ b/app/controllers/spree/admin/payment_methods_controller.rb @@ -12,13 +12,11 @@ module Spree def create force_environment - @payment_method = params[:payment_method]. - delete(:type). - constantize. - new(PermittedAttributes::PaymentMethod.new(params[:payment_method]).call) + @payment_method = payment_method_class.constantize.new(base_params) @object = @payment_method invoke_callbacks(:create, :before) + if @payment_method.save invoke_callbacks(:create, :after) flash[:success] = Spree.t(:successfully_created, resource: Spree.t(:payment_method)) @@ -34,16 +32,16 @@ module Spree force_environment invoke_callbacks(:update, :before) - payment_method_type = params[:payment_method].delete(:type) - if @payment_method['type'].to_s != payment_method_type + + if @payment_method['type'].to_s != payment_method_class @payment_method.update_columns( - type: payment_method_type, + type: payment_method_class, updated_at: Time.zone.now ) @payment_method = PaymentMethod.find(params[:id]) end - if @payment_method.update(params_for_update) + if @payment_method.update(update_params) invoke_callbacks(:update, :after) flash[:success] = Spree.t(:successfully_updated, resource: Spree.t(:payment_method)) redirect_to spree.edit_admin_payment_method_path(@payment_method) @@ -98,8 +96,12 @@ module Spree private + def payment_method_class + base_params.delete(:type) + end + def force_environment - params[:payment_method][:environment] = Rails.env unless spree_current_user.admin? + base_params[:environment] = Rails.env unless spree_current_user.admin? end def load_data @@ -143,7 +145,7 @@ module Spree @stripe_account_holder = Enterprise.find(@payment_method.preferred_enterprise_id) return if spree_current_user.enterprises.include? @stripe_account_holder - params[:payment_method][:preferred_enterprise_id] = @stripe_account_holder.id + update_params[:preferred_enterprise_id] = @stripe_account_holder.id end def stripe_payment_method? @@ -155,19 +157,26 @@ module Spree provider.name.ends_with?("StripeConnect", "StripeSCA") end + def base_params + @base_params ||= PermittedAttributes::PaymentMethod.new(params[:payment_method]). + call.to_h.with_indifferent_access + end + # Merge payment method params with gateway params like :gateway_stripe_connect # Also, remove password if present and blank - def params_for_update - gateway_params = params[ActiveModel::Naming.param_key(@payment_method)] || {} - params_for_update = params[:payment_method].merge(gateway_params) + def update_params + @update_params ||= begin + gateway_params = raw_params[ActiveModel::Naming.param_key(@payment_method)] || {} + params_for_update = base_params.merge(gateway_params) - params_for_update.each do |key, _value| - if key.include?("password") && params_for_update[key].blank? - params_for_update.delete(key) + params_for_update.each do |key, value| + if key.include?("password") && value.blank? + params_for_update.delete(key) + end end - end - PermittedAttributes::PaymentMethod.new(params_for_update).call + params_for_update + end end end end diff --git a/spec/controllers/spree/admin/payment_methods_controller_spec.rb b/spec/controllers/spree/admin/payment_methods_controller_spec.rb index 75abf32a63..1ed6f5de1a 100644 --- a/spec/controllers/spree/admin/payment_methods_controller_spec.rb +++ b/spec/controllers/spree/admin/payment_methods_controller_spec.rb @@ -52,6 +52,37 @@ module Spree end describe "#update" do + context "updating a payment method" do + let!(:payment_method) { create(:payment_method, :flat_rate) } + let(:params) { + { + id: payment_method.id, + payment_method: { + name: "Updated", + description: "Updated", + type: "Spree::PaymentMethod::Check", + calculator_attributes: { + id: payment_method.calculator.id, + preferred_amount: 456, + preferred_currency: "GBP" + } + } + } + } + + before { controller_login_as_admin } + + it "updates the payment method" do + spree_post :update, params + payment_method.reload + + expect(payment_method.name).to eq "Updated" + expect(payment_method.description).to eq "Updated" + expect(payment_method.calculator.preferred_amount).to eq 456 + expect(payment_method.calculator.preferred_currency).to eq "GBP" + end + end + context "on a StripeConnect payment method" do let!(:user) { create(:user, enterprise_limit: 2) } let!(:enterprise1) { create(:distributor_enterprise, owner: user) } @@ -61,7 +92,15 @@ module Spree before { allow(controller).to receive(:spree_current_user) { user } } context "when an attempt is made to change the stripe account holder (preferred_enterprise_id)" do - let(:params) { { id: payment_method.id, payment_method: { type: "Spree::Gateway::StripeConnect", preferred_enterprise_id: enterprise1.id } } } + let(:params) { + { + id: payment_method.id, + payment_method: { + type: "Spree::Gateway::StripeConnect", + preferred_enterprise_id: enterprise1.id + } + } + } context "as a user that does not manage the existing stripe account holder" do it "prevents the stripe account holder from being updated" do From 707d089419d8051ff778d3006e01e535656e1fb3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 8 Mar 2021 16:13:28 +0000 Subject: [PATCH 2/4] Update type handling in PaymentMethodsController and add tests --- .../spree/admin/payment_methods_controller.rb | 4 ++-- .../admin/payment_methods_controller_spec.rb | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb index aaee7fb306..9cfd7c2492 100644 --- a/app/controllers/spree/admin/payment_methods_controller.rb +++ b/app/controllers/spree/admin/payment_methods_controller.rb @@ -33,7 +33,7 @@ module Spree invoke_callbacks(:update, :before) - if @payment_method['type'].to_s != payment_method_class + if @payment_method.type.to_s != payment_method_class @payment_method.update_columns( type: payment_method_class, updated_at: Time.zone.now @@ -97,7 +97,7 @@ module Spree private def payment_method_class - base_params.delete(:type) + @payment_method_class ||= base_params.delete(:type) end def force_environment diff --git a/spec/controllers/spree/admin/payment_methods_controller_spec.rb b/spec/controllers/spree/admin/payment_methods_controller_spec.rb index 1ed6f5de1a..497d430de6 100644 --- a/spec/controllers/spree/admin/payment_methods_controller_spec.rb +++ b/spec/controllers/spree/admin/payment_methods_controller_spec.rb @@ -81,6 +81,23 @@ module Spree expect(payment_method.calculator.preferred_amount).to eq 456 expect(payment_method.calculator.preferred_currency).to eq "GBP" end + + context "when the given payment method type does not match" do + let(:params) { + { + id: payment_method.id, + payment_method: { + type: "Spree::Gateway::Bogus" + } + } + } + + it "updates the payment method type" do + spree_post :update, params + + expect(PaymentMethod.find(payment_method.id).type).to eq "Spree::Gateway::Bogus" + end + end end context "on a StripeConnect payment method" do From 6996001580fee6607ebf718713e0f698aea80406 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Tue, 9 Mar 2021 10:06:12 -0800 Subject: [PATCH 3/4] add defensive checking for stripe response --- lib/stripe/authorize_response_patcher.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/stripe/authorize_response_patcher.rb b/lib/stripe/authorize_response_patcher.rb index d67b8dbca9..3d8195f14a 100644 --- a/lib/stripe/authorize_response_patcher.rb +++ b/lib/stripe/authorize_response_patcher.rb @@ -19,12 +19,15 @@ module Stripe private def url_for_authorization(response) - next_action = response.params["next_source_action"] - return unless response.params["status"] == "requires_source_action" && - next_action.present? && - next_action["type"] == "authorize_with_url" + return unless %w(requires_source_action requires_action).include?(response.params["status"]) - url = next_action["authorize_with_url"]["url"] + next_action = response.params["next_source_action"] || response.params["next_action"] + return unless next_action.present? + + next_action_type = next_action["type"] + return unless %w(authorize_with_url redirect_to_url).include?(next_action_type) + + url = next_action[next_action_type]["url"] return url if url.match(%r{https?:\/\/[\S]+}) && url.include?("stripe.com") end From b7f9fb8d7223fc345693a1f1445956dc22b8ff9a Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Tue, 9 Mar 2021 10:06:31 -0800 Subject: [PATCH 4/4] send full redirect URL to stripe --- app/controllers/spree/admin/payments_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index f818022dfd..cddf685dd7 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -3,6 +3,8 @@ module Spree module Admin class PaymentsController < Spree::Admin::BaseController + include FullUrlHelper + before_action :load_order, except: [:show] before_action :load_payment, only: [:fire, :show] before_action :load_data @@ -153,7 +155,7 @@ module Spree def authorize_stripe_sca_payment return unless @payment.payment_method.class == Spree::Gateway::StripeSCA - @payment.authorize!(main_app.order_path(@payment.order, only_path: false)) + @payment.authorize!(full_order_path(@payment.order)) raise Spree::Core::GatewayError, I18n.t('authorization_failure') unless @payment.pending?