BC break in login auth changes from #3444978

Have raised as critical as it breaks logging in on sites which renders them unusable and would require a BC break (and therefore major version due to static analysis issues reported in #3456461-15: [2.x] Login fails with Drupal 10.3 → ) for email registration.

Given email registration 2.x is technically still in RC-5, this perhaps could be considered a major.

Joining from 🐛 UserAuth BC layer not working for modules that use username Active , same interpretation of root fault when using TFA 2.x (dev only) branch.

Linking the related issues to increase visibility.

Broken REST login here: 🐛 Error: Call to undefined method Drupal\mail_login\AuthDecorator::authenticateAccount() with Drupal 10.3.0 Active
This is highly critical.

I'm wondering if this even some simpler kind of bug??

Changing
$authenticated = $this->userAuth->authenticateAccount($credentials['name'], $credentials['pass']);
to
$authenticated = $this->userAuth->authenticate($credentials['name'], $credentials['pass']);
fixed the error for me, as it seems!

We ran into 🐛 Error: Call to undefined method Drupal\mail_login\AuthDecorator::authenticateAccount() with Drupal 10.3.0 Active after upgrading 10.3.0 blocking all logins.

For comparison:

interface UserAuthInterface {

  /**
   * Validates user authentication credentials.
   *
   * @param string $username
   *   The user name to authenticate.
   * @param string $password
   *   A plain-text password, such as trimmed text from form values.
   *
   * @return int|bool
   *   The user's uid on success, or FALSE on failure to authenticate.
   */
  public function authenticate($username, #[\SensitiveParameter] $password);

}

vs.

interface UserAuthenticationInterface {
  [...]

   *
   * This can be used where the account has already been located using the login
   * credentials.
   *
   * @param \Drupal\Core\Session\AccountInterface $account
   *   The account to authenticate.
   * @param string $password
   *   A plain-text password, such as trimmed text from form values.
   *
   * @return bool
   *   TRUE on success, FALSE on failure.
   */
  public function authenticateAccount(UserInterface $account, #[\SensitiveParameter] string $password): bool;

}

If that's the case, I guess we need to hotfix that? I can't really believe yet ...

Anybody → closed merge request !8533

Static patch if anyone else runs into this and needs a quickfix (esp. mail_login users I guess)

https://git.drupalcode.org/project/drupal/-/blob/77b8e596048aac5becd5646...

Yep that looks wrong too, it is a different bug than what we have reported here but is in the same ballpark of problems.

I wonder if it would be useful to add a test module that is a decorator for the UserAuth service that only implements the old interface and passes everything directly through (no modification) and duplicate several of the Functional/kernel Tests adding in the new test module (eg DecoratedSomethingTest extends SomethingTest).

If those tests fail it means the BC logic is faulty and may help us determine any other hidden bugs (otherwise given we have now had 3 separate issues with this it might be time for a line by line audit)

Unit tests would have been better but core is a far ways from being able to h it test everything.

Thanks for the feedbacj @cmlara, whao, seeing no tests fail with that function name change implies there are no tests for this. So I agree this should propably have (had) tests.

Still I think we should have a hotfix first, if it's clearly wrong, as updating to 10.3.0 is currently supposed to break a lot of Drupal projects logins using any of the contrib modules listed here: 📌 [11.1] Deprecate UserAuthInterface Active

Pushed a quick set of sample tests to 3456738-extended_tests.

My local lab is being a bit difficult at the moment (and I'm about to call it a night) so not tested in depth or validated for any coding standards or similar, however the root logic should be there if someone wants to run with it. We might just need to extend a few more of the existing tests to prove we fix all the scenarios (look at the files that were modified in the previous two issues and extend any tests that exercise the same code paths)

On a quick look it did appear that UserLoginHttpDecoratedTest did catch the BC break @Anybody reported.

Still I think we should have a hotfix first, if it's clearly wrong, as updating to 10.3.0 is currently supposed to break a lot of Drupal projects logins using any of the contrib modules listed here: #3427298: [11.1] Deprecate UserAuthInterface

I'll defer to core team, my concern at the moment is while this is a bad break, fixes have already been rushed once before, it would be nice to figure out what other tests we could extend and do a quick 'proof' run if it only takes a few more minutes.

I will note the linked issue indicates most of those modules are NOT implementers and would be less likely to be impacted (they are calling userAuth->authenticate() ). That is not to say this issue should be downgraded on priority, just that I'm not sure the core team is going to rush this into an emergency fix, if we end up waiting for the monthly bugfix window we might as well try and test to find out if any other faults exist (even if the tests don't make it into mainline).

Hi, we also run into issues and the hotfix here doesn't fix logging with the via email_registration module. If anyone could help out there it would be greatly appreciated!

@roromedia there's an issue in the email_registration module thats worth a look: https://www.drupal.org/project/email_registration/issues/3456461 🐛 [2.x] Login fails with Drupal 10.3 Needs review

Updated the tests fork to add one more test. it kinda tests the basic_auth through maybe I should have done it via BasicAuthTest instead.

On a positive note it seems like in my quick testing it may be just these two known faults.

I will note the elseif added in 📌 Optimize user logins by avoiding duplicate entity queries Needs work may not be exercised with just these 3 tests.

I also want to call out that ✨ [PP-1] Bump PHPStan to level 9 and accept a large baseline Postponed would have detected the issue with the UserAuthenticationController.

Unfortunately the PR here won't resolve the issue I was reporting (though may be dealing with a very similar issue via a different mechanism). It also seems to be using D11 code and targeting D10.3, which is results in loads of unrelated changes and D11 probably needs a separate fix as the BC layer is presumably removed.

Here's what I wrote on the email_registration issue:

Specifically, this was broken as part of trying to fix a different regression 🐛 UserAuth BC layer is not working for modules that use it to provide email based logins Active from the original change in 📌 Optimize user logins by avoiding duplicate entity queries Needs work .

So the fix here isn't to switch back to authenticateAccount, but rather to fix the BC layer that was intended in 📌 Optimize user logins by avoiding duplicate entity queries Needs work , and changed in 🐛 UserAuth BC layer is not working for modules that use it to provide email based logins Active .

My suggested approach as per the initial description:

perhaps move all the authentication finalisation to the end of the method with a BC if/else rather than trying to do inline with flood etc.

I’ll note I pinged Berdir on slack a couple days ago asking if we moved the ifelse code back under the block if it would still be acceptable to them (it seemed looking at 🐛 UserAuth BC layer is not working for modules that use it to provide email based logins Active it should be, and the real error was in extending the userauth class rather than implementing a proper decorator )

cmlara → changed the visibility of the branch 3456738-extended_tests to hidden.

Spoke with berdir, he confirms he has already updated his module however he opened the issue to ensure BC for other modules.

Here is a possibly less invasive version than rewriting to move all the logic to the end by instead just adding an else block for the call.

Also combined the tests and the BC break from @Anybody into !8581

Clearing the needs tests tag as I believe the tests I have extended are sufficient to cover the faults we are fixing though perhaps we still want one more tests to exercise the "user account is invalid" scenario/lookup

Reviewed.

Bringing discussion from the MR into the issue.

I think we made a pretty large mistake when we made UserAuthenticationInterface not extend UserAuthInterface.

I see no clean way to guarantee full support for D11 (and by extension d10.last) and D12 with the current BC layer.

It is possible modules may go all the way until D11.last using UserAuthInterface while at the same time modules may support only UserAuthenticationInterface as early as D10.3.

This could/will create issues for decorations when Core implements both interfaces and two decorators each implement alternative interfaces.

I’m not sure we can change the inheritance now in a patch release (interfaces are NOT api for implementers) but worth at least discussing how we want this to all work and what we are expecting from contrib.