Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I posted this same question on StackOverflow but guys over there told me to post it here.

I have programming in PHP since past 2 years and yet haven't found a perfect script for logging the user in. It's very messy when it comes to deciding whether to use Session or Cookies or both. What value to assign to the Session ID? Whether to store that value in MySQL table or not. How to auto-login the user? And so on...

I've been using this code for few months. Please tell me whether this is fine.

This is the main signin function.

<?php
    //this function is called after sanitizing the user-input
    //$u is username and $p is password
function signin($u,$p) {
    global $conn, $current_user, $signin_stat, $incorrect;
    $q_string="SELECT uid FROM users WHERE (emailid='$u' OR uname='$u') AND AES_DECRYPT(pass,CONCAT(uname,'$p'))='$p'";
    $q=$conn->query($q_string);
    if($q->num_rows==1) {
        $c=$q->fetch_row();
        $uid=$c[0]; //getting uid
        $current_user=new User($uid); //creating $current_user object

        set_session($uid); //setting session

        $signin_stat=1;

        return true;
    } else {
        signout(0);
        $incorrect=1;
        return false;
    }
}
?>

This is the set_session() function :

<?php
function set_session($uid) {
    global $conn;
    $sid=sha1($uid+time()+$_SERVER['REMOTE_ADDR']);
    $q_string="UPDATE users SET sid='$sid' WHERE uid=$uid";
    $q=$conn->query($q_string);
    $_SESSION['user_session']=$sid;
    if($q) return true; else return false;
}
?>

This is the auto_signin script :

<?php
if(isset($_SESSION['user_session'])) {
    $sid=$_SESSION['user_session'];
    global $conn, $current_user;
    $q_string="SELECT uid FROM users WHERE sid='$sid'";
    $q=$conn->query($q_string);
    if($q->num_rows==1) {
        $c=$q->fetch_row();
        $uid=$c[0];
        $current_user=new User($uid);
        $signin_stat=1;
        $current_user->update_online_stat();
    } else {
        signout(0);
    }
}
?>

This is the signout() function

<?php
function signout($uid) {
    global $conn, $current_user, $signin_stat;
    $sid='NULL';
    $q_string="UPDATE users SET sid='$sid' WHERE uid=$uid";
    $q=$conn->query($q_string);

    $signin_stat=0;
    $current_user=0;

    $_SESSION['user_session']='';
    unset($_SESSION['user_session']);
    unset($_POST);
    session_destroy();
}
?>

notes : $conn is the mysqli object

$current_user is the user object

sid is the field in mysql users table which holds the current session id of the respective user.

Is this script fine or would you suggest some changes?

Or is this completely crap and needs to be written again from scratch?

Please help me out.

share|improve this question
    
Try to avoid using globals everywhere. –  tsabz Dec 21 '12 at 13:57
    
@tsabz Why should I avoid using globals? How do I access the $conn variable which is the MySQLi object. Should I declare it again and again in every function? And other objects which are frequently accessed shouldn't also be declared global? –  ShuklaSannidhya Dec 21 '12 at 15:46
    
Gloabls are a security concern. Having your database connection as a global allows any application, malicious or otherwise, to gain access to your database. They are also considered bad because of how difficult it sometimes is to trace while debugging. In a large script a global can appear to be a simple variable and sometimes its not always obvious where it came from. See my answer for alternatives. –  mseancole Dec 21 '12 at 15:53
    
Your signin() function contains an SQL injection vulnerability. You're also storing passwords, which is a huge no no. You can read about working with databases and password hashing here: phptherightway.com These are much more important problems than whether to store session data in an encrypted cookie, or in database (both are fine). –  Alexey Lebedev Dec 23 '12 at 13:07
add comment

1 Answer

I'm no security expert, not even close, but I can tell you what I use and have observed. I only use cookies to remember the username. Cookies aren't secure so you want to limit what you save in them. I also use sessions to create persistence by passing the user object on. Is this the right way? I don't know, but it has served me well and does not contradict anything I have read on the subject. As for auto-login, you can do something similar to what SO does. You can create a client side cookie with a unique ID and pair it to an account. If the login computer has that cookie then you can have it automatically login. An added security feature might be to also match IP addresses. Not sure if this is exactly what SO does, but its similar. Again no security expert, so I would gather more opinions and do your research.

If you are going to provide comments about a function then you should use doccomments as they are more useful in that IDEs can recognize them and provide them as auto-complete hints. Additionally, I would not use single letter variables except for incrementals or throwaways. The abstraction can lead to confusion if you are not careful and it makes it more difficult to tell what is going on at a glance.

/** this function is called after sanitizing the user-input
 * 
 * @param string $user is username
 * @param string $pass is password
 */

As tsabz pointed out, you should never use globals. They are evil and have no place in this world. Help banish these foul demons by forgetting you ever heard of them. Especially don't use globals for something as important as a database connection. It is insecure. If you must use a variable in a function, then inject that variable into the function via the argument list. In this case that would add a very large list, so you might instead want to compact those values into an array and inject the array instead. However, I'll point out in a few that some of these variables are unnecessary, which will decrease the size of said list.

If you are returning from an if statement, then there is no reason to have an else statement, the else is implied. I would remove it to reduce the amount of indentation, however that is really a preference point and entirely up to you.

if( $q->num_rows == 1 ) {
    //etc...
    return TRUE;
}

//etc...
return FALSE;

There are a couple of different things you can do instead of using $current_user as a global. The easiest way would be to convert it to a session variable so that it can be shared. Another, more practical way would be to return the user object instead of a boolean and then later persist it if necessary. This later method makes sense for a number of reasons. First, it goes along with what the function says it is doing. By signing in I expect either a user object or a failure, not a TRUE/FALSE. Second, it allows you to do away with $signin_stat and $incorrect which were both redundant. You could accomplish the same thing with the boolean return (to illustrate: if( signin() ) { $signin_stat = 1; }). Third, it allows you to change how you persist the user object so you are not reliant on sessions.

$_SESSION[ 'current_user' ] = new User( $uid );
//or
if( $q->num_rows == 1 ) {
    //etc...
    return new User( $uid );
}

//etc...
return FALSE;

Always, always, always use braces. I know it is a feature and you can get away with it, but it can be confusing and can cause issues, especially if you combine two statements on one line like that. Not that an if/else statement is even necessary here. If you want to return a boolean value based on the return of an expression, just return the expression. Here it is a little different because there is no comparison to make this a true expression. Instead you are relying on PHP to automatically convert it to a boolean for the comparison. So you might want to typecast it in the return to get a true boolean.

//braces
if( $q ) {
    return TRUE;
} else {
    return FALSE;
}
//equivalent to above
return ( bool ) $q;

Some of this code is violating the "Don't Repeat Yourself" (DRY) Principle. As the name implies, your code should not repeat itself. If you look at your login function and compare it to your auto-login function you should be able to see the similarities. You could create a new function that accomplishes the shared task, but because you are using regular functions this would make bypassing the normal signin function a security concern. I don't really see a way around this, except to convert to using a class, which is something you might want to consider anyways. Many of the issues I pointed out above would be solved much easier in a class. For instance, you could convert those globals to class properties to the same effect. Then there is also the encapsulation that will allow you to hide properties and methods, such as the $conn property and shared login method I was mentioning. I'm not trying to pressure you into using a class, but I am suggesting you give it a look. Hope this helps.

share|improve this answer
    
all right, i have shifted to class but is making mysqli connection object public also a security issue? –  ShuklaSannidhya Jan 14 '13 at 1:34
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.