academe / sagepay Goto Github PK

Since Omnipay - https://github.com/omnipay/ - now supports third-party gateways, a wrapper should allow this gateway to be used as an Omnipay plugin. The advantage would be to take advantage of functionality specific to this library (even though much is yet to be completed).

Whether the wrapper is a separate library or built-in, I'm not sure. If it is compact, then building it in makes sense to me. In a similar way, a facade and provider class would make it easy to integrate with Laravel (whether you would want to do this, or go through Omnipay, is another question). Flexibility all round is a good thing, so long as it doesn't get bloated.

This injection example type-hints an abstract:

public function setTransactionModel(Model\TransactionAbstract $tx_model)

It kind of made sense to me, because extended transactions will derive from this abstract.

However, when creating models based on third-party classes, the ancestry (being a linear line, before PHP 5.5 traits at least) may not come from the abstract declared here. However, if the abstract implements an interface, then a third party model that does not extend any of the models here, can still implement the interface and so be valid for setTransactionModel().

It has taken me a while to realise the subtlety of this, as the abstract felt a lot more "useful" than an interface, but it is clear to me now :-)

And active record normally starts with a data storage layer, and models inherit from that layer and add their own fields and business logic.

The Transaction object here starts with the data properties and business logic, then allows you to use one of several data storage methods that inherit from the model.

That is a kind of up-side-down active record, and could make integration a little cumbersome.

Instead, we should use a data mapper. The models already are pretty much dumb data with some business rules. We just need to take the storage out of the line of inheritance and keep it as a separate mapper. Each storage type would then be an adapter for the mapper. I don't think it is a complicated change, but will make more sense for people familiar with more standard patterns, and will be easier to test (the model and the storage can be tested independently, while at the moment the storage can only be tested with a model).

The README gives us somewhere to put a quick-start, but there will be a tonne of documentation to write for this. There are so many different ways in which it can be used, and options available, that a good set of use-case working code would be good.

I setup the SagePay Server by following the example in readme.md, but Sage returned with the error saying that the VendorTxCode was not set.

To resolve this, I've added:
$server->setField('VendorTxCode', $storage->makeVendorTxCode());

just before
$server->sendRegistratrion();

HI. Thanks ever so much for publishing this project, which looks like it's going to save me a stack of work :)

I'm having a problem loading the correct customer details and am wondering what I'm doing wrong. My code is as follows:

// set the billing details
$billing_addr = new Academe\SagePay\Model\Address();
$billing_addr->setField('Surname', $data['c_surname']);
$billing_addr->setField('Firstnames', $data['c_firstname']);
$billing_addr->setField('Address1', $data['c_add1']);
$billing_addr->setField('Address2', $data['c_add2']);
$billing_addr->setField('City', $data['c_town']);
$billing_addr->setField('PostCode', $data['c_postcode']);
$billing_addr->setField('Country', 'GB');
$server->setBillingAddress($billing_addr);
// set the delivery details
$delivery_addr = new Academe\SagePay\Model\Address();
$delivery_addr->setField('Surname', $data['c_surname']);
$delivery_addr->setField('Firstnames', $data['c_firstname']);
$delivery_addr->setField('Address1', $data['c_add1']);
$delivery_addr->setField('Address2', $data['c_add2']);
$delivery_addr->setField('City', $data['c_town']);
$delivery_addr->setField('PostCode', $data['c_postcode']);
$delivery_addr->setField('Country', 'GB');
$server->setDeliveryAddress($delivery_addr);
// set the customer details
$customer_details = new Academe\SagePay\Model\Customer();
$customer_details->setField('CustomerEMail', $data['email']);
$server->setCustomerModel($customer_details);
// generate a unique VendorTxCode to identify the transaction
$server->save();

Both the local storage db and SagePay's systems record both the billing and delivery addresses with the data entered above for 'billing', and the email address is not recorded at all. If you have time, please could you explain what I'm doing wrong?

The following is an email we have received from SagePay regarding issues we have been having with PPRO not updating from "pending" to "OK" status. Could anyone suggest where this needs to be corrected (if indeed it is an issue within this library)?

I am writing to you in regards to the ongoing issue with pending PPRO payments.

The issue is to do with the MD5 signature when we call back to your Notification URL. Initially the callback from your server for a Status = Pending transactions is as normal, works fine no issues there, the signatures match, but when we call back the second time to change it from pending to OK, there is a problem with the MD5 signature built by your server.

We need to send another callback to your server as we wait for PPRO to contact Sage Pay to confirm the order has authorised.

For the below example we get this confirmation from PPRO about 20 minutes after the transaction goes through:

VendorTxCode=12345678,
VPSTxId={XYZ123-XYZ123-XYZ123-XYZ123-XYZ123},
Status=OK,
GiftAid=0,
VPSSignature=0123456789ABCDE

This needs to be built like:
12345678{XYZ123-XYZ123-XYZ123-XYZ123-XYZ123}OK0
VPSSignature=0123456789ABCDE

So the order for building this is:
VPSTxId
VendorTxCode
Status
GiftAid

So we need your server to reply with the correct VPSSignature for the second callback to acknowledge they change from Pending to OK in the Sage Pay system in the format shown above and this will mean that your transactions do not stay as Pending in My Sage Pay and revert to a successfully authorised transaction.

A means to validate and to transform data (into valid formats) is needed to provide a robust working environment.

Some items, such as basket descriptions, can be truncated and have invalid characters removed, and stuff will still work as expected. Other fields cannot be transformed (munged) in this way, and need to result in a failed payment request so that remedial action can be taken.

The validation actually needs to happen before anything is submitted to SagePay, as the error responses we get back are not technically useful (e.g. they don't make it easy to identify the incorrect field - "the basket format is invalid" is one cringe-worthy example than can mean anything and SagePay will halt at the first error it finds and report only that one error. If every field on my submitted address is invalid, I would want to know about all of them and not have to submit the form once for each error and slowly work through them).

Unless you request it and enable (and pay for) it on your account you don't generate 'Token' emails.

Server.php around line 240 has this line:

$this->setField('Token', $post['Token']);

which throws an error if Surcharge isn't sent.

For now I've just done

if (isset($post['Token'])) {
    $this->setField('Token', $post['Token']);
}

In a standard process, isPaymentSuccess is usually called on the notification callback URL and therefore the transaction status will have been set by SagePay. However if we test that transaction endpoint without setting the status, it returns an error: Undefined property: Academe\SagePay\Model\TransactionPdo::$Status

To fix, change line 550 in TransactionAbstract.php to:

if (isset($this->Status) && ($this->Status == 'OK' || $this->Status == 'AUTHENTICATED' || $this->Status == 'REGISTERED')) {

Not done this for a project before, so any advice on how to get started on this would be great.

Moved from Ticket #26

The earlier SagePay spec (2013-03-25) this package was coded from, did not document the discount records in the basket. This is now documented in the later version (2013-09-30). The API remains at version 3.00

The current workaround in the meantime is to extend the BasketAbstract and code your own discounts.

3D Secure v2 has been announced and will be mandatory from the end of 2020.

We have a bit of time :)

Do you have any plans to test/update the library for it?

If not - I'm happy to look into it or work with you on it. Thought I'd check before potentially duplicating work.

SagePay's (fairly basic) details:
https://www.sagepay.co.uk/support/12/36/3d-secure-explained

Most of the methods supported by the Server (or Direct or Shared) require access to a Transaction model with its relevant storage. That should be moved to the constructor so that we can guarantee that it is available, instead of keep checking whether it has been set whenever we need it.

Any apps that make use of the setter would need modifying, so this is a warning :-)

Similarly, TransactionPdo should also take a database object in its constructor, since it will not work without one.

Error 3006 gives us a clue:

http://www.sagepay.com/help/systemmessage/433

A currency that has no fractional part (e.g. Yen) must not be sent to SagePay with a fractional part - presumably even zero - or this error will result. A lookup table of formats and currencies is needed.

Proper header blocks on each method is required. Documentation is always the kind of last thing to do, when getting the library working for a project is the most urgent thing.

$server->setAmount('1,234.56', 'GBP');
echo $server->getField('Amount'); // Returns numeric 0

because of

if ( ! is_numeric($amount)) $amount = 0;

I'd reckon it should return something non-numeric at least, but I'd suggest it throws an exceptions since that number should be checked elsewhere.

This isn't a bug in the library so much as an annoyance in SagePay, but you may have some insight which helps.

When sending SurchargeXML there is a maximum length of the XML otherwise SagePay returns:

INVALID : 3175 : The Surcharge XML is too long.

I've not calculated exactly what the limit is (though it's around 800 characters) but there is enough space to set all but about three card types

$surcharge->addPercentage('VISA', $surchargeAmount)
    ->addPercentage('AMEX', $surchargeAmount)
    ->addPercentage('DELTA', $surchargeAmount)
    ->addPercentage('JCB', $surchargeAmount)
    ->addPercentage('DC', $surchargeAmount)
    ->addPercentage('MC', $surchargeAmount)
    ->addPercentage('UKE', $surchargeAmount)
    ->addPercentage('MAESTRO', $surchargeAmount)
//  ->addPercentage('MCDEB', $surchargeAmount)
//  ->addPercentage('IT', $surchargeAmount)
//  ->addPercentage('GIROPAY', $surchargeAmount)
    ->addPercentage('SOFORT', $surchargeAmount);

Additionally using a Fixed amount lets you specify all card types because "fixed" is a shorter word than "percentage".

It'd be good to create a validator to catch this.
If this is indeed an issue with SagePay I'd suggest workarounds are either:

1. Use Fixed amounts always, and calculate the percentage if need be
2. Recognise a few card types you're not going to use.

The isPaymentSuccess simply calls itself, resulting in an infinite loop

Hi
public function anyCallback() {

    // Gather the POST data.
    $post = $_POST;
    $host = Config::get('database.connections.mysql.host', 'localhost');
    $dbname = Config::get('database.connections.mysql.database', 'database');
    $dbuser = Config::get('database.connections.mysql.username', 'root');
    $dbpass = Config::get('database.connections.mysql.password', '');

    $this->storage = new Academe\SagePay\Model\TransactionPdo();
    $this->storage->setDatabase('mysql:host=' . $host . ';dbname=' . $dbname, $dbuser, $dbpass);
    $dbprefix = Config::get('database.connections.mysql.prefix', '');
    $this->storage->setTablename($dbprefix . 'sagepay_transactions');

    $this->server->setTransactionModel($this->storage);
    if ($this->server->getField('Status') == 'OK') {

the callback function is not taking $this->server->getField('Status') == 'OK'.. is there something i must consider?

Regards,
Prajwol

I don't think this is technically a bug (yet) since the library doesn't (yet) support Refunds. However I'd like it to :)

in the ServiceAbstract is a method queryData() which creates a Query String based on the $message_type, which are things like 'server-registration'.

Correct me if I'm mis-understanding something, but do the picked fields not depend on the transaction type? E.g PAYMENT or REFUND?

My thought is that the TxType should be passed into QueryData and in the Metadata\Transaction object the "source" arrays should be populated by TxTypes.

Hi, given that this class is not yet complete, does it carry out the md5 hash check using VPSSignature, please?

Omipay has a SagePay V2 driver, but not V3. It would be good (once this one is completely tidied up) to create a connector to expose it as an Omnipay driver. The driver would support all the base functionality that Omnipay provides, and then could provide extensions to the additional functionality that Omnipay does not generally support (or encourage), such as additional basket information, additional customer details, separate emails for billing and shipping.

The transport would need to be abstracted properly (it is currently stuck in ServiceAbstreact::postSagePay as a curl-based method). This would allow us to use the Omnipay transport mechanisms.

The network flow diagram here would help to understand how this would all link together:

https://github.com/judgej/omnipay-sagepay/tree/patch-1/docs

See:

https://github.com/thephpleague/omnipay

Sometimes Sage Pay does not get the initial notification response that we return, so it will send the notification again. At the moment, this library will log this as an error, so Sage Pay thinks the transaction is in error and will cancel it, but the merchant site thinks it has send a valid "OK" and has no idea Sage Pay lost that response.

The solution is to check if the notification is identical to the one stored, if the current state is not PENDING. If it is the same, then, just return OK as though it were the first time it were received, and not update anything in the database.

Don't change the directory structure however - just the autoload directive in composer.json

I've been debugging SagePay callback errors which were giving me the error Undefined index: CardType in Academe/SagePay/Server.php on line 247

I believe this is caused by the duplicated CardType in Metadata/Transaction.php (noted in comment on line 23) because the second CardType does not contain the "tamper": true, field (the first one does).

Since the key is duplicated, the first one is ignored - I'd suggest we merge the two card type lists and consider how we might separate them again in future if we need to have different data types per source (the validation will be slightly less strict, but at the moment I'm not sure how or why it would even be working for server-notification - the first one.

This will bring the parameters used for RELEASE/VOID/CANCEL/ABORT into line with the parameters defined by SagePay for AUTHORISE. More consistency will make throwing data around the APIs and the database a little easier.

This is a change to a data store field.

There will also be RelatedVPSTxId and RelatedSecurityKey to be added to the data store.

Hi i am integrating the package in laravel. Im going thru the comments and i see this:

// Create a storage model object.
// A basic PDO storage is provided, but just extend Model\Transaction and use your own.

I search transaction inside model folder but there's none? it is TransactionAbstract or is it Metadata\Transaction please help me understand.

Also how is it possible to using it with laravel eloquent?

Many thanks
Prajwol.

The current implementation of \Academe\SagePay\Model\TransactionAbstract::makeVendorTxCode is not guaranteed to be unique or unpredictable according to the PHP documentation for uniqid.

I would suggest the following function as (a) the transaction will fail if the VendorTxCode is accidentally repeated; (b) this function will not create predictable strings that a malicious user could brute force, if the VendorTxCode is used in any user input.

/**
 * This function returns a UUID.
 * Source: http://stackoverflow.com/a/15875555/1971539
 *
 * @return string A UUID following GUIDv4 spec, without braces. 36 characters long.
 */
public static function guidv4() 
{
    $data = openssl_random_pseudo_bytes(16);

    $data[6] = chr(ord($data[6]) & 0x0f | 0x40); // set version to 0100
    $data[8] = chr(ord($data[8]) & 0x3f | 0x80); // set bits 6-7 to 10

    return vsprintf('%s%s-%s-%s-%s-%s%s%s', str_split(bin2hex($data), 4));
}

However, if context is important then you could use the following function. It returns something quite similar to the existing function, but uses the full length allowed by Sagepay:

/**
 * Make a new VendorTxCode.
 * To be give the code some context, we start it with a timestamp before
 * we add on a random hex string.
 * The VendorTxCode is limited to 40 characters, so we use 12 bytes for the hex.
 * Override this method if you want a different format.
 */

public function makeVendorTxCode()
{
    $data = openssl_random_pseudo_bytes(12);

    return vsprintf('%s-%s', Array(date('Ymd-His'), bin2hex($data)));
}

I have created a pull request with the latter function. Whether you merge it or not is down to your feelings on introducing OpenSSL as a dependency.

The Registration class is going to get pretty big once SagePay Direct is added. Instead, it should be split into Direct and Server classes. We already have a Shared class for the shared services.

A Common class will be needed for the functionality that all three share.

Registration can inherit Server for backwards compatibility, but otherwise be deprecated.

The PDO transaction model has createTable() to create the database table for storage of the registered transactions. The table is create from the Transaction metadata (all fields with a "store" flag set true).

When the metadata is extended in library releases, there is no updateTable() method that can be used to bring the table into line with the current metadata. It could with one.