Problem/Motivation

user_password generates a random alphanumeric password.

The many contrib modules using it would be able to use this more easily if it were a service.

See http://grep.xnddx.ru/search?text=user_password&

Proposed resolution

Create a password_generator service that generates a password using the same algorithm as user_password currently does.

Remaining tasks

User interface changes

API changes

user_password() is deprecated.

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper created an issue. See original summary.

dpi’s picture

Title: Deprecate user_password and move to Password component » Deprecate user_password and move to a dedicated password component

Sounded like it existed already.

kim.pepper’s picture

Title: Deprecate user_password and move to a dedicated password component » Deprecate user_password and move to a Utility component

This doesn't seem big enough for it's own component. Maybe just in Utility?

kim.pepper’s picture

Title: Deprecate user_password and move to a Utility component » Deprecate user_password and move to Utility component
Status: Active » Needs review
FileSize
4.34 KB

Moved to Utility\Random::password()

Status: Needs review » Needs work

The last submitted patch, 4: 3153085-4.patch, failed testing. View results

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
11.27 KB
6.93 KB

Removes usages.

kim.pepper’s picture

❓Looking at the Random class, it only contains instance methods, so adding a static method to it seems a bit wrong. Happy to hear opinions from others?

Krzysztof Domański’s picture

Looking at the Random class, it only contains instance methods, so adding a static method to it seems a bit wrong.

Most of the Utility classes declare static methods. I think it can be static here.
#2564431: Convert the utility functions in Random to statics.
#2671188: Ultility Class Random has many bugs

Krzysztof Domański’s picture

Status: Needs review » Needs work

The Random class does not declare static methods like the other Utility classes. This is intentional. It is not recommended to use static methods for generating random data. See https://stackoverflow.com/questions/41087939/is-it-good-practice-to-use-static-methods-for-generating-random-numbers

kim.pepper’s picture

Status: Needs work » Needs review

@Krzysztof Domański you linked to a SO post about C#. There is no state that we need in the static method to worry about thread safety. My question was more about the design of having static methods on that utility. I'm leaning towards a RandomUtils or Password class.

vijaycs85’s picture

patch in #6 looks goosd to me.

Looking at the Random class, it only contains instance methods, so adding a static method to it seems a bit wrong. Happy to hear opinions from others?

I am not sure either. considering how Random is used, I wonder why the other methods are not static. There isn't any dependency or attributes (other than string and name). In any case, static seems fair for password().

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community
DerekCresswell’s picture

I'm going to plus one the #6 patch. Having password as a static function is the right way if you ask me.

I don't see any reason why we wouldn't want all of these random methods to be static. Perhaps we could transition more of these functions to static as well as random utilities lend themselves to static methods.

catch’s picture

Status: Reviewed & tested by the community » Needs review

The reason that the other methods on the class aren't static is because they all track the result of previous method calls, to ensure that duplicate strings aren't created in the same request. This is because the methods are mainly used for generating test data.

So... the method can be static, but I'm not sure this is the right place for it to live (but also not sure if there's a better place for it either).

kim.pepper’s picture

How about a new Password class in the same Utility namespace?

longwave’s picture

I don't see what difference it makes mixing static and non-static methods in the same class, and Random feels like the right place to add this to me.

+++ b/core/lib/Drupal/Component/Utility/Random.php
@@ -300,4 +300,31 @@ public function image($destination, $min_resolution, $max_resolution) {
+  public static function password($length = 10) {

If we are going to go down the route of #1158720: [policy, no patch] Add parameter type hinting to function declaration coding standards sooner or later, is this worth doing here while we are adding a new method? e.g.

public static function password(int $length = 10): string {
kim.pepper’s picture

FileSize
11.28 KB
745 bytes

Adds typehinting as per #16

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, back to RTBC. We haven't quite addressed @catch's comments from #14 but I am not sure what a better alternative looks like.

DerekCresswell’s picture

I couldn't think of a better spot for a 'random' password than the random class. A full class for just a password still seems overkill. If I was looking for a way to generate a random password I would think to type 'random password' not 'password make password' (or whatever it may be).

It would be trivial to add a 'unique' parameter to make sure the password is always unique. Though I don't feel this is to necessary and being static is fine.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So this is one of those funny things but this change represents a performance loss on pages that need this method. Because we have one more class to autoload - while user.module is always loaded. OTOH it's one less global function for every other page. So maybe a net gain.

I think #14 is an important point. Mixing this with the test random system is a bit confusing and breaks the expectation that something generated form this class is guaranteed to be unique for that request.

Also one of the common tasks of password policy modules is to require longer or more varied. That means that being able to swap or override this might be useful. In fact it would be good to see what modules like https://www.drupal.org/project/password_policy are doing with respect to password generation and see how making this change could make things easier.

kim.pepper’s picture

I think #14 is an important point. Mixing this with the test random system is a bit confusing and breaks the expectation that something generated form this class is guaranteed to be unique for that request.

Would you go with a separate class with a static method, e.g. Password::generate()? If we are talking swapping or overriding do we need a service?

I don't think Password Policy does any password generation? Had a look and couldn't see anything.

It does validation and constraints with a module/plugin for each different one:

  • password_policy_character_types
  • password_policy_characters
  • password_policy_consecutive
  • password_policy_history
  • password_policy_length
  • password_policy_username
alexpott’s picture

@kim.pepper well maybe that's because they can't - there's no way that user_password() can generate a valid password if you have that module installed.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
15.92 KB
4.64 KB

OK. Here's a naive implementation for discussion that takes some options for which character sets to use.

alexpott’s picture

I think we should refactor the existing function to a service and not change the functionality. This will allow modules like the password policy module to override it. I then think we might consider improving the core implementation - but we shouldn't do that in the same issue.

kim.pepper’s picture

FileSize
16.97 KB
5.04 KB

We could also go down the path of specifying the number of digits and symbols in the password.

kim.pepper’s picture

I think we should refactor the existing function to a service and not change the functionality. This will allow modules like the password policy module to override it.

The challenge here is the method signature becomes the contract. Overriding services might want to change the options per invocation (e.g. exposing a form with options for symbols, digits, length etc) and not with fixed service configuration.

kim.pepper’s picture

FileSize
11.1 KB
19.36 KB

OK. Discussed this with @larowlan in slack, and think a simple service with a default implementation that matches the current behaviour should be ok.

kim.pepper’s picture

Title: Deprecate user_password and move to Utility component » Deprecate user_password and move to Password Generator service
Issue summary: View changes
longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now, the Drupal\Core\Password namespace is ideal.

  • catch committed b7d7a1d on 9.1.x
    Issue #3153085 by kim.pepper, longwave, alexpott, vijaycs85, catch:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed b7d7a1d and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.