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] Adds password rule #36960

Merged
merged 17 commits into from Apr 23, 2021
Merged

[8.x] Adds password rule #36960

merged 17 commits into from Apr 23, 2021

Conversation

nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Apr 12, 2021

This pull request adds a new custom rule object to the framework that allows to replace existing password rules like so:

    public function store(Request $request)
    {
        $request->validate([
            'name' => 'required|string|max:255',
            'email' => 'required|string|email|max:255|unique:users',
        //  'password' => 'required|string|confirmed|min:8',
            'password' => ['required', 'confirmed', Password::min(8)],
        ]);

In addition, this new custom rule object contains built-in methods that you may use to ensure strong passwords:

        $request->validate([

            // Makes the password require at least one uppercase and one lowercase letter.
            'password' =>  ['required', 'confirmed', Password::min(8)->mixedCase()],

             // Makes the password require at least one letter.
            'password' =>  ['required', 'confirmed', Password::min(8)->letters()],

            // Makes the password require at least one number.
            'password' =>  ['required', 'confirmed', Password::min(8)->numbers()],

            // Makes the password require at least one symbol.
            'password' =>  ['required', 'confirmed', Password::min(8)->symbols()],

            // Ensures the password has not been compromised in data leaks.
            'password' =>  ['required', 'confirmed', Password::min(8)->uncompromised()],
        ]);

Of course, you can always use them all combined like so:

    public function store(Request $request)
    {
        $request->validate([
            'name' => 'required|string|max:255',
            'email' => 'required|string|email|max:255|unique:users',
            'password' => ['required', 'confirmed', Password::min(8)
                    ->mixedCase()
                    ->letters()
                    ->numbers()
                    ->symbols()
                    ->uncompromised(),
            ],
        ]);

Here is an example of the output:

EyycZD-XAAMs2Nm

@gocanto
Copy link
Contributor

gocanto commented Apr 13, 2021

It would also be convenient if we could do something like:

$food->ensureNotCompromisedUsing(function () => {})

then, we could use custom verifiers for it.

@nunomaduro
Copy link
Member Author

@gocanto You can already provide your own implementation here:

$this->app->singleton('validation.not_compromised', function ($app) {
.

@taylorotwell taylorotwell marked this pull request as ready for review April 13, 2021 12:57
@chris-ware
Copy link
Contributor

Some interesting reading that may want to be considered as possible additions to the mechanics presented here: https://pages.nist.gov/800-63-3/sp800-63b.html#sec5

These suggestions are being utilised in the following package: https://github.com/langleyfoxall/laravel-nist-password-rules, which doesn't fully have Laravel 8 support, but some elements of this PR render elements of the package obsolete.

@taylorotwell
Copy link
Member

taylorotwell commented Apr 15, 2021

@nunomaduro what about localization of these error messages? Should we be wrapping them in __()? Should keys be used that exist in the validation language file?

@JosephSilber
Copy link
Member

Besides ensureNotCompromised, what's password-specific about this?

Wouldn't it make more sense to just have a general way of chaining rules like that?

@lk77
Copy link

lk77 commented Apr 19, 2021

Hello,

i think you could add a rule to ensure that any character is not repeated more than X times,
in a lot of banking apps you can only have the same letter/number twice

@taylorotwell
Copy link
Member

Removed required and confirmed requirements...

Adjusted API slightly:

Password::min(8)->letters()->numbers()->uncompromised();

@nunomaduro nunomaduro marked this pull request as draft April 20, 2021 14:17
@JosephSilber
Copy link
Member

BTW, this thread wouldn't feel complete without the obligatory XKCD comic:

9B3A665A-14B7-4554-90B6-E671725C956D

@nunomaduro nunomaduro changed the title [8.x] [WIP] Adds password rule [8.x] Adds password rule Apr 21, 2021
@nunomaduro
Copy link
Member Author

nunomaduro commented Apr 21, 2021

  1. @taylorotwell Wondering if we should add "required" and "confirmed" methods so people can add password rule like so? Personally, I prefer not mixing string rules with object rules.
        $request->validate([
            'name' => 'required|string|max:255',
            'email' => 'required|string|email|max:255|unique:users',
        //  'password' => ['required', 'confirmed', Password::min(8)->mixedCase()->uncompromised()],
            'password' =>  Password::min(8)->required()->confirmed()->mixedCase()->uncompromised(),

  1. Also, any thoughts on the fact that we already have a "password" string rule? May this be somehow confusing? https://github.com/laravel/docs/blob/8.x/validation.md#password. Alternatively, we could equally use the "static factory pattern" here an have the same object creating two different rules:
        $request->validate([
            // Creates a normal password rule:
            'password' =>  Password::min(8)->required()->confirmed()->mixedCase()->uncompromised(),

            // Creates an authenticated guard password rule:
            'password' =>  Password::guard('api'),

Copy link
Contributor

@claudiodekker claudiodekker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me apart from a few minor things 👍

src/Illuminate/Validation/Rules/Password.php Outdated Show resolved Hide resolved
src/Illuminate/Validation/NotPwnedVerifier.php Outdated Show resolved Hide resolved
src/Illuminate/Validation/Rules/Password.php Outdated Show resolved Hide resolved
src/Illuminate/Validation/Rules/Password.php Outdated Show resolved Hide resolved
src/Illuminate/Validation/Rules/Password.php Outdated Show resolved Hide resolved
src/Illuminate/Validation/Rules/Password.php Outdated Show resolved Hide resolved
@lloricode
Copy link
Contributor

Hi @nunomaduro @taylorotwell

since the uncompromised uses external API,
is the a way to mock this when in unit testing?

@maciek-szn
Copy link

I would really like to know how to localize/create custom error messages for this. There is no info in the docs and I can't find any hint in the code.

@mateusjunges
Copy link
Contributor

mateusjunges commented Apr 27, 2021

@diverpl You can use the resources/lang/<your-locale>.json file to translate the messages.

For example, if your locale is pt-br, than create a new resources/lang/pt-br.json file in your project, and add these content to that file:

{
    "The :attribute must contain at least one letter.": "O campo :attribute deve conter pelo menos uma letra",
    "The :attribute must contain at least one number.": "O campo :attribute deve conter pelo menos um número.",
    "The :attribute must contain at least one symbol.": "O campo :attribute deve conter pelo menos um símbolo.",
    "The :attribute must contain at least one uppercase and one lowercase letter.": "O campo :attribute deve conter pelo menos uma letra maiúscula e uma minúscula.",
    "The given :attribute has appeared in a data leak. Please choose a different :attribute.": "O :attribute informado apareceu em um vazamento de dados. Por favor escolha uma :attribute diferente."
}

Note: I added the pt-br translations. You can do it with any locale you want to use.

@stefnats
Copy link

@lloricode

Hi @nunomaduro @taylorotwell

since the uncompromised uses external API,
is the a way to mock this when in unit testing?

Take a look at the file ValidationNotPwnedVerifierTest.php - it should show you how to mock it for tests.

@lloricode
Copy link
Contributor

@lloricode

Hi @nunomaduro @taylorotwell
since the uncompromised uses external API,
is the a way to mock this when in unit testing?

Take a look at the file ValidationNotPwnedVerifierTest.php - it should show you how to mock it for tests.

thanks will check now

@JosephSilber
Copy link
Member

JosephSilber commented Apr 28, 2021

Oh, and this thread wouldn't feel complete without this either:

passwords.mp4

bert-w pushed a commit to bert-w/framework that referenced this pull request Apr 29, 2021
* Adds password rule

* Typo

* Fixes default compromised number

* Adds "Add-Padding" header to not pwned verifier

* Improves testing

* work on rule

* Adds uncompromised threshold

* Updates docs

* Removes non used import

* Updates property name

* Fixes docs

* Updates test methods

* Adds more tests

* Removes mixed case test

* Adds more tests

* Adds tests

* Update NotPwnedVerifier.php

Co-authored-by: Taylor Otwell <taylorotwell@gmail.com>
Co-authored-by: Taylor Otwell <taylor@laravel.com>
@bebetoalves
Copy link

I would really like to know how to localize/create custom error messages for this. There is no info in the docs and I can't find any hint in the code.

Waiting for this too :)

@ghost
Copy link

ghost commented May 14, 2021

Hello, excuse my ignorance, but how do I customize the validation messages, that is, I want to pass them to Spanish, I know how to do it normally, but with this function I have not been able to do it, eye I do not want to touch the Password.php file. Thanks.

@ordago
Copy link

ordago commented May 16, 2021

@alex-gil-1981 you have to add the translation strings in the json file as mateusjunges explained here:

@diverpl You can use the resources/lang/<your-locale>.json file to translate the messages.

For example, if your locale is pt-br, than create a new resources/lang/pt-br.json file in your project, and add these content to that file:

{
    "The :attribute must contain at least one letter.": "O campo :attribute deve conter pelo menos uma letra",
    "The :attribute must contain at least one number.": "O campo :attribute deve conter pelo menos um número.",
    "The :attribute must contain at least one symbol.": "O campo :attribute deve conter pelo menos um símbolo.",
    "The :attribute must contain at least one uppercase and one lowercase letter.": "O campo :attribute deve conter pelo menos uma letra maiúscula e uma minúscula.",
    "The given :attribute has appeared in a data leak. Please choose a different :attribute.": "O :attribute informado apareceu em um vazamento de dados. Por favor escolha uma :attribute diferente."
}

Note: I added the pt-br translations. You can do it with any locale you want to use.

@Jxckaroo
Copy link

Hey, awesome feature! 😄

One question though... what is the use case for the threshold? As surely if a password appears once, it is compromised and increasing the threshold would be promoting bad practice?

@retosteffen
Copy link

This is awesome!
My two cents:
I'm using" use Illuminate\Support\Facades\Password;" to send reset links over an api and the fact that both are called "Password" isn't ideal in my opinion.
I know I can " use.... as ..."

But awesome feature!

@ghost
Copy link

ghost commented May 19, 2021

@ordago Excellent thank you very much!

@lorimay21
Copy link

lorimay21 commented Aug 10, 2021

@diverpl You can use the resources/lang/<your-locale>.json file to translate the messages.

For example, if your locale is pt-br, than create a new resources/lang/pt-br.json file in your project, and add these content to that file:

{
    "The :attribute must contain at least one letter.": "O campo :attribute deve conter pelo menos uma letra",
    "The :attribute must contain at least one number.": "O campo :attribute deve conter pelo menos um número.",
    "The :attribute must contain at least one symbol.": "O campo :attribute deve conter pelo menos um símbolo.",
    "The :attribute must contain at least one uppercase and one lowercase letter.": "O campo :attribute deve conter pelo menos uma letra maiúscula e uma minúscula.",
    "The given :attribute has appeared in a data leak. Please choose a different :attribute.": "O :attribute informado apareceu em um vazamento de dados. Por favor escolha uma :attribute diferente."
}

Note: I added the pt-br translations. You can do it with any locale you want to use.

@mateusjunges Thank you very much!!! This comment was very helpful~

@lorimay21
Copy link

I think it would be great to add an additional validation Password::max().
Some systems require passwords to be like 6-100 characters only.

But still, thanks a lot for this new feature!!

@andrey-helldar
Copy link
Contributor

@lorimay21, just use the laravel-lang/publisher package.

@kicker10BOG
Copy link

Is there a way to just require any 3 of the 4?

@georgefromohio
Copy link

Is there a way to just require any 3 of the 4?

Sending another vote for this, with the following suggestion of a magic method

Password::withAny(); // OR
Password::applyAny();

e.g.

'password' =>  ['required', 'confirmed', Password::withAny()->min(8)->mixedCase()->letters()],
'password' =>  ['required', 'confirmed', Password::withAnyTwo()->min(8)->mixedCase()->letters()->numbers()],
'password' =>  ['required', 'confirmed', Password::withAnyThree()->min(8)->mixedCase()->letters()->symbols()],

@Nimash68
Copy link

Hi there,
Is there a way to get a response as a string? I mean for example when we use the Password::min(8), we get it like 'min:8'?

@ivqonsanada
Copy link

ivqonsanada commented May 15, 2023

is there a way to change the uncompromised message?

        Password::defaults(function () {
            return Password::min(8)
                           ->uncompromised();
        });

        $rules = [
            'password' => ['required', Password::defaults()],
        ];

        $messages = [
            'password.uncompromised' => 'test 123',
        ];

        $request->validate($rules, $messages);

i tried that, but seems doesnt work

@begueradj
Copy link

Customizing the error messages of the built-in methods does not work the classic way.
The documentation does not cover this either.

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.

None yet