2
\$\begingroup\$

Here is my code for a class that is responsible for managing some common users task like registration, login and logout. It implements also a sub class that can be used to check the session of a logged in user. I've tested it and works fine, but I will improve it as soon as I can. Any suggestion on security improvements are appreciated.

<?php
require_once 'config.php';

Interface UserInterface{

  public function createUser(array $args);
  public function loginUser(array $args);
  public function logoutUser();

}


class User Implements UserInterface{

    private $db;

    private $stmt;

    private $email;

    private $username;

    private $password;

    private $id;

    private $sessioncode;

    private $args;


    /*
    * @param $db must be a PDO instance
    *
    */
    public function __construct(\PDO $db){
        $this->db = $db;
    }

    /*
    * @param $args must be a key/value array
    *
    */
    public function createUser(array $args){

        if($this->checkEmail($args['email'])){
            #header("HTTP/1.1 400 Email already exsist");
            #http_response_code(400);
            echo 'Email address already exsist.';
        }
        elseif($this->checkUsername($args['username'])){
            #header("HTTP/1.1 400 Username already exsist");
            #http_response_code(400);
            echo 'Username already exsist.';
        }
        else {
            $this->password = password_hash($args['password'], PASSWORD_BCRYPT);
            $stmt = $this->db->prepare('INSERT INTO _users (email, username, password) VALUES (?, ?, ? )');
              if($stmt->execute(array($args['email'],$args['username'],$this->password))){
                echo 'Account successful created';
            }
        }
    }

    /*
    * @param $args must be a key/value array
    *
    */
    public function loginUser(array $args){

        $stmt = $this->db->prepare('SELECT id,username,password FROM _users WHERE username = ?');
        $stmt->execute(array($args['username']));
        $result = $stmt->fetch(PDO::FETCH_OBJ);
                if(count($result) > 0 && password_verify($args['password'], $result->password)){
                  UserSessionHelper::setSession($result->username, $result->id);
                    #header('HTTP/1.1 200');
                    echo 'Logged in';
                }
                else {
                    echo 'Wrong username or password';
                    #header('HTTP/1.1 400');
                }
    }

    /*
    * This method wehn called will logout an user
    *
    */
    public function logoutUser(){
        UserSessionHelper::unsetSession();
        header('HTTP/1.1 200');
        #header('Location: ');
        #echo 'Logged out';
    }

    /*
    * @param $email is a key part of the $args array;
    * This method will check if a given email is already registered.
    */
    private function checkEmail($email){

        $stmt = $this->db->prepare('SELECT email FROM _users WHERE email = ?');
            $stmt->execute(array($email));
            $result = $stmt->fetch(PDO::FETCH_OBJ);
              if(count($result) > 0){
                return true;
              }
    }

    /*
    * @param $username is a key part of the $args array;
    * This method will check if a given username is already registered.
    */
    private function checkUsername($username){

        $stmt = $this->db->prepare('SELECT username FROM _users WHERE username = ?');
            $stmt->execute(array($username));
            $result = $stmt->fetch(PDO::FETCH_OBJ);
              if(count($result) > 0){
                return true;
              }
    }

}

interface UserSessionHelperInterface{

  public static function unsetSession();
  public static function setSession(string $username, int $user_id);
  public static function validateSessionID(string $session_id, string $session_hash);

}

class UserSessionHelper implements UserSessionHelperInterface{

    private $session_hash;
    private $username;
    private $user_id;

    /*
    * @params $username must be a string, $user_id must be an integer.
    * This method will register the $_SESSION variables when an user login.
    */
    public static function setSession(string $username,int $user_id){

        $_SESSION['session_'] = self::sessionHash();
        $_SESSION['id_'] = $user_id;
        $_SESSION['username_'] = $username;
        return true;
    }

    /*
    * @param
    * This method will remove all $_SESSION data wehn an user logout.
    */
    public static function unsetSession(){

        session_destroy();
        session_unset();
    }

    /*
    * @params $session_id must be a valid string, $session_hash must be a valid string.
    * This method will check for valid session credentials when an user is logged in.
    */
    public static function validateSessionID(string $session_id,string $session_hash){

        $computed_session_hash = hash('sha384', $session_id);

          if(!preg_match('/^[-,a-zA-Z0-9]{1,128}$/', $session_id) > 0){
              #return header('HTTP/1.1 403');
          }
          elseif(!hash_equals($computed_session_hash, $session_hash)){
              #return header('HTTP/1.1 403');
          }
          else{
              return true;
          }
    }

    /*
    * This method is responsable to hash the regenerated session id, then return it
    *
    */
    private function sessionHash(){

        session_regenerate_id();
        $session_hash = hash('sha384', session_id());
        return $session_hash;
    }

}


?>
\$\endgroup\$
  • \$\begingroup\$ Would validateSessionID() not get $session_id and $session_hash from session_id() and $_SESSION['session_']? Why have them as method arguments, if this is the case? \$\endgroup\$ – KIKO Software Oct 30 '18 at 8:10
2
\$\begingroup\$
  • For session management, you might want to take a look at SessionHandlerInterface. Generally, your class can handle it all, but further on - you can set custom session data storage, like Memcache or Redis.

  • It seems like you're using php7(param typehints), but no return types. Consider doing that too. For instance, UserSessionHelper::sessionHash can be declared as function sessionHash() : string

  • Class User sounds like a model class, while it's actually kind of a repository/manager. Consider re-naming.

  • It is considered a good practice to separate the resource handler logic and business logic, e.g. queries for DB and the user management. What I would do, is move the DB logic to a separate class, UserStore, for instance,

     class UserStore {
        public function isEmailUnique(string $email) : bool {
            // check in DB
        }
    
        public function isEmailUnique(string $username) : bool {
            // check in DB
        }
    
        public function create($email, $username, $password): UserModel {
    
        }
     }
    

    and inject it to User class. So that the UserManager class could looks something like this:

    class UserManager {
    
        /**
         * @var UserStore
         */
        protected $store;
    
        public function __construct(UserStore $store) {
            $this-store = $store;
        }
    
        /**
         * @param array $args
         * @return UserModel
         */
        public function createUser(array $args): UserModel {
            if (!$this->store->isEmailUnique($args['email'])) {
                throw new \LogicException('Email address already exsist');
            }
    
            if (!$this->store->isUsernameUnique($args['username'])) {
                throw new \LogicException('Username is already taken.');
            }
    
            return $this->store->create(
                $args['email'], 
                $args['username'], 
                password_hash($password, PASSWORD_BCRYPT)
            );
        }
     }
    

Hope that helps.

\$\endgroup\$
  • 2
    \$\begingroup\$ Thanks for formatting :) @your-common-sense \$\endgroup\$ – t1gor Oct 30 '18 at 9:21

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.