Login Throttling Help

I am building a simple login and am trying to add some basic throttling. Right now, my code is not working but close to working. I just cannot spot the issue (probably due to looking at it for 1 day straight).

Note: For my debugging i have made some variables redundent just so i can get the maths working so for example you will see $username but actually i have ignored it and just done id = 1 for my testing. That is what i need help with. Getting the correct amount of minutes to appear and to get the counter to work which breaks due to something in the throttleLogin private function but i am unable to figure it out. All help much appreciated.

Here is the code:

Attempts table SQL:

Controller login code:

    public function login() 
    {
        $getSettings = $this->start->getSettings(); // Load site settings
        $reCaptcha = new reCaptcha('login', '', ''); // load recaptcha class for spam protection

        #$locateUser = !startHelper::isBot($_SERVER['HTTP_USER_AGENT']) ? $this->start->getUserLocation(startHelper::getIP()) : false;  // Get user location data

        $always = [
            'getSettings' => $getSettings,
            'helpRecaptcha' => $reCaptcha,
        ];

        if ($_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['g-recaptcha-response'])) // Check form submitted via POST
        {
            $_POST = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING); // Clean $_POST data

            $formData = [
                'username' => trim($_POST['username']),
                'password' => $_POST['password'],

                'username_err' => '',
                'password_err' => '',
                'global_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'always' => $always,
                'title' => LANG['auth-login-title'],
                'description' => '',
            ];

            if (empty($data['formData']['username'])) { // Check Username not empty
                $data['formData']['username_err'] = LANG['auth-login-invalid1'];
            }
            if (empty($data['formData']['password'])) { // Check Password not empty
                $data['formData']['password_err'] = LANG['auth-login-invalid3'];
            }
            if (!$reCaptcha->success()) { // Check anti-spam reCaptcha.
                $data['formData']['global_err'] = LANG['auth-register-invalid20'];
            }

           /*
This code below commented out causes the database counter to break in the sense it won't go counter = counter + 1. I need it to only return the X minutes timeout if above the limits.
*/

          /* THIS IS REAL IF  if (!empty($data['formData']['username']) && !empty($data['formData']['password']) && ($remaining = $this->throttleLogin($data['formData']['username']))) {
                $data['formData']['global_err'] = $remaining;
            }*/
          /*THIS IS DEBUGGING IF
$remaining = $this->throttleLogin($data['formData']['username']);
          if (!$remaining) {
              echo $remaining;
              var_dump($remaining);
              die();
          }*/

            if (empty($data['formData']['username_err']) && empty($data['formData']['password_err']) && empty($data['formData']['global_err'])) {
                $userAuthenticated = $this->auth->doLogin($data['formData']['username'], $data['formData']['password']);
                if ($userAuthenticated) {
                    echo 'success';
                    #$this->createUserSession($userAuthenticated);
                } else {
                    $this->auth->updateLoginAttemptCounter();
                    echo 'fail';
                    #$remaining = $this->throttleLogin($data['formData']['username']);
                    #var_dump($remaining);
                }

            } else {
                $this->view('user/login', $data);
            }
            $this->view('user/login', $data);
        } else {
            $formData = [
                'username' => '',
                'password' => '',

                'username_err' => '',
                'password_err' => '',
                'global_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'always' => $always,
                'title' => LANG['auth-login-title'],
                'description' => '',
            ];

            $this->view('user/login', $data);
        }       
    }

    private function throttleLogin($username)
    {
        /* $throttle array, you should probably put this with the other Settings */
        $throttle = [
            2 => 8, //8 minute delay after 2 bad attempts
            3 => 10,
            9 => 'captcha', //captcha or ban account or send suspicous email etc.
            10 => 'lock', // lock account and send email to user saying suspicous activity
        ];
       
        $getSettings = $this->start->getSettings();
        $attemptExists = $this->auth->getLoginAttempts('::1', $username);

        if(!isset($attemptExists)) { return 0; }
       
        /* Read the "counter" value from the attempt record */
        $counter = $attemptExists['counter'];
       
        /* Now we calculate the $lockout using the $throttle values */
        $lockout = 5;
       
        foreach ($throttle as $throttle_key => $throttle_value)
        {  
            if ($counter === $throttle_key) // attempts more than or equal to throttle array attempts
            {
                if (is_numeric($throttle_value)) {  // if number array (minutes)
                    $lockout = 60 * $throttle_value; //by x minutes
                    return $lockout; // return value for calculations
                } elseif ($throttle_value === 'captcha') {
                    /* Here you should send the captcha etc. */
                    die('here, we would send an email and lock ur account');
                }
            }
        }

        $last_attempt = strtotime($attemptExists['last_attempt']);
        $since_last_attempt = time() - $last_attempt;
        $seconds_remaining = $lockout - $since_last_attempt;
        $minutes_remaining = ceil($seconds_remaining / 60);
        if($seconds_remaining <= 0) {
            $this->auth->resetLoginAttempts($username);
            return 0;
        } else {
            return $minutes_remaining;
        }
        #return false;
    }

Model code:

    public function updateLoginAttemptCounter()
    {
        $date = date("Y-m-d H:i:s");
        $bind = [
            #':ip' => $ip,
            #':username' => $username,
            ':attempt' => $date
        ];
        $this->db->run('UPDATE compat_users_attempts SET counter = counter + 1, last_attempt = :attempt WHERE id = 1', $bind);
        return true;
    }

    public function recordFailedLogin()
    {
        #$attemptExists = $this->getLoginAttempts($ip, $username);
        /*if(!$attemptExists) {
            $this->addLoginAttempt($ip, $username);
        } else {*/
            if($this->updateLoginAttemptCounter()) {
                return true;
            }
        #}
    }

    public function getLoginAttempts($ip, $username) // Get user login attempts
    {
        $bind = [
            ':username' => $username,
            ':ip' => $ip,
        ];
        return $this->db->selectOne('compat_users_attempts','id = 1', $bind);
    }

    public function resetLoginAttempts($username)
    {
        $bind = [':username' => $username];
        $this->db->run('UPDATE compat_users_attempts SET counter = 0 WHERE username = :username', $bind);
        return true;
    }

    public function addLoginAttempt($ip, $username)
    {
        $date = date("Y-m-d H:i:s");
        $insert = [
            'ip' => $ip,
            'username' => $username,
            'counter' => 1,
            'last_attempt' => $date
        ];
        $this->db->insert('compat_users_attempts', $insert);
        return true;
    }

Thanks very much.

Assuming your $this has persistence across a session…

$(this)->login_attempts = $(this)->login_attempts ? $(this)->login_attempts++ : 1;
if ($(this)->login_attempts > 5) { exit(); }

where are you getting login_attempts from? thanks

i’m inventing it. You can invent things.

Yes i can see that, i just can’t see how it ties in with my question and current code. I had a fixed system last time where if 5 attempts wrong, 15 minute lockout. I now have moved over to a foreach so i can have multiple levels of throttling.

3 => X minutes,
5 => X minutes
10 => 1 hour blocked from site, suspicious email sent and account locked for reactivation etc.

My issue is that the code is not quite there yet and cannot see why the minutes are not returning and also why it is affecting the sql counter.

Your code is simple yet effective but does not meet my current needs. Thanks very much tho.

Then you’ve asked for the wrong thing, because you asked to add basic throttling.

You also said you cant spot the issue, but you didn’t bother to say that your symptoms are

or

So… lesson 1 in asking for help: Give ALL the information, and ask for what you actually want.

Let’s take the first of your symptoms. The minutes. What exactly do you get back from the function throttleLogin? It must be a number.

That said, where in your currently listed code do you call throttleLogin?

if ($counter === $throttle_key) // attempts more than or equal to throttle array attempts

That is not what that line of code says.

I’m going to assume you’ve put a record into your SQL table that contains the ID 1 for testing purposes. What value is in last_attempt and counter for that record? Just so i can trace what should be happening given the expected input.

The second symptom: The counter doesnt work.

Your login failure code doesn’t call recordFailedLogin, it calls updateLoginAttemptCounter.
You don’t validate that the update command actually ran correctly (I dont know what controller you’re using for db, but it should implement something like… affected_rows or at least return a True when you run a command, just to say ‘nothing went wrong’)
You should put field delimiters (usually backticks) around your field and table names. ‘counter’ is a very common word…
Other than that, the structure looks… okay, so i’m not sure why you’d have problems with that.

Thanks and sorry. I will word it better: note: i have removed the ip address to make fixing this easier.

Ok so the issue is that when i have the line of code below on, the database update query no longer increases the counter but continues to update the last_attempt and regarding the minutes, it does not seem to return those either. It’s frustrating especially when i cannot see anything. Nothing returns other than 0.

            $remaining = $this->throttleLogin($data['formData']['username']);
            if ($remaining) {
                $data['formData']['global_err'] = $remaining;
            }

This is the code and somewhere in here there is an issue.

    public function login() 
    {
        $getSettings = $this->start->getSettings(); // Load site settings
        $reCaptcha = new reCaptcha('login', 'REDACTED', 'REDACTED'); // load recaptcha class for spam protection

        #$locateUser = !startHelper::isBot($_SERVER['HTTP_USER_AGENT']) ? $this->start->getUserLocation(startHelper::getIP()) : false;  // Get user location data

        $always = [
            'getSettings' => $getSettings,
            'helpRecaptcha' => $reCaptcha,
        ];

        if ($_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['g-recaptcha-response'])) // Check form submitted via POST
        {
            $_POST = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING); // Clean $_POST data

            $formData = [
                'username' => trim($_POST['username']),
                'password' => $_POST['password'],

                'username_err' => '',
                'password_err' => '',
                'global_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'always' => $always,
                'title' => LANG['auth-login-title'],
                'description' => '',
            ];

            if (empty($data['formData']['username'])) { // Check Username not empty
                $data['formData']['username_err'] = LANG['auth-login-invalid1'];
            }
            if (empty($data['formData']['password'])) { // Check Password not empty
                $data['formData']['password_err'] = LANG['auth-login-invalid3'];
            }
            if (!$reCaptcha->success()) { // Check anti-spam reCaptcha.
                $data['formData']['global_err'] = LANG['auth-register-invalid20'];
            }
            /*if (!empty($data['formData']['username']) && !empty($data['formData']['password']) && ($remaining = $this->throttleLogin($data['formData']['username']))) {
                $data['formData']['global_err'] = $remaining;
            }*/
            /* ----THIS HERE CAUSES THE DB NOT TO UPDATE THE COUNTER VALUE---- $remaining = $this->throttleLogin($data['formData']['username']);
            if (!$remaining) {
                $data['formData']['global_err'] = $remaining;
            }*/

            if (empty($data['formData']['username_err']) && empty($data['formData']['password_err']) && empty($data['formData']['global_err'])) {
                $userAuthenticated = $this->auth->doLogin($data['formData']['username'], $data['formData']['password']);
                if ($userAuthenticated) {
                    echo 'success';
                    #$this->createUserSession($userAuthenticated);
                } else {
                    $this->auth->recordFailedLogin($formData['username']);
                    echo 'fail';
                }
            } else {
                $this->view('user/login', $data);
            }
            $this->view('user/login', $data);
        } else {
            $formData = [
                'username' => '',
                'password' => '',

                'username_err' => '',
                'password_err' => '',
                'global_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'always' => $always,
                'title' => LANG['auth-login-title'],
                'description' => '',
            ];

            $this->view('user/login', $data);
        }       
    }
    
    private function throttleLogin($username)
    {
        /* $throttle array, you should probably put this with the other Settings */
        $throttle = [
            1 => 8, //8 minute delay after 2 bad attempts
            3 => 10,
            9 => 'captcha', //captcha or ban account or send suspicous email etc.
            10 => 'lock', // lock account and send email to user saying suspicous activity
        ];
       
        $getSettings = $this->start->getSettings();
        $attemptExists = $this->auth->getLoginAttempts($username);

        if(!isset($attemptExists)) { return 0; }
       
        /* Read the "counter" value from the attempt record */
        $counter = $attemptExists['counter'];
       
        /* Now we calculate the $lockout using the $throttle values */
        $lockout = 0;
       
        foreach ($throttle as $throttle_key => $throttle_value)
        {  
            if ($counter === $throttle_key) // counter === to the thottle count
            {
                if (is_numeric($throttle_value)) {  // if number array (minutes)
                    $lockout = 60 * $throttle_value; //by x minutes
                    return $lockout; // return value for calculations
                } elseif ($throttle_value === 'captcha') {
                    /* Here you should send the captcha etc. */
                    die('here, we would send an email and lock ur account');
                }
            }
        }

        $last_attempt = strtotime($attemptExists['last_attempt']);
        $since_last_attempt = time() - $last_attempt;
        $seconds_remaining = $lockout - $since_last_attempt;
        $minutes_remaining = ceil($seconds_remaining / 60);
        if($seconds_remaining <= 0) {
            $this->auth->resetLoginAttempts($username);
            return 0;
        }
        return $minutes_remaining;
    }

Thanks and that is literally all the code. It is no longer the database code. That all works perfectly, it is just the controller code that is somewhere causing an issue which i cannot spot so hoping the experts such as yourself will be able to help.

Thanks

thanks for removing the recaptcha details oops :expressionless:

Returning 0 is something, not nothing :wink:

If the code aborts your counter when you uncomment a line that begins with an “if”… the if returned something you wernt expecting. So.
if ($remaining) {
$data[‘formData’][‘global_err’] = $remaining;
}
implies that $remaining was something other than false.

Verify what you’re getting back by uncommenting just the line that sets $remaining and then var_dump($remaining).

Ye. :slight_smile:

I have found where that is tho:

        $last_attempt = strtotime($attemptExists['last_attempt']);
        $since_last_attempt = time() - $last_attempt;
        $seconds_remaining = $lockout - $since_last_attempt;
        $minutes_remaining = ceil($seconds_remaining / 60);
        if ($seconds_remaining <= 0) {
            $this->auth->resetLoginAttempts($username);
            return 0; // change to 5 and it returns 5
        }
        return $minutes_remaining;

That if statement, when editing the return 0 value to 5, 5 was then var_dumped so there is something not quite right i just cannot see where that is.

It is just very strange how it stops certain elements of the database queries working. e.g. when the $remaining if statement is commented out, the counter works fine but when it’s back then the counter no longer works but the last_attempt continues to update. Just strange.

Also you can test the code by creating a fake array maybe that will give you a further insight but this is killing me. Thanks for you help so far.

So. Your code has fallen down to resetting the login attempts. Which sets the counter to 0.
Your code then says that if something other than false came back, skip attempting the login and display the login view (because $data['formData']['global_err'] is no longer empty).

If your code follows that path, the counter should be 0 at the end of the execution, regardless of what it was before.

Also I cant test the code because I dont have the framework you’re using to define things like $this, and auth, and db. So you’re the one who’s gotta do the debugging :stuck_out_tongue:

EDIT: Also, just… for sanity check… you are testing this by giving a BAD password, right? Cause otherwise you’ll just… log in.

1 Like

Ok, i am removing the global_err code and just gonna do a simple echo with die();

and ye don’t worry. entering the wrong password. :slight_smile:

Sometimes we’ve gotta ask the silly questions juuuuust to make sure :wink:

1 Like

Ye. This code is killing me.

Ok so literally 0 just returns.

I have tried setting the counter to 9 to trigger the captcha. Still no luck. (the die())

In order to get any results i have to put an !$remaining now.

            $remaining = $this->throttleLogin($data['formData']['username']);
            if(!$remaining) {
                #print_r($remaining);
                echo $remaining; // always 0
            }

This is properly killing me.

Ok so when the code is like this: the counter works perfectly: 1, 2, 3 etc.

As soon as renable the $remaining if statement everything breaks (or same issues as before). Something is wrong. The code makes perfect logic to me. Does it make sense to you?

… and then when $remaining is turned back on, it won’t go any higher than 1.

Because you’ve locked yourself out. You have confused your code actually WORKING for it failing. Sort of. More work to do though.

So here’s what happens.

Counter = 0. Last_attempt = 0 (sake of argument, consider ‘now’ to be 100, cause i dont want to do timestamp maths)
throttleLogin starts.
lockout = 5.
None of the keys match.
last_attempt is 0.
100-0 = 100
seconds_remaining = 5-100 = -95
minutes_remaining = -1
seconds_remaining <= 0 is True
Reset loginAttempts. Counter is now 0.
Return 0
Remaining = 0;
If is false
Login attempt parses, and fails.
Counter is set to 1, last_attempt is now 100.

Next iteration.
Now = 110, Counter = 1, last_attempt = 100
throttleLogin starts.
lockout = 5.
Key matches 1 => 8.
throttle_value is numeric.
lockout = 480
return 480. (This is wrong, you’re supposed to return the number of minutes, not seconds, according to your later code)
remaining is 480
if is true (480 is not false)
Flag the global error
Skip the login attempt
Display login screen.

(Note that at no point there did I say increment the counter. So the counter is still at 1, and it will remain at 1 forever, because every iteration from now on will do the key check first.)

Fix: Move everything starting with $last_attempt = down to the end of the seconds_remaining if (Basically your whole code block from post #10 except for the last line) up to before your foreach. That way it’ll check the time condition first, rather than the counter condition.

by moving that whole block tho, you make other code redundant?

Thanks as well for the amount of time you have put in helping fix my issue.

You missed the words ‘except for the last line’ in my last post :stuck_out_tongue: leave the return $minutes_remaining; down at the bottom

my bad :slight_smile:

so final code would be?

    public function login() 
    {
        $getSettings = $this->start->getSettings(); // Load site settings
        $reCaptcha = new reCaptcha('login', '', ''); // load recaptcha class for spam protection

        #$locateUser = !startHelper::isBot($_SERVER['HTTP_USER_AGENT']) ? $this->start->getUserLocation(startHelper::getIP()) : false;  // Get user location data

        $always = [
            'getSettings' => $getSettings,
            'helpRecaptcha' => $reCaptcha,
        ];

        if ($_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['g-recaptcha-response'])) // Check form submitted via POST
        {
            $_POST = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING); // Clean $_POST data

            $formData = [
                'username' => trim($_POST['username']),
                'password' => $_POST['password'],

                'username_err' => '',
                'password_err' => '',
                'global_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'always' => $always,
                'title' => LANG['auth-login-title'],
                'description' => '',
            ];

            if (empty($data['formData']['username'])) { // Check Username not empty
                $data['formData']['username_err'] = LANG['auth-login-invalid1'];
            }
            if (empty($data['formData']['password'])) { // Check Password not empty
                $data['formData']['password_err'] = LANG['auth-login-invalid3'];
            }
            if (!$reCaptcha->success()) { // Check anti-spam reCaptcha.
                $data['formData']['global_err'] = LANG['auth-register-invalid20'];
            }
            /*if (!empty($data['formData']['username']) && !empty($data['formData']['password']) && ($remaining = $this->throttleLogin($data['formData']['username']))) {
                $data['formData']['global_err'] = $remaining;
            }*/
            $remaining = $this->throttleLogin($data['formData']['username']);
            if($remaining) {
                #print_r($remaining);
                echo $remaining;
            }

            if (empty($data['formData']['username_err']) && empty($data['formData']['password_err']) && empty($data['formData']['global_err'])) {
                $userAuthenticated = $this->auth->doLogin($data['formData']['username'], $data['formData']['password']);
                if ($userAuthenticated) {
                    echo 'success';
                    #$this->createUserSession($userAuthenticated);
                } else {
                    $this->auth->recordFailedLogin($formData['username']);
                    echo 'fail';
                }

            } else {
                $this->view('user/login', $data);
            }
            $this->view('user/login', $data);
        } else {
            $formData = [
                'username' => '',
                'password' => '',

                'username_err' => '',
                'password_err' => '',
                'global_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'always' => $always,
                'title' => LANG['auth-login-title'],
                'description' => '',
            ];

            $this->view('user/login', $data);
        }       
    }
    
    private function throttleLogin($username)
    {
        /* $throttle array, you should probably put this with the other Settings */
        $throttle = [
            1 => 8, //8 minute delay after 2 bad attempts
            3 => 10,
            9 => 'captcha', //captcha or ban account or send suspicous email etc.
            10 => 'lock', // lock account and send email to user saying suspicous activity
        ];
       
        $getSettings = $this->start->getSettings();
        $attemptExists = $this->auth->getLoginAttempts($username);

        if(!isset($attemptExists)) { return 0; }
       
        /* Read the "counter" value from the attempt record */
        $counter = $attemptExists['counter'];
       
        /* Now we calculate the $lockout using the $throttle values */
        $lockout = 0;

        $last_attempt = strtotime($attemptExists['last_attempt']);
        $since_last_attempt = time() - $last_attempt;
        $seconds_remaining = $lockout - $since_last_attempt;
        $minutes_remaining = ceil($seconds_remaining / 60);
        if ($seconds_remaining <= 0) {
            $this->auth->resetLoginAttempts($username);
            return 0;
        }
       
        foreach ($throttle as $throttle_key => $throttle_value)
        {  
            if ($counter === $throttle_key) // counter === to the thottle count
            {
                if (is_numeric($throttle_value)) {  // if number array (minutes)
                    $lockout = 60 * $throttle_value; //by x minutes
                    return $lockout; // return value for calculations
                } elseif ($throttle_value === 'captcha') {
                    /* Here you should send the captcha etc. */
                    die('here, we would send an email and lock ur account');
                }
            }
        }

        return $minutes_remaining;
    }

except now i’ve reversed the issue and destroyed the lockout timer because checking the timer first requires determining what the timer SHOULD be first. Okay. Let me think about this for a second.

What’s the correct sequence of events.

Login Begins
Check if User is Locked Out:
  If the user has no entry in the login_attempts table, create one.
  Retrieve the data from the login_attempts table for this user.
  If the user has a counter equal to 0 or is nonexistant (Shouldnt happen): Return 0
  Determine number of seconds for lockout
  Determine timer status:
  If Timer Has Expired:
     Set Counter to 0
     Return 0
  Else:
     Return minutes remaining on timer

How does this differ from your code…

Ok i’m a complete derp. Return your code to the order you had it in, but remove the line return $lockout;.

EDIT: Also, you ‘fixed’ my comment about that line not reading what you thought it read in post #6 by changing the comment. The point was that it should be a >= , not a ===