jenkins-bot has submitted this change and it was merged.
Change subject: DataValidator uses GatewayAdapter object
......................................................................
DataValidator uses GatewayAdapter object
This makes it possible to change validation based on adapter logic such
as country controlling allowable currencies, etc.
Includes a basic test of validation. TODO: expand to cover all validations.
Change-Id: Icb347e2a1a77d359ff604049481d8dca0693e91e
---
M extras/custom_filters/filters/ip_velocity/ip_velocity.body.php
M extras/session_velocity/session_velocity.body.php
M gateway_common/DataValidator.php
M gateway_common/DonationData.php
A tests/GatewayValidationTest.php
M tests/includes/test_gateway/TestingGenericAdapter.php
6 files changed, 125 insertions(+), 128 deletions(-)
Approvals:
Ejegg: Looks good to me, approved
jenkins-bot: Verified
diff --git a/extras/custom_filters/filters/ip_velocity/ip_velocity.body.php
b/extras/custom_filters/filters/ip_velocity/ip_velocity.body.php
index 76c7989..65227a1 100644
--- a/extras/custom_filters/filters/ip_velocity/ip_velocity.body.php
+++ b/extras/custom_filters/filters/ip_velocity/ip_velocity.body.php
@@ -23,12 +23,12 @@
$user_ip = $this->gateway_adapter->getData_Unstaged_Escaped(
'user_ip' );
//first, handle the whitelist / blacklist before you do
anything else.
- if ( DataValidator::ip_is_listed( $user_ip, 'IPWhitelist' ) ){
+ if ( DataValidator::ip_is_listed( $user_ip,
$this->gateway_adapter->getGlobal( 'IPWhitelist' ) ) ) {
$this->gateway_adapter->debugarray[] = "IP present in
whitelist.";
$this->cfo->addRiskScore( 0, 'IPWhitelist' );
return true;
}
- if ( DataValidator::ip_is_listed( $user_ip, 'IPBlacklist' ) ){
+ if ( DataValidator::ip_is_listed( $user_ip,
$this->gateway_adapter->getGlobal( 'IPBlacklist' ) ) ) {
$this->gateway_adapter->debugarray[] = "IP present in
blacklist.";
$this->cfo->addRiskScore(
$this->gateway_adapter->getGlobal( 'IPVelocityFailScore' ), 'IPBlacklist' );
return true;
diff --git a/extras/session_velocity/session_velocity.body.php
b/extras/session_velocity/session_velocity.body.php
index c1dd701..cebd5df 100644
--- a/extras/session_velocity/session_velocity.body.php
+++ b/extras/session_velocity/session_velocity.body.php
@@ -61,7 +61,7 @@
return true;
}
$gateway_adapter->debugarray[] = 'Session Velocity onFilter
hook!';
- return self::singleton( $gateway_adapter )->filter(
$gateway_adapter );
+ return self::singleton( $gateway_adapter )->filter();
}
/**
@@ -72,13 +72,13 @@
*
* @return bool Hook return, false stops processing of the hook chain
*/
- private function filter( &$gateway_adapter ) {
+ private function filter() {
$user_ip = $this->gateway_adapter->getData_Unstaged_Escaped(
'user_ip' );
// Determine IP status before doing anything complex
- $wl = DataValidator::ip_is_listed( $user_ip, 'IPWhitelist',
$gateway_adapter->getIdentifier() );
- $bl = DataValidator::ip_is_listed( $user_ip, 'IPBlacklist',
$gateway_adapter->getIdentifier() );
+ $wl = DataValidator::ip_is_listed( $user_ip,
$this->gateway_adapter->getGlobal( 'IPWhitelist' ) );
+ $bl = DataValidator::ip_is_listed( $user_ip,
$this->gateway_adapter->getGlobal( 'IPBlacklist' ) );
if ( $wl ) {
$this->gateway_adapter->debugarray[] =
"SessionVelocity: IP present in whitelist.";
@@ -95,12 +95,12 @@
}
// Obtain some useful information
- $gateway = $gateway_adapter->getIdentifier();
- $transaction = $gateway_adapter->getCurrentTransaction();
+ $gateway = $this->gateway_adapter->getIdentifier();
+ $transaction = $this->gateway_adapter->getCurrentTransaction();
$cRequestTime = $_SERVER['REQUEST_TIME'];
- $decayRate = $this->getVar( 'DecayRate', $transaction,
$gateway_adapter );
- $threshold = $this->getVar( 'Threshold', $transaction,
$gateway_adapter );
+ $decayRate = $this->getVar( 'DecayRate', $transaction );
+ $threshold = $this->getVar( 'Threshold', $transaction );
// Initialize the filter
if ( !array_key_exists( self::SESS_ROOT, $_SESSION ) ) {
@@ -122,7 +122,7 @@
// Update the filter if it's stale
if ( $cRequestTime != $lastTime ) {
$score = max( 0, $score - ( ( $cRequestTime - $lastTime
) * $decayRate ) );
- $score += $this->getVar( 'HitScore', $transaction,
$gateway_adapter );
+ $score += $this->getVar( 'HitScore', $transaction );
// Store the results
$_SESSION[self::SESS_ROOT][$gateway][$transaction][$this::SESS_SCORE] = $score;
@@ -148,14 +148,13 @@
*
* @param string $baseVar The root name of the variable
* @param string $txn The name of the transaction
- * @param object $gateway The current gateway object
*
* @return mixed The contents of the configuration variable
*/
- private function getVar( $baseVar, $txn, &$gateway ) {
- $var = $gateway->getGlobal( 'SessionVelocity_' . $txn . '_' .
$baseVar );
+ private function getVar( $baseVar, $txn ) {
+ $var = $this->gateway_adapter->getGlobal( 'SessionVelocity_' .
$txn . '_' . $baseVar );
if ( !isset( $var ) ) {
- $var = $gateway->getGlobal( 'SessionVelocity_' .
$baseVar );
+ $var = $this->gateway_adapter->getGlobal(
'SessionVelocity_' . $baseVar );
}
return $var;
diff --git a/gateway_common/DataValidator.php b/gateway_common/DataValidator.php
index 96f80b4..c090e08 100644
--- a/gateway_common/DataValidator.php
+++ b/gateway_common/DataValidator.php
@@ -9,6 +9,7 @@
* order to use it any/everywhere.
*
* @author khorn
+ * @author awight
*/
class DataValidator {
@@ -39,21 +40,7 @@
'order_id',
'numAttempt'
);
-
- /**
- * $gateway_classes
- * @var array A list of all possible gateway classes.
- * FIXME: get rid of this
- */
- protected static $gateway_classes = array(
- 'globalcollect' => 'GlobalCollectAdapter',
- 'payflowpro' => 'PayflowProAdapter',
- 'paypal' => 'PaypalAdapter',
- 'adyen' => 'AdyenAdapter',
- 'amazon' => 'AmazonAdapter',
- 'worldpay' => 'WorldPayAdapter'
- );
-
+
/**
* $card_types
* @var array A list of SOME card types we recognize
@@ -276,6 +263,7 @@
* wfLangSpecificFallback - returns the text of the first existant
message
* in the requested language. If no messages are found in that
language, the
* function returns the first existant fallback message.
+ * TODO: Belongs somewhere very else.
*
* @param string $language the code of the requested language
* @param array $msg_keys
@@ -322,7 +310,7 @@
* the main DonationInterface Form class to display. The array will be
empty
* if no errors were generated and everything passed OK.
*/
- public static function validate( $gateway, $data, $check_not_empty =
array() ){
+ public static function validate( GatewayAdapter $gateway, $data,
$check_not_empty = array() ){
//return the array of errors that should be generated on
validate.
//just the same way you'd do it if you were a form passing the
error array around.
@@ -334,8 +322,6 @@
* Third: Do validation that depends on multiple fields (making
sure you
* validated that all the required fields exist on step 1,
regardless of
* $check_not_empty)
- *
- * So, we need to know what we're about to do for #3 before we
actually do #1.
*
* $check_not_empty should contain an array of values that need
to be populated.
* One likely candidate for a source there, is the required
stomp fields as defined in DonationData.
@@ -458,11 +444,13 @@
switch ( $function ){
case 'validate_amount':
if (
self::checkValidationPassed( array( 'currency_code', 'gateway' ), $instructions
) ){
- $result =
$self::$function( $data[$field], $data['currency_code'], $data['gateway'] );
+ $priceFloor =
$gateway->getGlobal( 'PriceFloor' );
+ $priceCeiling =
$gateway->getGlobal( 'PriceCeiling' );
+ $result =
$self::$function( $data[$field], $data['currency_code'], $priceFloor,
$priceCeiling );
} //otherwise, just don't do
the validation. The other stuff will be complaining already.
break;
case 'validate_currency_code':
- $result = $self::$function(
$data[$field], $data['gateway'] );
+ $result = $self::$function(
$data[$field], $gateway->getCurrencies() );
break;
case 'validate_card_type':
//the contingent field in this
case isn't strictly required, so this is going to look funny.
@@ -487,8 +475,6 @@
throw new MWException( __FUNCTION__ . " BAD
PROGRAMMER. No $function function. ('calculated' rule for $field)" );
}
}
-// error_log( __FUNCTION__ . " " . print_r( $instructions, true )
);
-// error_log( print_r( $errors, true ) );
return $errors;
}
@@ -572,29 +558,20 @@
/**
* validate_amount
+ *
* Determines if the $value passed in is a valid amount.
- * NOTE: You will need to make sure that currency_code is populated
before
- * you get here.
* @param string $value The piece of data that is supposed to be an
amount.
- * @param string $currency_code Valid amounts depend on there being a
- * currency code also. This also needs to be passed in.
- * @param string $gateway The gateway needs to be provided so we can
- * determine that gateway's current price floor and ceiling.
+ * @param string $currency_code The amount was given in this currency.
+ * @param float $priceFloor Minimum valid amount (USD).
+ * @param float $priceCeiling Maximum valid amount (USD).
+ *
* @return boolean True if $value is a valid amount, otherwise false.
*/
- protected static function validate_amount( $value, $currency_code,
$gateway ){
+ protected static function validate_amount( $value, $currency_code,
$priceFloor, $priceCeiling ) {
if ( !$value || !$currency_code || !is_numeric( $value ) ) {
return false;
}
-
- // check amount
- $gateway_class = self::getGatewayClass($gateway);
- if ( !$gateway_class ){
- return false;
- }
-
- $priceFloor = $gateway_class::getGlobal( 'PriceFloor' );
- $priceCeiling = $gateway_class::getGlobal( 'PriceCeiling' );
+
if ( !preg_match( '/^\d+(\.(\d+)?)?$/', $value ) ||
( ( float ) self::convert_to_usd( $currency_code,
$value ) < ( float ) $priceFloor ||
( float ) self::convert_to_usd( $currency_code, $value
) > ( float ) $priceCeiling ) ) {
@@ -604,20 +581,12 @@
return true;
}
- protected static function validate_currency_code( $value, $gateway ) {
+ protected static function validate_currency_code( $value,
$acceptedCurrencies ) {
if ( !$value ) {
return false;
}
- $gateway_class = self::getGatewayClass($gateway);
- if ( !$gateway_class ){
- return false;
- }
-
- // FIXME: we should be checking currencies using the live
gateway
- // object, the result is often dependent on payment
method/submethod,
- // country, and so on.
- return in_array( $value, $gateway_class::getCurrencies() );
+ return in_array( $value, $acceptedCurrencies );
}
/**
@@ -667,6 +636,7 @@
* @return boolean True if $value is a valid boolean, otherwise false.
*/
protected static function validate_boolean( $value ){
+ // FIXME: this doesn't do the strict comparison we intended.
'hello' would match the "case true" statement.
switch ($value) {
case 0:
case '0':
@@ -706,14 +676,11 @@
* @return boolean True if $value is a valid gateway, otherwise false
*/
protected static function validate_gateway( $value ){
- if ( self::getGatewayClass( $value ) ){
- return true;
- }
-
- return false;
+ global $wgDonationInterfaceEnabledGateways;
+
+ return in_array( $value, $wgDonationInterfaceEnabledGateways,
true );
}
-
-
+
/**
* validate_not_empty
* Checks to make sure that the $value is present in the $data array,
and not null or an empty string.
@@ -899,23 +866,7 @@
}
return( ( $sum % 10 ) == 0 );
}
-
- /**
- * getGatewayClass
- * This exists to enable things like logging to the correct gateway,
and
- * retrieving gateway-specific globals.
- * @param string $gateway The gateway identifier.
- * @return string The name of the gateway class associated with that
- * identifier, or false if none exists.
- */
- protected static function getGatewayClass( $gateway ) {
- if ( array_key_exists( $gateway, self::$gateway_classes ) &&
class_exists( self::$gateway_classes[$gateway] ) ){
- return self::$gateway_classes[$gateway];
- }
- return false;
- }
-
-
+
/**
* Convert an amount for a particular currency to an amount in USD
*
@@ -939,7 +890,7 @@
if ( array_key_exists( $code, $rates ) ) {
$usd_amount = $amount / $rates[$code];
} else {
- $usd_amount = $amount;
+ throw new Exception( 'Bad programmer! Bad currency
made it too far through the portcullis' );
}
return $usd_amount;
}
@@ -1010,51 +961,22 @@
}
/**
- * Eventually, this function should pull from here and memcache.
- * @staticvar array $blacklist A cached and expanded blacklist
+ * Check whether IP matches a block list
+ *
+ * TODO: We might want to store the expanded list in memcache.
+ *
* @param string $ip The IP addx we want to check
- * @param string $list_name The global list, ostensibly full of IP
addresses,
- * that we want to check against.
- * @param string $gateway The gateway we're concerned with. Only
matters if,
- * for instance, $wgDonationInterfaceIPBlacklist is different from
- * $wgGlobalcollectGatewayIPBlacklist for some silly reason.
+ * @param array $ip_list IP list to check against
* @throws MWException
* @return bool
*/
- public static function ip_is_listed( $ip, $list_name, $gateway = '' ) {
- //cache this mess
- static $ip_list_cache = array();
- $globalIPLists = array(
- 'IPWhitelist',
- 'IPBlacklist',
- );
-
- if ( !in_array( $list_name, $globalIPLists ) ){
- throw new MWException( __FUNCTION__ . " BAD PROGRAMMER.
No recognized global list of IPs called $list_name. Do better." );
+ public static function ip_is_listed( $ip, $ip_list ) {
+ $expanded = array();
+ foreach ( $ip_list as $address ){
+ $expanded = array_merge( $expanded,
self::expandIPBlockToArray( $address ) );
}
-
- $class = self::getGatewayClass( $gateway );
- if ( !$class ){
- $class = 'GatewayAdapter';
- }
-
- if ( !array_key_exists( $class, $ip_list_cache ) ||
!array_key_exists( $list_name, $ip_list_cache[$class] ) ){
- //go get it and expand the block entries
- $list = $class::getGlobal( $list_name );
- $expanded = array();
- foreach ( $list as $address ){
- $expanded = array_merge( $expanded,
self::expandIPBlockToArray( $address ) );
- }
- $ip_list_cache[$class][$list_name] = $expanded;
- //TODO: This seems like an excellent time to stash this
expanded
- //thing in memcache. Later, we can look for that value
earlier. Yup.
- }
-
- if ( in_array( $ip, $ip_list_cache[$class][$list_name] ) ){
- return true;
- } else {
- return false;
- }
+
+ return in_array( $ip, $expanded, true );
}
/**
diff --git a/gateway_common/DonationData.php b/gateway_common/DonationData.php
index 3fc1dc8..35cc646 100644
--- a/gateway_common/DonationData.php
+++ b/gateway_common/DonationData.php
@@ -315,6 +315,8 @@
* be called multiple times against the same array.
*/
protected function normalize() {
+ // FIXME: there's a ghost invocation during DonationData
construction.
+ // This condition should actually be "did data come from
anywhere?"
if ( !empty( $this->normalized ) ) {
$updateCtRequired =
$this->handleContributionTrackingID(); // Before Order ID
$this->setNormalizedOrderIDs();
diff --git a/tests/GatewayValidationTest.php b/tests/GatewayValidationTest.php
new file mode 100644
index 0000000..1aeb1dd
--- /dev/null
+++ b/tests/GatewayValidationTest.php
@@ -0,0 +1,71 @@
+<?php
+/**
+ * Wikimedia Foundation
+ *
+ * LICENSE
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+/**
+ * @group Fundraising
+ * @group DonationInterface
+ * @group GatewayPage
+ */
+class GatewayValidationTest extends MediaWikiTestCase {
+
+ protected $page;
+ protected $adapter;
+
+ public function setUp() {
+ parent::setUp();
+
+ $this->setMwGlobals( array(
+ 'wgDonationInterfaceEnabledGateways' => array(
'donation' ), // base class. awkward.
+ 'wgDonationInterfacePriceFloor' => 2.00,
+ ) );
+
+ TestingGenericAdapter::$acceptedCurrencies[] = 'USD';
+
+ $this->page = new TestingGatewayPage();
+ $this->adapter = new TestingGenericAdapter();
+ $this->page->adapter = $this->adapter;
+ parent::setUp();
+ }
+
+ public function testPassesValidation() {
+ $this->adapter->addData( array(
+ 'amount' => '2.00',
+ 'country' => 'US',
+ 'currency_code' => 'USD',
+ ) );
+
+ $this->page->validateForm();
+
+ $this->assertTrue( $this->adapter->validatedOK() );
+ }
+
+ public function testLowAmountError() {
+ $this->adapter->addData( array(
+ 'amount' => '1.99',
+ 'country' => 'US',
+ 'currency_code' => 'USD',
+ ) );
+
+ $this->page->validateForm();
+
+ $this->assertFalse( $this->adapter->validatedOK() );
+
+ $errors = $this->adapter->getValidationErrors();
+ $this->assertArrayHasKey( 'amount', $errors );
+ }
+}
diff --git a/tests/includes/test_gateway/TestingGenericAdapter.php
b/tests/includes/test_gateway/TestingGenericAdapter.php
index c92d3b8..a70a404 100644
--- a/tests/includes/test_gateway/TestingGenericAdapter.php
+++ b/tests/includes/test_gateway/TestingGenericAdapter.php
@@ -29,6 +29,8 @@
public $revalidateCount = 0;
public static $fakeGlobals = array();
+ public static $acceptedCurrencies = array();
+
public function revalidate($check_not_empty = array()) {
if ( $this->errorsForRevalidate ) {
$fakeErrors =
$this->errorsForRevalidate[$this->revalidateCount];
@@ -95,6 +97,7 @@
}
public static function getCurrencies() {
+ return TestingGenericAdapter::$acceptedCurrencies;
}
}
--
To view, visit https://gerrit.wikimedia.org/r/184010
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Icb347e2a1a77d359ff604049481d8dca0693e91e
Gerrit-PatchSet: 14
Gerrit-Project: mediawiki/extensions/DonationInterface
Gerrit-Branch: master
Gerrit-Owner: Awight <[email protected]>
Gerrit-Reviewer: AndyRussG <[email protected]>
Gerrit-Reviewer: Awight <[email protected]>
Gerrit-Reviewer: Ejegg <[email protected]>
Gerrit-Reviewer: Katie Horn <[email protected]>
Gerrit-Reviewer: Ssmith <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
