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?
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.
ReplyDeleteThat 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().
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.
ReplyDeleteAlso, 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");
}
}
}