Jump to content

A login method


wjohn

Recommended Posts

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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; }

?>

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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;
}

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Loading...
×
×
  • Create New...