Update generate password logic to use proper methods

[Mark-H]

What does it do?

Re-up of #15894

Changes password generation method to be more secure.

Why is it needed?

Actually random generation.

How to test

Apply and see passwords still get generated.

Related issue(s)/PR(s)

Re-up of #15894 Fixes #15740

[codecov[bot]]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (73bfd27) 21.68% compared to head (185bd9b) 21.66%.

Files	Patch %	Lines
core/src/Revolution/modUser.php	57.14%	3 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##                3.x   #16521      +/-   ##
============================================
- Coverage     21.68%   21.66%   -0.03%     
  Complexity    10496    10496              
============================================
  Files           561      561              
  Lines         31703    31698       -5     
============================================
- Hits           6875     6867       -8     
- Misses        24828    24831       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[smg6511]

I've got a couple general questions on this before doing a quick review:

• I assume that since you changed the password dictionary variable from $options['allowable_characters'] to $options['alphabet'] that you're not worried about breaking usage of this method by custom implementations/extras. If that's the case, we might want to consider deprecating the $options param or go even further an deprecate both params and consider this a private method.
• Re the following block, it seems to me you'd want to define the alphabet if the $options['alphabet'] param is empty; or am I not seeing this right:

    if (!empty($options['alphabet'])) {
        $alphabet = array_merge(range('a', 'z'), range('A', 'Z'));
        shuffle($alphabet);
        return substr(implode($alphabet), 0, $length);
    }

in other words, is something like this what you're really going for?

    $alphabet = empty($options['alphabet']) 
        ? array_merge(range('a', 'z'), range('A', 'Z')) 
        : $options['alphabet']
        ;
    shuffle($alphabet);
    return substr(implode($alphabet), 0, $length);

[Mark-H]

Hmm, I overlooked the change from allowable_characters to alphabet recreating the original PR, but the behavior seems to have been intended differently. I don't know if the old behavior is worth keeping or not. Would welcome feedback on this, as well as examples that may use this code a certain way that would break.

I'd be fine with removing that block entirely and limiting the functionality of this method to only the new generation with random_bytes() to keep things simple.

[smg6511]

Yeah, my guess is there's likely been no usage of that method outside of the core. If, however, there has been and a dictionary were passed in via $options['allowable_characters'], that would break with the new var name. That said, I'd opt for dropping the block as you suggested.

[smg6511]

Looking at this again I see what you mean when you imply the change (changing the options key in question) wouldn't break anything. It might cause unexpected output if there was external use of this method where chars and/or a custom seed were passed into the $options, but would not prevent a password from being generated. I get now that your changed variable is meant to be a boolean value, not a character set (in which case a key of something like 'use_alpha_chars' would be more clear).

At any rate, I'd still say drop that block altogether because the new generation logic you added below it will create a better password anyway. Further, I'd even move toward making this method private and removing its params, making it even more clear that this is a utility method meant for internal use only. After all, I think the intention is that these passwords are temporary ones that users should immediately change.

[theboxer]

I'd agree with @smg6511 and nuke everything but the length & bin2hex usage :)

However I'd keep the signature same (even though $options will not be used), just to keep the BC going.