Bit of advice on O-O user management...

[plastik77]

Hi, I'm still pretty new to PHP and am trying hard to get away from procedural code and to start working on building my own O-O based libraries. I've started on building a user management system which I'll hopefully be able to use for future projects (once it's working properly).

 

I'm really just looking for some feedback on what I've done so far, before i get too far down the road and realise i need to get back to the drawing table!

 

I'm trying to use the MVC pattern and my main classes are as follows:

 

DBConnector.php (db connection info and performs db queries)

Validator.php (validates user inputs)

Sentry.php (processes data)

 

If anyone has the time to provide some feedback that would be great.

 

Here's an example of how the code works at present...

 

html form...

<p><label for="username">Username</label><input type="text" name="user" /></p><br/>
<p><label for="password">Password</label><input type="password" name="pass" /></p><br/>
<p><a href = "login.php?password=1">I forgot my password</a></p>
<p class="submit"><input type="submit" name="Submit" value="Submit" class="submit" /></p>

 

processing form data (at top of form page...not sure if this is good practice or not)

require_once("../includes/Sentry.php");
require_once("../includes/Validator.php");
$sentry = new Sentry();
$validator = new Validator();

//if normal login, validate then process
if (isset($_POST['Submit'])){

$validator->validateGeneral($_POST['user'],'Username');
$validator->validateGeneral($_POST['pass'],'Password');

// Check whether the validator found any problems
if ($validator->foundErrors()){
	$message = "<p><em>Error: There was a problem with the following fields: <br/>".$validator->listErrors(', ')."</em></p>"; // Show the errors, with a comma	between each
}	
else {

	if (!$sentry->checkLogin($_POST['user'],$_POST['pass'],'welcome.php')){ 
		$message = "<p><em>Error: There was a problem with the following fields: <br/>".$sentry->listMessages(', ')."</em></p>"; // Show the errors, with a comma	between each
	}
	else {
		header("Location: http://www.bbc.co.uk");
	}	
}
}

 

The validator class is the controller class, which checks the data before passing it to the model, below (apologies - i know it's quite a lot of code to post)...

 

<?php
////////////////////////////////////////////////////////////////////////////////////////
// Class: sentry
// Purpose: Control access to pages
///////////////////////////////////////////////////////////////////////////////////////
// Include database and create object
require_once ('DbConnector.php');

class sentry
{
    var $userdata; //  Array to contain user data
    var $message; // Array to store messages for user feedback

  /**
   * sentry::sentry()
   * 
   * @return
   */
    function sentry()
    {
        session_start();
        header("Cache-control: private");
    }

    //======================================================================================
    // Log out, destroy session
  /**
   * sentry::logout()
   * 
   * @return
   */
    function logout()
    {
        unset($this->userdata);
        session_destroy();
        return true;
    }

    //======================================================================================
    // Register new user, and either redirect to goodRedirect or badRedirect depending on success
  /**
   * sentry::register()
   * 
   * @param mixed $user
   * @param mixed $pass
   * @param mixed $email
   * @param mixed $goodRedirect
   * @return
   */
    function register($user='', $pass='', $email='', $goodRedirect='') 
    {
        //check if username is valid
        $loginConnector = new DbConnector();
        $checkUser = $loginConnector->query("select count(*), enabled from parents_details where username = '" .
            trim($user) . "' group by username");
        $row = $loginConnector->fetchArray($checkUser);
        if ($row['count(*)'] == 0)
        {
            $this->message[] = 'Username does not exist.' . mysql_error();
            return false;
        }
        //check to see if user has already registered
        if ($this->checkRegistered($user))
        {
            $this->message[] = 'You have already registered.';
            return false;
        }
        //ok to register new user
        $update = sprintf("update parents_details set password = sha1('$pass'), email = '%s', enabled=1 where username = '" .
            trim($user) . "'",
            mysql_real_escape_string($_POST['email']));
        if ($loginConnector->query($update))
        {
           return true; //header not working properly on wamp server - need to change url as appropriate when live

        } else
        {
            $this->message[] = 'There was a problem saving to the database: ' . mysql_error();
            return false;
        }
    }
    // Log in, and either redirect to goodRedirect or badRedirect depending on success
  /**
   * sentry::checkLogin()
   * 
   * @param mixed $user
   * @param mixed $pass
   * @param mixed $goodRedirect
   * @return
   */
    function checkLogin($user='', $pass='', $goodRedirect='') //initialising parameters so that security checks on each page will not require passing parameters to this function
    {
        //create object of db connector class
        $loginConnector = new DbConnector();

        // If user is already logged in then check credentials
        if ($_SESSION['user'] && $_SESSION['pass'])
        {
            $getUser = $loginConnector->query("select * from parents_details where username = '" .
                $_SESSION['user'] . "' AND password = '" . $_SESSION['pass'] . "'");

            if ($loginConnector->getNumRows($getUser) > 0)
            {
                return true;
            } 
            else
            {
                // Existing user not ok, logout
                $this->logout();
                $this->message[] = 'You have been logged out of the system.';
                return false;
            }

            // User isn't logged in, check credentials
        } else
        {
            // Look up user in DB
            $getUser = $loginConnector->query("select * from parents_details where username = '$user'");
            $this->userdata = $loginConnector->fetchArray($getUser);

            //first, check that username is valid
            if ($loginConnector->getNumRows($getUser) == 0)
            {
                $this->message[] = 'Your username is not recognised as a valid account on our system.';
                return false;
            }
            //username is valid, check password is correct for this user
            else
                if ($this->userdata['password'] != sha1("$pass"))
                    {
                        $this->message[] = 'You entered an incorrect password for this username. Please confirm the details and try again';
                        return false;
                    }
                
                else
                    //check that account has been activated
                    if (!$this->checkRegistered($user))
                       {
                        $this->message[] = 'Your account has not yet been activated. Please register using the link on this page.';
                        return false;
                    }
                     else
                    {
                        // Login OK, store session details
                        $_SESSION["user"] = $this->userdata['username'];
                        $_SESSION["pass"] = $this->userdata['password'];
                        //increment login counter
                        $increment = $loginConnector->query("update parents_details set logincount = logincount+1 where username = '" .
                            trim($user) . "'");                            
                        return true;
                    }
        }
    }
    
    //function to check if user had already registered
    function checkRegistered($user='')
    {
        //create object of db connector class
        $loginConnector = new DbConnector();
        
        $checkUser = $loginConnector->query("select enabled from parents_details where username = '" .
            trim($user) . "'");
        $this->userdata = $loginConnector->fetchArray($checkUser);
            if ($this->userdata['enabled'] == 1)
            {
                return true;
            }
            else
            {
                return false;
            }
    }

  /**
   * sentry::requestPassword()
   * 
   * @param mixed $user
   * @param mixed $email
   * @return
   */
    function requestPassword($user, $email)
    {

        //create object of db connector class
        $loginConnector = new DbConnector();
        // Look up user in DB
        $getUser = $loginConnector->query("select * from parents_details where username = '" .
            trim($user) . "'");
        $this->userdata = $loginConnector->fetchArray($getUser);

        //first, check that username is valid
        if ($loginConnector->getNumRows($getUser) == 0)
        {
            $this->message[] = 'Your username is not recognised as a valid account on our system.';
            return false;
        }
        //username is valid, now check that account has been activated
        else
            if ($this->userdata['enabled'] == 0)
            {
                $this->message[] = 'Your account has not yet been activated. Please register using the link on this page.';
                return false;
            } else
                if ($this->userdata['email'] != $email)
                {
                    $this->message[] = 'The email address you submitted does not match the one stored on our records.';
                    return false;
                }
        //process password request
                else
                {
                    $new_pass = $this->generatePassword(); //create random password
                    $update = $loginConnector->query("update parents_details set password = sha1('$new_pass') where username = '" .
                        trim($user) . "'");
                    $this->sendMail($email, $new_pass, "New Password");
                }
    }

  /**
   * sentry::checkMessages()
   * 
   * @return
   */
    function checkMessages()
    {
        if (count($this->message) > 0)
        {
            return true;
        } 
         else
        {
            return false;
        }
    }

    // Return a string containing a list of errors found,
    // Seperated by a given deliminator
  /**
   * sentry::listMessages()
   * 
   * @param string $delim
   * @return
   */
    function listMessages($delim = ' ')
    {
        return implode($delim, $this->message);
    }

    //function to generate a random password
  /**
   * sentry::generatePassword()
   * 
   * @return
   */
    function generatePassword()
    {
        $salt = "abchefghjkmnpqrstuvwxyz0123456789";
        srand((double)microtime() * 1000000);
        $i = 0;

        while ($i <= 
        {
            $num = rand() % 33;
            $tmp = substr($salt, $num, 1);
            $pass = $pass . $tmp;
            $i++;
        }
        return $pass;
    }
    
    //function for processing email sending
    function sendMail($email, $content, $subject){

        //email from field
        $mailfrom = "From: [email protected]";

        if (@mail($email, $subject, $content, $mailfrom)) {

             $this->message[] = 'Your new password has been sent to you.';
             return true;
        }
        else {

             $this->message[] = 'There was a problem sending the email.';
             return false;
        }
    }
}
?>

And finally, the DBConnector class....

<?php
////////////////////////////////////////////////////////////////////////////////////////
// Class: DbConnector
// Purpose: Connect to MySQL database
///////////////////////////////////////////////////////////////////////////////////////
require_once 'SystemComponent.php';
class DbConnector extends SystemComponent
{
    var $theQuery;
    var $link;

    //*** Function: DbConnector, Purpose: Connect to the database ***
    function DbConnector()
    {
        // Load settings from parent class
        $settings = SystemComponent::getSettings();

        // Get the main settings from the array we just loaded
        $host = $settings['dbhost'];
        $db = $settings['dbname'];
        $user = $settings['dbusername'];
        $pass = $settings['dbpassword'];

        // Connect to the database
        $this->link = mysql_connect($host, $user, $pass);
        mysql_select_db($db);
        register_shutdown_function(array(&$this, 'close'));
    }

    //*** Function: query, Purpose: Execute a database query ***
    function query($query)
    {
        $this->theQuery = $query;
        return mysql_query($query, $this->link);
    }

    //*** Function: getQuery, Purpose: Returns the last database query, for debugging ***
    function getQuery()
    {
        return $this->theQuery;
    }

    //*** Function: getNumRows, Purpose: Return row count***
    function getNumRows($result)
    {
        return mysql_num_rows($result);
    }

    //*** Function: fetchArray, Purpose: Get array of query results ***
    function fetchArray($result)
    {
        return mysql_fetch_array($result);
    }
    //*** Function: close, Purpose: Close the connection ***
    function close()
    {
        mysql_close($this->link);
    }
}
?>

[RMcLeod]

Firstly I noticed you are usng PHP 4, if it's available I'd recommend switching to PHP 5. PHP 5's Object Orientated support is a lot better, for instance PHP 5 supports abstract classes and interfaces, the static keyword and access control for member variables and methods (public, protected and private). All of these added features mean you can implement a lot of very useful OO patterns that aren't possible (at least without messy work arounds) in PHP 4. A prime candidate in your code is the SystemCompnent, this should really be a singleton

 

I noticed that a lot of your class functions are just wrappers for in built PHP functions, whilst they work fine they are a bit pointless, and in some circumstances are still open to the same security vulnerabilites as the PHP functions they wrap, your mail function is a prime example of this.

 

If you really want to learn OO PHP I would suggest making the switch to PHP 5, and I highly recommend the book PHP 5 Objects, Patterns and Practice by Matt Zandstra.