Please critique my user management script

[spudmurf2k]

Hi guys,

I've written a user management script for my site. It works perfectly and I'm just wondering could you all have a look and tell me if there's any gaping holes, security worries, crappy scripting or any thoughts that cross your mind while looking at it. Thanks.

This is my first go at OOP aswell, so any pointers there so I don't develop bad habits would be much appreciated.

First off here is the SQL for the user's db. (For Reference)

CREATE TABLE `users` (
  `user_id` int(7) unsigned zerofill NOT NULL auto_increment,
  `username` varchar(20) default NULL,
  `psword` blob NOT NULL,
  `email` varchar(255) NOT NULL,
  PRIMARY KEY  (`user_id`),
  UNIQUE KEY `email` (`email`),
  UNIQUE KEY `username` (`username`)
) ENGINE=InnoDB;

The idea of the script is that you can login/out from any page by just POSTing the login data to the page with a hidden field 'login' or logout by GETing the field logout.

It uses cookies to keep a user logged-in and if they log-out it remembers their email address.

Here is the class:

<?php
class User {
    var $loginfailed = FALSE;
    var $errmsg;
    var $userdata;
    var $remember = FALSE;

    function User () {
    # Can call login/out from any page and it will log them in and continue to display the page
    if ($_POST['login'] == 1) $this->login($_POST['username'], $_POST['password'], $_POST['remember']);
    elseif ($_GET['logout'] == 1) $this->logout();
    # Logs in unlogged-in users with cookies
    elseif (!isset($_SESSION['loggedin']) && isset($_COOKIE['my_site_name']['user_id'], $_COOKIE['my_site_name']['hash'])) { // loggedin is not set yet or is false
        $this->eatCookie();
        }
    }

    function login ($username, $password, $remember) {
        $this->remember = $remember;
        $result = mysql_query("SELECT user_id FROM users WHERE username = '$username' OR email = '$username';");
        if (mysql_num_rows($result) !== 1) {
            // No username like this exists, show login form again
            $this->loginfailed = TRUE;
            $this->errmsg = 'Sorry, Your Username Doesn\'t Exist';
            return FALSE;
            }
        else {
            $user_id = mysql_result($result, 0);
            $this->userdata = mysql_fetch_object(mysql_query("SELECT user_id, email, psword FROM users WHERE psword = PASSWORD('$password')"));
            if (!is_object($this->userdata)) {
                // Wrong password
                $this->loginfailed = TRUE;
                $this->errmsg = 'Sorry, Wrong Password';
                return FALSE;
                }
            else {
                if ($this->doLogin()) return TRUE;
                }
            }
        }

        function doLogin () {
            // set as logged in
            $_SESSION['loggedin'] = TRUE;
            $_SESSION['user_id'] = $this->userdata->user_id;
            // set the cookies
            if ($this->setCookies()) return TRUE;
            }

            function setCookies () {
                // set cookie to remember the username/email address anyway
                setcookie('my_site_name[email]', $this->userdata->email, mktime()+(86400*30), '/', '', FALSE);
                if ($this->remember) {
                    $cookie = md5($this->userdata->email . $this->userdata->psword);
                    if (setcookie('my_site_name[user_id]', $this->userdata->user_id, mktime()+(86400*30), '/', '', FALSE) &&
                        setcookie('my_site_name[hash]', $cookie, mktime()+(86400*30), '/', '', FALSE)) {
                        return TRUE;
                        }
                    }
                }

    function logout() {
        unset($_SESSION['loggedin']);
        unset($_SESSION['user_id']);
        // Delete cookie by setting value to nothing
        $this->clearCookies();
        }

        function clearCookies () {
            // clears 'remember my pass' cookies and leaves e-mail/username one
            if (setcookie('my_site_name[user_id]', '', mktime()+(86400*30), '/', '', FALSE) &&
                setcookie('my_site_name[hash]', '', mktime()+(86400*30), '/', '', FALSE)) {
                return TRUE;
                }
            }

    function eatCookie () {
        // Gets data to make hash using uid in cookie
        $this->userdata = mysql_fetch_object(mysql_query("SELECT user_id, psword, email FROM users WHERE user_id = {$_COOKIE['my_site_name']['user_id']};"));
        // makes hash
        $hash = md5($this->userdata->email . $this->userdata->psword);
        if ($hash == $_COOKIE['my_site_name']['hash']) {
            $this->remember = TRUE;
            if ($this->doLogin()) {
                return TRUE;
                };
            }
        }

    function showLoginForm () {
        if ($this->loginfailed) echo "<h6 class='error'>$this->errmsg</h6>"; ?>
        <form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post" enctype="multipart/form-data" name="loginform">
        <input name="login" type="hidden" value="1" />
        <input name="username" type="text" value="<?php if (!$this->loginfailed) echo $_COOKIE['my_site_name']['email']; ?>" />
        <input name="password" type="password" />
        <input name="remember" type="checkbox" value="TRUE" checked />
        <input name="" type="submit" />
        </form> <?php
        }

    }
?>

It would be used in a page as such:

<?php
require_once('userclass.php');
session_start();
connect(); // script to connect to MySQL db
$user = new User(); ?>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
<title>Log-In</title>
</head>
<body>
<h1>User Login</h1>

<?php $user->showLoginForm(); ?>

</body>
</html>

Please give me your thoughts on this. I was trying to use this script (click for its digg page (why does it have 'digg' as a bad word??)) but you can see from the bad comments themselves that its no good.

Thanks in advance for any input, and if you think its decent feel free to use it if you credit me (Patrick Murphy - www.spudmurphydesign.com). If the commenting is a bit sparse please let me know where you're caught up and I'll help out.

Thanks guys.

[Ezku]

As so many others before you, you have got all of your layers (Model, View and Controller or domain logic (database), display logic and decision making based on requests) mixed in a single class. That's a procedural way of doing things - quite understandable, if this is your first attempt at OOP. See here and here for a few similar discussions.