IBX-362: Fixed 'user/password' policy check

[barw4]

Question	Answer
JIRA issue	IBX-362
Bug	yes
New feature	no
Target version	7.5
BC breaks	no
Tests pass	yes
Doc needed	no

Not sure why the content/edit policy was required when changing the password, this one has been removed.

Also, this issue is occurring because of Role limitations like the Subtree one. What is happening is that the "User" content is validated against the Subtree limitation (and it fails because of limitation on Home content for example) which shouldn't be the case for user/* policies as we don't deal with the usual content here. Therefore, the additional target has been added in order to abstain from evaluating this limitation as it doesn't make sense in the case of a user.

TODO:

• Fix a bug.
• Implement tests.
• Fix new code according to Coding Standards ($ composer fix-cs).
• Ask for Code Review.

[gggeek]

What about the opposite: can a content/edit policy be used to alter the password ?

[glye]

I have my doubts that this is a bug at all, but I need more clarity in what the setup is. So: Assuming we have a role containing the user/password policy, assigned with subtree limitation on the Home folder.

Generally, a role assigned by subtree limitation applies when the content in question is located within the given subtree. Changing the password means making a change to the user. The user isn't in the Home subtree, so access is denied. This seems to make no sense for the user/password policy, at first glance.

However, what if I for whatever reason wanted to limit password changing to user group X? Then I might make a role with the user/password policy and assign it with subtree limitation to X. This would work as expected with the current code. After the fix, the role would work for all users, regardless of user group. That doesn't seem right.

Second, important concern: Will this change let people change the password of any other user, through the api? That would obviously be Very Bad™, and must be verified.

We can discuss whether it makes any sense to stop anyone from changing their own password, but as it stands, the role system does allow us to stop that, and I don't like to change that without very serious consideration by core devs. The requirement for content/edit is another thing, it does seem right to drop that, as the earlier fix indicates.

Now, if we take a step back and look at ezsystems/ezplatform-kernel#117 - it says:
Basically, to keep backward compatibility with previous kernel version, updateUser method is not really changed other than refactoring duplicate code. A new updateUserPassword is introduced that has the same arguments as updateUser.
Maybe the only fix we need is to make sure we use updateUserPassword rather than updateUser?

I would suggest some other changes:

• Doc: This should say that user/password must be assigned without limitation, or with a limitation matching the user in question. https://doc.ibexa.co/en/latest/guide/user_management/user_management/#changing-and-recovering-passwords
• Doc: This should not say that the user/password policy is "unused", when it clearly is in use. https://doc.ibexa.co/en/latest/guide/permissions/#available-policies
• Default data: All users should probably be allowed to change their own password by default, so we need a role with the user/password policy, assigned without limitations.

[mikadamczyk]

Removing policy check will be a BC break. I face this issue in https://issues.ibexa.co/browse/EZP-28917 To keep BC allow updating password if the user has content / edit policy. The other thing is we have the same check in updateUserPassword method. So, the bugfix should take care of an issue with the subtree limitation. Because some people could depend on this.

[barw4]

However, what if I for whatever reason wanted to limit password changing to user group X? Then I might make a role with the user/password policy and assign it with subtree limitation to X. This would work as expected with the current code. After the fix, the role would work for all users, regardless of user group. That doesn't seem right.

@glye I can understand the need, but shouldn't we be allowed to set limitations for user/password policy as well in this case? Please note that we are not allowed to put any limitations in the policy itself as there aren't any, whether, for other content-related policies, we do have Subtree limitations and others in the policy form itself. That means the user/password policy was not really prepared to handle any limitations. The user should be able to change his own password at any time if he has this policy. If we want to disable this option we should really remove such a policy from his role.

Second, important concern: Will this change let people change the password of any other user, through the api? That would obviously be Very Bad™, and must be verified.

As @mikadamczyk mentioned, I have brought back the content/edit policy check so that shouldn't occur.

To summarize, after d44d926 the only change that I did was to add additional targets to user/password validation, so the only difference that is now present is that all Limitations are skipped for user/password policy (and looks like it should be the case due to the points I have raised above).

[alongosz]

This can be covered by integration tests most likely

[barw4]

@alongosz test added in a314c4d

[micszo]

QA Approved on eZ Platform EE v2.5.19 with diff.

[lserwatka]

Could you merge it up?

[barw4]

Merged into:
1.2: ezsystems/ezplatform-kernel@6fb537e
1.3: ezsystems/ezplatform-kernel@606a517
master: ezsystems/ezplatform-kernel@49f35c4