Cookie troubles

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
User avatar
8ennett
Forum Commoner
Posts: 63
Joined: Sat Sep 06, 2008 7:05 am

Cookie troubles

Post by 8ennett »

Hey guys, been racking my brains trying to find where the problem is in this code. It's a login system (although still not complete) with the login page, registration page, even sends out a confirmation email.

At first the cookie was working fine, then I changed some code in the custom function 'insert_online_user($user_id)' (line 146 of functions.php) because i noticed there was an error in the MySQL query and suddenly login.php stops creating the new cookie.

Home.php which is where the login takes you when you have entered correct info (home.php has nothing in it except for "include('header.php');" but if the cookie doesn't exist it just sends you back to login.php.

Here's the code anyway, if you can see where the problem is then please let me know, I just can't figure it out!

Login.php

Code: Select all

<?php
// First check to see if user is already logged in
if (isset($_COOKIE["KoG"])){
    header('Location: home.php');
    exit;
}
// we must never forget to start the session
session_start();
include ('library/functions.php');
 
$errorMessage = '';
if (isset($_POST['txtUserId']) && isset($_POST['txtPassword'])) {
    // first check if the number submitted is correct
    $number   = strip_tags($_POST['txtNumber']);
    
    if (md5($number) == $_SESSION['ranval']) {
        open_db();      
        $userId   = strip_tags($_POST['txtUserId']);
        $password = strip_tags($_POST['txtPassword']);
        
        // check if the user id and password combination exist in database
        $sql = "SELECT * FROM memuserlist WHERE user='$userId' AND user_password=PASSWORD('$password')";
        
        $result = sql_query($sql) or die('MySQL ERROR. ' . mysql_error() . 'Please inform the administrator!'); 
        close_db();
        if (mysql_num_rows($result) == 1) {
            // the user id and password match, 
            // implode result and set it to a cookie
            $cookieinput = implode('.,.', $result);
            setcookie("KoG",$cookieinput);
            // remove the random value from session         
            $_SESSION['ranval'] = '';
            // after login we move to the main page
            header('Location: home.php');
            exit;
        }
        else {
            $errorMessage = 'User/Pass incorrect!';
        }
    }
    else {
        $errorMessage = 'Captcha incorrect!';
    }   
}
?>
Functions.php

Code: Select all

<?php
 
// Basic database query
function sql_query($query) {
    return mysql_query($query);
}
 
// Open database connection
function open_db(){
    include ('library/config.php');
    include ('library/opendb.php');
}
 
// Close database connection
function close_db(){
    include ('library/closedb.php');
}
 
// Check that all registry fields have been filled in
function check_reg_fields($mail, $mailconf, $user, $fname, $lname, $country, $pass, $passconf) {
    global $check_reg_fields;
    $mail = $mail;
    $mailconf = $mailconf;
    $user = $user;
    $fname = $fname;
    $lname = $lname;
    $country = $country;
    $pass = $pass;
    $passconf = $passconf;
    if ($mail == ''){
        $error = 'True';
    }
    if ($mailconf == ''){
        $error = 'True';
    }
    if ($user == ''){
        $error = 'True';
    }
    if ($fname == ''){
        $error = 'True';
    }
    if ($lname == ''){
        $error = 'True';
    }
    if ($country == 'Select Country'){
        $error = 'True';
    }
    if ($pass == ''){
        $error = 'True';
    }
    if ($passconf == ''){
        $error = 'True';
    }
    if($error == 'True'){
        $check_reg_fields = 'True';
    }
    else{
        $check_reg_fields = 'False';
    }
}
 
 
// Checks to see if username exists
function user_exists($user){
    global $user_exists;
    open_db();
    $query = "SELECT * FROM memuserlist WHERE user='".$user."'";
    $result = sql_query($query);
    if(mysql_num_rows($result)>0){
        $user_exists = 'True';
    }
    else{
        $user_exists = 'False';
    }
    close_db();
}
 
 
// Register a new user
function make_user($verifid, $mail, $user, $fname, $lname, $country, $pass) {
    global $make_user;
    $time = time();
    $regip = getenv(REMOTE_ADDR);
    open_db();
    $query = "INSERT INTO memuserlist (verifid, user_email, user, handle, firstname, lastname, country, user_password, register_date, regip, last_ip, last_access) VALUES ('".$verifid."', '".$mail."', '".$user."', '".$user."', '".$fname."', '".$lname."', '".$country."', PASSWORD('".$pass."'), '".$time."', '".$regip."', '".$regip."', '".$time."')";
    if(sql_query($query)){
        $make_user = 'True';
    }
    else{
        $make_user = 'False';
    }   
    close_db();
}
 
 
// Send verification e-mail
function send_verif_mail($verifid, $mail, $user, $fname, $lname) {
    global $send_verif_mail;
    // Below the $htmlbody variable contains all the HTML content of email.
    $htmlbody = "<html>
    <head>
    <title>Kingdoms of Glory: Verification E-Mail</title>
    </head>
    <body><center>
    <h2>Thank you for registering with Kingdoms of Glory!</h2></center>
    <p>Below you will find all your login information including your 8-digit validation code. Please enter this code the next
    time you login.</p>
    <p>Name: ".$fname." ".$lname."<br />
    Login Name: ".$user."<br />
    Verification Code: ".$verifid."</p>
    <p><a href=http://www.kingdomsofglory.com/login.php>Click here to login in to your account</a></p>
    <p>We'll see you inside!<br />
    The KoG Team</p></body></html>";
    // Call the swift mail SMTP connection settings.
    include ('library/Swift.php');
    include ('library/Swift/Connection/SMTP.php');
    // Connect to swiftmail -- settings are provided by webhost.
    $smtp =& new Swift_Connection_SMTP("localhost", 25);
    $smtp->setUsername("");
    $smtp->setpassword("");
    $swift =& new Swift($smtp);
    // Create the message.
    $message =& new Swift_Message("Kingdoms of Glory: Verification E-Mail", "$htmlbody", "text/html");
    // Send it and test if successful
        if ($swift->send($message, $mail, "noreply@kingdomsofglory.com")){
            $send_verif_mail = 'True';
        }
        else{
            $send_verif_mail = 'False';
        }
    // Disconnect from mail server
    $swift->disconnect();
}
 
// Returns the current amount of registered users
function get_reg_users(){
    global $get_reg_users;
    $query = "SELECT * FROM memuserlist";
    open_db();
    $result = sql_query($query);
    $get_reg_users = mysql_num_rows($result);
    close_db();
}
 
// Inserts to db user online status
function insert_online_user($user_id){
    // Set timeout for active users
    $timestamp = time(); 
    $ip = $_SERVER['REMOTE_ADDR'];
    $self = $_SERVER['PHP_SELF'];
    open_db();
    $query = "INSERT INTO usersonline VALUES ( '$timestamp', '$ip', '$self', '$user_id')";
    $result = sql_query($query);
    if (!($result)){
        echo 'ERROR A200: Please inform the administrator immediately!';
    }
    close_db();
}
 
// Deletes user online status from db
function remove_online_user($user_id){
    $timeoutseconds = 900; 
    $timestamp = time(); 
    $timeout = $timestamp-$timeoutseconds;
    $query = "DELETE FROM usersonline WHERE timestamp<$timeout";
    $result = sql_query($query);
    if(!($result)) { 
        echo 'ERROR A201: Please inform the administrator immediately!'; 
    }
}
 
// Grabs user online status from db
function grab_online_user(){
    global $grab_online_user;
    open_db();
    $query = "SELECT DISTINCT user_id FROM usersonline";
    $result = sql_query($query);
    if(!($result)) { 
        echo 'ERROR A202: Please inform the administrator immediately!'; 
    }
    else {
        $grab_online_user = mysql_num_rows($result);    
    }
    close_db();
}
?>
Header.php

Code: Select all

<?php
if (!isset($_COOKIE["KoG"])){
    header('Location: login.php');
    exit;
}
else {
    $userinfo = explode('.,.',$_COOKIE["KoG"]);
}
 
include ('library/functions.php');
insert_online_user($userinfo['user_id']);
remove_online_user($userinfo['user_id']);
?>
The rest of the pages are just the page design and layout etc.
In Functions.php it's only the bottom three custom functions that are recent, the rest all work perfectly.

And don't mock me for my code structure/method, i'm new to PHP remember?
User avatar
Stryks
Forum Regular
Posts: 746
Joined: Wed Jan 14, 2004 5:06 pm

Re: Cookie troubles

Post by Stryks »

8ennett wrote:And don't mock me for my code structure/method, i'm new to PHP remember?
I don't think that anyone is going to mock you, but there seems to be a lot to comment on in regards to your code. It's all good though ... advice, whether you take it or not, is given to help you get cleaner stronger code.

First off ... is there any specific reason you want to use cookies? You can store all that persistent data in sessions (that's what it's there for) and it'll be so much easier than fighting the cookie monster. If you do want to stay with cookies, then seriously, don't put the password in there. Just don't. It's insecure and it's not needed anyhow.

If you want to use cookies just to keep a user logged in over days or weeks, well .. it's not a good idea to start with, and even then there are better methods.

Also, there are a few functions in there that are just a bit of a waste of space at this point. I mean ...

Code: Select all

function sql_query($query) {
    return mysql_query($query);
}
What is that about? You have a problem with writing my in front of your database calls? I know ... it seems pretty harmless and petty ... but think six to eight months down the track ... will you remember that it just does a pass through. Or will you think "Oh yeah .. that's my database function, so it's safe". Just as a guideline I'd say not to make functions that don't add significant functionality to a native function, and when you do make a function, make the name descriptive so you'll know what it does.

That segways quite nicely into two other points. Database query security, and single use functions.

You MUST at least use mysql_real_escape_string() on ANYTHING that that comes from the user and goes into your database. Do a search on here for database security (I mean, you might be reading for weeks - it's a big topic) and you'll see that it's a big deal to get right. But a big step in the right direction here is to use mysql_real_escape_string().

This 'check_reg_fields' function you have ... I'd really recommend that you put that inline with the processing code. I know it seems nice to put it away somewhere ... it makes your code look all nice and neat. But it's really just a way of hiding problems somewhere where you might not think to look. I'm not saying it's wrong ... I'm just saying it's a complicating simplification.

Now for a few more basic things. In functions, you don't need to use global to send information back to the calling code. You can end the function and return whatever data you like using return. For example:

Code: Select all

echo sayHello('Fred');
 
function sayHello($name) {
   return 'Hello ' . $name;
}
That will give you "Hello Fred". Well .. it should do anyhow.

Also, if you're setting things for a yes/no test, then you would set it to the boolean true or false, not the string 'true' or 'false'. So ...

Code: Select all

$test = true;
 
if($test) echo "It's True";
if($test === true) echo "It's definitely true";
if(!$test) echo "It's False";
if($test !== true) echo "It's definitely false";
 
So if you've read this far, I hope you've got something out of it, because I have some bad news to finish with. I'm not sure why your code isn't working. :lol:

On the bright side though ... if you stick to sessions, you wont need to work out what is going wrong.

Cheers
User avatar
8ennett
Forum Commoner
Posts: 63
Joined: Sat Sep 06, 2008 7:05 am

Re: Cookie troubles

Post by 8ennett »

I don't think that anyone is going to mock you, but there seems to be a lot to comment on in regards to your code. It's all good though ... advice, whether you take it or not, is given to help you get cleaner stronger code.
I only said that because my forum experiences in the past (not this forum obviously) I was always met with people being condesending and so on!

That custom function sql_query($query); I always knew it was pointless and can't really figure out why I implemented it, but after it was done I just forgot about it.

Also, one of my reasons for grouping everything in to functions.php, I was planning on using the completed set of scripts (not just the login and so on but the whole rpg when it's finished) and renaming them in to easier functions and releasing the whole lot (after some modification) as a customised set of simple code for people who don't know/want to learn php. I know that's very ambitious for a beginner, but all this practical work is helping my own skills advance a lot faster than if I just sat down and work through a set of tutorials with standard examples!

I originally used sessions for the login instead of cookies, but wasn't aware you could store information apart from a session name. Guess that's another thing learned.

Thanks for the advice too, it's actually quite helpful!
User avatar
8ennett
Forum Commoner
Posts: 63
Joined: Sat Sep 06, 2008 7:05 am

Re: Cookie troubles

Post by 8ennett »

So if I ran my db query and stored the row in say $row would this be an efficient way of storing information in the session?

Code: Select all

<?php
session_start('user_id');
session_start('name');
session_start('gender');
 
$_SESSION('user_id') = $row['0'];
$_SESSION('name') = $row['1'];
$_SESSION('gender') = $row['2'];
 
or am I just completely off key there?

Oh and I also knew it was pointless to create the open_db and close_db custom functions where they just pull seperate scripts from elsewhere, why not just put the contents in to the functions.php script instead. Some tutorials etc. have strange methods which i'm only just picking up on now my skills are developing!
User avatar
Stryks
Forum Regular
Posts: 746
Joined: Wed Jan 14, 2004 5:06 pm

Re: Cookie troubles

Post by Stryks »

That's not quite how sessions work, but I can see how you've arrived there.

For a more complete example of using sessions ... read the manual page HERE.

But basically, you would just do the following.

Code: Select all

<?php
session_start();
 
// Get the values from the database
 
$_SESSION('user_id') = $row['0'];
$_SESSION('name') = $row['1'];
$_SESSION('gender') = $row['2'];
?>
Then on subsequent pages ....

Code: Select all

<?php
// Dont forget to start your sessions everywhere you need session values - and always at the very top of the page before any other output
session_start();
 
echo 'I happen to know that you are ' . $_SESSION['gender'];
?>
Easy as that. Although, when carrying a set of data around, I always try to keep it together. Something more like ...

Code: Select all

$_SESSION['USER'] = array('user_id'=>$row[0], 'name'=>$row[1], 'gender'=>$row[2]);
Then you can do either of the following ...

Code: Select all

 
session_start();
 
echo $_SESSION['USER']['gender'];
 
... but that's just my preference.
8ennett wrote:Oh and I also knew it was pointless to create the open_db and close_db custom functions
Actually ... I don't really see an issue with those functions. First, they group a specific functionality and the naming lets you know exactly what they do. It's much easier just to drop in a single line DB connection creation than to code it out on every page.

Anyhow ... hope that helps.
User avatar
8ennett
Forum Commoner
Posts: 63
Joined: Sat Sep 06, 2008 7:05 am

Re: Cookie troubles

Post by 8ennett »

It does thanks!

Although when retrieving the db query, maybe it would be a better idea to implode the variable instead and then write it to $_SESSION['USER'] or would that be a bad way of doing things considering the information from the query will be displayed on every page of the site, I suppose I would have to explode the string everytime the page loads.
User avatar
Stryks
Forum Regular
Posts: 746
Joined: Wed Jan 14, 2004 5:06 pm

Re: Cookie troubles

Post by Stryks »

I would just leave it as it is to be honest.

You can implode it I guess, but that means you have to explode it again in order to use anything. That give you no real gain for the price of 1 extra function call on session set and 1 extra function call for each subsequent view.

I take it that your worry here is in keeping it clean and tidy - manageable. If this is the case, then just keep in mind that sessions are serialized (see serialize() in the manual - check my sig) for storage. The process is automatic and transparent to you, so you can just set and forget.

And also, keep in mind that if you use the $_SESSION['USER'] storage approach I mentioned, you can clean up all the user information in one step with ...

Code: Select all

unset($_SESSION['USER']);
Job done. 8)
User avatar
8ennett
Forum Commoner
Posts: 63
Joined: Sat Sep 06, 2008 7:05 am

Re: Cookie troubles

Post by 8ennett »

Ok well after revising security and modifying my user tracking/logging system I think I'm on the right track now. Thanks for that!
Post Reply