PHP user class usability

Question

I'm new to PHP OOP programming and I'm trying to make a user class with login, register, logout and some other methods. I'm concentrating in the login method that will be used with Ajax.

Session class

class Session {
    // these are global variables defined in the config file
    private $lifetime = LIFETIME;
    private $path = PATH;
    private $domain = DOMAIN;
    private $secure = SECURE;
    private $http_only = HTTPONLY;


    public function __construct($name) {
        // method colled everytime an instance is called
        // $lifetime, $path, $domain, $secure, $http_only
        session_set_cookie_params($this->lifetime, $this->path, $this->domain, $this->secure, $this->http_only);
        session_name($name);
        session_start();
    }

    public function setVariable($name, $sv) {
        // Sets the session variable
        $_SESSION[$name] = $sv;
    }

    public function getVariables() {
        // gets the session array with all variables
        return $_SESSION;
    }

    public function getSingle($name) {
        // displays the session variable
        return $_SESSION[$name];
    }

    public function removeSessionVariable($name) {
        // removes session variable
        unset($_SESSION[$name]);
    }

    public function regenerate() {
        // regenerates the id of session
        session_regenerate_id();
    }

    public function killSession() {
        // kills the sessions.
        // to be used on logouts for example.
        $_SESSION = array();
        session_destroy();
    }
}

User class

class User extends Session {

    private $error = false;
    private $msg = '';
    private $fields = array();
    private $result = array();
    private $type = '';

    private $db;
    private $stmt;

    public function __construct($dbh) {
        $this->db = $dbh;
    }


    // verifies login form inputs
    public function verifyLoginFields($username, $password, $token, $bot, $sv) {
        if (strlen($username) < 3) {
            $this->error = true;
            array_push($this->fields, 'username');
        }

        if (strlen($password) < 3) {
            $this->error = true;
            array_push($this->fields, 'password');
        }

        if (strlen($bot) !== 0) {
            $this->error = true;
            array_push($this->fields, 'bot');
        }

        if ($token !== $sv) {
            $this->error = true;
            array_push($this->fields, 'token');
        }

        if ($this->error) {
            return false;
        } else {
            return true;
        }
    }

    // if no errors, proceed with login system
    public function login($username, $password, $token, $bot, $sv) {
        if($this->verifyLoginFields($username, $password, $token, $bot, $sv)) {
            $query = "SELECT id, username, password, role FROM users WHERE username = :un";
            $this->stmt = $this->db->prepare($query);
            $this->stmt->bindParam(':un', $username, PDO::PARAM_STR);
            if($this->stmt->execute()) {
                if($this->stmt->rowCount() == 1) {
                    $row = $this->stmt->fetch();
                    $pass = htmlentities($row['password'], ENT_QUOTES, 'utf-8');

                    if(!password_verify($password, $pass)) {
                        $this->error = true;
                        array_push($this->fields, 'password');
                        $this->msg = 'Existem erros no formul&aacute;rio!';
                        $this->type = 'error';
                    }
                    else {
                        $this->type = 'success';
                        $this->msg = 'Redirecting...';

                        $this->setVariable('username', htmlentities($row['username'], ENT_QUOTES, 'utf-8'));
                        $this->setVariable('id', htmlentities($row['id'], ENT_QUOTES, 'utf-8'));
                        $this->setVariable('role', htmlentities($row['role'], ENT_QUOTES, 'utf-8'));
                    }
                }
                else {
                    $this->error = true;
                    array_push($this->fields, 'username');
                    $this->msg = 'Existem erros no formulario!';
                    $this->type = 'error';
                }
            }
            else {
                $this->type = 'error';
                $this->msg = 'Fatal Error!';
            }
        }
        else {
            $this->type = 'error';
            $this->msg = 'Existem erros no formulário!';
        }

        // Sets a new token value
        $this->setVariable('token', bin2hex(random_bytes(32)));

        $this->result = [
            'type' => $this->type,
            'msg' => $this->msg,
            'fields' => $this->fields,
            'token' => $this->getsingle('token')
        ];

        echo json_encode($this->result);
    }
}

The way I'm using this is this:

include_once('includes/config.php');
include_once('includes/session.class.php');
include_once('includes/login.class.php');

$session = new Session('login');
if (!isset($_SESSION['token'])) {
    $session->setVariable('token', bin2hex(random_bytes(32)));
}

// $dbh comes from config file
$user = new User($dbh);

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    $username = $_POST['username'];
    $password = $_POST['password'];
    $token = $_POST['token'];
    $bot = $_POST['user'];

    $user->login($username, $password, $token, $bot, $session->getSingle('token'));
}

Right now, I'm only testing but this will be used with Ajax request. Am I on the right track? Any advice on how I can improve this class?

Answer 1

The main problem here is violation of the Single responsibility principle. A User class has too much responsibilities. It interacts with the client, verifies a CSRF token, decides what kind of response will be sent, etc. An top of that, User extends Session which is outright wrong. Classes must be related to each other. An Apple class could extend a Fruit class, because an apple is a fruit. But a user is not a session. That's completely different entities that have nothing in common. Moreover, they belong to different realms, a user belongs to the Business logic and a session belongs to the Client interaction. There are PHP applications that do not interact with a client, what would they do with a session?

The login() function should just return a boolean value, whereas all interactions with the client should be done elsewhere.

On a side note, you are seem under the impression that regular variables are not used in classes. Well, you are wrong. On the contrary, a variable that is used only in one method must be not a class variable, but just a regular variable. Such as $type, $msg, $stmt.

In the end, there should be a distinction between a code that does the business logic and a code that interacts with a client. And MVC pattern does it perfectly. According to it, all form verifications must be done in the Controller (I've got an article that explains MVC, you may find it helpful). Assuming your "I'm using this is this" code is sort of a Controller, all the client interaction should be moved there. So, login() method should accept only username and password, and verify them against a database like this (the code is taken from my canonical PDO authentication example):

class User
{
    private $db;

    public function __construct($dbh) {
        $this->db = $dbh;
    }
    public function login($username, $password)

    {
        $sql = "SELECT id, username, password, role FROM users WHERE username = ?";
        $stmt = $this->db->prepare($sql);
        $stmt->execute([$username]);
        $user = $stmt->fetch();

        if ($user && password_verify($password, $user['password']))
            return $user;
        }
    }
}

as you can see, a lot of useless and even harmful code is removed. For example,

• if($this->stmt->execute()) makes no sense at all as in case of error an exceptjion will be thrown and therefore this condition will never be evaluated to false.
• if($this->stmt->rowCount() == 1) is useless as well, as the fetched row could serve as such a flag
• htmlentities($row['password'] is just weird. There is not a single reason to do so, while it could do a severe harm changing the password so it will return false for the correct password.
• all client interaction including session handling are also removed for the reasons explained above.

So all the client interaction must be written in the controller. If you want to encapsulate the form verification routines, it should be a distinct class (as you are going to use it with other forms as well, aren't you?):

class Form()
{
    public function verify($sessionToken, $inputToken)
    {
        return hash_equals($sessionToken, $inputToken)
    }
}

and then finally you are ready to process the user input.

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    $user = new User($dbh);
    $form = new Form();

    if (!$form->verify($session->getSingle('token'), $_POST['token'])) {
        $result = [
            'type' => 'error',
            'msg' => 'Existem erros no formulário!',
            'token' => $this->getsingle('token')
        ];
        echo json_encode($result);
        exit;
    }

    if ($user = $user->login($username, $password)
    {
          $session->setVariable('username', ($user['username']);
          $session->setVariable('id', $user['id']);
          $session->setVariable('role', $user['role']);
          $result = [
              'type' => 'success',
              'msg' => 'Redirecting...',
              'token' => $this->getsingle('token')
          ];
          echo json_encode($result);
          exit;
    } else {
        // a response saying that login or password are incorrect
    }
}

This is not the full code as I don't know some of your internal considerations such as what is $this->fields or $bot but just to give you an idea.