Fix(User): fix default group / profile / entity security #16880
Conversation
stonebuzz
requested review from
AdrienClairembault,
trasher,
orthagh and
cedric-anne
There was a problem hiding this comment.
The code will be executed only if the rules evaluated. Indeed, it is located in the applyRightRules() method and has the following specific condition now:
if (
isset($this->fields['_ruleright_process'])
|| isset($this->input['_ruleright_process'])
) {
Also, it is now executed from the post_updateItem() method, so it would not block a default profile/default entity update that would be made from the UI.
(note this condition has been changed in bugfixes branch recently) |
There was a problem hiding this comment.
After reading a second time the PR, I do not understand what the fix is. Your are triggering a new update that was not present before. The security checks are not supposed to be used to trigger a new update, but to prevent potential updates to be done.
If there are some problematic updates done during LDAP data sync, then they should probably be blocked.
src/User.php
Outdated
| if (!in_array($input['profiles_id'], Profile_User::getUserProfiles($input['id']))) { | ||
| unset($input['profiles_id']); | ||
| // Security on default profile update | ||
| if (!isset($input['_ruleright_process'])) { |
There was a problem hiding this comment.
The input should be unset even if there are rules to process. Indeed, the user update will be done before the rules processing and we do not want these fields to be updated if tested conditions are met.
The check on the default
entity/profileandgroupdoes not take account of data after the rules (LDAP/Right)This PR fix this