4
\$\begingroup\$

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)
    {

    }
}
\$\endgroup\$
  • \$\begingroup\$ 5 hours, not a single comment or answer. must be dead over here. \$\endgroup\$ – r3wt Jul 7 '14 at 16:59
  • \$\begingroup\$ i added a a new method for generating csrf tokens. i'm wondering if sha256 is sufficient for the job. \$\endgroup\$ – r3wt Jul 8 '14 at 13:33
5
\$\begingroup\$

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.
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ i used die to return the json encoded response. i'm building a poor mans angular js with php and javascript. the code is not unnecessarily complex in my opinion, it just seems that way. the reason i'm fetching all this data and casting the user object to the session is so the that later on i have no need to fetch data from the database about the loggedin user \$\endgroup\$ – r3wt Jul 7 '14 at 23:06
  • \$\begingroup\$ When I said complex, I meant that the object has too much responsibility \$\endgroup\$ – Alex L Jul 7 '14 at 23:27
  • \$\begingroup\$ I don't understand why or how it has too much responsibility? it is essentially constructing a loggedInUser object that can be stored in a session, then retrieved for the duration of the session and public methods will still be accessible. Is it insecure? \$\endgroup\$ – r3wt Jul 8 '14 at 13:39
  • \$\begingroup\$ @r3wt I made an update, I hope that clears things up. If not I can try and find more resources to help you understand! \$\endgroup\$ – Alex L Jul 8 '14 at 17:20

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.