From 5af27bb14ef2823f2dbdcbfebd9441a898c72132 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 26 Feb 2020 15:02:10 +0000 Subject: [PATCH 1/5] Make checkout controller handle strong parameters --- app/controllers/checkout_controller.rb | 19 +++++++++++++++- app/services/checkout/form_data_adapter.rb | 6 +---- .../checkout/form_data_adapter_spec.rb | 22 ++++++------------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 3e00127d8a..ce8728ee37 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -43,7 +43,7 @@ class CheckoutController < Spree::StoreController def update params_adapter = Checkout::FormDataAdapter.new(params, @order, spree_current_user) - return update_failed unless @order.update_attributes(params_adapter.order_params) + return update_failed unless @order.update_attributes(order_params(params_adapter.params)) fire_event('spree.checkout.update') @@ -237,4 +237,21 @@ class CheckoutController < Spree::StoreController end end end + + def order_params(params) + params.require(:order).permit( + :email, :special_instructions, + payments_attributes: + [ + :payment_method_id, :amount, + source_attributes: [ + :gateway_payment_profile_id, :cc_type, :last_digits, + :month, :year, :first_name, :last_name, + :number, :verification_value + ] + ], + bill_address_attributes: permitted_address_attributes, + ship_address_attributes: permitted_address_attributes + ) + end end diff --git a/app/services/checkout/form_data_adapter.rb b/app/services/checkout/form_data_adapter.rb index 8fc10fc4ca..a374444637 100644 --- a/app/services/checkout/form_data_adapter.rb +++ b/app/services/checkout/form_data_adapter.rb @@ -3,7 +3,7 @@ # Adapts checkout form data (params) so that the order can be directly saved to the database module Checkout class FormDataAdapter - attr_reader :shipping_method_id + attr_reader :params, :shipping_method_id def initialize(params, order, current_user) @params = params.dup @@ -19,10 +19,6 @@ module Checkout @shipping_method_id = @params[:order].delete(:shipping_method_id) end - def order_params - @params[:order] - end - private # For payment step, filter order parameters to produce the expected diff --git a/spec/services/checkout/form_data_adapter_spec.rb b/spec/services/checkout/form_data_adapter_spec.rb index b1b92b3053..be248a2af0 100644 --- a/spec/services/checkout/form_data_adapter_spec.rb +++ b/spec/services/checkout/form_data_adapter_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Checkout::FormDataAdapter do - describe '#order_params' do + describe '#params' do let(:params) { { order: { order_id: "123" } } } let(:order) { create(:order) } let(:user) { create(:user) } @@ -11,9 +11,7 @@ describe Checkout::FormDataAdapter do let(:adapter) { Checkout::FormDataAdapter.new(params, order, user) } it "returns the :order item in the params provided" do - order_params = adapter.order_params - - expect(order_params).to eq params[:order] + expect(adapter.params[:order]).to eq params[:order] end describe "when payment_attributes are provided" do @@ -25,9 +23,7 @@ describe Checkout::FormDataAdapter do before { params[:payment_source] = { "123" => source_attributes } } it "moves payment source attributes to the order payment attributes" do - order_params = adapter.order_params - - expect(order_params[:payments_attributes]. + expect(adapter.params[:order][:payments_attributes]. first[:source_attributes]).to eq source_attributes end end @@ -36,9 +32,7 @@ describe Checkout::FormDataAdapter do before { order.total = "50.0" } it "sets the payment attributes amount to the order total" do - order_params = adapter.order_params - - expect(order_params[:payments_attributes].first[:amount]).to eq order.total + expect(adapter.params[:order][:payments_attributes].first[:amount]).to eq order.total end end @@ -51,10 +45,8 @@ describe Checkout::FormDataAdapter do before { params[:order][:existing_card_id] = credit_card.id } it "adds card details to payment attributes" do - order_params = adapter.order_params - - expect(order_params[:payments_attributes].first[:source][:id]).to eq credit_card.id - expect(order_params[:payments_attributes]. + expect(adapter.params[:order][:payments_attributes].first[:source][:id]).to eq credit_card.id + expect(adapter.params[:order][:payments_attributes]. first[:source][:last_digits]).to eq credit_card.last_digits end end @@ -63,7 +55,7 @@ describe Checkout::FormDataAdapter do let(:credit_card) { create(:credit_card) } it "raises exception if credit card provided doesnt belong to the current user" do - expect { adapter.order_params }.to raise_error Spree::Core::GatewayError + expect { adapter.params[:order] }.to raise_error Spree::Core::GatewayError end end end From d7cccd41437cc435b973b1293978e2872150d446 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 2 Mar 2020 13:31:34 +0000 Subject: [PATCH 2/5] Add guard clause in checkout_controller for empty params[:order] --- app/controllers/checkout_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index ce8728ee37..e3c083e7eb 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -239,6 +239,8 @@ class CheckoutController < Spree::StoreController end def order_params(params) + return params[:order] if params[:order].empty? + params.require(:order).permit( :email, :special_instructions, payments_attributes: From 5ae2e6865c2ba38076726c4c34873e26fe7acf75 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 6 Mar 2020 10:14:45 +0000 Subject: [PATCH 3/5] Add one more needed permitted attribute to checkout controller --- app/controllers/checkout_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index e3c083e7eb..83ecd4cc37 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -249,7 +249,8 @@ class CheckoutController < Spree::StoreController source_attributes: [ :gateway_payment_profile_id, :cc_type, :last_digits, :month, :year, :first_name, :last_name, - :number, :verification_value + :number, :verification_value, + :save_requested_by_customer ] ], bill_address_attributes: permitted_address_attributes, From 1d9a6edefbb2cdf36467489cc2e67163bfa0d141 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 9 Mar 2020 11:38:45 +0000 Subject: [PATCH 4/5] Permit params in checkout controller before we adapt them to bypass issues with permitting added attributes like source CreditCard --- app/controllers/checkout_controller.rb | 41 +++++++++++++++----------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 83ecd4cc37..0b9de7a174 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -42,8 +42,8 @@ class CheckoutController < Spree::StoreController end def update - params_adapter = Checkout::FormDataAdapter.new(params, @order, spree_current_user) - return update_failed unless @order.update_attributes(order_params(params_adapter.params)) + params_adapter = Checkout::FormDataAdapter.new(permitted_params, @order, spree_current_user) + return update_failed unless @order.update_attributes(params_adapter.params[:order]) fire_event('spree.checkout.update') @@ -238,23 +238,28 @@ class CheckoutController < Spree::StoreController end end - def order_params(params) - return params[:order] if params[:order].empty? - - params.require(:order).permit( - :email, :special_instructions, - payments_attributes: - [ - :payment_method_id, :amount, - source_attributes: [ - :gateway_payment_profile_id, :cc_type, :last_digits, - :month, :year, :first_name, :last_name, - :number, :verification_value, - :save_requested_by_customer - ] + def permitted_params + params.permit( + order: [ + :email, :special_instructions, + :existing_card_id, :shipping_method_id, + payments_attributes: [ + :payment_method_id, + source_attributes: payment_source_attributes ], - bill_address_attributes: permitted_address_attributes, - ship_address_attributes: permitted_address_attributes + ship_address_attributes: permitted_address_attributes, + bill_address_attributes: permitted_address_attributes + ], + payment_source: payment_source_attributes ) end + + def payment_source_attributes + [ + :gateway_payment_profile_id, :cc_type, :last_digits, + :month, :year, :first_name, :last_name, + :number, :verification_value, + :save_requested_by_customer + ] + end end From 2c453359c186e4bdfd0fe6fa321b7f38ec17a78f Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 21 Mar 2020 16:38:11 +0000 Subject: [PATCH 5/5] Extract permitted params into a specific service --- app/controllers/checkout_controller.rb | 23 +------------ app/services/permitted_attributes/address.rb | 11 ++++++ app/services/permitted_attributes/checkout.rb | 34 +++++++++++++++++++ 3 files changed, 46 insertions(+), 22 deletions(-) create mode 100644 app/services/permitted_attributes/address.rb create mode 100644 app/services/permitted_attributes/checkout.rb diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 0b9de7a174..d5f2e0ffee 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -239,27 +239,6 @@ class CheckoutController < Spree::StoreController end def permitted_params - params.permit( - order: [ - :email, :special_instructions, - :existing_card_id, :shipping_method_id, - payments_attributes: [ - :payment_method_id, - source_attributes: payment_source_attributes - ], - ship_address_attributes: permitted_address_attributes, - bill_address_attributes: permitted_address_attributes - ], - payment_source: payment_source_attributes - ) - end - - def payment_source_attributes - [ - :gateway_payment_profile_id, :cc_type, :last_digits, - :month, :year, :first_name, :last_name, - :number, :verification_value, - :save_requested_by_customer - ] + PermittedAttributes::Checkout.new(params).call end end diff --git a/app/services/permitted_attributes/address.rb b/app/services/permitted_attributes/address.rb new file mode 100644 index 0000000000..4fd7908297 --- /dev/null +++ b/app/services/permitted_attributes/address.rb @@ -0,0 +1,11 @@ +module PermittedAttributes + class Address + def self.attributes + [ + :firstname, :lastname, :address1, :address2, + :city, :country_id, :state_id, :zipcode, + :phone, :state_name, :alternative_phone, :company + ] + end + end +end diff --git a/app/services/permitted_attributes/checkout.rb b/app/services/permitted_attributes/checkout.rb new file mode 100644 index 0000000000..c5bda0739a --- /dev/null +++ b/app/services/permitted_attributes/checkout.rb @@ -0,0 +1,34 @@ +module PermittedAttributes + class Checkout + def initialize(params) + @params = params + end + + def call + @params.permit( + order: [ + :email, :special_instructions, + :existing_card_id, :shipping_method_id, + payments_attributes: [ + :payment_method_id, + source_attributes: payment_source_attributes + ], + ship_address_attributes: PermittedAttributes::Address.attributes, + bill_address_attributes: PermittedAttributes::Address.attributes + ], + payment_source: payment_source_attributes + ) + end + + private + + def payment_source_attributes + [ + :gateway_payment_profile_id, :cc_type, :last_digits, + :month, :year, :first_name, :last_name, + :number, :verification_value, + :save_requested_by_customer + ] + end + end +end