From 4a8fa76c562c71513cdd29221bddf55acac06465 Mon Sep 17 00:00:00 2001 From: Eric Saupe Date: Wed, 19 Dec 2018 11:18:15 -0800 Subject: [PATCH 1/4] Match namespacing convention and inheritance of other core models --- .../app/models/spree/wallet_payment_source.rb | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/core/app/models/spree/wallet_payment_source.rb b/core/app/models/spree/wallet_payment_source.rb index 32c834f5568..8abefe17355 100644 --- a/core/app/models/spree/wallet_payment_source.rb +++ b/core/app/models/spree/wallet_payment_source.rb @@ -1,19 +1,21 @@ # frozen_string_literal: true -class Spree::WalletPaymentSource < ActiveRecord::Base - belongs_to :user, class_name: Spree::UserClassHandle.new, foreign_key: 'user_id', inverse_of: :wallet_payment_sources - belongs_to :payment_source, polymorphic: true, inverse_of: :wallet_payment_sources +module Spree + class WalletPaymentSource < Spree::Base + belongs_to :user, class_name: Spree::UserClassHandle.new, foreign_key: 'user_id', inverse_of: :wallet_payment_sources + belongs_to :payment_source, polymorphic: true, inverse_of: :wallet_payment_sources - validates_presence_of :user - validates_presence_of :payment_source + validates_presence_of :user + validates_presence_of :payment_source - validate :check_for_payment_source_class + validate :check_for_payment_source_class - private + private - def check_for_payment_source_class - if !payment_source.is_a?(Spree::PaymentSource) - errors.add(:payment_source, :has_to_be_payment_source_class) + def check_for_payment_source_class + if !payment_source.is_a?(Spree::PaymentSource) + errors.add(:payment_source, :has_to_be_payment_source_class) + end end end end From efd9f5f685dfcbdd4a4b54bdc18a486ac6d31943 Mon Sep 17 00:00:00 2001 From: Eric Saupe Date: Wed, 19 Dec 2018 11:47:16 -0800 Subject: [PATCH 2/4] Validate user has payment source already in their wallet Without this validation it's possible for a user to add the same PaymentSource to their wallet multiple times causing dirty data and additional code in places where the wallet is displayed to remove duplicates. --- .../app/models/spree/wallet_payment_source.rb | 4 ++++ core/config/locales/en.yml | 2 ++ .../spree/wallet_payment_source_spec.rb | 24 ++++++++++++++++--- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/wallet_payment_source.rb b/core/app/models/spree/wallet_payment_source.rb index 8abefe17355..ee3313a660e 100644 --- a/core/app/models/spree/wallet_payment_source.rb +++ b/core/app/models/spree/wallet_payment_source.rb @@ -7,6 +7,10 @@ class WalletPaymentSource < Spree::Base validates_presence_of :user validates_presence_of :payment_source + validates :user_id, uniqueness: { + scope: [:payment_source_type, :payment_source_id], + message: :payment_source_already_exists + } validate :check_for_payment_source_class diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index 93d2f2175a9..f8992c6eff0 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -488,6 +488,8 @@ en: attributes: payment_source: has_to_be_payment_source_class: has to be a Spree::PaymentSource + user_id: + payment_source_already_exists: already has the Spree::PaymentSource in their wallet models: spree/address: one: Address diff --git a/core/spec/models/spree/wallet_payment_source_spec.rb b/core/spec/models/spree/wallet_payment_source_spec.rb index 45f02d31b1a..3efe6648f05 100644 --- a/core/spec/models/spree/wallet_payment_source_spec.rb +++ b/core/spec/models/spree/wallet_payment_source_spec.rb @@ -6,6 +6,8 @@ subject { Spree::WalletPaymentSource } describe "validation" do + let(:user) { create(:user) } + context 'with a non-PaymentSource model' do with_model 'NonPaymentSource', scope: :all do model do @@ -19,7 +21,7 @@ it "errors when `payment_source` is not a `Spree::PaymentSource`" do wallet_payment_source = Spree::WalletPaymentSource.new( payment_source: payment_source, - user: create(:user) + user: user ) expect(wallet_payment_source).not_to be_valid @@ -29,10 +31,26 @@ end end + it "is invalid if `payment_source` is already in the user's wallet" do + credit_card = create(:credit_card) + Spree::WalletPaymentSource.create( + payment_source: credit_card, + user: user + ) + wallet_payment_source = subject.new( + payment_source: credit_card, + user: user + ) + expect(wallet_payment_source).not_to be_valid + expect(wallet_payment_source.errors.messages).to eq( + { user_id: ["already has the Spree::PaymentSource in their wallet"] } + ) + end + it "is valid with a `credit_card` as `payment_source`" do valid_attrs = { payment_source: create(:credit_card), - user: create(:user) + user: user } expect(subject.new(valid_attrs)).to be_valid end @@ -40,7 +58,7 @@ it "is valid with `store_credit` as `payment_source`" do valid_attrs = { payment_source: create(:store_credit), - user: create(:user) + user: user } expect(subject.new(valid_attrs)).to be_valid end From 3835f9ddf78fe99a65f7e6b171ff6c061bcbdf12 Mon Sep 17 00:00:00 2001 From: Eric Saupe Date: Wed, 19 Dec 2018 11:56:05 -0800 Subject: [PATCH 3/4] Validate user owns the payment source before it is added to their wallet This fixes the possibility of a User gaining access to a PaymentSource that isn't theirs by validating the WalletPaymentSource user_id is the same as the PaymentSource user_id if one is present. --- core/app/models/spree/wallet_payment_source.rb | 10 ++++++++++ core/config/locales/en.yml | 1 + .../models/spree/wallet_payment_source_spec.rb | 17 ++++++++++++++--- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/wallet_payment_source.rb b/core/app/models/spree/wallet_payment_source.rb index ee3313a660e..d79e0e2d907 100644 --- a/core/app/models/spree/wallet_payment_source.rb +++ b/core/app/models/spree/wallet_payment_source.rb @@ -13,6 +13,7 @@ class WalletPaymentSource < Spree::Base } validate :check_for_payment_source_class + validate :validate_payment_source_ownership private @@ -21,5 +22,14 @@ def check_for_payment_source_class errors.add(:payment_source, :has_to_be_payment_source_class) end end + + def validate_payment_source_ownership + return unless payment_source.present? + + if payment_source.respond_to?(:user_id) && + payment_source.user_id != user_id + errors.add(:payment_source, :not_owned_by_user) + end + end end end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index f8992c6eff0..61546f65c67 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -488,6 +488,7 @@ en: attributes: payment_source: has_to_be_payment_source_class: has to be a Spree::PaymentSource + not_owned_by_user: does not belong to user user_id: payment_source_already_exists: already has the Spree::PaymentSource in their wallet models: diff --git a/core/spec/models/spree/wallet_payment_source_spec.rb b/core/spec/models/spree/wallet_payment_source_spec.rb index 3efe6648f05..5eb97f56b8e 100644 --- a/core/spec/models/spree/wallet_payment_source_spec.rb +++ b/core/spec/models/spree/wallet_payment_source_spec.rb @@ -32,7 +32,7 @@ end it "is invalid if `payment_source` is already in the user's wallet" do - credit_card = create(:credit_card) + credit_card = create(:credit_card, user: user) Spree::WalletPaymentSource.create( payment_source: credit_card, user: user @@ -47,9 +47,20 @@ ) end + it "is invalid when `payment_source` is not owned by the user" do + wallet_payment_source = subject.new( + payment_source: create(:credit_card), + user: user + ) + expect(wallet_payment_source).not_to be_valid + expect(wallet_payment_source.errors.messages).to eq( + { payment_source: ["does not belong to user"] } + ) + end + it "is valid with a `credit_card` as `payment_source`" do valid_attrs = { - payment_source: create(:credit_card), + payment_source: create(:credit_card, user: user), user: user } expect(subject.new(valid_attrs)).to be_valid @@ -57,7 +68,7 @@ it "is valid with `store_credit` as `payment_source`" do valid_attrs = { - payment_source: create(:store_credit), + payment_source: create(:store_credit, user: user), user: user } expect(subject.new(valid_attrs)).to be_valid From 28c66c8fcdbf7c8b274c29bd6a2abdc80f17517e Mon Sep 17 00:00:00 2001 From: Eric Saupe Date: Wed, 19 Dec 2018 15:44:12 -0800 Subject: [PATCH 4/4] Updated specs to respect payment owners With the changes made to validate users cannot add payment sources from other users to their wallet. Some of our specs were using and adding payment sources created for other users to the wallet of the user being tested. --- backend/spec/features/admin/orders/payments_spec.rb | 2 +- core/spec/models/spree/order/checkout_spec.rb | 10 +++------- core/spec/models/spree/wallet_spec.rb | 2 +- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/backend/spec/features/admin/orders/payments_spec.rb b/backend/spec/features/admin/orders/payments_spec.rb index 5e5a931ab4c..034bd517305 100644 --- a/backend/spec/features/admin/orders/payments_spec.rb +++ b/backend/spec/features/admin/orders/payments_spec.rb @@ -182,7 +182,7 @@ context "user existing card" do let!(:cc) do - create(:credit_card, payment_method: payment_method, gateway_customer_profile_id: "BGS-RFRE") + create(:credit_card, payment_method: payment_method, gateway_customer_profile_id: "BGS-RFRE", user: order.user) end before do diff --git a/core/spec/models/spree/order/checkout_spec.rb b/core/spec/models/spree/order/checkout_spec.rb index cc0856bc45b..9deaa48db1d 100644 --- a/core/spec/models/spree/order/checkout_spec.rb +++ b/core/spec/models/spree/order/checkout_spec.rb @@ -359,6 +359,7 @@ def assert_state_changed(order, from, to) before do user = create(:user, email: 'spree@example.org', bill_address: user_bill_address) + default_credit_card.update(user: user) wallet_payment_source = user.wallet.add(default_credit_card) user.wallet.default_wallet_payment_source = wallet_payment_source order.user = user @@ -497,12 +498,7 @@ def assert_state_changed(order, from, to) context "with a payment in the pending state" do let(:order) { create :order_ready_to_complete } - let(:payment) { create :payment, state: "pending", amount: order.total } - - before do - order.payments = [payment] - order.save! - end + let(:payment) { create :payment, order: order, state: "pending", amount: order.total } it "allows the order to complete" do expect { order.complete! }. @@ -542,7 +538,7 @@ def assert_state_changed(order, from, to) order.user = FactoryBot.create(:user) order.store = FactoryBot.create(:store) order.email = 'spree@example.org' - order.payments << FactoryBot.create(:payment) + order.payments << FactoryBot.create(:payment, order: order) # make sure we will actually capture a payment allow(order).to receive_messages(payment_required?: true) diff --git a/core/spec/models/spree/wallet_spec.rb b/core/spec/models/spree/wallet_spec.rb index 4235f65cf1c..f198a3e76fb 100644 --- a/core/spec/models/spree/wallet_spec.rb +++ b/core/spec/models/spree/wallet_spec.rb @@ -123,7 +123,7 @@ end context 'with a wallet payment source that does not belong to the wallet' do - let(:other_wallet_credit_card) { other_wallet.add(credit_card) } + let(:other_wallet_credit_card) { other_wallet.add(other_credit_card) } let(:other_wallet) { Spree::Wallet.new(other_user) } let(:other_credit_card) { create(:credit_card, user_id: other_user.id) } let(:other_user) { create(:user) }