User class and 'generic' Database class

Question

This may be a bit overwhelming, but me and a friend of mine have written a Database class with methods for our queries. All of these queries should be generic so we can use them in multiple ways. We make use of PDO.

These are the thing bothering me;

• We need a session class to handle all the sessions within our classes (tips, anybody)?
• The user class is not fully written in OOP
• Don't mind the use of public, protected, private, everything is public, this is something we will fix when we have a full working system

Can anybody please review our code and give us some best practices, tips, thing we did wrong, etc?

Comments are in Dutch. If you have a question feel free to ask.

THE DATABASE CLASS

<?php

class Database{

    //constantes zijn geen properties want die kan je niet veranderen
    const DB_HOSTNAME = "localhost";
    const DB_USERNAME = "root";
    const DB_PASSWORD = "";
    const DB_NAME = "quiz";
    private $typedb = "mysql";
    public $db;

    public $sql;

    public function __construct(){

        //heeft geen conditie nodig. moet uitgevoerd worden dus try
        try
        {
            //object aanroepen van de class PDO (ingebouwd in PHP)
            $this->db = new PDO($this->typedb.':host='.self::DB_HOSTNAME.';dbname='.self::DB_NAME, self::DB_USERNAME, self::DB_PASSWORD);
            $this->db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }
        //anders wat? heeft wel een conditie nodig, bij een exception van PDO dus, die store je in de $e
        catch(PDOException $e)
        {
            return $e->getMessage();
        }
    }

    //$values verwacht $_POST waardes dus is al array
    public function insertQuery($table, array $cols, array $values){

        $query = "INSERT INTO $table SET ";

        $column_count = count($cols);

        for($i = 0; $i < $column_count; $i++){
            //voeg toe aan de string
            $query .= $cols[$i]."= :param".$i." ,";   
        }

        //return een deel van de string, dus de laatste komma moet weg
        //start bij 0, en return alleen het laatste (-1)
        $query = substr($query, 0, -1);
        $this->sql = $this->db->prepare($query);

        $value_count = count($values);

        //bijvoorbeeld :param1 = values['1']
        for($j = 0; $j < $value_count; $j++){
            $this->sql->bindParam(":param".$j, $values[$j]);
        }

        $this->sql->execute();

    }

    public function selectQuery(array $cols, $table, array $wherecolumns, array $values){

        $column_count = count($cols);
        $wherecolumn_count = count($wherecolumns);
        $query = "SELECT ";

        // SELECT column1, columns2 FROM table WHERE value1 = 4 ORDER BY

        //bij meerdere kolommen selecteren
        if($column_count > 1)
        {
            for($i = 0; $i < $column_count; $i++){
                //voeg toe aan de string
                $query .= $cols[$i].", ";
            }

            //substr komma weghalen
            $query = substr($query, 0, -2);
        }

        //bij 1 of alle (*) kolommen selecteren
        elseif($column_count == 1)
        {
            $singleColumn = implode(" ", $cols);
            $query .= $singleColumn;
        }

        $query .= " FROM $table";

        //als het geen globale query is, dus heeft wheres en values
        if(!empty($wherecolumns) && !empty($values)){

            $query .= " WHERE ";
            for($j = 0; $j < $wherecolumn_count; $j++)
            {
                    //de where columns en values uittellen en toevoegen aan de query
                $query .= $wherecolumns[$j]." = :value".$j." && ";   
            }
            $query = substr($query, 0, -3);

        }

        $this->sql = $this->db->prepare($query);

        //de parameters binden
        for($k = 0; $k < $wherecolumn_count; $k++){
            $this->sql->bindParam(":value".$k, $values[$k]);   
        }

        $this->sql->execute();

    }

    public function updateQuery(){


    }

    public function deleteQuery(){


    }

}



?>

THE USER CLASS (note; this may not be fully OOP?)

<?php

class User extends Database{

    public $username;
    public $password;
    public $type;

    public function register(){

        $db = new Database();

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

            $this->username = $_POST['regUsername'];
            $this->password = sha1($_POST['regPassword']);
            $this->type = 2;

            if(!empty($_POST['regUsername']) OR !empty($_POST['regPassword']) OR !empty($_POST['retypePassword']))
            {
                if($_POST['regPassword'] == $_POST['retypePassword']){
                    $register_tablename = 'users';
                    $register_users_cols = array("username", "password", "usertype_id");
                    $register_post_values = array($this->username, $this->password, $this->type);


                    $db->insertQuery($register_tablename, $register_users_cols, $register_post_values);
                }
            }
        }
    }

    public function login(){

        $db = new Database();

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


            $this->username = $_POST['loginUsername'];
            $this->password = sha1($_POST['loginPassword']);

            if(!empty($_POST['loginUsername']) OR !empty($_POST['loginPassword']))
            {
                $countColumns = array("COUNT(*)");
                $loginTable = "users";
                $postWhereCols = array("username", "password");
                $loginValues = array($this->username, $this->password);


                $db->selectQuery($countColumns, $loginTable, $postWhereCols, $loginValues);

                if($db->sql->fetchColumn() == 1)
                {
                    $loginColumns = array("*");

                    $db->selectQuery($loginColumns, $loginTable, $postWhereCols, $loginValues);

                    $auth = $db->sql->fetch(PDO::FETCH_ASSOC);

                    $_SESSION['userid'] = $auth['id'];
                    $_SESSION['username'] = $auth['username'];
                    $_SESSION['type'] = $auth['usertype_id'];

                    header("Location: home.php");
                }

            }

        }

    }

    public function logout(){

        unset($_SESSION['userid']);
        unset($_SESSION['username']);
        unset($_SESSION['type']);

    }


}

?>

Answer 1

First things first:

Do not extend or wrap PDO

From the off, let me be clear about this: PDO offers a clean, concise and fully OO API. To attempt to extend, or in your case wrap it into something that is, somehow better is Impossiburu! I've been quite vocal about this in many of my previous PDO related reviews.

Unless...

Of course, PDO is, even in my projects, often hidden away, tucked into some connection manager or abstraction layer of sorts. But that layer does so much more than your class does. And it certainly does a lot more than any given class can or indeed should do. For that reason ORM's and DBAL's like Propel or Doctrine exist, and are in common usage. Don't make the mistake of building your own, by yourself from scratch. By the time you get it close to something that could be considered a stable release, the existing open-source projects will have surpassed anything you can build many times over. If for no other reason than the simple fact that these projects are being actively developed by a multitude of developers. You simply can't compete with that by yourself.

On to the code itself

Constructors can't return strings

Your Database constructor contains a really ugly "faux-pas" in the catch block. You're catching a possible PDOExceptionthat might have been thrown and try to return its message (A string) from the constructor. That's not possible. Can't be done. What will happen is you'll end up with an instance of the class as though the constructor hasn't been called. IE: all of the properties that the constructor should've initialized will still be null.
In short remove this bit:

catch (PDOException $e)
{
    return $e->getMessage();
}

Exceptions should be caught by code that can handle them

What you should do is not catch the exception. Think about it: your class needs the DB connection to do its job. If something in the constructor (which creates the PDO instance, and sets it up for future use) went awry, the class can no longer safely do its job, and so the user (code that creates the instance) must be notified that an exceptional error has occurred. That's the whole point of exceptions, and that's exactly why they keep propagating through the call-stack right up to the point where they're either caught, or handled by an exception handler, or they cause the application to halt.

If the DB connection is not critical, the code that creates the Database instance might have an alternative at the ready, in which case, that code might look like this:

try {
    $db = new Database();
} catch (PDOException $e) {
    $logger->log(
        sprintf(
            'DB error: %s - switching to mongodb (or whatever)',
            $e->getMessage()
        )
    );
    $db = new MongoClient();//or some other class
}

Of course, it's far more likely that a DB connection in a PHP app is vital, but a class shoudln't assume that it's critical to the application in which it is being used. That's entirely up to you, the programmer.

Missing charset in PDO's DSN string

It's great to see you're using prepared statements (PS) (though I haven't looked at how in detail), but it's important to note that PS aren't 100% safe just like that. A matter which has been well covered in the past. Specifying the charset is an easy thing to do, and an absolute must if you want your code to be as safe as possible, while maintaining the portability that is often required of a PHP project.

TL;TR: Just add the damn ;charset=UTF-8 to your dsn string when creating a PDO instance.

Avoid silly one-liners and pointless repetition

While I'll be one of the last people to tell you to reduce the number of lines of code you write, I really don't like it when I come across PDO code that insists on calling setAttribute right after a new instance is created. Look at the docs for PDO::__construct():

public PDO::__construct ( string $dsn [, string $username [, string $password [, array $options ]]] )

The last argument is an array of options (or attributes if you will). Your current code:

$this->db = new PDO($this->typedb . ':host=' . self::DB_HOSTNAME . ';dbname=' . self::DB_NAME, self::DB_USERNAME, self::DB_PASSWORD);
$this->db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

Could, and IMHO should, have been written like so (the first bit, using sprintf is a personal preference, though):

$dsn = sprintf(
    '%s:host=%s;dbname=%s;charset=%s',
    $this->dbtype,
    self::DB_HOSTNAME,
    self::DB_NAME,
    self::DB_CHARSET//you need to define this one, though
);
$this->db = new PDO(
    $dsn,
    self::DB_USERNAME,
    self::DB_PASSWORD,
    [
        PDO::ATTR_ERRMODE      => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_ORACLE_NULLS => PDO::NULL_NATURAL,
        //other default options
    ]
);

Effectively, creating the instance and setting all attributes in one go without sacrificing readability (IMO).

What's the point of $this->dbtype?

Your class defines $dbtype and initializes it to mysql. OK, that's up to you, but you're using the property in the constructor as if its value isn't a given anymore. That's just not true. There's nothing the user can do between constructing a new Database instance, and the PDO instance being created that might even come remotely close to changing the value of the $dbtype property. Why would you bother to use a property for this? Why isn't it a constant like the rest of the parameters? Either way (a constant or a property), the mere fact that these params are hard-coded in your class is a problem, which brings me on to:

A class and its configuration are 2 different things

Lord knows we've all done it, and some of us occasionally still do it: hard-coding configuration into a class. However, a lot of us have learnt the hard way that this is a sure-fire way to introduce bugs and other issues. Especially since your comments (having the advantage of being able to read Dutch) has me believe you're not taking into account possible issues that may be caused by extending you class.

Class constants can be overridden! Suppose I want to extend you class, but use it for another connection:

class ChildDb extends Database
{
    const DB_USERNAME = 'myuser';
    const DB_PASSWORD = '123pass';
    const DB_HOSTNAME = 'anotherhost';
    const DB_NAME = 'myTable';
}

What would happen if I tried to create a new instance? To what DB will I connect?:

$db = new ChildDb();//connects to the DB in Database!

I'll connect to the DB as specified in the parent class. To address this issue, I'll be forced to change the parent constructor to use late static binding:

$dsn = sprintf(
    '%s:host=%s;dbname=%s;charset=%s',
    $this->dbtype,
    static::DB_HOSTNAME,
    static::DB_NAME,
    static::DB_CHARSET//you need to define this one, though
);

replacing self:: with static:: ensures PHP will use the constants as defined on the called class (ie ChildDb) where it can.

Alternatively, I might choose to override the parent constructor, which makes a breach of contract all the more likely. Someone might come along, overriding the constructor and might want a constructor that allows the user to pass connection attributes to the constructor, or a value to assign to the $dbtype property:

public function __construct($dbType)
{
    $this->dbtype = $dbType;
    parent::__construct();
}

Now you might think "So? What's the problem?" The problem is that this breaks the inherited contract set out by Database. PHP is a very forgiving language, especially when it comes to constructors. To quote the docs

Unlike with other methods, PHP will not generate an E_STRICT level error message when __construct() is overridden with different parameters than the parent __construct() method has.

However, PHP might (ATM) find it OK to override the constructor like this. Other languages sure as hell don't, and if you throw in abstract classes, there are some oddities that this can cause

extends means just that: it extends what is already there

Your User class extends the Database class. Apart from issues mentioned by others (SRP concerns), I can't, for the life of me, understand why your register method would still require you too do this:

$db = new Database();

If class User extends Database then new User is an instance of Database anyway, and you can simply write:

$this->insertQuery();

instead of $db->insertQuery();

That's it for now, I'll revisit this answer, as there are many, many more things left for me to discuss. Let me know if you want me to explain something I've previously mentioned but that isn't quite clear.

Answer 2

Security

        $this->password = sha1($_POST['loginPassword']);

Your password scheme is not secure. For a secure password scheme, please add a salt to the password prior to hashing.

For optimum security, salts must be unique.

See this Security.SE question on how to securely hash passwords. Additionally, this Security.SE question deals with storing the salts.

SHA-1 isn't that secure either, but if you're already using this code in an online service, short of a forced password change, there isn't much you can do.

password_hash uses bcrypt internally, which makes for a good hashing scheme if you add a unique salt.

Magic Numbers

        $this->type = 2;

It's a magic number! Where did it come from? Nobody knows!

I don't know what 2 means. Maybe 1 is Admin, and 2 is User? What if in the future you have superadmins too? Or super users?

At the very least, make a constant TYPE_USER or something like that somewhere. Then you can set $type to TYPE_USER. Nobody needs to know it's actually 2.

Answer 3

Instead of reviewing your code, I took the liberty of reviewing your lack of design. And don't take this personally ;)

Don't just slap your keyboard

A big mistake a lot of developmers make, is to simply start coding. They tap tap tap characters to a file, use the word class and ship it. This is ok - they think - because at the end of they day, it works. Everyone happy.

single responsibility

The code that is created, slapping away at the keyboard, is however crap piled on top of crap. A very good indication of this crap are things like:

class User extends Database

Where in the design, did a guy decide that a User is a Database? The answer often is:

What design? We don't need a design...

And there is the real problem of this code. It is not bade code if you read line per line. But the overall design is just missing.

Every little piece of code you write should have One Single Responsibility. The first rule of SOLID.

Analysing what you have

You wrote 2 class: User and Database. Where User is a special case of a Database. Like a Dog extends Animal.

Your Database class does the following things:

• Keep track of the DB settings
• Create a PDO object
• Handle PDO errors
• Generate SQL queries
• Interact with the PDO object to execute the generated SQL query

Your User class extends the Database class but instead of using the methods it inherited, it construct a new Database object everytime it has to do something.

If look at the User class, it actually is not a User class. It is some kind of authentication handler that authenticates a User over HTTP POST that interacts with the $_SESSION

Again, a lot

Requirements change

Just image the following scenarios and how they would affect your code

• The host does not have PDO installed and you need to use mysqli as driver
• You want to switch to a file-storage with serialized data instead of a Database
• Your session-data should be stored in a database
• All data in the user table needs to be encrypted

These are all scenarios that can happen, and scenarios that should not require a lot of extra work. If the design is good.

Implementing interfaces

When programming, start by making everything private. Because once you make it public, it should stay that way, and it can no longer change. Or you will have a lovely weekend refractoring your code and a happy man/wife at home.

Another approach, the one I prefer is a little more strict. It uses interfaces and typehinting.

First, we start by defining a neat little interface called Storage that defines how our code will interact with Storage. We don't care (and shouldn't) where the data is stored, is the file-system, a database, ftp, ...

interface Storage {

    /**
     * Select the give $fields from the given $resource with optional $filters
     * @param  {String} $resource
     * @param  {Array} $fields
     * @param  {Array} $filters
     * @return {Array} An array of array-results
     */
    public function select($resource, $fields, $filters=array());

    /**
     * Insert a single row into the resource
     * @param  {String} $resource
     * @param  {Array} $data
     * @return {void}
     * @throws DatabaseInsertException
     */
    public function insert($resource, $data);
}

Now our UserAuthentication class can use this interface:

class UserAuthenticator {
    public function __construct(Storage $storage) {
        $this->storage = $storage;
    }

    /**
     * Trys to authenticate the given username & password
     * Returns true on success, false otherwise
     * @param  {String} $username
     * @param  {String} $password
     * @return {Boolean:false} on failure
     * @return {int} the user.id
     */
    public function authenticate($username, $password) {
        $users = $this->storage->select(
            'user',
            array('id','password'),
            array('username' => $username)
        );

        if ( count($users) !== 1) {
            return false;
        }

        if (password_verify($password, $users[0]['password'])) {
            return $users[0]['id'];
        }
    }
}

We would use as follow:

$authenticator = new UserAuthenticator($storageObject);

if ( false !== ($userid = $authenticator->authenticate($username, $password)) ) {
    $session->set('user',$userid);
}

//somewhere else

$session->get('u')

Our session interface would thus look like

interface Session {
    public function set($key, $val);
}

A very simple session class would look like this:

class SimpleSession implements Session {
    private $data;
    public function __construct() {
        session_start();
        $this->data = $_SESSION;
    }

    public function set($key, $val) {
        $this->data[$key] = $val;
    }

    public function get($key) {
        return isset($this->data[$key]) ? $this->data[$key] : null;
    }

    public function __destruct() {
        $_SESSION = $this->data;
    }
}

I'll leave the implementation of the PDOStorageAdapter to you ;)

Answer 4

Security:

Your Database class is vulnerable to SQL injections. The way you currently use it actually prevents that, but the core problem and core truth remains:

If you don't fix this I'm prepared to bet it comes back to bite you.

Short attack vectors for insertQuery:

• $table: you use the table inputted without any further checking. If there's anything unfriendly in that $table you're screwed.
• $cols: Same problem here. You don't check for the contents. You assume they aren't malicious.

You did a good job on securing the values though ;)

Attack vectors for selectQuery:

• $cols, $table, $wherecolumns: same problem as described for the insertQuery.

The remedy:

Your syntax allows you to do something very cool. If any of these attack-vectors contain multiple words, you can outright reject the input. (this means for $cols and $wherecolumns if any of the array elements contain multiple words)

Reasoning:

Table and column names never contain spaces. if you find one (or splitting at one gives you more than a single element) one can assume that the input is either malicious or malformed.