Building a User class in PHP, making it more secure

Question

I'm building a User class from scratch, for both learning experience, and for a work project. I needed to learn more about login security, and this class I created was built from several examples from around the web, then updated to the tune of the criticisms of the discussions and other documents.

My msqli_object is not part of this class, but I included it for testing. It's actually part of my MyApp_DB class, but because the [lack of] security of this class is probably the biggest hole in my code, I wanted it here. In other words, I know that it needs to be separated from my User class. I didn't include SessionManager class, because I'm not finished building it.

The class... "works", but I have several problems in my code and I feel like I'm building a house with broken blocks:

1. I do not have mysqlnd, so I do not have access to mysqli_stmt::get_result(). In order to keep the mysqli object as a member of a class and not a global (and thus difficult to locate) variable, I had to use eval() as a workaround until I could figure out how to send members of an array as a set of arguments. Any help there would be awesome.
2. I don't know how secure this code is overall. I'm destroying the $hash variable as soon as it's used up, and my eval() line does not use any user input. I don't think I understand the password_verify() function well enough to know that I'm using it correctly
3. Code documentation... I have always documented my code, but only for myself. The project I'm working on now will have several people working on it with me, and I need to be able to document cleanly. I'm installing PHPdoc, but I haven't really figured out the whole tagging system just yet.
4. Code structure. Any criticism on my structure to make it easy to read is welcome

5. Logic-- Are my logic and methodology even close to reasonable?

   class User
   {
   
       public $user_id;
       public $first_name;
       public $last_name;
       public $username;
       public $email;
       public $ip_address;
   
       public $is_logged_in = false;
       public $errors = false;
       public $login_type;
       private $session_id = false;
       private $mysqli;
   
       public function __construct()
       {
           $this->ip_address = $_SERVER["REMOTE_ADDR"];
   
           //borrowed from SessionManager class, needed for testing
           $this->mysqli = new mysqli(MYAPP_DB_HOST, MYAPP_DB_USER, MYAPP_DB_PW, MYAPP_DB_NAME);
   
       }
   
       public function __destruct()
       {
           //clean up... necessary?
           unset($this->mysqli);
       }
   
       /**
        * Comes from my SessionManager class, but here for testing and convenience
        */
       private function _db_select($table, $cols, $conds, $params)
       {
           $results = false;
   
           $query  = "SELECT "
                   . implode($cols, ", ")
               . " FROM "
                   . $table
               . " WHERE "
                   . implode($conds, " ")
                   . " LIMIT 1";
   
           if ( $stmt = $this->mysqli->prepare($query) )
           {
               foreach ($params as $param)
               {
                   $stmt->bind_param($param[0], $param[1]);
               }
   
               $stmt->execute();
   
               //Use the parameters to bind results, in leu of msql_stmt::get_result()
               //Be sure there is no user data here from $params! Better way to do this?
               eval('$stmt->bind_result($results["' . implode($cols, '"], $results["') . '"]);');
   
               $stmt->fetch();
               $stmt->close();
           }
   
           return $results;
   
       }
   
       /**
        * Populate the user class with an actual user
        */
       public function getByUserName($username)
       {
   
           $this->username = $username;
   
           $user_data = $this->_db_select(
               "users",
               ["user_id", "first_name", "last_name", "email",],
               ["username =?",],
               [ [ "s", $username ], ]);
   
           foreach($user_data as $key => $value)
           {
               $this->{$key} = $value;
           }
       }
   
       /*
        * Separate logon function, so User can still be used as a tool admin
        * passing by reference &SessionManager class as parameter
        */
       public function logon($user_submit_password, $session_mgr)
       {
           $user_check = $this->_db_select(
               "users",
               ["hash",],
               ["username =?",],
               [ [ "s", $this->username ], ]);
   
           if ( !$session_mgr::get_session())
           {
               if ( password_verify($user_submit_password, $user_check["hash"]) )
               {
                   $this->login_type = "form_submit";
                   $this->is_logged_in = $session_mgr::start_session($this->username, $this->ip_address, $this->mysqli);
               }
   
               else
               {
                   $this->error("Login Error.", "The information you entered is not valid. Please try again, or contact the system administrator.");
               }
           }
   
           else
           {
               $this->login_type = "session";
   
               //returns true if the ip address in DB for the given session matches the user's ip address
               if ( $session_mgr::check_session() )
               {
                   $this->is_logged_in = true;
               }
   
               else
               {
                   $this->error("Session Expired.", "Your session is not valid, please log in again.");
               }
           }
   
           //get rid of password hash so it can't be dumped (does this do anything, really?)
           unset($user_check->hash);
   
           return $this->is_logged_in;
   
       }
   
       //Plan to add more functionality here
       private function error($name = "Unknown Error.", $message = "An unknown error occurred processing your request. Please try again later.", $target = false)
       {
   
           array_push($this->errors, ["name" => $name, "message" => $message, "target" => $target]);
   
       }
   
   }
   

To sum up, what mistakes have I made building this User class? I tried to actually write as much of it as I could, based on what I know, borrowing the logic, but not the code from different places around the web. It works, in the sense that I don't get any errors, but I want to learn to write more cleanly and learn standards.

Answer 1

Are my logic and methodology even close to reasonable?

Unfortunately, no.

public function __construct()

Imagine you are going to list 100 users on a page. How many mysql connections will be created by your script?

public function __construct($mysqli) {
    $this->mysqli = $mysqli;

it have to be.

private function _db_select($table, $cols, $conds, $params)

Two problems with this one.

1. Your User is not an SQL driven relational database. You cannot run an SQL query against a user. A user can only use a database service. thus there should be no such function in the User class at all.
2. Honestly, whole function is A MESS. You are trying to save yourself typing of just few words, namely SELECT, FROM and WHERE. And for this you are making an ugly gibberish out of the beautiful and readable SQL. Not to mention that the SQL subset supported by this function is ridiculously small.

Take some advice; make a function that runs an arbitrary SQL instead of this Frankenstein. Or look for a ready made queryBuilder.

To use PDO will be also a good idea, because with PDO you will need no evil eval or other dirty tricks.

Here is your function rewritten with PDO:

public function getByUserName($username)
{
    $sql = "SELECT user_id, username, first_name, last_name, email FROM users WHERE username=?";
    $stmt = $this->pdo->prepare($sql);
    $stmt->execute([$username]);
    $stmt->setFetchMode(PDO::FETCH_INTO, $this);
    $stmt->fetch();
}

Answer 2

Create an abstract db class using PDO. That is probably your biggest mistake you've done using mysql_*. Read more about pdo and implement it.

Also you can read more about some basic project structure. You have models/services, controllers and repositories. The idea of the repositories is to communicate with the database. You write your queries there. No other place. So what you can do is create another class called for example UserRepository where you talk to the user table in your database. What you can do in the construct is pass an array and create an UserRepository object. Based on the array keys, for example you can pass id / username and password. Then based on them you either populate only the object (select the user by id) or log him in and populate the object using the private function logon.

Another cool thing you can do is throw exceptions. No need to always check the returns of your functions, they must either return the right thing or nothing at all. So in the logon function you could throw an exception if you can't authenticate the user so that nothing will go wrong after that.

Another mistake is setting every field to public, like username, email, userId etc. Just overwrite the magic method __get and use it.

You can start off with this. I think it will be a good practice.