From 6634e8c829de775acb92c1a7d51703c38c2b5e26 Mon Sep 17 00:00:00 2001 From: Florian Schmidt Date: Sun, 4 Nov 2018 16:15:42 +0100 Subject: [PATCH] Enable GeoLocation service being replaced by other providers Making GeoLocation an interface makes it easier to replace the underlying implementation from the current Http backed method. Change-Id: I2beb97772fd74ab08b2214c08d82dbc1ebfcdcd2 --- includes/Decisions.php | 13 ++-- includes/GeoLocation.php | 70 ++------------------ includes/HttpGeoLocation.php | 55 +++++++++++++++ includes/NoopGeoLocation.php | 15 +++++ includes/ServiceWiring.php | 12 +++- tests/phpunit/includes/DecisionsTest.php | 3 +- tests/phpunit/includes/HooksTest.php | 3 +- tests/phpunit/includes/ServiceWiringTest.php | 37 +++++++++++ 8 files changed, 130 insertions(+), 78 deletions(-) create mode 100644 includes/HttpGeoLocation.php create mode 100644 includes/NoopGeoLocation.php create mode 100644 tests/phpunit/includes/ServiceWiringTest.php diff --git a/includes/Decisions.php b/includes/Decisions.php index 4629e8a..284f494 100644 --- a/includes/Decisions.php +++ b/includes/Decisions.php @@ -90,20 +90,19 @@ class Decisions { } wfDebugLog( 'CookieWarning', 'Try to locate the user\'s IP address.' ); - $located = $this->geoLocation->locate( $currentIP ); - if ( !$located ) { + $location = $this->geoLocation->locate( $currentIP ); + if ( $location === null ) { wfDebugLog( 'CookieWarning', 'Locating the user\'s IP address failed or is misconfigured.' ); return ''; } - $lookedUpCountryCode = $this->geoLocation->getCountryCode(); - $this->cache->set( $cacheKey, $lookedUpCountryCode ); + $this->cache->set( $cacheKey, $location ); - wfDebugLog( 'CookieWarning', 'Locating the user was successful, located' . ' region: ' . - $lookedUpCountryCode ); + wfDebugLog( 'CookieWarning', + 'Locating the user was successful, located region: ' . $location ); - return $lookedUpCountryCode; + return $location; } } diff --git a/includes/GeoLocation.php b/includes/GeoLocation.php index 13c18cc..db1e592 100644 --- a/includes/GeoLocation.php +++ b/includes/GeoLocation.php @@ -2,73 +2,13 @@ namespace CookieWarning; -/** - * GeoLocation implementation - */ - -use Config; -use ConfigException; -use Http; -use InvalidArgumentException; -use IP; - -/** - * Implements the GeoLocation class, which allows to locate the user based on the IP address. - */ -class GeoLocation { - private $config; - private $countryCode; - +interface GeoLocation { /** - * @param Config $config - */ - public function __construct( Config $config ) { - $this->config = $config; - } - - /** - * Returns the country code, if the last call to self::locate() returned true. Otherwise, NULL. - * - * @return null|string - */ - public function getCountryCode() { - return $this->countryCode; - } - - /** - * Tries to locate the IP address set with self::setIP() using the geolocation service - * configured with the $wgCookieWarningGeoIPServiceURL configuration variable. If the config - * isn't set, this function returns NULL. If the config is set, but the URL is invalid or an - * other problem occures which resulted in a failed locating process, this function returns - * false, otherwise it returns true. + * Tries to locate the given IP address. * * @param string $ip The IP address to lookup - * @return bool|null NULL if no geolocation service configured, false on error, true otherwise. - * @throws ConfigException + * @return null|string NULL on error or if locating the IP was not possible, the country + * code otherwise */ - public function locate( $ip ) { - $this->countryCode = null; - if ( !IP::isValid( $ip ) ) { - throw new InvalidArgumentException( "$ip is not a valid IP address." ); - } - if ( !$this->config->get( 'CookieWarningGeoIPServiceURL' ) ) { - return null; - } - $requestUrl = $this->config->get( 'CookieWarningGeoIPServiceURL' ); - if ( substr( $requestUrl, -1 ) !== '/' ) { - $requestUrl .= '/'; - } - $json = Http::get( $requestUrl . $ip, [ - 'timeout' => '2' - ] ); - if ( !$json ) { - return false; - } - $returnObject = json_decode( $json ); - if ( $returnObject === null || !property_exists( $returnObject, 'country_code' ) ) { - return false; - } - $this->countryCode = $returnObject->country_code; - return true; - } + public function locate( $ip ); } diff --git a/includes/HttpGeoLocation.php b/includes/HttpGeoLocation.php new file mode 100644 index 0000000..f95839f --- /dev/null +++ b/includes/HttpGeoLocation.php @@ -0,0 +1,55 @@ +geoIPServiceURL = $geoIPServiceURL; + } + + /** + * {@inheritdoc} + * @param string $ip The IP address to lookup + * @return string|null + */ + public function locate( $ip ) { + if ( isset( $this->locatedIPs[$ip] ) ) { + return $this->locatedIPs[$ip]; + } + if ( !IP::isValid( $ip ) ) { + throw new InvalidArgumentException( "$ip is not a valid IP address." ); + } + if ( substr( $this->geoIPServiceURL, -1 ) !== '/' ) { + $this->geoIPServiceURL .= '/'; + } + $json = Http::get( $this->geoIPServiceURL . $ip, [ + 'timeout' => '2', + ] ); + if ( !$json ) { + return null; + } + $returnObject = json_decode( $json ); + if ( $returnObject === null || !property_exists( $returnObject, 'country_code' ) ) { + return null; + } + $this->locatedIPs[$ip] = $returnObject->country_code; + + return $this->locatedIPs[$ip]; + } +} diff --git a/includes/NoopGeoLocation.php b/includes/NoopGeoLocation.php new file mode 100644 index 0000000..a174e4b --- /dev/null +++ b/includes/NoopGeoLocation.php @@ -0,0 +1,15 @@ +makeConfig( 'cookiewarning' ); }, 'GeoLocation' => function ( MediaWikiServices $services ) { - return new GeoLocation( $services->getService( 'CookieWarning.Config' ) ); + $geoIPServiceURL = $services + ->getService( 'CookieWarning.Config' ) + ->get( 'CookieWarningGeoIPServiceURL' ); + + if ( !is_string( $geoIPServiceURL ) || !$geoIPServiceURL ) { + return new NoopGeoLocation(); + } + return new HttpGeoLocation( $geoIPServiceURL ); }, 'CookieWarning.Decisions' => function ( MediaWikiServices $services ) { return new Decisions( $services->getService( 'CookieWarning.Config' ), diff --git a/tests/phpunit/includes/DecisionsTest.php b/tests/phpunit/includes/DecisionsTest.php index e74b092..a74d398 100644 --- a/tests/phpunit/includes/DecisionsTest.php +++ b/tests/phpunit/includes/DecisionsTest.php @@ -28,8 +28,7 @@ class DecisionsTest extends MediaWikiTestCase { $geoLocation = $this->getMockBuilder( GeoLocation::class ) ->disableOriginalConstructor() ->getMock(); - $geoLocation->method( 'locate' )->willReturn( true ); - $geoLocation->method( 'getCountryCode' )->willReturn( 'EU' ); + $geoLocation->method( 'locate' )->willReturn( 'EU' ); $geoLocation->expects( $this->once() )->method( 'locate' ); $cookieWarningDecisions = new Decisions( diff --git a/tests/phpunit/includes/HooksTest.php b/tests/phpunit/includes/HooksTest.php index a20cd5c..3eee09f 100644 --- a/tests/phpunit/includes/HooksTest.php +++ b/tests/phpunit/includes/HooksTest.php @@ -191,8 +191,7 @@ class HooksTest extends MediaWikiLangTestCase { $geoLocation = $this->getMockBuilder( GeoLocation::class ) ->disableOriginalConstructor() ->getMock(); - $geoLocation->method( 'locate' )->willReturn( true ); - $geoLocation->method( 'getCountryCode' )->willReturn( 'US' ); + $geoLocation->method( 'locate' )->willReturn( 'US' ); $this->setService( 'GeoLocation', $geoLocation ); } } diff --git a/tests/phpunit/includes/ServiceWiringTest.php b/tests/phpunit/includes/ServiceWiringTest.php new file mode 100644 index 0000000..8a17647 --- /dev/null +++ b/tests/phpunit/includes/ServiceWiringTest.php @@ -0,0 +1,37 @@ +setMwGlobals( [ + 'wgCookieWarningGeoIPServiceURL' => null + ] ); + + $geoLocation = MediaWikiServices::getInstance()->getService( 'GeoLocation' ); + + $this->assertInstanceOf( NoopGeoLocation::class, $geoLocation ); + } + + /** + * @covers \CookieWarning\HttpGeoLocation + */ + public function testGeoLocationWithServiceURL() { + $this->setMwGlobals( [ + 'wgCookieWarningGeoIPServiceURL' => 'http://localhost/' + ] ); + + $geoLocation = MediaWikiServices::getInstance()->getService( 'GeoLocation' ); + + $this->assertInstanceOf( HttpGeoLocation::class, $geoLocation ); + } +}