Pastulio Posted June 25, 2007 Share Posted June 25, 2007 Ok so my deal is, I have just started coding in OOP and I've written a login class. Just wanted you all to have a look at it, and help me out on how to improve it, because I don't want there to be any unneeded code. Thanks in advance, Pastulio Here is my code login.php <?php /* +---------------------------------------------------+ | | | PHP MySQL login class | | | +---------------------------------------------------+ | Filename : login.php | | Created : 25/06/2007 20:29 | | Created By : Pascal Van Acker (a.k.a Pastulio) | | Email : | | Version : 1.0 | | | +---------------------------------------------------+ */ class login { // MySQL config var $host; // MySQL host (usually 'localhost') var $db_username; // MySQL username var $db_password; // MySQL password var $database; // MySQL database var $connection; // MySQL connection variable var $db_select; // MySQL selection variable var $db_table; // MySQL table selection function db_Connect () { // Connect to the MySQL server $this -> $connection = mysql_connect ($this -> host, $this -> db_username, $this -> db_password); // Test the MySQL connection if (!$this -> connection) { return false; } // Select the MySQL database $this -> db_select = mysql_select_db ($this -> database); // Test the database selection if (!$this -> db_select) { return false; } } // END db_Connect function db_Disconnect () { mysql_close ($this -> connection); } // END db_Disconnect function verify_User ($username, $password) { $query = "SELECT * FROM `$this -> db_table`"; $result = mysql_query ($query); // Loop our results and search for a match while ($user_Info = mysql_fetch_array($result)) { if ($username == $user_Info['username'] && $password == md5 ($user_Info['password'])){ return true; } else { return false; } } } // END verify_User function user_Logged ($ip) { // Check if the user is already logged $query = "SELECT * FROM `$this -> db_table` WHERE `ip` = '$ip'"; $result = mysql_query ($query); if (mysql_num_rows ($result) > 0) { // User was already logged in return true; } else { // User has not logged in return false; } } // END user_Logged function check_Login ($username, $password, $ip, $remember) { // If the database connection is not possible, a login cannot be checked if (!$this -> db_Connect) { return false; } // Check if the user is already logged in if ($this -> user_Logged($ip)){ return true; } else { // verify the user login if (!$this -> verify_User($username, $password)) { return false; } else { // If the user wishes to be remembered, insert him into the database if ($remember == '1') { $query = "UPDATE `$this -> db_table` SET `ip` = '$ip' WHERE `username` = '$username'"; } return true; } } $this -> db_Disconnect; } // END check_Login } /* THE CODE TO INCLUDE IN THE FILE WHERE YOU NEED TO CALL THE LOGIN $login = new login (); // Call the class $login -> host = ''; // Set the MySQL host $login -> db_username = ''; // Set the MySQL username $login -> db_password = ''; // Set the MySQL password $login -> database = ''; // Set the MySQL database $login -> db_table = ''; // Set the MySQL table if ($login->check_Login($username, $password, $ip, $remember)) { // User is logged in } else { // user is not logged in } $login -> db_Disconnect(); */ ?> Quote Link to comment Share on other sites More sharing options...
per1os Posted June 25, 2007 Share Posted June 25, 2007 I would suggest using a different scheme for the database portion. Especially be very cautious about closing the mysql connection. Using mysql_close() isn't usually necessary, as non-persistent open links are automatically closed at the end of the script's execution. See also freeing resources. www.php.net/mysql_close That part is unnecessary. I would look into making a database class and either passing the class to the constructor or creating functions that call the class functions. Anyhow I would suggest making the database connection part at least outside of the user login, as chances are more than just the login will need to use the database. Might as wall make it available to all classes. Quote Link to comment Share on other sites More sharing options...
Pastulio Posted June 25, 2007 Author Share Posted June 25, 2007 Yes I figured that, thanks though but I put it in there simply for testing purposes right now, there will be another class for the connection later on . Thanks a lot though btw, I will remove the mysql_close also So this would be the final version: <?php /* +---------------------------------------------------+ | | | PHP MySQL login class | | | +---------------------------------------------------+ | Filename : login.php | | Created : 25/06/2007 20:29 | | Created By : Pascal Van Acker (a.k.a Pastulio) | | Email : | | Version : 1.0 | | | +---------------------------------------------------+ */ class login { function verify_User ($username, $password) { $query = "SELECT * FROM `$this -> db_table`"; $result = mysql_query ($query); // Loop our results and search for a match while ($user_Info = mysql_fetch_array($result)) { if ($username == $user_Info['username'] && $password == md5 ($user_Info['password'])){ return true; } else { return false; } } } // END verify_User function user_Logged ($ip) { // Check if the user is already logged $query = "SELECT * FROM `$this -> db_table` WHERE `ip` = '$ip'"; $result = mysql_query ($query); if (mysql_num_rows ($result) > 0) { // User was already logged in return true; } else { // User has not logged in return false; } } // END user_Logged function check_Login ($username, $password, $ip, $remember) { // Check if the user is already logged in if ($this -> user_Logged($ip)){ return true; } else { // verify the user login if (!$this -> verify_User($username, $password)) { return false; } else { // If the user wishes to be remembered, insert him into the database if ($remember == '1') { $query = "UPDATE `$this -> db_table` SET `ip` = '$ip' WHERE `username` = '$username'"; } return true; } } } // END check_Login } /* THE CODE TO INCLUDE IN THE FILE WHERE YOU NEED TO CALL THE LOGIN $login = new login (); // Call the class if ($login->check_Login($username, $password, $ip, $remember)) { // User is logged in } else { // user is not logged in } */ ?> Quote Link to comment Share on other sites More sharing options...
trq Posted June 25, 2007 Share Posted June 25, 2007 Not really related to OOP specifically, but your verify_User() method is uses a very inificient way to check for a valid user. Something like... <?php function verify_User ($username, $password) { $uname = mysql_real_escape_string($username); $upass = md5(mysql_real_escape_string($password)); $query = "SELECT uname,upass FROM `$this -> db_table` WHERE uname = '$uname' && upass = '$upass' LIMIT 1"; if ($result = mysql_query ($query)) { return mysql_num_rows($result); } } ?> Quote Link to comment Share on other sites More sharing options...
Pastulio Posted June 25, 2007 Author Share Posted June 25, 2007 The reason for doing this is because it is the most safe way of Advanced SQL injection that you can use in my opinion, but the function works fine too. But thanks Quote Link to comment Share on other sites More sharing options...
per1os Posted June 26, 2007 Share Posted June 26, 2007 The reason for doing this is because it is the most safe way of Advanced SQL injection that you can use in my opinion, but the function works fine too. But thanks Wow I am with thorpe. Thorpes way will prevent from sql injection, it is probably even overkill with the password part being escaped. The function as it is is very unefficient even if it works now, if you have 10,000 users in the database and you are always retrieveing every user any time someone logs in, wow...memory consupmtion database consumption...yea. I would listen to thorpe. Quote Link to comment Share on other sites More sharing options...
Pastulio Posted June 26, 2007 Author Share Posted June 26, 2007 Ok didn't know that since I don't have 10 000 users, I'll change it now, thanks. Quote Link to comment Share on other sites More sharing options...
-Mike- Posted June 27, 2007 Share Posted June 27, 2007 Is thorpes "LIMIT 1" so that when the first matching result is returned (should also be the only one in the table) it stops, erm, querying the rest of the table right there? ie, if it starts at the beginning in a DB of 1000 users, and the match is number 5 - it only goes through 5 rows before stopping due to the limit, whereas without the limit - it'd find the match on 5 yet continue through the lot? I always assumed it'd stop anyway, so clarification/correction of my viewpoint would be much appreciated thanks! Am also building something similiar, although my code is different (have a db class, have a login class, have a "page" class). It's tough trying to find out whether your method is good/bad Quote Link to comment Share on other sites More sharing options...
per1os Posted June 27, 2007 Share Posted June 27, 2007 If you know only 1 record should be returned always limit it. You do not want to fetch 5 rows when 1 will do, but for that you would need to make sure that usernames are unique etc. Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.