Fixed Issue #18581: Blocking users after X failed attempts counts inc…

…orrectly (off by 1) (#2914)

Co-authored-by: Lapiu Dev <devgit@lapiu.biz>

application/models/FailedLoginAttempt.php

@@ -104,15 +104,15 @@ public function isLockedOut(string $attemptType): bool
 
         if (Yii::app()->getConfig('DBVersion') <= 480) {
             $criteria = new CDbCriteria();
-            $criteria->condition = 'number_attempts > :attempts AND ip = :ip';
+            $criteria->condition = 'number_attempts >= :attempts AND ip = :ip';
             $criteria->params = array(
                 ':attempts' => $maxLoginAttempt,
                 ':ip' => $ip,
             );
             $row = $this->find($criteria);
         } else {
             $criteria = new CDbCriteria();
-            $criteria->condition = 'number_attempts > :attempts AND ip = :ip AND is_frontend = :is_frontend';
+            $criteria->condition = 'number_attempts >= :attempts AND ip = :ip AND is_frontend = :is_frontend';
             $criteria->params = array(
                 ':attempts' => $maxLoginAttempt,
                 ':ip' => $ip,

tests/unit/models/FailedLoginAttemptTest.php

@@ -0,0 +1,58 @@
+<?php
+
+namespace ls\tests;
+
+use FailedLoginAttempt;
+
+class FailedLoginAttempTest extends TestBaseClass
+{
+    public function testAddDeleteAttemp()
+    {
+        // Save Ip
+        $ip = substr(getIPAddress(), 0, 40);
+
+        // Make sure there are no records for the ip
+        FailedLoginAttempt::model()->deleteAttempts(FailedLoginAttempt::TYPE_LOGIN);
+        $this->assertNull(FailedLoginAttempt::model()->findByAttributes((array('ip' => $ip))));
+
+        // Verify that the try counter increases by one
+        FailedLoginAttempt::model()->addAttempt();
+        $data = FailedLoginAttempt::model()->findByAttributes((array('ip' => $ip)));
+        $this->assertEquals(1, $data->number_attempts);
+
+        // Verify that the try counter increases by one
+        FailedLoginAttempt::model()->addAttempt();
+        $data = FailedLoginAttempt::model()->findByAttributes((array('ip' => $ip)));
+        $this->assertEquals(2, $data->number_attempts);
+
+        // Verify that all records are deleted
+        FailedLoginAttempt::model()->deleteAttempts(FailedLoginAttempt::TYPE_LOGIN);
+        $this->assertNull(FailedLoginAttempt::model()->findByAttributes((array('ip' => $ip))));
+    }
+
+    public function testIsLockedOut()
+    {
+        $maxLoginAttempt = \Yii::app()->getConfig('maxLoginAttempt');
+
+        // Verify that the user has attempts available
+        FailedLoginAttempt::model()->deleteAttempts(FailedLoginAttempt::TYPE_LOGIN);
+        for ($i = 0; $i < $maxLoginAttempt - 1; $i++) {
+            FailedLoginAttempt::model()->addAttempt();
+            $this->assertFalse(FailedLoginAttempt::model()->isLockedOut(FailedLoginAttempt::TYPE_LOGIN));
+        }
+
+        // Verify that the user has no attempts available
+        FailedLoginAttempt::model()->addAttempt();  
+        $this->assertTrue(FailedLoginAttempt::model()->isLockedOut(FailedLoginAttempt::TYPE_LOGIN));
+    }
+
+    /**
+     * @return void
+     */
+    public static function tearDownAfterClass(): void
+    {
+        FailedLoginAttempt::model()->deleteAttempts(FailedLoginAttempt::TYPE_LOGIN);
+
+        parent::tearDownAfterClass();
+    }    
+}