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