Password Validation improvements

[ziming]

WHY

BEFORE - What was wrong? What was happening before this PR?

Backpack\CRUD\app\Http\Controllers\Auth\RegisterController has a min password length of 6 Backpack\CRUD\app\Library\Auth\ResetsPasswords.php has a min password length of 8

NIST recommends a min password length of 8. laravel also use a min password length of 8 now by default

As an admin, we should also have stronger password requirements, like disallowing compromised passwords

https://laravel-news.com/password-validation-rule-object-in-laravel-8

AFTER - What is happening after this PR?

min password length to be 8 on RegisterController bump min laravel to 8.39 Use the uncompromised() rule.

HOW

How did you achieve that, in technical terms?

See the changes in the code

Is it a breaking change?

There might be a tiny chance the user cannot upgrade from laravel 8.26 to 8.39

the uncompromised rule also makes a network call and might be inconvenient in local development if you are on a plane testing registration and resetting password. It is also common to use easy to guess passwords during local development. However that would be so unlikely.

https://haveibeenpwned.com/API/v3#PwnedPasswords

How can we test the before & after?

Just use the register and reset password feature in backpack

Side Thoughts?

Instead of doing Password::min(8)->uncompromised() we might want to use Password::defaults() to use the user password rules they defined in AuthServiceProvider boot method if they defined it. The ->rules() u see in the code example below requires laravel 8.87 however. While Password::defaults() exist since laravel 8.42

https://laravel.com/docs/8.x/validation#defining-default-password-rules

https://github.com/laravel/framework/commits/8.x/src/Illuminate/Validation/Rules/Password.php

// AuthServiceProvider
public function boot(): void
{
        Password::defaults(function () {
            $rule = Password::min(8);

            return $this->app->isProduction()
                ? $rule
                    ->uncompromised()
                : $rule;
        });
}

Then in our validation rules. (Note that the ->rules() method is only added in 8.78)

[
    'required', 
    'confirmed',
    'min:8',
    Password::defaults()->rules([ // The extra ->rules() method is added in laravel 8.78
        'additional-rules-we-might-want-to-enforce-for-the-password-field-like-uncompromised'
        'or it might be a bad idea',
    ]),
]

[ziming]

After giving it some thoughts I decide to change the rules to

['required', 'confirmed', 'min:8', Password::defaults()]

I also bumped the min laravel to 8.78 even though Password::defaults() is added in 8.42

Reason for that is so that people can use the ->rules() method too like the example below which is added in 8.78. But if you want to be safe bumping to just 8.42 minimum is good enough too

// AuthServiceProvider

use Illuminate\Validation\Rules\Password;
use Ziming\LaravelZxcvbn\Rules\ZxcvbnRule;

public function boot(): void
{
    Password::defaults(function () {
            $rule = Password::min(8);

            return $this->app->isProduction()
                ? $rule
                    ->uncompromised()
                    ->rules([
                       new ZxcvbnRule([
                           request('email'),
                           request('name'),
                      ]),
                   ])
                : $rule;
        });
}

[tabacitu]

I agree with this, but it's kind of a breaking change, so we can only do it in v6, I'm afraid (end-of-year). I'll move it there.

Until then, I'll ask @pxpm to double-check this.

Thanks @ziming !

[ziming]

could it be included in this version of backpack if we do some if check on laravel/framework version?

see:

https://getcomposer.org/doc/07-runtime.md#knowing-the-version-of-package-x

[pxpm]

@ziming yes, we can add checks for Laravel version and get this. I am not much in favor of version constrains in code unless is something that completely breaks the app if not done.

I am also holding to bump laravel version to fix spatie/laravel-tags conflict.

I 100% agree with this, but not sure we should add version constrains in code for this scenario at this time.

[ziming]

laravel 8.75 has a security patch for blade. would that be enough incentive to bump the minimum?

https://github.com/laravel/framework/security/advisories/GHSA-66hf-2p6w-jqfw (patched in 8.75)

https://github.com/laravel/framework/security/advisories/GHSA-4mg9-vhxq-vm7j (patched in 8.40)

if not. it's fine. :)

[pxpm]

laravel 8.75 has a security patch for blade. would that be enough incentive to bump the minimum?

GHSA-66hf-2p6w-jqfw (patched in 8.75)

GHSA-4mg9-vhxq-vm7j (patched in 8.40)

if not. it's fine. :)

For me it is a good reason indeed. I will discuss this with @tabacitu .

Thanks @ziming

[tabacitu]

Totally. But let's do that as a separate PR please - that's a non-breaking change, while this PR is not.

[tabacitu]

About the PR here: I don't think this change (stronger password) is worth polluting the code and documentation with conditionals. It's a breaking change, we treat it as such and push it on the next major version.

This also forces you to ask yourself: is this important enough to launch a new version right away? And the answer is no, no it's not 😀 Kinda neat, right?

[ziming]

I'm a little confused here. If the minimum laravel is agreed to be bumped, then conditionals are not necessary anymore.

But okay, fine with next major version :)

[pxpm]

It will be a breaking change since we will be changing the password length.

If we bump the version and only account for the password rule it will be non-breaking I guess.

[tabacitu]

If we bump the version and only account for the password rule it will be non-breaking I guess.

Pedro expressed this much better than I could, sorry 😀

[ziming]

Hmm I don't really see it as a breaking change since existing users won't feel it (since their accounts are already created) while new users who register will never know the password min length used to be 6 and is now 8

But alright I changed the min password length for RegisterController back to 6. 👍🏻

[tabacitu]

Thank you @ziming ! @promatik could you also please review this?

[promatik]

Hey @ziming! Thank you for the PR, in my opinion this is a MUST 👌

In the between, I added a commit because there was a PHP error, Password class was being imported twice, from different sources. So I renamed the new one, here: https://github.com/Laravel-Backpack/CRUD/pull/4442/commits/2c71a479be2993934abdb3414422de70471ff970

[promatik]

@tabacitu, idea;

Should backpack help on it? Maybe it would be a nice thing to have it on configs.

configs/backpack/base.php

'password_validation_rules' => [
    'required',
    'min:8',
    // 'uncompromised',
    // 'mixedCase',
    // Password::defaults(),
],

If devs uncommented the rules (if array length > 0) we would automatically assign this rules to the password defaults. (prod env only) They would be able to do it them self anyway.

Making it easy for devs to secure their backends is a super nice thing to have, I think 🤷‍♂️

[tabacitu]

Let's do this:

• [ ] let's add a new config option to base.php like Antonio suggested here; but for ALL env, not only prod; notice the Password::defaults() are commented out; that's in order for this to be a non-breaking change in 100% use cases;
• [x] let's fix the inconsistency and have both files use min:8 (that will get fixed automatically when we use the config above);

That way, we have the best of both worlds:

• if the dev wants to customize the password rules for the admin only, they can, by changing the config;
• if the dev wants to customize the password rules for both admin and user, they can, by changing the config;
• if the dev wants to customize the password rules for a certain env, they can, by changing Password::defaults();
• if the dev wants nothing, they get nothing 😀 by default;

[ziming]

Hi,

I added the password_validation_rules config to base.php under Authentication Section

in RegisterController and ResetsPassword I changed to use ...config('backpack.base. password_validation_rules')

This is PHP 7.4 syntax (array spread operator) but if you all want to support the EOL PHP 7.3 then feel free to change to use array_merge(). I think it is a good idea to specify in composer.json so contributors know more explicitly which is the minimum PHP version we should support in our PRs

I did not test this but 1 thing I'm wondering is, can you use request() inside config files? Since some people might want to do Zxcvbn or other kinds of password validation that takes into account the value of other inputs like email or name to ensure the password is not too similar to it.

Another thing to note is not every method in the Password class is static

Only min(), required(), sometimes(), defaults(), default() are static, the rest need to be chained after 1 of these methods

Hence why I did not follow @promatik array example exactly as there is no Password::uncompromised()

https://github.com/laravel/framework/blob/8.x/src/Illuminate/Validation/Rules/Password.php

Another part of me is thinking if it is better if config('backpack.base.password_validation_rules') is not an array but this instead:

'password_validation_rules' => Password::min(8) // ->uncompromised() // ->mixedCase() // ->numbers() // ->symbols() // ->defaults() ->rules([ // 'Your Zxcvbn or whatever extra rules', or they can define in the defaults() method in AppServiceProvider ]),

Thoughts?

[ziming]

I just thought of an even better approach. Compatible with PHP 7.3 and more flexible too. Will make my new commit when i reach home

[ziming]

Okay pushed, it's 2+ am here now.

Here is an explanation of how it works

1st change: config('backpack.base.password_validation_rules') is no longer an array

This is because most password rules on the Password class are not static methods. They need to be chained after min(), required(), sometimes(). If people still need to tag on new rules, they can do something like Password::min(8)->rules([])

2nd change: Creation of a Backpack class to configure the backpack default password rules.

If we look at official laravel packages like Telescope, Horizon, Passport. you would see that configurations are not only done in the config files. But also through static methods you can call on Telescope class, Horizon class and Passport class in your own ServiceProviders boot() method.

https://laravel.com/docs/9.x/passport#main-content

https://laravel.com/docs/9.x/horizon#main-content

https://laravel.com/docs/9.x/telescope#main-content

So if a backpack user want to do more granular customizations, she can do something like this

class AuthServiceProvider
{
    public function boot()
    {

        // Laravel runs the the core and packages service providers first before this

// define Password::defaults() here 1st maybe.

        // Backpack::passwordRulesDefaults(
              // Password::min(8)
              //    ->uncompromised()
              //    ->when($this->app->isProduction()
        // );

        // Backpack::passwordRulesDefaults(Password::defaults());

        Backpack::passwordRulesDefaults(function () { 
            $rule = Password::min(8);

            return $this->app->isProduction()
                ? $rule
                    ->uncompromised()
                : $rule;
        });
    }
}

But if the user do not define Backpack::passwordRulesDefaults() in their own service provider, then backpack loads the one from config, as seen in BackpackServiceProvider boot() method. The code and docblock in passwordRulesDefaults() and passwordRulesDefault() are pretty much the same as Laravel Password::defaults() and Password::default() with some slight modifications.

I think in the future more things could be move to this new Backpack class too, such as license check (see https://github.com/spatie/laravel-medialibrary/blob/main/src/Support/MediaLibraryPro.php) or other kinds of configurations that need to be more flexible than just being in a config file as can be seen here

I think my pr is done now. feel free to edit as needed if I make a mistake somewhere or to make things simpler

Having it be a static array in Backpack.php class may be simpler actually for setting and getting the password rules. But it also make it less similar to Laravel own default password rules implementation. I can't decide. So I leave it to you all to decide to make it array or Password object.

[tabacitu]

Sorry guys, but I have to shut this down.

@ziming , very sorry for all the time you spent on this, but... I think we should stop searching for a solution for this. This current implementation is by far the cleanest (great job 👏👏👏) but it introduces a new level of configuration, that

• is unlike anything we have currently, so it won't be intuitive;
• is not scalable, since every new configuration at the boot level will require us to have a new call in our ServiceProvider;

We might introduce something like that in the future (good idea), but not as a side-effect of this minor problem we're solving here. We don't make architecture changes to solve minor bugs... that's a sure way to get to bloat. IF that solution has value, we should talk about it in a different thread, think cost vs benefits, and do then maybe do it. With this fix being a sideeffect. But not the other way around.

In the context of:

• Authentication most likely becoming a separate package in v6 or v14;
• This being a COULD DO (not a SHOULD, not a MUST);
• Us having spent hours and hours on this already;
• All solutions we found so far being either too intrusive or overengineered;
• The benefit for this feature being minimal, for very very few Backpack devs;

I have to put an end to this, sorry. Don't get me wrong. I'm sympathetic to those who want to customize their password rules. We can create a tutorial for them on how to do that, by extending the classes - please do create that tutorial. And create a separate PR that makes both validations min:8. But going through all this trouble... for such a small feature that so few developers need... this is not worth it, sorry. Let's KISS.

I'm sorry I haven't seen this before, or said anything of this nature before, and have let you spend all this time on it. I should have put a stop to this earlier, or tell you in clear words - "be careful, this is not KISS, it looks like a slippery slope and we've already spent too much time on this". That's my failure, as the decision-maker here, to warn you about that.... I'm sorry, I know this must be frustrating. In the future, let's follow a new rule: If it's a COULD-DO... and it takes more than 1-2h to do... we don't do it. It becomes a WON'T DO. That should stop us from working on problems that aren't worth fixing. Again, I'm sorry for this mistake of mine, for letting you guys work on it for so long just to result in a "won't merge" 🙏

Let's move our focus to more important issues, please 🙏 Keep in mind that... our time is limited. Working on this prevents us from working on something more important.

Feel free to make your case if you think I'm wrong - I often am. Happy to reevaluate its importance. But the way I see it, this is a COULD, and we've already spent too much time on this. I'd rather we quit working on this and solve actual problems, that actual users face. The priority is fixing problems that >80% developers face, not 1%. Those are the problems that are worth our time.

Sorry and thank you 🙏