PHP OOP Login Class

Question

This is a login class I made for my personal website.

What I don't care about, is how secure this login class is, because I know it's not. But it keeps out the rare unwanted guests because I do not want to have my information public. I made this mainly for learning purposes. I want to know how "OOP" this is and what I could do differently.

Login Form:

    if($_SERVER['REQUEST_METHOD'] == 'POST')
    {
        $user       = $_POST['username'];
        $pass       = $_POST['password'];

            if($login->userLogin($user, $pass))
            {
                echo "Successfully logged in!";
            }
            else {
                echo $login->getError();
            }
    }

Login Class:

class UserLogin
{
    private $id;
    private $user;
    private $pass;
    private $salt;
    private $hash;
    private $error;

    protected $mysqli;

    public function __construct()
    {
        $this->mysqli = mysqliSingleton::init();
    }



    function setError($val)
    {
        $this->error = $val;
    }



    function getError()
    {
        return $this->error;
    }



    public function userLogin($user, $pass)
    {
        if($this->checkUser($user) && $this->checkPass($pass))
        {
            $_SESSION['memberID']   = $this->id;
            $_SESSION['memberName'] = $this->user;

            return true;
        }
        else {
            $this->setError("Invalid username or password");
        }
    }



    public function checkUser($user)
    {
        $stmt = $this->mysqli->prepare("SELECT memberID, userName, salt, hash FROM members WHERE userName = ? LIMIT 1");

        $stmt->bind_param("s",$user); 
        $stmt->execute();
        $stmt->bind_result($this->id, $this->user, $this->salt, $this->hash);

        if (null === ($stmt->fetch()))
        {
            return false;
        }
            return true;
    }



    public function checkPass($password)
    {
        return (hash_hmac("sha256", $password, $this->salt) === $this->hash);
    }
}

Answer 1

Madara Uchiha sums up some problems.

Here are some more:

Too many if conditions

A methods containing multiple if's smells. It makes testing harder since there are more edge cases. It also creates a mess where none should be.

For instance your getError method. A better approach would be:

function getError()
{
    return $this->error;
}

And if you want it to return false if no error has been set:

private $error = false; //defaults to false

This makes it a private variable (as it should be) instead of it being added on run-time (what you are doing now).

Then, your userLogin method also contains a lot of if's. And no, you are giving away to much info. Don't tell what was wrong (username or password). Only tell that the combination is wrong. This way, you don't give away any information. So:

public function userLogin($user, $pass)
{
    if( $this->checkUser($user) && $this->checkPass($pass) )
    {
        $_SESSION['memberName'] = $this->user;
        return true;
    }
    else
    {
        $this->setError("Invalid username/password");
    }
}

I also change return(true) to return true. What you are doing is evaluating true. Pointless.

Only take what you need

Every time you write SELECT *, a cute small kitten dies. You only need the userName, hash and salt.

SELECT userName, hash, salt FROM members ...

Then you should add some parameter binding spices and your all good to go!

Then, what happens after is weird. We know that if a user exists, only 1 should. So we can add a LIMIT 1 statement to the SQL. This way, the database will stop searching once it has found 1 member instead of continuing to search the entire members table.

After that simply doing this:

if ( null === ($userData = $result->fetch_assoc()) )
{
    return false;
}

$this->user = $userData['userName'];
$this->user = $userData['salt'];
$this->user = $userData['hash'];

return true;

is cleaner, better to read and does the exact same thing.

Your checkPass function has the same problem: you evaluate something to a boolean. Then you evaluate that boolean in the if-condition and then you return an evaluated boolean. A better method is:

public function checkPass($password)
{
    return (hash_hmac("sha256", $password, $this->salt) === $this->hash);
}

And that is the code review.

OO review

Your code is OO, you are using Objects and stuff and the class keyword. But I think what you are asking is: is it good OO?

No it isn't

A good place to start is SOLID. Starting with Single responsibility.

Every class should have one single responsibility. Is it authentication? or is it retrieving user-data? or it it connection to a database? Your class does a little bit of all three.

A good place to start when writing OO-code is in starting with your controllers first. Here you should write how you would like to react with you application. for instance:

if ( $auth->login($username, $password) )
{
    print 'welcome master';
}
else
{
    print $auth->getError();
}

Here we know that we want an interface with 2 methods. login and getError()

interface Authentication
{
    public function login($uuid, $pass);
    public function getError();
}

and then implement that interface. For instance a UserAuthentication class that authenticates using users. Later on you could change that with a LDAPAuthentication class without having to worry that the rest of your app breaks. As long as the interface is the same ;)

Answer 2

Yes, there are several things I can see with your implementation, from most critical to less critical:

• SQL Injection - You has it! Since you're already using mysqli, learn about prepared statements with mysqli and apply them. You are very vulnerable to Little Bobby Tables.
• Your password hashing is not secure - sha256 is insecure for password hashing in on itself, you're also apply a $this->salt which isn't defined anywhere. See password_hash() and password_verify() for better, more secure password hashing, which includes iteration over the hash, use of proper hashing algorithms (such as CRYPT_BLOWFISH), and simpler and more readable UI.

• Your class does too many things - Why is it the business of a login class, that's supposed to handle users, to create a database connection instance? Why not just pass it in?

  public function __construct(mysqli $databaseConnection)
  

  This way, if you ever change your credentials or even move to a different server, you won't need to change your class's code, but only the code which is responsible for creating the connection. That makes your class reusable.

• Naming conventions - It's common to use CapitalCase for class and object names, and camelCase for variables and functions.

Answer 3

A couple of things I smell:

• Returning multiple values at once (aka returning a "Tuple") means your methods won't need as many side-effects.
  • In PHP, you accomplish this by returning an array and using the list function to assign the return value to multiple variables at once.
• Keep your method implementations to a Single Level of Abstraction (Clean Code, p. 36).
  • For example, in your "userLogin" method, you're using both higher-level abstractions (checkUser() and checkPass()) and low-level abstractions ($_SESSION[]).
  • To fix this, use the Extract Method refactor.

For example:

//Login form:
...
list($success, $error) = $login->userLogin($user, $pass);
echo $success ? 'Success!' : $error;
...


//Login class:
...
/**
 * Attempts a user login.
 * Side effects: if successful, saves the current user in $_SESSION.
 * @return A tuple of [ $successful:BOOLEAN, $error:string ]
 */
public function userLogin($user, $pass) {
    if ($this->checkUser($user) && $this->checkPass($pass)) {
       $this->somethingThatSetsUpTheCurrentSessionMember();
       $successful = TRUE;
       $error = NULL;
    } else {
       $successful = FALSE;
       $error = 'Invalid username or password';
    }
    return array($successful, $error);
}
...

Answer 4

The LIMIT 1 on your query looks suspicious. LIMIT is rarely useful without ORDER BY, and you didn't specify ORDER BY. Have you either made the username column the primary key of the members table, or put a UNIQUE constraint on the column? If you haven't already done so, I recommend it, to let the database help you enforce data integrity.