Merge pull request #3007 from ericsaupe/wallet-payment-source-protection

Verify ownership of payment_source when creating WalletPaymentSource

backend/spec/features/admin/orders/payments_spec.rb

@@ -228,7 +228,7 @@ def create_payment(opts = {})
 
     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

core/app/models/spree/wallet_payment_source.rb

@@ -1,19 +1,35 @@
 # 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
+    validates :user_id, uniqueness: {
+      scope: [:payment_source_type, :payment_source_id],
+      message: :payment_source_already_exists
+    }
 
-  validate :check_for_payment_source_class
+    validate :check_for_payment_source_class
+    validate :validate_payment_source_ownership
 
-  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
+
+    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

core/config/locales/en.yml

@@ -492,6 +492,9 @@ 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:
       spree/address:
         one: Address

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)

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,18 +31,45 @@
       end
     end
 
+    it "is invalid if `payment_source` is already in the user's wallet" do
+      credit_card = create(:credit_card, user: user)
+      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 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),
-        user: create(:user)
+        payment_source: create(:credit_card, user: user),
+        user: user
       }
       expect(subject.new(valid_attrs)).to be_valid
     end
 
     it "is valid with `store_credit` as `payment_source`" do
       valid_attrs = {
-        payment_source: create(:store_credit),
-        user: create(:user)
+        payment_source: create(:store_credit, user: user),
+        user: user
       }
       expect(subject.new(valid_attrs)).to be_valid
     end

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) }