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 creation does not work with Server Gateway #105

Closed
drn5910 opened this issue Sep 6, 2018 · 11 comments
Closed

Token creation does not work with Server Gateway #105

drn5910 opened this issue Sep 6, 2018 · 11 comments
Assignees
Labels

Comments

@drn5910
Copy link

drn5910 commented Sep 6, 2018

Hello,

When using the Server gateway and requesting that a token is created, it is ignored and SagePay isn't told to create a token for the card details the end user will enter.

I've spent the last few hours trying to work out why I couldn't get token generation working, thinking I was missing something and trying to use params vs the setCreateToken method. I've now dived deeper into the code and the getData call on ServerAuthorizeRequest never adds the CreateToken data.

In comparison, the DirectAuthorizeRequest adds the CreateToken parameter when it calls getCardData, and this is the only place that parameter is used. The getCardData isn't called for a Server gateway request.

I added the $data['CreateToken'] = $this->getCreateToken(); into ServerAuthorizeRequest@getData and this does cause a token to be created and sent back to me, however the isValid then fails as the VPS signatures don't match... I'm not sure why.

This issue is encountered in both 2.x and 3.x

Thanks

@judgej
Copy link
Member

judgej commented Sep 6, 2018

Thanks, I'll take a look and come back with any questions.

When this driver was first written, Sage Pay did not support tokens, and repeat transactions were run against past authorisations. To do that, three pieces of information were used from those past transactions, and they were provided by Omnipay as a JSON string. I think if we can fully move over to the newer (but still quite old) token system then we could simplify repeat payments a lot.

@drn5910
Copy link
Author

drn5910 commented Sep 6, 2018

Hi Jason,

Thanks for the rapid response!

I've done a bit more investigation to see why the VPSSignature doesn't match, and it turns out SagePay aren't using the token as part of their VPS signature. I commented out line 58 in ServerNotifyTrait (which is $this->getToken(), and the VPS Signature now matches the one they send.

I'm unsure if that is used in other signatures at all (e.g. when using Direct), but if it isn't then perhaps that line could be safely removed?

As per the SagePay docs (https://www.sagepay.co.uk/file/1161/download-document/serverprotocoland - Page 66) it doesn't seem like the token is used in the signature. However the docs here (https://www.sagepay.co.uk/file/1171/download-document/sagepaytokensystemprotocolandintegrationguidelinev3.0_0.pdf - page 14) imply the token is used in this method, but I'm unsure if that relates to anything this library handles currently?

Is there a way we could get a small patch out to get tokens working for the server gateway?

I'm happy to submit a PR, but as a I've only been briefly using this new version (we had token support prior to the update by creating our own Gateway), I'm unsure if the ServerAuthorizeRequest@getData is the best place to add the CreateToken parameter or whether it needs to be higher up the chain so other items benefit from it...

@judgej
Copy link
Member

judgej commented Sep 6, 2018

What I normally do in situations like this, is fork it, publish the fork with the changes in, and run off that fork in whatever environment. That way you get your patch to run with, and everyone gets visibility of the solution that works for you, and it can be turned into a PR. It's just a temporary thing, but we can all try it out in different environments (Server, Direct, and even Pi).

@drn5910
Copy link
Author

drn5910 commented Sep 6, 2018

Hi Jason,

Thanks, in the meantime I've just created a new TokenGateway that provides overrides for the purchase and acceptNotification so that I can override the getData and buildSignature with amendments respectively (this seemed simplest to do as it's essentially what we had done when token's weren't support in this library).

@judgej
Copy link
Member

judgej commented Sep 7, 2018

Just tried createCard() with both Server and Direct gateways. Both of those worked for me - they created a cardReference without performing any authorisation. I'll try creating a reference while authorising next.

@judgej
Copy link
Member

judgej commented Sep 7, 2018

I've created branch issue105 to track some changes to fix this:

https://github.com/thephpleague/omnipay-sagepay/tree/issue105

For both Server and Direct, you can now set createCard or createToken during an authorise or purchase, and that will return a getCardReference() on the result. For Direct the result is in the synchronous response. For Server this is provided by the asynchronous notification handler.

The cardReference (looking like this {FCCF1047-05F7-7FA9-6D75-7294F0CFA9A4}) can then be supplied with an authorize or purchase. For Direct it will authorize immediately, but may still require a currenct CVV to be supplied in the credit card object. This will depend on security settings in the account. The user would be prompted for their CVV and that would be used, but do remember this is a PCI nightmare.

For Server, using the cardReference is more secure. The user will be redirected to the Sage Pay site as normal, but instead of entering full credit card details, the user is prompted only for their CVV. Then a normal Server authorisation process takes place.

When creating a card token, remember to save the expiry date on your site with the card too. That way you will know when it has expired before attempting to use it, and the user can be asked for their updated card details to get a new token/card reference.

I still want to update the documentation on this, and the code needs a major refactor IMO (it has grown over the years), but if you want to give this feature branch a try and let me know how it goes, that would be immensely helpful. This is just on the 3.x branch.

@judgej
Copy link
Member

judgej commented Sep 9, 2018

@drn5910 the question of the Token being part of the signature is not so clear-cut. Sometimes it is, and sometimes it is not.

When fetching a card token standalone (createCard()) then it is included. When fetching a token with an athorisation then it is not included.

I have Sage Pay wanted to use it for the newer endpoints, but wanted to avoid BC breaks for the older endpoints. A silly idea IMO, but I'll make the fixes to incorporate this.

judgej added a commit that referenced this issue Sep 9, 2018
@drn5910
Copy link
Author

drn5910 commented Sep 12, 2018

Thanks, that looks like it handles the incorrect signature that was being generated. However the token generation doesn't work still when doing a purchase request via Server Gateway. See example code below:

//
// This is the initial purchase request
//
// Server gateway creation
$gateway = Omnipay::create('SagePay\Server');
$gateway->setVendor('vendor');
$gateway->setTestMode(true);

// Dummy card details for when we get to SagePay
$card = new CreditCard;
$card->setName('Firstname Lastname');
$card->setAddress1('Address');
$card->setCity('City');
$card->setPostcode('Postcode');
$card->setCountry('GB');

// Required parameters
$params = [
    'amount'        => 'amount here',
    'card'          => $card,
    'currency'      => 'GBP',
    'description'   => 'transaction description',
    'transactionId' => 'unique ID for transaction',
    'notifyUrl'     => 'URL for notification from SagePay',
];

$gateway_request = $gateway->purchase($params);
// This has no affect later on, as it's not included in the data sent to SagePay.
$gateway_request->setCreateToken(true);
// Set low profile for minimal IFRAME integration
$gateway_request->setProfile(ServerPurchaseRequest::PROFILE_LOW);
$response = $gateway_request->send();

if ($response->isRedirect()) {
    // handle showing the iframe
}

//
// This is for the notify URL handler
//

$gateway = Omnipay::create('SagePay\Server');
$gateway->setVendor('vendor');
$gateway->setTestMode(true);

$gateway_request = $gateway->acceptNotification();

// Retrieve transaction details into $transaction

$gateway_request->setTransactionReference(json_encode([
    // The stored parts from previous, to recreate the transaction reference
    'SecurityKey'  => $transaction->security_key,
    'VPSTxId'      => $transaction->VPSTxId,
    'VendorTxCode' => $transaction->transaction_id,
]));

$response = $gateway_request->send();

if ( ! $response->isSuccessful()) {
    // Handle failure
    return;
}

// Handle success but the $response->getToken() is empty (and no token was provided by SagePay in their POST).

If using the code above, the initial CreateToken isn't sent to SagePay as there's nothing in the functions for the ServerPurchaseRequest@getData to set the token request. I fix it on myside by adding a TokenPurchaseRequest to override the ServerPurchaseRequest which just contains this function:

    public function getData()
    {
        $data = parent::getData();

        $data['CreateToken'] = 1;
        $data['StoreToken'] = 1;

        return $data;
    }

Of course, the CreateToken etc. should be set based on whether it was actually needed, but my requirements always end up with a token being needed as it's recurring billing.

@drn5910
Copy link
Author

drn5910 commented Sep 12, 2018

Sorry - I hadn't seen the latest work on that branch you've made, and I can see that you've resolved the above (from just looking at code, haven't tested) by adding the missing CreateToken parts.

@judgej
Copy link
Member

judgej commented Sep 12, 2018

Yeah, it's turning into a monster PR now, with quite a lot of refactoring. I'm trying not to introduce any BC breaks though. Still updating the docs with examples too (e.g. Direct payments and 3D-Secure handling).

Just tried what's pushed to issue105 now, making a payment and setting createCard. The notification handler got this raw data from the gateway:

Array
(
    [VPSProtocol] => 3.00
    [TxType] => PAYMENT
    [VendorTxCode] => phpne-demo-86801794
    [VPSTxId] => {4D9CAB01-FE91-F953-XXXX-35D8XXXXXXXX}
    [Status] => OK
    [StatusDetail] => 0000 : The Authorisation was Successful.
    [TxAuthNo] => 18809215
    [AVSCV2] => SECURITY CODE MATCH ONLY
    [AddressResult] => NOTMATCHED
    [PostCodeResult] => NOTMATCHED
    [CV2Result] => MATCHED
    [GiftAid] => 0
    [3DSecureStatus] => NOTCHECKED
    [CardType] => VISA
    [Last4Digits] => 5559
    [VPSSignature] => DDF2C7507911486E640C76AF4305A53E
    [DeclineCode] => 00
    [ExpiryDate] => 1218
    [Token] => {08C70AE6-AC9F-23B4-29F0-C9EBB93B7977}
    [BankAuthCode] => 999777
)

Notice the Token is in there.

It is confusing that there are two ways to create offline repeat payments, both still active. The first is to create a Direct repeatPayment() based on a previous authorised payment. The second way is to create a Direct payment() using a saved cardReference. The cardReference can also be used in the Server API, which the repeat payment cannot.

The Direct cardReference method may still need a CVV to be supplied by the end user, so it is not exactly equivalent to a repeatPayment() which is 100% user-not-present.

It is easy to leave the createCard option on for all payments. Don't do that - it will create a new token for every single payment, ever, and they will all remain on the Sage Pay account until the cards expire. It really must be linked to a user choice, and only set if the user expressly asks for it. I think Sage Pay offers an option to put a "remember my card" checkbox on the Direct card entry form to make sure the user agrees with this (or I may be mixing it up with Adyen - I'll check).

@judgej judgej self-assigned this Sep 25, 2018
@judgej
Copy link
Member

judgej commented Sep 30, 2018

Release 3.1.0

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

No branches or pull requests

2 participants