patternphpModerate
PHP OOP Login Class
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:
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) =
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:
-
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?
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
- 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 -
sha256is insecure for password hashing in on itself, you're also apply a$this->saltwhich isn't defined anywhere. Seepassword_hash()andpassword_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.