Setting session variable from query result

Question

I've been modifying a user authentication system and I'm having trouble setting a session for the admin. The reguser session is setting just fine, but I can't figure out why admin won't set.

A user with a userlevel of 9 is an admin. Yes, I know how to protect against SQL injection. I'm just trying to keep it as simple and easy to read for now. This probably won't get used for anything, I'm just getting some experience with PHP.

Hi everyone, thanks for your help! I got it to work. I had been staring at it for so long that my mind wasn't clear. Took a break from it yesterday, came back to it today and was able to figure it out in less than 5 minutes! You guys are awesome, I love stackoverflow!

function checklogin($email, $pass) {
        $server = 'localhost';
        $user = 'root';
        $password = '';
        $connection = mysql_connect($server, $user, $password) or die(mysql_error());
        mysql_select_db(udogoo, $connection) or die(mysql_error());
        $pass = md5($pass);
        $result = mysql_query("SELECT userid from users WHERE email = '$email' AND password = '$pass'");
        $user_data = mysql_fetch_array($result);
        $no_rows = mysql_num_rows($result);
        if ($no_rows == 1) 
    {
        $_SESSION['reguser'] = true;
        $_SESSION['userid'] = $user_data['userid'];
        $userid = $user_data['userid'];
        $isadmin = mysql_query("SELECT userlevel FROM users WHERE userid = '$userid'");
        $isadmin2 =  mysql_fetch_array($isadmin);
        $isadmin3 = $isadmin2['userlevel'];
        if ($isadmin3 == "9"){
        $_SESSION['admin'] = true;
        return true;
    }
    }
        else
    {
        return FALSE;
    }
}

Well, to begin with, $result is a resource returned from mysql_query(), so your inclusion of it in the SELECT userlevel ... is not actually searching by the userid as you think it is. It's probably searching for a Resource ID #XX or something. — Jared Farrish
– Jared Farrish, Commented Dec 31, 2011 at 22:51
Instead of using a plain md5() value for password, you might look into crypt(), using the CRYPT_BLOWFISH hash type with a salt value, which makes storing passwords much safer. — Jared Farrish
– Jared Farrish, Commented Dec 31, 2011 at 23:07

Niet the Dark Absol · Accepted Answer · 2011-12-31 22:53:17Z

3

You have a return true; if the user data exists. In fact, you only check or admin-ness if the user doesn't exist.

Remove that return true;, as it's not needed there. If you want, add else return false; after the check for the user's existence, and return true; right at the end.

answered Dec 31, 2011 at 22:53

Niet the Dark Absol

326k86 gold badges480 silver badges604 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Jared Farrish Over a year ago

Yeah, that's another problem. Good catch.

Phoenix · Accepted Answer · 2012-01-01 00:38:48Z

3

Your logic is flawed as well, here:

function checklogin($email, $pass) 
{
    $server = 'localhost';
    $user = 'root';
    $password = '';
    $connection = mysql_connect($server, $user, $password) or die(mysql_error());
    mysql_select_db(test, $connection) or die(mysql_error());

    $email = mysql_real_escape_string($email);
    $pass = md5($pass);

    $sql = "SELECT `userid`,`userlevel` 
            FROM `users` 
            WHERE `email` = '$email' 
            AND `password` = '$pass' 
            LIMIT 1";  //I certainly hope you check email for injection before passing it here.  Also want the LIMIT 1 on there because you are only expecting a single return, and you should only get one since `email` should be unique since you're using it as a credential, and this will stop it from looking through all the rows for another match once it finds the one that matches.

    $result = mysql_query($sql);

    $user_data = mysql_fetch_array($result);
    $numrows = mysql_num_rows($result);

    if ($numrows == 1) 
    {
        $_SESSION['reguser'] = true;
        $_SESSION['userid'] = $user_data['userid'];

        if($user_data['userlevel'] == 9)
        {
            $_SESSION['admin'] = true;
        }
        else
        {
            $_SESSION['admin'] = false;
        }
        return true;
    }
    return false;
}

This should work. No good reason to do two queries when one will do just fine. Returns true if user is logged in, false if user doesn't exist or credentials don't match.

Oops, small syntax error in the SQL statement, corrected. Bigger syntax error also corrected.

And here's how you do the top part in PDO:

function checklogin($email, $pass) 
{
    $server = 'localhost';
    $user = 'root';
    $password = '';
    $dbname = 'test';
    $dsn = 'mysql:dbname=' . $dbname . ';host=' . $server;

    $conn = new PDO($dsn,$user,$password);  //Establish connection   

    $pass = md5($pass);

    $sql = "SELECT `userid`,`userlevel` 
            FROM `users` 
            WHERE `email` = :email 
            AND `password` = :pass 
            LIMIT 1";

    $stmt = $conn->prepare($sql);
    $stmt->bindParam(':email',$email,PDO::PARAM_STR,128)  //First param gives the placeholder from the query, second is the variable to bind into that place holder, third gives data type, fourth is max length
    $stmt->bindParam(':pass',$pass,PDO::PARAM_STR,32)  //MD5s should always have a length of 32

    $stmt->setFetchMode(PDO::FETCH_ASSOC);
    $stmt->execute();  //almost equivalent to mysql_query
    $user_data = $stmt->fetch();  //Grab the data

    if(is_array($user_data) && count($user_data) == 2)  //Check that returned info is an array and that we have both `userid` and `userlevel`
    {
        //Continue onwards

edited Jan 1, 2012 at 0:38

answered Dec 31, 2011 at 23:12

Phoenix

4,5361 gold badge23 silver badges13 bronze badges

9 Comments

Jared Farrish Over a year ago

You could demonstrate how to escape the email address (although the OP does mention escaping in the email). Note, escaping is just a good practice, not necessarily a security practice.

Phoenix Over a year ago

Really, since it's an email, just checking it for valid email format would be enough of an escapement on it. Can't think of any SQL injections you can fit in the format [email protected] or [email protected]. Me, I just make sure my inputs fit what they're supposed to and use PDO with prepared statements and bound parameters and don't worry too much about it anymore.

Jared Farrish Over a year ago

That's some unusual advice. As the example and your answer do not use PDO, escaping is a matter of fact requirement. Validation notwithstanding, inputs in queries should be escaped in most cases.

Jared Farrish Over a year ago

I just realized something: You're missing FROM users in the query.

Phoenix Over a year ago

@Jared Farrish Fixed FROM users. He's not using PDO so I didn't do it in PDO, so I noted it should be escaped. PDO escapes inputs and checks their data type for validity when using prepared statements and bound parameters. If his question was "how do you escape mysql_query query inputs" then I would have gone into how to escape it.

|

Zul · Accepted Answer · 2011-12-31 23:15:19Z

1

$userid = $user_data['user_id'];
$isadmin = mysql_query("SELECT userlevel FROM users WHERE userid = $userid");

$user_data = mysql_fetch_array($result);
$userlevel = $user_data['userlevel'];

if($userlevel == '9')
{
  $_SESSION['admin'] = true;
}

so, your complete code look like this::

<?php
function checklogin($email, $pass) 
{
        $server = 'localhost';
        $user = 'root';
        $password = '';
        $connection = mysql_connect($server, $user, $password) or     die(mysql_error());
        mysql_select_db(test, $connection) or die(mysql_error());
        $pass = md5($pass);
        $result = mysql_query("SELECT userid from users WHERE email = '$email'  AND password = '$pass'");
        $user_data = mysql_fetch_array($result);
        $numrows = mysql_num_rows($result);
        if ($numrows == 1) 
        {
            $_SESSION['reguser'] = true;
            $_SESSION['userid'] = $user_data['userid'];

            //MY ANSWER START HERE
            $userid = $_SESSION['userid']; 
            $isadmin = mysql_query("SELECT userlevel FROM users WHERE userid = $userid");

            $user_data = mysql_fetch_array($result);
            $userlevel = $user_data['userlevel'];

            if($userlevel == '9')
            {
              $_SESSION['admin'] = true;
            }
            //END HERE 

        }
        else
        {
            return false;
        }
}

?>

edited Dec 31, 2011 at 23:15

answered Dec 31, 2011 at 22:38

Zul

3,6063 gold badges24 silver badges36 bronze badges

10 Comments

user1104854 Over a year ago

I tried something similar to this before and it didn't work. The strange part is it's nearly identical to how the "reguser" session is set.

Jared Farrish Over a year ago

It's possible the user_id is an alphanumeric field, so it might need to be '$userid'. But this does appear to deal with the obvious mistake of using $result to search on.

Jared Farrish Over a year ago

Just an aside, but if $_SESSION['userid'] is set correctly, you could use it instead of a function-scoped variable...

Jared Farrish Over a year ago

Note @Kolink's answer, though, which discusses the fact that the function short-circuits before getting to your for valid users. That return TRUE needs to come out too.

user1104854 Over a year ago

@JaredFarrish what do you mean by instead of a "function-scoped variable". Sorry, I never had any formal programming lessons so I don't know a lot of the terminology. I probably know what you mean, just not the definition of it.

|

Collectives™ on Stack Overflow

Setting session variable from query result

3 Answers 3

1 Comment

9 Comments

10 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

9 Comments

10 Comments

Your Answer

Sign up or log in

Post as a guest

Related