2
\$\begingroup\$

I have written an Auth class. It's not 100% finished, but it's fully working as expected (tested).

Now my questions are, do you suggest any improvements regarding the structure and are there any security concerns? Note that the script will be running over SSL.

<?php

namespace Slimproject\Auth;

use Slimproject\Models\User;
use Slimproject\Models\Attempt;
use Slimproject\Models\Authentication;

/*
*   Notes:
*   Minimum PHP version: 7.0.0 (Ternary operator '??' and 'random_bytes' are used)
*   Cookies required. Without the system does'nt work (atm.)
*
*   Definitions:
*   Auth = Refers to the actual authentication with all of its aspects
*   Authentication = Refers to the database table where the active user sessions are stored and tracked
*/

class Auth
{
    protected $user;
    protected $attempt;
    protected $authentication;

    public function __construct(User $user, Attempt $attempt, Authentication $authentication)
    {
        $this->user = $user;
        $this->attempt = $attempt;
        $this->authentication = $authentication;
    }

    /**
    * Logs a user in
    * @return boolean
    */
    public function login(string $email, string $password)
    {
        $user = $this->getUser($email);

        if (!$user || !$this->verifyPassword($user, $password)) {
            $this->addAttempt(false, $email);
            return false;
        }

        $this->addAttempt(true, $email);
        return $this->createAuth($user->id);
    }

    /**
    * Logs out the user and removes the remember me
    * @return boolean
    */
    public function logout(string $auth_id)
    {
        $this->deleteAuth($auth_id);
        return (!$this->isLoggedIn());
    }

    /**
    * Verify user's password
    * @return bool
    */
    public function verifyPassword(User $user, string $password)
    {
        return password_verify($password, $user->password);
    }

    /**
    * Returns is user logged in
    * @return boolean
    */
    public function isLoggedIn(bool $force_regenerate_auth = false) 
    {
        $session = $this->getActiveSession();

        if ($session && !$this->isExpiredDate($session['refresh_time']) && $this->checkIp($session['ip'])&& $this->checkAgent($session['agent']) && !$force_regenerate_auth) {
            return true;
        }

        $auth_id = $this->getAuthIdFromCookie();
        $auth_token = $this->getAuthTokenFromCookie();

        if (!$auth_id || !$auth_token) {
            $this->deleteAuth($auth_id);
            return false;
        }

        $authentication = $this->getAuthentication($auth_id);

        if (!$authentication) {
            return false; // Invalid cookie ID or session is deleted
        }

        // Values must be true to pass
        $a = (hash_equals($authentication->hash, sha1($auth_token)));
        $b = (!$this->isExpiredDate($authentication->expiredate));
        $c = ($this->checkIp($authentication->ip));
        $d = ($this->checkAgent($authentication->agent));

        if (!$a || !$b || !$c || !$d) {
            $this->deleteAuth($auth_id);
        }

        $this->deleteAuth($auth_id); // Remove old session
        $this->createAuth($authentication->user->id, 86400); // Reset session with new expiration time

        return session_regenerate_id(true);
    }

    /**
    * Adds an attempt to database
    * @return object $attempt
    */
    public function addAttempt(bool $success, string $input = '')
    {
        return $this->attempt->create([
            'input' => $input,
            'user_id' => $this->user->where('email', $input)->first()->id ?: '',
            'ip' => $this->getUserIp(),
            'agent' => $this->getUserAgent(),
            'success' => $success,
        ]);
    }

    /**
    * Sets all needed data to authenticate a user
    * @return boolean true
    */
    public function createAuth(int $user_id, int $expire = 86400)
    {
        $token = $this->createToken();
        $hash = sha1($token);
        $expiredate = $this->createExpireDate($expire);

        $this->deleteAuthenticationsByUserAndIp($user_id);
        $this->deleteExpiredAuthentications();

        $authentication = $this->authentication->create([
            'user_id' => $user_id,
            'hash' => $hash,
            'expiredate' => $expiredate,
            'ip' => $this->getUserIp(),
            'agent' => $this->getUserAgent(),
        ]);

        if (!$authentication) {
            return false;
        }

        $a = $this->setAuthSession([
            'user_id' => $user_id,
            'ip' => $this->getUserIp(),
            'agent' => $this->getUserAgent(),
            'refresh_time' => $this->createExpireDate(300),
        ]);

        $b = $this->setCookie(getenv('COOKIENAME_AUTHID'), $authentication->id, $expire);
        $c = $this->setCookie(getenv('COOKIENAME_AUTHTOKEN'), $token, $expire);

        return ($a && $b && $c);
    }

    /**
    * Retrieves all sessions for a user by it's user id
    * @return object $authentication
    */
    public function getAuthentications(int $user_id)
    {
        return $this->authentication->where('user_id', $user_id) ?: false;
    }

    /**
    * Retrieves the session associated with a given auth id
    * @return object $authentication
    */
    public function getAuthentication(int $auth_id)
    {
        return $this->authentication->where('id', $auth_id)->first() ?: false;
    }

    /**
    * Returns current session token
    * @return string $auth_token
    */
    public function getAuthTokenFromCookie()
    {
        return $_COOKIE[getenv('COOKIENAME_AUTHTOKEN')] ?? false;
    }

    /**
    * Returns current session id
    * @return string $auth_id
    */
    public function getAuthIdFromCookie()
    {
        return $_COOKIE[getenv('COOKIENAME_AUTHID')] ?? false;
    }

    /**
    * Returns current authentication session
    * @return string $session
    */
    public function getActiveSession()
    {
        return $_SESSION['Slimproject']['auth'] ?? false;
    }

    /**
    * Gets user data for a given UID and returns an array
    * @return object $user
    */
    public function getUser($user)
    {
        $user = (is_int($user)) ? $this->user->where('id', $uid) : $this->user->where('email', trim(strtolower($user)));

        return $user->first() ?: false;
    }

    /**
    * Removes all session's components
    * @return boolean
    */
    public function deleteAuth(int $auth_id = null)
    {
        $a = $this->deleteAuthSession();
        $b = $this->unsetCookie(getenv('COOKIENAME_AUTHID'));
        $c = $this->unsetCookie(getenv('COOKIENAME_AUTHTOKEN'));
        $d = (isset($auth_id)) ? $this->authentication->where('id', $auth_id)->delete() : true;

        return ($a && $b && $c && $d);
    }

    /**
    * Removes all existing sessions for a given user_id
    * @return int $affected_rows
    */
    protected function deleteAuthentications(int $user_id)
    {
        return $this->authentication->where('user_id', $user_id)->delete();
    }

    /**
    * Removes all sessions for a user by its IP
    * @return int $affected_rows
    */
    protected function deleteAuthenticationsByUserAndIp(int $user_id)
    {
        return $this->authentication->where('user_id', $user_id)->where('ip', $this->getUserIp())->delete();
    }

    /**
    * Removes all expired sessions
    * @return int $affected_rows
    */
    protected function deleteExpiredAuthentications()
    {
        return $this->authentication->where('expiredate', '<', date('Y-m-d H:i:s'))->delete();
    }

    /**
    * Returns a random hash
    * @return string $token
    */
    public function createToken()
    {
        return bin2hex(random_bytes(20));
    }

    /**
    * Returns IP address
    * @return string $ip
    */
    protected function getUserIp()
    {
        if (isset($_SERVER['HTTP_X_FORWARDED_FOR']) && $_SERVER['HTTP_X_FORWARDED_FOR'] != '') {
           return $_SERVER['HTTP_X_FORWARDED_FOR'];
        }
        return $_SERVER['REMOTE_ADDR'];
    }

    /**
    * Returns User agent / browser
    * @return string $agent
    */
    protected function getUserAgent()
    {
        return $_SERVER['HTTP_USER_AGENT'];
    }

    /**
    * Creates a session
    * @return boolean
    */
    public function setAuthSession(array $session_data) 
    {
        $session = $_SESSION['Slimproject']['auth'] = $session_data;
        session_regenerate_id(true);
        return $session;
    }

    /**
    * Deletes a session
    * @return boolean
    */
    public function deleteAuthSession() 
    {
        $_SESSION['Slimproject']['auth'] = [];
        unset($_SESSION['Slimproject']['auth']);
        return session_regenerate_id(true);
    }

    /**
    * Creates a cookie
    * @return boolean
    */
    public function setCookie($name, $value, $expire) 
    {
        return setcookie($name, $value, time() + $expire, '/', NULL, false, true);
    }

    /**
    * Removes a cookie
    * @return boolean
    */
    public function unsetCookie($name) 
    {
        return setcookie($name, '', time() - 1, '/', NULL, false, true);
    }

    /**
    * Check if an IP matches the current user IP
    * @return boolean
    */
    public function checkIp(string $ip) 
    {
        return ($ip === $this->getUserIp());
    }

    /**
    * Check if an browser agent matches the current user agent
    * @return boolean
    */
    public function checkAgent(string $agent) 
    {
        return ($agent === $this->getUserAgent());
    }

    /**
    * Returns an new date with the expiration time added
    * @return string $date
    */
    public function createExpireDate(int $expire = 86400) 
    {
        return date('Y-m-d H:i:s', time() + $expire);
    }

    /**
    * Returns true if an date has expired. Otherwise false is returned.
    * @return boolean
    */
    public function isExpiredDate(string $date) 
    {
        return (time() > strtotime($date));
    }

}
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$

That's quite a lot to review but what I could spot.

  1. http://blog.ircmaxell.com/2012/11/anatomy-of-attack-how-i-hacked.html
    Frankly, following a widespread superstition in pursue a better security, you are getting the opposite result, giving a hacker perfect means to spoof the IP address and thus hijack the session.
  2. For some reason you have $user as a class property but many functions accept user as a parameter. Choose one?
  3. To my taste, there are about ten times more methods in this class than should be. I would either drop out most of one-line methods, or, if you indeed want to be more catholic than the Pope, decouple this class to a series of service classes, all the cookie stuff for example.
  4. I've no idea what Authentication is but where() method looks alien to it. I would expect such a method in a Query Builder class but authentication is not a query builder. Judging from the usage, Authentication should provide methods that you made parts of this class, keeping all where() calls internal.
\$\endgroup\$
1
  • \$\begingroup\$ Interesting blog you posted! I indeed forgot to remove the $user property, after i've had it first DI'd. I thought i was good practice to keep things separated and lets each function do only 1 thing. But i can for sure get rid of some one liners :P. So you mean the queries should be in the database models? Makes sense.. Thanks for your tips! \$\endgroup\$ May 3, 2017 at 17:08
1
\$\begingroup\$

This class is doing way too much.

Amongst other things, this class:

  • Performs user logins/logouts
  • Records user login attempts to a database.
  • Performs password verifications.
  • Reads data about the request (cookies, end use IP address, user agent, etc.).
  • Generates tokens/hashes.
  • Exposes generalized cookie setting/unsetting functionality.
  • Persists authentication status to session.
  • Persists authentication status to cookies.
  • Loads user data from the database.

But I think my main concern with it is that it uses sessions vs. cookie indiscriminately.

I will start with the assumption that you are propagating sessions via cookie, as is considered best practice. If that is the case, why do you need to persist information about the user's authentication state outside of session data in their own cookies?


I worry that you are arbitrarily setting expiries on cookies that may or may not match your session cookies lifetime value. You could really get your application into a bad state of perhaps have valid authentication cookie values but invalid session cookie or similar.


Some more specific code comments follow:

    $_SESSION['Slimproject']['auth'] = [];
    unset($_SESSION['Slimproject']['auth']);

Why is this first line of code necessary?


   if (!$a || !$b || !$c || !$d) {

Consider meaningful variable names (typical throughout).


* @return object $authentication
*/
public function getAuthentications(int $user_id)
{
    return $this->authentication->where('user_id', $user_id) ?: false;

I like that you are using PhpDoc, but you seem be be using improperly in some cases. The @return here should probably be:

@return Authentication|false

There should not be variable name after type declaration here. It has no meaning, though a description is optional. Be specific about the object type returned. If you can return multiple types like in this example, be explicit about them.

If you want description it could be something like:

@return Authentication|false Authentication object on success. False on failure.

Do you really want to tie IP address to session/authentication validity? I think this is an old paradigm that still pops up in "secure" web application design that really should be called into question. There are legitimate cases where a user's IP address may change during a session (a mobile user for example) and there are even more cases where a user's IP address may not be "unique" to them during a session (users on a common LAN, users behind proxies, etc.).

\$\endgroup\$
3
  • \$\begingroup\$ The line above unset(sess) is for security, as i've read on other topics to safely logout. Im not using PhpDoc but Eloquent (from Laravel, but not using Laravel itself). I should transfer the queries to the database models right? I'm recreating an facebook like session. So, keeping users logged in. Sessions are used to not have to hit the database on every pageload, but only every 5 minutes. The login state of a user is mainly keeped tracked of in the database. The cookie has an ID which related to a database record. My thoughts about the agent and ip validations was 'its better than nothing' \$\endgroup\$ May 3, 2017 at 16:50
  • \$\begingroup\$ Thanks for pointing the description improvements. I didn't used them before. So good to know some best practices! And about the IP validity, any better solutions/suggestions anno 2017? Can you point me to some up to date topics/posts? \$\endgroup\$ May 3, 2017 at 17:13
  • \$\begingroup\$ @RamonBakker WRT line above unset. How does setting session value to empty array the line before unsetting it altogether improve security? I think just unsetting it is fine. WRT IP address. I would say just remove it. It is reasonable that user agent doesn't change during a session, maybe not so reasonable that IP doesn't change. \$\endgroup\$
    – Mike Brant
    May 3, 2017 at 17:40
0
\$\begingroup\$

You are already requesting the user’s id in the login method, but you are sending the email into the addAttempt method, and in the addAttempt method you are requesting the user id from the email, you should use the user id in the addAttempt method instead. (The attempt can’t be added if no user with given email found)

You are only using getUser with a string as email, so i would remove the option to parse in a id, or at least split it up in 2 function so a user can't send a number and get a user using a user id in other parts of the site.

\$\endgroup\$
2
  • \$\begingroup\$ I want to add an attemp even if no user is found. So its storing the actual user input, and if an user is found with that input it stores also the user id. If no input is given or an user is found an attemp will be created anyway. About getUser, An user can never send an integer. Posted values are always strings. So it will never conflict with the id. \$\endgroup\$ May 3, 2017 at 9:40
  • 1
    \$\begingroup\$ A developer could still parse user input to a int and accidentally use it in the get user function as long the function is public. It’s still possible to add an optional userid parameter to the addAttempt method so it doesn’t have to look it up if the email was found. \$\endgroup\$
    – Benjaco
    May 3, 2017 at 9:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.