moving from->to: mysql->pdo, md5->pbkdf2, procedural->oop

Question

I'm upgrading/rewriting our online client portal and wondering if my log in page code looks okay so far. during this rewriting/learning process I am upgrading a number of things: PHP 5.2 to 5.3.5, MYSQL 4.1 to 5.5, md5() to SHA512 using pbkdf2, salt and multiple iterations, MySQL functions to PDO, HTML4 to 5. I'm learning about and trying to implement prepared statements, exceptions and OOP, thanks in large part to reading StackOverflow. Its a lot to wrap my head around and its not done yet (just starting) but I just want to make sure I am on the right track. I connect to MySQL on the webserver for log in/user management and I connect to Sybase on a fileserver for client account database. I have looked into classes but not sure I am ready to go there yet. So please tell me how the code looks, what I can do different/better, logic, flow.... thanks

<?php
set_include_path( 'c:/server/web/new/includes' );

require 'functions.php';
debug_errors();
secure_session_start();

require 'pbkdf2.php';
require 'connect_mysql.php';
require 'connect_sybase.php';



if( isset( $_SESSION[ 'user_id' ] ) )
{
    header( "Location: index.php" );
    exit();
}



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

    try
    {
        $pdo_mysql = pdo_mysql();
        $pdo_sybase = pdo_sybase();

        $user_id = $_POST[ 'user_id' ];
        $password = $_POST[ 'password' ];

        if( !( preg_match( '/^[a-zA-Z0-9]{1,60}$/', $user_id ) ) || strlen( $password ) < 1 || strlen( $password ) > 1024 )
        {
            $error = 'Please enter a valid User ID and Password';
        }
        else 
        {

            try
            {
                $mysql_select = $pdo_mysql->prepare( "SELECT userid, password, email, login_attempts FROM users WHERE userid = :userid LIMIT 1" );
                $mysql_select->bindParam( ':userid', $user_id );
                $mysql_select->execute();

                if( $mysql_select->rowCount() == 1 )
                {
                    $row = $mysql_select->fetch( PDO::FETCH_ASSOC );

                    if( $row[ 'login_attempts' ] == 5 )
                    {
                        $error = 'Your login account has been locked.';
                    }
                    else
                    {
                        if( validate_password( $password, $row[ 'password' ] ) ) //password is correct
                        {
                            //session stuff here
                            //header to index
                        }
                        else //password is wrong
                        {
                            $error = 'Incorrect User ID and/or Password';

                            $login_attempts =++ $row[ 'login_attempts' ];

                            $mysql_update = $pdo_mysql->prepare( "UPDATE users SET login_attempts = :login_attempts WHERE userid = :userid" );
                            $mysql_update->bindParam( ':login_attempts', $login_attempts );
                            $mysql_update->bindParam( ':userid', $row[ 'userid' ] );
                            $mysql_update->execute();

                        }

                    }

                }
                else
                {
                     $error = 'Incorrect User ID and/or Password';
                }

            }
            catch( PDOException $ex )
            {
                $error = "A database query error occured.";
                log_action( "pdo_mysql", "(" . $ex->getCode() . ")" . $ex->getMessage() );
            }

        }

    }
    catch( PDOException $ex )
    {
        $error = "Unable to connect to database.";
        log_action( "pdo_mysql", "(" . $ex->getCode() . ")" . $ex->getMessage() );
    }

}



?>
<!doctype html>

<html lang="en">

<head>
    <meta charset="utf-8" />
    <title>login</title>
</head>

<body>
    <div>login page</div>

    <?php
    if( !( empty( $error ) ) )
    {
        echo $error;
    }
    ?>

    <form action="login.php" method="post">
        <label for="user_id">User ID</label>
        <input type="text" name="user_id" />
        <label for="password">Password</label>
        <input type="text" name="password" />
        <input type="hidden" name="process" value="true"/>
        <input type="submit" name="submit" value="Submit" />
    </form>
</body>

mseancole · Accepted Answer · 2012-09-20 19:22:39Z

Since you are updating anyways, you should just update to the most recent version of PHP if you can. I believe its supposed to be faster in addition to offering a bunch of new toys to play with, but I haven't gotten IT to update me yet and I haven't gotten around to playing with it in my spare time :(

I'm not sure about any other devs, but I tend to avoid messing with the include path. Instead, I just provide the full path whenever necessary. But I'd be interested in hearing what the others think about this.

require $_SERVER[ 'DOCUMENT_ROOT' ] . 'relative/path/to/file.php';

If you are trying to learn OOP, then you are going to have to learn how to use functions first. The main problem with this script is the flow and legibility. Both would benefit quite a bit from the use of functions. Once you have everything set up in a function, then you will want to take a look at the Single Responsibility Principle to help you separate the logic into separate functions, and those functions into separate files. Then you are going to want to apply the "Don't Repeat Yourself" (DRY) Principle to them to make them more efficient. Once you have these concepts mastered, adapting that knowledge to OOP should be relatively simple. Trying to do it backwards is just going to confuse you. I'll try and explain some of the basics of the above concepts below.

Another way to check if a form has been submitted is to use the super global, $_SERVER[ 'REQUEST_METHOD' ]. Both methods are fine, but I think having a form element just to verify that POST was done with YOUR form is a security measure that is easily bypassed. The only exception might be a unique key generated for each session id with a limited shelf life, but this is getting into some more advanced stuff that I don't have much experience in.

if( $_SERVER[ 'REQUEST_METHOD' ] == 'POST' ) {

While the concept of indenting your code is good, having to indent your code over large blocks and nesting is not. This is called the Arrow Anti-Pattern. Google it to learn more, but essentially it states that heavy indentation is bad. Break early if necessary by using returns, or by reversing if/else statements so that the larger block is outside of the if/else logic and only the smaller remains then use a return in the remaining if block to prevent further parsing. For example

if( ! isset( $_POST[ 'process' ] ) ) {
    return FALSE;
}

//rest of code

Of course this concept will only get you so far without functions, so you'll have to implement some first. The same also holds true for try/catch blocks.

Something from 5.2 that is still available in the newer versions is a neat, but little-known, function called filter_input(). This function makes REGEX unnecessary by using built in filtering/sanitizing/validating. Eliminating REGEX is always a plus in my book. Take a look at the documentation for it for a full list of what it can do, but here's an example:

$user_id = filter_input( INPUT_POST, 'user_id', FILTER_SANITIZE_STRING );

Those $error strings are perfect examples of "early return" candidates. Returning in the case of an error ensures that no additional overhead is accrued when not necessary. Of course the drawback of this is that you can't register more than one error at a time, but sometimes this is more effort than it is worth. Besides, the way you have your current structure set up, that's all you would have gotten anyways.

I'm not too certain what you are doing when you say "session stuff here" and "header to index", but it looks like it might be violating DRY by repeating the first redirect. Again, this is where functions would come in handy. I feel there is going to need to be more rework here, but I can't grasp what that will be at this point. The main thing that is throwing me right now is the need for an else statement. I feel incrementing the attempt before attempting authentication might be a better tactic. I'm assuming you reset this once validated, so the increment wont make a difference then. Although, an even better tactic might be to use a session variable for this, and once that variable has been incremented to the max, then log that the account has been locked. No need to access the database so frequently for something so trivial.

Let's see... I went over the Arrow Anti-Pattern and DRY. That leaves the Single Responsibility Principle. You don't really have a good example for me to demonstrate with, but let's look at the following bit of code. This is one of the more obvious candidates for a function. Lets say the function is called reloadIndex(). That's a horrible name, but bare with me. Going by that name we can assume that the function is going to reload the index. Looking at the code we can confirm this, but it is also checking if the session variable "user_id" is set first. This is not following that principle. This function should only be concerned with reloading the index, not what is required before the index can be reloaded. That's the responsibility of whatever is calling this function. That's not to say that a function can't do more than one thing, but everything it does should be related to what it is saying its going to do. So for instance, the function that would call this one might be called authenticate(), and this is what it does on success.

function reloadIndex() {
    if( isset( $_SESSION[ 'user_id' ] ) )
    {
        header( "Location: index.php" );
        exit();
    }
}

The last thing I would like to point out is that your catch statements are redundant. I'm not too familiar with try/catch, but if I'm remembering correctly, you throw an interior try error, and catch it on the outside. But, as I pointed out above, this also violates the Arrow Anti-Pattern. The easiest way to fix this is to only encapsulate the creation of the database object in the try block, as that is the only thing being tried. And then do the same with the query.

try {
    $pdo_mysql = pdo_mysql();
} catch( PDOException $ex ) {//this will break execution
    $error = "Unable to connect to database.";
    log_action( "pdo_mysql", "(" . $ex->getCode() . ")" . $ex->getMessage() );
}

//rest of code assuming success

Yes, PHP 5.4 is better than PHP 5.3. It's faster and it is easier to code with. We expect the next PHP 5.5 version not that late, too. So if you do not want to jump over one version and still eager, try PHP 5.4 today. It's mainly backwards compatible, so you can normally just replace your current PHP binary with the new one. You can keep the old one if you want to run both next to each other (I sometimes do that for tests for community projects, so next to PHP 5.4 I can run them against 5.3, too, but normally this is not needed). — hakre, Sep 21 '12 at 11:39
Thanks. Ive researched what you've said and have already begun improving. I am moving stuff into functions (where I can figure out how) and I am seeing the value of early returns. my arrow anti patterns are also disappearing. The reason for only PHP 5.3.5 is that its the last VC6 version and I didn't wan t to upgrade my Apache also. — bdscorvette, Sep 26 '12 at 17:05

Community · Accepted Answer · 2017-05-23 12:41:01Z

First of all, if you are aware of a security issue, fix it first and deploy the fixes. That is before doing something new.

Related to this would be to switch to prepared SQL statements, which on it's own might already take you some time. You should write yourself some procedural helper functions for an easier transition.

Doing so will also help you to identify all the places where your current code-base needs maintenance and love.

You can then better decide with all you want to do, if that is possible.

And btw., why didn't you list SVN -> GIT or similar? Probably you're totally missing the uttermost important tooling here? What is your version control? Do you have got any?

procedural->oop

Apart from switching to PDO - which has got an object oriented interface - I would not suggest you to switch to OO too much. From the code example you give, you should first learn more procedural and improve your style with it, you will then pick OO when you actually need it.

Concepts like DRY work very well in many paradigms.

You might be also interested in a sort of checklist:

How to implement MVC style on my PHP/SQL/HTML/CSS code?

Stack Exchange Network

current community

your communities

more stack exchange communities

moving from->to: mysql->pdo, md5->pbkdf2, procedural->oop

2 Answers 2

Your Answer

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

Hot Network Questions

moving from->to: mysql->pdo, md5->pbkdf2, procedural->oop

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions