Secure login and authentication in PHP

Question

This seems to work for me, but I want to make it more secure. I want to see how far I can go without pre-built packages/bundles, so please don't suggest any.

How secure is this? What are some steps I can take to improve it?

In a controller:

public function loginUser($request, $response) {
 if (Security::isUser()) { return $this->redirect('home'); }
 $username = $request->getParam('username');
 $password = $request->getParam('password');
 $user = Security::authenticate($username, $password);
 if ($user) {
 Security::login($user->id);
 return $this->redirect('home');
 } else {
 $this->flash->addMessage('error', 'Invalid username and/or password.');
 return $this->redirect('login');
 }
}

Here is part of the Security class:

public static function hash($string) {
 return password_hash($string, PASSWORD_DEFAULT);
}
public static function authenticate($username, $password) {
 $user = User::where('username', $username)->orWhere('email', $username)->first();
 if (password_verify($password, $user->password) && $user->banned === 0) { return $user; }
 else { return false; }
}
public static function getUser() {
 if (isset($_SESSION['uid'])) {
 $user = User::where('id', $_SESSION['uid'])->first();
 if ($user->banned === 1) { return false; }
 else { return $user; }
 } else { return false; }
}
public static function isUser() {
 if (!self::getUser()) { return false; } else { return true; }
}
public static function isAdmin() {
 $user = self::getUser();
 if (!$user) { return false; }
 else if ($user->role === 'admin') { return true; }
 else { return false; }
}
public static function login($uid) {
 session_regenerate_id();
 $_SESSION['uid'] = $uid;
}
public static function logout() {
 session_unset();
 session_destroy();
}

Then, to secure something:

public function loginUser($request, $response) {
 if (!Security::isUser()) { // or use ::isAdmin()
 // deny access
 }
 // allow access
}

Csrf tokens are checked by Slim's Guard. This is all over https.

Answer 1

This isn't a matter of security, but mashing things onto a single line

else { return $user; }

is considered poor form in most style guidelines.

---

public static function isUser() {
 if (!self::getUser()) { return false; } else { return true; }
}

reads a bit more naturally if you don't use an inverted condition:

public static function isUser() {
 if (self::getUser()) { return true; } else { return false; }
}

And then it's easy to see that you can simplify it to just

public static function isUser() {
 return (bool)(self::getUser());
}

---

Similarly,

else if ($user->role === 'admin') { return true; }
else { return false; }

can be simplified to

return $user->role === 'admin';

---

$user = User::where('username', $username)->orWhere('email', $username)->first();

is a bit strange. Users can log in via either email or username? Are they both unique? It will simplify your life (and theirs) a lot if there's only one unique identifier for each user.

---

You're using password_hash and password_verify, which is good. You may want to consider setting the cost parameter to password_hash to something other than the default to get something that makes sense for your hardware.