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.

I incorporated the feedback I received from my previous post into a very simple Login class.

Main question:

  • How can I design this class and it's implementation so that it looks like it was developed by a seasoned senior level PHP developer that follows all of the best practices? I'm trying to design this class so that it can be reused in other applications. I also have some other questions that are located inside of the code. Thank you in advance for helping!

Please overlook commenting on the following 2 things:

I know I have to learn these!!! but just not yet!! :-)

  • Docblock commenting
  • Unit testing

Login Class

    <?php
    class Login
    {

    private $username;
    private $password;

    //PDO mysql connection object
    private $pdo;

    /**
     * Question 1. 
     * Is this the correct way to use an error object? I am passing it in to 
     * the constructor for DI (Dependency Injection) just like the $pdo 
     * object? I will have multiple classes that need error management. 
     * 
     * Question 2. 
     * Would each class that uses the error object need to have a method 
     * called getErrors() as shown below and should I implement an interface 
     * called 'errors' that has a getErrors() method or should I use an abstract
     * class that implements it once so all classes can use it?
     */
    private $error;

    public function __construct(PDO $pdo, Error $error, $loginCredentials)
    {

        /**
        * Throw exception if missing Login Credentials 
        */
        if (!isset($loginCredentials['username']) || !isset($loginCredentials['password'])) {
            throw new InvalidArgumentException(
                "Login credentials must be passed into the ". __CLASS__ . " class in the form of an 
                 associative array with the keys 'username' and 'password'."
            ); 
        }

        $this->pdo = $pdo;
        $this->error = $error;
        $this->username = $loginCredentials['username'];
        $this->password = $loginCredentials['password'];

    }

    public function authenticate()
    {
        /**
         * Question 3. 
         * In each class that uses a PDO connection object, 
         * is it proper to do all these database interaction details 
         * in each method that requires database interaction? 
         */
        $query = "SELECT * FROM admin WHERE user = :user AND password = :pass";

        //Prepare statement
        $stmt = $this->pdo->prepare($query);

        //Set fetch mode
        $stmt->setFetchMode(PDO::FETCH_ASSOC);

        //Set params to bind
        $params = array(
            ':user'=>$this->username,
            ':pass'=>sha1($this->password)
        );

        $stmt->execute($params);

        //If the user is found, log them in.
        if ($stmt->rowCount()) {
            $this->loginUser();
            return true;
        } else {
            /**
             * Is this the best way to set errors?
             */
            $this->error->set('Invalid Username and or Password combination');
        }

    }

    /**
     * Question 4. 
     * Is this the proper way to display errors back to the user? 
     * Once a use submits the login form, the client code calls 
     * authenticate(). If it returns false, it runs this method. 
     */
    public function getErrors() {
        return $this->error->getErrors();
    }

    private function loginUser()
    {
        $_SESSION['loggedIn'] = 1;
        $this->isLoggedIn = true;
    }

}

Error Class

class Error 
{
    private $errors = array();

    public function set($msg)
    {
        $this->errors[] = $msg;
    }

    public function getErrors()
    {
        return $this->format();
    }

    private function format()
    {
        if (count($this->errors) > 0) {
            $errors = '';
            foreach ($this->errors as $error) {
                $errors .= '<p>'.$error.'</p>';
            }
            return $errors;
        }
        return false;
    }
}

Implementation

if (isset($_POST['submit'])) {

    /**
     * Question 5.
     * Looking at the below code, is there a better way so that I can comply 
     * with the DRY (Don't Repeat Yourself) principle in order to not repeat 
     * the try{} catch() {} block very time I pass a $pdo object into a class? 
     * 
     * Is the proper way accomplished by first creating a new class i.e. PDOConfig 
     * that extends PDO, and inside the constructor put the try catch block? 
     * 
     * If so, is it always better to pass the details into PDOConfig like this:
     * $Login = new Login(new PDOConfig(DB_DSN, DB_USER, DB_PASS), new Error(), $_POST)
     * or is it better to hard code the constants into the PDOConfig 
     * class's constructor, and then call it like this:
     * $Login = new Login(new PDOConfig(), new Error(), $_POST)
     */
    try{
        $pdo = new PDO(DB_DSN, DB_USER, DB_PASS);
        $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    } catch(PDOException $e) {
        echo "There was an error connecting: " . $e-getMessage();
    }

    $Login = new Login($pdo, new Error(), $_POST);
    if ($Login->authenticate()) {
       header("Location: index.php");
       exit();
    } else {
          echo $Login->getErrors();
    }
}
share|improve this question
    
Why is everyone doing the password check using SQL instead of PHP? SQL will use a collation when comparing strings that is not always predictable (CI, AI). PHP will compare the strings in a safer way. Just fetch the user by username and check fields (lockout, password, expiry) using PHP. –  SandRock Nov 6 '12 at 13:43

4 Answers 4

up vote 3 down vote accepted

The numbered questions

1 Correct usage of an Error object?

Probably not. Using DI is fine, creating an object to inject into the construtor where it's not fundamentally required is not.

The use of the term "an Error object" implies it's a common/global concept - I'm not familiar with this. Javascript has error objects for example, PHP has exceptions. In both cases though, they are classes that are created to represent one error and to be thrown and not something that's passed around and especially not something that's eagerly created incase an error condition occurs.

2 all classes need getErrors?

The Error object is not something I feel should exist.

Ignoring that though, from what you've said in the question, the Error object is supposed to be used with various, possibly quite different classes. Since (I assume) the same instance of the error object is passed around there is no need for the getErrors method at all - just get a reference to the singleton Error instance, and query it directly.

3 PDO usage?

You're better off using some abstraction, doctrine is a reasonable choice if you aren't using anything at all right now.

4 Proper way to display errors?

Probably not. This class isn't presentational, it should be returning only data.

5 DRY try/catch?

Only catch exceptions that you know how to handle. in the example code a PDOException is caught, no corrective action is taken which simply means that a fatal error will be thrown on this line:

if ($Login->authenticate()) {

since it tries to talk to the database, which isn't connected. You can use set_exception_handler so that you don't need to scatter try catch blocks around your code where the intended result is to do nothing except logging and show the user (or developer) a 500 error message.

With that said, some more detail in the order points are encountered reading the code:

Don't mix presentation logic into classes

Unless the class is dedicated to presentation logic - don't put presentation logic in it. This simply means don't return or assume html, instead just return appropriate data (booleans, arrays - as appropriate). The function Error::format is an example whereby it would be better to just return the array stack and let something else loop over it and dump to html,json, the log etc.

Don't create needless classes

The error class doesn't seem to offer any benefits.

I.e. this:

$foo = new Login($pdo, new Error(), array(..));
$return = $foo->authenticate();
...
$errors = $foo->getErrors();

Could be used almost identically if the error object didn't exist:

/**
 * Is this the best way to set errors?
 */
$this->errors['user'] = 'User not found'; // as appropriate
$this->errors['password'] = 'Invalid Password combination'; // or this error

If there's a class tracking all errors (why, what's the use/benefit) - it should either be asking individual instances what errors they contain, or some intermediary logic should be doing that. If the error class just contains an array of strings, there's no context to know why of where an error came from. Using DI is fine, but not having a needless dependency is better.

As a related point the function authenticate only returns true or null. It would be preferable to always return something:

function authenticate() {
    if () {
        ...
        return true;
    }
    return false; // default deny
}

Write reusable/configurable classes

Instead of this:

$foo = new Login($pdo, ...);
...
$bar = new Login($pdo, ...); // oops, need to try and login again with different props

Make it easy to (re)configure an existing instance:

$foo = new Login($pdo);
if ($foo->authenticate($_POST)) {
    ...
}
...
$foo->reset();
$foo->checkOldPasswordTable(); // old passwords were stored with a different hash function
if ($foo->authenticate($_POST)) {
    ...
}

Write testable code

The above point is a pseudo-contrived example, unit tests are possibly a better example of when you'd want to not keep creating new objects.

the login credentials are not something that are required at the time the Login class is created:

$foo = new Login($pdo);

$result = $foo->authenticate(array('user' => 'testuser1', 'password' => 'correct password'));
$this->assertTrue($result, "User was not authenticated but expected to be so");

$result = $foo->authenticate(array('user' => 'testuser1', 'password' => 'wrong password'));
$this->assertFalse($result, "User was authenticated with a wrong password");

When the time comes to write unit tests, the code shouldn't need to be modified. Changes to make code testable are usually trivial - and just mean planning ahead and having appropriate methods to set and get data.

Be secure

As indicated by other answers an unsalted sha1 password is weak. Passwords should be stored such that even if someone gets access to the database, they can't determine the original password.

It's not hard to store passwords securely, here's some pseudo code:

function register($username, $password) {
    $salt = '$2a$10$' . substr(md5(time()), 0, 22);
    $storeThis = crypt($password, $salt);

    return $this->save(array(
        'username' => $username,
        'password' => $storeThis
    ));
}

function login($username, $password) {
    $dbUser = $this->findByUsername($username);

    if (!$dbUser) {
        return false;
    }
    if (crypt($password, $dbUser->password) === $dbUser->password) {
        return true;
    }
    return false;
}

By using crypt and blowfish (the string '$2a$' indicates to use blowfish encryption) passwords are stored securely such that password leaks such as the recent linkedin incident become a none issue.

Be configurable

In the above code example, blowfish and a cost parameter of 10 is used. Instead it would be better to make the hash algorighm used configurable:

protected $_algorighm = 'blowfish';

protected $_cost = 10;

public function setAlgorithm($algo, $cost = null) {
    // verify algo is a possibility and the cost is appropriate
    ...
    $this->_algorithm = $algo;
    $this->_cost = $cost;
}

function register($username, $password) {
    $salt = $this->_getSalt();
    $storeThis = crypt($password, $salt);

    return $this->save(array(
        'username' => $username,
        'password' => $storeThis
    ));
}

protected function _getSalt($salt = '') {
    $length = 0;

    switch ($this->_algorithm) {
        case 'des':
            $template = '%s';
            $length = 2;
            break;
        case 'md5':
            $template = '$1$%s';
            $length = 12;
            break;
        case 'blowfish':
            $template = '$2a$%d$%s';
            break;
        case 'sha256':
            $template = '$5$rounds=%d';
            break;
        case 'sha512':
            $template = '$6$rounds=%d';
            break;
        default:
            $template = '';
    }

    return sprintf($template, $this->_cost, $this>_getRandomString($length));
}

protected function _getRandomString($length = 0) {
    return substr(md5(time()), 0, $length);
}
share|improve this answer
    
Hey AD7six! I don't know where to start, your answer was quite descriptive. It is much appreciated. Your answer is definitely going to help a lot of people! Everyone's answer was great but this one goes above and beyond! Thanks for that. In response to what you wrote about the handling errors part. The current project I am working on (like you assumed) has multiple classes. If there is an error when a user logs in, or there is an error when a user submits one of the other many forms, I need some way to display the list of errors to the user. –  darga33 Nov 4 '12 at 4:04
    
I'm very confused on what to do? I also want to learn the best way for much more complex projects than the current one I am working on. I want to learn the best way for complex projects. How would you handle errors? With an array or with an error class? You mention both ways. If I use an error class as a singleton, then I can call the getErrors() method of it. And pass the Error object into each of the classes that use it. But you also mention not using the singleton, and instead using a simple errors array. I'm confused as far as that goes. So I'm kind of just as stuck on the "errors" portion –  darga33 Nov 4 '12 at 4:06
    
as I was when I first made the post. Although I learned a TREMENDOUS amount from everything else you wrote and I really appreciate those teachings!!! –  darga33 Nov 4 '12 at 4:07
    
I edited the answer to clarify that the error object is not something I recommend. if you want to put all errors in a single place you can do so e.g. like this $_SESSION['errors'][] = "a new error occurred"; and loop on $_SESSION['errors'] somewhere in your presentational code to advise the user (and delete from the session on display). That would be by querying objects for error or in addition to logging errors to an internal class property. –  AD7six Nov 4 '12 at 11:21
    
Thanks!!! So are you suggesting that the best way to capture errors is.. inside of each class that captures errors, whenever an error occurs like "invalid user", "required field missing", etc, would be to do $_SESSION['errors']['Name To Display To Screen']= "description of message to display to user". ? But if I do this, my classes will be sprinkled with $_SESSION['errors'][] = "". Is that still following with best practices? I'm a tough cookie, I know. I just want to do it the best way! :-) –  darga33 Nov 4 '12 at 16:31

Red flag

     $query = "SELECT * FROM admin WHERE user = :user AND password = :pass";

Ok, so you've heard that you should hash passwords before storing them, because it turns out that pass is a SHA hash. But you don't seem to have heard of salt. The password database is going to be wide open to a rainbow table attack.

I would recommend that you use a framework which has a good password storage mechanism (if there is one). If not, read the OWASP Password Storage Cheat Sheet very carefully before you rewrite this code.

share|improve this answer
    
Using a salt is of course something to change, but I feel this answer is misleading. It doesn't mention you'd need another exploit to gain access to the db, to make use of that weakness. I.e. unless someone gains access to the db, it changes nothing in terms of the security of the application. crypt with blowfish, is a better recommendation than a salted sha1. It's also a single relatively trivial (in terms of effort at least) point, there are other fish to fry. -1 for tone "you don't seem to have heard of salt". –  AD7six Oct 31 '12 at 14:58
    
@AD7six, it's certainly not trivial, in terms of effort or in any other terms. Doing anything security-related correctly is very hard: that's why I recommend using a framework which does it well over implementing it yourself. Various high-profile leaks of password databases over the past year should be sufficient demonstration of the value of storing them properly. Not sure what problem you have with the tone. Too friendly? –  Peter Taylor Oct 31 '12 at 18:07
    
Users don't post their code to be ridiculed for what they don't know. –  AD7six Oct 31 '12 at 18:23
    
@AD7six, ridiculed, no, of course not, but I don't intend or see any ridicule. People do post their code to be told what they don't know: that's the point of code review. –  Peter Taylor Oct 31 '12 at 20:14
    
I've noticed that is the tough thing with "text". To one person a word or phrase might sound like a bad tone, and to the person writing the word or phrase, they do not intend for it to happen. But it's ok you guys, thanks for both wanting to help :-) –  darga33 Nov 1 '12 at 13:29

I would not recommend returning user messages from the Login class, but instead to return a 'status' which can be converted to a message. This will provide better maintainability and extensibility. There are a couple of options for doing this. First I'll start by demonstrating a problem with the approach of using an Error object when it comes time to differentiate between different failure scenarios:

$Login = new Login($pdo, new Error(), $_POST);
if ($Login->authenticate()) {
   header("Location: index.php");
   exit();
} else {
    // Do something additional based on the failure
    foreach ($Login->getErrors() as $error) {
        if ($error === 'Invalid username or password') {
            // Do something special for invalid credentials
        }
    }
}

A better approach to handle this case is to throw an exception when authentication fails:

class Login {
    public function authenticate(...) {
        // ...
        if ($stmt->rowCount()) {
            $this->loginUser();
            return true;
        } else {
            throw new Exception('Invalid username or password');
        }
    }
}

Lets evaluate this from the point of view of code that uses the Login class (consumer code).

try {
    $login = new Login(/*...*/);
    $login->authenticate(/*...*/);

    // ... Do whatever needs to happen on successful login
} catch (Exception $e) {

    // Here is where we run into problems with this approach because we
    // don't know for sure why this exception occurred, it could very well
    // be a PDOException
    if ($e->getMessage() === 'Invalid username or password') {
        // ...
    } else {
       // ... Handle other types of exceptions
    }
}

This is very fragile and difficult to maintain because every time the message is updated this code needs to be updated as well. You could use a constant for the message, but this still involves a complicated catch clause to determine the nature of the exception and doesn't allow for parameterized messages. This can be improved by a subclass of exception specifically for login related errors:

class LoginException extends Exception {}

class Login {
    public function authenticate(...) {
        // ...
        if ($stmt->rowCount()) {
            $this->loginUser();
            return true;
        } else {
            throw new LoginException('Invalid username or password');
        }
    }
}

Now the updated consumer code:

try {
    // ... same as above
} catch (LoginException $e) {
    echo $e->getMessage();
} /* Other types of exceptions will bubble or can be caught here if it is possible to handle them */

This is perfectly fine for a single type of error, but lets suppose the login class is expanded to protect against brute force attacks by locking accounts after a certain number of failed attempts. The authenticate method would now look something like this:

class Login {
    public function authenticate(...) {
        // ...
        $acctInfo = $stmt->fetch();
        if ($acctInfo !== null) {
            if ($acctInfo['locked']) {
                throw new LoginException('Account is locked, you must contact an administrator before you can login');
            }
            $this->loginUser();
            return true;
        } else {
            throw new LoginException('Invalid username or password');
        }
    }
}

Now the consumer code:

try {
    // ...
} catch (LoginException $e) {
    if ($e->getMessage() === 'Invalid username or password') {
        // ...
    } else if ($e->getMessage() === 'Account is locked, you must contact an administrator before you can login') {
        // ... Do something different
    }
}

This has the same problem as the first example. Again this can be simplified by using constants to hold the error messages but the same issues apply. Another solution would be to create an exception subclass for each type of error resulting in the following consumer code:

try {
    // ...
} catch (InvalidUsernameOrPasswordLoginException $e) {
    // Common exception handling setup...

    // Differentiated exception handling for invalid credentials...

    // Common exception handling output...

} catch (AccountLockedLoginException $e) {
    // Common exception handling setup...

    // Differentiated exception handling for locked account...

    // Common exception handling output...
}

Now we're getting somewhere, there is no reliance on the actual message to distinguish between failure types. However, this style of multiple catch statements can make it difficult to eliminate duplication. Lets take a step back and instead of introducing multiple types of exception for login failure use an error code to distinguish failure types:

class LoginException extends Exception {
    const INVALID_USERNAME_OR_PASSWORD = 'login.failure.invalidUsernameOrPassword';
    const ACCOUNT_LOCKED = 'login.failure.accountLocked';

    public function __construct($code) {
        parent::__construct('Authentication failed', $code);
    }
}

class Login {
    public function authenticate(...) {
        // ...
        $acctInfo = $stmt->fetch();
        if ($acctInfo !== null) {
            if ($acctInfo['locked']) {
                throw new LoginException(LoginException::ACCOUNT_LOCKED);
            }
            $this->loginUser();
            return true;
        } else {
            throw new LoginException(LoginException::INVALID_USERNAME_OR_PASSWORD);
        }
    }
}

Now the consumer code will be similar to the code we had before but is relying instead on codes to distinguish between failure types.

try {
    // ...
} catch (LoginException $e) {

    // Do common exception handling setup here...

    switch ($e->getCode()) {
        case LoginException::INVALID_USERNAME_OR_PASSWORD:
        // Differentiated exception handling for invalid credentials
        break;

        case LoginException::ACCOUNT_LOCKED:
        // Differentiated exception handling for a locked account
        break;
    }

    // Do common exception handling output here...
}

This is preferable to relying on messages which are prone to change or may not even be the same for each execution. Consider trying to determine the type of failure for the the following message: "Invalid password or unrecognized user: $username".

Not getting into details, using codes has additional benefits over generating messages in internal classes when your application is localized or when support is added for additional clients.

share|improve this answer
    
Hey pgraham, Thanks for the advice but I'm not sure what you mean when you say this "messages should only be generated at the boundary of your application, immediately before you pass it to the client. Before this the failure should simply be represented as an object encapsulating a status which is not tied to any particular message:" Keep in mind, I am VERY VERY new to programming, and OOP. My first and only language is PHP. I'm trying to learn as fast as I can, but in order to really understand and learn, I need to see simple complete examples. –  darga33 Nov 1 '12 at 20:15
    
If you look at the Implementation section of the code, the status code is only transformed into an actual message (by L10N) immediately before being echoed. By "boundary" I mean the point where data is passed to user. In the case of PHP this is usually done with echo. –  pgraham Nov 1 '12 at 20:37
    
Oh yeah I saw L10N, I just don't know what L10N is. I have a tremendous amount to learn in order to understand these concepts clearly. I just thought that it would be a good idea to try to find a project that was developed in PHP using the SOLID acronym princples and all of the best practices. Something that is small, and has around 15 classes. Do you happen to know of such a program? –  darga33 Nov 1 '12 at 22:18
    
L10N stands for localization (first letter L, last letter N with 10 letters in between). In my code sample it is a theoretical class that takes a code and uses that to generate a localized message. I don't know of any such program off the top of my head but you could start by looking at symfony.com. It's not small but you could probably look at one of its components in isolation. –  pgraham Nov 2 '12 at 13:55
    
I think talking about localization is just distracting - most code doesn't need it. A method to set the texts used in the class however, would be appropriate. Pointing at symfony for someone "VERY VERY new to programming, and OOP" is not a good idea. Symfony's source code is confusing for experienced developers. –  AD7six Nov 2 '12 at 21:52
<?php

class Login {

    private $pdo;
    private $hasher;

    public function __construct(PDO $pdo, IHasher $hasher) {
        $this->pdo = $pdo;
        $this->hasher = $hasher;
    }

    public function Authenticate($userName, $password) {
        $query = "SELECT * FROM admin WHERE user = :user";

        $stmt = $this->pdo->prepare($query);

        $stmt->setFetchMode(PDO::FETCH_ASSOC);

        $params = array(
            ':user'=>$this->username
        );

        $stmt->execute($params);

        if ($stmt->rowCount() == 1) {
            // ... get user row or object say:

            $user = new User(); //or something else

            if ($this->hasher->Compare($password, $user->Password)) {   
                //check other stuff, disabled user, locked etc.

                                /* a PHP session has been already started but we start a new with different session id see session handling on php.net */
                $session = Session::Start($user);

                return $session;
            }
        }

        throw new \Exception('Invalid Username and or Password combination');
    }
}

Error class

If something goes wrong simply throw an exception and handle it. No need for some custom magic error handling stuff.

PDO usage

Yes you can use PDO like this but if i were you i would create a wrapper for it to minimalize code duplicates and provide more readability. I recommend you to fatch objects from database rows not associative arrays becouse it is a bad habbit in the PHP community (arrays are for storing data which are same types).

Hashing

First see @Peter Taylor's comment. In my example i'm injecting an IHasher implementation through the constructor but this is not the best i think. You should have some membership provider stuff and that can have a configurable hashing method what can be used at authentication/user registration and password changing scenarios.

share|improve this answer
    
Hi Peter, thank you for posting. The reason I want to use an error class, is because I want to manage all errors in all my classes the same way. I'm trying to get practice using other objects inside objects. As far as the PDO usage, could you provide an example of that wrapper? I agree with fetching objects instead of assoc arrays, I overlooked that. Thank you! As far as hashing, I will look into the salt. I've heard of it, just have never used it yet. Thxs! –  darga33 Nov 1 '12 at 13:36
    
See Database class here: stackoverflow.com/questions/13100427/… This is a very basic class, the next could be a new Exucute and Query method, for example: public function Query($queryString, array $params) { /* ... */ } here you can not use names parameters int the query instead those you have to use question marks. –  Peter Kiss Nov 1 '12 at 14:27
    
Hey Peter, I took a look! Thanks!! I have come to the conclusion that in order for me to learn the right way to program, I need to find an open source application that is VERY SMALL, probably less than 20 classes. But large enough to see how it is developed and how the individual classes interact. Do you happen to know of such a project? I only want to study the absolute best principles. I absolutely hate to ask, but do you think you could write the entire Login class the way that you would write it and post it? –  darga33 Nov 1 '12 at 22:57
    
That way I can learn from it. Most likely it will be the accepted answer because it will be most beneficial to many people who are trying to learn proper class design. The sad thing is that when people post questions on here, they truely are looking to learn ALOT, but sometimes answers get posted that just teach a little. Like for instance, the post that has the highest score. It simply talks about adding a SALT instead of a sha1 hash. Ok that's great, but it doesn't teach any true class design. You see what I mean? –  darga33 Nov 1 '12 at 23:00
    
When I say "the exact way you would do it" I am referring to the "wrapper" and the whole nine yards. A true class design for all to feast off of who really want to learn. –  darga33 Nov 1 '12 at 23:02

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.