diff --git a/src/Http/Controllers/TicketingSettingsController.php b/src/Http/Controllers/TicketingSettingsController.php index 87dbd87..6369b70 100644 --- a/src/Http/Controllers/TicketingSettingsController.php +++ b/src/Http/Controllers/TicketingSettingsController.php @@ -3,6 +3,7 @@ namespace Dashboard\Ticketing\Http\Controllers; use App\Models\Setting; +use App\Models\User; use Dashboard\Ticketing\Models\EmailConnection; use Dashboard\Ticketing\Models\PriorityLevel; use Dashboard\Ticketing\Models\TicketingAgentAccess; @@ -31,8 +32,21 @@ class TicketingSettingsController extends Controller */ private function isSiteAdmin(): bool { - $user = Auth::user(); - return $user && ($user->is_super_admin || $user->can('ticketing.settings')); + return $this->isSiteAdminUser(Auth::user()); + } + + /** + * 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 * 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. */ 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('role', 'manager') ->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 @@ -159,6 +284,12 @@ class TicketingSettingsController extends Controller 'emailConnections' => $emailConnections, 'myGroupIds' => $myGroupIds, '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, 'isSiteAdmin' => $isSiteAdmin, 'autoCloseDays' => (int) Setting::get('ticketing.auto_close_days', 0), @@ -345,13 +476,28 @@ class TicketingSettingsController extends Controller '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( ['user_id' => $validated['user_id'], 'group_id' => $validated['group_id']], ['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.'); }