Page 1 of 2

Proper way to do a user login? Cookies?

Posted: Mon Nov 29, 2010 5:31 pm
by Stacks
When it comes to users logging in what is the purpose of setting cookies?

I accomplish my login by using the $_SESSION object.

My login class looks like this. What am I doing wrong, and what can I improve upon?

Code: Select all

<?php

class LoginManager
{
	private $errorMessage = null;

 	public function isLoggedIn()
	{
		$valid = false;
		if(isset($_SESSION['loginId']))
			$valid = true;
		return $valid;
	}
	public function logout()
	{
		session_destroy();
	}	
	public function login($email, $password)
	{
		$valid = false;
		if(!isset($_SESSION['loginId']))
		{
			$loginId = $this->authenticate($email, $password);
			if ($loginId > 0)
			//Authentication succesfull
			{
				$_SESSION['loginId'] = $loginId;
				$valid = true;
			}
		}
		else
			$this->errorMessage = "User is already logged in, please logout";

		return $valid;
	}
	private function authenticate($email, $password)
	{
		$loginId = 0;
		$login = new Login();		
		$loginResult = $login->selectLoginDetailsByEmail($email);
		unset($login);

		if($loginResult->rowCount() == 1)
		{
			$loginRow = $loginResult->fetch();
			if (strcmp($loginRow['password'], sha1($password . SALT)) == 0)
			{
				$loginId = $loginRow['loginId'];
			}
			else
				$this->errorMessage = "The password entered is incorrect.  If you need help recovering your password click <a href=\"forgotPassword.php\">here</a>";
		}
		else
			$this->errorMessage = "The email entered is not in our system";
		return $loginId;
	}
	public function errorMessage()
	{
		return $this->errorMessage;
	}
}

?>

Re: Proper way to do a user login? Cookies?

Posted: Mon Nov 29, 2010 7:38 pm
by curlybracket
1. Authentication using sessions is OK. You may need cookies for other purposes, let's say you want to add "remember me" option to your login system - you will have to use cookies.

I don't think you do anything wrong, however you could improve few elements in your code:

2. You created method isLoggedIn() to check if user is authenticated. But in 2. line of login() method you are duplicating the code instead of using $this->isLoggedIn() method. This will force you to change your code twice (at least) if you ever change name of session variable.

3. Your code could be shorter without few not necessarily variables, for example instead of:

Code: Select all

public function isLoggedIn()
        {
                $valid = false;
                if(isset($_SESSION['loginId']))
                        $valid = true;
                return $valid;
        }
you can write:

Code: Select all

public function isLoggedIn()
  {
    if(isset($_SESSION['loginId'])) { return true; } return false;
  }
4. For me, login() method is doing almost nothing, you could throw this code to authenticate() method.
5. I'm not sure about your solution with pulling one record from database by email. It is faster and safer to pull record by email AND password than to pull it by email and than checking its password. You need few more lines of code to check everything and I don't like that.
6. I'm not sure if it is right to inform users which is wrong: password or login. You are giving out informations about your users, I can check in that way if somebody is registered in your service. Look around, you will rarely see that kind of messages.

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 1:38 am
by social_experiment

Code: Select all

<?php $valid = false; ?>
It's always good to predefine variables. You could add another check along with the one for $_SESSION['loginId']), maybe a generated session id (a custom one, not PHPSESSID) or check for a database value that is set on login.

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 3:56 am
by curlybracket
It is good, but in this case I don't think he need any variable.

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 5:02 am
by social_experiment
Just out of curiosity, why not?

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 7:06 am
by curlybracket
You have a code:

Code: Select all

$valid = false;
if(isset($_SESSION['loginId']))
{
     $valid = true;
}
return $valid;
I don't see any reason for $valid variable and I can proove it by writing much shorter code that do exactly the same thing:

Code: Select all

if(isset($_SESSION['loginId']))
{
     return true;
}
return false;

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 7:29 am
by social_experiment
Although register_globals have been deprecated (and removed) from PHP, it's still a good idea (and good coding practise) to predefine variables. One less line of code won't make a noticeable change performance. IMO, rather safe than sorry, your code is only as secure as its weakest link.

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 7:40 am
by curlybracket
I'm not sure if we understand each other. I agree, that it is good idea to predefine variables - otherwise you will get PHP notice. But in case of this specific code he don't need variable $valid. In other words, I am not suggesting to do this:

Code: Select all

if(isset($_SESSION['loginId']))
{
     $valid = true;
}
return $valid;
but to do this:

Code: Select all

if(isset($_SESSION['loginId']))
{
     return  true;
}
return false;

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 8:37 am
by social_experiment
Yes there is probably a misunderstanding :) Personaly I would code the example as follows:

Code: Select all

<?php
 // predefine variable
 $valid = false;

 if (isset($_SESSION['loginId'])) { 
  $valid = true;
  return $valid;
 }
 else {
  $valid = false;
  return $valid;
 }
?>

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 9:14 am
by curlybracket
That's your choice. I don't want to say that your code is wrong, but it is longer than mine:

Code: Select all

if (isset($_SESSION['loginId'])) { return true; } return false;
If you write

Code: Select all

$valid = true;
return $valid;
why not to write

Code: Select all

$valid = true;
$toReturn = $valid;
return $toReturn;
This is joke of course, I don't want to be mean, but there if there is no reason to do something - why to waste time for writing 100 letters instead of writing 10...

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 11:20 am
by social_experiment
The question is not one of size, it's one of coding practice. From the php manual (sic) ...although it really is generally a good programming practice to initialize variables first

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 12:34 pm
by Stacks
curlybracket wrote:1. Authentication using sessions is OK. You may need cookies for other purposes, let's say you want to add "remember me" option to your login system - you will have to use cookies.
So that is really the only reason to use cookies? Other than that there is no need for me to add them?
curlybracket wrote:I don't think you do anything wrong, however you could improve few elements in your code:
2. You created method isLoggedIn() to check if user is authenticated. But in 2. line of login() method you are duplicating the code instead of using $this->isLoggedIn() method. This will force you to change your code twice (at least) if you ever change name of session variable.
Haha lol I don't know how this one slipped past me. I must have wrote my login function before I wrote isLoggedIn()
curlybracket wrote:3. Your code could be shorter without few not necessarily variables, for example instead of:

Code: Select all

public function isLoggedIn()
        {
                $valid = false;
                if(isset($_SESSION['loginId']))
                        $valid = true;
                return $valid;
        }
you can write:

Code: Select all

public function isLoggedIn()
  {
    if(isset($_SESSION['loginId'])) { return true; } return false;
  }
I see what you are saying. For some reason when I learned to code I had been told it was bad practice to have to separate returns in the same function.
curlybracket wrote:4. For me, login() method is doing almost nothing, you could throw this code to authenticate() method.

5. I'm not sure about your solution with pulling one record from database by email. It is faster and safer to pull record by email AND password than to pull it by email and than checking its password. You need few more lines of code to check everything and I don't like that.
Could you explain to me what makes it safer and faster?
curlybracket wrote:6. I'm not sure if it is right to inform users which is wrong: password or login. You are giving out informations about your users, I can check in that way if somebody is registered in your service. Look around, you will rarely see that kind of messages.
For some reason when I log into an old site and i can't remember which user name i used or which password I always have found it nice when they inform you which one is wrong. So that is what I was going for. But from a security point. I like what you say. It makes a lot of sense. It would be much safer to give them a generic message informing them the login credentials are incorrect.

Thanks for all the points. Some things for me to consider. I appreciate your feedback.

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 12:36 pm
by Stacks
social_experiment wrote:

Code: Select all

<?php $valid = false; ?>
It's always good to predefine variables. You could add another check along with the one for $_SESSION['loginId']), maybe a generated session id (a custom one, not PHPSESSID) or check for a database value that is set on login.
Could you give me an example? I've never seen a "custom" session Id.

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 12:53 pm
by curlybracket
So that is really the only reason to use cookies? Other than that there is no need for me to add them?
No, this is not the only reason for them, but in context of your post - I think yes. Here you can read about some other ideas:
http://en.wikipedia.org/wiki/HTTP_cookie#Uses
I had been told it was bad practice to have to separate returns in the same function.
I can't see any drawbacks of that, maybe you or somebody can write something more about that?
Could you explain to me what makes it safer and faster?
It is faster because SQL is usually much faster that PHP. If you have to filter data from SQL database it is much better if you can do this using SQL than to process redundant data with PHP. And safer: I can't find instant way to hack your code now, but I can imagine situation, where for example something is suddenly wrong with your $loginResult->fetch() method or with strcmp() function for some reason and it spit out details of this record from database into the browser. Maybe it is not possible in this case, but it is a good practice to keep sensitive data as far from the users as you can.

Re: Proper way to do a user login? Cookies?

Posted: Tue Nov 30, 2010 3:38 pm
by Stacks
So that is really the only reason to use cookies? Other than that there is no need for me to add them?
curlybracket wrote:No, this is not the only reason for them, but in context of your post - I think yes. Here you can read about some other ideas:
http://en.wikipedia.org/wiki/HTTP_cookie#Uses
I had been told it was bad practice to have to separate returns in the same function.
curlybracket wrote:I can't see any drawbacks of that, maybe you or somebody can write something more about that?
It was only what I was taught. It's just someone's opinion. It might be a tiny bit easier to look at, but that is even up to debate. Apples vs. Oranges.
Could you explain to me what makes it safer and faster?
curlybracket wrote:It is faster because SQL is usually much faster that PHP. If you have to filter data from SQL database it is much better if you can do this using SQL than to process redundant data with PHP. And safer: I can't find instant way to hack your code now, but I can imagine situation, where for example something is suddenly wrong with your $loginResult->fetch() method or with strcmp() function for some reason and it spit out details of this record from database into the browser. Maybe it is not possible in this case, but it is a good practice to keep sensitive data as far from the users as you can.
The comparison between speed of SQL and PHP makes sense. So what you are saying is it would be better to select with the email and password login details. If it returns the one record, then authenticate. If not then the login is incorrect. This way I don't have to fetch any data and there would be less possibilities of data being spit out in an error in the chance there is a security attack. Thank you for your insight.