New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify ownership of payment_source when creating WalletPaymentSource #3007

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ericsaupe
Copy link
Contributor

ericsaupe commented Dec 18, 2018

This merge request takes care of these things:

  1. It's currently possible for payment sources created by one user to be added to the wallet of another user. This potentially allows one user to use payments stored for other users.
  2. Cleans up data by restricting payment sources being added to a user's wallet if that payment source has already been added to the wallet.
  3. Matches namespacing and inheritance for the Spree::WalletPaymentSource class to be the same as the other models in spree_core inheriting from Spree::Base
return unless payment_source.present?

if payment_source.respond_to?(:user_id) &&
payment_source.user_id != user_id

This comment has been minimized.

@houndci-bot

houndci-bot Dec 18, 2018

Layout/MultilineOperationIndentation: Align the operands of a condition in an if statement spanning multiple lines.

errors.add(:payment_source, :has_to_be_payment_source_class)
def validate_payment_source_ownership
return unless payment_source.present?

This comment has been minimized.

@houndci-bot

houndci-bot Dec 18, 2018

Layout/TrailingWhitespace: Trailing whitespace detected.

@ericsaupe ericsaupe force-pushed the ericsaupe:wallet-payment-source-protection branch from b66a8a6 to 749f9c5 Dec 18, 2018

@skukx

skukx approved these changes Dec 18, 2018


validates_presence_of :user
validates_presence_of :payment_source
validates_presence_of :user

This comment has been minimized.

@skukx

skukx Dec 18, 2018

Contributor

I would prefer validates :user, presence: true. Not a big deal though

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

This comment has been minimized.

@skukx

skukx Dec 18, 2018

Contributor

It may be worth adding a custom message here. I found the default message on this is going to be User id has already been taken. That message doesn't really denote that the violation is due to the tuple key

This comment has been minimized.

@kennyadsl

kennyadsl Dec 19, 2018

Member

Also, can we please document why this change is needed in this PR?

validates_presence_of :user
validates_presence_of :payment_source
validates :user_id, uniqueness: { scope: [:payment_source_type, :payment_source_id] }
validate :check_for_payment_source_class

This comment has been minimized.

@skukx

skukx Dec 18, 2018

Contributor

I'm guessing the reasoning for this is to mimic what happens in the Spree::Wallet#add method? I'm a fan prepending validation methods with validate so you know the purpose of that method is to validate and if called will add to the object errors. validate_payment_source_class

@kennyadsl
Copy link
Member

kennyadsl left a comment

Thanks @ericsaupe, I left some comments to improve PR documentation and also notice the build to fail on something that should be related to this, can you please take a look?

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

This comment has been minimized.

@kennyadsl

kennyadsl Dec 19, 2018

Member

can you please change the namespace/inheritance stuff into one or more other commits, so they are independently documented? Also, why are these change needed?

This comment has been minimized.

@ericsaupe

ericsaupe Jan 10, 2019

Contributor

It's to follow the format used in the other model files.

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

This comment has been minimized.

@kennyadsl

kennyadsl Dec 19, 2018

Member

Also, can we please document why this change is needed in this PR?

ericsaupe added some commits Dec 19, 2018

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.
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.
@skukx

This comment has been minimized.

Copy link
Contributor

skukx commented Dec 19, 2018

To clarify some of these changes (each change should probably be documented in it's own commit):

validates :user_id, uniqueness: { scope: [:payment_source_type, :payment_source_id] }

This change is due to a unique tuple key on the database. Adding the validation mirrors what is enforced at the database layer and allows activerecord to handle it. What was a PG::UniqueViolation error becomes an ActiveRecord::RecordInvalid error.


For

validate :validate_payment_source_ownership

It is possible that a user's wallet can have a payment source added to it which does not belong to the user. This means a user could make transactions on a payment source that does not belong to them.

@ericsaupe ericsaupe force-pushed the ericsaupe:wallet-payment-source-protection branch from 749f9c5 to f4df47b Dec 19, 2018

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.

@ericsaupe ericsaupe force-pushed the ericsaupe:wallet-payment-source-protection branch from f4df47b to 28c66c8 Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment