Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So I have a php authentication script and everything works fine. But I'm very unsure about the way I programmed it (i hardcoded some stuff) and I was hoping stack could look through this and point out any potential problems. Here's the script:

<?php
require_once 'Bcrypt.php';
class Mysql {
    private $conn;

    function __construct() {
        $this->conn =  new PDO('mysql:host=***;dbname=***;charset=UTF-8','***','***') or 
                      die('There was a problem connecting to the database.');
    }

    function verify_Username_and_Pass($un, $pwd) {
        ini_set('display_errors', 'On');
        error_reporting(E_ALL | E_STRICT);
        $query = "SELECT *
                FROM Conference
                WHERE Username = :un";

        $stmt = $this->conn->prepare($query);

        $stmt->bindParam(':un', $un);
        //$stmt->bindParam(':pwd', $pwd);
        $stmt->execute();
        $row = $stmt->fetchAll();
        $hash = $row[0]["Password"];
        $is_correct = Bcrypt::check($pwd, $hash);

        if ($is_correct) {
            // User exist
            $firstName = $row[0]["First Name"];
            $_SESSION["FirstName"] = $firstName;
            return true;
            $stmt->close();
        }
        else {
            // User doesn't exist
            return false;
            $stmt->close();
        }
    }
}
?>

So how does it look?

share|improve this question

1 Answer 1

I've tried cleaning it up a bit here, removed instances of setting a variable once and using it directly below where it was set (yes, improves readability, but still).

require_once 'Bcrypt.php';

class Mysql {
    private $conn;
    private $host    = 'host';
    private $dbName  = 'dbname'; 
    private $charset = 'UTF-8';
    private $checkUserQuery = 'SELECT * FROM Conference WHERE Username = :un';

    function __construct() {
        $this->conn =  new PDO('mysql:host=' . $this->host . ';dbname=' . $this->dbname . ';charset=' . $this->charset) 
                       or die('There was a problem connecting to the database.');
    }

    function verifyUsernameAndPass($un, $pwd) {
        $stmt = $this->conn->prepare($this->checkUserQuery);

        $stmt->bindParam(':un', $un);
        #$stmt->bindParam(':pwd', $pwd);
        $stmt->execute();
        $row = $stmt->fetchAll();
        $stmt->close(); # Moved up, so closes before we return/exit function

        # You're setting $hash here, and using it directly below.
        # See new code below
        //$hash = $row[0]['Password'];
        //$is_correct = Bcrypt::check($pwd, $hash);

        //if ($is_correct) {
        if( Bcrypt::check($pwd, $row[0]['Password']) ){
            # User exist

            # You're setting $firstName once, and using it directly below it.
            # See new code below
            //$firstName = $row[0]['First Name']; 
            //$_SESSION['FirstName'] = $firstName;

            $_SESSION['FirstName'] = $row[0]['First Name'];

            return true;
        } else {
            # User does not exist
            return false;
        }
    }
}

And here's the same code, without my comments:

require_once 'Bcrypt.php';

class Mysql {
    private $conn;
    private $host    = 'host';
    private $dbName  = 'dbname'; 
    private $charset = 'UTF-8';
    private $checkUserQuery = 'SELECT * FROM Conference WHERE Username = :un';

    function __construct() {
        $this->conn =  new PDO('mysql:host=' . $this->host . ';dbname=' . $this->dbname . ';charset=' . $this->charset) 
                       or die('There was a problem connecting to the database.');
    }

    function verifyUsernameAndPass($un, $pwd) {
        $stmt = $this->conn->prepare($this->$checkUserQuery);

        $stmt->bindParam(':un', $un);
        $stmt->execute();
        $row = $stmt->fetchAll();
        $stmt->close();

        if( Bcrypt::check($pwd, $row[0]['Password']) ){ # User exists
            $_SESSION['FirstName'] = $row[0]['First Name'];
            return true;
        } else { # User does not exist
            return false;
        }
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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