patternphpMinor
My first login class in PHP with PDO and bcrypt
Viewed 0 times
withphploginfirstbcryptandclasspdo
Problem
This is the first time using a class. Please review this and tell me if it's secure and if it's the right way to do it. The code itself is working, but I have doubts in the way I used all this.
```
// register.php
10));
....
// store in the 'users' table only the hash of the password provided by user.
?>
// clases/manageUsers.php
link = $db_connection->connect();
return $this->link;
}
public function LogIn($id,$email,$pass,$hash){
$query = $this->link->prepare("SELECT id,username,email,pass FROM users WHERE email = :email AND activated = 'y' LIMIT 1");
$query->bindParam(':email', $email, PDO::PARAM_STR);
$query->execute();
$data = $query->fetch(PDO::FETCH_ASSOC);
return $data;
if(password_verify($pass,$hash)){
return TRUE;
}else{
return FALSE;
}
}
public function logOut($id){
$query=$this->link->prepare("UPDATE users SET verify = '0', activenow = 'n' WHERE id= :id LIMIT 1");
$query->bindParam(':id', $id, PDO::PARAM_INT);
$query->execute();
$countsOut = $query->rowCount();
return $countsOut;
}
}
?>
// login.php
LogIn($id,$email,$pass,$hash);
$hash=$data['pass'];
if(password_verify($pass,$hash) == TRUE){
$db = new dbConnection();// connect(); // with new login data. I think this is WRONG...
session_regenerate_id(TRUE);
$id=$data['id'];
$username=$data['username'];
$query = $dbh->prepare("UPDATE users SET verify= :token, lastlog=NOW(), activenow='y' WHERE id= :id LIMIT 1");
$query->bindParam(':id', $data['id'], PDO::PARAM_INT);
$query->bindParam(':token', $token, PDO::PARAM_STR);
$query->execute();
$_SESSION['id'] = $data['id'];
$_SESSION['username'] = $data['username'];
$_SESSION
```
// register.php
10));
....
// store in the 'users' table only the hash of the password provided by user.
?>
// clases/manageUsers.php
link = $db_connection->connect();
return $this->link;
}
public function LogIn($id,$email,$pass,$hash){
$query = $this->link->prepare("SELECT id,username,email,pass FROM users WHERE email = :email AND activated = 'y' LIMIT 1");
$query->bindParam(':email', $email, PDO::PARAM_STR);
$query->execute();
$data = $query->fetch(PDO::FETCH_ASSOC);
return $data;
if(password_verify($pass,$hash)){
return TRUE;
}else{
return FALSE;
}
}
public function logOut($id){
$query=$this->link->prepare("UPDATE users SET verify = '0', activenow = 'n' WHERE id= :id LIMIT 1");
$query->bindParam(':id', $id, PDO::PARAM_INT);
$query->execute();
$countsOut = $query->rowCount();
return $countsOut;
}
}
?>
// login.php
LogIn($id,$email,$pass,$hash);
$hash=$data['pass'];
if(password_verify($pass,$hash) == TRUE){
$db = new dbConnection();// connect(); // with new login data. I think this is WRONG...
session_regenerate_id(TRUE);
$id=$data['id'];
$username=$data['username'];
$query = $dbh->prepare("UPDATE users SET verify= :token, lastlog=NOW(), activenow='y' WHERE id= :id LIMIT 1");
$query->bindParam(':id', $data['id'], PDO::PARAM_INT);
$query->bindParam(':token', $token, PDO::PARAM_STR);
$query->execute();
$_SESSION['id'] = $data['id'];
$_SESSION['username'] = $data['username'];
$_SESSION
Solution
-
Your class manages Users. Its job is to handle users. So connecting to a database is not its job! Inject the database connection to the class like so:
-
Return values from
-
When you
-
-
A user's logged in status should normally not be kept in the database, but in a session.
-
There's no point in initializing variables with
-
Using
-
-
If you determined that the class's responsibility is to manage the authentication of your users, all of the authentication logic should be in that class. It's pointless if you do it outside the class :)
-
What's the point of a token in a session? A token is usually used in a cookie for the purpose of "remember me". It has no meaning in a session.
-
-
Coding style, spacing and indentation -
Your class manages Users. Its job is to handle users. So connecting to a database is not its job! Inject the database connection to the class like so:
public function __construct(\PDO $connection) {-
Return values from
__construct() are discarded when used from new Object.-
When you
return, the function ends. So in your LogIn method anything after return $data won't run. Basically, you won't verify the password, and $data will almost always mean true when checked against a boolean. (Basically, you are accepting any password to an existing account).-
password_verify() returns a boolean, you can simplify to:return password_verfiy($pass, $hash);-
A user's logged in status should normally not be kept in the database, but in a session.
-
There's no point in initializing variables with
'' if you're immediately overwrite those values with other literals. (It only makes sense if you append data to them in a loop or something similar).-
Using
stripslashes() and strip_tags means that passwords with certain characters in them would be changed. You don't care about a password having HTML tags because you are not displaying that password as HTML, ever.-
if (SOMETHING == true) can be simplified to if (SOMETHING).-
If you determined that the class's responsibility is to manage the authentication of your users, all of the authentication logic should be in that class. It's pointless if you do it outside the class :)
-
What's the point of a token in a session? A token is usually used in a cookie for the purpose of "remember me". It has no meaning in a session.
-
"Not logged out" is not really a good error message. Explain what went wrong.-
Coding style, spacing and indentation -
functionNames() and $variableNames should be camel cased, ClassNames should be camel caps. Indent your lines according to the structure block they are found in. Sometimes you use param,param,param and sometimes param, param, param. Pick one and stick with it.Code Snippets
public function __construct(\PDO $connection) {return password_verfiy($pass, $hash);Context
StackExchange Code Review Q#55122, answer score: 3
Revisions (0)
No revisions yet.