PDO Login Script Always Re-Directing To Header Page

Question

<?php
    include "config.php";
    class Users extends Config {
        public function login($username, $password) {
            try {
                $this->db->beginTransaction();
                $stmt = $this->db->prepare("SELECT `username`, `password` FROM `users` WHERE `username` = ? AND `password` = ? LIMIT 1");
                $stmt->execute(array($username, $password));
                if($stmt->rowCount == 1) {
                    while($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
                        header("Location: index.php?p=loggedin");
                        exit();
                    }
                } else {
                    header("Location: index.php?p=false");
                    exit();
                }
                $this->db->commit();
            } catch(PDOException $ex) {
                $this->db->rollBack();

                echo $ex->getMessage();
            }
        }
    }
    $users = new Users();
?>

As you can see above, my code has two header() methods, where one re-directs to the logged in page while the other re-directs to the error page.
I believe at the best of my knowledge, my script should re-direct to the logged in page as per my input which is correct, but it behaves in an unexpected way.
It re-directs to the error page instead of the logged in page. Please take a look and tell me the reason why it doesn't work? Thanks! Also, if you see any flaws in my code, feel free to throw in some criticism as I wish to improve my self. Thanks a lot.

Answer 1

I'm going to post something that is hopefully a little more useful than those posts above. There are several best practices that can be employed here to make your life easier when coding; after you've wrapped your head around them of course.

First and foremost, this looks like your attempt at an authentication part of the application. If the user passes in a correct username and password, you want to display the correct page. I have no idea on the scope of your application so I'm not going to suggest massive changes.

Autoloading

It looks like you're manually including your files. PHP has spl_autoload_register which, if you're following PSR-0 (and you really should take a look at that) means you can map your directory structure to your class heirachy 1:1 and have classes automatically resolved for you when you ask for them. So when you say new Object, or even (in your case) extends Config, if your Config class is in the top level of your heirachy it will be automatically found and used. This will involve you learning about namespaces.

Once you've understood how namespaces work and how autoloading can save you development time (and I mean significant time in the long run), you can then move onto a Dependency Management tool like composer. Composer is basically a script you run which generates your autoloader for you, then all you need to do at the beginning of your application is require_once __DIR__ . "/vendor/autoload.php". But seriously, take a look at some autoloading tutorials - once you use them, you will never go back and will basically use them in every single application you ever write in the future.

You'll also never need to type include more than once as well - which, btw, only throws a Warning if the file you're trying to include isn't found. It looks like this file needs the config file, so you should use require_once instead in this case - meaning your code will Fatal if this file isn't found (which it seems is what you want).

OOP

So what's wrong with class Users extends Config. Well done at least for having a good naming convention (caps for class names). Basically, extends implies an is-a relationship. You don't only extend to gain extra functionality - that's a code smell. You are saying here that a User is a Config. This is not the case, and if you were to hand this code to any other developer in the future - they would be like "WAIT, WTF". Hopefully in a nicer way though.

Your aim is: to have configuration variables in a class. So, pass them into the class instead. I can't tell what's in your Config class, but you should take a look at the Dependency Injection pattern; here's a tutorial. Basically, you would pass your Config object in via DI (via the constructor), which then decouples your code so that you can test each object individually on it's own merits, mocking out the other objects instead. Testable code. Dependency Injection. Google it.

So, don't extend Config. Pass in your configuration instead:

class Users
{
    /**
     * @var Config
     */
    protected $config;

    /**
     * @constructor
     *
     * @param Config $config The class configuration, duh
     */
    public function __construct(Config $config)
    {
        $this->config = $config;
    }

    /** Snip **/
}

Also take a look at the phpdoc syntax to help you write commented code that other developers can understand and parse with ease.

Transactions

You're also using transactions, which you really don't need to use for SELECT. Why not? Because transactions allow you to either commit or rollback - but when you do a select, you're not changing any data. So there's no point in performing a transaction. So remove those, and just try and retrieve the data via a SELECT.

Your Header Code

Each class, each object, should have one reason to change. One task that it does and that's it. If you have some code that reads from the database, compares some strings and saves to a file, then you should have 3 classes at least: one that reads from your persistence, one that performs the comparison and one that saves to a file. This is called the Single Responsibility Principle which you can read about and google.

With this in mind, this individual class should not be responsible for sending headers. Return either true or false and have the class using this one do that instead. It is not the responsibility of this class - this should only be dealing with authentication.

If you picked up a framework, you would do this either in your Controller or your View depending on how close you stick to the MVC pattern - which is not actually possible in it's strictest sense for web applications (I put that there for the anal people out there) but really - you should be separating your concerns and at least having SRP adhered to throughout your codebase.

Security

I am by no means a security expert, but what you don't do is pass both your username and password to the database. If you are hashing your passwords, which you should be, you just retrieve the User from the database via the username only, then compute whether or not the password matches the hash within your PHP code. This way you are not sending your password to the database, and this helps (albeit only a little) against SQL Injection. PHP 5.5 contains some really useful password_* functions to help you in this regard, but as you're probably not using PHP 5.5, check out the password_compat library.

What is SQL injection? There's more info from Bobby Tables, and also this really useful image below.

Conclusion

These are just a few pointers for you to head in the right direction toward better code that you can pass to other developers who will understand it and you can be proud of it. Take a look at a framework like Symfony so you don't have to worry about authentication, autoloading and things like this - it's all done for you. Composer is definitely a must. You need to start debugging your code line-by-line, var_dump() by var_dump() until you see something that isn't what you expect. Debugging 101, it'll help you and also help us to help you more.