PHP user class (login/logout/signup)

Question

Started experimenting with building classes, and I've began by converting my user registration/login into a single class. Wanted to stop and ask for feedback before getting too far.

class UserService
{
    private $_email;
    private $_password;

    public function login($email, $password)
    {
        $this->_email = mysql_real_escape_string($email);
        $this->_password = mysql_real_escape_string($password);

        $user_id = $this->_checkCredentials();
        if($user_id){
            $_SESSION['user_id'] = $user_id;
            return $user_id;
        }
        return false;
    }

    protected function _checkCredentials()
    {
        $query = "SELECT *
                    FROM users
                    WHERE email = '$this->_email'";
        $result = mysql_query($query);
        if(!empty($result)){
            $user = mysql_fetch_assoc($result);
            $submitted_pass = sha1($user['salt'] . $this->_password);
            if($submitted_pass == $user['password']){
                return $user['id'];
            }
        }
        return false;
    }   
}

One of the questions I do have related to my class is: should I be building it as this:

$User = new UserService();
$User->login($_POST['email'], $_POST['password']);

Where the login method calls the _checkCredentials method automatically. Or should it be built like:

$User = new UserService();
$UserId = $User->checkCredentials($_POST['email'], $_POST['password']);
$User->login($UserId);

Other than that I've love some tips on how to restructure this and please point out anything I'm doing wrong!

thanks guys

Answer 1

I think your main idea was to separate the user handling (session) from the database query, which is a good thing in my opinion.

However, this is not the case with your actual implementation, because login escapes the data to be sent to the database, even if the rest of the method does not having anything to do with databases. Not to say that your database query depends on a global resource to work. While I'm at it, I will also suggest you use PDO.

Also, your properties $_email and $_password are in the private scope, but are to be accessed by a protected method. This may cause problems. The properties and the method should have equivalent visibility.

Now, I can see that your UserService requires three things: a database handler, an email and a password. It would make sense to put it in a constructor.

Here's how I would do it:

class UserService
{
    protected $_email;    // using protected so they can be accessed
    protected $_password; // and overidden if necessary

    protected $_db;       // stores the database handler
    protected $_user;     // stores the user data

    public function __construct(PDO $db, $email, $password) 
    {
       $this->_db = $db;
       $this->_email = $email;
       $this->_password = $password;
    }

    public function login()
    {
        $user = $this->_checkCredentials();
        if ($user) {
            $this->_user = $user; // store it so it can be accessed later
            $_SESSION['user_id'] = $user['id'];
            return $user['id'];
        }
        return false;
    }

    protected function _checkCredentials()
    {
        $stmt = $this->_db->prepare('SELECT * FROM users WHERE email=?');
        $stmt->execute(array($this->email));
        if ($stmt->rowCount() > 0) {
            $user = $stmt->fetch(PDO::FETCH_ASSOC);
            $submitted_pass = sha1($user['salt'] . $this->_password);
            if ($submitted_pass == $user['password']) {
                return $user;
            }
        }
        return false;
    }

    public function getUser()
    {
        return $this->_user;
    }
}

Then use it as such:

$pdo = new PDO('mysql:dbname=mydb', 'myuser', 'mypass');

$userService = new UserService($pdo, $_POST['email'], $_POST['password']);
if ($user_id = $userService->login()) {
    echo 'Logged it as user id: '.$user_id;
    $userData = $userService->getUser();
    // do stuff
} else {
    echo 'Invalid login';
}

Answer 2

I have said this a lot on stackoverflow before but what I think you are doing wrong is that you again are trying to create a login-system(even Jeff Atwood agrees with me on this) which is probably going to be unsafe. Just to name a few things that could go wrong:

• You don't do authentication over safe connection(https) which means that that your username/password could be sniffed from the wire.
• It could have XSS-hole.
• The passwords aren't stored safe in the database because of incorrect use of salt. You aren 't a security expert so I don't think you should even store such sensitive information in your database anyway!
• It has a CSRF-hole.

Then there is the annoyance that we have yet to create another account on your server. You could and should avoid this hassle by using one of the freely available alternatives which have been tested for security vulnerabilities by experts:

• openid => Lightopenid is a really easy library to use/integrate. Even stackoverflow/jeff atwood is using it because he knows it hard to get login-system correctly. Even if you are a security expert.
• google friend connect.
• facebook connect.
• twitter single sign-in.

So safe yourself the time of again devising another login-system and instead use for example the really simple lightopenid library and let users sign in with there google account. The snippet below is the only code you need to get it working:

<?php
# Logging in with Google accounts requires setting special identity, so this example shows how to do it.
require 'openid.php';
try {
    $openid = new LightOpenID;
    if(!$openid->mode) {
        if(isset($_GET['login'])) {
            $openid->identity = 'https://www.google.com/accounts/o8/id';
            header('Location: ' . $openid->authUrl());
        }
?>
<form action="?login" method="post">
    <button>Login with Google</button>
</form>
<?php
    } elseif($openid->mode == 'cancel') {
        echo 'User has canceled authentication!';
    } else {
        echo 'User ' . ($openid->validate() ? $openid->identity . ' has ' : 'has not ') . 'logged in.';
    }
} catch(ErrorException $e) {
    echo $e->getMessage();
}

Answer 3

It depends on your design and the notion of a more simpler solution, and how you want to organize your code and keep it simpler, smaller and make it maintainable at the same time.

Ask yourself why you would call checkCredentials and then call login and then maybe some other method. Do you have a good reason for doing so, is this a good design, what do I want to achieve with this.

Calling just login, and performing every login operation is much more simple and elegant. Giving it another name while doing so is much more understandable and also more maintainable.

If you ask me, I would use a constructor.

Answer 4

The answer really depends on other architecture considerations, such as whether the checkCredentials() method needs to be available outside of the class scope for other system elements. If its sole purpose is to be called by login() method, consider combining the two into just one method.

One more recommendation I might have is to use verbs in naming methods, meaning that 'login' may be too general for understanding its effects at first glance.