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>

diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php
index b2524b63c63fa082bb2490596e3449af1c9070b3..ee02bc5a1c49e67a70f3ab6251e112f4a1a8bcb5 100644 (file)
--- 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 9679d0c1759130547cf18aaf1836a4be5c46d3fe..aba3dd0f1d5d46b54b70fee0cb0a86154e0be2ee 100644 (file)
--- 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
                );
        }
 }