HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpModerate

PHP OOP Login Class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
phploginclassoop

Problem

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) =

Solution

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.

Code Snippets

public function __construct(mysqli $databaseConnection)

Context

StackExchange Code Review Q#58609, answer score: 15

Revisions (0)

No revisions yet.