diff --git a/CHANGELOG.md b/CHANGELOG.md index e1befc3b..c29b56ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] + +### Fixed + +- Fix infinite loop / Gateway Timeout when a ticket's assigned technician and assigned group are changed simultaneously, caused by a synchronous actor removal during `pre_item_update()` + ## [2.10.6] - 2026-07-31 ### Fixed diff --git a/hook.php b/hook.php index 3d33301f..4655b2e2 100644 --- a/hook.php +++ b/hook.php @@ -532,6 +532,22 @@ function plugin_escalade_pre_item_add_group_ticket($item) 'group_id' => $item->input['groups_id'], 'timestamp' => time(), ]; + + // Fallback for group-only reassignment (no new tech to consume the pending flag). + $tickets_id = $item->input['tickets_id']; + if (!empty($_SESSION['plugin_escalade']['pending_remove_assign_users'][$tickets_id])) { + unset($_SESSION['plugin_escalade']['pending_remove_assign_users'][$tickets_id]); + PluginEscaladeTicket::removeAssignUsers($item); + } + } + + return $item; +} + +function plugin_escalade_pre_item_add_ticket_user($item) +{ + if ($item instanceof Ticket_User) { + return PluginEscaladeTicket::pre_item_add_ticket_user($item); } return $item; diff --git a/inc/ticket.class.php b/inc/ticket.class.php index b39f5bc6..c407694d 100644 --- a/inc/ticket.class.php +++ b/inc/ticket.class.php @@ -194,7 +194,8 @@ function ($carry, $type) use ($item) { $item->input['status'] = $_SESSION['glpi_plugins']['escalade']['config']['ticket_last_status']; } - self::removeAssignUsers($item); + // Deferred to pre_item_add (Ticket_User/Group_Ticket): removing here re-enters and loops. + $_SESSION['plugin_escalade']['pending_remove_assign_users'][$item->getID()] = true; } elseif (count($old_groups) === count($new_groups)) { $old_group_ids = []; foreach ($old_groups as $old_group) { @@ -224,7 +225,6 @@ function ($carry, $type) use ($item) { */ public static function item_update(CommonDBTM $item) { - if ($_SESSION['glpi_plugins']['escalade']['config']['remove_group']) { //solve ticket if (isset($item->input['status']) && $item->input['status'] == CommonITILObject::SOLVED) { @@ -268,6 +268,30 @@ public static function item_update(CommonDBTM $item) } } + /** + * Consume pre_item_update()'s pending removal flag before the new tech is inserted (delete-before-add). + * @param Ticket_User $item the actor being added + */ + public static function pre_item_add_ticket_user(Ticket_User $item) + { + if (($item->input['type'] ?? null) != CommonITILActor::ASSIGN) { + return $item; + } + + $tickets_id = $item->input['tickets_id'] ?? null; + if ($tickets_id && !empty($_SESSION['plugin_escalade']['pending_remove_assign_users'][$tickets_id])) { + unset($_SESSION['plugin_escalade']['pending_remove_assign_users'][$tickets_id]); + self::removeAssignUsers($item); + + // Protect this tech from the group's removeAssignUsers() call coming right after. + if (isset($item->input['users_id'])) { + $_SESSION['plugin_escalade']['keep_new_assign_users'][$tickets_id][] = $item->input['users_id']; + } + } + + return $item; + } + /** * When a ticket is solved, if group histories exists, assign the first group on the ticket @@ -536,8 +560,10 @@ function (array $actor) use ($groups_id, &$seen_new_group): bool { self::removeAssignGroups($tickets_id, $groups_id); } - // The config is checked in the function. - self::removeAssignUsers($item); + // Safety net for qualification()'s category-based reassignment, which bypasses pre_item_add hooks. + $keep_users_id = $_SESSION['plugin_escalade']['keep_new_assign_users'][$tickets_id] ?? false; + unset($_SESSION['plugin_escalade']['keep_new_assign_users'][$tickets_id]); + self::removeAssignUsers($item, $keep_users_id); if ($_SESSION['glpi_plugins']['escalade']['config']['ticket_last_status'] != self::MANAGED_BY_CORE) { $ticket = new Ticket(); @@ -736,10 +762,10 @@ public static function removeAssignUsers($item, $keep_users_id = false, $type = return; } - $tickets_id = $item->input['id'] ?? $item->fields['id']; - - if ($item instanceof Group_Ticket) { + if ($item instanceof Group_Ticket || $item instanceof Ticket_User) { $tickets_id = $item->input['tickets_id'] ?? $item->fields['tickets_id']; + } else { + $tickets_id = $item->input['id'] ?? $item->fields['id']; } $where_keep = [ diff --git a/setup.php b/setup.php index 7ee4f485..9fd6a959 100644 --- a/setup.php +++ b/setup.php @@ -114,6 +114,7 @@ function plugin_init_escalade() $PLUGIN_HOOKS['pre_item_add']['escalade'] = [ 'Group_Ticket' => 'plugin_escalade_pre_item_add_group_ticket', 'Ticket' => 'plugin_escalade_pre_item_add_ticket', + 'Ticket_User' => 'plugin_escalade_pre_item_add_ticket_user', ]; $PLUGIN_HOOKS['post_prepareadd']['escalade'] = [ 'Ticket' => 'plugin_escalade_post_prepareadd_ticket', diff --git a/tests/Units/TicketTest.php b/tests/Units/TicketTest.php index d3a305a6..a3f9b249 100644 --- a/tests/Units/TicketTest.php +++ b/tests/Units/TicketTest.php @@ -1459,4 +1459,177 @@ public function testSlaRuleDuringEscalation(): void ); } } + + /** + * Regression test for the Gateway Timeout / infinite loop reported when a ticket's + * assigned technician and assigned group are changed in the same action (ticket #45821). + */ + public function testSimultaneousGroupAndTechAssignmentDoesNotLoop() + { + $this->initConfig([ + 'remove_tech' => 1, + ]); + + $old_tech = new User(); + $old_tech->getFromDBbyName('tech'); + $this->assertGreaterThan(0, $old_tech->getID()); + + $new_tech = new User(); + $new_tech->getFromDBbyName('glpi'); + $this->assertGreaterThan(0, $new_tech->getID()); + + $group = $this->createItem('Group', [ + 'name' => 'Group for simultaneous assignment test', + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + $ticket = $this->createItem('Ticket', [ + 'name' => 'Test ticket', + 'content' => 'Content', + 'entities_id' => 0, + ]); + $ticket_id = $ticket->getID(); + + // Initial state: a technician assigned, no group. + $this->updateItem(Ticket::class, $ticket_id, [ + '_actors' => [ + 'assign' => [ + [ + 'items_id' => $old_tech->getID(), + 'itemtype' => 'User', + ], + ], + ], + ]); + + // Reassign: new group and new technician submitted together. + $this->updateItem(Ticket::class, $ticket_id, [ + '_actors' => [ + 'assign' => [ + [ + 'items_id' => $group->getID(), + 'itemtype' => 'Group', + ], + [ + 'items_id' => $new_tech->getID(), + 'itemtype' => 'User', + ], + ], + ], + ]); + + $this->assertEquals( + 1, + countElementsInTable(Group_Ticket::getTable(), ['tickets_id' => $ticket_id, 'type' => CommonITILActor::ASSIGN]), + ); + $this->assertEquals( + 1, + countElementsInTable(Ticket_User::getTable(), ['tickets_id' => $ticket_id, 'type' => CommonITILActor::ASSIGN]), + ); + $this->assertEquals( + 0, + countElementsInTable(Ticket_User::getTable(), ['tickets_id' => $ticket_id, 'type' => CommonITILActor::ASSIGN, 'users_id' => $old_tech->getID()]), + 'Old technician should have been removed', + ); + $this->assertEquals( + 1, + countElementsInTable(Ticket_User::getTable(), ['tickets_id' => $ticket_id, 'type' => CommonITILActor::ASSIGN, 'users_id' => $new_tech->getID()]), + 'New technician should be assigned', + ); + } + + /** + * Ensures the old-actor removal is deferred to the pre_item_add hook of the new actor + * (Ticket_User/Group_Ticket) and no longer performed synchronously in pre_item_update(), + * which was the cause of the reentrancy loop, while still happening strictly before the + * new actor is added (delete-before-add notification ordering). + */ + public function testOldAssignRemovalIsDeferredUntilNewActorIsAdded() + { + $this->initConfig([ + 'remove_tech' => 1, + ]); + + $tech = new User(); + $tech->getFromDBbyName('tech'); + $this->assertGreaterThan(0, $tech->getID()); + + $group = $this->createItem('Group', [ + 'name' => 'Group for deferred removal test', + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + $ticket = $this->createItem('Ticket', [ + 'name' => 'Test ticket', + 'content' => 'Content', + 'entities_id' => 0, + ]); + $ticket_id = $ticket->getID(); + + $this->updateItem(Ticket::class, $ticket_id, [ + '_actors' => [ + 'assign' => [ + [ + 'items_id' => $tech->getID(), + 'itemtype' => 'User', + ], + ], + ], + ]); + + $ticket_instance = new Ticket(); + $this->assertTrue($ticket_instance->getFromDB($ticket_id)); + + $mock_ticket = $this->getMockBuilder(Ticket::class) + ->onlyMethods(['prepareInputForUpdate']) + ->getMock(); + $mock_ticket->method('prepareInputForUpdate')->willReturnArgument(0); + $mock_ticket->fields = $ticket_instance->fields; + $mock_ticket->input = [ + 'id' => $ticket_id, + '_actors' => [ + 'assign' => [ + [ + 'items_id' => $group->getID(), + 'itemtype' => 'Group', + ], + ], + ], + ]; + + PluginEscaladeTicket::pre_item_update($mock_ticket); + + $this->assertTrue( + !empty($_SESSION['plugin_escalade']['pending_remove_assign_users'][$ticket_id]), + 'pre_item_update() should flag the old assign removal as pending instead of performing it', + ); + $this->assertEquals( + 1, + countElementsInTable(Ticket_User::getTable(), ['tickets_id' => $ticket_id, 'type' => CommonITILActor::ASSIGN]), + 'The old technician must still be assigned right after pre_item_update()', + ); + + // Simulate the new group about to be added: the fallback hook must consume the + // pending flag and remove the old technician before the Group_Ticket row exists. + $group_ticket = new Group_Ticket(); + $group_ticket->input = [ + 'tickets_id' => $ticket_id, + 'groups_id' => $group->getID(), + 'type' => CommonITILActor::ASSIGN, + ]; + plugin_escalade_pre_item_add_group_ticket($group_ticket); + + $this->assertEquals( + 0, + countElementsInTable(Ticket_User::getTable(), ['tickets_id' => $ticket_id, 'type' => CommonITILActor::ASSIGN]), + 'The old technician must be removed before the new group is added', + ); + $this->assertArrayNotHasKey( + $ticket_id, + $_SESSION['plugin_escalade']['pending_remove_assign_users'] ?? [], + 'The pending flag must be consumed once removal is done', + ); + } }