3
\$\begingroup\$

I have a class that has a couple methods - a standard login (when the HTML login form is sent) and login via cookie (for automatic login of returning users).

Let's now focus on the login method. It does two things - sanitizes the input and sets a login session and a cookie if the user has marked the "remember me" checkbox. NetBeans is saying my method is too big - 59 lines and the allowed size is only 20 lines. I guess I can separate the input sanitizing from the sessions and cookies setting. But are methods supposed to be for a single operation? Because as I said already, mine does more than a single operation. Also, in the real world and in a bigger project how is this limit of 20 lines working out? I really want my project to comply to the best practices.

<?php

namespace App\Models\Auth;

use \App\Core\Database;
use \App\Models\Auth\PasswordEncryption as PasswordEncryption;
use \App\Core\FlashMessage;
use \DateTime;

class Login
{    

    protected $db;

    public function __construct() 
    {
        $this->db = Database::getInstance();
    }

    /** 
     * @param String $username - The username of the user
     * @param String $password - User's password (non hashed)
     * @param bool $rememberMe - If TRUE, set a cookie.
     * @param String $token - Login token that is kept in a cookie if $rememberMe is TRUE.
     * @param String $userAgent - returns md5($_SERVER['HTTP_USER_AGENT'])
     * @param String|Null $redirect - URL to redirect to when login is successful. 
     */
    public function login(string $username, string $password, bool $rememberMe, string $csrf, string $userAgent, string $redirect)
    {
        /**
         * Check against cross-site forgery
         */
        if($csrf != \App\Core\CSRF::check($csrf))
        {
            $err[] = 'CSRF error.';
        }

        /**
         * Check if fields are filled.
         */
        if(empty($username) || empty($password))
        {
            $err[] = 'Please fill all fields.';
        }

        /**
         * Get data from the database for the requested username.
         */
        $query = $this->db->getRows("SELECT userid, password, type FROM users WHERE username = ?", [$username]);
        if($query == null)
        {
            $err[] = 'Username does not exist.';
        }

        /**
         * Compare if the password from the field matches with the password from the database.
         */
        $passwordEncryption = new PasswordEncryption();
        if($passwordEncryption->check($password, $query[0]->password) === false)
        {
            $err[] = 'Incorrect password.';
        }

        /**
         * Check if any errors have been registered so far.
         */
        if($err)
        {
            FlashMessage::error(implode('<br />', $err));
            redirect(SITE_ADDR.'/public/user/login');
        }

        /**
         * If $rememberMe is set, keep the csrf token as a cookie identifier.
         */
        if($rememberMe)
        {
            $time = new DateTime;
            $expiry_timestamp = $time->getTimestamp() + LOGIN_COOKIE_VALIDITY;
            /**
             * Delete the old cookie identifier from the database and insert a new one.
             */
            $this->db->deleteRow("DELETE FROM users_cookies 
                        WHERE (userid = ?)
                        AND (agent_hash = ? OR expiry < ?)", [
                            $query[0]->userid, 
                            $userAgent, 
                            $time->getTimestamp()
                        ]);
            $this->db->insertRow("INSERT INTO users_cookies 
                        (userid, cookie_hash, agent_hash, expiry) 
                        VALUES 
                        (?, ?, ?, ?)", [
                            $query[0]->userid, 
                            $csrf, 
                            $userAgent, 
                            $expiry_timestamp
                        ]);
            /**
             * Keep the cookie identifier in a cookie.
             */
            setcookie("cookie_hash", $csrf, time() + LOGIN_COOKIE_VALIDITY, '/');
        }

        /**
         * Login the user by setting sessions.
         */
        $_SESSION['userid']          = $query[0]->userid;
        $_SESSION['username']        = $username;
        $_SESSION['HTTP_USER_AGENT'] = $userAgent;
        $_SESSION['type']            = $query[0]->type;

        redirect($redirect);
    }

    /**
     * Log in with a cookie if a login session is not set but a cookie exists.
     * @param string $cookie - user's login cookie
     * @param string $session - user's login session
     */
    public function cookieLogin(string $cookie){
        if(!empty($cookie) && empty($_SESSION['username'])){
            $query = $this->db->getRows("SELECT userid, username, type FROM users WHERE userid = ?", [self::getCookieHashUserId()]);

            $_SESSION['userid']          = $query[0]->userid;
            $_SESSION['cookie_hash']     = $_COOKIE['cookie_hash'];
            $_SESSION['username']        = $query[0]->username;
            $_SESSION['HTTP_USER_AGENT'] = md5($_SERVER['HTTP_USER_AGENT']);
            $_SESSION['type']            = $query[0]->type;

            redirect("http://localhost/MVC/public/user/login");
        }
    }

    /**
     * Gets which user the Remember me cookie belongs to
     * @return int
     */
    public function getCookieHashUserId(){
        $query = $this->db->getRow("SELECT `userid` FROM `users_cookies` WHERE `cookie_hash` = ?", [$_COOKIE['cookie_hash']]);
        return $query->userid;
    }

}
\$\endgroup\$
2
  • \$\begingroup\$ "I guess I can separate the input sanitizing from the sessions and cookies setting." Single Responsibility Principle. One function should have one and only one function. Splitting them up would improve modularity and maintainability. \$\endgroup\$
    – Mast
    Commented Sep 30, 2017 at 19:17
  • \$\begingroup\$ How would you recommend me do it? Put the sanitizing method in the same class or create a separate class that contains all sanitizing methods for all kinds of html forms? It's an MVC project. \$\endgroup\$
    – pidari
    Commented Sep 30, 2017 at 19:30

2 Answers 2

2
\$\begingroup\$

are methods supposed to be [responsible] for a single operation? ... Also, in the real world and in a bigger project how is this limit of 20 lines working out?

Yes.

Some methods might be just six lines long. In the real world, "too long" methods really do need to be rewritten as part of code maintenance. A fixed limit will always be arbitrary, and some methods should be longer than 20 lines. But people really will have trouble comprehending your method if it is longer than a screenful of code, if they have to scroll up and down, and up, and down, to read it.

/** 
 * @param String $username - The username of the user
 * @param String $password - User's password (non hashed)
 * @param bool $rememberMe - If TRUE, set a cookie.
 * @param String $token - Login token that is kept in a cookie if $rememberMe is TRUE.
 * @param String $userAgent - returns md5($_SERVER['HTTP_USER_AGENT'])
 * @param String|Null $redirect - URL to redirect to when login is successful. 
 */
public function login(string $username, string $password,
    bool $rememberMe, string $csrf, string $userAgent, string $redirect)

You might elide the obvious "The username of the user", or just put "username" if a string is needed.

The "User's password (non hashed)" comment is good and useful. More briefly you might say "cleartext password".

Comments should say in (sometimes ambiguous) English what the code says with precision. The comment "returns md5($_SERVER['HTTP_USER_AGENT'])" is too precise, it reveals implemenation details and is redundant with the code. Better to say "Hashed user-agent". Then it will still be correct when you switch to sha224.

That comment block is verbose, yet it fails to describe login(). It talks about $token rather than $csrf. Worse, it does not have a sentence explaining what login() is responsible for.

    /**
     * Check against cross-site forgery
     */

I think you wanted to introduce that with /* rather than /**. This is a pattern that extends across your within-method comments. The comment itself is valuable, it explains the abbreviation, mentioning most of the words in "cross-site request forgery".

    /**
     * Check if fields are filled.
     */

Please delete this comment - the (well written) code is perfectly obvious. Except for the part where you overwrote a potential report of a CSRF error. I don't understand why you're not appending error messages. Also, some of your errors, such as CSRF, should be fatal, causing immediate return from your function.

     * Get data from the database for the requested username.

This comment is too vague, because "data" is usually very vague. You meant to say "Get password".

    $query = $this->db->getRows("SELECT userid, password, type FROM users WHERE username = ?", [$username]);

I assume userid is a unique integer and username is a user-friendly string. I don't understand why you queried userid, only to ignore it. The name $query is clearly incorrect, as the variable is holding DB results rather than a SQL query string.

     * Compare if the password from the field matches with the password from the database.

Comments are helpful if they take less time to read than the code. Here, "Validate password." would have sufficed.

    if ($passwordEncryption->check($password, $query[0]->password) === false)

You wrote if (c === false). This is more naturally expressed as if (!c).

    /**
     * Check if any errors have been registered so far.
     */

This description is too passive, and wrong. Yes, we can see that if is "checking". What is missing from the comment is the critically important detail that redirect() won't return. It will call exit(). So the rest of the function does not execute. That is what the English language comment should be calling our attention to.

    redirect($redirect);

I don't understand that line. The method signature comment explained the value may be null. It's not obvious to me that redirect(null) is safe.

Several times you wrote interior comments to introduce a chunk of code. Whenever you feel compelled to do that, consider if you should instead be breaking out a chunk of code into a helper method. Then you will find it easier to stick to the single responsibility principle.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your comment! Very good advice! I will look into making changes to my comments and the other things you suggested. \$\endgroup\$
    – pidari
    Commented Sep 30, 2017 at 21:21
1
\$\begingroup\$

A small but important aspect of your code is the hashing method itself. And I've seen this gone wrong many times.

DON'T use MD5! These hashing methods are way too fast, and therefore can easily be cracked. If you want to hash your passwords properly, use a slower hashing algorithm like blowfish.

In php5.3 and above this is present in the crypt function.

php hash manual

\$\endgroup\$
2
  • \$\begingroup\$ Yes, actually I know. I use md5 for testing purposes because I found it easier to implement. \$\endgroup\$
    – pidari
    Commented Sep 30, 2017 at 21:22
  • \$\begingroup\$ Good :) I was just stating best practises regarding password hashing. \$\endgroup\$
    – Ludisposed
    Commented Sep 30, 2017 at 21:29

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.