3
\$\begingroup\$

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
<?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

<?php
include_once('connect.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);
    $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

<?php
session_start();
error_reporting(E_ALL);
ini_set('display_errors', '1');


$errorMsg = '';
$email = '';
$pass = '';
$hash="";
$id='';
$token='';
$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);
            $hash=$data['pass'];

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

            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['token'] = $token;

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

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

//logout.php

<?php
session_start();
include_once 'clases/manageUsers.php';

if(isset ($_SESSION['id']) && isset ($_SESSION['username']) && isset ($_SESSION['token'])){
$id=$_SESSION['id'];
    $LgOut = new ManageUsers();
    $loggingOut = $LgOut->logOut($id);
    if($loggingOut > 0){
    session_destroy();
    session_regenerate_id(TRUE);
    header ('location: login.php');
    }else{
    echo 'Not Logged Out';
    }
}
?>
\$\endgroup\$
  • \$\begingroup\$ $pass = strip_tags($pass); => This invalidates passwords like eSD<eV3>u \$\endgroup\$ – Marek Jun 24 '14 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 '14 at 13:45
  • \$\begingroup\$ Please don't edit your code based on the suggestions we give. That invalidates them. \$\endgroup\$ – Madara Uchiha Jun 24 '14 at 13:49
  • \$\begingroup\$ @MadaraUchiha please check the update of my script and review please. \$\endgroup\$ – Abandoned Account Jun 24 '14 at 14:37
3
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ Dont you think if(something == true) is a good practise? \$\endgroup\$ – Abandoned Account Jun 24 '14 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\$ – Madara Uchiha Jun 24 '14 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 '14 at 16:32

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.