OOP Code Review

[benanamen]

I was practicing OOP and made a simple class to log logins. Does anyone see any problems with this or improvements that can be made? Any issue with using NOW() in the query string instead of a placeholder?

 

In another thread, @Jaques1 said:

You should also use type declarations in your methods to make sure that the $pdo argument is in fact a PDO instance.

 

How would I implement that? I rtfm and don't understand it as of yet.

<?php
// ----------------------------------------------------------------------------
// Database Connection
// ----------------------------------------------------------------------------

$dbhost = 'localhost';
$dbname = 'test';
$dbuser = 'root';
$dbpass = '';
$charset = 'utf8';
$dsn = "mysql:host=$dbhost;dbname=$dbname;charset=$charset";
$opt = [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, PDO::ATTR_EMULATE_PREPARES => false, ];
$pdo = new PDO($dsn, $dbuser, $dbpass, $opt);

//------------------------------------------------------------------------
//
//------------------------------------------------------------------------

$valid_login = new LogLoginStatus($pdo);
$valid_login->validLogin('goodusername');

$invalid_login = new LogLoginStatus($pdo);
$invalid_login->invalidLogin('bad_username', 'bad_password');

//------------------------------------------------------------------------
//
//------------------------------------------------------------------------

class LogLoginStatus
    {
    /**
     * Log Valid/Invalid logins
     *
     * @param string login_username
     * @param string login_password
     */

    public function __construct($pdo)
        {
        $this->pdo = $pdo;
        }

    function validLogin($username)
        {
        $sql  = "INSERT INTO user_login (login_status, login_ip, login_username,login_password, login_datetime) values(?, INET_ATON(?), ?, ?, NOW())";
        $stmt = $this->pdo->prepare($sql);
        $stmt->execute(array(
            1,
            $_SERVER['REMOTE_ADDR'],
            $username,
            '***'
        ));
        }

    function invalidLogin($username, $password)
        {
        $sql  = "INSERT INTO user_login (login_status, login_ip, login_username,login_password, login_datetime) values(?, INET_ATON(?), ?, ?, NOW())";
        $stmt = $this->pdo->prepare($sql);
        $stmt->execute(array(
            0,
            $_SERVER['REMOTE_ADDR'],
            $username,
            $password
        ));
        }
    }
?>

CREATE TABLE `user_login` (
  `login_id` int(11) NOT NULL AUTO_INCREMENT,
  `login_status` tinyint(1) DEFAULT NULL,
  `login_ip` int(10) unsigned DEFAULT NULL,
  `login_username` varchar(255) DEFAULT NULL,
  `login_password` varchar(255) DEFAULT NULL,
  `login_datetime` datetime DEFAULT NULL,
  PRIMARY KEY (`login_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

Edited October 5, 2016 by benanamen

[Jacques1]

Are you storing the plaintext password when the log-in fails?

 

 

 

How would I implement [type declarations]? I rtfm and don't understand it as of yet.

 

PHP allows you to specify the type of a parameter in the method signature:

<?php

class LoginAttemptsLog
{
    public function __construct(PDO $pdo)   // note the "PDO" type declaration
    {

    }
}

// passing anything other than a PDO instance leads to an error
$loginAttemptsLog = new LoginAttemptsLog(123);

The benefits are that PHP can automatically validate the parameter types, you get a parameter documentation for free, and good IDEs can use the information to assist you and point out mistakes.

 

In PHP 7, you can even have type declarations for scalar types (strings, integers etc.).

 

As to the improvements:

• LoginAttemptsLog may be a better name
• Static values like the number 1 belong directly into the query; it doesn't make sense to pass them as parameters
• When you have multiple methods doing almost the same thing, create a private or protected method for the common code and call that in the public methods instead of literally duplicating the code.

 

Any issue with using NOW() in the query string instead of a placeholder?

 

No. Unless you have reasons to believe that the timestamp returned by NOW() is not what you want.

 

Edited October 5, 2016 by Jacques1

[benanamen]

Are you storing the plaintext password when the log-in fails?

 

Currently it does. It should be logging Bad Username/Bad Password and Valid username/Bad password. I know right now it will also do the undesired bad username/good password which is a fail since a good pass is in plain text. This started from an old procedural login code and right now is just for learning OOP. The idea was to see when a login fails, what was entered when it failed.

 

* Edit: Original code did not log good password in plain text.

 

I see what you did with the PDO type declaration. I never would have got that from the Manual. Does that fall under the self type on the man page linked to?

 

Agree with your class name suggestion

 

• When you have multiple methods doing almost the same thing, create a private or protected method for the common code and call that in the public methods instead of literally duplicating the code.

 

I could see that it was near duplicate code and figured that was an area that could be improved. With that said I don't understand how to implement what you just said. I do get the gist of it, but implementing in OOP I am lost at the moment.

 

I still have less than 24 hours into serious OOP study.

Edited October 6, 2016 by benanamen

[kicken]

Currently it does. It should be logging Bad Username/Bad Password and Valid username/Bad password. I know right now it will also do the undesired bad username/good password which is a fail since a good pass is in plain text. This started from an old procedural login code and right now is just for learning OOP. The idea was to see when a login fails, what was entered when it failed.

You should not be logging a plain-text password regardless of whether it's valid or not.

 

A key point from the above Q/A:

 

 

The biggest risk is if this database of invalid passwords becomes accessible to attackers. Either they compromise the server, perform SQL injection, or retrieve it in some other way.

...

A common source of login failures is minor typos during the password entry process. So my password is Muffins16 but I type in mUFFINS16 because my caps lock is on. Or Muffins166 because I hit the same key twice. Or Muffina16 because my finger hit the wrong key. As you can see these variations are close enough to the original that attackers can probably determine the valid password from invalid passwords by trying a few minor alterations or comparing wrong passwords to likely dictionary words or names.

 

 

I see what you did with the PDO type declaration. I never would have got that from the Manual. Does that fall under the self type on the man page linked to?

self is a keyword that references the same class the method is defined in. It's just an easy way to reference the class without having to do so by it's specific name. For example you could do new self() within a method to create a new instance of the class or specify a method takes an instance of itself as an argument via function compare(self $other){ ... }.

 

The line above self says Class/interface name, that is the relevant line. It means you can use any class or interface name as a type hint.

 

 

 

 

I could see that it was near duplicate code and figured that was an area that could be improved. With that said I don't understand how to implement what you just said. I do get the gist of it, but implementing in OOP I am lost at the moment.

Something like this:

class LoginAttemptsLog {
    public function validLogin($username){
        $this->logAttempt($username, '***', 1);
    }

    public function invalidLogin($username, $password){
        $this->logAttempt($username, $password, 0);
    }

    private function logAttempt($username, $password, $isValid){
        $sql  = "INSERT INTO user_login (login_status, login_ip, login_username,login_password, login_datetime) values(?, INET_ATON(?), ?, ?, NOW())";
        $stmt = $this->pdo->prepare($sql);
        $stmt->execute(array(
            $isValid,
            $_SERVER['REMOTE_ADDR'],
            $username,
            $password
        ));
    }
}

[Jacques1]

I know right now it will also do the undesired bad username/good password which is a fail since a good pass is in plain text.

 

It's a security vulnerability in any case, because even wrong passwords will often be very close to the actual passwords. They may in fact be actual passwords from other sites which the user mixed up, or they're old passwords which can be used to figure out the new passwords.

 

You really cannot log any password of any kind. I'm not even sure how that data would be useful.

 

 

 

I see what you did with the PDO type declaration. I never would have got that from the Manual. Does that fall under the self type on the man page linked to?

 

No, it's a class/interface name (the first case in the list).

 

 

 

I could see that it was near duplicate code and figured that was an area that could be improved. With that said I don't understand how to implement what you just said. I do get the gist of it, but implementing in OOP I am lost at the moment.

<?php

/**
 *
 */
class LoginAttemptsLog
{
    /**
     * @var PDO the connection to the underlying database
     */
    protected $database;

    /**
     * @param PDO $database the connection to the underlying database
     */
    public function __construct(PDO $database)
    {
        $this->database = $database;
    }

    /**
     * @param string $username
     */
    public function logFailedAttempt($username)
    {
        $this->logAttempt($username, false);
    }

    /**
     * @param string $username
     */
    public function logSuccessfulAttempt($username)
    {
        $this->logAttempt($username, true);
    }

    /**
     * @param string  $username
     * @param boolean $successful
     */
    protected function logAttempt($username, $successful)
    {
        $attemptStmt = $this->database->prepare('
            INSERT INTO
              user_login (login_status, login_ip, login_username, login_datetime)
            VALUES
              (?, INET_ATON(?), ?, NOW())
        ');
        $attemptStmt->execute([($successful ? 1 : 0), $_SERVER['REMOTE_ADDR'], $username]);

    }
}

[benanamen]

You should not be logging a plain-text password regardless of whether it's valid or not.

 

Oh, I very much get that. As the link you referenced starts "Our client has come up with the requirement that". As I said, this is just for practicing OOP right now and was old procedural code from long, long ago.

 

The class as you have modified it is very clean now. I will just remove the password storage and it should be good to go.

 

More importantly, I just picked up a couple more tidbits of OOP knowledge which was the purpose of doing this in OOP. 

 

Thanks!

Edited October 6, 2016 by benanamen

[benanamen]

Thanks @Jaques1.

 

I don't know the pros/cons between what you posted and what @kicken posted. Is it an either one will work or is one better than the other for some reason?

[Jacques1]

There aren't any big differences. But I find true/false more appropriate than 1/0, and declaring methods as private means you prohibit subclasses from using them, which doesn't really make sense in this case.

[benanamen]

 

It's a security vulnerability in any case, because even wrong passwords will often be very close to the actual passwords. They may in fact be actual passwords from other sites which the user mixed up, or they're old passwords which can be used to figure out the new passwords.

 

You really cannot log any password of any kind. I'm not even sure how that data would be useful.

 

 

 

 

No, it's a class/interface name (the first case in the list).

<?php

/**
 *
 */
class LoginAttemptsLog
{
    /**
     * @var PDO the connection to the underlying database
     */
    protected $database;

    /**
     * @param PDO $database the connection to the underlying database
     */
    public function __construct(PDO $database)
    {
        $this->database = $database;
    }

    /**
     * @param string $username
     */
    public function logFailedAttempt($username)
    {
        $this->logAttempt($username, false);
    }

    /**
     * @param string $username
     */
    public function logSuccessfulAttempt($username)
    {
        $this->logAttempt($username, true);
    }

    /**
     * @param string  $username
     * @param boolean $successful
     */
    protected function logAttempt($username, $successful)
    {
        $attemptStmt = $this->database->prepare('
            INSERT INTO
              user_login (login_status, login_ip, login_username, login_datetime)
            VALUES
              (?, INET_ATON(?), ?, NOW())
        ');
        $attemptStmt->execute([($successful ? 1 : 0), $_SERVER['REMOTE_ADDR'], $username]);

    }
}

 

Continuing from https://forums.phpfreaks.com/topic/302938-oop-class-code-review/ in regards to a global being used in a class, my research shows that you do not want to use globals in a class. How then should this last line be properly dealt with? In another thread @requinix said as long as you are not changing anything it is ok. Reference  https://forums.phpfreaks.com/topic/302901-oop-convert-function-to-class/?do=findComment&comment=1541244

$attemptStmt->execute([($successful ? 1 : 0), $_SERVER['REMOTE_ADDR'], $username]);

[Jacques1]

my research shows that you do not want to use globals in a class.

 

And what are the specific arguments for that statement?

 

The superglobals are a core feature of PHP, so it's normal to use them in an application. Wrapping them in custom classes only makes sense if you have a concrete reason (extra features, access control, ...). Otherwise you're just making the application more complex.

 

I know that some automated scanners and IDEs complain about superglobals, but that doesn't mean you should blindly follow this advice.

[benanamen]

 

 

I know that some automated scanners and IDEs complain about superglobals, but that doesn't mean you should blindly follow this advice.

The auto scanner Sensio is mostly where it came from. So, this will not affect unit testing?

[Jacques1]

You can fill the superglobals with whatever test data you need. If you don't like this, that is a valid reason for a wrapper. My point is that you shouldn't write wrappers just because some scanner said so.

[benanamen]

I am just looking to learn the optimum best practices for OOP. I have just started to read up on unit testing and writing tests. Have not actually wrote one yet. OOP is finally starting to make sense to me.

[ignace]

You can also pass the IP address as a constructor argument. Decoupling your class from the globals. If you use dependency injection it's even easier to do.

 

Mind that when you are behind a proxy getting the IP address is not as simple as querying REMOTE_ADDR which further warrants the need for injection.

 

class LoginAttemptsLog
{
/**
* @var PDO the connection to the underlying database
*/
protected $database;

/**
* @var string
*/
protected $ipAddress;

/**
* @param PDO $database the connection to the underlying database
* @param string $ipAddress the IP of the user
*/
public function __construct(PDO $database, $ipAddress)
{
$this->database = $database;
$this->ipAddress = $ipAddress;
}

// ..
}