diff --git a/src/Http/Controllers/TicketController.php b/src/Http/Controllers/TicketController.php index 411bb6c..21a4d84 100644 --- a/src/Http/Controllers/TicketController.php +++ b/src/Http/Controllers/TicketController.php @@ -1082,9 +1082,17 @@ class TicketController extends Controller return $refusal; } + // Fetched and deleted one model at a time rather than as a query-builder + // delete, because a builder delete fires no model events — and since + // #4781 a TicketParticipant row IS an audited fact (who can see this + // ticket's correspondence). Deleting in bulk here would have recorded + // every add and no removal, which is a worse trail than none: it reads + // as though nobody is ever taken off a ticket. ->get() rather than + // ->first(), so a duplicated pair is still fully removed. TicketParticipant::where('ticket_id', $ticket->id) ->where('user_id', $userId) - ->delete(); + ->get() + ->each->delete(); return back()->with('success', 'Participant removed.'); } diff --git a/src/TicketingServiceProvider.php b/src/TicketingServiceProvider.php index 1f52c25..a90d51c 100644 --- a/src/TicketingServiceProvider.php +++ b/src/TicketingServiceProvider.php @@ -24,6 +24,11 @@ class TicketingServiceProvider extends ServiceProvider // Notifications is absent or dark — the notifier no-ops. \Dashboard\Ticketing\Support\TicketingNotifier::registerCategories(); + // Declare this app's models to the shell's audit trail (#4781), so a + // ticket, a group retirement and above all an AGENT GRANT are on the + // Changes log. Guarded inside — central-logs is optional on a host. + $this->registerAuditedModels(); + if ($this->app->runningInConsole()) { $this->commands([ \Dashboard\Ticketing\Console\Commands\AutoCloseTickets::class, @@ -78,4 +83,141 @@ class TicketingServiceProvider extends ServiceProvider ->dailyAt(\App\Support\Ops\ScheduleStagger::dailyAt('ticketing:auto-close')); }); } + + /** + * HELP DESK WRITES TO THE SHELL'S CHANGES LOG — [central-ticketing] #4781. + * + * Until this method existed, `ticketing` was not one of the apps + * `audit_events` had ever seen: 4,378 rows, fifteen distinct apps, not one + * of them this one. A QA sweep signed in as a super admin, created a ticket, + * replied to it, and found that the newest row in the table afterwards was + * the shell recording the sweep's own visit to the log page. So "who closed + * this", "who deleted that group" and — the one that actually matters — + * "who gave this person the keys to the HR desk's queue" had no answer + * anywhere in Central. + * + * Nothing in core was wrong. `AuditRegistry`'s own docblock is explicit that + * snap-ins register their own models from their own providers and that core + * never references a snap-in. Ticketing simply never declared. This is the + * declaration, in the shape `FormsServiceProvider::registerAuditedGrants()` + * uses for `FormCollaborator` and `FormSharedGroup` — the precedent is exact + * and so is the reason: granting somebody access to other people's + * correspondence is a permission change, and a permission change belongs on + * the record. + * + * Declaring a model is the whole job. The registry attaches `AuditObserver` + * itself, so every create / declared-field update / delete on these four + * classes lands in the Changes tab under app `ticketing` (derived from the + * namespace by `Audit::appForClass()`), with the actor and the subject's + * label frozen into the row at write time. + * + * ── THE FIELD LISTS ARE THE VOLUME CONTROL ─────────────────────────────── + * + * Only declared fields are diffed and an update touching none of them + * records nothing at all, so these lists — not the tables' width — decide + * what the log costs. Each is the set of columns somebody would ask an + * auditor about: where a ticket lives, who is on the hook for it, what state + * it is in, and who may see it. `color` on a group and `description` on a + * ticket are left out deliberately: a palette tweak is not an audit event, + * and the body of a complaint is correspondence we are not copying into a + * second table to be kept for seven years. + * + * ── NOTHING HERE IS `sensitive`, AND THAT IS A DECISION ────────────────── + * + * `sensitive: true` buys exactly one thing: read-audit. It makes + * `RecordSensitiveViews` record a `viewed` row every time a route implicitly + * binds the model on a successful GET. For `Ticket` that is every agent + * opening every ticket all day — volume that would bury the writes this card + * is about, to answer a question nobody asked. It changes no retention + * window either (writes are kept on the operator's Changes window + * regardless; `sensitive` only lengthens what READ rows get). So: writes, + * declared; reads, not. If ticketing ever needs "who read this ticket", that + * is its own card and its own conversation about volume. + * + * ── WHAT IS DELIBERATELY NOT DECLARED ──────────────────────────────────── + * + * `TicketMessage`. The reporter expected a row for the reply they posted, + * and it is the right instinct on the wrong model: a message can only ever + * be created (there is no edit route and no delete route — see + * `TicketMessageController`, which has one method), so auditing it could + * only ever mean one row per message. Every inbound email through an + * `EmailConnection` writes one, under `system`, and a merge rewrites whole + * runs of them. That is a stream of "a message exists" against the record of + * who CHANGED something, which is what the Changes tab is for — and the + * messages themselves are already the permanent, readable record of what was + * said. The ticket's own row says who opened it and who closed it; the + * thread says what was written in between. + * + * A group hard-delete cascades its agent-access rows in the DATABASE (see + * the migration's foreign key), so those grants end without a `deleted` row + * of their own. The group's deletion is recorded, which is the question + * being asked — but it is worth knowing that the two are one event here and + * not several. + * + * Guarded on the class and wrapped in a catch for the same reason Forms and + * Files are: central-logs is optional on any given host, and a declaration + * must never be the reason a box fails to boot. + */ + private function registerAuditedModels(): void + { + if (! class_exists(\Central\Logs\Support\AuditRegistry::class)) { + return; + } + + try { + /* + * THE GRANT. The reason this card was filed and the first thing + * declared: `ticketing.settings.agents.store` / `.destroy` hand out + * and take back a person's access to a help desk group's entire + * queue, and did so leaving no trace at all. + * + * The label is resolved the long way round because this model has no + * `user()` relation and must not grow one — `TicketingSettingsController` + * HYDRATES `$access->user` as a plain attribute for the Settings page, + * and a relation of the same name would fight it. + */ + \Central\Logs\Support\AuditRegistry::audit( + \Dashboard\Ticketing\Models\TicketingAgentAccess::class, + fields: ['user_id', 'group_id', 'role'], + label: fn ($access) => (\App\Models\User::find($access->user_id)?->name ?? 'Deleted user') + .' on '.($access->group?->name ?? 'a help desk group'), + ); + + // The desk itself: renamed, re-prefixed, pointed at a different + // inbox, archived, restored, or deleted outright. `archived_at` is + // what tells archive from delete — one button does both, chosen by + // whether the group still owns tickets. + \Central\Logs\Support\AuditRegistry::audit( + \Dashboard\Ticketing\Models\TicketingGroup::class, + fields: ['name', 'prefix', 'email_address', 'archived_at'], + label: fn ($group) => ($group->name ?: 'Help desk group') + .($group->prefix ? ' ('.$group->prefix.')' : ''), + ); + + // The ticket: created, deleted, moved between desks, re-assigned, + // re-prioritised, closed, merged. `merged_into_id` is declared + // beside `status` because a merge is a pair of columns moving + // together and reading either one alone misreports it. + \Central\Logs\Support\AuditRegistry::audit( + \Dashboard\Ticketing\Models\Ticket::class, + fields: [ + 'group_id', 'status', 'assigned_to', 'priority_id', + 'project_id', 'due_date', 'merged_into_id', 'title', + ], + label: fn ($ticket) => trim(($ticket->number ?: 'Ticket').' '.($ticket->title ?? '')), + ); + + // Participants are the OTHER way somebody comes to see a ticket's + // correspondence — narrower than an agent grant (one ticket, not a + // whole queue) but the same kind of fact. + \Central\Logs\Support\AuditRegistry::audit( + \Dashboard\Ticketing\Models\TicketParticipant::class, + fields: ['ticket_id', 'user_id', 'added_by'], + label: fn ($participant) => (\App\Models\User::find($participant->user_id)?->name ?? 'Deleted user') + .' on ticket '.($participant->ticket?->number ?? '#'.$participant->ticket_id), + ); + } catch (\Throwable) { + // Never let a declaration break boot. + } + } }