1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.