From 6f519c961813f62d493887f3f4e11d1a43619a9b Mon Sep 17 00:00:00 2001
From: Florian Schmidt <florian.schmidt.stargatewissen@gmail.com>
Date: Wed, 18 Jul 2018 12:37:17 +0200
Subject: [PATCH] Add caching functionality for IP lookup

Instead of always looking up a single IP address, even for multiple
requests, cache the result of the first lookup. IP addresses usually
doesn't move that fast, caching the result should therefore be
reasonably ok.

Change-Id: Ice78ec08ff886e77c542a75086610498eaa6c6b4
---
 includes/CookieWarningDecisions.php           | 53 ++++++++++++++-----
 includes/ServiceWiring.php                    |  2 +-
 .../includes/CookieWarningDecisionsTest.php   | 34 ++++++++++++
 3 files changed, 74 insertions(+), 15 deletions(-)
 create mode 100644 tests/phpunit/includes/CookieWarningDecisionsTest.php

diff --git a/includes/CookieWarningDecisions.php b/includes/CookieWarningDecisions.php
index d5fe9cc..e68f9f5 100644
--- a/includes/CookieWarningDecisions.php
+++ b/includes/CookieWarningDecisions.php
@@ -3,14 +3,19 @@
 class CookieWarningDecisions {
 	private $config;
 	private $geoLocation;
+	private $cache;
+
+	const CACHE_KEY = 'cookieWarningIpLookupCache:';
 
 	/**
 	 * @param Config $config
 	 * @param GeoLocation $geoLocation
+	 * @param WANObjectCache $cache
 	 */
-	public function __construct( Config $config, GeoLocation $geoLocation ) {
+	public function __construct( Config $config, GeoLocation $geoLocation, WANObjectCache $cache ) {
 		$this->config = $config;
 		$this->geoLocation = $geoLocation;
+		$this->cache = $cache;
 	}
 
 	/**
@@ -35,8 +40,6 @@ class CookieWarningDecisions {
 	/**
 	 * Checks, if the user is in one of the configured regions.
 	 *
-	 * @TODO: This function or the function users should set the cookie or user option, if this
-	 * function returns false to avoid a location lookup on each request.
 	 * @param IContextSource $context
 	 * @return bool
 	 * @throws ConfigException
@@ -50,18 +53,9 @@ class CookieWarningDecisions {
 			return true;
 		}
 
-		wfDebugLog( 'CookieWarning', 'Try to locate the user\'s IP address.' );
-		$located = $this->geoLocation->locate( $context->getRequest()->getIP() );
-		if ( !$located ) {
-			wfDebugLog( 'CookieWarning',
-				'Locating the user\'s IP address failed or is' . ' configured false.' );
+		$countryCode = $this->getCountryCodeFromIP( $context->getRequest()->getIP() );
 
-			return true;
-		}
-		wfDebugLog( 'CookieWarning', 'Locating the user was successful, located' . ' region: ' .
-			$this->geoLocation->getCountryCode() );
-
-		return array_key_exists( $this->geoLocation->getCountryCode(),
+		return array_key_exists( $countryCode,
 			$this->config->get( 'CookieWarningForCountryCodes' ) );
 	}
 
@@ -73,4 +67,35 @@ class CookieWarningDecisions {
 		return $this->config->get( 'CookieWarningGeoIPLookup' ) === 'js' &&
 			is_array( $this->config->get( 'CookieWarningForCountryCodes' ) );
 	}
+
+	/**
+	 * @param $currentIP
+	 * @return bool|mixed|null|string
+	 * @throws ConfigException
+	 */
+	private function getCountryCodeFromIP( $currentIP ) {
+		$cacheKey = $this->cache->makeGlobalKey( __CLASS__, self::CACHE_KEY . $currentIP );
+		$lookedUpCountryCode = $this->cache->get( $cacheKey );
+
+		if ( is_string( $lookedUpCountryCode ) ) {
+			return $lookedUpCountryCode;
+		}
+
+		wfDebugLog( 'CookieWarning', 'Try to locate the user\'s IP address.' );
+		$located = $this->geoLocation->locate( $currentIP );
+		if ( !$located ) {
+			wfDebugLog( 'CookieWarning',
+				'Locating the user\'s IP address failed or is' . ' configured false.' );
+
+			return true;
+		}
+
+		$lookedUpCountryCode = $this->geoLocation->getCountryCode();
+		$this->cache->set( $cacheKey, $lookedUpCountryCode );
+
+		wfDebugLog( 'CookieWarning', 'Locating the user was successful, located' . ' region: ' .
+			$lookedUpCountryCode );
+
+		return $lookedUpCountryCode;
+	}
 }
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index b328fa2..8a15ebe 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -12,6 +12,6 @@ return [
 	},
 	'CookieWarning.Decisions' => function ( MediaWikiServices $services ) {
 		return new CookieWarningDecisions( $services->getService( 'CookieWarning.Config' ),
-			$services->getService( 'GeoLocation' ) );
+			$services->getService( 'GeoLocation' ), $services->getMainWANObjectCache() );
 	},
 ];
diff --git a/tests/phpunit/includes/CookieWarningDecisionsTest.php b/tests/phpunit/includes/CookieWarningDecisionsTest.php
new file mode 100644
index 0000000..df47ea8
--- /dev/null
+++ b/tests/phpunit/includes/CookieWarningDecisionsTest.php
@@ -0,0 +1,34 @@
+<?php
+
+use MediaWiki\MediaWikiServices;
+
+class CookieWarningDecisionsTest extends MediaWikiTestCase {
+	/**
+	 * @covers CookieWarningDecisions::shouldShowCookieWarning()
+	 * @throws ConfigException
+	 * @throws MWException
+	 */
+	public function testShouldNotCallGeoLocationMultiple() {
+		$this->setMwGlobals( [
+			'wgCookieWarningEnabled' => true,
+			'wgCookieWarningGeoIPLookup' => 'php',
+			'wgCookieWarningForCountryCodes' => [ 'EU' => 'European Union' ],
+		] );
+
+		$geoLocation = $this->getMockBuilder( GeoLocation::class )
+			->disableOriginalConstructor()
+			->getMock();
+		$geoLocation->method( 'locate' )->willReturn( true );
+		$geoLocation->method( 'getCountryCode' )->willReturn( 'EU' );
+
+		$geoLocation->expects( $this->once() )->method( 'locate' );
+		$cookieWarningDecisions = new CookieWarningDecisions(
+			MediaWikiServices::getInstance()->getService( 'CookieWarning.Config' ),
+			$geoLocation,
+			new WANObjectCache( [ 'cache' => new HashBagOStuff() ] )
+		);
+
+		$cookieWarningDecisions->shouldShowCookieWarning( RequestContext::getMain() );
+		$cookieWarningDecisions->shouldShowCookieWarning( RequestContext::getMain() );
+	}
+}