From cd0f2a34110d4603145900c74b961a41a844a55d Mon Sep 17 00:00:00 2001 From: eileen Date: Sun, 6 Sep 2020 12:50:55 +1200 Subject: [PATCH] Add try catch to main loops on core ipn classes --- CRM/Core/Payment/AuthorizeNetIPN.php | 101 ++++++------- CRM/Core/Payment/PayPalIPN.php | 136 +++++++++--------- CRM/Core/Payment/PayPalProIPN.php | 122 ++++++++-------- .../CRM/Core/Payment/PayPalProIPNTest.php | 20 +-- 4 files changed, 191 insertions(+), 188 deletions(-) diff --git a/CRM/Core/Payment/AuthorizeNetIPN.php b/CRM/Core/Payment/AuthorizeNetIPN.php index 83697bd338..3b36f17e9a 100644 --- a/CRM/Core/Payment/AuthorizeNetIPN.php +++ b/CRM/Core/Payment/AuthorizeNetIPN.php @@ -35,61 +35,66 @@ class CRM_Core_Payment_AuthorizeNetIPN extends CRM_Core_Payment_BaseIPN { * @return bool|void */ public function main($component = 'contribute') { + try { + //we only get invoice num as a key player from payment gateway response. + //for ARB we get x_subscription_id and x_subscription_paynum + $x_subscription_id = $this->retrieve('x_subscription_id', 'String'); + $ids = $objects = $input = []; - //we only get invoice num as a key player from payment gateway response. - //for ARB we get x_subscription_id and x_subscription_paynum - $x_subscription_id = $this->retrieve('x_subscription_id', 'String'); - $ids = $objects = $input = []; + if ($x_subscription_id) { + // Presence of the id means it is approved. + $input['component'] = $component; - if ($x_subscription_id) { - // Presence of the id means it is approved. - $input['component'] = $component; + // load post vars in $input + $this->getInput($input, $ids); - // load post vars in $input - $this->getInput($input, $ids); + // load post ids in $ids + $this->getIDs($ids, $input); - // load post ids in $ids - $this->getIDs($ids, $input); - - // Attempt to get payment processor ID from URL - if (!empty($this->_inputParameters['processor_id'])) { - $paymentProcessorID = $this->_inputParameters['processor_id']; - } - else { - // This is an unreliable method as there could be more than one instance. - // Recommended approach is to use the civicrm/payment/ipn/xx url where xx is the payment - // processor id & the handleNotification function (which should call the completetransaction api & by-pass this - // entirely). The only thing the IPN class should really do is extract data from the request, validate it - // & call completetransaction or call fail? (which may not exist yet). - Civi::log()->warning('Unreliable method used to get payment_processor_id for AuthNet IPN - this will cause problems if you have more than one instance'); - $paymentProcessorTypeID = CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_PaymentProcessorType', - 'AuthNet', 'id', 'name' - ); - $paymentProcessorID = (int) civicrm_api3('PaymentProcessor', 'getvalue', [ - 'is_test' => 0, - 'options' => ['limit' => 1], - 'payment_processor_type_id' => $paymentProcessorTypeID, - 'return' => 'id', - ]); - } + // Attempt to get payment processor ID from URL + if (!empty($this->_inputParameters['processor_id'])) { + $paymentProcessorID = $this->_inputParameters['processor_id']; + } + else { + // This is an unreliable method as there could be more than one instance. + // Recommended approach is to use the civicrm/payment/ipn/xx url where xx is the payment + // processor id & the handleNotification function (which should call the completetransaction api & by-pass this + // entirely). The only thing the IPN class should really do is extract data from the request, validate it + // & call completetransaction or call fail? (which may not exist yet). + Civi::log()->warning('Unreliable method used to get payment_processor_id for AuthNet IPN - this will cause problems if you have more than one instance'); + $paymentProcessorTypeID = CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_PaymentProcessorType', + 'AuthNet', 'id', 'name' + ); + $paymentProcessorID = (int) civicrm_api3('PaymentProcessor', 'getvalue', [ + 'is_test' => 0, + 'options' => ['limit' => 1], + 'payment_processor_type_id' => $paymentProcessorTypeID, + 'return' => 'id', + ]); + } - if (!$this->validateData($input, $ids, $objects, TRUE, $paymentProcessorID)) { - return FALSE; - } - if (!empty($ids['paymentProcessor']) && $objects['contributionRecur']->payment_processor_id != $ids['paymentProcessor']) { - Civi::log()->warning('Payment Processor does not match the recurring processor id.', ['civi.tag' => 'deprecated']); - } + if (!$this->validateData($input, $ids, $objects, TRUE, $paymentProcessorID)) { + return FALSE; + } + if (!empty($ids['paymentProcessor']) && $objects['contributionRecur']->payment_processor_id != $ids['paymentProcessor']) { + Civi::log()->warning('Payment Processor does not match the recurring processor id.', ['civi.tag' => 'deprecated']); + } - if ($component == 'contribute' && $ids['contributionRecur']) { - // check if first contribution is completed, else complete first contribution - $first = TRUE; - if ($objects['contribution']->contribution_status_id == 1) { - $first = FALSE; + if ($component == 'contribute' && $ids['contributionRecur']) { + // check if first contribution is completed, else complete first contribution + $first = TRUE; + if ($objects['contribution']->contribution_status_id == 1) { + $first = FALSE; + } + return $this->recur($input, $ids, $objects, $first); } - return $this->recur($input, $ids, $objects, $first); } + return TRUE; + } + catch (CRM_Core_Exception $e) { + Civi::log()->debug($e->getMessage()); + echo 'Invalid or missing data'; } - return TRUE; } /** @@ -107,9 +112,7 @@ class CRM_Core_Payment_AuthorizeNetIPN extends CRM_Core_Payment_BaseIPN { // do a subscription check if ($recur->processor_id != $input['subscription_id']) { - CRM_Core_Error::debug_log_message('Unrecognized subscription.'); - echo 'Failure: Unrecognized subscription

'; - return FALSE; + throw new CRM_Core_Exception('Unrecognized subscription.'); } $contributionStatus = CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name'); diff --git a/CRM/Core/Payment/PayPalIPN.php b/CRM/Core/Payment/PayPalIPN.php index 289bdcd745..230804d871 100644 --- a/CRM/Core/Payment/PayPalIPN.php +++ b/CRM/Core/Payment/PayPalIPN.php @@ -53,8 +53,6 @@ class CRM_Core_Payment_PayPalIPN extends CRM_Core_Payment_BaseIPN { public function retrieve($name, $type, $abort = TRUE) { $value = CRM_Utils_Type::validate(CRM_Utils_Array::value($name, $this->_inputParameters), $type, FALSE); if ($abort && $value === NULL) { - Civi::log()->debug("PayPalIPN: Could not find an entry for $name"); - echo "Failure: Missing Parameter

" . CRM_Utils_Type::escape($name, 'String'); throw new CRM_Core_Exception("PayPalIPN: Could not find an entry for $name"); } return $value; @@ -286,84 +284,90 @@ class CRM_Core_Payment_PayPalIPN extends CRM_Core_Payment_BaseIPN { * @throws \CiviCRM_API3_Exception */ public function main() { - $objects = $ids = $input = []; - $component = $this->retrieve('module', 'String'); - $input['component'] = $component; + try { + $objects = $ids = $input = []; + $component = $this->retrieve('module', 'String'); + $input['component'] = $component; - $ids['contact'] = $this->retrieve('contactID', 'Integer', TRUE); - $contributionID = $ids['contribution'] = $this->retrieve('contributionID', 'Integer', TRUE); - $membershipID = $this->retrieve('membershipID', 'Integer', FALSE); - $contributionRecurID = $this->retrieve('contributionRecurID', 'Integer', FALSE); + $ids['contact'] = $this->retrieve('contactID', 'Integer', TRUE); + $contributionID = $ids['contribution'] = $this->retrieve('contributionID', 'Integer', TRUE); + $membershipID = $this->retrieve('membershipID', 'Integer', FALSE); + $contributionRecurID = $this->retrieve('contributionRecurID', 'Integer', FALSE); - $this->getInput($input); + $this->getInput($input); - if ($component == 'event') { - $ids['event'] = $this->retrieve('eventID', 'Integer', TRUE); - $ids['participant'] = $this->retrieve('participantID', 'Integer', TRUE); - } - else { - // get the optional ids - $ids['membership'] = $membershipID; - $ids['contributionRecur'] = $contributionRecurID; - $ids['contributionPage'] = $this->retrieve('contributionPageID', 'Integer', FALSE); - $ids['related_contact'] = $this->retrieve('relatedContactID', 'Integer', FALSE); - $ids['onbehalf_dupe_alert'] = $this->retrieve('onBehalfDupeAlert', 'Integer', FALSE); - } + if ($component == 'event') { + $ids['event'] = $this->retrieve('eventID', 'Integer', TRUE); + $ids['participant'] = $this->retrieve('participantID', 'Integer', TRUE); + } + else { + // get the optional ids + $ids['membership'] = $membershipID; + $ids['contributionRecur'] = $contributionRecurID; + $ids['contributionPage'] = $this->retrieve('contributionPageID', 'Integer', FALSE); + $ids['related_contact'] = $this->retrieve('relatedContactID', 'Integer', FALSE); + $ids['onbehalf_dupe_alert'] = $this->retrieve('onBehalfDupeAlert', 'Integer', FALSE); + } - $paymentProcessorID = $this->getPayPalPaymentProcessorID($input, $ids); + $paymentProcessorID = $this->getPayPalPaymentProcessorID($input, $ids); - Civi::log()->debug('PayPalIPN: Received (ContactID: ' . $ids['contact'] . '; trxn_id: ' . $input['trxn_id'] . ').'); + Civi::log()->debug('PayPalIPN: Received (ContactID: ' . $ids['contact'] . '; trxn_id: ' . $input['trxn_id'] . ').'); - // Debugging related to possible missing membership linkage - if ($contributionRecurID && $this->retrieve('membershipID', 'Integer', FALSE)) { - $templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution($contributionRecurID); - $membershipPayment = civicrm_api3('MembershipPayment', 'get', [ - 'contribution_id' => $templateContribution['id'], - 'membership_id' => $membershipID, - ]); - $lineItems = civicrm_api3('LineItem', 'get', [ - 'contribution_id' => $templateContribution['id'], - 'entity_id' => $membershipID, - 'entity_table' => 'civicrm_membership', - ]); - Civi::log()->debug('PayPalIPN: Received payment for membership ' . (int) $membershipID - . '. Original contribution was ' . (int) $contributionID . '. The template for this contribution is ' - . $templateContribution['id'] . ' it is linked to ' . $membershipPayment['count'] - . 'payments for this membership. It has ' . $lineItems['count'] . ' line items linked to this membership.' - . ' it is expected the original contribution will be linked by both entities to the membership.' - ); - if (empty($membershipPayment['count']) && empty($lineItems['count'])) { - Civi::log()->debug('PayPalIPN: Will attempt to compensate'); - $input['membership_id'] = $this->retrieve('membershipID', 'Integer', FALSE); - } - if ($contributionRecurID) { - $recurLinks = civicrm_api3('ContributionRecur', 'get', [ + // Debugging related to possible missing membership linkage + if ($contributionRecurID && $this->retrieve('membershipID', 'Integer', FALSE)) { + $templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution($contributionRecurID); + $membershipPayment = civicrm_api3('MembershipPayment', 'get', [ + 'contribution_id' => $templateContribution['id'], 'membership_id' => $membershipID, - 'contribution_recur_id' => $contributionRecurID, ]); - Civi::log()->debug('PayPalIPN: Membership should be linked to contribution recur record ' . $contributionRecurID - . ' ' . $recurLinks['count'] . 'links found' + $lineItems = civicrm_api3('LineItem', 'get', [ + 'contribution_id' => $templateContribution['id'], + 'entity_id' => $membershipID, + 'entity_table' => 'civicrm_membership', + ]); + Civi::log()->debug('PayPalIPN: Received payment for membership ' . (int) $membershipID + . '. Original contribution was ' . (int) $contributionID . '. The template for this contribution is ' + . $templateContribution['id'] . ' it is linked to ' . $membershipPayment['count'] + . 'payments for this membership. It has ' . $lineItems['count'] . ' line items linked to this membership.' + . ' it is expected the original contribution will be linked by both entities to the membership.' ); + if (empty($membershipPayment['count']) && empty($lineItems['count'])) { + Civi::log()->debug('PayPalIPN: Will attempt to compensate'); + $input['membership_id'] = $this->retrieve('membershipID', 'Integer', FALSE); + } + if ($contributionRecurID) { + $recurLinks = civicrm_api3('ContributionRecur', 'get', [ + 'membership_id' => $membershipID, + 'contribution_recur_id' => $contributionRecurID, + ]); + Civi::log()->debug('PayPalIPN: Membership should be linked to contribution recur record ' . $contributionRecurID + . ' ' . $recurLinks['count'] . 'links found' + ); + } + } + if (!$this->validateData($input, $ids, $objects, TRUE, $paymentProcessorID)) { + return; } - } - if (!$this->validateData($input, $ids, $objects, TRUE, $paymentProcessorID)) { - return; - } - self::$_paymentProcessor = &$objects['paymentProcessor']; - if ($component == 'contribute') { - if ($ids['contributionRecur']) { - // check if first contribution is completed, else complete first contribution - $first = TRUE; - $completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); - if ($objects['contribution']->contribution_status_id == $completedStatusId) { - $first = FALSE; + self::$_paymentProcessor = &$objects['paymentProcessor']; + if ($component == 'contribute') { + if ($ids['contributionRecur']) { + // check if first contribution is completed, else complete first contribution + $first = TRUE; + $completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); + if ($objects['contribution']->contribution_status_id == $completedStatusId) { + $first = FALSE; + } + $this->recur($input, $ids, $objects, $first); + return; } - $this->recur($input, $ids, $objects, $first); - return; } + $this->single($input, $ids, $objects); + } + catch (CRM_Core_Exception $e) { + Civi::log()->debug($e->getMessage()); + echo 'Invalid or missing data'; } - $this->single($input, $ids, $objects); } /** diff --git a/CRM/Core/Payment/PayPalProIPN.php b/CRM/Core/Payment/PayPalProIPN.php index 024d845b42..f6b2f016d8 100644 --- a/CRM/Core/Payment/PayPalProIPN.php +++ b/CRM/Core/Payment/PayPalProIPN.php @@ -412,77 +412,83 @@ class CRM_Core_Payment_PayPalProIPN extends CRM_Core_Payment_BaseIPN { public function main() { CRM_Core_Error::debug_var('GET', $_GET, TRUE, TRUE); CRM_Core_Error::debug_var('POST', $_POST, TRUE, TRUE); - if ($this->_isPaymentExpress) { - $this->handlePaymentExpress(); - return; - } - $objects = $ids = $input = []; - $this->_component = $input['component'] = self::getValue('m'); - $input['invoice'] = self::getValue('i', TRUE); - // get the contribution and contact ids from the GET params - $ids['contact'] = self::getValue('c', TRUE); - $ids['contribution'] = self::getValue('b', TRUE); - - $this->getInput($input); - - if ($this->_component == 'event') { - $ids['event'] = self::getValue('e', TRUE); - $ids['participant'] = self::getValue('p', TRUE); - $ids['contributionRecur'] = self::getValue('r', FALSE); - } - else { - // get the optional ids - //@ how can this not be broken retrieving from GET as we are dealing with a POST request? - // copy & paste? Note the retrieve function now uses data from _REQUEST so this will be included - $ids['membership'] = self::retrieve('membershipID', 'Integer', 'GET', FALSE); - $ids['contributionRecur'] = self::getValue('r', FALSE); - $ids['contributionPage'] = self::getValue('p', FALSE); - $ids['related_contact'] = self::retrieve('relatedContactID', 'Integer', 'GET', FALSE); - $ids['onbehalf_dupe_alert'] = self::retrieve('onBehalfDupeAlert', 'Integer', 'GET', FALSE); - } + try { + if ($this->_isPaymentExpress) { + $this->handlePaymentExpress(); + return; + } + $objects = $ids = $input = []; + $this->_component = $input['component'] = self::getValue('m'); + $input['invoice'] = self::getValue('i', TRUE); + // get the contribution and contact ids from the GET params + $ids['contact'] = self::getValue('c', TRUE); + $ids['contribution'] = self::getValue('b', TRUE); + + $this->getInput($input); + + if ($this->_component == 'event') { + $ids['event'] = self::getValue('e', TRUE); + $ids['participant'] = self::getValue('p', TRUE); + $ids['contributionRecur'] = self::getValue('r', FALSE); + } + else { + // get the optional ids + //@ how can this not be broken retrieving from GET as we are dealing with a POST request? + // copy & paste? Note the retrieve function now uses data from _REQUEST so this will be included + $ids['membership'] = self::retrieve('membershipID', 'Integer', 'GET', FALSE); + $ids['contributionRecur'] = self::getValue('r', FALSE); + $ids['contributionPage'] = self::getValue('p', FALSE); + $ids['related_contact'] = self::retrieve('relatedContactID', 'Integer', 'GET', FALSE); + $ids['onbehalf_dupe_alert'] = self::retrieve('onBehalfDupeAlert', 'Integer', 'GET', FALSE); + } - if (!$ids['membership'] && $ids['contributionRecur']) { - $sql = " + if (!$ids['membership'] && $ids['contributionRecur']) { + $sql = " SELECT m.id FROM civicrm_membership m INNER JOIN civicrm_membership_payment mp ON m.id = mp.membership_id AND mp.contribution_id = %1 WHERE m.contribution_recur_id = %2 LIMIT 1"; - $sqlParams = [ - 1 => [$ids['contribution'], 'Integer'], - 2 => [$ids['contributionRecur'], 'Integer'], - ]; - if ($membershipId = CRM_Core_DAO::singleValueQuery($sql, $sqlParams)) { - $ids['membership'] = $membershipId; + $sqlParams = [ + 1 => [$ids['contribution'], 'Integer'], + 2 => [$ids['contributionRecur'], 'Integer'], + ]; + if ($membershipId = CRM_Core_DAO::singleValueQuery($sql, $sqlParams)) { + $ids['membership'] = $membershipId; + } } - } - $paymentProcessorID = CRM_Utils_Array::value('processor_id', $this->_inputParameters); - if (!$paymentProcessorID) { - $paymentProcessorID = self::getPayPalPaymentProcessorID(); - } + $paymentProcessorID = CRM_Utils_Array::value('processor_id', $this->_inputParameters); + if (!$paymentProcessorID) { + $paymentProcessorID = self::getPayPalPaymentProcessorID(); + } - if (!$this->validateData($input, $ids, $objects, TRUE, $paymentProcessorID)) { - return; - } + if (!$this->validateData($input, $ids, $objects, TRUE, $paymentProcessorID)) { + return; + } - self::$_paymentProcessor = &$objects['paymentProcessor']; - //?? how on earth would we not have component be one of these? - // they are the only valid settings & this IPN file can't even be called without one of them - // grepping for this class doesn't find other paths to call this class - if ($this->_component == 'contribute' || $this->_component == 'event') { - if ($ids['contributionRecur']) { - // check if first contribution is completed, else complete first contribution - $first = TRUE; - $completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); - if ($objects['contribution']->contribution_status_id == $completedStatusId) { - $first = FALSE; + self::$_paymentProcessor = &$objects['paymentProcessor']; + //?? how on earth would we not have component be one of these? + // they are the only valid settings & this IPN file can't even be called without one of them + // grepping for this class doesn't find other paths to call this class + if ($this->_component == 'contribute' || $this->_component == 'event') { + if ($ids['contributionRecur']) { + // check if first contribution is completed, else complete first contribution + $first = TRUE; + $completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); + if ($objects['contribution']->contribution_status_id == $completedStatusId) { + $first = FALSE; + } + $this->recur($input, $ids, $objects, $first); + return; } - $this->recur($input, $ids, $objects, $first); - return; } + $this->single($input, $ids, $objects, FALSE, FALSE); + } + catch (CRM_Core_Exception $e) { + Civi::log()->debug($e->getMessage()); + echo 'Invalid or missing data'; } - $this->single($input, $ids, $objects, FALSE, FALSE); } /** diff --git a/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php index 7e97b7b77f..201b5cc07d 100644 --- a/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php +++ b/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php @@ -167,25 +167,15 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { * However, for Paypal Pro users payment express transactions can't work as they don't hold the component * which is required for them to be handled by either the Pro or Express class * - * So, the point of this test is simply to ensure it fails in a known way & with a better message than - * previously & that refactorings don't mess with that - * - * Obviously if the behaviour is fixed then the test should be updated! + * So, the point of this test is simply to ensure it fails in a known way */ public function testIPNPaymentExpressNoError() { $this->setupRecurringPaymentProcessorTransaction(); $paypalIPN = new CRM_Core_Payment_PayPalProIPN($this->getPaypalExpressTransactionIPN()); - try { - $paypalIPN->main(); - } - catch (CRM_Core_Exception $e) { - $contribution = $this->callAPISuccess('contribution', 'getsingle', ['id' => $this->_contributionID]); - // no change - $this->assertEquals(2, $contribution['contribution_status_id']); - $this->assertEquals('Paypal IPNS not handled other than recurring_payments', $e->getMessage()); - return; - } - $this->fail('The Paypal Express IPN should have caused an exception'); + $paypalIPN->main(); + $contribution = $this->callAPISuccess('contribution', 'getsingle', ['id' => $this->_contributionID]); + // no change + $this->assertEquals(2, $contribution['contribution_status_id']); } /** -- 2.25.1