jenkins-bot has submitted this change and it was merged.
Change subject: Fix comm status and error checking for PaymentStatus
......................................................................
Fix comm status and error checking for PaymentStatus
Create helper functions to inspect the two different types of
responses.
Communication status should still be true if response is valid
but indicates something else is wrong. It should only be false
when the response is malformed or missing.
Bug: T90504
Change-Id: I02dc5496fd1527ffa1cb9d2e56b9e6e71fec7587
---
M astropay_gateway/astropay.adapter.php
M gateway_common/ResponseCodes.php
M tests/Adapter/Astropay/AstropayTest.php
A tests/includes/Responses/astropay/NewInvoice_notJson.testresponse
4 files changed, 137 insertions(+), 103 deletions(-)
Approvals:
Awight: Looks good to me, approved
jenkins-bot: Verified
diff --git a/astropay_gateway/astropay.adapter.php
b/astropay_gateway/astropay.adapter.php
index 742caeb..132757c 100644
--- a/astropay_gateway/astropay.adapter.php
+++ b/astropay_gateway/astropay.adapter.php
@@ -396,55 +396,6 @@
$this->staged_data['donor_id'] = substr( $hashed, 0, 20 );
}
- function parseResponseCommunicationStatus( $response ) {
- if ( $response === NULL || !isset( $response['status'] ) ) {
- return false;
- }
- return $response['status'] === '0';
- }
-
- function parseResponseErrors( $response ) {
- $logme = false;
- $errors = array();
- $code = 'internal-0000';
- $level = LogLevel::ERROR;
-
- if ( $response === NULL ) {
- $logme = 'Astropay response was not valid JSON. Full
response: ' .
- $this->transaction_response->getRawResponse();
- } else if ( !isset( $response['status'] ) ) {
- $logme = 'Astropay response does not have a status
code. Full response: ' .
- $this->transaction_response->getRawResponse();
- } else if ( $response['status'] !== '0' ) {
- $logme = "Astropay response has non-zero status
{$response['status']}. ";
- if ( isset( $response['desc'] ) ) {
- // They don't give us codes to distinguish
failure modes, so we
- // have to parse the description.
- if ( preg_match( '/invoice already used/i',
$response['desc'] ) ) {
- $code =
ResponseCodes::DUPLICATE_ORDER_ID;
- }
- $logme .= 'Error description: ' .
$response['desc'];
- } else {
- $logme .= 'Full response: ' .
$this->transaction_response->getRawResponse();
- }
- $level = LogLevel::WARNING;
- }
-
- if ( $logme ) {
- $this->logger->log( $level, $logme );
- //TODO: move translation and debug display out of this
fn
- $generic = $this->getErrorMapByCodeAndTranslate(
'internal-0000' );
- $debug = $this->getGlobal( 'DisplayDebug' );
- $errors[$code] = array(
- 'logLevel' => $level,
- 'debugInfo' => $debug,
- 'message' => $debug ? $logme : $generic
- );
- }
-
- return $errors;
- }
-
static function getCurrencies() {
$currencies = array(
'ARS', // Argentinian peso
@@ -473,12 +424,6 @@
if ( !$this->transaction_response ) {
$this->transaction_response = new
PaymentTransactionResponse();
}
- $this->transaction_response->setCommunicationStatus(
- $this->parseResponseCommunicationStatus( $response )
- );
- $this->transaction_response->setErrors(
- $this->parseResponseErrors( $response )
- );
$this->transaction_response->setData( $response );
if ( !$response ) {
throw new ResponseProcessingException(
@@ -487,39 +432,60 @@
);
}
switch( $this->getCurrentTransaction() ) {
- case 'PaymentStatus':
- if ( !$this->verifyStatusSignature( $response )
) {
- $this->logger->error( 'Bad signature in
response to PaymentStatus call.' );
- throw new ResponseProcessingException(
- 'Bad signature in response to
PaymentStatus call.',
- ResponseCodes::BAD_SIGNATURE
- );
- }
- break;
- case 'ProcessReturn':
- if ( !$this->verifyStatusSignature( $response )
) {
- $this->logger->error( 'Bad signature in
data POSTed to resultswitcher.' );
- throw new ResponseProcessingException(
- 'Bad signature in data POSTed
to resultswitcher.',
- ResponseCodes::BAD_SIGNATURE
- );
- }
- if ( isset( $response['x_document'] ) ) {
-
$this->transaction_response->setGatewayTransactionId( $response['x_document'] );
- } else {
- $this->logger->error( 'Astropay did not
post back their transaction ID in x_document' );
- throw new ResponseProcessingException(
- 'Astropay did not post back
their transaction ID in x_document',
-
ResponseCodes::MISSING_TRANSACTION_ID
- );
- }
- $status = $this->findCodeAction(
'PaymentStatus', 'result', $response['result'] );
- $this->logger->info( "Payment status $status
coming back to ResultSwitcher" );
- $this->finalizeInternalStatus( $status );
- break;
- case 'NewInvoice':
- $errors = $this->getTransactionErrors();
- if ( isset(
$errors[ResponseCodes::DUPLICATE_ORDER_ID] ) ) {
+ case 'PaymentStatus':
+ $this->processStatusResponse( $response );
+ break;
+ case 'ProcessReturn':
+ $this->processStatusResponse( $response );
+ if ( !isset( $response['x_document'] ) ) {
+ $this->logger->error( 'Astropay did not post
back their transaction ID in x_document' );
+ throw new ResponseProcessingException(
+ 'Astropay did not post back their
transaction ID in x_document',
+ ResponseCodes::MISSING_TRANSACTION_ID
+ );
+ }
+ $this->transaction_response->setGatewayTransactionId(
$response['x_document'] );
+ $status = $this->findCodeAction( 'PaymentStatus',
'result', $response['result'] );
+ $this->logger->info( "Payment status $status coming
back to ResultSwitcher" );
+ $this->finalizeInternalStatus( $status );
+ break;
+ case 'NewInvoice':
+ $this->processNewInvoiceResponse( $response );
+ if ( isset( $response['link'] ) ) {
+ $this->setTransactionResult( $response['link'],
'redirect' );
+ }
+ break;
+ }
+ }
+
+ /**
+ * Sets communication status and errors for responses to NewInvoice
+ * @param array $response
+ */
+ protected function processNewInvoiceResponse( $response ) {
+ if ( !isset( $response['status'] ) ) {
+ $this->transaction_response->setCommunicationStatus(
false );
+ $this->logger->error( 'Astropay response does not have
a status code' );
+ throw new ResponseProcessingException(
+ 'Astropay response does not have a status code',
+ ResponseCodes::MISSING_REQUIRED_DATA
+ );
+ }
+ $this->transaction_response->setCommunicationStatus( true );
+ if ( $response['status'] === '0' ) {
+ if ( !isset( $response['link'] ) ) {
+ $this->logger->error( 'Astropay NewInvoice
success has no link' );
+ throw new ResponseProcessingException(
+ 'Astropay NewInvoice success has no
link',
+ ResponseCodes::MISSING_REQUIRED_DATA
+ );
+ }
+ } else {
+ $logme = "Astropay response has non-zero status
{$response['status']}";
+ if ( isset( $response['desc'] ) ) {
+ // They don't give us codes to distinguish
failure modes, so we
+ // have to parse the description.
+ if ( preg_match( '/invoice already used/i',
$response['desc'] ) ) {
$this->logger->error( 'Order ID
collision! Starting again.' );
throw new ResponseProcessingException(
'Order ID collision! Starting
again.',
@@ -527,10 +493,49 @@
array( 'order_id' )
);
}
- if ( isset( $response['link'] ) ) {
- $this->setTransactionResult(
$response['link'], 'redirect' );
- }
- break;
+ $logme .= 'Error description: ' .
$response['desc'];
+ } else {
+ $logme .= 'Full response: ' .
$this->getTransactionRawResponse();
+ }
+ $this->logger->warning( $logme );
+ $this->transaction_response->setErrors( array(
+ 'internal-0000' => array (
+ 'message' =>
$this->getErrorMapByCodeAndTranslate( 'internal-0000' ),
+ 'debugInfo' => $logme,
+ 'logLevel' => LogLevel::WARNING
+ )
+ ) );
+ }
+ }
+
+ /**
+ * Sets communication status and errors for responses to PaymentStatus
or
+ * parameters POSTed back to ResultSwitcher
+ * @param array $response
+ */
+ protected function processStatusResponse( $response ) {
+ if ( !isset( $response['result'] ) ||
+ !isset( $response['x_amount'] ) ||
+ !isset( $response['x_invoice'] ) ||
+ !isset( $response['x_control'] ) ) {
+ $this->transaction_response->setCommunicationStatus(
false );
+ $message = 'Astropay response missing one or more
required keys. Full response: '
+ . print_r( $response, true );
+ $this->logger->error( $message );
+ throw new ResponseProcessingException( $message,
ResponseCodes::MISSING_REQUIRED_DATA );
+ }
+ $this->verifyStatusSignature( $response );
+ if ( $response['result'] === '6' ) {
+ $logme = 'Astropay reports they cannot find the
transaction for order ID ' .
+ $this->getData_Unstaged_Escaped( 'order_id' );
+ $this->logger->error( $logme );
+ $this->transaction_response->setErrors( array(
+ 'internal-0000' => array (
+ 'message' =>
$this->getErrorMapByCodeAndTranslate( 'internal-0000' ),
+ 'debugInfo' => $logme,
+ 'logLevel' => LogLevel::ERROR
+ )
+ ) );
}
}
@@ -538,7 +543,7 @@
* Check whether a status message has a valid signature.
* @param array $data
* Requires 'result', 'x_amount', 'x_invoice', and 'x_control'
keys
- * @return boolean true when signature is valid, otherwise false
+ * @throws ResponseProcessingException if signature is invalid
*/
function verifyStatusSignature( $data ) {
if ( $this->getCurrentTransaction() === 'ProcessReturn' ) {
@@ -553,7 +558,11 @@
$data['x_invoice'];
$signature = $this->calculateSignature( $message );
- return ( $signature === $data['x_control'] );
+ if ( $signature !== $data['x_control'] ) {
+ $message = 'Bad signature in transaction ' .
$this->getCurrentTransaction();
+ $this->logger->error( $message );
+ throw new ResponseProcessingException( $message,
ResponseCodes::BAD_SIGNATURE );
+ }
}
protected function calculateSignature( $message ) {
diff --git a/gateway_common/ResponseCodes.php b/gateway_common/ResponseCodes.php
index a1235ca..e0eae50 100644
--- a/gateway_common/ResponseCodes.php
+++ b/gateway_common/ResponseCodes.php
@@ -8,6 +8,7 @@
const BAD_SIGNATURE = 'BAD_SIGNATURE';
const DUPLICATE_ORDER_ID = 'DUPLICATE_ORDER_ID';
const MISSING_TRANSACTION_ID = 'MISSING_TRANSACTION_ID';
+ const MISSING_REQUIRED_DATA = 'MISSING_REQUIRED_DATA';
const NO_RESPONSE = 'NO_RESPONSE';
const UNKNOWN = 'UNKNOWN';
}
diff --git a/tests/Adapter/Astropay/AstropayTest.php
b/tests/Adapter/Astropay/AstropayTest.php
index 3e78349..b5f7211 100644
--- a/tests/Adapter/Astropay/AstropayTest.php
+++ b/tests/Adapter/Astropay/AstropayTest.php
@@ -112,8 +112,23 @@
}
/**
- * When Astropay sends back valid JSON with status "1", we should set
txn
- * status to false and error array to generic error and log a warning.
+ * If astropay sends back non-JSON, communication status should be false
+ */
+ function testGibberishResponse() {
+ $init = $this->getDonorTestData( 'BR' );
+ $this->setLanguage( $init['language'] );
+ $gateway = $this->getFreshGatewayObject( $init );
+ $gateway->setDummyGatewayResponseCode( 'notJson' );
+
+ $gateway->do_transaction( 'NewInvoice' );
+
+ $this->assertEquals( false, $gateway->getTransactionStatus(),
+ 'Transaction status should be false for bad format' );
+ }
+
+ /**
+ * When Astropay sends back valid JSON with status "1", we should set
+ * error array to generic error and log a warning.
*/
function testStatusErrors() {
$init = $this->getDonorTestData( 'BR' );
@@ -122,9 +137,6 @@
$gateway->setDummyGatewayResponseCode( '1' );
$gateway->do_transaction( 'NewInvoice' );
-
- $this->assertEquals( false, $gateway->getTransactionStatus(),
- 'Transaction status should be false for code "1"' );
$expected = array(
'internal-0000' => wfMessage(
'donate_interface-processing-error')->inLanguage( $init['language'] )->text()
@@ -178,8 +190,8 @@
$results = $gateway->getTransactionData();
$this->assertEquals( $expected, $results,
'PaymentStatus response not interpreted correctly' );
- $valid = $gateway->verifyStatusSignature( $results );
- $this->assertTrue( $valid, 'Signature should be interpreted as
valid' );
+ // Should not throw exception
+ $gateway->verifyStatusSignature( $results );
}
/**
@@ -194,8 +206,8 @@
$gateway->do_transaction( 'PaymentStatus' );
$results = $gateway->getTransactionData();
- $valid = $gateway->verifyStatusSignature( $results );
- $this->assertFalse( $valid, 'Signature should not be
interpreted as valid' );
+ $this->setExpectedException( 'ResponseProcessingException' );
+ $gateway->verifyStatusSignature( $results );
}
/**
diff --git a/tests/includes/Responses/astropay/NewInvoice_notJson.testresponse
b/tests/includes/Responses/astropay/NewInvoice_notJson.testresponse
new file mode 100644
index 0000000..5545628
--- /dev/null
+++ b/tests/includes/Responses/astropay/NewInvoice_notJson.testresponse
@@ -0,0 +1,12 @@
+HTTP/1.1 200 OK
+Server: nginx/1.7.9
+Date: Wed, 08 Apr 2015 00:19:52 GMT
+Content-Type: text/xml; charset=UTF-8
+Content-Length: 132
+Connection: keep-alive
+X-Powered-By: PHP/5.3.27
+
+<?xml version = "1.0"?>
+<XML>
+ <ERROR>Your request was so malformed I'm sending you a response in a
different format</ERROR>
+</XML>
--
To view, visit https://gerrit.wikimedia.org/r/206327
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I02dc5496fd1527ffa1cb9d2e56b9e6e71fec7587
Gerrit-PatchSet: 9
Gerrit-Project: mediawiki/extensions/DonationInterface
Gerrit-Branch: master
Gerrit-Owner: Ejegg <eeggles...@wikimedia.org>
Gerrit-Reviewer: Awight <awi...@wikimedia.org>
Gerrit-Reviewer: Ssmith <ssm...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
