name is not a property of the Entity user

[dafriend]

DictionaryValidator uses the following to check for personal information in the password

if ($user !== null)
{
    $names = [
        strtolower($user->name),
        strtolower(str_replace(' ', '', $user->name)),
        strtolower(str_replace(' ', '.', $user->name)),
        strtolower(str_replace(' ', '-', $user->name)),
    ];

The problem is that, as far as I can determine, the Entity used in myth/auth does not have a name property. Should they all be $user->username?

I have been thinking of offering a PR on this functionality anyway and, if $user->name needs fixing I can fix it there.

[MGatner]

“name” used to hold a user’s actual name and was removed to make space for developers extending the database with their own fields. This is just a legacy reference that I missed and can probably be removed.

#46

[dafriend]

I figured it was something like that. Thanks.

[lonnieezell]

It does provide an additional layer of protecting a user from themselves, though, and using common fields. We could always make a configuration that allows the developers to specify one or more fields that should be checked against. What do you guys think?

[dafriend]

I think the idea of preventing password leaking by way of username or email is really important. It would be nice if, for instance, a developer added name field(s) to the entity to be able to keep that data out of the password. But allowing for and checking against ad hoc fields feels... daunting... maybe... or maybe not. I'll have to think about that.

I have been thinking that the "no personal info" check should be in a Validator of its own instead of being part of the DictionaryValidator. I'd call it the NothingPersonalValidator. Being stand-alone would help keep things DRY as that code would not need to be repeated in PwnedValidator.

Here's where I'm at with NothingPersonalValidator::check()

/**
 * Returns true if $password contains no part of the username 
 * or the user's email. Otherwise, it returns false. 
 * If true is returned the password will be passed to next validator.
 * If false is returned the validation process will be immediately stopped.
 *
 * @param string $password
 * @param Entity $user
 *
 * @return boolean
 */
public function check(string $password, Entity $user = null): bool
{
    $userName = \strtolower($user->username);
    $email = \strtolower($user->email);
    $password = \strtolower($password);

    // remove all spaces from $userName and password
    $collapsedUserName = str_replace(' ', '', $userName);
    $collapsedPassword = str_replace(' ', '', $password);

    $valid = true;
    if($userName === $email ||
        $password === $email ||
        $password === $userName ||
        $collapsedPassword === $userName ||
        $collapsedPassword === $collapsedUserName)
    {
        $valid = false;
    }

    if($valid)
    {
        // Use spaces to replace non-word characters and underscores in $password
        $strippedPassword = trim(preg_replace('/[\W_]+/', ' ', $password));
        $words = explode(' ', $strippedPassword);

        $trivial = [
            'a', 'an', 'and', 'as', 'at', 'but', 'for',
            'if', 'in', 'not', 'of', 'or', 'so', 'the', 'then'
        ];

        // search for password pieces in username - ignore trivials
        foreach($words as $word)
        {
            if(\in_array($word, $trivial))
            {
                continue;
            }

            if(\strpos($userName, $word) !== false)
            {
                $valid = false;
                break;
            }
        }
    }

    if( ! $valid)
    {
        $this->error = lang('Auth.errorPasswordPersonal');
        $this->suggestion = lang('Auth.suggestPasswordPersonal');
    }

    return $valid;
}

[lonnieezell]

@dafriend I think you're right that it should be a separate validator. I think what you've shown here is a pretty good solution to it, too. Now - if we can add ad-hoc fields that would be awesome :)

I would think it would be too difficult. A config file holds the column names that should be checked against ( in addition to the ones you've already got). Loop over them and check their value against the password.

[dafriend]

I'm making good progress on this. Integrating ad-hoc fields proved to be pretty easy.

During testing, I came across some situations that may or may not be edge cases. For instance, $userName = 'captainjoe' and $password = 'joecaptain' should probably not be acceptable, but this is really hard to catch. It's the lack of a word boundary that makes it difficult and I don't have any way to parse them into individual words that is worth the processing effort. (The separate words case is already handled no matter the word order.)

One solution might be using similar_text() to compare $username and $password then rejecting the password if the similarity exceeds a certain percentage. If that's a good idea then adding a config item to set the point where rejection occurs seems logical and would likely be welcomed by end-users. Working values would be 1-100 and 0 to turn off this feature. I think 66 would be a good default value. (A value less the 30 would probably be madness.)

I've done some testing and it works pretty well. It even finds anagrams objectionable in most cases using a break-point of 66.

What do you guys think?