Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a login class I made for my personal website.

What I don't care about, is how secure this login class is, because I know it's not. But it keeps out the rare unwanted guests because I do not want to have my information public. I made this mainly for learning purposes. I want to know how "OOP" this is and what I could do differently.

Login Form:

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

            if($login->userLogin($user, $pass))
            {
                echo "Successfully logged in!";
            }
            else {
                echo $login->getError();
            }
    }

Login Class:

class UserLogin
{
    private $id;
    private $user;
    private $pass;
    private $salt;
    private $hash;
    private $error;

    protected $mysqli;

    public function __construct()
    {
        $this->mysqli = mysqliSingleton::init();
    }



    function setError($val)
    {
        $this->error = $val;
    }



    function getError()
    {
        return $this->error;
    }



    public function userLogin($user, $pass)
    {
        if($this->checkUser($user) && $this->checkPass($pass))
        {
            $_SESSION['memberID']   = $this->id;
            $_SESSION['memberName'] = $this->user;

            return true;
        }
        else {
            $this->setError("Invalid username or password");
        }
    }



    public function checkUser($user)
    {
        $stmt = $this->mysqli->prepare("SELECT memberID, userName, salt, hash FROM members WHERE userName = ? LIMIT 1");

        $stmt->bind_param("s",$user); 
        $stmt->execute();
        $stmt->bind_result($this->id, $this->user, $this->salt, $this->hash);

        if (null === ($stmt->fetch()))
        {
            return false;
        }
            return true;
    }



    public function checkPass($password)
    {
        return (hash_hmac("sha256", $password, $this->salt) === $this->hash);
    }
}
share|improve this question

4 Answers 4

up vote 7 down vote accepted

Madara Uchiha sums up some problems.

Here are some more:

To many if

A methods containing multiple if's smells. It makes testing harder since there are more edge cases. It also creates a mess where none should be.

For instance your getError method. A better approach would be:

function getError()
{
    return $this->error;
}

And if you want it to return false if no error has been set:

private $error = false; //defaults to false

This makes it a private variable (as it should be) instead of it being added on run-time (what you are doing now).

Then, your userLogin method also contains a lot of if's. And no, you are giving away to much info. Don't tell what was wrong (username or password). Only tell that the combination is wrong. This way, you don't give away any information. So:

public function userLogin($user, $pass)
{
    if( $this->checkUser($user) && $this->checkPass($pass) )
    {
        $_SESSION['memberName'] = $this->user;
        return true;
    }
    else
    {
        $this->setError("Invalid username/password");
    }
}

I also change return(true) to return true. What you are doing is evaluating true. Pointless.

Only take what you need

Every time you write SELECT *, a cute small kitten dies. You only need the userName, hash and salt.

SELECT userName, hash, salt FROM members ...

Then you should add some parameter binding spices and your all good to go!

Then, what happens after is weird. We know that if a user exists, only 1 should. So we can add a LIMIT 1 statement to the SQL. This way, the database will stop searching once it has found 1 member instead of continuing to search the entire members table.

After that simply doing this:

if ( null === ($userData = $result->fetch_assoc()) )
{
    return false;
}

$this->user = $userData['userName'];
$this->user = $userData['salt'];
$this->user = $userData['hash'];

return true;

is cleaner, better to read and does the exact same thing.

Your checkPass function has the same problem: you evaluate something to a boolean. Then you evaluate that boolean in the if-condition and then you return an evaluated boolean. A better method is:

public function checkPass($password)
{
    return (hash_hmac("sha256", $password, $this->salt) === $this->hash);
}

And that is the code review.

OO review

Your code is OO, you are using Objects and stuff and the class keyword. But I think what you are asking is: is it good OO?

No it isn't

A good place to start is SOLID. Starting with Single responsibility.

Every class should have one single responsibility. Is it authentication? or is it retrieving user-data? or it it connection to a database? Your class does a little bit of all three.

A good place to start when writing OO-code is in starting with your controllers first. Here you should write how you would like to react with you application. for instance:

if ( $auth->login($username, $password) )
{
    print 'welcome master';
}
else
{
    print $auth->getError();
}

Here we know that we want an interface with 2 methods. login and getError()

interface Authentication
{
    public function login($uuid, $pass);
    public function getError();
}

and then implement that interface. For instance a UserAuthentication class that authenticates using users. Later on you could change that with a LDAPAuthentication class without having to worry that the rest of your app breaks. As long as the interface is the same ;)

share|improve this answer
1  
Lovely, thanks :) I am already checking the prepared statements for now, and I will check your feedback soon as well to learn more, and I will change my script accordingly :) Thanks a lot! –  Ron Brouwers Jul 31 at 8:06
    
I have changed the class a bit after reading up on some advice that you and Madara Uchiha posted. Using a prepared statement now and removed a lot of "ifs". Unfortunately I cannot use password_hash yet since my server isn't running PHP 5.5 yet. –  Ron Brouwers Jul 31 at 10:54
    
looks a lot cleaner now :) –  Pinoniq Jul 31 at 11:47

Yes, there are several things I can see with your implementation, from most critical to less critical:

  • SQL Injection - You has it! Since you're already using mysqli, learn about prepared statements with mysqli and apply them. You are very vulnerable to Little Bobby Tables.
  • Your password hashing is not secure - sha256 is insecure for password hashing in on itself, you're also apply a $this->salt which isn't defined anywhere. See password_hash() and password_verify() for better, more secure password hashing, which includes iteration over the hash, use of proper hashing algorithms (such as CRYPT_BLOWFISH), and simpler and more readable UI.
  • Your class does too many things - Why is it the business of a login class, that's supposed to handle users, to create a database connection instance? Why not just pass it in?

    public function __construct(mysqli $databaseConnection)
    

    This way, if you ever change your credentials or even move to a different server, you won't need to change your class's code, but only the code which is responsible for creating the connection. That makes your class reusable.

  • Naming conventions - It's common to use CapitalCase for class and object names, and camelCase for variables and functions.

share|improve this answer
    
the salt is defined in the checkUser() method. But all the rest is true! –  Pinoniq Jul 31 at 7:38
    
Thanks, This is useful and I can work with this :) I will take a look into everything you said. Thanks a lot :) –  Ron Brouwers Jul 31 at 7:39
    
@RonBrouwers "Why is it the business of a login class, that's supposed to handle users, to create a database connection instance? Why not just pass it in?" <- In programming parlance this is referred to as 'Dependency Injection'. Just a quick tip FYI. –  Mark Jul 31 at 15:20

The LIMIT 1 on your query looks suspicious. LIMIT is rarely useful without ORDER BY, and you didn't specify ORDER BY. Have you either made the username column the primary key of the members table, or put a UNIQUE constraint on the column? If you haven't already done so, I recommend it, to let the database help you enforce data integrity.

share|improve this answer
    
Yes, the I have a "memberID" which is unique. –  Ron Brouwers Jul 31 at 10:20
4  
I also add a LIMIT 1 to some queries, just so that I don't have to worry about an edge-case, where someone disables the primary-key constrained, or does something evil with data-load and deferred constraints... - LIMIT 1 clearly signals to the reader you only get one row and don't have to check. I think it's ok together with a primary key –  Falco Jul 31 at 11:57

A couple of things I smell:

  • Returning multiple values at once (aka returning a "Tuple") means your methods won't need as many side-effects.
    • In PHP, you accomplish this by returning an array and using the list function to assign the return value to multiple variables at once.
  • Keep your method implementations to a Single Level of Abstraction (Clean Code, p. 36).
    • For example, in your "userLogin" method, you're using both higher-level abstractions (checkUser() and checkPass()) and low-level abstractions ($_SESSION[]).
    • To fix this, use the Extract Method refactor.

For example:

//Login form:
...
list($success, $error) = $login->userLogin($user, $pass);
echo $success ? 'Success!' : $error;
...


//Login class:
...
/**
 * Attempts a user login.
 * Side effects: if successful, saves the current user in $_SESSION.
 * @return A tuple of [ $successful:BOOLEAN, $error:string ]
 */
public function userLogin($user, $pass) {
    if ($this->checkUser($user) && $this->checkPass($pass)) {
       $this->somethingThatSetsUpTheCurrentSessionMember();
       $successful = TRUE;
       $error = NULL;
    } else {
       $successful = FALSE;
       $error = 'Invalid username or password';
    }
    return array($successful, $error);
}
...
share|improve this answer
    
Wow thanks, didn't know this :) It's good to learn this stuff. –  Ron Brouwers Jul 31 at 16:43

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.