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.

Sign up for GitHub

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

Jump to bottom

[10.x] Fixes Str::password() does not always generate password with numbers #48681

Merged
merged 4 commits into from Oct 10, 2023
+34 −19
Merged

[10.x] Fixes Str::password() does not always generate password with numbers #48681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f82c0f
wip
crynobone Oct 10, 2023
97bb3c6
wip
crynobone Oct 10, 2023
bc3db22
formatting
taylorotwell Oct 10, 2023
d667fc1
Apply fixes from StyleCI
StyleCIBot Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 27 additions & 19 deletions src/Illuminate/Support/Str.php
View file
Open in desktop
Expand Up @@ -841,25 +841,33 @@ public static function pluralStudly($value, $count = 2)
*/
public static function password($length = 32, $letters = true, $numbers = true, $symbols = true, $spaces = false)
{
return (new Collection)
->when($letters, fn ($c) => $c->merge([
'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k',
'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G',
'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R',
'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
]))
->when($numbers, fn ($c) => $c->merge([
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
]))
->when($symbols, fn ($c) => $c->merge([
'~', '!', '#', '$', '%', '^', '&', '*', '(', ')', '-',
'_', '.', ',', '<', '>', '?', '/', '\\', '{', '}', '[',
']', '|', ':', ';',
]))
->when($spaces, fn ($c) => $c->merge([' ']))
->pipe(fn ($c) => Collection::times($length, fn () => $c[random_int(0, $c->count() - 1)]))
->implode('');
$password = new Collection();

$options = (new Collection([
'letters' => $letters === true ? [
'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k',
'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G',
'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R',
'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
] : null,
'numbers' => $numbers === true ? [
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
] : null,
'symbols' => $symbols === true ? [
'~', '!', '#', '$', '%', '^', '&', '*', '(', ')', '-',
'_', '.', ',', '<', '>', '?', '/', '\\', '{', '}', '[',
']', '|', ':', ';',
] : null,
'spaces' => $spaces === true ? [' '] : null,
Comment on lines +846 to +862
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be sufficient to determine if a value is true or not:

Suggested change
$options = (new Collection([
'letters' => $letters === true ? [
'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k',
'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G',
'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R',
'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
] : null,
'numbers' => $numbers === true ? [
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
] : null,
'symbols' => $symbols === true ? [
'~', '!', '#', '$', '%', '^', '&', '*', '(', ')', '-',
'_', '.', ',', '<', '>', '?', '/', '\\', '{', '}', '[',
']', '|', ':', ';',
] : null,
'spaces' => $spaces === true ? [' '] : null,
$options = (new Collection([
'letters' => $letters ? [
'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k',
'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G',
'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R',
'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
] : null,
'numbers' => $numbers ? [
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
] : null,
'symbols' => $symbols ? [
'~', '!', '#', '$', '%', '^', '&', '*', '(', ')', '-',
'_', '.', ',', '<', '>', '?', '/', '\\', '{', '}', '[',
']', '|', ':', ';',
] : null,
'spaces' => $spaces ? [' '] : null,

Since there is no native PHP type declaration, it is safer to treat all truthy values as true.

Copy link
Member Author

Choose a reason for hiding this comment

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

but the parameter isn't typehinted as bool

Copy link
Contributor

@mpyw mpyw Oct 10, 2023 •
edited

Choose a reason for hiding this comment

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

but the parameter isn't typehinted as bool

Yes, I rather think so, which is why === true should be omitted. For example, if you pass (int)1 here, I think it should be handled as true because it is a truthy value.

In a situation where either true or false is determined, it seems somewhat unnatural that all truthy values other than true would be false. It would be also consistent in the case of adopting a native PHP type constraint, where 1 would be cast to true at the start of function execution.

If we move to using native type constraints somewhere in the roadmap, using === true at this stage would be a breaking change.

https://3v4l.org/buI6E

Copy link
Member Author

Choose a reason for hiding this comment

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

framework/src/Illuminate/Support/Str.php

Line 842 in 09137f5

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

What's your expectation with without === true and the following example:

Str::password(numbers: 'nah');

Copy link
Contributor

@mpyw mpyw Oct 10, 2023 •
edited

Choose a reason for hiding this comment

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

Str::password(numbers: 'nah'); should be Str::password(numbers: true); because PHP treats "nah" as truthy. The important thing is that we should follow the interpretation of the PHP standard, not determine "what should be true" at the the framework.

You would be correct if Laravel were to adopt the rule of always treating truthy values other than true as false, but it is not actually happened so far. I don't think there is a lot of code that judges === true for something received with @param bool $arg, so I think that needs to be consistent throughout the framework.

Copy link
Member Author

Choose a reason for hiding this comment

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

You would be correct if Laravel were to adopt the rule of always treating truthy values other than true as false, but it is not actually happened so far

I would disagree https://github.com/search?q=repo%3Alaravel%2Fframework%20%3D%3D%3D%20true&type=code

Copy link
Contributor

@mpyw mpyw Oct 10, 2023 •
edited

Choose a reason for hiding this comment

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

@crynobone I see, that is certainly the case in some codes. But apparently it is not consistent across the board. Somewhat confused.

For the moment, I see that there seems to be no need to discuss this excessively here. I think this has been an off-topic discussion, but thank you for your patience.

]))->filter()->each(fn ($c) => $password->push($c[random_int(0, count($c) - 1)])
)->flatten();

$length = $length - $password->count();

return $password->merge($options->pipe(
fn ($c) => Collection::times($length, fn () => $c[random_int(0, $c->count() - 1)])
))->shuffle()->implode('');
}

/**
Expand Down
7 changes: 7 additions & 0 deletions tests/Support/SupportStrTest.php
View file
Open in desktop
Expand Up @@ -1288,6 +1288,13 @@ public function testItCanSpecifyAFallbackForAUlidSequence()
public function testPasswordCreation()
{
$this->assertTrue(strlen(Str::password()) === 32);

$this->assertStringNotContainsString(' ', Str::password());
$this->assertStringContainsString(' ', Str::password(spaces: true));

$this->assertTrue(
Str::of(Str::password())->contains(['0', '1', '2', '3', '4', '5', '6', '7', '8', '9'])
);
}
}

Expand Down