Mark IP as whitelisted if brute force protection is disabled

Currently, when disabling the brute force protection no new brute force attempts are logged. However, the ones logged within the last 24 hours will still be used for throttling.

This is quite an unexpected behaviour and caused some support issues. With this change when the brute force protection is disabled also the existing attempts within the last 24 hours will be disregarded.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

2 files changed, 55 insertions, 8 deletions

diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php
index b2524b63c63..ee02bc5a1c4 100644
--- a/lib/private/Security/Bruteforce/Throttler.php
+++ b/lib/private/Security/Bruteforce/Throttler.php
@@ -133,6 +133,10 @@ class Throttler {
 	 * @return bool
 	 */
 	private function isIPWhitelisted($ip) {
+		if($this->config->getSystemValue('auth.bruteforce.protection.enabled', true) === false) {
+			return true;
+		}
+
 		$keys = $this->config->getAppKeys('bruteForce');
 		$keys = array_filter($keys, function($key) {
 			$regex = '/^whitelist_/S';
diff --git a/tests/lib/Security/Bruteforce/ThrottlerTest.php b/tests/lib/Security/Bruteforce/ThrottlerTest.php
index 9679d0c1759..aba3dd0f1d5 100644
--- a/tests/lib/Security/Bruteforce/ThrottlerTest.php
+++ b/tests/lib/Security/Bruteforce/ThrottlerTest.php
@@ -54,19 +54,19 @@ class ThrottlerTest extends TestCase {
 			$this->logger,
 			$this->config
 		);
-		return parent::setUp();
+		parent::setUp();
 	}
 
 	public function testCutoff() {
 		// precisely 31 second shy of 12 hours
-		$cutoff = $this->invokePrivate($this->throttler, 'getCutoff', [43169]);
+		$cutoff = self::invokePrivate($this->throttler, 'getCutoff', [43169]);
 		$this->assertSame(0, $cutoff->y);
 		$this->assertSame(0, $cutoff->m);
 		$this->assertSame(0, $cutoff->d);
 		$this->assertSame(11, $cutoff->h);
 		$this->assertSame(59, $cutoff->i);
 		$this->assertSame(29, $cutoff->s);
-		$cutoff = $this->invokePrivate($this->throttler, 'getCutoff', [86401]);
+		$cutoff = self::invokePrivate($this->throttler, 'getCutoff', [86401]);
 		$this->assertSame(0, $cutoff->y);
 		$this->assertSame(0, $cutoff->m);
 		$this->assertSame(1, $cutoff->d);
@@ -136,16 +136,23 @@ class ThrottlerTest extends TestCase {
 	}
 
 	/**
-	 * @dataProvider dataIsIPWhitelisted
-	 *
 	 * @param string $ip
-	 * @param string[] $whitelists
+	 * @param string[]$whitelists
 	 * @param bool $isWhiteListed
+	 * @param bool $enabled
 	 */
-	public function testIsIPWhitelisted($ip, $whitelists, $isWhiteListed) {
+	private function isIpWhiteListedHelper($ip,
+										 $whitelists,
+										 $isWhiteListed,
+										 $enabled) {
 		$this->config->method('getAppKeys')
 			->with($this->equalTo('bruteForce'))
 			->willReturn(array_keys($whitelists));
+		$this->config
+			->expects($this->once())
+			->method('getSystemValue')
+			->with('auth.bruteforce.protection.enabled', true)
+			->willReturn($enabled);
 
 		$this->config->method('getAppValue')
 			->will($this->returnCallback(function($app, $key, $default) use ($whitelists) {
@@ -159,8 +166,44 @@ class ThrottlerTest extends TestCase {
 			}));
 
 		$this->assertSame(
+			($enabled === false) ? true : $isWhiteListed,
+			self::invokePrivate($this->throttler, 'isIPWhitelisted', [$ip])
+		);
+	}
+
+	/**
+	 * @dataProvider dataIsIPWhitelisted
+	 *
+	 * @param string $ip
+	 * @param string[] $whitelists
+	 * @param bool $isWhiteListed
+	 */
+	public function testIsIpWhiteListedWithEnabledProtection($ip,
+															 $whitelists,
+															 $isWhiteListed) {
+		$this->isIpWhiteListedHelper(
+			$ip,
+			$whitelists,
+			$isWhiteListed,
+			true
+		);
+	}
+
+	/**
+	 * @dataProvider dataIsIPWhitelisted
+	 *
+	 * @param string $ip
+	 * @param string[] $whitelists
+	 * @param bool $isWhiteListed
+	 */
+	public function testIsIpWhiteListedWithDisabledProtection($ip,
+															 $whitelists,
+															 $isWhiteListed) {
+		$this->isIpWhiteListedHelper(
+			$ip,
+			$whitelists,
 			$isWhiteListed,
-			$this->invokePrivate($this->throttler, 'isIPWhitelisted', [$ip])
+			false
 		);
 	}
 }