Problem
Spin-off from #2405943: User entity validation misses form validation logic:
When you edit your user account, you have to provide the existing password when you want to change the password or e-mail. This security feature, is currently by-passed by REST user updates as you can change the password or e-mail without providing the password.
The current validation logic lives in user_validate_current_pass().
Given that means there are security implications when you enable REST for user account updates, I think this is critical.
Proposed resolution
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.
Remaining tasks
-
Follow-up: #2462105: Failed user password hashing message is confusing
User interface changes
none.
API changes
For REST requests which change either the password or the mail an 'existing' password is needed.
For API calls changing the password (with enabled validation), it is required to provide $user->password->existing
.
The methods setExistingPassword()
and checkExistingPassword()
are added to UserInterface
.
Comment | File | Size | Author |
---|---|---|---|
#117 | rest_user_updates-2418119-117.patch | 40.65 KB | nlisgo |
#108 | rest-user-updates-2418119-108.patch | 40.64 KB | jhedstrom |
#108 | interdiff.txt | 854 bytes | jhedstrom |
#98 | d8_user_val_rest.patch | 40.62 KB | fago |
#98 | d8_user_val_rest.interdiff.txt | 4.02 KB | fago |
Comments
Comment #1
fagoI don't think we can solve this on entity validation level, as the provided values can validate fine regardless of the provided password. Instead, this is a security feature that we do not allow updating account name, mail or password unless the user provided extra-authentication (password). Thus, I think this is probably best addressed by special casing user account updates in the REST system?
Comment #2
BerdirCan you clarify why it is bypassed? if there is a validator that fails if the e-mail changes, then that should work too? Note that I did not read yet what you actually did ;)
Comment #3
fagoAs you mentioned in #2405943: User entity validation misses form validation logic, it does not require the user to provide the current password. Also, we cannot check during entity validation whether the user provided the password or not, as we do not know which value was provided by the user or not.
Or could / should we require the password to be available in plain text on the user account object for that changes to validate, and validate it matches?
Comment #4
fagoNote: I did not actually check you can update pass/mail via REST, but from looking at the code I see no reasons it shouldn't work as for every other field.
Comment #5
BerdirAh, I see, I thought you had implemented a validation constraint that would flag an e-mail change as invalid unless the password is provided as well for the user.
As you did not, yes, then we either need to solve that in a generic, or rest-specific way.
Comment #6
catchComment #7
klausiI would suggest a temporary "currentPassword" property on the account object. That way we could share the validation between form submissions and REST requests. We could add a "ProtectedUserFieldConstraint" to the name, password and email fields on user account entities. It would throw a violation if the password in $account->currentPassword is not present or does not match the user's current password.
Workflow for forms:
* Set $account->currentPassword in the validation handler of the user form.
* Run the validation handlers, which will trigger the constraints.
* unset($account->currentPassword) in the validation handler.
Workflow for REST:
* Create a custom normalizer for user account entities.
* Populate $account->currentPassword if present in the denormalized data from the request.
* Validation runs as usual as it does now and throws violations (no change there).
So this is a bit obscure with the artificial $account->currentPassword field flying around. We would have to document how and where this field can be set for REST PATCH requests for user entities. I think we should not make currentPassword a method on the User entity interface, because accessing plain text passwords should not be done by anybody else.
Comment #8
fagoyeah, that approach sounds reasonable to me. Not necessarily nice, but still the best option I can think of.
Comment #9
catchComment #10
Gábor HojtsyI looked at this one but did not find any clear way to make it apply to the right fields dynamically. Not a very fancy solution but I put in a switch() based on the definition name. Any better ideas? This is a very rough start :)
I am not sure I am the best person to help with this one just wanted to help get the ball rolling.
Comment #12
larowlanThis is a security issue only if rest uses cookie authentication. The original intent was to prevent xss password changes - Google 'anything you can do xss can do better' to see a screencast from @coltrane on how xss could pawn the admin account in d6, where this feature doesn't exist. So definitely needs doing.
Comment #13
larowlanCan we use $items->getEntity()?
This can be injected (see #2197029: Allow to inject dependencies into validation constraints)
Needs a blank line.
Comment #14
larowlanpoking along
Comment #15
larowlanbattery getting flat
still needs tests
Comment #16
klausiShould be "Check if plain text password was provided in order to change a user field."
This should be removed in the end, since the form validation should also rely on this?
Also: let's write the test case first in UserValidationTest.
I wonder if we really need to temporarily store the plain text password on the user entity. We could also store the existing hash? Not sure how hacky that will look when the user denormalizer needs to load the existing hash from the DB, but we could try.
Comment #17
larowlanhalf-assed rest test
Comment #19
larowlanFurther iterations on halfarsology™
I guess so, but not sure of the best approach. Pity we can't make the typeddata bubble to the user entity and set a protected flag.
Comment #21
BerdirWorking on this.
Comment #22
BerdirThis is a tough one, took me quite a few attempts to get it working, and that's only covering half the complexity we'll need to replace the form validation.
- As @larowlan correctly hinted at in #12, this is only needed if you are updating your own user
- PATCH copies *fields* and only fields from the submitted entity. the rest submitted fields thing does not exist on the entity that is being validated, nor anything else. So the *only* way short of overriding that code for user somehow is to make current_pass a computed field on the user, with methods to get and set it
- Added a flag to skip this for the form for now, see todo below.
- Just checking that the current pass is there is not quite enough, we also need to validate it, added tests for that.
- Updated the validation message to be the same as in the form.
- Keeping just the hash or so is impossible. The password service doesn't work like that, hash() generates a different hash every time, you must call check($current_pass, $user_entity).
Todo:
- Make it more generic, I think we can just compare the getValue() of the new and old field, didn't try that yet.
- More tests, for the other fields, also seems like a good idea to have test coverage that changing the password is possible too.
- Use it for the form, although think that should maybe be a non-critical follow-up, because it is going to be tricky: There is the $_SESSION['pass_reset_'. $user->id()] stuff that we need to deal with.
Comment #23
BerdirOk, this might be easier than I thought after all. This removes the user_validate_current_pass() validate function (and a test for it), and instead relies on the validation API. i kept the flag, but only using it for the password reset flag thingy.
I don't think it makes sense to keep that function, and with that, that test. The funny thing is that the test is almost passing still, only an exception because the function no longer exists. Because the test just makes sure we can bypass it, not if it actually works ;)
This function is a very annoying problem in #2227381: Apply formatters and widgets to User base fields 'name' and 'email', removing it will simplify that as well.
Funny side note: The test was added by alexpott and might have been the first time he indirectly (well, Dries did, because he forgot to commit the new files as usual) broke HEAD :)
Also added tests to change the password, and found more rest field access check bugs, it wasn't just create that used the wrong permission, it's also update. And that didn't actually break any tests other than the one that we're adding here :(
Updated the constraint to use getValue() for the comparison, seems to work fine.
Note: We should also update UserValidationTest for the new validation constraint.
Note 2: I don't think that name is actually protected in HEAD in the form, I don't think so. Let's see if this breaks some tests.
Comment #25
BerdirIgnoring empty passwords
Comment #27
BerdirYes, there is one test failing due to this not being validated on the username in HEAD.
Comment #28
yched CreditAttribution: yched commentedUninteresting remarks :
About non-field custom properties on ContentEntities - not stricly related, but I need you to review the patch over there, so shameless plug on #2426509: ContentEntityBase::__set() messes with values that happen to be TypedData :-p
Welcome back, computed field in core :-)
Could maybe use a word of comment on how it's used and what's the code flow (since a computed field usually goes along with a custom ItemList class to actually provide the values) ?
Unintended phpstorm linebreak ?
+ extra empty line
Also : known issue, but wow the code ratio to move from a form validate callback to a Typeddata constraint really sucks :-/
Comment #29
klausiWhy do we make an "official" field out of this? The current password should be secret and not be used by anyone else.
Do not use \Drupal here, inject the current_user service instead with the create() static method.
Same here, password service should be injected.
I think we should NOT make this part of the public API. The current password is highly sensitive data and it should never be used except in this rare user change validation
So I'm very uncomfortable with the idea that we have the current password flying around, and even as API function. It just invites bad practices where people are going to use the current plain text password for other purposes thereby creating security vulnerabilities.
I briefly discussed with fago the idea that we could hand around the password hash from the database instead of the current plain text password. It would mean that we have to perform a user load unchanged to get the current hash, verify it with the password service and stick the current password hash onto the user object. Berdir has conveniently avoided creating a specialized user denormalizer here, but that would be needed to do this password hash preprocessing. It makes the denormalizer a bit ugly, since it has to call the password service which is a bit over the top of its usual business. On the other hand we can remove the plain text password earlier from the pipeline with that, which has a security benefit.
I was also thinking about the field type PasswordItem.php and if that should support the current password to be set as property next to value. Of course it would never be allowed to save that current password, so it would not be part of schema(), but it could be part of propertyDefinitions()?
So I think the minimum that has to happen here is the removal of the currentPassword() API functions. We should just use $user->_currentPassword or $user->pass->currentPassword if we can make it work with the Password field.
Otherwise: cool test coverage, thanks Berdir!
Comment #30
Berdir1.
Please read #22 (again) :)
It might be possible to make it part of the password field type, I didn't try that, but I'm also not sure I like that. It is a user specific concept, not something that has any meaning on a password field, if that is re-used anywhere else.
I also don't really get the "it is a secret" statement. It is not. This issue makes an API. You are required to set it if you are changing the current user based on user input and want to validate it. So it shouldn't be some secret, it should be a documented API. I'm also considering to make the current flag for skipping it that I added there an API. Because if you really want to be able to write a custom UI trough the API, you will need to replicate the password-recovery feature as well. I don't think this is a security problem, we need the password, and transmitting it to the server seems like a much bigger risk than being able to access it in code?
Comment #31
klausiYes, my assumption is that we would write a new denormalizer for user entities which can do whatever it wants to populate the current password anywhere on the user on PATCH requests. That denormalizer could also perform an user unchanged load thereby getting the existing password hash from the DB. That way we don't get a different hash, but the very same hash as stored in the users table. We can then check the submitted plain text password if it matches that hash and either discard it or set it on the user object.
I don't think that needing the existing password to change a password is a user specific concept. This could be useful for other password fields on other entities as well. If an entity does not need that validation they simply leave out the validation constraint and it should work as it is right now.
My fear is that the security team will get bogus bug reports like "The user plain text password is accessible on the user object, WTF does Drupal not hash user passwords?!!!!1elf" during the D8 release cycle.
Setting this back to needs review as we need a third opinion whether we should have the public current password API methods on the user interface.
Comment #32
BerdirI actually had a denormalizer at one point, but that can not influence what happens in EntityResource::patch().
If you want move all that logic to the denormalizer, then the form code would have to do the same, as would anyone that is saving user objects like this. IMHO, that is even more reason to have setCurrentPassword() on the user entity, maybe that could then do that check, and just store a flag (which still needs to be stored as a field value, somehow), and getCurrentPassword() would be come isCurrentPasswordValid() or something.
If we make the API work as described above or similar, then it is definitely a user specific thing, but I guess we could keep that logic on the user entity and have a current_password_valid property on PasswordItem, but without anything actually using or considering that outside of user specific code, I still don't really see how that is useful?
How would the current password then be submitted in the normalized/serialized structure? having it as a computed field makes that very easy, if we don't have that, then that gets pretty weird. I don't think sending it as part of 'pass'/PasswordItem works, because then you could not do that without being forced to also the send the existing password as value, but it has to be separate, to also support a new password.
Would it be a custom top-level array that we re remove in the normalizer? BTW, I had all that already written and implemented (a custom normalizer), until I noticed that I can't get around EntityResource::patch().
You have more experience with (bogus) security reports, but I honestly don't see how that is any more a problem than it is already. It is computed and won't be there unless it is explicitly set, which only happpens in rest/form submissions. And for those, the new password is already in $user->pass->value as well, if submitted, before it gets hashed. And it is definitely not worse, more likely less than D7. But anyway, if we can make something as the stuff described above work, then that's fine with me I think.
Comment #33
fagoI must say that I agree with klausi that having
getCurrentPassword();
on the user interface is a bit weird, in particular in addition to getPassword() (is that not current?). However, I also agree with berdir that generally this should be a properly documented API if it's required for having users validated.But as it's only there temporarily, I'm not sure it has to be on the interface really as it might be confusing else. Maybe, we can avoid confusing with a good naming though.
Related, UserInterface::getPassword()/setPassword() is already weird as it is not idempotent. Imho, the password field should be just about the hashed password always and e.g. have an optional property for setting the plain password only. They really are UserInterface::getPasswordHash()/setPassword()
PasswordInterface::check($password, UserInterface $account);
Imho the interface is bad as it requires the user account just for calling getPassword() on it. A better decoupled API would just require one to pass the password and the stored hash. But that's off-topic for this issue.
Given we already have a PasswordItem now, couldn't we use that and keep track of when the password has been updated? I.e., it could internally separate a newly set unhashed password and an existing hashed password. Then both passwords can be there (and accessible via properties) until the user entity is saved. Upon save we can use the insert() and update() methods of the field to remove the plain text password and update the hashed password (or skip the update if it's the same password). Given that, doing the validation constraint would be simple as we have both the hash and the new password available.
Proposed naming:
$user->setNewPassword()
$user->getNewPassword() (documented to be empty unless a new password is to be set)
$user->getPasswordHash()
$user->setPasswordHash()
$user->pass->value (hash)
$user->pass->new_password (plain text)
Thoughts?
Comment #34
fagoThinking more about this, I figured the following:
1. The problem of my above suggestion is that it's completely non-intuitive to have to set a *new* password being the existing password for being able to change things like mail addresses. It's really three things you can set: new passwords, the existing one for the check and the password hash. So what about the following:
$user->setExistingPassword() (plain)
$user->checkExistingPassword() (no reason for a getter, all we need is checking right? :)
$user->getNewPassword()
$user->setNewPassword()
$user->getPasswordHash()
$user->setPasswordHash()
$user->pass->value (hash)
$user->pass->new (plain text, only set if changed)
$user->pass->existing (plain text, only set if provided.)
Then, I think it would make sense to have PasswordItem::checkExistingPassword(), which can be used by $user->checkExistingPassword() and thus by the validator.
2. I'm not sure it's a good idea to run this checks on user validation by default. Yes, it should be done for regular form submissions and REST requests, but in other use cases it's undesired behaviour. Consider importing data, e.g. with migrate, then you might just check whether the imported, trusted data is valid - but you will always get the security violation error and there is no way to make it pass (as you don't know the password).
The patch already runs into this problem and adds
_skipProtectedUserFieldConstraint
for that. However, that's not a proper API either and this really seems to be something people would run into and need to me, thus should be a proper API.So I was thinking whether we could leverage validation groups for that? Symfony validator has that, but we did not use it so far (and probably have to fix some small things to make it work in our integration). So we could bind the ProtectedUserFieldConstraint to a different validation group and pass that group by default.
E.g. we could add an optional $groups parameter to the validate() methods, defaulting to
['Default', 'Strict']
That way you can opt-out of strict validation by passing ['Default'] in case you have use-cases like data imports, while the (suggested) default could be['Default', 'Strict']
?if (!$account->isNew() && $account->id() == \Drupal::currentUser()->id()) {
This seems to help already, but the problem for newly imported users would still be there afaik. Probably migrate does not check data integrity for now? Even if not, other imports might want to?
Comment #35
BerdirI need to read this again, but it does sound interesting.
I don't think there's a big issue with running this by default:
- Migrate doesn't actually validate. By definition, migrations can be partial, e.g. you always import the base fields first, and then configurable fields later, and there's nothing that you can do if there is invalid data anyway. We defined a while ago that migrate doesn't do validation, at least not the default entity destinations
- I doesn't actually apply to new accounts, obviously, so it would only be an issue when updating existing users
- The validation only runs if the user being saved is the current user, which should happen seldom to never in anything except form/rest or conceptually similar things
- For special use cases, there is the flag to prevent it, and if we need to make that more public, a method might fit more or less well into that set of suggested methods.
I'm not sure if the existing password should be available or not. Maybe not through an explicit method, but there *are* use cases to access it, when saving it, there's a module that saves encryped passwords instead of hashes, or you might need to send it to some service, because that needs to be kept in sync, or things like that. So making sure that it is kept available through the process of saving a user might actually be a good idea, but if we have a password field type that can theoretically do that, and your suggestion would be perfectly capable of that, then we can argue (fight?) over that in another issue that is a not a critical :)
Comment #36
fagoAgreed. That flag is somewhat ugly, but I agree should do the job. Separating default vs strict constraint would be a nice to have at this point and not required, as you are right that this behaviour should be fine to have by default.
Yeah, so I think this would be solvable with having
$user->pass->existing (plain text, only set if provided.)
to *set* the existing password. We could unset it immediately afterwards even, i.e. make it write-only - not sure it wouldn't be weird? So maybe we just leave it set until the save operation has ended?
If we leave it there, it already would allow accessing the password for other use cases well. I'd rather avoid putting it on UserInterface though, but yeah that could be discussed elsewhere. First question, should it be readable at all? Even if not, other modules could plug-out the password field to do their job.
Comment #37
cpj CreditAttribution: cpj commented@fago
We have REST use-cases where we need to do the sort of things hinted at by @berdir in #35:
For example, we receive user updates via REST and need to pass them on to our SSO server, which passes back values to be saved. I'm not 100% clear what's being proposed here, but I'm hoping
means I can still access and override the raw data.
Comment #38
BerdirAfter reading through @fago's comment again, I still don't see how to get the existing password through the REST process with that kind of password field.
Here are the requirements of this:
a) It must be a field, because EntityResource can only copy field changes
b) *If* a field is copied, it replaces the existing values completely.
c) The rest client *never* knows the existing hash.
Now, if you would just send 'existing' password field property over REST, then a new user entity is created with *just* ->pass->existing set, no existing hash, this is copied to the existing user, now we no longer have the existing hash available, only the existing unhashed password.
Which means we can *not* validate the existing password, without loading the unchanged user object again.
Given how EntityResource::patch() works, only see two things that work:
- A standalone field for the existing password.
- Introducing a hook/event/hardcoded special case so that we can somehow work around this limitation and e.g. copy the hash from the existing to the user entity that came from the serializer.
Comment #39
klausiThe ProtectedUserFieldConstraintValidator loads the unchanged user object, then call PasswordInterface::check() with $user->pass->existing and $user_unchanged, correct? That way we could handle all this with an "existing" property on PasswordItem.php (better names for "existing" welcome).
The $user->save() call after validation will invoke the preSave() method on the user entity, which populates $user->pass->value with the existing hash if it is empty, so we won't override anything with an empty password as far as I can see.
Comment #40
BerdirWe'd have to specifically implement re-hashing based on the existing password to support REST, normally you'd only want to hash for a *new* password?
But you're right, we load the unchanged account to verify the password, so that part should work.
Comment #41
klausiYou mean if a REST client sends the existing hash in $user->pass->value? Then the new hash value is equal to the old one and preSave() will not hash it. Or what do you mean by re-hashing to support REST?
Comment #42
BerdirA rest client can not send the existing hash, because we never tell him that. And because he does not, we don't have the existing hash, we have no choice but to add a special case somewhere to set the hash based on the existing passwort (instead of ->value as it is now, aka ->new, as it would be with @fago's suggestion.
Comment #43
klausiI don't think so: if the REST client supplies a password value then it will be hashed automatically in User::preSave(). If the REST client does not send a password value then the hash will be populated automatically in User:preSave().
We only need the existing hash in ProtectedUserFieldConstraintValidator - and there it will be used from the loaded unchanged user account object. What am I missing?
Comment #44
BerdirThe point is that the rest client *does* send a value for the password field, but not the hash.
PasswordItem would have 3 properties:
- value: the hash
- existing: computed, temporarily set during submit for validation
- new: a new password, also computed
A rest client would then just send existing, it can not send the existing hash. In that case, we end up with:
- value: NULL
- existing: mypass
- new: NULL
if he wants to change the email.
Now, what would User::preSave() do?
Comment #45
BerdirI'm in #drupal-contribute if you want to discuss this, but I have a call first.
Comment #46
fagoWhatever User::preSave() does I think we should remove the logic from User::preSave() and move it to the PasswordItem. Now as we have it, it just makes not much sense on the Entity and helps devs understanding the flow.
Yeah, I still think that's the set of properties we should have. That way we can clearly distinguish all cases:
- create user:
set "new" pass
- change pass:
set existing pass + new pass
- change mail
set existing pass + mail
So, if there is no 'new' or 'existing' pass we can just keep the existing password hash in 'value'?
Comment #47
klausiFine with me. I think we could make it work with just 2 properties where "value" contains the new password. But yeah, "value" containing sometimes a hash and sometimes a plain text password is confusing, so 3 properties are probably better.
Comment #48
BerdirDiscussed this a bit further with @fago, we said that an empty hash value is already supported for the current UI (and is then automatically set back to the original password) and while we shouldn't do that, it helps us here too.
After starting, I noticed that most of the suggested changes are not needed to solve the problem here I think (all the new password stuff), so I tried to change as little as possible for now. Assuming it actually can be solved, but we're kind of tripping over our own feet at the moment.
Work in progress patch, and problems that I have with this now:
- checkExistingPassword() is weird, yes, the validator uses the existing account, but what is that method supposed to use then? obviously it can't use itself, but then we either have to load it again or pass the existing account in, as I did for now. Weird API...
- while preSave() might be able to deal with it, the user pass *is* a required field, and sending an empty value now fails our own validation. I really don't see a way around this atm.
- We can't make those additonal fields computed, becaue there is no API to get them out then, neither getValue() or toArray() works, and we removed the additional arguments to also get computed values. I guess computed at this point doesn't mean the same as non-stored. It literally means, value is computed based on other properties... ?
- Yes, pass sometimes being the hash and sometimes the actual password is strange, but so far, we do not have anything like the suggested PasswordItem field type, which basically has input and output properties, and while a new value is supposed to be set to ->new or ->existing, depending on the meaning, what is read is usually the hash. Unless we start to add magic for this in setValue(), this is going to break lots and lots of existing calls that create users with a password. I'm not sure that's worth it.
So far, the only good change in here IMHO is the rename from current password to existing password, which is much clearer.
Comment #52
yched CreditAttribution: yched commentedSorry for being a spectator here, this is largely above my head atm :-/
Not sure what would be needed for a "computed field" approach to work here, but maybe that could be part of the discussion in #2392845: Add a trait to standardize handling of computed item lists ?
Comment #53
Berdir@yched: computed fields actually work here, computed properties are tricky in this case :) so that issue will not help much, but I know you'd like to discuss it ;)
Comment #54
xjmThis issue was discussed with @catch, @alexpott, @webchick, @xjm, and @effulgentsia on Feb. 5, and the branch maintainers confirmed that it is critical.
Comment #55
klausiThe current user should be injected, same as the user storage.
Should be "Sets the existing plain text password."
Same here, we should mention plain text to avoid confusion with hashes.
"current" should be "existing" here for consistency, right? Also I would say "Checks that the provided plain text password matches the stored hash." (hope that is under 80 characters)
And we need unit tests for the constraint validation.
The user password is only a required field for new user accounts (user creation). It is not a required field for updating user accounts. Not sure where you run into problems, we have not defined it as required in User::baseFieldDefinitions()?
Otherwise I think this is on the right track.
Comment #56
Berdir"'Unprocessable Entity: validation failed. pass.0.value: This value should not be null."
the pass is not required. But ->value inside is required, so if pass is set, then ->value is required. We override that now, so I could make it not required I guess, but then we are missing validation for it to be required if the user is new I guess then. But it seems that we are already missing that now.
Comment #57
klausiYep, we should add a validation constraint on the password field which makes it required for $entity->isNew() and not required for existing users. Not sure if we should set it in the property level in PasswordItem.php or on the field level in User.php.
Maybe we already have such a "RequiredIfNew" constraint somewhere?
Comment #58
fagoI see - I agree that's there no point in doing it if it doesn't make solving this easier.
I was thinking about that also, and thought that a good solution might be to implement checkExistingPass() on the field, which has a method for password-massaging and does the massaging before checking also?
We use t() everywhere else, shouldn't we keep that consistent?
Could use a description to clarify how it works.
I think those trim() calls should be in the code processing form submissions and not here? But, seems to be out of scope for this.
Needs to be updated to existing.
Comment #59
BerdirOk, this passes the tests, by making the value property not required.
> I was thinking about that also, and thought that a good solution might be to implement checkExistingPass() on the field, which has a method for password-massaging and does the massaging before checking also?
I don't think that is not an option. We need to support sending a *new* password as well, we can' really on the currently set value being the existing password hash.
Addressed some of the review, still needs more tests and a PasswordIsRequiredIfNew constraint.
> We use t() everywhere else, shouldn't we keep that consistent?
No, we don't. This is copied from StringItemBase. This is why I was hoping translation of the description issue would get rid of it ;) See #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries and #2288123: Basic authentication broken for non-english sites. I think it's just luck we're not running into more issues right now, which is why we need #2418521: Translating field definition descriptions problematic due to early t(), hard to test.
> Could use a description to clarify how it works.
Probably, but I'm not sure what to write. It's something rules/search_api would also show, so it shouldn't be too technical.
> trim()
Yeah, just moving code around :)
Comment #60
YesCT CreditAttribution: YesCT commented#1696648-109: [META] Untie content entity validation from form validation made this issue a child of that.
Comment #61
klausiThis should be enabled for testing again.
Only white space changes, they should be removed.
current user should be injected.
should be "The unchanged user entity to compare against."
All just minor stuff. More important is the test coverage for the new constraint.
Now that the value property of PasswordItem is not required anymore does that mean that we don't need the "RequiredIfNew" constraint to solve this critical?
Comment #62
dawehnerWorked on the issue summary.
Comment #63
nlisgo CreditAttribution: nlisgo commentedThis is a re-roll of #59 as core/modules/user/src/AccountForm.php had changed.
I have also addressed points 1, 2 and 4 in #61.
Point 3 of #61 still needs addressing so I will mark this as needs work even if the test run passes.
Comment #65
nlisgo CreditAttribution: nlisgo commentedI've read through the comments and I can't find any useful explanation of why the testPatchUpdate was altered to not run by renaming to dtestPatchUpdate. The first appearance of this is in #48. @Berdir could you shed some light on this for us.
Comment #66
nlisgo CreditAttribution: nlisgo commented@Berdir reached out to me on IRC and said,
So we need to understand why these tests are failing.
Comment #67
BerdirIt fails because of this.
I think the test there is either incorrectly implemented or bogus.
There is no field delete access, just like there is no update/create. field access just cares about edit/view.
So, entity_test_entity_field_access() should check for operation edit, not delete. It doesn't matter if you want to save a value or not (that is controlled by required validation) changing a field is always edit.
Comment #68
nlisgo CreditAttribution: nlisgo commentedThe 3 issues that remain:
Comment #69
martin107 CreditAttribution: martin107 commentedResolved.
Will try and work on this some more next week.
( Also corrected a trivial double blank line )
Comment #70
nlisgo CreditAttribution: nlisgo commentedTriggering testbot
Comment #72
martin107 CreditAttribution: martin107 commentedjust want to see what testbot thinks.
Comment #73
martin107 CreditAttribution: martin107 commentedThat knocking that you can hear is the sound of my head on my desk....
The interdiff skips my last patch and starts from the last known good #63
Comment #75
nlisgo CreditAttribution: nlisgo commented@Berdir in comment #67 you refer to tests that have been removed in patches on this issue before I began work on this.See patch and interdiff at #48.
I misunderstood comment #67. There are no tests in the referred code either.
@Berdir clarified on IRC that
Comment #76
BerdirWell, yes and no.
Nothing changed, the password is as required/not required as before (meaning, it is not validated to be required for new users). It's not really related to the thing we're fixing here and it is possibly not a critical issue to change it to work correctly because while there's no validation, there is an exception that is thrown when it is empty, so it is not possible to create users without a password, but it's not done properly. Maybe a non-critical follow-up to add proper validation for that?
Here's an update with the tests fixed.
I still don't like the API around checkExistingPassword(), I'd rather have getExistingPasword() and document it than this.
Comment #78
BerdirFixing tests for real.
More weird stuff, was having trouble because it was running access after changing the values. That means it didn't work for delete and it is also inconsistent with how we check access for the UI. So I changed that, but then had to update a test that was somehow relying on this to work.
Comment #79
klausiThat change does not make sense, suddenly we check the text format? Why? So the test case was wrong before? Could you change the message to "Text format was not updated." in this case?
Comment does not match the code.
Indentation error, I think the last closing ");" should be one level to the left.
I like User::checkExistingPassword() more than User::getExistingPasword() because it does not suggest that you can get the plain text password of a user, which developers should never do.
The biggest piece missing now is a unit test checking all the variations in ProtectedUserFieldConstraintValidator::validate():
* Test $account->_skipProtectedUserFieldConstraint
* Test $account->isNew()
* Test $account->id() != $this->currentUser->id()
* Test with pass field with correct password
* Test with pass field with incorrect password
* Test with email field with correct password
* Test with email field with incorrect password
* Test empty pass field
Comment #80
Berdir1. Before you changed the value and format together but used the value to verify. That is no longer the case because we first have to set the value back to something we can write and then we only update the format, so we need to check the format. Which makes more sense anyway, because that's what we are testing? changing the message makes sense, maybe 'Format of text field was not updated'?
2. Yep, probably copy & paste, just "Send a different email to trigger the protected user fields constraint." should be enough?
I know you do. But it's extremely confusing that you're required to pass in a user entity that has the current password hash when you are calling the method on the user entity already? We can't solve that problem, because the user we're asking it on might be changing his password too, so we can't use that hash, ever. I'd rather have getExistingPassword() than this.
On tests, Do you really mean unit tests? Those might not be easy to write, while kernel tests in the existing user validation test should be fairly easy.
Comment #81
klausiI would not insist on PHPUnit tests, but they certainly make sense here to write. Since we now inject all the stuff this is perfect for unit tests. Sure, you would have to mock a couple of objects, but that should not be a hard problem for someone who wrote a PHPUnit test before :-)
Comment #82
jhedstromI'll work on some additional tests.
Comment #83
jhedstromI started down the PHPUnit test approach. The thing I dislike about this style of testing the internals, is that it ties the tests extremely closely to the internal logic of the method.
Once I got to testing the actual password field changes, I got confused by this part of the patch:
This appears to do the opposite of ignoring empty, rather it ignores the password field if it has a value. Furthermore, in the list of tests in #79, there is nothing specific to the mail field in this logic either, so any field requires a password if the value has changed.
Comment #85
jhedstromI finished the PHPUnit approach since it was close. I started looking at a kernel test instead, but couldn't find other examples of directly testing these validators with kernel tests.
There was a logic issue in the phpunit implementation in #83 since mocks don't by default return variables (the
__get()
method is mocked to return null). Given that, I changed the direct access of the value variable to usegetValue()
instead.Comment #87
BerdirThat doesn't work in PHP 5.4 :)
Did you try $items->isEmpty()? That should be the same and is easy to mock.
Comment #88
jhedstromThis patch switches to use the
isEmpty()
method as suggested. I've also added more comments to the test to make each case more clearly defined.Comment #90
jhedstromApparently
isEmpty
isn't properly reflecting the current value. I'm digging into this, but may end up just going back to either!empty($items->value)
or$items->getValue()
.Comment #91
jhedstromSwitching back to
$items->value
gets things working again. I had to split that into a separate line though since I don't think PHPUnit was able to mock the __get method from withinempty()
call.I split the test cases out to a provider for easier debugging.
Comment #92
jhedstromComment #94
jhedstromThe changes in #2431329: Make (content) translation language available as a field definition make the additions here to the
UpdateTest::testUpdateUser()
fail, but I can't immediately see why.LogicException: The default translation flag cannot be changed (x-default)
Comment #95
plach@Berdir pointed me to this issue: I had the same problem in #1867518: Leverage entityDisplay to provide fast rendering for fields and fixed it there. I guess we could just copy the solution from over there and whichever gets in first wins. There's also test coverage so it should be ready to go.
Comment #96
klausiklausi opened a new pull request for this issue.
Comment #97
klausiChanged that on line from #1867518: Leverage entityDisplay to provide fast rendering for fields, now the REST update tests should pass. Since that issue is also critical we don't need to copy the test cases from over there and keep this to a minimum here.
Comment #98
fagoReviewed patch again and addressed simple things on the way - updated interdiff attached:
- We do not use the constraint suffix in constraint plugin ids, we only have it for (some) classes. Thus removed it there.
- Fixed constraint message to leverage regular t() % replacements.
- Fixed a few small comment glitches.
Yes, exactly! :-/ And as a consequence it's a pain to change any little detail - but anyway, great work on all combinations there!
Yeah, neither option is nice. Still, I think checkExistingPassword() is the better trade-off as it avoids any confusion just from looking at the list of method names, e.g. while auto-completing them.
So if the not so nice checkExistingPassword() is acceptable - this seems to be good to go now.
Comment #99
fagoComing back to the test issue, I looked up how symfony validator does it. It has a AbstractConstraintValidatorTest we could simply rely on as well, what should be straight forward and requires less coupling on validators internals at least. Besides that, we probably should extend test coverage to the violation message and the replacement. Then, imo it shouldn't mock the constraint class as it belongs to the thing that's tested? Symfony, doesn't mock that either.
Comment #100
rteijeiro CreditAttribution: rteijeiro commentedComment #101
BerdirUsing the symfony base class would mean that we can't use ours. While that should work, we might lose some stuff, a trait would be nicer ;)
I still think that checkExistingPassword() is highly confusing, but I won't fight it. Let's see what the committers have to say about it.
I think we should open a follow-up for the correct validation of the user password (it does throw an exception right now, but it's not properly validated first, so not critical, but it could be nicer).
Other than that, agreed that it is complete.
Comment #102
klausiI reviewed the PHPUnit tests and they seem to cover all cases. Overall I think we are almost finished here.
Message should be changed to "Text format was not updated."
Should be "... for the 'user' entity type."
Comment is wrong, no UUID here.
I think this should only be done if the password is NOT empty. That might also be the reason why $items->isEmpty() did not work for jhedstrom above, because this will create a field item. It is a bit confusing to me that the usual PHP empty() works differently than ->isEmpty() on field items. But on the other hand we are creating a field item here with an empty string, so the filed item list is not empty in some sense. I'm looking forward to interesting security vulnerabilities in D8 where developers expect ->isEmpty() to work the same way as PHP's empty(). Do we have an issue or doc page for this difference in the Entity Field API?
Should be "Checks if the plain text password was provided for editing a protected user field." (hope that is under 80 chars).
indentation error for the closing ");"
I agree with fago that we should use the actual constraint class here, we don't need the mock.
Then we also need a change record draft:
* mention the changes to UserInterface
* explain the changes to update a protected user field over REST
Comment #103
jhedstromI'll work on addressing #102.
Comment #104
jhedstromThis should address #102.
I didn't switch to using Symfony's base testing class yet.
Comment #105
jhedstromThis switches to use Symfony's abstract class for testing constraints.
Comment #106
jhedstromComment #107
klausiLooks good, thanks! Only minor stuff:
why do you need an empty setUp() method that only calls the parent? Please add a comment or remove it.
are we on version 2.5 yet? #2343035: Upgrade validator integration for Symfony versions 2.5+ is not done yet? I think it does not matter for this test case, but TypedDataManager.php still sets API_VERSION_2_4.
Then we also need a change record draft:
* mention the changes to UserInterface
* explain the changes to update a protected user field over REST
Comment #108
jhedstromGood catch. The
setUp
method was left over from various works in progress.Comment #109
klausiGreat, looks good to me now.
The next step is to create the CR draft before this goes to RTBC.
Comment #110
fagoWorking on that.
Comment #111
fagoI added two change record drafts, please take a look:
https://www.drupal.org/node/2460247
https://www.drupal.org/node/2460231
Also, I took another look at the patch and I agree it should be ready so far. I'm still not happy with the amount of mocking going on in the unit test, but that's not something unique to this one.
Comment #112
YesCT CreditAttribution: YesCT commentedWhat follow-ups did people want?
Comment #113
BerdirChange records look good to me, the only follow-up I'm aware of is improving the required-when-new validation of the password field.
@klausi made one small change here, but that was just copying over a fix from another issue, so I think he's best suited to RTBC this. Time to get this in :)
Comment #114
klausi@berdir: let's stop this "I have worked on an issue so I must never RTBC it" nonsense. This issue has lots of agreement from multiple people already on the approach and the patch. Let's trust each other. Let's make core development not harder as it needs to be.
I'm on mobile right now, so cannot review properly, but you certainly have the right to RTBC this.
Comment #117
nlisgo CreditAttribution: nlisgo commentedReroll
Comment #118
cpj CreditAttribution: cpj commented@klausi, @berdir - I've been watching this from the sidelines, and have tested various versions of the code, so I'm pretty much up to speed with what's going on. I've reviewed the latest version of the code and it looks fine to me. So, I'm going to mark this RTBC - OK ?
Comment #119
cpj CreditAttribution: cpj commentedComment #120
fagoThanks, cpj - I agree this is ready. I created the follow-up mentioned by berdir: #2462105: Failed user password hashing message is confusing
Comment #121
alexpottWith this change the existing password can be serialized into a user object in plain text. I'm not sure that we want that do we? I.e. in caches etc.
If you do:
You see the existing password.
Comment #122
BerdirTalked a bit about this with @alexpott. If I understood him correctly, then we can make him happy by adding a __sleep() to PasswordItem and drop the value there. Personally, I'm not convinced that's needed ;)
Comment #123
alexpott@Berdir pointed out that we already have this if a password is set so perhaps it is nothing to worry about or should be explored in a followup. In my mind if an existing password or password is set in clear text on the user object we should mark the object as unsafe for serialization and error if the object is serialized.
Comment #124
fagoI do not think it's possible to drop it. When I consider doing a multi-step workflow for updating the user account where the last step saves the input, while the first step needs password verification, it would loose the password during serialization in step 2 and fail validation afterwards.
Furthermore, the password would be already in serialized form state, so is it really an issue if it is an the user object also? But probably more important is that it's skipped by the symfony serialization system what's already covered generally for PasswordItem by #2338559: Never serialize password fields by default.
Comment #125
YesCT CreditAttribution: YesCT commentedSounds like a follow-up would help. In case it does... #2463113: Plain text passwords can be accidentally dumped to the database by code that doesn't intend to do that
Comment #126
fagoberdir agreed with #124 via IRC:
So I'm setting this back to RTBC such that alexpott can have another look. If we agree on that we should close #2463113: Plain text passwords can be accidentally dumped to the database by code that doesn't intend to do that as "works as designed".
Comment #127
fagoLooking details up, #124 and #126 is not correct in the following:
>Furthermore, the password would be already in serialized form state,...
> the password is around in $form_state values and so on as well, which are serialized too
The password is not in form state as part of the form values - as those are not part of its "cached" version. However, it becomes part of the new user object when changing or creating a user until it's saved (it get's converted to the hash in presave). Thus it is already serialized as part of the user object if it ends up in form state storage for multi-steps or ajax requests on the profile form. So having the plain text password in there sometimes is not something new.
Comment #128
alexpottI've tested to see if we currently serialise a plain text password or the new existing property - we don't see #2095771-100: alexpott's test issue. I think we might not want to ever store a plain text password on a user object the idea of plain text passwords ending up in form state storage gives me nightmares - but lets explore solutions for that in #2463113: Plain text passwords can be accidentally dumped to the database by code that doesn't intend to do that.
This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3c0892d and pushed to 8.0.x. Thanks!
Fixed on commit.
Comment #130
fagoThanks! I've published the change record drafts for this issue.
Comment #131
neclimdulThis is kinda late but should we document we require the intl extension for testing now? The Symfony test class this is extending calls \Locale in its setup method so we by extension require it.
http://php.net/manual/en/class.locale.php
Comment #132
Wim LeersAha! This is causing PHPUnit to fail locally. I thought it was maybe something on my machine.
That is a HUGE and important change!
Comment #133
fagoouch! We missed that so far (at least I did), probably best we just revert back to another test base class. Added a follow-up: #2463879: PHP unit tests fail if intl extension is missing
Comment #135
amateescu CreditAttribution: amateescu for Drupal Association commentedThis issue is tagged with 'D8 upgrade path' but I don't see anything in the patch that would need an update function in the HEAD to HEAD module. Is that correct or am I missing something? :)
Comment #136
klausiNope, nothing to do for HEAD to HEAD module here. I think it was tagged because catch's definition of upgrade path includes any open security issue.
Comment #137
amateescu CreditAttribution: amateescu for Drupal Association commented@klausi, thanks for the confirmation.
Comment #138
pwolanin CreditAttribution: pwolanin commentedI think the fix here is wrong. It should not be directly calling the password service, but should instead be calling the user.auth service which implements the \Drupal\user\UserAuthInterface
Comment #139
pwolanin CreditAttribution: pwolanin commentedFiled a follow-up at #2503113: REST user updates need to validate password using UserAuthInterface to address that problem
Comment #140
klausiLet's leave it at the old status.