Classes for user registration and authentication

Question

<?php
namespace Users;

use mysqli;

class Users {
    protected $db;
    public function __construct() {
        $this->db = new mysqli('localhost', 'root', 'php123', 'sms');
    }
    public function login($username, $password) {
        $query = $this->db->prepare("SELECT username, password FROM users WHERE username=? AND password=? LIMIT 1");
        $password = UserData::hash($password);
        $query->bind_param('ss', $username, $password);
        $query->execute();
        $query->bind_result($username, $password);
        $query->store_result();
        if($query->num_rows ==1) {
            while($query->fetch()) {
                $_SESSION['loggedin'] = 1;

                UserData::getAndSet($username);

                header("Location: dashboard.php");
            }
        } else {
            return false;
        }
        $query->free_result();
        $query->close();
    }
    public function register($username, $password, $rank, $name) {
        $errors = array();
        $query = $this->db->prepare("INSERT INTO users VALUES('', $username, $password, $rank, $name);");

        $password = UserData::hash($password);

        $query->bind_param('ssss', $username, $password, $rank, $name);

        if(strlen($username) < 3 || strlen($username) > 15) {
            $errors[] = "The length of the username can only range from 3 to 15 characters.";
        }
        if(strlen($password) < 3 || strlen($password) > 15) {
            $errors[] = "The length of the password can only range from 3 to 15 characters.";
        }
        if(strlen($name) < 3 || strlen($name) > 15) {
            $errors[] = "The length of the name can only range from 3 to 15 characters.";
        }

        $query->execute();
        $query->close();
    }
}
class UserData {
    protected $db;

    public $username;
    public $password;
    public $rank;
    public $name;

    public function __construct() {
        $this->db = new mysqli('localhost', 'root', 'php123', 'sms');
    }

    public function getAndSet($username) {
        $query = $this->db->prepare("SELECT username, password, rank, name FROM users WHERE username=?");
        $query->bind_param('s', $username);
        $query->execute();
        $query->bind_result($username, $password, $rank, $name);
        $query->store_result();
        while($query->fetch()) {
            $this->username = $username;
            $this->password = $password;
            $this->rank     = $rank;
            $this->name     = $name;
        }
        $query->free_result();
        $query->close();
    }
    public function hash($string) {
        $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-";
        $sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+";

        $sting = md5(md5($fSalt) . md5($string) . md5($sSalt));

        for($i=0; $i < 3; $i++) {
            $string = md5($string);
            $string = sha1($string);
            $string = bin2hex($string);
            $string = md5($string);
        }

        return $string;
    }
}
class UserUtil {
    protected $db;
    public function __construct() {
        $this->db = new mysqli('localhost', 'root', 'php123', 'sms');
    }
}

$Users    = new Users();
$UserData = new UserData();
$UserUtil = new UserUtil();

?>

This is the beginning of my project, "Student Management System" and before I proceed I want this code to be rated, and how can I improve this code with better ways.

Answer 1

• Why are you starting a new database connection for each class? Pass in a database connection object to the constructor:

   public function __construct(mysqli $dbConnection) {
  

  This allows your class to work without worrying about actually doing the connection. The class's job is to handle users, not handle database connections!

• You not only said LIMIT 1 in your query, but you also check that $query->num_rows is 1, and then you even loop over the results (even though there's only one). This sort of triple check is redundant and unreadable. You know there's either 1 or 0 results because you LIMITed the query!

• Your hashing algorithm is not secure. Use PHP 5.5's password_hash(), or if you don't use 5.5 (And you should!!!) use Password Compat, a library written by the same guy who wrote the password_hash() function for PHP's core.

• Do not limit your users' password length - Setting a minimum length is good. Setting a maximum length is a kitten killing crime. Don't.

• Your login() method returns false on error, but nothing on success. Either return true on success, or simply throw an exception on errors.

• Your UserData class is not a class, it's a namespace. You only use it for static functions (even though you don't declare them as static in your class!).

• What does your getAndSet method does? It's not clear. Use a proper name or add documentation.

• Your UserUtil class does absolutely nothing.

---

Generally, I'd say you can improve the structure of your code by having the following 3 classes:

• UserService - is responsible for registering, authenticating and verifying users using the following two objects. Basically higher level management.

• User - Represents a single user in the system. Has absolutely no database interaction whatsoever, and has no idea where it came from. It accepts parameters through the constructor and getters/setters:

   class User {
       public function __construct($id, $username, $hashedPassword, $name) {
  

• UserMapper - Which is responsible for mapping and/or creating user objects to/from the database. For example

   class UserMapper {
       public function fetch(User $user) {
           //$user object will already have the ID filled in. Fetch according to that and fill the same $user object.
       }
   }
  

Example of execution:

class UserService {

    private $mapper;
    
    public function __construct(UserMapper $mapper) { ... }

    public function register($username, $password, $name) {
        $user = new User($username, $password, $name);
        $user->validateRegistrationDetails(); //Throws ValidationException on validation error.
        $user->generateHash();
        $this->mapper->save($user); //Save user to database;
    }
}

Note that in the above, the User is unaware of the database, which means it's perfectly reusable across applications with different storage systems.

Answer 2

1. Database connections all over the place.
2. Make your code modular.
3. Instead of using a lot of classes in one file, make one class per file, then include them where needed.
4. Try to write code that conforms to loose-coupling.

The following code has not been tested but should give some insight into how to structure your code, more or less (just some structure to ponder about).

<?php

ini_set("display_error", "1");
error_reporting(E_ALL);

interface IConnectInfo {

    const HOST = "localhost";
    const UNAME = "root";
    const DBNAME = "sms";
    const PW = "php123";

    function testConnection();
}

class ConSQL implements IConnectInfo {

    // Passing values using scope resolution operator
    private $server = IConnectInfo::HOST;
    private $currentDB = IConnectInfo::DBNAME;
    private $user = IConnectInfo::UNAME;
    private $pass = IConnectInfo::PW;

    public function testConnection() {

        $hookup = new mysqli($this->server, $this->user, $this->pass, $this->currentDB);

        if (mysqli_connect_error()) {
            die("bad mojo");
        }

        echo "You're hooked up Ace! <br>". $hookup->host_info;

        $hookup->close();
    }
}

class Users extends ConSQL {

    public function login($username, $password) {
        $query = $this->db->prepare("SELECT username, password FROM users WHERE username=? AND password=? LIMIT 1");
        $password = UserData::hash($password);
        $query->bind_param('ss', $username, $password);
        $query->execute();
        $query->bind_result($username, $password);
        $query->store_result();
        if($query->num_rows ==1) {
            while($query->fetch()) {
                $_SESSION['loggedin'] = 1;

                UserData::getAndSet($username);

                header("Location: dashboard.php");
            }
        } else {
            return false;
        }
        $query->free_result();
        $query->close();
    }
    public function register($username, $password, $rank, $name) {
        $errors = array();
        $query = $this->db->prepare("INSERT INTO users VALUES('', $username, $password, $rank, $name);");

        $password = UserData::hash($password);

        $query->bind_param('ssss', $username, $password, $rank, $name);

        if(strlen($username) < 3 || strlen($username) > 15) {
            $errors[] = "The length of the username can only range from 3 to 15 characters.";
        }
        if(strlen($password) < 3 || strlen($password) > 15) {
            $errors[] = "The length of the password can only range from 3 to 15 characters.";
        }
        if(strlen($name) < 3 || strlen($name) > 15) {
            $errors[] = "The length of the name can only range from 3 to 15 characters.";
        }

        $query->execute();
        $query->close();
    }
}

class UserData extends ConSQL {

    public $username;
    public $password;
    public $rank;
    public $name;

    public function getAndSet($username) {
        $query = $this->db->prepare("SELECT username, password, rank, name FROM users WHERE username=?");
        $query->bind_param('s', $username);
        $query->execute();
        $query->bind_result($username, $password, $rank, $name);
        $query->store_result();
        while($query->fetch()) {
            $this->username = $username;
            $this->password = $password;
            $this->rank     = $rank;
            $this->name     = $name;
        }
        $query->free_result();
        $query->close();
    }
    public function hash($string) {
        $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-";
        $sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+";

        $sting = md5(md5($fSalt) . md5($string) . md5($sSalt));

        for($i=0; $i < 3; $i++) {
            $string = md5($string);
            $string = sha1($string);
            $string = bin2hex($string);
            $string = md5($string);
        }

        return $string;
    }
}

$Users    = new Users();
$Users->testConnection();
$Users->login(); 
$Users->register();

$UserData = new UserData();
$UserData->testConnection();

$UserUtil = new UserUtil();
$UserUtil->testConnection();


?>

Get this book, which gave me some valuable insights into thinking in an OOP way.