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
-
Use the user.auth service: Instead of calling
User::checkExistingPassword(), the
ProtectedUserFieldConstraintValidatorshould invoke the
UserAuthInterfaceservice. This allows sites using external or alternative
authentication mechanisms to validate passwords properly. -
Deprecate
User::checkExistingPassword(): Since we can now rely on
UserAuthInterface, we propose deprecatingcheckExistingPassword()
in theUserentity to reduce confusion.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 2503113-6.patch | 5.27 KB | dawehner |
| #6 | 2503113-6-fail.patch | 4.4 KB | dawehner |
Issue fork drupal-2503113
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
Comment #1
pwolanin commentedComment #2
alexpottComment #3
dawehnerThat 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.
Comment #4
andypostComment #5
dawehnerLet's try to work on that.
Thank you @andypost for adding this sort of related issue
Comment #6
dawehnerI think we don't need it, because we call this just in case we change an already saved user.
Comment #7
webchickTalked 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.
Comment #8
dawehnerSo 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?
Comment #13
wim leersComment #27
quietone commentedIt 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
Comment #28
catchThis 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 weThere'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.
Comment #30
rafuel92 commentedComment #32
rafuel92 commentedHello, 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.
Comment #33
smustgrave commentedBelieve we are still suppose to use dependency injection.
Also issue summary should be complete
Thanks
Comment #34
rafuel92 commentedSure, dependency injection added to the Constraint Validator, let me know if i can help with other tasks.
Comment #35
rafuel92 commentedComment #36
smustgrave commentedHaven't fully reviewed but issue summary is incomplete
Comment #37
rafuel92 commentedComment #38
rafuel92 commentedSummary updated
Comment #39
rafuel92 commentedComment #40
smustgrave commentedAppears to have pipeline failures in the unit tests.