Skip to content

Conversation

@daliendev
Copy link

Minor update of Str::password() method

The aim is to empower end users with the capability to define password generation rules. These rules allow to specify whether the generated password should include letters, numbers, symbols, or spaces by passing an array of such rules.

Although users can already instruct the generator to include these elements, there's no guarantee that the generated password will adhere to the validation rules set for the web app. This update addresses this concern by enabling alignment between the generator and the web app's password validation rules.

Existing logic has been preserved, incorporating specific modifications to introduce rules-based restrictions.
Old usage remains unaffected by this update.

@dennisprudlo
Copy link
Contributor

dennisprudlo commented Aug 23, 2023

There are some cases I'd like to point out which we should discuss:

// This generates a string with a length of 2, although the length was set to 1. What argument should be prioritized?
Str::password(1, true, true, true, true, ['letters', 'numbers']); // "q3"

// This generates a string with a letter, because the `$rules` argument enforces letters. However the `$letters` argument is set to false. Again, should the `$rules` argument be the prioritized?
Str::password(1, false, true, true, true, ['letters']); // "d"

Then, I don't know if it's an issue on my end, but when sorting the $password_chars The enforced letter is always somewhere at the end of the password. The best I could get was the letter being the second to last character. So maybe a good 'ol shuffle() on the collection would be better:

-usort($password_chars, fn() => random_int(-1, 1));
-return collect($password_chars)->implode('');
+return collect($password_chars)->shuffle()->implode('');

I got better results with that.

And I would change the casing of the variables from $snake_case to $camelCase since laravel uses this casing internally.

@daliendev
Copy link
Author

Indeed, I started with the assumption that the $rules enforce the presence of specific characters. However, you're right to bring this up.
Without a refactoring on params which will introduce a breaking change I don't see how to avoid this kind of confusion.

As a clearer alternative @param array $rules could be replaced by a @param bool $enforce based on previous passed params. The logic will be a bit different but it can be a good solution.

Regarding the shuffle, I'll conduct some tests on my end as well, I hesitated on this point too.

Thanks also for the reminder about camelCase, I'll take care of it in a reworked V2.
As for now I draft this pull request.

@daliendev daliendev marked this pull request as draft August 23, 2023 09:59
@daliendev
Copy link
Author

[10.x] Enhance password generation constraints using enforce parameter

In comparison to the previous commit, logic was reworked with a boolean variable $enforce.

public static function password($length = 32, $letters = true, $numbers = true, $symbols = true, $spaces = false, $enforce = false) { ... }

This allows to use the parameters already existing in the current version of the Laravel \Illuminate\Support\Str::password() method, which is also offering greater clarity and minimizes potential misunderstandings.

Additionally, the shuffle is now directly made on the collection as mentioned before: it is more efficient.

snake_case was replaced by camelCase for consistency with Laravel codebase.

@daliendev daliendev marked this pull request as ready for review August 24, 2023 21:02
@taylorotwell
Copy link
Member

I really think if someone needs to customize this method further they should just write their own password method for their application.

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.

3 participants