Problem/Motivation

In #2418119, we introduced code in
ProtectedUserFieldConstraintValidator to validate that a user’s password is correct. However, that fix calls User::checkExistingPassword(), which only checks Drupal’s local password hash.
This fails for sites using external authentication or alternative password storage, because the
validation never goes through their external auth system.

Drupal provides a UserAuthInterface service for pluggable authentication, but it was
never fully integrated into this constraint validator. Therefore, password validation still bypasses
any external auth mechanism if you try to change protected fields via REST or other entity-based updates.

Proposed Resolution

  1. Use the user.auth service: Instead of calling
    User::checkExistingPassword(), the
    ProtectedUserFieldConstraintValidator should invoke the
    UserAuthInterface service. This allows sites using external or alternative
    authentication mechanisms to validate passwords properly.
  2. Deprecate User::checkExistingPassword(): Since we can now rely on
    UserAuthInterface, we propose deprecating checkExistingPassword()
    in the User entity to reduce confusion.
CommentFileSizeAuthor
#6 2503113-6.patch5.27 KBdawehner
#6 2503113-6-fail.patch4.4 KBdawehner

Issue fork drupal-2503113

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

pwolanin’s picture

alexpott’s picture

Issue tags: -Triaged D8 critical
dawehner’s picture

Issue summary: View changes

Implement a new ProtectedUserFieldConstraint that validates the password and email field for users on form submissions and REST requests. Add methods to UserInterface to set the existing plain text password so that the validation constraint can check if the correct password was supplies to change the protected user and email fields. Add a new "existing" password property to PasswordItem which holds the supplied plain text password but is never saved.

That seems to be just a copy of the old issue summary ... let's skip it to not get confused ...

I'm wondering whether the password checking for itself adds value or whether we always want to use the user authentication.
Maybe we could provide a special field which provides user authentication, given that the password field is a generic field type as part of core
and user authentication is an additional concept on top of that.

The user entity would then go with the password field with the user authentication constraint, while other people can still use a generic password field.

dawehner’s picture

Let's try to work on that.
Thank you @andypost for adding this sort of related issue

dawehner’s picture

Status: Active » Needs review
StatusFileSize
new4.4 KB
new5.27 KB

The UserAuthInterface may need to be enhanced to also have a method to accept a pre-loaded user entity rather than always loading based on user name.

I think we don't need it, because we call this just in case we change an already saved user.

webchick’s picture

Priority: Critical » Major

Talked to Peter. This will cause issues for anyone using REST + non-local auth, but that isn't "the system" being unusable:

"it's not grossly broken, just using the wrong api"

Downgrading to major as a result. If there's something directly exploitable, let's revisit. ATM it sounds like people just won't be able to authenticate at all.

dawehner’s picture

So let's assume you want to simple use a different authentication. You replace the user.auth service so the UI uses that to login.
Would you expect the password field in your user edit form to talk with the authentication service or with the storage in user, which stores something totally different?

The last submitted patch, 6: 2503113-6-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2503113-6.patch, failed testing.

The last submitted patch, 6: 2503113-6-fail.patch, failed testing.

The last submitted patch, 6: 2503113-6.patch, failed testing.

wim leers’s picture

Component: rest.module » user.module
Issue tags: -D8 upgrade path

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

It has been over 9 years since there was work here.

Can someone confirm if this is still relevant?

I am setting the status to Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks

catch’s picture

Title: REST user updates need to validate password using UserAuthInterface » ProtectedUserFieldConstraintValidator should use the user auth service instead of ::checkExistingPassword()
Status: Reviewed & tested by the community » Needs work

This looks like it's still valid, at least in the sense that User::checkExistingPassword() has the same code that's changed in this issue and it's still called from ProtectedUserFieldConstraintValidator. I think we

There's now UserAuthenticationInterface which accepts an $account, which covers the last paragraph of the issue summary.

Having said that I'm not convinced that the patch is correct, it's calling the auth service from the user entity, but I think we might want to call the auth service from the validator instead, and then maybe deprecate the method on User.

rafuel92 made their first commit to this issue’s fork.

rafuel92’s picture

Assigned: Unassigned » rafuel92

rafuel92’s picture

Status: Needs work » Needs review

Hello, I've created a merge request to move the checkExistingPassword method into the UserAuthenticationInterface and use it in the ProtectedUserFieldConstraintValidator. I've also deprecated the method in the User entity. Let me know if I can do any other task.

smustgrave’s picture

Status: Needs review » Needs work

Believe we are still suppose to use dependency injection.

Also issue summary should be complete

Thanks

rafuel92’s picture

Sure, dependency injection added to the Constraint Validator, let me know if i can help with other tasks.

rafuel92’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Haven't fully reviewed but issue summary is incomplete

rafuel92’s picture

Issue summary: View changes
rafuel92’s picture

Status: Needs work » Needs review

Summary updated

rafuel92’s picture

Issue summary: View changes
smustgrave’s picture

Status: Needs review » Needs work

Appears to have pipeline failures in the unit tests.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.