First attempt at making a user class

Question

I've just begun creating my first user class. First, I need some clarifications.

Am I using try and catch correctly?

To clarify, the way I would be instantiating this object is like so:

try{
    $loggedInUser = new LoggedIn/User($email,$password,$useragent);
    $_SESSION['user'] = $loggedInUser;//now session holds user object
    die(json_encode('type'=>'success'=>'message'=>'account_fetch_view'));
}
catch(Exception $e) {
    unset($_SESSION['user']);
    die(json_encode(['type'=>'error','message'=>$e]));
}

Looking at my class, will I be able to do this as I've anticipated? If not, should I just construct it as I used to do (i.e. manually from the login processing script)?

namespace LoggedIn;
class User {
    public $email      = NULL;
    public $hash       = NULL;
    public $user_id    = NULL;
    public $user_agent = NULL;
    public $sign_in    = NULL;
    public $csrf_token = NULL;
    public $btc_addr   = NULL;
    public $throttle   = NULL;
    public $last_ajax  = NULL;
    public $verified   = NULL;
    public $reputation = NULL;

    function __construct($e_mail,$p_word,$u_agent)
    {
        if($this->check_pass($p_word) === true && $this->check_email($e_mail) === true)
        {
            $result = $this->fetch_details($e_mail,$p_word);
            if($result !== false && is_array($result) === true)
            {
                $this->email      = $result['u_email'];
                $this->hash       = $result['u_pass'];
                $this->user_id    = $result['u_id'];
                $this->user_agent = $u_agent;
                $this->sign_in    = time();
                $this->csrf_token = $this->csrf_create($this->sign_in,$this->hash);
                $this->btc_addr   = $result['u_addr'];
                $this->throttle   = 0;
                $this->last_ajax  = ($this->sign_in - 5);
                $this->verified   = $result['u_verified'];
                $this->reputation = $this->fetch_rep($this->user_id);
            }else{
                throw new Exception('Invalid Username or Password');
            }
        }
    }

    private function csrf_create($timestamp,$hash)
    {
            return hash('sha256', str_shuffle(openssl_random_pseudo_bytes(4) . $hash . $timestamp));
    }
    private function fetch_details($e_mail,$p_word)
    {

    }
    private function check_email($e_mail)
    {

    }
    private function check_pass($p_word)
    {

    }

    public function log_out()
    {

    }

    public function bid($amount,$auction)
    {

    }

    public function buy_now($amount,$auction)
    {

    }

    private function fetch_rep($u_id)
    {

    }
}

Answer 1

Here on Code Review, it's common to not receive some sort of feedback for a few hours. We all try our best to be on and answering!

---

I'm having a hard time deciding what you mean in your question. It's hard to understand... As of now, your class seems to be indescribably complex for such little functionality. You constructor is doing way to much work. It should really only be setting up the class that's being called! Login and authentication should either happen in separate methods, or even separate classes.

Have you not tested your class to see if $this works as it should? To the best of my knowledge, it shouldn't. And it seems unnecessary since you already made your variables public. Which they shouldn't be.

I'd also avoid throwing an exception from the constructor. It's uncommon and wouldn't really be expected.

---

This code because it seems jumbled and crammed. If the User class had less functionality (currently it's handling authentication, authorization, log outs, application functionality, and more) then it'd be easier for you to piece together the code. Let the code breathe, having different classes isn't a bad thing!

---

On a good note, the aesthetics of the code are very nice. Only a couple notes on the formatting:

• Consistency is key. I noticed you have $useragent, $user_agent, and $u_agent. Which is it? Choose one and stick to it!
• You've used die... Please don't use die. Handle the errors in a fashionable sense and give some proper error reporting to the user.
• LoggedIn is not such a good namespace. More namespace info here.
• Typically, class variables aren't all public. Using encapsulation is a good way to keep uniformity and conciseness.
• Variables should give the reader some details about what the variable is meant for. What data type is $sign_in? What exactly is $btc_addr? $last_ajax of what, last Ajax call? $result is very ambiguous.
• You could switch your conditionals around so you can simplify to the following code:

Notice I take out the false check, if it's an array, it won't be false.

$result = $this->fetch_details($e_mail,$p_word);
if (!is_array($result)) {
    throw new Exception('Invalid Username or Password');
}
$this->email      = $result['u_email'];
$this->hash       = $result['u_pass'];
$this->user_id    = $result['u_id'];
$this->user_agent = $u_agent;
$this->sign_in    = time();
$this->csrf_token = $this->csrf_create($this->sign_in,$this->hash);
$this->btc_addr   = $result['u_addr'];
$this->throttle   = 0;
$this->last_ajax  = ($this->sign_in - 5);
$this->verified   = $result['u_verified'];
$this->reputation = $this->fetch_rep($this->user_id);

Updates

• When I say it has too much responsibility, I'm saying that the class is doing to many things unrelated to each other. Setting a bid is completely different than logging out. I'm saying that these things should be separated. The S in SOLID stands for Single responsibility (highly recommended read). You want your code to adhere to these principles. A class of User shouldn't be handling CSRF creation, and authentication, and data retrieval, and application business (i.e. bidding).
• OWASP has graciously given some sample PHP CSRF information. They're using mt_rand, however, I'd have to agree with you that openssl_random_pseudo_bytes is a better choice. I can't see the benefit in infusing the timestamp and password into the string, but I also cannot see the downfalls.