2
\$\begingroup\$

After a little bit of troubleshoots, I've finished this simple users registration and login class. It uses PDO to connect to a mysql database, I've tried also to manage the session when an user do the login or logout. The main objective of this code form me was to make an exercise for using correctly the PDO and related functions, and to start improving my coding skills to manage account creations and login using PHP OOP.

<?php
reqiure_once 'config.php';
class User{

private $db = null;
private $email;    
private $username;   
private $password;
private $uid;
private $sessioncode;        

public function __construct(\PDO $db){

    $this->db = $db;
}    

public function createUser($email,$username,$password){

    if($this->checkEmail($email)){
        echo 'Email address already exsist.';
    } elseif($this->checkUsername($username)){
        echo 'Username already exsist.';
    } else {
        $this->password = password_hash($password,PASSWORD_BCRYPT);
        $stmt = $this->db->prepare('INSERT INTO users (email,username,password) VALUES (?, ?, ? )');      
        if($stmt->execute(array($email,$username,$this->password))){
        echo 'Account successful created';     
        }       
    } 
}    

public function loginUser($username,$password){

    $stmt = $this->db->prepare('SELECT id,username,password FROM users WHERE username = ?');
        $stmt->execute(array($username));
            if($stmt->rowCount() > 0){
                $result = $stmt->fetch(PDO::FETCH_OBJ);
                if(password_verify($password, $result->password)){
                $this->setSession($result->username,$result->id);

            } else {
                echo 'wrong password';
                }
            } else {
                echo 'Username or email does not exist in the database.';
            }     
}    

public function logoutUser($uid){

    $this->unsetSession($uid);    
}

private function checkEmail($email){

    $stmt = $this->db->prepare('SELECT email FROM users WHERE email = ?');
        $stmt->execute(array($email));
        if($stmt->rowCount() > 0){
        return true;
        } else {
        return false;    
        }
}    

private function checkUsername($username){

    $stmt = $this->db->prepare('SELECT username FROM users WHERE username = ?');
        $stmt->execute(array($username));
        if($stmt->rowCount() > 0){
        return true;
        } else {
        return false;    
        }        
}    

private function setSession($username,$uid){

    $this->sessioncode = bin2hex(rand());
    $_SESSION['id'] = $uid;    
    $_SESSION['username'] = $username;
    $_SESSION['session_code'] = $this->sessioncode;
        $stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
        if($stmt->execute(array($this->sessioncode,$uid))){
        header('Location: u/dashboard.php');    
    }   
}    

private function unsetSession($uid){

    $this->uid = $uid;
    $this->sessioncode = '';
    $stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
        if($stmt->execute(array($this->sessioncode,$this->uid))){
        session_unset();
        session_destroy();
        header('Location: ../index.php');    
    }    
}

}
?>
\$\endgroup\$
  • \$\begingroup\$ I've got a little suggestion for checkEmail function. as of the rest, did you actually try to implement this class in the actual code? All those echos look terribly misplaced. Where such an output is supposed to be shown? \$\endgroup\$ – Your Common Sense Jun 3 '18 at 11:18
  • \$\begingroup\$ @YourCommonSense I wrote echos because for now I didn't write the AJAX code to request the various class taskes such createUser ecc. so I need a simply output to show when something was wrong. I don't know if can be a good idea to sobstitute echos with return for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echos will be appreciated. I'm also planning to add a method to send verification email, but for now I'm evaluting on how to proceed. \$\endgroup\$ – user9741470 Jun 3 '18 at 11:24
  • \$\begingroup\$ Like I suggested before, the best would be to throw a custom exception, then catch it in the AJAX handling code (outside the class). There is an explanation i wrote in the other question (ider Error reporting section) codereview.stackexchange.com/questions/193023/… \$\endgroup\$ – Your Common Sense Jun 3 '18 at 11:33
  • 1
    \$\begingroup\$ yes, it's a mistake. like any other tool, try catch has its use, and this is a perfectly legitimate case for it. All other ways to check for error would be more tedious and your current approach is just no-go. Please read my article on MVC, it explains, why a code in Model should never output a single character - you just cannot tell in which context it will be used in the future. \$\endgroup\$ – Your Common Sense Jun 3 '18 at 12:04
  • 1
    \$\begingroup\$ as of rowCount() for me it's a matter of sanity: you are selecting some data but not fetching it. For me, it makes no sense. At the same time rowCount() is not guaranteed to be available. After all, fetch() makes uniform processing \$\endgroup\$ – Your Common Sense Jun 3 '18 at 14:08
2
\$\begingroup\$

Whole user management system is very hard to do properly even with structural code. When you get a grasp of entire logic & security nuances OOP itself takes it to the next level, because this logic will be hard to encapsulate in objects of separate concerns - session, cookies, http request and response, domain models, view models, factories, routing... potential abstraction leaks are everywhere. That said, let's take a look at your class...

From OOP perspective

Output:

This class is not concerned about output - you don't know its format (json/html) nor channel/destination (http/console/log file) - you will validate passed values against your data model, but nothing more. For example when registering new user, the response should be the same user data (withut password - not needed anymore) with user id or class specific validation errors attached like (bool) usernameConflict, (bool) emailConflict - that's it. The rest is up to clients of this class (they might not want to reveal users' emails in this indirect way)

Database:

You should separate yourself from database - it's not relevant to your application where data comes from. Use port & adapter technique - interface to query methods (taking only required parameters) and its PDO implementation.

Session:

Session at this point is a global dependency and hidden structural dependency (there needs to be session_start() somewhere). Remove it from this class and return what it needs instead. It should be something that provides user id & not only that. There will be other parties interested in authentication result, so return user data without password hash and other "secrets".

Handling hashes:

This is the only concern of your class: get credentials for concrete user (defined by username, email, cookie token id) and its hash signature (password hash, persistent cookie token, ...), compare result & return data useful to others - beyond this class hashes should be no use to anyone. You won't have methods covering entire login and logout procedure - just a part of it.

and side-effects...

There will be also some side-effect logic in this class like possible password rehashing on algorithm's change (after succesful match), removing cookie tokens (id+hash) on mismatch, hashing token (id+token is generated outside and passed as response cookie)... etc.

Some security concerns:

  • After succesful login session id should be regenerated (see: session fixation attack)
  • After failed login/registration - don't inform that same email is in database (mentioned in "output" section). Registering user with existing email might be handled by sending some (merge/restore) options in activation email.
  • Session code doesn't protect you from anything - it all happens inside application environment (only session id goes outside). Holding user's id for authentication (in case user is deactivated/removed) is enough
  • Make sure that httpOnly and Secure flags are set for session-type cookies (https might be hard to setup for localhost env, but it's neccessary on remote)
  • All forms should be signed/marked with CSRF tokens - new session id = generate new token is most convenient approach (every form with new token doensn't add much to protection, but causes problems with multiple open tabs, back button...)
  • More...
\$\endgroup\$
  • \$\begingroup\$ So you suggest to remove the session code and use instead the session id to identify the current active user session, plus renew the session_id() on success login right?About the output, I'm removing from the final version of the code all the echos, I want to use returns and in case of error as suggested I'm working to catch() the errors. \$\endgroup\$ – user9741470 Jun 5 '18 at 19:32
  • 1
    \$\begingroup\$ @user9741470 I suggested removing session handling from this class but implementing in upper layers (the class that directly/indirectly usues yours). session_id() is internal mechanism and it just works that way. I was saying that logged in user_id is enough to be saved in session storage (next to CSRF tokens) and session_regenerate_id() should be called right before that. \$\endgroup\$ – shudder Jun 5 '18 at 19:54
  • \$\begingroup\$ Perfect, I will follow your suggestions. \$\endgroup\$ – user9741470 Jun 6 '18 at 7:14
1
\$\begingroup\$

Early Returns

I prefer early returns for example

if(!$stmt->rowCount()){
    echo 'Username or email does not exist in the database.';
    return false;
}

$result = $stmt->fetch(PDO::FETCH_OBJ);

if(password_verify($password, $result->password) !== true){
    echo 'wrong password';
    return false;
}

$this->setSession($result->username,$result->id);

Session Ids

Its against best practise (as i understand it) to create your own session id you can very easily get the current sessions id by calling session_id()

\PDO Is Not Injectable (without custom definitions)

I like dependency injection (especially auto wiring) so in the future if you were to use auto wiring you would have to add a definition some where for the \PDO object so it might be better to put that pdo object in a class so you can get it in the constructor

public function __construct(database $database)
{
    $this->db = $database->dbObject; 
    // Or 
    $this->db = $database->getSomeObjectOrThrowSomeException()
}

but thats just me

The last thing id say which yourcommonsense has already pointed out dont echo in the model

\$\endgroup\$
  • 1
  • \$\begingroup\$ well you could have kept it, as it works for the OP. a good note on the injectability by the way \$\endgroup\$ – Your Common Sense Jun 4 '18 at 12:57
  • \$\begingroup\$ The id is the user id and not the session_id(). I use it to manage the user logout. But I will add also the session_id() to the informations stored into the database to make more secure the app. For the Dependency injection, I've tried to Inject a PDO object but without success, maybe it was a mistake of mine, so I've opted for this solution. See this question stackoverflow.com/questions/50641139/… \$\endgroup\$ – user9741470 Jun 4 '18 at 13:04
  • 1
    \$\begingroup\$ Yeah you wont be able to inject \PDO without a definition telling the dependency injection container which \PDO instance you want to use. if $this->sessioncode = bin2hex(rand()); is the uesr id you need to re-think your naming convention, you should just use the session id generated by php instead of this (imo) \$\endgroup\$ – Dan Jun 4 '18 at 13:38

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.