How to check if user is banned before logging in with PHP

Question

I currently have a login function which works fine however i'm looking to add something into the function so it also checks if the users status is 1.

1 = banned

0 = unbanned

function:

public function doLogin($uname,$umail,$upass)
    {
        try
        {
            $stmt = $this->conn->prepare("SELECT user_id, user_name, user_email, user_pass, status FROM users WHERE user_name=:uname OR user_email=:umail ");
            $stmt->execute(array(':uname'=>$uname, ':umail'=>$umail));
            $userRow=$stmt->fetch(PDO::FETCH_ASSOC);
            if($stmt->rowCount() == 1) 
            {
                if(password_verify($upass, $userRow['user_pass']))
                {
                    $_SESSION['user_session'] = $userRow['user_id'];
                    return true;
                }
                else
                {
                    return false;
                }
            }
        }
        catch(PDOException $e)
        {
            echo $e->getMessage();
        }
    }

login.php

if(isset($_POST['btn-login']))
{
    $uname = strip_tags($_POST['txt_uname_email']);
    $umail = strip_tags($_POST['txt_uname_email']);
    $upass = strip_tags($_POST['txt_password']);

    if(empty($uname) || empty($umail) || empty($upass)){
            $error = "Please enter all fields";
    }


    if($login->doLogin($uname,$umail,$upass))
    {
        $success = "Logged in successfully, redirecting..";
        header( "refresh:3;url=debits" );
        //$login->redirect('debits');
    }
    else
    {
        $error = "Incorrect username or password";
    }   
}

I tried doing if(password_verify($upass, $userRow['user_pass']) && $userRow['status'] == 0) and I think this works however when I try testing with a banned account, I still receive the error message Incorrect username or password when i'd prefer this to be Your account has been banned.

Answer 1

I suggest you return array that contains both booleans of correct password and banned status of the user. Now when validating, check the password correct boolean to know whether user entered correct password or otherwise display incorrect email or password as you are already doing. Now if password is correct you can then check the status boolean to know if user is banned or not and send an appropriate error message.

public function doLogin($uname,$umail,$upass)
{
    try
    {
        $stmt = $this->conn->prepare("SELECT user_id, user_name, user_email, user_pass, status FROM users WHERE user_name=:uname OR user_email=:umail ");
        $stmt->execute(array(':uname'=>$uname, ':umail'=>$umail));
        $userRow=$stmt->fetch(PDO::FETCH_ASSOC);
        if($stmt->rowCount() == 1) 
        {
            if(password_verify($upass, $userRow['user_pass']))
            {
                $_SESSION['user_session'] = $userRow['user_id'];
                return ["correctPass"=>true,"banned"=> ($userRow['status']== 1) ? true : false];
            }
            else
            {
                return ["correctPass"=>false];
            }
        }
    }
    catch(PDOException $e)
    {
        echo $e->getMessage();
    }
}

login.php

if(isset($_POST['btn-login']))
{
    $uname = strip_tags($_POST['txt_uname_email']);
    $umail = strip_tags($_POST['txt_uname_email']);
    $upass = strip_tags($_POST['txt_password']);

    if(empty($uname) || empty($umail) || empty($upass)){
        $error = "Please enter all fields";
    }


    $validation = $login->doLogin($uname,$umail,$upass))
    if($validation["correctPass"])){
        if($validation["banned"]){
            $error = "User has been banned";
        }else{
            $success = "Logged in successfully, redirecting..";
            header( "refresh:3;url=debits" );
            //$login->redirect('debits');
        }
    }
    else{
        $error = "Incorrect username or password";
    }   
}

Answer 2

Ideally, every method of a class should have a single purpose. As you can see you are trying to get your method doLogin() to:

• fetch a row from the database
• verify the username and password combination
• verify the user is enabled/disabled
• set a session variable

You also want to keep each method small, in particular public methods. Usually when you're over just a few lines of code, you might want to break some of the functionality off into another method.

And one recommended way to do form validation with multiple return types is to throw an Exception containing the error message and then catch it later when you want to handle the error.

Might I instead suggest a bit of refactoring? (note, this is untested and may contain syntax errors)

public function doLogin($uname,$umail,$upass)
{
    $user = $this->fetchUserFromDatabase($uname, $umail);
    if ($user !== false || $this->isBanned($user)) {
        throw new Exception('This account has been banned');
    }
    elseif ($user === false || !$this->validatePassword($upass, $user)) {
        throw new Exception('Invalid username or password combination');
    }
    $this->startSession($user['user_id']);
    return true;
}

private function fetchUserFromDatabase($uname, $umail)
{
    $stmt = $this->conn->prepare("SELECT user_id, user_name, user_email, user_pass, status 
        FROM users
        WHERE user_name=:uname 
        OR user_email=:umail ");
    $stmt->execute(array(':uname'=>$uname, ':umail'=>$umail));
    return $stmt->fetch(PDO::FETCH_ASSOC);
}

private function isBanned(Array $user)
{
    return $user['status'] == 1;
}

private function validatePassword($upass, Array $user)
{
    return password_verify($upass, $user['user_pass'])
}

private function startSession($user_id)
{
    $_SESSION['user_session'] = $user['user_id'];
}

Then in your other file:

if(isset($_POST['btn-login']))
{
    $uname = strip_tags($_POST['txt_uname_email']);
    $umail = strip_tags($_POST['txt_uname_email']);
    $upass = strip_tags($_POST['txt_password']);

    if(empty($uname) || empty($umail) || empty($upass)){
        $error = "Please enter all fields";
    }

    try {
        $login->doLogin($uname,$umail,$upass);
        $success = "Logged in successfully, redirecting..";
        header( "refresh:3;url=debits" );
    }
    catch (Exception $e) {
        $error = $e->getMessage();
    }
}

On another note, you may want to consider getting PDO errors to throw Exceptions instead of try/catch blocks for every single query. Doing it how you have it gets super tedious if you have more than just a couple of queries. You can then set a global exception handler to handle any PDOException thrown.