TalkPHP

TalkPHP (http://www.talkphp.com/forums.php)
-   Advanced PHP Programming (http://www.talkphp.com/advanced-php-programming/)
-   -   Please rip my code to pieces: generic user management class (http://www.talkphp.com/advanced-php-programming/3261-please-rip-my-code-pieces-generic-user-management-class.html)

adamsargant 08-19-2008 06:14 PM
Please rip my code to pieces: generic user management class
 
I'm working on my first proper (i.e non "Hello World") class, a generic class to manage users who might be logged into a user restricted area. I would be grateful if you would take it and rip it to pieces... I was going to ask you to be gentle, but don't, because if I've done anything really stupid, I want to know

It is definitely a work in progress, but I'm at a point where constructive feedback would be useful in improving my standards

Thanks

Adam

Code:
<?
/*Methods
 * $user->getuser($username,$password);
 * returns an object that represents details of the user, log-in status, etc
 *
 * $user->setuser($username, $password, $useremail, $usertype);
 * attempts to enter a new user after checking that username and email are unique and valid
 *
 * $user->validateuser();
 * carries out checks to see if the current user object is valid and hasn't been hijacked by session fixation
 *
 * $user->changepassword($username, $oldpassword, $newpassword)
 * changes password only if logged in and if username and old password entered correctly
 *
 * $user->generatepassword($username, $useremail)
 * create a new 12 digit password and email it to the user... does not require that user is logged in, on the assumption that if they are logged in, they know their password
 *
 * $user->changeemail($password, $useremail)
 * changes the users email address, must be logged in and supply password again
 *
 *Properties
 * $user->validlogin
 * boolean true or false
 * $user->loggedin
 * boolean true or false
 * $user->username
 * like it says in the tin
 * $user->useremail
 * like it says in the tin
 * $user->password
 * actually the sha1 encryption of the (username + the password)
 * $user->usertype
 * an integer corresponding to the usertype in the database
 * $user->error
 * indicates if the method was called succesfully
 * $user->errormessage[]
 * array containing a description of any errors that may have occurred
 * $user->usersIP
 * contains the users IP at the time the method was called
 * $user->usersbrowser
 * contains the users User Agent at the time the method was called
 *
 *
 *
 */
class user{
        private $mysqli;
        private $checkuseremail;
        private $checkusername;
        private $regexp;
        private $result;
        private $row;
        private $valid;
        public $validlogin;
        public $loggedin;
        public $username;
        public $useremail;
        public $password;
        public $usertype;
        public $error;
        public $errormessage;
        public $usersbrowser;
        public $usersIP;

        function user(){
                require('includes/config.php');
                $this->mysqli = new mysqli($dbhost, $dbusername, $dbpassword, $dbname);
        }

        function getuser($username, $password){
                unset($this->errormessage);
                $username=$this->mysqli->real_escape_string($username);
                $password=sha1($username.$password);
                $result=$this->mysqli->query("SELECT * FROM userdata WHERE username='$username' and password='$password'");
                $this->username=$username;
                $this->password=$password;
                if($result->num_rows==1){
                        session_regenerate_id();
                        $row=$result->fetch_object();
                        $this->validlogin=TRUE;
                        $this->loggedin=TRUE;
                        $this->usertype=$row->usertype;
                        $this->useremail=$row->useremail;
                        $this->error=FALSE;
                        $this->errormessage[]=FALSE;
                }
                else{
                        $this->validlogin=FALSE;
                        $this->loggedin=FALSE;
                        $this->usertype=FALSE;
                        $this->useremail=FALSE;
                        if(!$this->mysqli->query("SELECT * FROM userdata WHERE username='$username'")){
                                $this->errormessage[]="That username is not registered";
                        }
                        if(!$this->mysqli->query("SELECT * FROM userdata WHERE password='$password'")){
                                $this->errormessage[]="That password is not registered";
                        }
                        if($result->num_rows>1){
                                $this->error=TRUE;
                                $this->errormessage[]="There are multiple entries with those details!";
                        }                               
                }
                $this->usersIP=$_SERVER['REMOTE_ADDR'];       
                $this->usersbrowser=$_SERVER['HTTP_USER_AGENT'];       
        }

        function setuser($username, $password, $useremail, $usertype){
                unset($this->errormessage);
                $username=$this->mysqli->real_escape_string($username);
                $password=sha1($username.$password);
                $usertype=$this->mysqli->real_escape_string($usertype);
                $useremail=$this->mysqli->real_escape_string($useremail);
                $this->username=$username;
                $this->password=$password;
                $this->usertype=$usertype;
                $this->useremail=$useremail;
                $this->loggedin=FALSE;
                $this->error=FALSE;
                $validate_email=$this->validate_email($useremail);
                if($validate_email==1){
                        $checkusername=$this->mysqli->query("SELECT * FROM userdata WHERE username='$username'");
                        $checkuseremail=$this->mysqli->query("SELECT * FROM userdata WHERE useremail='$useremail'");
                        if($checkusername->num_rows>0){
                                $this->error=TRUE;
                                $this->errormessage[]="That username has already been taken";
                        }
                        if($checkuseremail->num_rows>0){
                                $this->error=TRUE;
                                $this->errormessage[]="That email is already associated with an account";
                        }               
                        if(!$this->error){
                                $this->validlogin=TRUE;
                                if($this->mysqli->query("INSERT INTO userdata (username, useremail, password, usertype) VALUES ('$username','$useremail','$password','$usertype')")){
                                        $this->errormessage[]=FALSE;                       
                                }
                                else{
                                        $this->error=TRUE;
                                        $this->errormessage[]="Unable to register that username and password";               
                                        $this->validlogin=FALSE;
                                }
                        }
                        else{
                                $this->validlogin=FALSE;
                        }
                }
                else{
                        $this->error=TRUE;
                        $this->errormessage[]=$validate_email;
                }
                $this->usersIP=$_SERVER['REMOTE_ADDR'];       
                $this->usersbrowser=$_SERVER['HTTP_USER_AGENT'];       
        }

        function changepassword($username, $oldpassword, $newpassword){
                unset($this->errormessage);
                $this->error=FALSE;
                $username=$this->mysqli->real_escape_string($username);
                $oldpassword=sha1($username.$oldpassword);
                $newpassword=sha1($username.$newpassword);
                //first checked logged on
                if($this->loggedin){
                        if($this->username!==$username){
                                $this->error=TRUE;
                                $this->errormessage[]="You can only change your own password!!";                               
                        }
                        if($this->password!==$oldpassword){
                                $this->error=TRUE;
                                $this->errormessage[]="Your current password was entered incorrectly";                               
                        }
                        if(!$this->error){
                                if($this->mysqli->query("UPDATE userdata set password='$newpassword' WHERE username='$username' and password='$oldpassword'")){
                                        $this->errormessage[]=FALSE;
                                        $this->password=$newpassword;                       
                                }
                                else{
                                        $this->error=TRUE;
                                        $this->errormessage[]="Unable to update password";               
                                        $this->validlogin=FALSE;
                                }                               
                        }
                }
                else{
                        return "You need to be logged in to change password";
                }
        }
       
        function changeemail($password, $useremail){
                unset($this->errormessage);
                $this->error=FALSE;
                $useremail=$this->mysqli->real_escape_string($useremail);
                $password=sha1($this->username.$password);
                //first checked logged on
                if($this->loggedin){
                        $validate_email=$this->validate_email($useremail);
                        if($validate_email==1){
                                if($this->password!==$password){
                                        $this->error=TRUE;
                                        $this->errormessage[]="Your password was entered incorrectly";                               
                                }
                                if(!$this->error){
                                        if($this->mysqli->query("UPDATE userdata set useremail='$useremail' WHERE username='$this->username' and password='$password'")){
                                                $this->errormessage[]=FALSE;
                                                $this->useremail=$useremail;                       
                                        }
                                        else{
                                                $this->error=TRUE;
                                                $this->errormessage[]="Unable to update email";               
                                        }                               
                                }
                        }
                        else{
                                $this->error=TRUE;
                                $this->errormessage[]=$validate_email;                                       
                        }
                }
                else{
                        return "You need to be logged in to change email";
                }
               
        }
       
        function generatepassword($username, $useremail){
                $newpassword="";
                $username=$this->mysqli->real_escape_string($username);
                $useremail=$this->mysqli->real_escape_string($useremail);
                $password = "";
                $possible = "0123456789bcdfghjkmnpqrstvwxyz";
                $i = 0;
                while ($i < 12) {
                        $char = substr($possible, mt_rand(0, strlen($possible)-1), 1);
                        if (!strstr($password, $char)) {
                                $password .= $char;
                                $i++;
                        }
                }
                $newpassword=sha1($username.$password);
                if($this->mysqli->query("UPDATE userdata set password='$newpassword' WHERE username='$username' and useremail='$useremail'")){
                        //now mail password... in real life probably use swiftmailer class
                        $subject = 'New Password';
                        $message = "Your new password is $password";
                        $headers = 'From: webmaster@example.net' . "\r\n" .
                                'Reply-To: webmaster@example.net' . "\r\n" .
                                'X-Mailer: PHP/' . phpversion();
                        mail($useremail, $subject, $message, $headers);                       
                }
                else{
                        return "Unable to update password";               
                }                               
        }

        function validateuser(){
                unset($this->errormessage);
                $this->error=FALSE;
                if(isset($this->validlogin)){
                        if($this->usersIP!==$_SERVER['REMOTE_ADDR']){
                                $this->error=TRUE;
                                $this->errormessage[]="This session has been intercepted by a new IP";
                                //actually we are not going to change the status to logged out, as there may be valid reasons why IP has changed
                                //but we might want to log the change or notify administrators
                                //$this->loggedin=FALSE;       
                        }
                        if($this->usersbrowser!==$_SERVER['HTTP_USER_AGENT']){
                                $this->error=TRUE;
                                $this->errormessage[]="This session has been intercepted by a new User Agent...original agent=$this->usersbrowser, new agent=".$_SERVER['HTTP_USER_AGENT'];
                                $this->loggedin=FALSE;
                        }
                }
                else{
                        return "There is no user to validate!";               
                }
        }

        private function validate_email($email){
          // Create the syntactical validation regular expression
          $regexp = "^([_a-z0-9-]+)(\.[_a-z0-9-]+)*@([a-z0-9-]+)(\.[a-z0-9-]+)*(\.[a-z]{2,4})$";
          // Presume that the email is invalid
          $valid = "Invalid Domain";
          // Validate the syntax
          if (eregi($regexp, $email))
          {
                  list($username,$domaintld) = split("@",$email);
                  // Validate the domain
                  if (getmxrr($domaintld,$mxrecords))
                        $valid = 1;
          } else {
                  $valid = "Invalid syntax";
          }
          return $valid;
        }
}

}
?>

Enfernikus 08-19-2008 06:43 PM
Since your already using scope for your variables (private/protected/public) you might as well use the constructor method though that's more out of preference and it just looks cleaner to me. Also preg_match is preferred over eregi

Village Idiot 08-19-2008 10:07 PM
From a quick run though:

-You have that extensive documentation comment block on the top, but almost nothing as you go.

-Your user function should use an existing connection. Not only is a connection not its intended purpose, it makes it harder to port.

-Why both validlogin and loggedin? Wouldn't one imply the other?

-Don't track users by username, use an auto-incrementing primary key int. This way refferences to that user (such as in a URL) will not break. Lets say UserMan changes his name to ManUser. The link User.php?name=UserMan that the person gave to someone last week will no longer work. However, if you use a unique ID that will never change, it can always be User.php?id=1 no matter what changes. It is also faster for a database to track by primary key, not to mention you can index.

-Instead of setting all these variables to false if the query fails, just return false. The code you would use would be something like:
PHP Code:
if($user->getuser($username,$password) == true)
{
code
}
else
{
echo "Login failed";
} 
-Don't assign things to a variable then put that variable to another variable (eg. line 109). Just assign directly to the variable.

-Why assign a class method to a variable and run it in a context where you could just call the member itself (line 119)? Just run the method.

-Line 121, your query would be better suited as something like (I didnt debug it due to lack of your db schema):
Code:
SELECT username, email, COUNT(username) AS nameCount,COUNT(useremail) AS emailCount FROM userdata WHERE username='$username'
You then just check the count of result nameCount and emailCount.

-Don't track sessions by IP, they change too often and are inconsistent..

adamsargant 08-21-2008 11:08 PM
Thanks guys... very useful feedback

dschreck 08-25-2008 05:00 AM
I've stopped relying upon getmxrr(), there are various reasons why, but what it comes down to is it's not reliable.

If you want to make sure the user has a working e-mail address just send a verification e-mail + activation code.


Also be aware that $_SERVER['HTTP_USER_AGENT']; can change while a user is logged in with some browsers.


You also switch between using boolean values, and doing if conditions and checking for 1 for true.
I'd recommend just using one convention, and maybe even using '===' to check your value and type.



Also, your functions only return strings if there's an error, if you ask me, that's what the $errormessge[] array is for.

Inside of classes you should return a true or false, and then retrieving your output from another getter.

Hope that helps a little.

Kalle 08-26-2008 12:52 AM
getmxrr() isn't implemented on Windows so I wouldn't say its reliable if your planning to use your program on cross operating systems =)

ReSpawN 09-12-2008 11:22 AM
Above all, getmxrr() slows down, breaks down or messes up anything you are trying to accomplish. When it's an correct email, it sometimes takes up to 40 seconds, but when it's a non-existing one, it takes up to about 2 min to release you script.

MX Records are depreciated. Final.

A way to fix this, is to perhaps CURL the URL with a POST, to just imply sending the activation email to that address or make a trusted list.
Tweakers.net, here in The Netherlands, doesn't allow cheap emails as hotmail, forfree and stuff. You could also go with that.


All times are GMT. The time now is 02:26 PM.

Powered by vBulletin® Version 3.6.8
Copyright ©2000 - 2013, Jelsoft Enterprises Ltd.
Search Engine Optimization by vBSEO 3.1.0