Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Added ability to define extra default password rules #40137

Merged
merged 3 commits into from
Dec 22, 2021

Conversation

ash-jc-allen
Copy link
Contributor

Hi! This PR is only fairly simple but I think could be really useful. It adds the functionality for you to define your own custom validation rules that should be run when using the Password::defaults() validation rule.

As far as I could see, this functionality doesn't already exist; but if it does, apologies.

To give a bit of context, I have an app that has 5 different places where users can be created and updated. At the moment, I'm using the password defaults, but also want to add a rule to make sure that every password passes a zxcvbn validation check. So my form request rules look similar to this:

public function rules()
{
    return  [
        // ...
       'password' => ['required', Password::defaults(), new ZxcvbnRule()],
        // ...
    ];
}

By default, I always want my passwords to pass the ZxcvbnRule validation check, so I have this in every place where I'm using Password::defaults(). So, in a way, I'm also treating that rule as a default one too.

So, with my proposed change, I can do something like this to define some extra default password rules:

Password::defaults(function () {
    return Password::min(8)
        ->symbols()
        ->mixedCase()
        ->uncompromised()
        ->rules(new ZxcvbnRule());
});

This means that I can then clean up my form request rules and be confident that the ZxcvbnRule() will always be run wherever the password defaults are used. It could look more like this:

public function rules()
{
    return  [
        // ...
       'password' => ['required', Password::defaults()],
        // ...
    ];
}

The new ->rules() method that I've added supports rule objects and closures and they can be passed in individually or as an array like so:

Password::defaults(function () {
    return Password::min(8)
        ->rules(new ZxcvbnRule());
});
Password::defaults(function () {
    return Password::min(8)
        ->rules([new ZxcvbnRule(), new AnotherRule()]);
});
Password::defaults(function () {
    return Password::min(8)
        ->rules(function ($attribute, $value, $fail) {
            if ($value !== 'foo') {
                $fail('Password is not foo');
            }
        });
});

As far as I can see, this appears to be working. If it's something that you might consider merging, please let me know if you need any changes making 😄

@taylorotwell taylorotwell merged commit 32fbf39 into laravel:8.x Dec 22, 2021
@ash-jc-allen ash-jc-allen deleted the custom-default-rule branch December 22, 2021 20:23
@yusufonur
Copy link

I think very good feature, thanks @ash-jc-allen

@onlime
Copy link
Contributor

onlime commented Jan 6, 2022

Thanks @ash-jc-allen for this great addition!

I only find the order where the customRules are applied rather confusing and in the case of ZxcvbnRule not really what I would expect. The custom rules are merged right at the beginning and are evaluated before any mixedCase, letters, symbols, numbers, uncompromised conditions.

            [$attribute => array_merge(['string', 'min:'.$this->min], $this->customRules)],

Could this be improved by moving the custom rules to the bottom of the stack? I find this more intuitive and a custom rule usually is more complex to evaluate, so it should come after the simple built-in ones.

@AshishDhamalaAD
Copy link

This is really useful. Thanks for contributing. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants