From 51073fba1e1affb9a0864b794d57379cb6884cdf Mon Sep 17 00:00:00 2001 From: central-worker-4 Date: Wed, 21 Oct 2026 06:22:24 -0700 Subject: [PATCH] #5002: the orphan recovery goes both ways MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4806 gave a site admin one write on a help desk group nobody is left to run, and #4926 made the Person field of that form a people search. Neither closed the other half: `storeAgent()` was gated on requireManagerAccessOrOrphanRecovery(), `destroyAgent()` on plain requireManagerAccess(). So the rescue could appoint and never un-appoint — the row it had just written sat in the rescuer's own Agents list marked "Not yours to manage", with no Remove control and a 403 behind the DELETE. A `manager` press was worse: the write ends the orphan state, so the group left `orphanedGroupIds` and the admin could not remove the row, appoint anybody else, or appoint themselves. The only person who could undo it was the person wrongly appointed — #4806's frozen group, reached through #4806's cure. destroyAgent() now asks requireRemoveAccess(): a manager of the group, or a site admin while the group is STILL orphaned (symmetric with the write and strictly narrower — on such a group they could appoint themselves and then remove anything anyway), or a site admin undoing a row their own rescue wrote. That last arm needs provenance, hence `recovered_by`: "the group would be orphaned without this row" cannot tell a mistaken appointment from the sole working manager of a healthy group, and admitting the second would let any admin strip a manager and seize the desk (#4710). The stamp is cleared the moment a manager of the group writes the row themselves. `removableAgentIds` is that gate in prop form, so the Remove button and the DELETE cannot disagree, and the confirmation says when a removal hands the group back to having nobody. Co-Authored-By: Claude Opus 5 (1M context) --- ...recovered_by_to_ticketing_agent_access.php | 43 +++++++ .../TicketingSettingsController.php | 119 ++++++++++++++++-- src/Models/TicketingAgentAccess.php | 8 +- 3 files changed, 161 insertions(+), 9 deletions(-) create mode 100644 database/migrations/2026_10_21_000001_add_recovered_by_to_ticketing_agent_access.php diff --git a/database/migrations/2026_10_21_000001_add_recovered_by_to_ticketing_agent_access.php b/database/migrations/2026_10_21_000001_add_recovered_by_to_ticketing_agent_access.php new file mode 100644 index 0000000..06022e0 --- /dev/null +++ b/database/migrations/2026_10_21_000001_add_recovered_by_to_ticketing_agent_access.php @@ -0,0 +1,43 @@ + rescued the group, and + * it is theirs to undo". No foreign key — `user_id` on this table has none + * either, and a deleted rescuer should leave the row alone, not take it. + */ +return new class extends Migration +{ + public function up(): void + { + Schema::table('ticketing_agent_access', function (Blueprint $table) { + $table->unsignedBigInteger('recovered_by')->nullable()->after('role'); + }); + } + + public function down(): void + { + Schema::table('ticketing_agent_access', function (Blueprint $table) { + $table->dropColumn('recovered_by'); + }); + } +}; diff --git a/src/Http/Controllers/TicketingSettingsController.php b/src/Http/Controllers/TicketingSettingsController.php index d5d36c2..e83657a 100644 --- a/src/Http/Controllers/TicketingSettingsController.php +++ b/src/Http/Controllers/TicketingSettingsController.php @@ -79,11 +79,15 @@ class TicketingSettingsController extends Controller * manager rights over every group. Seeing a group you do not manage is the * fix; acting on it is a separate decision for a human to make. * - * ONE write does not come through here: `storeAgent()` uses - * requireManagerAccessOrOrphanRecovery() instead, because a group with no - * manager left has nobody this gate could ever admit, and the group and - * its tickets are frozen for good (#4806). That is the whole exception — - * every other write on somebody else's group still ends up here. + * TWO writes do not come through here, and they are the same write in + * both directions. `storeAgent()` uses + * requireManagerAccessOrOrphanRecovery(), because a group with no manager + * left has nobody this gate could ever admit, and the group and its + * tickets are frozen for good (#4806); `destroyAgent()` uses + * requireRemoveAccess(), so that recovery is not one-way (#5002) — an + * appoint that could never be un-appointed froze the group again through + * its own cure. Every other write on somebody else's group still ends up + * here. */ private function requireManagerAccess(int $groupId): void { @@ -259,6 +263,27 @@ class TicketingSettingsController extends Controller $agents->each(fn($a) => $a->user = $agentUsers[$a->user_id] ?? null); } + // #4806 — the groups with no manager left who could still use this + // page. Computed once: the page names them in the add form, and #5002 + // asks the same question again for every Remove button below. + $orphanedGroupIds = $isSiteAdmin && !$isBootstrap ? $this->orphanedGroupIds() : []; + + // #5002 — requireRemoveAccess() in prop form, the way myManagedGroupIds + // is requireManagerAccess()'s. It cannot be `canManageGroupId(row.group)` + // any more: a rescuer may also remove a row on a group that is still + // orphaned, and the row their own rescue wrote even after that rescue + // un-orphaned the group. A row an administrator can create has to be a + // row they can see the Remove on — the list said "Not yours to manage" + // over a row the reader had written seconds earlier. + $orphanedSet = array_map('intval', $orphanedGroupIds); + $removableAgentIds = $agents + ->filter(fn ($a) => $myManagedGroupIds->contains($a->group_id) + || ($isSiteAdmin && (in_array((int) $a->group_id, $orphanedSet, true) + || ($a->recovered_by !== null && (int) $a->recovered_by === (int) $userId)))) + ->pluck('id') + ->values() + ->all(); + $priorities = $isBootstrap ? collect() : PriorityLevel::orderBy('sort_order')->get(); @@ -291,7 +316,10 @@ class TicketingSettingsController extends Controller // so the Agents tab can offer the recovery instead of printing // "ask a manager of the group" about a group that has none. // Empty for a non-admin: the recovery is not theirs to make. - 'orphanedGroupIds' => $isSiteAdmin && !$isBootstrap ? $this->orphanedGroupIds() : [], + 'orphanedGroupIds' => $orphanedGroupIds, + // #5002 — the access rows this user may actually remove. Superset + // of "rows on a group I manage": see requireRemoveAccess(). + 'removableAgentIds' => $removableAgentIds, 'isBootstrap' => $isBootstrap, 'isSiteAdmin' => $isSiteAdmin, 'autoCloseDays' => (int) Setting::get('ticketing.auto_close_days', 0), @@ -552,7 +580,20 @@ class TicketingSettingsController extends Controller TicketingAgentAccess::updateOrCreate( ['user_id' => $validated['user_id'], 'group_id' => $validated['group_id']], - ['role' => $validated['role']] + [ + 'role' => $validated['role'], + // WHO RESCUED THIS GROUP — #5002. A recovery write is the one + // write here that can lock its own author out: appoint a + // `manager` and the group is no longer orphaned, so the arm + // that allowed the press is shut by the press. Stamping the + // row is what lets destroyAgent() hand it back. + // + // Set to null on an ordinary manager's write, including a + // manager editing a row an earlier rescue wrote: once the + // group has somebody of its own touching the row, it is + // theirs and the rescuer has no further claim on it. + 'recovered_by' => $isRecovery ? Auth::id() : null, + ] ); /* @@ -597,15 +638,77 @@ class TicketingSettingsController extends Controller : "{$who} added to “{$name}” as {$asWhat}."); } + /** + * REMOVE IS THE SAME DOOR AS ADD — [central-ticketing] #5002. + * + * This was `requireManagerAccess()` while `storeAgent()` was + * requireManagerAccessOrOrphanRecovery(), so the recovery was ONE-WAY: the + * site admin who rescued a group could appoint and never un-appoint. The + * row they had just written appeared in their own Agents list marked "Not + * yours to manage", with no Remove control, and the DELETE answered 403 — + * the same person, the same second, the same row. + * + * Worse for a `manager` press, which is the one the card about the people + * search (#4926) called out as the expensive mistake: the write ends the + * orphan state, so the admin could not remove the row, could not appoint + * anyone else, and could not appoint themselves. The only person left who + * could undo it was the person wrongly appointed. That is #4806's frozen + * group again, reached through #4806's own cure. + * + * So two arms beyond an ordinary manager, both narrower than the write + * they undo: + */ public function destroyAgent(TicketingAgentAccess $access) { - $this->requireManagerAccess($access->group_id); + $this->requireRemoveAccess($access); $access->delete(); return back()->with('success', 'Agent removed.'); } + /** + * May the signed-in user take this agent access row away? + * + * 1. A manager of the group — the ordinary rule, unchanged. + * 2. A site admin, while the group is STILL orphaned. Exactly symmetric + * with storeAgent()'s arm and strictly narrower than it: on a group + * with no manager left, an admin can already appoint themselves + * manager and then remove anything, so this grants nothing new — it + * just does not make them take the group over first to clear a + * dangling row. + * 3. A site admin undoing a row THEY wrote as a rescue (`recovered_by`). + * This is the one that reopens the door a mistaken `manager` press + * closed, and it is deliberately not "the group would be orphaned + * without this row": that test cannot tell a mistaken appointment + * from the sole working manager of a healthy group, and admitting the + * second would let any site admin strip a manager from a group that + * is none of their business, then appoint themselves (#4710). A + * rescuer taking back their own appointment claims nothing they did + * not already hold at the moment they made it. + * + * Removing the last manager re-orphans the group, which is the correct + * end state: it is recoverable again, by whoever gets there next. + */ + private function requireRemoveAccess(TicketingAgentAccess $access): void + { + if ($this->isManagerOf($access->group_id)) { + return; + } + + if ($this->isSiteAdmin()) { + if ($this->orphanedGroupIds([$access->group_id]) !== []) { + return; + } + + if ($access->recovered_by !== null && (int) $access->recovered_by === (int) Auth::id()) { + return; + } + } + + abort(403); + } + public function storePriority(Request $request) { if (!$this->isSiteAdmin()) { diff --git a/src/Models/TicketingAgentAccess.php b/src/Models/TicketingAgentAccess.php index 65f89dd..8e66bfb 100644 --- a/src/Models/TicketingAgentAccess.php +++ b/src/Models/TicketingAgentAccess.php @@ -9,7 +9,13 @@ class TicketingAgentAccess extends Model { protected $table = 'ticketing_agent_access'; - protected $fillable = ['user_id', 'group_id', 'role']; + /** + * `recovered_by` is #5002: the site admin who wrote this row as an orphan + * recovery (#4806), and may therefore take it back. Null on every row + * written by an ordinary manager — see the migration for why this is not + * a general-purpose `created_by`. + */ + protected $fillable = ['user_id', 'group_id', 'role', 'recovered_by']; public function group(): BelongsTo {