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

Allow configuration of additional password validations #3732

Closed
wants to merge 2 commits into from

Conversation

rrelmy
Copy link
Contributor

@rrelmy rrelmy commented May 17, 2021

This is a basic implementation of custom validation rules for user passwords discussed in statamic/ideas#422
We need this to satisfy a government customer requiring more strict password rules

I replaced all occurrences of required|confirmed on password validations, please check if all three are ok.

It allows to add any validation to the optional config parameter

'password_validation' => ['min:8'],

I did my best writing a description matching the pattern, feel free to give feedback on naming.


Edit: I additionally added the password validation to the MakeUser command, before the password was not validated at all. An empty password could be entered (did not work to login!)

Copy link
Member

@duncanmcclean duncanmcclean left a comment

Choose a reason for hiding this comment

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

Hope you don't mind @rrelmy but I noticed a couple of little things...

But it's a really nice implementation though! Can definitely see it coming in useful 😄

|
*/

'password_validation' => [],
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it might be worth adding in the default password validation rules here so the user knows what they are extending upon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to do that because removing them could be catastrophically.
But I am open to suggestions :-)

Also the confirmed is not always needed


And adding something like min:6 what would make sense IMHO but could be a breaking change

Copy link
Member

Choose a reason for hiding this comment

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

I didn't want to do that because removing them could be catastrophically.

What do you mean, as in, stuff would fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not test if but removing confirmed could lead to user input errors and unknown passwords.
Removing required could lead to empty password being accepted I suppose

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's a good shout. I guess that's probably something for the Core Team to decide on...

@@ -75,7 +75,7 @@ protected function rules()
return [
'token' => 'required',
'email' => 'required|email',
'password' => 'required|confirmed|min:8',
'password' => array_merge(['required', 'confirmed'], config('statamic.users.password_validation', [])),
Copy link
Member

Choose a reason for hiding this comment

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

If you go down the route of adding the default password rules in the config, it means you'll no longer need the array_merge here anymore (or in the other places you're doing it).

You'll probably also want to fallback to the default rules, in case the user doesn't add the option to their config file.

'password' => config('statamic.users.password_validation', ['required', 'confirmed']),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as in the first comment, as confirmed is not be needed everywhere (see MakeUser command) I see little benefit in it.

IMHO it is safer and cleaner to have the hardcoded rules and adding additional rules.

I would rather drop support for PHP < 7.4 and be able to use 😂

['required', 'confirmed', ...config('statamic.users.password_validation')]

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I see where you're coming from... maybe it can just stay how it is 😎

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.

Laravel recently introduced default password rules.

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

I think it might be a good idea to leverage that.

Since it's only on Laravel 8, I think that's fine, and we can just fall back to the existing required|confirmed|min:8 if using <8.

@rrelmy
Copy link
Contributor Author

rrelmy commented May 23, 2021

I have to look into how such an implementation would look like.
@jasonvarga Do you have an example how you already leverage a feature depending on the laravel version?

@jasonvarga
Copy link
Member

jasonvarga commented May 24, 2021

Here's an example where we check if Laravel is 7+.

return version_compare(app()->version(), 7, '>=')

@rrelmy
Copy link
Contributor Author

rrelmy commented May 31, 2021

@jasonvarga With a fallback you mean something like:

public static function getPasswordRules() {
  if (version_compare(app()->version(), 8, '<') ) {
    // fallback to the minimum on laravel < 8
    return 'min:8';
  }

  return Password::defaults();
}

// ...

'password' => ['required', Something::getPasswordRules()],

pseudocode, not test

And on Laravel 8+ one can use the existing rules
https://laravel.com/docs/8.x/validation#defining-default-password-rules

@jasonvarga
Copy link
Member

That's right. If you are using an older Laravel version, you are stuck with whatever password rules we have been using up until now.

aj-adl added a commit to framecreative/cms-1 that referenced this pull request Jun 10, 2021
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 Author

rrelmy commented Jun 10, 2021

Closing because of #3823

@rrelmy rrelmy closed this Jun 10, 2021
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

3 participants