Password is null if user has never logged in which causes PHP 8 warning

We also encountered this issue when user accounts get created outside of the normal registration process (in our case, they're getting created via the External Authentication module).

Like Michelle suggested, since the documentation for PasswordInterface::check expects to get a string for the hash, it seems we should avoid calling check altogether.

Attaching an updated patch(es) with a failing test and a fix.

Forgot to include the test in the full patch. Updated patches.

Fixing whitespace issues in previous patches.

This could also happen when users are primarily logging in via SSO. In that case, they do not/may not have a password for local login.
(I do not want to go into details about which module is used for SSO because the fact that the `pass`column in `users_field_data` database table is nullable should justify why this issue should be fixed.)

1. +++ b/core/modules/user/src/Entity/User.php
   @@ -407,6 +407,7 @@ public function setExistingPassword($password) {
   +      $account_unchanged->getPassword() !== NULL &&
   
   +++ b/core/modules/user/src/UserAuth.php
   @@ -47,7 +47,7 @@ public function authenticate($username, $password) {
   +        if ($account->getPassword() !== NULL && $this->passwordChecker->check($password, $account->getPassword())) {
   

   The fact we had to do this twice in core makes me think that there may be other instances in contrib that need to allow for this.

   For that reason I think the fix should be in the password service rather than in the user module.

   If we look at the ::check method we see it passes the second argument ($hash) to substr to check
   a) if it is a legacy updated password (starts with U$'
   b) is a d7 password (starts with $S$)
   c) is a phpass/phpbb3 password (starts with '$P$' or '$H$'

   If it is none of these it returns FALSE.

   So I think the ::check method should check before it calls substr that $hash is a string and non-zero length. If it is neither of those, it can return FALSE early, as all of the substr tests will fail anyway, ending up in the default clause of the switch.

2. +++ b/core/modules/user/tests/src/Unit/UserAuthTest.php
   @@ -280,4 +284,30 @@ public function testAddCheckToUrlForTrustedRedirectResponse(): void {
   +    $this->assertFalse($this->userAuth->authenticate($this->username, $this->password));
   

   This test doesn't have a positive assert, so in theory the code path inside the service could change, and the test would keep passing because it may no longer call the method we're asserting should never be called.

   I think this is moot anyway if we move the fix into the password check service.

   So we should remove this test and update \Drupal\Tests\Core\Password\PasswordHashingTest to add a case where $hash passed is NULL. We can then add a positive assert (e.g. assertFalse($password->check($a_password, NULL)))

I think larowlan makes a valid point. It's the password checker that uses a method which requires a non-null value, and there might be valid use cases for the User->getPassword() method to return null. IMO it would make sense to fix this issue in the password checker instead of in UserAuth or the User entity.

Along the lines of #10, here's a patch for 9.5.2 for those users who need to move past this. My site generates all accounts programmatically, so all with null passwords. I have confirmed the patch with manual testing. This patch won't apply to D10 because of the function signature change to use the SensitiveParmeter attribute. I simply cast to string because the code body handles empty strings cleanly and quickly and the code is simpler and more concise.

A bit of clean-up for #6, hope this will address feedback on #10.1

probably it needs follow-up for the interdiff because changing interface

Even if user created by drush it still can have null password, so we should change interface

Filed follow-up #3346756: UserInterface::getPassword() can return NULL

Code looks good and it fixes the errors.

Only one thing: we use snake_case for variables declared in the functions.
$userWithoutPassword = $this->createMock('Drupal\user\Entity\User');

I'm attaching a patch and the inter-diff of this change only. So I think I can move to RTBC after tests pass.

Think this one is good to go now.

I don't see that the two points in #10 have been addressed. Setting back to NW.

1. +++ b/core/modules/user/tests/src/Unit/UserAuthTest.php
   @@ -278,4 +282,30 @@ public function testAddCheckToUrlForTrustedRedirectResponse(): void {
   + * Tests the authenticate method when the user has no stored password.
   
   Let's be clear that this is testing when the user password is NULL.

+++ b/core/modules/user/tests/src/Unit/UserAuthTest.php
@@ -278,4 +282,30 @@ public function testAddCheckToUrlForTrustedRedirectResponse(): void {
+   * In https://www.drupal.org/project/drupal/issues/3305807 it was discovered
+   * that attempting to login as a user that has no stored password will
+   * generate PHP notices due to calling substr on a NULL value.

I think testing when the password is NULL is a valid test and we don't need this extra paragraph. And especially it should not be referring to a d.o issue. I am usually all for comments but I don't think this is adding useful information. What would be useful is to add how the password can be NULL as comments in the fix.

Needs re-roll as comited #3346756: UserInterface::getPassword() can return NULL

re-roll

Think I jumped the gun too early before, that's my mistake. Moving to NW per #20

yes, this extra paragraph is useless, there's git for history

Think this is good now.

In #20 I suggested adding a comment explaining how the password can be NULL for an existing user. I still that is worth doing.

I don't see an answer to why this shouldn't be done in the password service asked in #10.1

Can we elaborate on why we didn't go that way?

Added comment as suggested in #26

Still needs an answer on 10.1

Fixed CCF from #28

Also, needs work for #20/#26.. The changes in the patch in #3 didn't add the comment in the fix.

Basically any user created without password will have it as NULL, no idea where to document it

patch #30 was not applying, Attached rerolled patch.
Unable to find comments line from #20, #26
thanks

Per #31

@larowlan I agree with your suggestion and here is the patch for your comment #10. I've found, that PasswordHashingTest is now LegacyPasswordHashingTest due to recent changes in password processing logic. So, I've made a change to the new service and to the old one, because as long as this code is presented it needs to work properly although it's deprecated. Please, correct me, if I'm wrong.

Thanks @ilya.no I missed this point but I checked and there's only string, empty string and NULL values possible

Moreover in context of #3050720: [Meta] Implement strict typing in existing code it can use follow-up to add deprecation if not a string|null is passed

+++ b/core/lib/Drupal/Core/Password/PhpPassword.php
@@ -45,6 +45,10 @@ public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $
+    // Newly created account may have empty password.
+    if (!is_string($hash) || (is_string($hash) && mb_strlen($hash) === 0)) {

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php
@@ -242,6 +242,9 @@ public function hash(#[\SensitiveParameter] $password) {
   public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $hash) {
+    if (!is_string($hash) || (is_string($hash) && mb_strlen($hash) === 0)) {
+      return FALSE;

I bet we can simplify it with if ($hash === NULL || $hash === '')

because it must be a string or null here

Re #10.1

Can we elaborate on why we didn't go that way?

I wanted to prevent calling service ("carbonless-optimization") as both places already checking previous argument for NULL to prevent useless function call.

I still sure we need to use the same pattern for saved hash as there's many reasons it could appear a NULL from database

Added changes mentioned in #36

Re #38 is wrong pat h as changes only in interdiff

Oh, sorry, here is correct patch

thank you

Seems part of #10 has been addressed. Think the part that @larowlan was looking for was a positive assertion somewhere.

@smustgrave I'm not sure, that I've got your point. In comment #10 it's stated, that we need to add test case, which looks like

assertFalse($password->check($a_password, NULL))

In my patch I've added following code

$this->assertFalse($this->passwordHasher->check($this->password, NULL));

which looks the same, as suggestion from comment #10.

Could you correct me, if I'm wrong? Or maybe I didn't understand you. Thanks!

Yes but it was request for a positive assertion. AssertTrue somewhere

Logic of latest patch is pretty different from what was addressed in #10

So I don't see how we could have positive assertion for latest patch logic

11.x is the current dev branch.

Also #49 seems to be removing some of the original patch from 40.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Patch from #40 can be applied to 11.x as well

Going to see what the committers think if we need to do some trickery for a positive assertion. Going to mark it so it doesn't get lost in the shuffle of things.

Which patch is RTBC? 40 or 49? If it's 40, that should be re-uploaded so it's the newest patch on the issue. I think just a negative assertion is fine, we're testing there's no PHP warning.

@catch RTBC is for patch #40.
Patch from comment #49 is for 10.0.x branch without any explanation and I think, that we can ignore it.

@ilya.no hid patch 49.

So #40 should be at the top.

Thanks for working on this! A few small things:

1. index f16eeb200c..e6863b6c3e 100644
   --- a/core/lib/Drupal/Core/Password/PhpPassword.php
   
   index 052c2be81e..2cbdb3d617 100644
   --- a/core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php
   
   @@ -242,6 +242,9 @@ public function hash(#[\SensitiveParameter] $password) {
   +    if ($hash === NULL || $hash === '') {
   +      return FALSE;
   +    }
   

   I think the same inline comment needs to be added above this hunk as well, unless we want to abstract it into a protected checkEmptyPassword() method somewhere.

2. +++ b/core/lib/Drupal/Core/Password/PhpPassword.php
   @@ -45,6 +45,10 @@ public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $
   +    // Newly created account may have empty password.
   

   Very minor nitpick: This is not a complete sentence, and complete sentences are required by our documentation guidelines:

   All documentation and comments should form proper sentences, use proper grammar and punctuation, and generally follow the same style guidelines as Drupal.org content: http://drupal.org/style-guide/content

   So, I think this should be:

   Newly created accounts may have empty passwords.

3. +++ b/core/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php
   @@ -122,4 +122,13 @@ public function testPasswordRehashing() {
   +   * Tests password check in case provided hash is NULL.
   

   Tests password validation when the hash is NULL.

4. +++ b/core/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php
   @@ -122,4 +122,13 @@ public function testPasswordRehashing() {
   +    $this->assertFalse($this->passwordHasher->check($this->password, NULL));
   

   Shouldn't we also cover empty string in the same method?

5. +++ b/core/tests/Drupal/Tests/Core/Password/PhpPasswordTest.php
   @@ -124,4 +124,13 @@ public function providerLongPasswords() {
   +   * Tests password check in case provided hash is NULL.
   +   *
   +   * @covers ::check
   +   */
   +  public function testEmptyHash() {
   +    $this->assertFalse($this->passwordHasher->check($this->password, NULL));
   +  }
   

   Same comments as above.

I'm working on this issue and @xjm and @allisonherodevs are mentoring me on it.

Addressed #58 and added return type hints to the new methods.

So I just spent 15 minutes trying to figure out what the heck was going on with #60. The patches are readable in dreditor or if opened in their own tab. However, when I try to curl them -- or even "File... Save as..." I get an empty file.

I think this is a previously-unknown-to-me security "feature" of Drupal.org in that it won't serve files with the word "password" in the name, which also means DrupalCI gets an empty patch. It's imperfect though because I can open them in the browser in Dreditor or their own tab. Weird, yo. Edit: Hypothesis false per @drumm. Instead, the files might be encoded as UTF-16 and #2922638: No charset on response for patch & text files could then mean curl (and therefore DrupalCI) don't know how to interpret it.

What I had to do was create a new file locally, copy the text from the patch opened in a tab, and paste it into my editor. I thought of naming them "hash-nid-etc.patch" but thought that could be, er, misinterpreted.) These are just versions of @ChrisPerko's patch and test-only patch that Drupal.org will deign to serve, so I'm still eligible to review/commit this when appropriate. Fun times!

Anyway, adding credit for Allison as well as the correct mentoring attribution for myself.

Looks like stuff is mis-indented which at least is easier to fix. 😂

FILE: ...ore/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 131 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 6
     |       |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
 132 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 6
     |       |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...w/html/core/tests/Drupal/Tests/Core/Password/PhpPasswordTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 133 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 6
     |       |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
 134 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 6
     |       |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I fixed the spacing, I'm crossing my fingers that I created the interdiff and patches correctly.

All three files seem to contain the interdiff. :)

Hopefully this is right this time. If not, someone else can fix it, or I will get to it Monday morning. Thanks for your help @xjm

Those look like the correct patches, but they still have whatever weird encoding problem unfortunately.

Repeating what I did in #62 except with the original filenames. The interdiff is attached to #68 and works fine since DrupalCI does not need to apply it.

There we go. :) Correct test failures.

Thank you, back to RTBC

Looking much simpler, nice work.

I think we're missing a documentation update here.

Currently \Drupal\Core\Password\PasswordInterface::check documents that hash is a string, but we're now effectively saying we also support NULL. I think we should update the @parameter type-hint from string to string|null.

I have updated the @parameter type-hint from string to string|null in core/lib/Drupal/Core/Password/PasswordInterface.php as per #75. Please review. Thanks

CC failure

Saving credit for ChrisPerko for keeping this issue moving forward.

Fixed CS

+++ b/core/lib/Drupal/Core/Password/PasswordInterface.php
@@ -27,14 +27,14 @@ public function hash(#[\SensitiveParameter] $password);
    * @param string $password
-   *   A plain-text password
-   * @param string $hash
+   *   A plain-text password.
+   * @param string|null $hash

this change is out of scope a bit but I included it as part of context

Change Looks good.

+++ b/core/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php
@@ -122,4 +122,14 @@ public function testPasswordRehashing() {
+  /**
+   * Tests password validation when the hash is NULL.
+   *
+   * @covers ::check
+   */
+  public function testEmptyHash(): void {
+    $this->assertFalse($this->passwordHasher->check($this->password, NULL));
+    $this->assertFalse($this->passwordHasher->check($this->password, ''));
+  }

This test didn't fail - and that's because of #3393072: Ensure Unit tests in phpass run and remove unneeded LegacyPasswordHashingTest::testInvalidArguments, so postponing on that

And that's because its in a Tests folder when it should be in a folder named Unit

There are two tests in that folder, and neither of them are running in HEAD

Blocker is in

reroll after https://git.drupalcode.org/project/drupal/-/commit/9d4f46c05be69bf125f3a...

Reroll seems good and tests all green!

Committed to 11.x and backported to 10.2.x and 10.1.x

Thanks all.

The final patch was nice and small which is always a good sign

Thank you! it was the last bit of PHP 8.0 compatibility! Now looking for 8.3)

sorry, the 8.1 compatibility)

@larowlan
It would be great to backport the MR also to 10.3
if there is a way I can help on that, let me know