HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

My first login class in PHP with PDO and bcrypt

Submitted by: @import:stackexchange-codereview··
0
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

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:

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.