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.