#4806: a help desk group whose last manager can no longer act is recoverable
Every write in TicketingSettingsController is requireManagerAccess() — an existing manager of that exact group, or 403 — and settings.agents.store, the only route that writes a `ticketing_agent_access` row, is gated the same way. storeGroup() auto-appoints a manager only for a group it creates. So the moment a group's last manager stopped being able to act, the group froze for good: no rename, no archive, no delete, no project, no agent, and no ticket in it ever assigned, worked or closed. The page's own answer on the Agents tab was "Ask a manager of the group to add you", which on such a group names nobody. On dev that was every group: all 15 held exactly one agent row, the same person on every one, and that account had been rewritten into an admissions-only applicant (#4202/#4203) and could not reach /app/ticketing at all. Fifteen groups, fourteen tickets, nobody who could administer any of it, and nothing a super admin could press — the "ticketing lifecycle is DEAD on dev" observation of 2026-10-18 is this, not a seed gap. One carve-out, and no more: a site admin may write an agent row for a group that has NO manager left who could still use this page. "Still" is the operative word and canStillAdminister() is its definition — the account must exist, be active, and hold `ticketing.settings`, which is exactly what the route itself demands, so it answers "could this manager have done this write themselves". A manager row outlives all three. #4710's rule is untouched: the carve-out is a property of the GROUP, not of the caller, so a group somebody is still running remains none of an admin's business, and the rescue grants the rescuer nothing beyond the row they wrote — appoint somebody else and the very next request is refused again. The page gets `orphanedGroupIds` so the Agents tab can offer the recovery, label those groups "— no manager", and explain what appointing one does (shell side). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -3,6 +3,7 @@
|
|||||||
namespace Dashboard\Ticketing\Http\Controllers;
|
namespace Dashboard\Ticketing\Http\Controllers;
|
||||||
|
|
||||||
use App\Models\Setting;
|
use App\Models\Setting;
|
||||||
|
use App\Models\User;
|
||||||
use Dashboard\Ticketing\Models\EmailConnection;
|
use Dashboard\Ticketing\Models\EmailConnection;
|
||||||
use Dashboard\Ticketing\Models\PriorityLevel;
|
use Dashboard\Ticketing\Models\PriorityLevel;
|
||||||
use Dashboard\Ticketing\Models\TicketingAgentAccess;
|
use Dashboard\Ticketing\Models\TicketingAgentAccess;
|
||||||
@@ -31,8 +32,21 @@ class TicketingSettingsController extends Controller
|
|||||||
*/
|
*/
|
||||||
private function isSiteAdmin(): bool
|
private function isSiteAdmin(): bool
|
||||||
{
|
{
|
||||||
$user = Auth::user();
|
return $this->isSiteAdminUser(Auth::user());
|
||||||
return $user && ($user->is_super_admin || $user->can('ticketing.settings'));
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The same question asked about SOMEBODY ELSE — one definition, two callers.
|
||||||
|
*
|
||||||
|
* #4806 needs to know whether the manager a group already has could still
|
||||||
|
* use this page, which is isSiteAdmin()'s test applied to a user who is not
|
||||||
|
* signed in. Two copies of that rule would drift, and the drift would be
|
||||||
|
* invisible: the recovery path below opens exactly when this returns false
|
||||||
|
* for every manager of a group.
|
||||||
|
*/
|
||||||
|
private function isSiteAdminUser(?User $user): bool
|
||||||
|
{
|
||||||
|
return $user !== null && ($user->is_super_admin || $user->can('ticketing.settings'));
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -62,16 +76,127 @@ class TicketingSettingsController extends Controller
|
|||||||
* visibility bug, not a licence to hand every `ticketing.settings` holder
|
* visibility bug, not a licence to hand every `ticketing.settings` holder
|
||||||
* manager rights over every group. Seeing a group you do not manage is the
|
* 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.
|
* 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.
|
||||||
*/
|
*/
|
||||||
private function requireManagerAccess(int $groupId): void
|
private function requireManagerAccess(int $groupId): void
|
||||||
{
|
{
|
||||||
$isManager = TicketingAgentAccess::where('user_id', Auth::id())
|
if (!$this->isManagerOf($groupId)) {
|
||||||
|
abort(403);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Does the signed-in user hold a `manager` row on this group? */
|
||||||
|
private function isManagerOf(int $groupId): bool
|
||||||
|
{
|
||||||
|
return TicketingAgentAccess::where('user_id', Auth::id())
|
||||||
->where('group_id', $groupId)
|
->where('group_id', $groupId)
|
||||||
->where('role', 'manager')
|
->where('role', 'manager')
|
||||||
->exists();
|
->exists();
|
||||||
if (!$isManager) {
|
|
||||||
abort(403);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Could this person still administer a help desk group today?
|
||||||
|
*
|
||||||
|
* NOT "do they hold a manager row" — the row is what we are judging. This
|
||||||
|
* asks whether, if they signed in right now, the Agents tab's own POST
|
||||||
|
* would work for them: the account has to still exist, still be active,
|
||||||
|
* and still hold the permission every route in this file sits behind.
|
||||||
|
*
|
||||||
|
* A manager row outlives all three. On dev the sole manager of all 15
|
||||||
|
* groups was a live row pointing at an account that had been rewritten
|
||||||
|
* into an admissions-only applicant (#4202/#4203) — it could not reach
|
||||||
|
* /app/ticketing at all, and the Agents tab was still telling site admins
|
||||||
|
* to go and ask them (#4806).
|
||||||
|
*/
|
||||||
|
private function canStillAdminister(?User $user): bool
|
||||||
|
{
|
||||||
|
if ($user === null || !($user->is_active ?? true)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
return $this->isSiteAdminUser($user);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The groups nobody is left to run — every `manager` row on them points at
|
||||||
|
* somebody canStillAdminister() says no to (or there is no manager row at
|
||||||
|
* all).
|
||||||
|
*
|
||||||
|
* This is the ONLY condition under which a site admin may write an agent
|
||||||
|
* row for a group they do not manage. It is deliberately a property of the
|
||||||
|
* GROUP, not of the caller: an orphaned group cannot be rescued by anyone
|
||||||
|
* else, and a group with a working manager is still none of an admin's
|
||||||
|
* business (#4710).
|
||||||
|
*
|
||||||
|
* `$onlyGroupIds` narrows both queries for the single-group gate; `null`
|
||||||
|
* means "every group", which is what the page's prop needs.
|
||||||
|
*
|
||||||
|
* @param int[]|null $onlyGroupIds
|
||||||
|
* @return int[]
|
||||||
|
*/
|
||||||
|
private function orphanedGroupIds(?array $onlyGroupIds = null): array
|
||||||
|
{
|
||||||
|
$groupIds = TicketingGroup::when($onlyGroupIds !== null,
|
||||||
|
fn($q) => $q->whereIn('id', $onlyGroupIds))
|
||||||
|
->pluck('id');
|
||||||
|
|
||||||
|
if ($groupIds->isEmpty()) {
|
||||||
|
return [];
|
||||||
|
}
|
||||||
|
|
||||||
|
$managerRows = TicketingAgentAccess::where('role', 'manager')
|
||||||
|
->whereIn('group_id', $groupIds)
|
||||||
|
->get(['group_id', 'user_id']);
|
||||||
|
|
||||||
|
// One can() per distinct manager, not per row: the dev case is one
|
||||||
|
// person holding the manager row on all fifteen groups.
|
||||||
|
$managerIds = $managerRows->pluck('user_id')->unique();
|
||||||
|
$actingIds = $managerIds->isEmpty()
|
||||||
|
? collect()
|
||||||
|
: User::whereIn('id', $managerIds)->get()
|
||||||
|
->filter(fn($u) => $this->canStillAdminister($u))
|
||||||
|
->pluck('id');
|
||||||
|
|
||||||
|
$covered = $managerRows
|
||||||
|
->filter(fn($r) => $actingIds->contains($r->user_id))
|
||||||
|
->pluck('group_id')
|
||||||
|
->unique();
|
||||||
|
|
||||||
|
return $groupIds->reject(fn($id) => $covered->contains($id))->values()->all();
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Manager rights over one group, OR a site admin rescuing an orphaned one.
|
||||||
|
*
|
||||||
|
* The second arm is #4806 and nothing wider: the moment a group's last
|
||||||
|
* usable manager leaves, is deactivated or is diverted elsewhere, every
|
||||||
|
* write in this file 403s for everybody — the group cannot be renamed,
|
||||||
|
* archived, given a project or given a single agent, and its tickets can
|
||||||
|
* never be assigned or closed. The documented answer on the Agents tab was
|
||||||
|
* "ask a manager of the group", which on such a group names nobody.
|
||||||
|
*
|
||||||
|
* So: a site admin may appoint somebody to a group that has no manager
|
||||||
|
* left. That one write un-orphans the group, and from the next request on
|
||||||
|
* the ordinary requireManagerAccess() rule governs it again — including
|
||||||
|
* for the admin, who gets no standing rights over it unless they appointed
|
||||||
|
* themselves.
|
||||||
|
*/
|
||||||
|
private function requireManagerAccessOrOrphanRecovery(int $groupId): void
|
||||||
|
{
|
||||||
|
if ($this->isManagerOf($groupId)) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->isSiteAdmin() && $this->orphanedGroupIds([$groupId]) !== []) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
abort(403);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function index(?string $tab = null): Response
|
public function index(?string $tab = null): Response
|
||||||
@@ -159,6 +284,12 @@ class TicketingSettingsController extends Controller
|
|||||||
'emailConnections' => $emailConnections,
|
'emailConnections' => $emailConnections,
|
||||||
'myGroupIds' => $myGroupIds,
|
'myGroupIds' => $myGroupIds,
|
||||||
'myManagedGroupIds' => $myManagedGroupIds,
|
'myManagedGroupIds' => $myManagedGroupIds,
|
||||||
|
// #4806 — groups with no manager left who could still use this
|
||||||
|
// page. A site admin may add an agent to these and nothing else,
|
||||||
|
// 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() : [],
|
||||||
'isBootstrap' => $isBootstrap,
|
'isBootstrap' => $isBootstrap,
|
||||||
'isSiteAdmin' => $isSiteAdmin,
|
'isSiteAdmin' => $isSiteAdmin,
|
||||||
'autoCloseDays' => (int) Setting::get('ticketing.auto_close_days', 0),
|
'autoCloseDays' => (int) Setting::get('ticketing.auto_close_days', 0),
|
||||||
@@ -345,13 +476,28 @@ class TicketingSettingsController extends Controller
|
|||||||
'role' => 'required|in:agent,manager',
|
'role' => 'required|in:agent,manager',
|
||||||
]);
|
]);
|
||||||
|
|
||||||
$this->requireManagerAccess($validated['group_id']);
|
// #4806 — the one way back into a group whose last manager can no
|
||||||
|
// longer act. Asked BEFORE the write, because the write is what ends
|
||||||
|
// the condition: appointing yourself makes isManagerOf() true.
|
||||||
|
$isRecovery = !$this->isManagerOf($validated['group_id'])
|
||||||
|
&& $this->orphanedGroupIds([$validated['group_id']]) !== [];
|
||||||
|
$this->requireManagerAccessOrOrphanRecovery($validated['group_id']);
|
||||||
|
|
||||||
TicketingAgentAccess::updateOrCreate(
|
TicketingAgentAccess::updateOrCreate(
|
||||||
['user_id' => $validated['user_id'], 'group_id' => $validated['group_id']],
|
['user_id' => $validated['user_id'], 'group_id' => $validated['group_id']],
|
||||||
['role' => $validated['role']]
|
['role' => $validated['role']]
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Say which group was rescued and that it had nobody: a recovery is a
|
||||||
|
// rare, consequential click and "Agent added." does not describe it.
|
||||||
|
if ($isRecovery) {
|
||||||
|
$name = TicketingGroup::find($validated['group_id'])?->name;
|
||||||
|
|
||||||
|
return back()->with('success', $name === null
|
||||||
|
? 'Agent added — that group had no manager who could still act.'
|
||||||
|
: "Agent added to “{$name}”, which had no manager who could still act.");
|
||||||
|
}
|
||||||
|
|
||||||
return back()->with('success', 'Agent added.');
|
return back()->with('success', 'Agent added.');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user