I would like to ask your advice on my simple code to login and registration sessions.
In the User class, login function:
public function login($username, $password){
$this->db->query("SELECT * FROM users WHERE username = :username AND status = :status LIMIT 1");
$this->db->bind(':username', $username);
$this->db->bind(':status', 1);
$row = $this->db->single();
$count = $this->db->rowCount();
if ($count > 0) {
if (password_verify($password, $row['password'])) {
$_SESSION['session'] = [
'id' => $row['id'],
'username' => $row['username'],
'email' => $row['email'],
];
return true;
} else {
return false;
}
}
}
check logged:
public function isLoggedIn() {
if (isset($_SESSION['session'])) {
return true;
}
}
And login page with check logged:
if ($user->isLoggedIn()) {
header('Location: index.php');
exit();
}
if(isset($_POST['login'])){
//Retrieve the field values from our login form.
$username = !empty($_POST['username']) ? trim($_POST['username']) : null;
$password = !empty($_POST['password']) ? trim($_POST['password']) : null;
if($user->login($username, $password)) {
header('Location: index.php');
exit();
} else {
$message[] = 'We found problems with the login, please try again.';
}
}
1 Answer 1
The could looks like it would work properly, and I think it's clearly written and easy to understand.
When you check that a user is logged in, you should check for something in the session that clearly states they are logged in. For instance you could check that $_SESSION['user']['loggedIn'] === true
. It's risky to simply test for isset($_SESSION['session'])
as another programmer, or even you in six months time, could write code to set that when the user is not logged in. Perhaps they will want to store a message in the session, like "sorry your password didn't match try again".
I would replace $_SESSION['session']
with $_SESSION['User']
. You might then want to only access $_SESSION['User']
from the User class of your application. Other parts of the application may want to store other things in the session - messages to display on next request, or a basket of goods, or whatever the app deals with.
I assume you have called session_start()
before the code we see runs.
Using password_verify() is good. Far too many apps implement their own insecure password algorithms.
I don't see the 'new' keyword, which is good - that means you may be doing dependency injection by having the constructor of User take a db as a parameter. If that's true you can easily write a test for the login function that uses a mock db.
login()
on line 100 it creates a new session. Innewsession()
(line 248 ff.) it creates a new session and saves it to the db. This session is tied to the user and IP address. Although session generation is not optimal (microtime()
is not a good example), it generates unique sessions and invalidates them, if they expire (checksession()
, line 370 ff.). I wouldn't recommend the whole class because of weak security but the base is very robust. \$\endgroup\$