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

Add support for Laravel 8.x's Default Password Rules #3823

Merged
merged 5 commits into from Jun 16, 2021

Conversation

aj-adl
Copy link
Contributor

@aj-adl aj-adl commented Jun 10, 2021

An updated version of #3732 utilising the suggestions from @jasonvarga

Props to @rrelmy for much of the code / work on this one, I believe with time #3732 would have got there I just have a client waiting for this so I'm impatient!

Really the only thing I'm unsure of is if it was the right move to make the PasswordDefaults class - possibly that function could have lived off something else, but this keeps it explicit, it seems like it's in an appropriate namespace etc

Most users will need to upgrade their underlying Laravel framework version to use this - 8.43.0 only dropped 2 weeks ago.

  • Opens up support for utilising Laravel's Password::defaults() function
  • Checks for the minimum framework version (8.43.0)
  • Behaviour is exactly the same for users on older versions
  • Behaviour is exactly the same for users who do not utilise this feature

Closes statamic/ideas#422

An update version of statamic#3732 utilising the suggestions from @jasonvarga

Props to @rrelmy for much of the code / work on this one.

- Opens up support for utilising Laravel's Password::defaults() function
- Checks for the minimum framework version (8.43.0)
- Behaviour is exactly the same for users on older versions
- Behaviour is exactly the same for users who do not utilise this feature
@rrelmy
Copy link
Contributor

rrelmy commented Jun 10, 2021

Thanks Alex, it was on my list but I didn't had time yet.

Before not at all places min:8 was used, so this PR changes the default behaviour on setting passwords.
This should be checked if ok.

Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

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

There are 2 test failures in RegisterFormTest.

You can just change the passwords to longer ones.

image

@jasonvarga
Copy link
Member

By the way, this great.

And to answer @rrelmy I think having the default min:8 password in the places that it wasn't before is fine. 👍

@aj-adl
Copy link
Contributor Author

aj-adl commented Jun 11, 2021

Thanks @jasonvarga and @rrelmy

I didn't consider that some functions didn't enforce the min:8 before, but to me this seems like a reasonable change - having one set of rules for a password when creating a user, and then another when updating a password, seems like an oversight rather than intended behaviour.

In my mind password policy should be sitewide in all contexts, but I do get that it's not 100% backwards compatible.

I'll have a go at a docs PR to explain this change, as I think communicating with the users is key. Also considering a couple comments in the respective classes so that if people are spelunking around the code trying to find where to change the password validation, we can point them to the right direction.

@michaelr0
Copy link
Contributor

michaelr0 commented Jun 11, 2021

I started to explore doing something like this, Is it worth adding a config file for the developer/manager of the code, with the ability to set password options and then have a Statamic provider register those options as the default password settings?

@aj-adl
Copy link
Contributor Author

aj-adl commented Jun 11, 2021

@michaelr0 feedback on #3732 was that the Statamic team would rather use the Laravel functionality than make it a config option.

I understand 'both' is a valid choice, ie: if config is set Statamic could utilise it within it's AuthServiceProvider's boot method to set it - it would be a minor change code wise to make it happen (played around with it myself and got it working) but it's more a strategic decision - should this be something Statamic concerns itself with or should it be a 'use the framework' kind of thing.

Personally I feel that with some additions to the docs this PR would be fine as is, but happy for direction from the CMS team.

@michaelr0
Copy link
Contributor

michaelr0 commented Jun 11, 2021

@michaelr0 feedback on #3732 was that the Statamic team would rather use the Laravel functionality than make it a config option.

I understand 'both' is a valid choice, ie: if config is set Statamic could utilise it within it's AuthServiceProvider's boot method to set it - it would be a minor change code wise to make it happen (played around with it myself and got it working) but it's more a strategic decision - should this be something Statamic concerns itself with or should it be a 'use the framework' kind of thing.

Personally I feel that with some additions to the docs this PR would be fine as is, but happy for direction from the CMS team.

It could be plausible to do something like the following.
(Untested code)

        // example parameters from: Illuminate\Validation\Rules\Password
        // In config
        // commented out by default?
        // 'passwords' => [

        //     /**
        //      * The minimum size of the password.
        //      *
        //      * @var int
        //      */
        //     // 'min' => 8,

        //     /**
        //      * If the password requires at least one uppercase and one lowercase letter.
        //      *
        //      * @var bool
        //      */
        //     // 'mixedCase' => false,

        //     /**
        //      * If the password requires at least one letter.
        //      *
        //      * @var bool
        //      */
        //     // 'letters' => false,

        //     /**
        //      * If the password requires at least one number.
        //      *
        //      * @var bool
        //      */
        //     // 'numbers' => false,

        //     /**
        //      * If the password requires at least one symbol.
        //      *
        //      * @var bool
        //      */
        //     // 'symbols' => false,

        //     /**
        //      * If the password should has not been compromised in data leaks.
        //      *
        //      * @var bool
        //      */
        //     // 'uncompromised' => false,

        //     /**
        //      * The number of times a password can appear in data leaks before being consider compromised.
        //      *
        //      * @var int
        //      */
        //     // 'compromisedThreshold' => 0,
        // ];

        // In Provider
        if (! empty(config('statamic.auth.passwords'))) {
            Password::defaults(function () {
                $passwordDefaults = Password::default();

                foreach (config('statamic.auth.passwords') as $method => $value) {
                    if (method_exists($passwordDefaults, $method)) {
                        if (in_array($method, ['mixedCase', 'letters', 'numbers', 'symbols', 'uncompromised']) && $value === true) {
                            $passwordDefaults->$method();
                        } else {
                            $passwordDefaults->$method($value);
                        }
                    }
                }

                return $passwordDefaults;
            });

That would use Laravel's defaults, but give the ability in the config (by uncommenting) to say, hey I want passwords to be mixed case, so I'll uncomment mixedCase, The default password validation would then be Laravel + mixed case.

@aj-adl
Copy link
Contributor Author

aj-adl commented Jun 11, 2021

Yep pretty much exactly what I'd played around with locally, though you'd need to make some edits to the code to make sure the various options are array keys, not vars (easy to do when pseudocoding though!)

Like I said, down for input by @jasonvarga and co on this one, pretty trivial to implement. In many cases statamic is used alongside Laravel, so changing Laravels' defaults is sort of a big deal, but I do get that it makes it nice and easy to use.

If we were going to go down the config route, I'm not sure the 'fixed array' or params is the right way to go, you loose a lot by making it an array vs a function, as you can't check for context , do dynamic rules etc To me it feels wrong defining an anonymous function as a config param, so that leads me back to the conclusion of 'do it the laravel way', but again, that's just my 'vibe' would rather align with the overall CMS aesthetic / approach here

@michaelr0
Copy link
Contributor

Yep pretty much exactly what I'd played around with locally, though you'd need to make some edits to the code to make sure the various options are array keys, not vars (easy to do when pseudocoding though!)

Like I said, down for input by @jasonvarga and co on this one, pretty trivial to implement. In many cases statamic is used alongside Laravel, so changing Laravels' defaults is sort of a big deal, but I do get that it makes it nice and easy to use.

If we were going to go down the config route, I'm not sure the 'fixed array' or params is the right way to go, you loose a lot by making it an array vs a function, as you can't check for context , do dynamic rules etc To me it feels wrong defining an anonymous function as a config param, so that leads me back to the conclusion of 'do it the laravel way', but again, that's just my 'vibe' would rather align with the overall CMS aesthetic / approach here

Good catch, yeah that was a copy paste from the Password class, I have updated my example to a keyed array.

@jasonvarga
Copy link
Member

Good discussion, but I still feel like the solution provided by Laravel is already so simple, we don't need to reinvent any wheels. If anything I would say it would complicate it further.

It's a couple of lines to copy paste from somewhere in the docs.

@aj-adl If you make those 2 line changes I requested, I'll merge this. I would do it myself but I don't have permission to edit your fork.

@aj-adl
Copy link
Contributor Author

aj-adl commented Jun 11, 2021

All done, cheers!

@jasonvarga
Copy link
Member

Sorry one more thing. Just move the namespace up a line.

See https://github.styleci.io/analyses/6459yl

src/Console/Commands/MakeUser.php Outdated Show resolved Hide resolved
@jasonvarga jasonvarga merged commit 73bbd9f into statamic:3.1 Jun 16, 2021
@jasonvarga
Copy link
Member

FYI I've added this to docs, since you said you were going to try adding it. You don't have to. https://statamic.dev/users#password-validation

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.

Ability to hook into password rules
4 participants