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
$db_password = $hash = password_hash($pass, PASSWORD_BCRYPT, array("cost" => 10)); 
// store in the 'users' table only the hash of the password provided by user.

// clases/manageUsers.php


class ManageUsers{
public $link;
public $hash;   

function __construct(){
    $db_connection = new dbConnection();
    $this->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);
    $data = $query->fetch(PDO::FETCH_ASSOC);        
    return $data;       

            return TRUE;                
            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);
    $countsOut = $query->rowCount();
    return $countsOut;      


// login.php

ini_set('display_errors', '1');

$errorMsg = '';
$email = '';
$pass = '';
$token = md5(uniqid(rand(),TRUE));

if (isset($_POST['e'])) {   
$email = $_POST['e'];
$pass = $_POST['p'];

$email = stripslashes($email);
$pass = stripslashes($pass);
$email = strip_tags($email);
$pass = strip_tags($pass);

if ((!$email) || (!$pass)) { 
    //header("location: index.php");
    $errorMsg = 'Please fill in both fields';

} else { 
    include_once 'clases/manageUsers.php'; 
    include_once 'clases/connect.php'; 

$loginUsers = new ManageUsers();
$data = $loginUsers->LogIn($id,$email,$pass,$hash);

    if(password_verify($pass,$hash) == TRUE){

        $db = new dbConnection();// <- New connection to database to update members table
        $dbh = $db->connect();   //  with new login data. I think this is WRONG...


            $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);

                $_SESSION['id'] = $data['id'];
                $_SESSION['username'] = $data['username'];
                $_SESSION['token'] = $token;

            header("location: profile.php?id=$id"); 

    } else { 
            header("location: login.php");
            $errorMsg = "Incorrect login data, please try again";


include_once 'clases/manageUsers.php';

if(isset ($_SESSION['id']) && isset ($_SESSION['username']) && isset ($_SESSION['token'])){
    $LgOut = new ManageUsers();
    $loggingOut = $LgOut->logOut($id);
    if($loggingOut > 0){
    header ('location: login.php');
    echo 'Not Logged Out';
  • \$\begingroup\$ $pass = strip_tags($pass); => This invalidates passwords like eSD<eV3>u \$\endgroup\$
    – Marek
    Jun 24, 2014 at 12:55
  • \$\begingroup\$ You are right. I just want to 'filter' a little the input, but using prepared statements it's useless. And limiting the password characters, may result in weak passwords. Thank you. \$\endgroup\$
    – remux -
    Jun 24, 2014 at 13:45
  • \$\begingroup\$ Please don't edit your code based on the suggestions we give. That invalidates them. \$\endgroup\$ Jun 24, 2014 at 13:49
  • \$\begingroup\$ @MadaraUchiha please check the update of my script and review please. \$\endgroup\$ Jun 24, 2014 at 14:37

1 Answer 1

  • 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.

  • \$\begingroup\$ Dont you think if(something == true) is a good practise? \$\endgroup\$ Jun 24, 2014 at 14:40
  • \$\begingroup\$ @HassanAlthaf Not really because it's exactly the same as if (something). If it were if (something === true) it would have been a different story. \$\endgroup\$ Jun 24, 2014 at 15:40
  • \$\begingroup\$ @Madara Uchiha - Thank you for answer. I'll read more about classes, review the code, i'll make the changes (structure and style) and i will post the new format when will be ready. \$\endgroup\$
    – remux -
    Jun 24, 2014 at 16:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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