Conversation
cbf1133 to
bd189b5
Compare
Rom1-B
left a comment
Rom1-B
left a comment
There was a problem hiding this comment.
Can you add a test covering PluginFieldsContainer::preItemUpdate() directly (e.g. a central-interface user without ticket update right but with write access on the container) asserting _plugin_fields_data is stripped/not persisted?
| if ( | ||
| isset($_SESSION['glpiactiveprofile']['id']) | ||
| && $_SESSION['glpiactiveprofile']['id'] != null | ||
| && $item instanceof CommonITILObject | ||
| && Session::getCurrentInterface() === 'helpdesk' | ||
| && !$item->canRequesterUpdateItem() | ||
| ) { |
There was a problem hiding this comment.
Scoping this to helpdesk + CommonITILObject reopens the bug for central-interface users without update right (e.g. an observer with only READASSIGN). The patch validated on ticket !46589 used !$item->canUpdateItem() unconditionally, matching the field.class.php display-side fix — Ticket::canUpdateItem() already handles both interfaces correctly.
| if ( | |
| isset($_SESSION['glpiactiveprofile']['id']) | |
| && $_SESSION['glpiactiveprofile']['id'] != null | |
| && $item instanceof CommonITILObject | |
| && Session::getCurrentInterface() === 'helpdesk' | |
| && !$item->canRequesterUpdateItem() | |
| ) { | |
| if ( | |
| isset($_SESSION['glpiactiveprofile']['id']) | |
| && $_SESSION['glpiactiveprofile']['id'] !== null | |
| && !$item->canUpdateItem() | |
| ) { |
| * | ||
| * @return boolean | ||
| */ | ||
| public static function preItemUpdate(CommonDBTM $item) |
There was a problem hiding this comment.
This is a rights fix and the PR adds no test, even though tests/Units/ContainerItemRightTest.php already covers this area. A non-regression test should log in as a self-service observer, call $ticket->update([... dom field values ...]) and assert the stored value did not change. A second test should assert that a requester who is still allowed to edit (no followup yet) can save.
| } | ||
|
|
||
| $canedit = $right > READ; | ||
| $canedit = $right > READ && ($item->isNewItem() || $item->canUpdateItem()); |
There was a problem hiding this comment.
The new $canedit rule ($item->isNewItem() || $item->canUpdateItem()) is untested, both here and in showForTabContainer() at line 894. A rendering test should assert that the inputs are read-only for a helpdesk observer and editable on a new item.
6262e88 to
83e4dc2
Compare
| && !$item->canRequesterUpdateItem()) | ||
| ) | ||
| ) { | ||
| unset($item->input['_plugin_fields_data']); |
There was a problem hiding this comment.
Submitted values are dropped silently and the item update still succeeds, so the user gets no feedback. Acceptable since the UI renders fields read-only, but a Session::addMessageAfterRedirect() warning would make any future UI/server mismatch visible.
| 'interface' => 'helpdesk', | ||
| ]); | ||
| // Grant write access on container | ||
| $this->setRightOnContainerForProfile($observer_profile->getID(), $container->getID(), READ); |
There was a problem hiding this comment.
The observer test grants only READ on the container (the comment says "write access"). With READ, $canedit = $right > READ is already false before this PR, so the assertion passes with or without the fix and does not protect against regression. Grant UPDATE (or CREATE | UPDATE) so that canUpdateItem() is the only reason the field is read-only.
Rom1-B
left a comment
Rom1-B
left a comment
There was a problem hiding this comment.
Please address ALL the previous comments before requesting a review again
3 participants
Checklist before requesting a review
Please delete options that are not relevant.
Description
because rights were only checked at the container-profile level, not against the
specific item. Fields are now rendered as read-only for users who cannot update the
item, and the pre_item_update hook enforces the same check server-side.