REST user updates bypass tightened user account change validation

Can 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 ;)

Ah, 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.

I 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.

I 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.

This 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.

1. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraint.php
   @@ -0,0 +1,85 @@
   +    $account = $this->context->getMetadata()->getTypedData()->getEntity();
   

   Can we use $items->getEntity()?

2. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraint.php
   @@ -0,0 +1,85 @@
   +      $account_unchanged = \Drupal::entityManager()
   +        ->getStorage('user')
   +        ->loadUnchanged($account->id());
   

   This can be injected (see #2197029: Allow to inject dependencies into validation constraints)

3. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraint.php
   @@ -0,0 +1,85 @@
   +  }
   

   Needs a blank line.

poking along

battery getting flat
still needs tests

1. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraint.php
   @@ -0,0 +1,29 @@
   + * Check if plain text password was provided for protected user field.
   + *
   

   Should be "Check if plain text password was provided in order to change a user field."

2. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
   @@ -0,0 +1,94 @@
   +    if (empty($account->_restSubmittedFields)) {
   +      // This isn't a rest-based validation, no need to continue processing.
   +      return;
   +    }
   

   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.

half-assed rest test

Further iterations on halfarsology™

This should be removed in the end, since the form validation should also rely on this?

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.

Working on this.

This 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.

Ok, 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.

Ignoring empty passwords

Yes, there is one test failing due to this not being validated on the username in HEAD.

Uninteresting remarks :

1. +++ b/core/modules/user/src/AccountForm.php
   @@ -387,11 +381,17 @@ public function validate(array $form, FormStateInterface $form_state) {
   +    $account->_skipProtectedUserFieldConstraint = $form_state->get('user_pass_reset');
   

   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

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -514,14 +528,21 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setComputed(TRUE);
   

   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) ?

3. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
   @@ -0,0 +1,85 @@
   +      $account_unchanged = $this->userStorage
   +        ->loadUnchanged($account->id());
   +
   +
   

   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 :-/

1. +++ b/core/modules/user/src/Entity/User.php
   @@ -514,14 +528,21 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['current_pass'] = BaseFieldDefinition::create('password')
   +      ->setLabel(t('Current Password'))
   +      ->setDescription(t('The current password of this user.'))
   +      ->setComputed(TRUE);
   

   Why do we make an "official" field out of this? The current password should be secret and not be used by anyone else.

2. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
   @@ -0,0 +1,85 @@
   +    if (!$account->isNew() && $account->id() == \Drupal::currentUser()->id()) {
   

   Do not use \Drupal here, inject the current_user service instead with the create() static method.

3. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
   @@ -0,0 +1,85 @@
   +      if ($changed && (!$account->getCurrentPassword() || !\Drupal::service('password')->check(trim($account->getCurrentPassword()), $account_unchanged))) {
   

   Same here, password service should be injected.

4. +++ b/core/modules/user/src/UserInterface.php
   @@ -202,4 +202,26 @@ public function block();
   +  /**
   +   * Sets the current password.
   +   *
   +   * Required for validation when changing the password, name or email fields.
   +   *
   +   * @param string $password
   +   *   The current password of the user.
   +   *
   +   * @return $this
   +   */
   +  public function setCurrentPassword($password);
   +
   +  /**
   +   * Returns the current password if set.
   +   *
   +   * @return string|null
   +   *   The current password if previously set.
   +   *
   +   * @see UserInterface::setCurrentPassword().
   +   */
   +  public function getCurrentPassword();
   +
   

   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!

1.

Please read #22 (again) :)

- 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
...
- 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).

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?

Yes, 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.

I 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.

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.

PasswordInterface::check($password, UserInterface $account);

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().

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.

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.

I 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 :)

@fago

We have REST use-cases where we need to do the sort of things hinted at by @berdir in #35:

or you might need to send it to some service, because that needs to be kept in sync, or things like that

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

Even if not, other modules could plug-out the password field to do their job.

means I can still access and override the raw data.

After 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.

The 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.

We'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.

You 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?

A 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.

I 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?

If the REST client does not send a password value then the hash will be populated automatically in User:preSave().

The 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?

I'm in #drupal-contribute if you want to discuss this, but I have a call first.

Fine 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.

Discussed 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.

Sorry 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 ?

@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 ;)

This issue was discussed with @catch, @alexpott, @webchick, @xjm, and @effulgentsia on Feb. 5, and the branch maintainers confirmed that it is critical.

1. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
   @@ -0,0 +1,85 @@
   +    if (!$account->isNew() && $account->id() == \Drupal::currentUser()->id()) {
   

   The current user should be injected, same as the user storage.

2. +++ b/core/modules/user/src/UserInterface.php
   @@ -202,4 +202,29 @@ public function block();
   +   * Sets the existing password.
   

   Should be "Sets the existing plain text password."

3. +++ b/core/modules/user/src/UserInterface.php
   @@ -202,4 +202,29 @@ public function block();
   +   * @param string $password
   +   *   The existing password of the user.
   

   Same here, we should mention plain text to avoid confusion with hashes.

4. +++ b/core/modules/user/src/UserInterface.php
   @@ -202,4 +202,29 @@ public function block();
   +   * Checks the current password if set.
   

   "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.

"'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.

Yep, 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?

Ok, 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 :)

#1696648-109: [META] Untie content entity validation from form validation made this issue a child of that.

1. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -27,7 +27,7 @@ class UpdateTest extends RESTTestBase {
   -  public function testPatchUpdate() {
   +  public function dtestPatchUpdate() {
   

   This should be enabled for testing again.

2. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   index eec36d9..71b367d 100644
   --- a/core/modules/serialization/src/Normalizer/NullNormalizer.php
   
   --- a/core/modules/serialization/src/Normalizer/NullNormalizer.php
   +++ b/core/modules/serialization/src/Normalizer/NullNormalizer.php
   

   Only white space changes, they should be removed.

3. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
   @@ -0,0 +1,85 @@
   +    if (!$account->isNew() && $account->id() == \Drupal::currentUser()->id()) {
   

   current user should be injected.

4. +++ b/core/modules/user/src/UserInterface.php
   @@ -202,4 +202,29 @@ public function block();
   +   * @param \Drupal\user\UserInterface $account_unchanged
   +   *   The unchanged user entity compare against.
   

   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?

Worked on the issue summary.

This 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.

I'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.

@Berdir reached out to me on IRC and said,

there is no reason. It's just a trick to run a test faster by disabling all but the method you care about. And then forgetting to revert that again.

So we need to understand why these tests are failing.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -145,11 +145,8 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-      if ($field->isEmpty() && !$original_entity->get($field_name)->access('delete')) {
-        throw new AccessDeniedHttpException(String::format('Access denied on deleting field @field.', array('@field' => $field_name)));
-      }

It 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.

The 3 issues that remain:

• Address the failing tests - see #67
• Introduce current user with dependency injection - see #61 point 3.
• Add test coverage for the new constraint - see #61

Introduce current user with dependency injection - see #61 point 3.

Resolved.

Will try and work on this some more next week.

( Also corrected a trivial double blank line )

Triggering testbot

just want to see what testbot thinks.

That 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

@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

entity_test_entity_field_access() should be changed from operation delete to operation edit.

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?

Well, 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.

Fixing 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.

1. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -123,7 +126,7 @@ public function testPatchUpdate() {
        // Re-load the entity from the database.
        $entity = entity_load($entity_type, $entity->id(), TRUE);
   -    $this->assertEqual($entity->field_test_text->value, 'no delete access value', 'Text field was not updated.');
   +    $this->assertEqual($entity->field_test_text->format, 'plain_text', 'Text field was not updated.');
   

   That 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?

2. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -160,4 +163,71 @@ public function testPatchUpdate() {
   +    // Try to send invalid data to trigger the entity validation constraints.
   +    // Send a UUID that is too long.
   +    $account->set('mail', 'new-email@example.com');
   +    $context = ['account' => $account];
   

   Comment does not match the code.

3. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
   @@ -0,0 +1,98 @@
   +    return new static(
   +      $container->get('entity.manager')->getStorage('user'),
   +      $container->get('current_user')
   +      );
   

   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

1. 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 like User::checkExistingPassword() more than User::getExistingPassword() because it does not suggest that you can get the plain text password of a user, which developers should never do.

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.

I 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 :-)

I'll work on some additional tests.

I 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:

+++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
@@ -0,0 +1,98 @@
+      // Special case for the password, it being empty means that the existing
+      // password should not be changed, ignore empty password fields.
+      if ($field->getName() != 'pass' || !empty($items->value)) {
+        // Compare the values of the field this is being validated on.
+        $changed = $items->getValue() != $account_unchanged->get($field->getName())->getValue();
+      }

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.

I 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 use getValue() instead.

+++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
@@ -85,7 +85,7 @@ public function validate($items, Constraint $constraint) {
       // password should not be changed, ignore empty password fields.
-      if ($field->getName() != 'pass' || !empty($items->value)) {
+      if ($field->getName() != 'pass' || !empty($items->getValue())) {
         // Compare the values of the field this is being validated on.

That doesn't work in PHP 5.4 :)

Did you try $items->isEmpty()? That should be the same and is easy to mock.

This 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.

Apparently 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().

Switching 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 within empty() call.

I split the test cases out to a provider for easier debugging.

The 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)

@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.

klausi opened a new pull request for this issue.

Changed 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.

Using 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.

I reviewed the PHPUnit tests and they seem to cover all cases. Overall I think we are almost finished here.

1. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -123,7 +126,7 @@ public function testPatchUpdate() {
   -    $this->assertEqual($entity->field_test_text->value, 'no delete access value', 'Text field was not updated.');
   +    $this->assertEqual($entity->field_test_text->format, 'plain_text', 'Text field was not updated.');
   

   Message should be changed to "Text format was not updated."

2. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -160,4 +163,71 @@ public function testPatchUpdate() {
   +  /**
   +   * Tests several valid and invalid update requests for 'user' entity type.
   +   */
   

   Should be "... for the 'user' entity type."

3. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -160,4 +163,71 @@ public function testPatchUpdate() {
   +    // Try to send invalid data to trigger the entity validation constraints.
   +    // Send a UUID that is too long.
   +    $account->set('mail', 'new-email@example.com');
   +    $context = ['account' => $account];
   

   Comment is wrong, no UUID here.

4. +++ b/core/modules/user/src/AccountForm.php
   @@ -377,6 +371,9 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +
   +    $account->setExistingPassword($form_state->getValue('current_pass'));
   +
   

   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?

5. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraint.php
   @@ -0,0 +1,29 @@
   +/**
   ...
   + *
   

   Should be "Checks if the plain text password was provided for editing a protected user field." (hope that is under 80 chars).

6. +++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
   @@ -0,0 +1,99 @@
   +    return new static(
   +      $container->get('entity.manager')->getStorage('user'),
   +      $container->get('current_user')
   +      );
   

   indentation error for the closing ");"

7. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidatorTest.php
   @@ -0,0 +1,310 @@
   +    $this->constraint = $this->getMock('Symfony\Component\Validator\Constraint');
   

   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

I'll work on addressing #102.

This should address #102.

I didn't switch to using Symfony's base testing class yet.

This switches to use Symfony's abstract class for testing constraints.

Looks good, thanks! Only minor stuff:

1. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidatorTest.php
   @@ -7,31 +7,35 @@
   +  protected function setUp() {
   +    parent::setUp();
   +  }
   

   why do you need an empty setUp() method that only calls the parent? Please add a comment or remove it.

2. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidatorTest.php
   @@ -7,31 +7,35 @@
   +  protected function getApiVersion() {
   +    return Validation::API_VERSION_2_5;
   +  }
   

   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

Good catch. The setUp method was left over from various works in progress.

Great, looks good to me now.

The next step is to create the CR draft before this goes to RTBC.

What follow-ups did people want?

Change 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 :)

@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.

Reroll

@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 ?

With 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:

$user = \Drupal\user\Entity\User::load(1);
$user->setExistingPassword('blah');
print serialize($user);

You see the existing password.

Talked 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 ;)

@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.

Sounds 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

I'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!

diff --git a/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
index 1efb91f..d99fc5d 100644
--- a/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
+++ b/core/modules/user/src/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidator.php
@@ -6,7 +6,6 @@
 
 namespace Drupal\user\Plugin\Validation\Constraint;
 
-use Drupal\Component\Utility\String;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
 use Drupal\Core\Session\AccountProxyInterface;
 use Drupal\user\UserStorageInterface;

Fixed on commit.

This 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

This 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.

Aha! This is causing PHPUnit to fail locally. I thought it was maybe something on my machine.

That is a HUGE and important change!

This 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? :)

Nope, 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.

@klausi, thanks for the confirmation.

I 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

Filed a follow-up at #2503113: REST user updates need to validate password using UserAuthInterface to address that problem

Let's leave it at the old status.