I have a question for all you gurus out there?

The other day I posted something on another forum and a person stated that the following was incorrect.

[php]function login_user(array $data, PDO $pdo) {

/*
 * Setup the Query for reading in login data from database table
 */
$query = 'SELECT id, username, password, email FROM users WHERE username=:username';

try {
    $stmt = $pdo->prepare($query); // Prepare the query:
    $stmt->execute([':username' => $data['username']]); // Execute the query with the supplied user's parameter(s):
} catch (Exception $ex) {
    die("Failed to run query: " . $ex->getMessage()); // Do Not Use in Production Website - Log error or email error to admin:
}

$stmt->setFetchMode(PDO::FETCH_OBJ); // Fetch the user's information as object(s):
$user = $stmt->fetch();

/*
 * If username is in database table users then proceed to check password entered against stored password.
 */
if ($user) {
    $loginStatus = password_verify($data['password'], $user->password); // Check the user's entry to the stored password:
    unset($data['password'], $user->password); // Password(s) not needed then unset the password(s)!:
} else {
    return FALSE;
}

/*
 * If passwords word match then store user's information in sessions and return true from function.
 */
if ($loginStatus) {
    $_SESSION['user'] = $user; // Set the session variable of user:
    return TRUE;
} else {
    return FALSE;
}

}[/php]

Specifically line 16, say that it might cause problems with other database formats (other than MySQL). The poster wasn’t too friendly and I thought a little full of himself. Needless to say I probably stick to PHPHelp to post those kind of threads from now on. Anyways, I went to my php books where I learned majority of my PHP and that is basically what they had. However, I did tighten up the code to the following:
[php]function login_user(array $data, PDO $pdo) {

/* 
 * Setup the Query for reading in login data from database table 
 */
$query = "SELECT id, username, password, email, DATE_FORMAT(birthday, '%M %e, %Y') as birthday FROM users WHERE username=:username";

try {
    $stmt = $pdo->prepare($query); // Prepare the query:
    $stmt->execute([':username' => $data['username']]); // Execute the query with the supplied user's parameter(s):
} catch (Exception $ex) {
    die("Failed to run query: " . $ex->getMessage()); // Do Not Use in Production Website - Log error or email error to admin:
}

$results = $stmt->fetch(PDO::FETCH_OBJ);
if (count($results) > 0 && password_verify($data['password'], $results->password)) {
    unset($data['password'], $results->password);
    $_SESSION['user'] =$results; 
    return (boolean) TRUE;
}

}[/php]

Anyways I think he’s wrong or incorrect, but I just want to be sure from someone else who might have experience in other databases other than MySQL.

They said this line,
[php]$user = $stmt->fetch();[/php]
? ? ?

The joy of PDO is that it is a wrapper for any driver installed. So, on that side he is incorrect. BUT, the syntax of some sql statements can get you in trouble. Like this,

[php]DATE_FORMAT(birthday, ‘%M %e, %Y’)[/php]
That is a MySQL/MariaDB function and not usable in SQL Server or Postgres.

Some suggestions on your “tightened” code:

  1. Even though you have the comments to not use the $ex->getMessage() in production, Joe newbie is gonna copy/paste your code and use it anyway. Get rid of all your Try/Catch blocks (almost all of them) and use set_exception_handler(). http://php.net/manual/en/function.set-exception-handler.php . One case where you would keep the try/catch is when catching a duplicate username so you can display a custom message to the user. That assumes you are attempting to insert users the correct way instead of the widely (and wrongly) used check if user exists/enter if not pattern.

This part if (count($results) > 0 could be shortened up to if ($results)

You also shouldn’t be storing the password, hashed or not, in the session.

Yeah, I was gonna say something about that too. I dont see you putting password in a session which is even more reason there is no need to unset it. It cannot persist just because you selected it.

While not a big deal, but since you are only getting one row, it would be more clear in reading to use $row instead of $results. Results infers multiple rows. Hard pressed to apply it to multiple columns which generally would be expected on a select.

$row = $stmt->fetch(PDO::FETCH_OBJ);

Learn something new everyday, but this one really confuses me to no end (I would write something else, but I want to keep this rated G. ;D)

For example:
[php]function login_user(array $data, PDO $pdo) {

/* 
 * Setup the Query for reading in login data from database table 
 */
$query = "SELECT id, username, password, email, DATE_FORMAT(birthday, '%M %e, %Y') as birthday FROM users WHERE username=:username";

try {
    $stmt = $pdo->prepare($query); // Prepare the query:
    $stmt->execute([':username' => $data['username']]); // Execute the query with the supplied user's parameter(s):
} catch (Exception $ex) {
    die("Failed to run query: " . $ex->getMessage()); // Do Not Use in Production Website - Log error or email error to admin:
}

$row = $stmt->fetch(PDO::FETCH_OBJ);
if ($row && password_verify($data['password'], $row->password)) {
    unset($data['password'], $row->password); 
    $_SESSION['user'] =$row; 
    return (boolean) TRUE;
}

}[/php]

I understand get rid of the try-catch aspect of it, but not sure how to implement the set_exception_handler() part of it (even after going to php.net). Though it’s late at night and I’ll look at it better in the morning. Maybe a light bulb will turn on ;D
As for the password being stored in sessions that what I thought I was doing when I unset it before I stored the row in sessions? By the way this isn’t something I came up with, I got this a long time ago from a PHP book by Larry Ullman. Though I like him, I have notice on a rare occasion in the past that he is capable of making a mistake and leading a person a little astray . :wink:

By the way I know it was the line $user = $stmt->fetch() that the guy was referring to, for I didn’t have DATE_FORMAT(birthday, ‘%M %e, %Y’) in at the time (plus I even knew that about DATE_FORMAT) when he made that comment. So I now know he was full of it. 8)

Thanks for the help and maybe I getting too old for programming. (I think this is just the tiredness coming out)

With my fast typing, I probably meant this (PDOException)
[php]try {

} catch (PDOException $e) {

}[/php]

OK, that’s it until morning. :::yawn:::

I wish I could sleep >:(, OK I think I’m starting to get this in my head.

So I basically should write my PDO files this way (unless I want to do something else if the query fails like Kevin Rubio already stated):
[php] function login_user(array $data, PDO $pdo) {

  /* 
   * Setup the Query for reading in login data from database table 
   */
  $query = "SELECT id, username, password, email, DATE_FORMAT(birthday, '%M %e, %Y') as birthday FROM users WHERE username=:username";

  $stmt = $pdo->prepare($query); // Prepare the query:
  $stmt->execute([':username' => $data['username']]); // Execute the query with the supplied user's parameter(s):
 $row = $stmt->fetch(PDO::FETCH_OBJ);
 if ($row && password_verify($data['password'], $row->password)) {
     unset($data['password'], $row->password); 
     $_SESSION['user'] =$row; 
     return (boolean) TRUE;
 }

}[/php]

Then I would want (or should) to write a simple exception handler that logs or notifies me of database errors that occur in the production environment and displays a friendly error message to my users instead of the exception trace. That is when set_exception_handler() should be used and that something I’m slowly wrapping my head around with. Thanks again for the help and maybe I’m not too old after all, either that I’m just putting rose-colored glasses on. 8)

Missed that line.

If I don't unset it the $row will put the password in session as an object

That’s because you are doing it wrong.

THIS:
[php]$_SESSION[‘user’] =$row;[/php]

SHOULD BE:
[php]$_SESSION[‘user’] = $row[‘username’];[/php]

  • EDIT: Not sure why you would want the username in a session anyways. What you should be doing is putting the user_id in a session and using that for whatever. The only other data you might want in a session from the login is first and last name for display purposes. If you are using roles you would also want the role(s) ID as well.

Sorry, I meant to hit quote, but I accidently hit modify. oops ::slight_smile:

Though it got me thinking in what you’re saying and I do believe you’re right that I have been going at this all wrong. Instead of unsetting I should be just assigning the variables to session (in this case the user’s id) that way there’s no possibility of some sensitive info getting into sessions by accident. Then I could just access the id to create my own class or records (objects). Thanks again.

Just one comment at the moment about $user = isset($_SESSION[‘user’]) ? $_SESSION[‘user’] : NULL;

Having that just hanging around in a utility’s file presents a definite readability problem.

Say we are in random_page.php that includes the utility file and I assume several other files potentially and you do like echo “Hello $username”;. All of a sudden you have this “Magic” variable that appears out of no where from who knows where. You are also creating a completely unnecessary variable. There has not been any transformation on the user name at all so it is redundant. And if for some reason the project is not setting a username to session, then you just have this lonely code hanging out in the utility closet wishing it had a purpose in life. And then Joe OtherKoder reads the utility file and scratches his hair out trying to figure out why that code is there.

If you did like echo “Hello {$_SESSION[‘username’]}”; it is self documenting and is obviously clear where that data is coming from.

True, but I have to disagree with you a little on this one. For the only time you’ll use $_SESSION[‘username’] is when the user logs in. An if it’s $_SESSION[‘username’] then it’s going to be [php]$username = isset($_SESSION[‘username’]) ? $_SESSION[‘username’] : NULL; [/php] and the only way it’s going to be “sitting” around is you have no purpose of logging in the user. It’s always going to be null in that case. It’s not something that I came up with for I seen in it a tutorial (not from the internet but from a book). It’s basically a lazy programmer’s way of writing :

[php]if (isset($_SESSION[‘user’]) && $_SESSION[‘user’]->security_level === “member”) {
// redirect user:
}[/php]

when you can just write this
[php]if ($user && $user->security_level === “member”) {
// redirect user:
}[/php]

Don’t get me wrong I do see your point about readability , but I have used it enough to see how it can’t be orphaned (Though on the internet I’m finding anything is possible. ;D).

Going back to what I posted earlier, it should be the user_id that you are setting to session, not the username.

Lets go to some stripped down login code from our login.php. Please Sir, tell me where, why, or how, I would want or need the $username isset line in my utility’s file based on the following code.

[php]<?php
if (!empty($_POST))
{
$sql = “SELECT
user_id,
username,
password,
name_first,
name_last
FROM users
WHERE username = ?”;

    $stmt = $pdo->prepare($sql);
    $stmt->execute(array(
        $_POST['username']
    ));
    $row = $stmt->fetch(PDO::FETCH_ASSOC);

    //------------------------------------------------------------------------
    // Username didn't match, Redirect.
    //------------------------------------------------------------------------

    if (!$row)
        {
        die(header("Location: {$_SERVER['SCRIPT_NAME']}?fail"));
        }

    //------------------------------------------------------------------------
    // Compare the password to the expected hash.
    //------------------------------------------------------------------------

    if (password_verify($_POST['password'], $row['password']))
        {
        //------------------------------------------------------------
        // Password is good . Set Session Variables
        //------------------------------------------------------------

        session_regenerate_id(true);

        $_SESSION['login'] = true;
        $_SESSION['user_id']   = $row['user_id'];
        $_SESSION['name_first'] = $row['name_first'];
        $_SESSION['name_last']  = $row['name_last'];
        die(header("Location: ./index.php"));
        }
    else
        {
        die(header("Location: {$_SERVER['SCRIPT_NAME']}?fail"));
        }
} // End (!empty($_POST))

?>[/php]

After successful login:
index.php

[php]Welcome <?= $_SESSION['name_first'] ?> <?= $_SESSION['name_last'] ?>. You are now logged in! [/php]

Hmm, learn something new everyday, I never knew about session_regenerate_id and checking php.net it’s been around since PHP 4. Though is this an example of a good script or a bad script, for I’m a little confused with the comment “it should be the user_id that you are setting to session, not the username.”? Yet, first_name and last_name is set in sessions?

This is an example of a good script (some stuff like error checking was stripped out for this discussion).

First and last name is not a login user name and is only set to session for display purposes as in a “Logged in as Firstname Lastname” display and has noting to do with the user Id. As I said previously, their could be other data you would need to set to session as well such as the user roles id’s.

What I said about user_id is the same. Once you set user_id to session and you need some data related to that user you would just do SELECT columns from TABLE where user_id={$_SESSION[‘user_id’]}

Please tell me your not using the actual username as a key in other tables.

I will be switching over to user_id (it should be a quick fix, I hope). So basically you don’t want any sensitive data having a chance in getting into sessions. An to answer you question the basic answer is no I was not using username as the key for other tables (I’m 99 percent certain for even doing the old way I was using the id to grab the data). It never occurred to me about the username. However, I’m 99.9 percent sure now that switching over to this way of doing it, I will be finally on the same page and hopefully ending my confusion. I swear a while back it was about where to sanitize the user’s input and thanks to you guys I solved that. I hope this will resolve this and if I have any questions after I implement this I will ask questions, but I think I understand it now. Thanks once again, John

One reason I advocate only adding the user id to session and then fetching the data you need on each request is that the data might change. If you have moderators, admins, subscriptions or cron scripts running that can modify user accounts then you can not trust something like a users role that you’ve fetched from session. A user could have been banned/demoted/deleted or otherwise modified since the last request.

Sponsor our Newsletter | Privacy Policy | Terms of Service