wjohn Posted January 10, 2011 Report Share Posted January 10, 2011 Hi I'm working on a login method to my class and so far it look like this function login() { //Database injection fix $_POST = db_escape($_POST); $username = $_POST['username']; $password = sha1($_POST['password'] . $this->salt); $query = "SELECT id, username, userlevel FROM users " . "WHERE username = '" . $username . "' AND password = '" . $password . "')"; $result = mysql_query( $query ); $user_data = mysql_fetch_assoc( $result ); if( mysql_num_rows( $result ) == 1 ) { } The login method is triggered from my constructor function Auth() { if (isset($_POST['username']) && isset($_POST['password'])) { $this->login(); } else if (isset($_GET['logout'])) { $this->logout(); } } Now I wonder how I should return the data, Let's say Im on the login.php and the users doesn't exists or is banned, Something like this: function login() { //Database injection fix $_POST = db_escape($_POST); $username = $_POST['username']; $password = sha1($_POST['password'] . $this->salt); $query = "SELECT id, username, userlevel FROM users " . "WHERE username = '" . $username . "' AND password = '" . $password . "')"; $result = mysql_query( $query ); $user_data = mysql_fetch_assoc( $result ); if( mysql_num_rows( $result ) == 0 ) { //Oh noes the user doesn't exists I want to print this out to the user, What should I return ? and how I print it out? } Thanks in advance. Quote Link to comment Share on other sites More sharing options...
falkencreative Posted January 10, 2011 Report Share Posted January 10, 2011 I would return false. if login() returns false, you know there is an issue with the username or password, and can set a variable that will display out to the user when you display the form. Quote Link to comment Share on other sites More sharing options...
wjohn Posted January 10, 2011 Author Report Share Posted January 10, 2011 Well, What about there was 2 options, you can get logged in, but what if you're banned, that is a second check, before Im going to set the sessions. As I clearly want to state if they login was fail because of username and password or banned. Quote Link to comment Share on other sites More sharing options...
falkencreative Posted January 10, 2011 Report Share Posted January 10, 2011 Or you could return false either way, and use a session variable store the error message. Or return false and pass a message variable back by reference (http://php.net/manual/en/language.references.pass.php). A very rough example: <?php function login(&$msg) { // if invalid: $msg = 'Invalid username/password!'; return false; // if banned $msg = 'Sorry, you\'ve been banned!'; return false; } $msg = ''; login($msg); if (login($msg) == FALSE) { echo $msg; } ?> Quote Link to comment Share on other sites More sharing options...
wjohn Posted January 10, 2011 Author Report Share Posted January 10, 2011 What would you recommend? Is my method of login class even good? I want to optimise as much as I can, and by the way thanks a lot for the help! It's very appreciated. Quote Link to comment Share on other sites More sharing options...
falkencreative Posted January 10, 2011 Report Share Posted January 10, 2011 What would you recommend? Functionality-wise, I think either would work. I used the $_SESSION[] method in a recent tutorial I did on OOP PHP login (http://www.killersites.com/community/index.php?/blog/5/entry-20-new-php-oop-videos/ - if you are a subscriber to the KillerSites Video Library it might be worth a look). I'm not sure if either is "better" but maybe other members will have opinions. The only thing I question is this: if (isset($_POST['username']) && isset($_POST['password'])) { $this->login(); } else if (isset($_GET['logout'])) { $this->logout(); } Based on how I personally code, I would think that this sort of logic really belongs in the controller, not in the Auth object. I would probably have my controller handle checking for post data and assuming the input itself is valid, then pass it off to the $Auth->login() function (or the controller checks for $_GET['logout'] and then calls $Auth->logout().) Working with OOP is something I am continually growing in, so I say that as my personal opinion, not necessarily a best practice. Quote Link to comment Share on other sites More sharing options...
wjohn Posted January 10, 2011 Author Report Share Posted January 10, 2011 I don't really use MVC model, but I guess I could put away the CONSTRUCTOR and put it in just a plain login file, and then call if the data is valid. <?php if (isset($_POST['submit'])) { //validate data then call login() } ?> Quote Link to comment Share on other sites More sharing options...
wjohn Posted January 10, 2011 Author Report Share Posted January 10, 2011 function login(&$msg) { //Database injection fix $_POST = $this->db_escape($_POST); $username = $_POST['username']; $password = sha1($_POST['password']); $query = "SELECT id, username, userlevel FROM users " . "WHERE username = '" . $username . "' AND password = '" . $password . "'"; $result = mysql_query($query); if(mysql_num_rows($result) == 0) { $msg = 'Fel användarnamn eller lösenord!'; return false; } $user_data = mysql_fetch_assoc( $result ); if ($this->IsUserBanned($user_data["id"])) { $query = "SELECT * FROM bans WHERE uid = '" . $user_data['id'] . "'"; $result = mysql_query($query); $ban_data = mysql_fetch_assoc( $result ); $msg = '<strong>AVSTÄNGD</strong><br /> ANLEDNING: ' . $ban_data["reason"] . '<br /> BEVIS: ' . $ban_data["evidence"] . '<br /> TID: ' . $ban_data["time"] . '<br /> AV: ' . $ban_data["administrator"]; return false; } return true; } I came up with this, anything I can improve, security such. This is the db_escape function: function db_escape ($post) { if (is_string($post)) { if (get_magic_quotes_gpc()) { $post = stripslashes($post); } return mysql_real_escape_string($post); } foreach ($post as $key => $val) { $post[$key] = $this->db_escape($val); } return $post; } Quote Link to comment Share on other sites More sharing options...
falkencreative Posted January 11, 2011 Report Share Posted January 11, 2011 I'm not really a PHP security expert (still something I am learning about myself) but... You might consider looking at MySQLi or PDO - they are used instead of the standard MySQL code to connect and query a database. They both support prepared statements, which prevent SQL injection. I'm not sure what your skill level with PHP is, but it might be worth a look. I usually use MySQLi instead of dealing with mysql_real_escape_string(). In addition to MySQLi, I also use htmlentities() to properly escape data that could be interpreted as html. Quote Link to comment Share on other sites More sharing options...
wjohn Posted January 11, 2011 Author Report Share Posted January 11, 2011 The only thing, I consider right now is to use htmlentities() on the outputs at the ban data, as if some "admin" would freak out and try to XSS inject. Quote Link to comment Share on other sites More sharing options...
kraxzy Posted January 17, 2011 Report Share Posted January 17, 2011 I'd let the webserver handle the uphold of banned users, and spare the server of having to do extra database lookups. 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.
Note: Your post will require moderator approval before it will be visible.