1
\$\begingroup\$

Context:

I am working on the login module of a PHP web-application and I am trying to implement this using PHP/PDO and the DAO pattern. I would like someone to review the code below and let me know whether my understanding is correct or not,i.e, am I implementing it correctly.

The application would require the user to provide his/her username and password and then it would just check the database whether he/she is a valid user or not.

user_master:

enter image description here

Project Structure:

enter image description here

login_handler.php:

<?php

require ("include_dao.php");

// Fetch the supplied username and password.
$username = $_POST['username'];

$password = $_POST['password'];

// Instantiate and populate UserMaster bean.
$usermaster = new UserMaster;

$usermaster->setUsername($username);

$usermaster->setPassword($password);

// Instantiate and invoke the desired DAO method.
$usermasterDAO = new UserMasterMySqlDAO;

$usermasterDAO->queryByUsernameAndPassword($username, $password);

?>

queryByUsernameAndPassword():

public function queryByUsernameAndPassword($username, $password) {

        // Initiate DatabaseHandler instance.
        $databaseHandler = new DatabaseHandler;

        // Establish a connection
        $pdoConnection = $databaseHandler -> connect();

        try {
            $query = $pdoConnection -> prepare('SELECT * FROM cbiz.user_master WHERE username = ? AND password = ?');

            $query -> bindParam(1, $username, PDO::PARAM_STR);

            $query -> bindParam(2, $password, PDO::PARAM_STR);

            $query -> execute();

            if ($query -> execute()) {
                echo "TRUE";
            } else {
                echo "FALSE";
            }

        } catch (PDOException $pdoException) {// Write into log and show the exception message.

            $databaseHandler -> closeConnection();

            echo $databaseHandler -> logException($pdoException -> getMessage());

            die();
        }
    }

DatabaseHandler - Constructor:

/*
     * ###################### Constructor.######################
     *
     * 1. Instantiate Log class
     * 2. Connects to the database.
     * 3. Creates the parameters array.
     */
    public function __construct() {
        $this -> log = new LogHandler();
        $this -> connect();
    }

DatabaseHandler - connect():

/**
     *  ########## METHOD 1 - ESTABLISHES A CONNECTION TO THE DATABASE #########
     *
     * 1. Reads the database settings from an ini file.
     * 2. Tries to connect to the database.
     * 3. If connection failed, exception is displayed and a log file gets created.
     */
    public function connect() {

        $path_to_config_file = $_SERVER['DOCUMENT_ROOT'] . '/SMS/core/config/' . 'config.ini';

        // Read settings from INI file.
        $this -> settings = parse_ini_file($path_to_config_file);

        // Set the Data Source name - mysql:host=127.0.0.1;dbname=cbiz
        $dsn = 'mysql:dbname=' . $this -> settings["dbname"] . ';host=' . $this -> settings["host"] . '';

        $username = $this -> settings["db_user"];

        $password = $this -> settings["db_password"];

        // Set UTF8 .
        $options = array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8', );

        try {

            $this -> pdoObject = new PDO($dsn, $username, $password, $options);

            // Log any exceptions on Fatal error.
            $this -> pdoObject -> setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

            // Disable emulation of prepared statements, use REAL prepared statements instead.
            $this -> pdoObject -> setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

            // Connection succeeded, set the boolean to true.
            $this -> isConnected = true;

            // echo "Connection established!";          

            return $this->pdoObject;

        } catch (PDOException $pdoException) {// Write into log and show the exception message.

            echo $this -> logException($pdoException -> getMessage());

            die();
        }

    }

Any help, suggestions, criticisms are appreciated.

\$\endgroup\$
1
\$\begingroup\$
  • You have way too many newlines. Newlines should separate logical units of code, not each line from each other, because this actually makes your code harder to read.
  • Be consistent with your spacing. Either always surround -> with spaces, or never do so.
  • Don't echo or die in classes, as it takes options away from the calling classes (they can't change the output, they can't recover from errors, etc).
  • Most of your comments don't add all that much, they just repeat the code, which actually leads to less readable code (a reader has to find the comments that actually matter).
  • Hash your passwords.
  • Do you expect the need for a different DBMS in the near future? Because if not, I wouldn't design for it now. Having UserMasterDAO and UserMasterMySqlDAO without the need for it just adds additional complexity, with little benefit (if you do need to support a different DBMS in the far future, extracting the interface of the DAO can be easily done with any IDE).
  • You call connect in the constructor of the handler, but after constructing it, you call connect a second time.
  • You also execute twice.
  • It's not all that clear what UserMaster is needed for right now (you set the username and password, but never use it anywhere).
\$\endgroup\$
  • \$\begingroup\$ Points 1,2, 7 and 8. fixed. As to the last point, I was thinking it might be better to pass the bean itself to the queryByUsernameAndPassword() rather than the values. Since the original post is bit too long, I have added the modified code as pastebin link here. PHP/PDO - DatabaseHandler class, UserMasterMySqlDAO - queryByUsernameAndPassword(). You may want to have a look and suggest anything further. \$\endgroup\$ – Sandeep Chatterjee Mar 18 '15 at 22:10
  • \$\begingroup\$ @Sandeep regarding passing the model itself to the DAO: for persisting it that's great, for the login check I probably wouldn't do it. Regarding your linked queryByUsernameAndPassword: The DAO shouldn't redirect, that is the task of the controller. \$\endgroup\$ – tim Mar 18 '15 at 22:14
  • \$\begingroup\$ Ok, so what would be the correct way to handle the control flow in this case? \$\endgroup\$ – Sandeep Chatterjee Mar 18 '15 at 22:22
  • \$\begingroup\$ just return execute() in queryByUsernameAndPassword, rename it to something like checkLogin, and then use it in your login_handler pseudo-code): if (checkLogin(user, pass)) { header(ok); { else header(error); } \$\endgroup\$ – tim Mar 18 '15 at 22:28
  • \$\begingroup\$ @Sandeep oh, just one more thing. I was going off your original code, but of course you should not return $query->execute(), because that just returns the success/failure of the query, not if the user/password combo exists. I'm pretty sure that your query will always return true. But still, the idea should hopefully be clear. \$\endgroup\$ – tim Mar 18 '15 at 22:34
3
\$\begingroup\$

We clearly only see the portions of your code, you want us to comment on. I can understand that, the question would get too long. Your question is very broad though: Whether you 'understand' it. That's rather difficult to say from these parts. It seems like you do, but I did notice some obvious things:

Never store passwords

It is good practice never to store password in a database, not even encrypted. What you store is a hash and a salt, and then you use these to see if access is allowed. It would be a very long answer to explain this all, so take a look here: https://stackoverflow.com/questions/401656/secure-hash-and-salt-for-php-passwords and here: https://crackstation.net/hashing-security.htm which explains it much better than I can.

Open database once, then reuse

I assume you have one, or perhaps a few, databases with a lot of, perhaps quite unrelated information in them. Just one of the many tables present might be your user_master table. You even have a nice INI file which contains your database access information. (Make sure this INI file is not served to the world through your webserver.)

It is a bit strange that you open a connection to the database within the queryByUsernameAndPassword() method. This method shouldn't be doing that, it's got one task: To see if the username and password occur in the database. A better name would be: userExists().

My advice would be to open a connection to the database at the beginning of your application, and then reuse this throughout the application. Opening a database is a lengthy process which you do not want to repeat many times.

\$\endgroup\$
  • \$\begingroup\$ Ok. So, Password Hashing is something very important which I need to work on - thanks for the links. :) \$\endgroup\$ – Sandeep Chatterjee Mar 18 '15 at 20:37
  • \$\begingroup\$ As to your second point, I agree with you that opening/releasing a database connection is very crucial. That is the reason why I have a closeConnection() method(not posted in the original question) which sets pdoObject to null as soon as the connection is released. Besides, I have an interface UserMasterDAO and UserMasterMySqlDAO is its DAO implementation. So, that is where I thought is the best place to open the connection since I would like to separate the database logic from the business and presentation logic. \$\endgroup\$ – Sandeep Chatterjee Mar 18 '15 at 20:37
  • \$\begingroup\$ @Sandeep If I understood KIKO correctly, I think the point was that you should pass a DatabaseHandler to the queryByUsernameAndPassword as argument, so the same connection can be used for this method, other methods in your user DAO, and then also DAOs for different models than the user. \$\endgroup\$ – tim Mar 18 '15 at 22:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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