jwed/central-ticketing
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

#5002: the orphan recovery goes both ways

Browse Source 
#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) <noreply@anthropic.com>
This commit is contained in:
central-worker-4
2026-10-21 06:22:24 -07:00
parent 8b4f788e43
commit 51073fba1e
3 changed files with 161 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

119
src/Http/Controllers/TicketingSettingsController.php
View File

@@ -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()) {