Skip to content
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

"Token billing" should use cardReference to supply the tokenised card #89

Closed
judgej opened this issue Sep 1, 2017 · 19 comments
Closed

Comments

@judgej
Copy link
Member

judgej commented Sep 1, 2017

The Token Billing page describes the ability for a gateway to store a permanent or long-term credit card in the gateway account, and reference that card using a token. The token is called cardReference here.

This driver supports this token billing, but refers to the card token as token instead of cardReference. It needs to be switched around for consistency.

Some additional discussions here:

#25

judgej added a commit to academe/omnipay-sagepay that referenced this issue Sep 2, 2017
@judgej
Copy link
Member Author

judgej commented Sep 3, 2017

Thinking about this a little more, Sage Pay has a storeToken flag that must be set on each use of the token in order to keep it permanent. Without that flag being set on the next use, it will be deleted on that use.

So it could be argued that Sage Pay supports both single-use and permanent tokens. Our use of setToken() vs setCardReference() in a transaction could be an implicit use of the storeToken flag. So setToken in a payment would always ask for the token to be thrown away after use, and setCardReference would always ask for the token to be stored for future reuse.

I will check whether it is possible to tokenise a credit card using AJAX, as that could open up many possibilities. I have not seen that done in this gateway API, since both Direct and Server APIs use the site's IP address as a security check (the ONLY real security check TBH, which is a little crazy, especially on shared hosting), so it is probably not possible. However, using Sage Pay Server to just capture the credit card details in an iframe as a one-off token, then creating the actual transaction server-to-server (Sage Pay Direct) is very possible, and could make a lot of sense in some use-cases.

So I think I would propose using token to implement a single-use tokenised card, and cardReference to implement a long-term stored card reference. and take the storeToken flag away from direct use.

judgej added a commit to academe/omnipay-sagepay that referenced this issue Sep 3, 2017
…they are saved or not.

Documentation updated to show more clearly how this is used. Needs a
bit of a tidy-up, but I'm pushing to get some eyes on it.
judgej added a commit to academe/omnipay-sagepay that referenced this issue Sep 3, 2017
Trying to get some consistency between the methods available in
the notify handler and the direct Response class, since there is
a lot of similarities between the two.
judgej added a commit to academe/omnipay-sagepay that referenced this issue Sep 5, 2017
Some additional tests will be needed for this.
Sage Pay Direct can take the CC details or a token/reference,
and all always be able to take the CVV.
Sage Pay Server will optional take  atoken/reference.
This will need some further refactering when PayPal support
is implemented, since that overrides both tokens and CC details.
judgej added a commit to academe/omnipay-sagepay that referenced this issue Sep 5, 2017
Realised through testing that the card details are never returned
in the response from Sage Pay Direct createToken or transaction
requests, since the merchant site will already have these details.
They are only ever returned to the Sage Pay Server notification
handler.
judgej added a commit to academe/omnipay-sagepay that referenced this issue Sep 5, 2017
During testing, some inconsistencies were found in the way Sage Pay
generates its signatures. These changes code around those bizarre
inconsistencies.
@vekien
Copy link

vekien commented Sep 6, 2017

Hello, great work on this. Looking promising.

I'm still getting a little confused:

In order to get the request I need to do some action, eg:

  • $request = $gateway->purchase($options);

but at this point I need to provide the $options = [] argument to purchase which required card, so not sure when in the logic to apply $request->setCardReference()

I can provide sample code if it helps!

@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

Yes, that can be a little confusing, but it's due to the origins of Omnipay.

The CreditCard class holds both the credit card details and the billing and shipping user details. If you are sending any of those details into Omnipay then you need to use the CreditCard class. You may just set the CC details (number, expiry etc,) or just set the user details (billing names, address etc.) or both. In a future release of Omnipay, 4.0+, I would like to see these separated. In the meantime, just be aware this class serves two purposes.

The second thing to know is that options can be set in two main ways. These are equivalent, they set the card reference the same in both cases. Just use whichever is most useful to you in the context, or mix the two. You may have all the options up front, so pass in the array. You may build them up bit-by-bit, so use the setter methods. That's a feature across all Omnipay drivers for all options:

$request = $gateway->purchase();
$request->setCardReference('{abc123}');

and

$request = $gateway->purchase(['cardReference => '{abc123}']);

So in your particular case, you can set the cardReference you created earlier using either of these methods, but also pass in the card object with the billing and shipping address details only:

$request = $gateway->purchase(['cardReference => '{abc123}', 'card' => $card, ...]);

A slight variation to that, is that for Sage Pay Direct you may have to get the CVV from the user and add that to the CreditCard object, because Sage Pay - along with all gateways, I hope - does not store the CVV along with long-term tokens. For Sage Pay Server (less PCI burden to use) the Sage Pay site will prompt the user for their CVV but not their other card details, when using a reusable token. Just as a twist, the gateway will store the CVV the first time the card token is created, and will keep it until the first time that token is used to make a payment or authorisation. This lets you take all the CVV details in an iframe form to get the token, and then build up the basket, finally paying using that token without the need to prompt the user for any further CC details and it will/can do CVV checks.

Does that clear things up?

@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

I'll update this page to explain the different ways the options can be added:

https://omnipay.thephpleague.com/api/authorizing/

@vekien
Copy link

vekien commented Sep 6, 2017

I see, card is always required as the contact name/info + billing/shipping is used. I tested omitting the card details and this seems to work now, eg:

// $card = CreditCard object
$card
    ->setNumber(null)
    ->setCvv(null)
    ->setExpiryMonth(null)
    ->setExpiryYear(null);
    
// perform payment request
$request = $gateway->purchase([
    'amount' => $paymentData->amount,
    'currency' => strtoupper($paymentData->currency),
    'description' => $paymentData->description,
    'card' => $card,
    'cardReference' => $savedCard->getReference(),
    'transactionId' => $transactionId,
]);

This worked fine, so that is fantastic. I think I am going to have to clean up my code to handle whether the request is a "new card" or a "saved card" and using $request->setAmount() and similar functions will be cleaner to handle this condition, so thank you for informing me of those methods!

Is this token persistent (until deleted?) using this method:

  • purchase([ 'cardReference' => '{X}' ]) ?

There is mention of a cardReference/token being deleted after the purchase completed.

judgej added a commit to judgej/omnipay that referenced this issue Sep 6, 2017
This helps clarify some issues that have been raised in this discussion:
thephpleague/omnipay-sagepay#89 (comment)
@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

You don't need to do the set null stuff on the card, as all those parameters start out as null if you don't set them at all.

If you pass in a cardReference then it will be persistent. If you pass in the same saved card as a token then it will be treated as single-use and will NOT persist on the gateway. So if you know you will only be using it once as a part of your UX flow, then pass it in as token. If you know you want to reuse it, then pass it in as a cardReference.

Either way, you cannot get a list of tokens from the gateway, so you are 100% responsible for making sure you delete them when no longer needed (explicitly or by using as a token). They will be automatically deleted when the card expires too.

You can explicitly tell the gateway whether to keep the card or not using the storeToken parameter, but I would recommend not doing that. Just use token or cardReference and this driver will take care of making sure the token is deleted for you.

@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

I would like to get this PR release ASAP, so any feedback and details of how well it runs in your application would be appreciated to help get that moving. Note that this full cardReference support is only in PR #90, from this branch: https://github.com/academe/omnipay-sagepay/tree/issue89

@vekien
Copy link

vekien commented Sep 6, 2017

I tested your example and it is working fine, I just need to clean up my code a bit. I'm going to read over the documentation readme now and let you know if anything is confusing to me (someone who isn't literate in all the Payment Processing terminology and process flow so not sure how much it will help).

At first glance under "Generating a Token or CardReference"

  • $request->CreateToken() - transaction option to generate a token with a transaction.

I couldn't find this method, should it be camelCase ? I see a setCreateToken in the class. Maybe a magic method or typo?

I will feedback with anything else.

@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

A typo. Thanks.

One thing I am very much aware of, is that it must really be read with the Sage Pay documentation to hand. To pull in all the details that are specific to that gateway (e.g. the values that some parameters accept) would make the README extremely long. But maybe that's what it needs...?

All good stuff - your feedback is valulable. Bad or unclear documentation is bad or unclear, whether you know the lingo or not. It can't tell you everything, but I hope to get it to the level where it makes sense to newbies and experienced equally, and references to external documentation where necessary.

@vekien
Copy link

vekien commented Sep 6, 2017

In the example code it shows creating a gateway in this style:

  • create('SagePay\Direct')

However you can also do:

  • create('SagePay_Direct')

Does this matter? Or is it handled by Omnipay repo and not the adapter.

@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

Both underscore and backslash are supported:

https://github.com/thephpleague/omnipay-common/blob/master/src/Common/Helper.php#L135

I think the _ came from the early PSR-0 autoloaders before namespaces were a thing.

@vekien
Copy link

vekien commented Sep 6, 2017

Super minor typo: $cardReference = $response->getCardReference();; double ;;

@vekien
Copy link

vekien commented Sep 6, 2017

I can confirm everything is working as expected for me. In my setup, Omnipay sits on a REST API, details are passed with the param savecard, if set it will call:

$request = $gateway->createCard([
    'card' => $card,
    'name' => 'test',
    'currency' => $paymentData->currency,
]);

$response = $request->send();
$cardReference = $response->getCardReference();

And then save the $cardReference returned, I have used the card reference for subsequent payments successfully.

I have a small question regarding this, does new CreditCard([]) confirm the authenticity of the card or would only performing a purchase request do this? (thus letting Sagepay authenticate the card is real). I know CreditCard has some validation for checking types, country and card number, but I wonder how far it goes.

My thought is, when in my control flow would I save the token on my end to allow reference later on, before purchase or after purchase.

@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

I don't believe createCard does any verification on the card. It may do some simple validity checks such as a Lunn check and future expiry date. But so far as contacting the account is concerned, I don't think it does, but best check out the Sage Pay Token Billing documentation.

Yes, you would save the token for later use. It will remain available for later use until it is either used (without saving) or you explicitly delete it, or its expiry date is reached. It is not not a temporary token that may last for only a couple of minutes, as used on AJAX-based gateways. So you could capture the card details on Monday, and use it to take a payment on Tuesday.

One thing about your REST API, if you are using Sage Pay Direct, then the CC details will be passing through your application, so be aware you need to be PCI checked. That is very important to be aware of. If using Sage Pay Server, then the PCI checks are not as rigorous, since the CC details do not do near your application. Almost nobody should be using Sage Pay Direct for transactions and creating cards.

@vekien
Copy link

vekien commented Sep 6, 2017

When you say "It will remain available for later use until it is either used (without saving) or you explicitly delete it", do I need to save the token each time I request do a request? or can the same token be used, for example:

$cardReference = $response->getCardReference();

$request = $gateway->purchase([ 'cardReference' => $cardReference ]);
$request = $gateway->purchase([ 'cardReference' => $cardReference ]);
$request = $gateway->purchase([ 'cardReference' => $cardReference ]);
...

or would this not work? The "Used - Without Saving" seems a bit confusing.

@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

So long as you use it as a cardReference as you have shown, then this driver will make sure it is saved. There are other fields set in the background to tell Sage Pay to save it.

If your second purchase used a 'token' => $cardReference instead, then the third one would fail because the token would be gone. I'm assuming you do a ->send() on each of your lines of code above to actually make a payment. And again, each of those could be on different days or weeks, it makes no difference how long you wait. If you don't want CVV protection, then you don't even need the user present. Scary, eh? Look after those tokens well.

@vekien
Copy link

vekien commented Sep 6, 2017

Thanks for all the hard work, I will talk with my manager to make sure all PCI stuff is dealt with, I'm just integrating the interface since in the past we've always used an SDK (paypal sdk, stripe sdk, sagepay sdk...) and it becomes frustrating when trying to maintain a core ecommerce platform.

Everything for my needs is working great and you have cleared up a lot of knowledge for me, I am super grateful. Right now Ive got the PR checked out on testing. Will switch back to master once you merge.

@judgej
Copy link
Member Author

judgej commented Sep 6, 2017

Cool. Thanks for your help too. Much of what we have here will apply to most other Omnipay drivers. There are at least a hundred of them once you start looking.

PCI be very careful about. It may be really easy to bypass it and just use a gateway that appears to work. But if caught, especially after a data breach, it can bring a company down, especially with new laws coming up.

judgej added a commit to academe/omnipay-sagepay that referenced this issue Sep 12, 2017
judgej added a commit to academe/omnipay-sagepay that referenced this issue Sep 12, 2017
Minor refactoring only done with tests written first.
@judgej
Copy link
Member Author

judgej commented May 8, 2018

This got merged a while ago.

@judgej judgej closed this as completed May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants