Converting MySQL to PDO

Question

I've been advised a couple of times on here to start changing my code to PDO and I've finally got round to doing it. My problem is that I'm having incredible difficulty with converting my existing login script. For the last few lines of the code below (after the line $result = $query->fetchAll(); ) I haven't been able to find any resources online that could help me re-write it:

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

$db=getConnection();

$username = mysql_real_escape_string($username);

$query = $db->prepare( "SELECT password, salt, 'employer' as user_type
FROM JB_Employer
WHERE Username = '$username'

UNION
SELECT password, salt, 'jobseeker' as user_type
FROM JB_Jobseeker
WHERE User_Name = '$username'");

$result = $query->fetchAll();

$qData = mysql_fetch_array($result, MYSQL_ASSOC);
$hash = hash('sha256', $qData['salt'] . hash('sha256', $password) );

if ($result -> rowcount() <1 ;) { print “Fail, No such user”;}

if ($hash != $qData['password']) { header('Location: register.php?loginStatus=fail');  exit;}

else {$_SESSION['user'] = $username;
$_SESSION['permission'] = $qData['user_type'];}

Can anyone advise how I could go about acheiving this?

How many records do you expect the result set to have? I.e. can there be only 0 or 1 records for $username in all three tables? Why are there three distinct yet structurally identical tables for the same purpose? — VolkerK
– VolkerK, Commented Feb 9, 2012 at 14:52
Sorry, there are actually only two tables, that was my mistake, I've updated it now. The query returns one user who will either be in the JB_Jobseeker or JB_Employer table. — David
– David, Commented Feb 9, 2012 at 14:56
But still, why is there more than one table for this? In it's simplest form you could have a field user_type to the table instead of adding it to the resultset. — VolkerK
– VolkerK, Commented Feb 9, 2012 at 15:03
Yes I agree, unfortunatly I'm doing this as part of a group project and I had no say in the design of the database. A bit stupid I know but I'm having to make the best of it and this is all I could come up with. — David
– David, Commented Feb 9, 2012 at 15:07

Daan Timmer · Accepted Answer · 2012-02-09 15:29:06Z

Take a look at this and please concider your code again especially regarding the XSS vulnerability! Also, for a good developers sake, refactor/rewrite your database. This is ALL but the way to go.

Also, code is untested.

<?php

$db = getConnection(); //assuming you are returning a PDO object here!

$username = getUsername(); //assuming you are NOT escaping the username!
$password = getPassword(); //assuming your hashed password here!

$query = "SELECT password, salt, 'emplyer' as user_type
FROM  JB_Employer
WHERE Username = :username

UNION

SELECT password, salt, 'jobseeker' as user_type
FROM JB_Jobseeker
WHERE User_Name = :username";

//$statement == PDOStatement
$statement = $db->prepare($query);

//bind the $username param to :username, this is the real power of PDO,
//no more SQL Injections. Don't use mysql_real_escape-esque things!
//they are not nececary with PDO
$statement->bindParam(":username", $username);

//execute the statement
if($statement->execute()){
    $result = $statement->fetchAll();

    $rowCount = count($result);

    if($rowCount < 1){
        // redirect?
        die("No Such user");
    }else{
        // more than one user can be possible, this is not the correct way, but it appears to be your way so let's continue
        $firstRow = $result[0];

        if( isPasswordEqual($firstRow['salt'], $password) ){
            $_SESSION['user'] = $username; //security risk here. Vulnerable for XXS
            $_SESSION['permission'] = $firstRow['user_type'];
        }else{
            //Don't tell them this! It will give them knowledge of which accounts do exist.
            //Just say some general message like "login failed"
            die("wrong information");
        }
    }
}

FtDRbwLXw6 · Accepted Answer · 2012-02-09 14:54:36Z

0

You should consider renaming your variables to make more sense. In particular, $db->prepare() returns a statement, not a query. You pass it a query, and it prepares that query and returns a statement. It will save you headaches down the road if you follow this naming convention.

That said, you should change this code:

$result = $query->fetchAll();

$qData = mysql_fetch_array($result, MYSQL_ASSOC);

Into this:

$qData = $query->fetch(\PDO::FETCH_ASSOC);

And the rest should fall into line. PDOStatement::fetch(\PDO::FETCH_ASSOC) returns an associative array, just like mysql_fetch_array(..., MYSQL_ASSOC) or mysql_fetch_assoc().

Edit: Also, you will need to change $result->rowCount() to $query->rowCount().

answered Feb 9, 2012 at 14:54

FtDRbwLXw6

29.1k16 gold badges74 silver badges108 bronze badges

1 Comment

Daan Timmer Over a year ago

@kai, care took look at the example I provided? It gives more information regarding how PDO works :-) (not saying drrcknlsn is wrong ofcourse)

Collectives™ on Stack Overflow

Converting MySQL to PDO

2 Answers 2

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related