From 0b0de8e22211d61fcfcb48a99e6a95cd85c8e340 Mon Sep 17 00:00:00 2001 From: Noel Light-Hilary Date: Wed, 23 Aug 2017 13:47:26 +0100 Subject: [PATCH 1/2] GP-685 - Fix handling of Error XML responses, correctly pulling through Worldpay's error message & code instead of 'PENDING'. Add support for 'username' property, which is optional and distinct from merchant code. Test some more realistic error cases, including the non-XML response when a request is unauthenticated. --- .../WorldpayCGHosted/Message/Notification.php | 6 +-- .../Message/PurchaseRequest.php | 21 ++++++++ .../Message/RedirectResponse.php | 2 +- .../WorldpayCGHosted/Message/Response.php | 48 +++++++++++------ .../Message/ResponseTrait.php | 31 +++++++---- .../Message/NotificationTest.php | 11 ---- .../WorldpayCGHosted/Message/ResponseTest.php | 54 +++++++++++++++++-- .../Mock/PurchaseErrorDuplicateOrder.txt | 11 ++++ ...haseError.txt => PurchaseErrorGeneric.txt} | 2 +- .../Mock/PurchaseErrorSecurityViolation.txt | 11 ++++ .../Mock/PurchaseUnauthenticated.txt | 28 ++++++++++ 11 files changed, 180 insertions(+), 45 deletions(-) create mode 100755 tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorDuplicateOrder.txt rename tests/Omnipay/WorldpayCGHosted/Mock/{PurchaseError.txt => PurchaseErrorGeneric.txt} (78%) create mode 100755 tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorSecurityViolation.txt create mode 100755 tests/Omnipay/WorldpayCGHosted/Mock/PurchaseUnauthenticated.txt diff --git a/src/Omnipay/WorldpayCGHosted/Message/Notification.php b/src/Omnipay/WorldpayCGHosted/Message/Notification.php index 7eb4d0a..9faecd3 100644 --- a/src/Omnipay/WorldpayCGHosted/Message/Notification.php +++ b/src/Omnipay/WorldpayCGHosted/Message/Notification.php @@ -44,7 +44,7 @@ public function __construct($data, $notificationOriginIp) } $document = simplexml_import_dom($responseDom->documentElement); - $this->data = $document->notify->orderStatusEvent; + $this->data = $document->notify; } /** @@ -62,7 +62,7 @@ public function getStatus() return null; } - return $this->data->payment->lastEvent->__toString(); + return $this->getOrder()->payment->lastEvent->__toString(); } /** @@ -70,7 +70,7 @@ public function getStatus() */ public function hasStatus() { - return !empty($this->data->payment->lastEvent); + return !empty($this->getOrder()->payment->lastEvent); } /** diff --git a/src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php b/src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php index 919684f..c249850 100755 --- a/src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php +++ b/src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php @@ -81,6 +81,27 @@ public function setMerchant($value) return $this->setParameter('merchant', $value); } + /** + * Get the separate username if configured (more secure approach for basic auth) or fallback to merchant if not + * + * @return string + */ + public function getUsername() + { + return $this->parameters->get('username', $this->getParameter('merchant')); + } + + /** + * Set basic auth username + * + * @param string $value + * @return AbstractRequest + */ + public function setUsername($value) + { + return $this->setParameter('username', $value); + } + /** * Get pa response * diff --git a/src/Omnipay/WorldpayCGHosted/Message/RedirectResponse.php b/src/Omnipay/WorldpayCGHosted/Message/RedirectResponse.php index 8241175..192f421 100755 --- a/src/Omnipay/WorldpayCGHosted/Message/RedirectResponse.php +++ b/src/Omnipay/WorldpayCGHosted/Message/RedirectResponse.php @@ -38,7 +38,7 @@ public function getRedirectMethod() */ public function getRedirectUrl() { - $url = $this->data->reference->__toString(); + $url = $this->getOrder()->reference->__toString(); // Custom result URLs available: http://bit.ly/2uRpuX0 if (!empty($this->successUrl)) { diff --git a/src/Omnipay/WorldpayCGHosted/Message/Response.php b/src/Omnipay/WorldpayCGHosted/Message/Response.php index 40732e1..52491bf 100755 --- a/src/Omnipay/WorldpayCGHosted/Message/Response.php +++ b/src/Omnipay/WorldpayCGHosted/Message/Response.php @@ -14,7 +14,6 @@ class Response extends AbstractResponse { use ResponseTrait; - /** @noinspection PhpMissingParentConstructorInspection */ /** * @param RequestInterface $request Request * @param string $data Data @@ -25,15 +24,21 @@ public function __construct(RequestInterface $request, $data) $this->request = $request; if (empty($data)) { - throw new InvalidResponseException(); + throw new InvalidResponseException('Empty data received'); } $responseDom = new DOMDocument; - $responseDom->loadXML($data); + if (!@$responseDom->loadXML($data)) { + throw new InvalidResponseException('Non-XML notification response received'); + } - $this->data = simplexml_import_dom( - $responseDom->documentElement->firstChild->firstChild + $this->data = @simplexml_import_dom( + $responseDom->documentElement->firstChild // or ); + + if (empty($this->data)) { + throw new InvalidResponseException('Could not import response XML: ' . $data); + } } /** @@ -91,27 +96,38 @@ public function getMessage() 97 => 'SECURITY BREACH' ]; - $message = 'PENDING'; - if (isset($this->data->error)) { - $message = 'ERROR: ' . $this->data->error; + return 'ERROR: ' . $this->data->error; // Cast to string to get CDATA content } - if (isset($this->data->payment->ISO8583ReturnCode)) { - $returnCode = $this->data->payment->ISO8583ReturnCode->attributes(); + $payment = $this->getOrder()->payment; + if (isset($payment->ISO8583ReturnCode)) { + $returnCode = $payment->ISO8583ReturnCode->attributes(); foreach ($returnCode as $name => $value) { if ($name == 'code') { - $message = $codes[intval($value)]; + return $codes[intval($value)]; } } } if ($this->isSuccessful()) { - $message = $codes[0]; + return $codes[0]; + } + + return 'PENDING'; + } + + /** + * @return string|null + */ + public function getErrorCode() + { + if (!isset($this->data->error) || empty($this->data->error->attributes()['code'])) { + return null; } - return $message; + return (string) $this->data->error->attributes()['code']; } /** @@ -121,7 +137,7 @@ public function getMessage() */ public function isRedirect() { - return (isset($this->data->reference)); + return (isset($this->getOrder()->reference)); } /** @@ -131,11 +147,11 @@ public function isRedirect() */ public function getTransactionReference() { - if (empty($this->data->reference)) { + if (empty($this->getOrder()->reference)) { return null; } - $attributes = $this->data->reference->attributes(); + $attributes = $this->getOrder()->reference->attributes(); if (isset($attributes['id'])) { return $attributes['id']; diff --git a/src/Omnipay/WorldpayCGHosted/Message/ResponseTrait.php b/src/Omnipay/WorldpayCGHosted/Message/ResponseTrait.php index 006b69f..b848ebc 100644 --- a/src/Omnipay/WorldpayCGHosted/Message/ResponseTrait.php +++ b/src/Omnipay/WorldpayCGHosted/Message/ResponseTrait.php @@ -5,8 +5,6 @@ /** * Encapsulates response-like behaviour shared between actual Worldpay response objects and notifications, which * actually come in Worldpay-initiated requests. - * - * @method \SimpleXMLElement|null getData() */ trait ResponseTrait { @@ -28,11 +26,11 @@ trait ResponseTrait */ public function getTransactionId() { - if (empty($this->getData())) { + if (empty($this->getOrder())) { return null; } - $attributes = $this->getData()->attributes(); + $attributes = $this->getOrder()->attributes(); if (isset($attributes['orderCode'])) { return $attributes['orderCode']; @@ -48,12 +46,12 @@ public function getTransactionId() */ public function isSuccessful() { - if (!isset($this->getData()->payment->lastEvent)) { + if (!isset($this->getOrder()->payment->lastEvent)) { return false; } return in_array( - strtoupper($this->getData()->payment->lastEvent), + strtoupper($this->getOrder()->payment->lastEvent), [ self::$PAYMENT_STATUS_AUTHORISED, self::$PAYMENT_STATUS_CAPTURED, @@ -70,12 +68,12 @@ public function isSuccessful() */ public function isPending() { - if (!isset($this->getData()->payment->lastEvent)) { + if (!isset($this->getOrder()->payment->lastEvent)) { return false; } return in_array( - strtoupper($this->getData()->payment->lastEvent), + strtoupper($this->getOrder()->payment->lastEvent), [ self::$PAYMENT_STATUS_SENT_FOR_AUTHORISATION, ], @@ -90,16 +88,29 @@ public function isPending() */ public function isCancelled() { - if (!isset($this->getData()->payment->lastEvent)) { + if (!isset($this->getOrder()->payment->lastEvent)) { return false; } return in_array( - strtoupper($this->getData()->payment->lastEvent), + strtoupper($this->getOrder()->payment->lastEvent), [ self::$PAYMENT_STATUS_CANCELLED, ], true ); } + + public function getOrder() + { + if (isset($this->data->orderStatusEvent)) { + return $this->data->orderStatusEvent; // Notifications + } + + if (isset($this->data->orderStatus)) { + return $this->data->orderStatus; // Order responses + } + + return null; + } } diff --git a/tests/Omnipay/WorldpayCGHosted/Message/NotificationTest.php b/tests/Omnipay/WorldpayCGHosted/Message/NotificationTest.php index 31bbf50..471dc30 100644 --- a/tests/Omnipay/WorldpayCGHosted/Message/NotificationTest.php +++ b/tests/Omnipay/WorldpayCGHosted/Message/NotificationTest.php @@ -18,7 +18,6 @@ public function testAuthorisedValid() $http->getBody(), self::ORIGIN_IP_VALID ); - $notification->getData(); $this->assertTrue($notification->isValid()); $this->assertTrue($notification->isAuthorised()); @@ -39,7 +38,6 @@ public function testSentForAuthorisationValid() $http->getBody(), self::ORIGIN_IP_VALID ); - $notification->getData(); $this->assertTrue($notification->isValid()); $this->assertFalse($notification->isAuthorised()); @@ -60,7 +58,6 @@ public function testAuthorisedFromBadIp() $http->getBody(), self::ORIGIN_IP_BAD ); - $notification->getData(); $this->assertFalse($notification->isValid()); $this->assertFalse($notification->isAuthorised()); @@ -81,7 +78,6 @@ public function testAuthorisedFromInvalidIp() $http->getBody(), 'not-a-real-ip' ); - $notification->getData(); $this->assertFalse($notification->isValid()); $this->assertFalse($notification->isAuthorised()); @@ -102,7 +98,6 @@ public function testAuthorisedFromMissingIp() $http->getBody(), '' // no origin IP ); - $notification->getData(); $this->assertFalse($notification->isValid()); $this->assertFalse($notification->isAuthorised()); @@ -126,7 +121,6 @@ public function testAuthorisedMissingOrderCode() $http->getBody(), self::ORIGIN_IP_VALID ); - $notification->getData(); $this->assertTrue($notification->isValid()); $this->assertTrue($notification->isAuthorised()); @@ -150,7 +144,6 @@ public function testCaptured() $http->getBody(), self::ORIGIN_IP_VALID ); - $notification->getData(); $this->assertTrue($notification->isValid()); $this->assertTrue($notification->isAuthorised()); @@ -171,7 +164,6 @@ public function testRefused() $http->getBody(), self::ORIGIN_IP_VALID ); - $notification->getData(); $this->assertTrue($notification->isValid()); $this->assertFalse($notification->isAuthorised()); @@ -192,7 +184,6 @@ public function testCancelled() $http->getBody(), self::ORIGIN_IP_VALID ); - $notification->getData(); $this->assertTrue($notification->isValid()); $this->assertFalse($notification->isAuthorised()); @@ -213,7 +204,6 @@ public function testRefundRequest() $http->getBody(), self::ORIGIN_IP_VALID ); - $notification->getData(); $this->assertTrue($notification->isValid()); $this->assertFalse($notification->isAuthorised()); @@ -246,7 +236,6 @@ public function testUnexpectedXmlBody() $http->getBody(), self::ORIGIN_IP_VALID ); - $notification->getData(); $this->assertFalse($notification->isValid()); $this->assertFalse($notification->isAuthorised()); diff --git a/tests/Omnipay/WorldpayCGHosted/Message/ResponseTest.php b/tests/Omnipay/WorldpayCGHosted/Message/ResponseTest.php index 87c19e5..d2df2c0 100755 --- a/tests/Omnipay/WorldpayCGHosted/Message/ResponseTest.php +++ b/tests/Omnipay/WorldpayCGHosted/Message/ResponseTest.php @@ -9,6 +9,7 @@ class ResponseTest extends TestCase { /** * @expectedException \Omnipay\Common\Exception\InvalidResponseException + * @expectedExceptionMessage Empty data received */ public function testConstructEmpty() { @@ -61,9 +62,9 @@ public function testPurchaseFailure() $this->assertSame('CARD EXPIRED', $response->getMessage()); } - public function testPurchaseError() + public function testPurchaseErrorGeneric() { - $httpResponse = $this->getMockHttpResponse('PurchaseError.txt'); + $httpResponse = $this->getMockHttpResponse('PurchaseErrorGeneric.txt'); $response = new Response( $this->getMockRequest(), @@ -72,7 +73,54 @@ public function testPurchaseError() $this->assertFalse($response->isSuccessful()); $this->assertFalse($response->isRedirect()); - $this->assertEquals('T0211234', $response->getTransactionId()); $this->assertSame('ERROR: Nasty internal error!', $response->getMessage()); + $this->assertNull($response->getErrorCode()); + } + + public function testPurchaseErrorDuplicateOrder() + { + $httpResponse = $this->getMockHttpResponse('PurchaseErrorDuplicateOrder.txt'); + + $response = new Response( + $this->getMockRequest(), + $httpResponse->getBody() + ); + + $this->assertFalse($response->isSuccessful()); + $this->assertFalse($response->isRedirect()); + $this->assertSame('ERROR: Duplicate Order', $response->getMessage()); + $this->assertSame('5', $response->getErrorCode()); + } + + /** + * You can get this e.g. if you are authenticated but your merchant code in the body is wrong. + */ + public function testPurchaseErrorSecurityFailure() + { + $httpResponse = $this->getMockHttpResponse('PurchaseErrorSecurityViolation.txt'); + + $response = new Response( + $this->getMockRequest(), + $httpResponse->getBody() + ); + + $this->assertFalse($response->isSuccessful()); + $this->assertFalse($response->isRedirect()); + $this->assertSame('ERROR: Security violation', $response->getMessage()); + $this->assertSame('4', $response->getErrorCode()); + } + + /** + * @expectedException \Omnipay\Common\Exception\InvalidResponseException + * @expectedExceptionMessage Could not import response XML: + */ + public function testPurchaseUnauthenticated() + { + $httpResponse = $this->getMockHttpResponse('PurchaseUnauthenticated.txt'); + + new Response( + $this->getMockRequest(), + $httpResponse->getBody() + ); } } diff --git a/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorDuplicateOrder.txt b/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorDuplicateOrder.txt new file mode 100755 index 0000000..1738ea6 --- /dev/null +++ b/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorDuplicateOrder.txt @@ -0,0 +1,11 @@ +HTTP/1.1 200 OK +Connection: close +Server: VPS-3.033.00 +Date: Sat, 23 Feb 2013 05:17:32 GMT +Content-type: text/xml +Content-length: 341 + + + + diff --git a/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseError.txt b/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorGeneric.txt similarity index 78% rename from tests/Omnipay/WorldpayCGHosted/Mock/PurchaseError.txt rename to tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorGeneric.txt index fd0dd47..fa1641f 100755 --- a/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseError.txt +++ b/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorGeneric.txt @@ -8,4 +8,4 @@ Content-length: 376 -Nasty internal error! +Nasty internal error! diff --git a/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorSecurityViolation.txt b/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorSecurityViolation.txt new file mode 100755 index 0000000..93fa6c2 --- /dev/null +++ b/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseErrorSecurityViolation.txt @@ -0,0 +1,11 @@ +HTTP/1.1 200 OK +Connection: close +Server: VPS-3.033.00 +Date: Sat, 23 Feb 2013 05:17:32 GMT +Content-type: text/xml +Content-length: 344 + + + + diff --git a/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseUnauthenticated.txt b/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseUnauthenticated.txt new file mode 100755 index 0000000..3f70a89 --- /dev/null +++ b/tests/Omnipay/WorldpayCGHosted/Mock/PurchaseUnauthenticated.txt @@ -0,0 +1,28 @@ +HTTP/1.1 401 Unauthorized +Connection: close +Server: VPS-3.033.00 +Date: Sat, 23 Feb 2013 05:17:32 GMT +Content-type: text/html +Content-length: 767 + + + + + + 401: Requires Authentication + + + + +
+ Requires Authentication +
Status: 401
+

+ This request requires HTTP authentication. + +
+

+
+ + From 8a8592839bbd1319298455e00a64d4ac8e5b0ee0 Mon Sep 17 00:00:00 2001 From: Noel Light-Hilary Date: Wed, 23 Aug 2017 14:18:22 +0100 Subject: [PATCH 2/2] GP-685 - Use new username for basic auth where set --- src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php b/src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php index c249850..d3b4193 100755 --- a/src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php +++ b/src/Omnipay/WorldpayCGHosted/Message/PurchaseRequest.php @@ -298,7 +298,7 @@ public function sendData($data) $document->appendChild($node); $authorisation = base64_encode( - $this->getMerchant() . ':' . $this->getPassword() + $this->getUsername() . ':' . $this->getPassword() ); $headers = [