From d0cec54f9bfc292cdf2eac6c01ec1e8c2c8d889c Mon Sep 17 00:00:00 2001 From: central-worker-2 Date: Thu, 5 Nov 2026 05:20:07 -0700 Subject: [PATCH] #6217: the other half of Help Desk Settings reaches the Changes log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `registerAuditedModels()` (#4781) declared four models. The Settings page has seven tabs, and a sweep found Groups and Agents fully audited beside Priorities, Statuses, Projects, Email connections and Automation writing nothing at all — zero rows for any of their routes in the whole store. `PriorityLevel`, `TicketStatus` and `TicketingProject` are now declared, in the shape the existing four use. `color` and `description` are left out of all three for the reason the group's colour is. The Email tab could not be declared. Its mailbox host, port, username and PASSWORD live in one `config` JSON column, and `Audit::redact()` masks by top-level field name only — `normalize()` walks into an array and never redacts inside one — so declaring `config` would write the school's mailbox credential in clear into a table read under `logs.view` alone. Declaring only the scalar columns is the other trap: an update that moved no declared field records nothing, so re-pointing a connection at a different mailbox would still have been silent, which is the write the card was filed about. `EmailConnectionAudit` is therefore the one writer for that tab, flattening `config` into `config.host` / `config.password` keys — the spelling the central redaction masks — and masking the secret at source as well, so the credential never reaches the logger at all. `EmailConnectionAuditLabels` declares the label with no observer, the shape `PulseAuditLabels` and `GroupAuditLabels` use, so those rows are not doubled. The Automation tab writes shell `Setting` rows, which no declaration here could reach; `AutomationSettingsAudit` records one row per save naming only what moved. Co-Authored-By: Claude Opus 5 --- .../TicketingSettingsController.php | 26 +- src/Support/AutomationSettingsAudit.php | 142 +++++++++++ src/Support/EmailConnectionAudit.php | 225 ++++++++++++++++++ src/Support/EmailConnectionAuditLabels.php | 105 ++++++++ src/TicketingServiceProvider.php | 90 ++++++- 5 files changed, 586 insertions(+), 2 deletions(-) create mode 100644 src/Support/AutomationSettingsAudit.php create mode 100644 src/Support/EmailConnectionAudit.php create mode 100644 src/Support/EmailConnectionAuditLabels.php diff --git a/src/Http/Controllers/TicketingSettingsController.php b/src/Http/Controllers/TicketingSettingsController.php index f5482c1..9ca9833 100644 --- a/src/Http/Controllers/TicketingSettingsController.php +++ b/src/Http/Controllers/TicketingSettingsController.php @@ -12,6 +12,8 @@ use Dashboard\Ticketing\Models\TicketingAgentAccess; use Dashboard\Ticketing\Models\TicketingGroup; use Dashboard\Ticketing\Models\TicketingProject; use Dashboard\Ticketing\Models\TicketStatus; +use Dashboard\Ticketing\Support\AutomationSettingsAudit; +use Dashboard\Ticketing\Support\EmailConnectionAudit; use Dashboard\Ticketing\Support\MergedTicketRepair; use Illuminate\Http\Request; use Illuminate\Routing\Controller; @@ -906,13 +908,17 @@ class TicketingSettingsController extends Controller $this->requireManagerAccess($validated['group_id']); - EmailConnection::create([ + $connection = EmailConnection::create([ 'group_id' => $validated['group_id'], 'type' => $validated['type'], 'active' => $request->boolean('active', true), 'config' => $request->input('config', []), ]); + // #6217 — by hand, not by declaration: the password lives in `config` + // and the registry would copy it into the log. See EmailConnectionAudit. + EmailConnectionAudit::created($connection); + return back()->with('success', 'Email connection saved.'); } @@ -932,12 +938,18 @@ class TicketingSettingsController extends Controller 'config.mailbox' => 'nullable|string|max:255', ]); + // #6217 — read BEFORE the save: an update's two sides have to be read + // the same way, and after `update()` the old config is gone. + $before = EmailConnectionAudit::state($connection); + $connection->update([ 'type' => $request->type, 'active' => $request->boolean('active', true), 'config' => $this->mergedConnectionConfig($request, $connection), ]); + EmailConnectionAudit::updated($connection, $before); + return back()->with('success', 'Email connection updated.'); } @@ -977,6 +989,11 @@ class TicketingSettingsController extends Controller $this->requireAgentAccess(); $this->requireManagerAccess($connection->group_id); $connection->delete(); + + // #6217 — the confirm says the saved credentials go with it. This is + // the row that says they were ever there. + EmailConnectionAudit::deleted($connection); + return back()->with('success', 'Email connection removed.'); } @@ -1069,6 +1086,9 @@ class TicketingSettingsController extends Controller 'auto_repair_merged_on_deploy' => 'sometimes|boolean', ]); + // #6217 — both keys as they stand, read before anything writes. + $before = AutomationSettingsAudit::snapshot(); + Setting::set('ticketing.auto_close_days', $validated['auto_close_days']); // #938 option A is a behaviour rule, so it is a setting, and it defaults @@ -1080,6 +1100,10 @@ class TicketingSettingsController extends Controller ); } + // One row for the save, naming only what moved — a tab that re-posts + // both boxes every time must not record "somebody pressed Save". + AutomationSettingsAudit::record($before); + return back()->with('success', 'Automation settings saved.'); } } diff --git a/src/Support/AutomationSettingsAudit.php b/src/Support/AutomationSettingsAudit.php new file mode 100644 index 0000000..3ed541a --- /dev/null +++ b/src/Support/AutomationSettingsAudit.php @@ -0,0 +1,142 @@ + + */ + private const DEFAULTS = [ + 'ticketing.auto_close_days' => '0', + MergedTicketRepair::SETTING => '0', + ]; + + /** + * The settings as they stand right now. Call BEFORE the writes and hand the + * result to {@see record()}. + * + * ONE query and not one `Setting::get()` apiece, and every key is present in + * the answer, so `record()` can read the key list off the snapshot and the + * two halves cannot drift apart. + * + * @return array + */ + public static function snapshot(): array + { + $stored = Setting::query() + ->whereIn('key', array_keys(self::DEFAULTS)) + ->pluck('value', 'key') + ->all(); + + $out = []; + foreach (self::DEFAULTS as $key => $default) { + $out[$key] = (string) ($stored[$key] ?? $default); + } + + return $out; + } + + /** + * Diff the snapshot against what is stored now and record one row for what + * moved. Call AFTER the writes, with the array `snapshot()` handed back. + * + * @param array $before + */ + public static function record(array $before): void + { + if ($before === [] || ! class_exists(\Central\Logs\Support\Audit::class)) { + return; + } + + $after = static::snapshot(); + $changes = []; + + foreach ($before as $key => $was) { + $now = $after[$key] ?? null; + + // Settings are a TEXT column, so everything stored is a string and + // the only pair worth special-casing is "no row" against "a row + // holding nothing" — both read as the shipped default. + if ((string) ($was ?? '') === (string) ($now ?? '')) { + continue; + } + + $changes[$key] = ['from' => $was, 'to' => $now]; + } + + if ($changes === []) { + return; // rule 2 + } + + \Central\Logs\Support\Audit::record( + self::ACTION, + self::SUBJECT, + $changes, + ['app' => self::APP], + ); + } +} diff --git a/src/Support/EmailConnectionAudit.php b/src/Support/EmailConnectionAudit.php new file mode 100644 index 0000000..a6434c4 --- /dev/null +++ b/src/Support/EmailConnectionAudit.php @@ -0,0 +1,225 @@ + 'array']`). + * + * - Declaring `config` copies the PASSWORD into `audit_events.changes`. + * `Audit::redact()` masks by TOP-LEVEL FIELD NAME, and `normalize()` walks + * into an array without redacting inside it, so a declared `config` is a + * credential written in clear to a table read under `logs.view` alone. + * - Declaring only `group_id` / `type` / `active` — the scalar columns — is + * silent on the write the card is actually about: the observer records + * nothing when no DECLARED field moved, so re-pointing a connection at a + * different host, or changing its password, would still leave no trace. + * + * So the config is FLATTENED here into `config.host`, `config.username`, + * `config.password`, … — which is the spelling `Audit::isSecretField()` masks, + * because it matches the substring `password`. {@see write()} masks it at the + * source as well, so the real credential is never handed to the logger at all + * and the central redaction is the second of two layers rather than the only + * one. + * + * ── THE RULES IT KEEPS, WHICH ARE THE OBSERVER'S OWN ──────────────────────── + * + * 1. ONE ROW PER SAVE. A save that moves the type AND the host is one edit and + * reads as one row, the way an observer-written row would. + * 2. NOTHING MOVED, NOTHING RECORDED. The Email tab re-posts every box it + * renders, so "somebody pressed Save" is not news. A blank password box + * means "keep the one you have" ({@see + * \Dashboard\Ticketing\Http\Controllers\TicketingSettingsController::mergedConnectionConfig()}), + * and keeping it is not a change. + * 3. THE VERBS ARE `created` / `updated` / `deleted`. Not a private + * `ticketing.email.*` spelling: the other six tabs on this page are + * observer-written under exactly these three, the Changes page's Action + * filter is a SELECT DISTINCT over them, and an operator asking "what was + * deleted in Help Desk today" should not have to know that one tab writes + * its rows by hand. + * + * The `app` is left to `Audit::appFor()`, which answers `ticketing` from the + * `Dashboard\Ticketing\…` namespace of the subject. The subject is the MODEL + * and not a string for the same reason: a string subject files under `shell` + * and leaves `EmailConnection` out of the Changes log's Subject-type facet, + * which is the census the card read the gap off. + * + * central-logs is optional on any given host — guarded once, in {@see write()}, + * and not at the three call sites. + */ +final class EmailConnectionAudit +{ + /** + * The config keys the Email tab writes, in the order the form asks for them. + * + * A fixed list and not `array_keys($config)`: the audit row should say the + * same thing about every connection, and an unknown key arriving in the blob + * is not something to copy into the log sight-unseen. + */ + private const CONFIG_KEYS = ['host', 'port', 'encryption', 'username', 'password', 'mailbox']; + + public static function created(EmailConnection $connection): void + { + static::write('created', $connection, static::sided(static::state($connection), 'to')); + } + + /** + * @param array $before {@see state()}, taken BEFORE the save + */ + public static function updated(EmailConnection $connection, array $before): void + { + $changes = []; + + foreach (static::state($connection) as $key => $now) { + $was = $before[$key] ?? null; + + if (static::unchanged($was, $now)) { + continue; + } + + $changes[$key] = ['from' => $was, 'to' => $now]; + } + + if ($changes === []) { + return; // rule 2 + } + + static::write('updated', $connection, $changes); + } + + public static function deleted(EmailConnection $connection): void + { + // What the row HELD when it was deleted is usually the whole question + // being asked — the same side the observer snapshots on a delete. + static::write('deleted', $connection, static::sided(static::state($connection), 'from')); + } + + /** + * The connection as the log describes it: the scalar columns, plus `config` + * flattened one level into dotted keys. + * + * Public because `updateEmailConnection()` has to take the BEFORE side + * itself, before the save — the two halves of an update's diff must be read + * the same way or they cannot be compared. + * + * `last_polled_at` is deliberately absent: it is written by the collector, + * not by a person, and a timestamp moving on its own is not a change + * anybody made. + * + * @return array + */ + public static function state(EmailConnection $connection): array + { + $config = is_array($connection->config) ? $connection->config : []; + + $state = [ + 'group_id' => $connection->group_id === null ? null : (int) $connection->group_id, + 'type' => $connection->type, + 'active' => (bool) $connection->active, + ]; + + foreach (self::CONFIG_KEYS as $key) { + $value = $config[$key] ?? null; + + $state['config.'.$key] = ($value === '' ? null : $value); + } + + return $state; + } + + /** + * A whole state as one-sided {from,to} pairs, for a create (`to`) or a + * delete (`from`). Unset fields are dropped: an empty box is noise, not + * information — `AuditObserver::snapshot()` drops them for the same reason. + * + * @param array $state + * @return array + */ + private static function sided(array $state, string $side): array + { + $out = []; + + foreach ($state as $key => $value) { + if ($value === null) { + continue; + } + + $out[$key] = $side === 'from' + ? ['from' => $value, 'to' => null] + : ['from' => null, 'to' => $value]; + } + + return $out; + } + + /** + * Did this field move? + * + * Compared as strings because the two sides come from different places — the + * stored JSON on one and a freshly-posted form on the other — so a port that + * was saved as `993` and re-posted as `"993"` is the same port, and recording + * it as a change would make every Save look like an edit. + */ + private static function unchanged(mixed $was, mixed $now): bool + { + return static::scalar($was) === static::scalar($now); + } + + private static function scalar(mixed $value): string + { + if (is_bool($value)) { + return $value ? '1' : '0'; + } + + return is_array($value) ? json_encode($value) : (string) ($value ?? ''); + } + + /** + * Write one row, with every secret masked before it leaves this class. + * + * The mask is `Audit::isSecretField()`'s own answer, applied to the same + * flattened key the row will carry, so this and `Audit::redact()` cannot + * disagree about what a secret is. NULL is preserved on either side: "was + * unset, now set" is information that gives nothing away, and it is the one + * thing the trail can honestly say about a credential. + * + * @param array $changes + */ + private static function write(string $action, EmailConnection $connection, array $changes): void + { + if (! class_exists(\Central\Logs\Support\Audit::class)) { + return; + } + + foreach ($changes as $key => $pair) { + if (! \Central\Logs\Support\Audit::isSecretField((string) $key)) { + continue; + } + + $mask = fn ($v) => ($v === null || $v === '') ? null : \Central\Logs\Support\Audit::REDACTED; + + $changes[$key] = ['from' => $mask($pair['from'] ?? null), 'to' => $mask($pair['to'] ?? null)]; + } + + \Central\Logs\Support\Audit::record($action, $connection, $changes); + } +} diff --git a/src/Support/EmailConnectionAuditLabels.php b/src/Support/EmailConnectionAuditLabels.php new file mode 100644 index 0000000..1716f9a --- /dev/null +++ b/src/Support/EmailConnectionAuditLabels.php @@ -0,0 +1,105 @@ + 'array']`), and `Audit::redact()` masks by TOP-LEVEL + * FIELD NAME only — `normalize()` walks into an array but never redacts inside + * one. So declaring `config` would copy the mailbox password verbatim into + * `audit_events.changes`, and declaring only the scalar columns would say + * nothing at all when somebody re-points the connection at a different mailbox, + * which is the write this card was filed about. + * + * {@see EmailConnectionAudit} is therefore the one writer of this trail — it + * flattens `config` into `config.host` / `config.password` / … keys, which is + * the spelling `Audit::isSecretField()` masks. That leaves only the label to + * declare, and it must be declared WITHOUT an observer or every row + * `EmailConnectionAudit` writes would be written a second time. + * + * Same shape and same reason as central-pulse's `PulseAuditLabels` (#5980), the + * shell's `App\Support\Groups\GroupAuditLabels` (#6046) and + * central-verification's `VerificationAuditLabels` (#5276): `AuditRegistry` + * holds its declarations in a `protected static` array and attaches the + * observer in the same call, so the only way to put a label closure in front of + * `Audit::subjectLabel()` without attaching anything is to subclass the registry + * and write the declaration directly — a subclass shares the parent's static + * storage, so this IS `AuditRegistry`'s own array and `labelClosureFor()` reads + * it back. + */ +final class EmailConnectionAuditLabels extends AuditRegistry +{ + /** + * Declare `EmailConnection`'s label to the Changes log. + * + * Called from `TicketingServiceProvider::registerAuditedModels()`, behind + * the `class_exists()` that method already keeps on the registry — + * central-logs is optional on any given host. + */ + public static function register(): void + { + self::declareLabel(EmailConnection::class, fn (EmailConnection $connection) => static::label($connection)); + } + + /** + * The mailbox, then the host, then the group — the SAME order the Email tab + * itself names a row in (`emailConnectionLabel()` in `Settings.vue`), so the + * log calls a connection what the page the operator was looking at called it. + * + * `config['mailbox']` is deliberately NOT in that list: on this form it is + * the IMAP FOLDER and defaults to `INBOX`, so falling back to it would label + * every half-filled connection "INBOX". + * + * The group comes second because it is the other half of what the row + * decides — which desk's queue this mailbox's mail lands in. + */ + private static function label(EmailConnection $connection): string + { + $config = is_array($connection->config) ? $connection->config : []; + + $mailbox = (string) ($config['username'] ?? $config['host'] ?? ''); + + return ($mailbox !== '' ? $mailbox : 'Mailbox') + .' → '.($connection->group?->name ?: 'a help desk group'); + } + + /** + * One label closure, and no observer. + * + * `fields: []` and `sensitive: false` are stated rather than left out so the + * shape of the declaration is the shape `AuditRegistry::audit()` writes — + * `fieldsFor()`, `isSensitive()` and `all()` all read this array. + */ + private static function declareLabel(string $modelClass, Closure $label): void + { + try { + static::$models[$modelClass] = [ + 'fields' => [], + 'label' => $label, + 'sensitive' => false, + ]; + } catch (\Throwable $e) { + // The registry changed shape under us. Reported so it is visible, + // never rethrown: the cost is the id-label we have always had. + report($e); + } + } +} diff --git a/src/TicketingServiceProvider.php b/src/TicketingServiceProvider.php index a90d51c..567f3b6 100644 --- a/src/TicketingServiceProvider.php +++ b/src/TicketingServiceProvider.php @@ -106,11 +106,27 @@ class TicketingServiceProvider extends ServiceProvider * the record. * * Declaring a model is the whole job. The registry attaches `AuditObserver` - * itself, so every create / declared-field update / delete on these four + * itself, so every create / declared-field update / delete on these seven * 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. * + * ── HALF A SETTINGS PAGE IS NOT A DECISION — #6217 ─────────────────────── + * + * #4781 declared four models. Help Desk Settings has SEVEN tabs, and a + * second sweep a fortnight later found that Groups and Agents were fully + * audited while Priorities, Statuses, Projects, Email connections and + * Automation — same page, same buttons, same super admin — wrote nothing at + * all. The census was flat: zero rows for `settings.priorities.*`, + * `settings.statuses.*`, `settings.projects.*`, `settings.email.*` and + * `settings.automation.update` in the whole store, and none of those models + * in the Changes log's Subject-type facet. QA had driven the Priorities + * routes on the box inside 48 hours and the log held none of it. + * + * `PriorityLevel`, `TicketStatus` and `TicketingProject` join the list + * below. The other two could not, and are handled in their own classes for + * reasons that are theirs and not an omission — see the next section. + * * ── THE FIELD LISTS ARE THE VOLUME CONTROL ─────────────────────────────── * * Only declared fields are diffed and an update touching none of them @@ -134,6 +150,32 @@ class TicketingServiceProvider extends ServiceProvider * declared; reads, not. If ticketing ever needs "who read this ticket", that * is its own card and its own conversation about volume. * + * ── WHAT IS AUDITED, BUT NOT FROM HERE ─────────────────────────────────── + * + * `EmailConnection` and the Automation tab are both on the record; neither + * can be put there by a declaration. + * + * `EmailConnection` keeps its mailbox host, port, username and PASSWORD in + * one `config` JSON column. The registry diffs declared fields by reading + * them, and `Audit::redact()` masks by TOP-LEVEL FIELD NAME only — it never + * redacts inside an array — so declaring `config` would write the school's + * mailbox password in clear into `audit_events.changes`, a table readable + * under `logs.view` alone. Declaring only the scalar columns is the other + * trap: an update that moved no DECLARED field records nothing, so + * re-pointing a connection at a different mailbox would still be silent, + * and that is the exact write #6217 was filed about. + * {@see Support\EmailConnectionAudit} therefore writes the three verbs by + * hand with `config` flattened into `config.host` / `config.password` / … + * keys — the spelling the central redaction masks — and + * {@see Support\EmailConnectionAuditLabels} declares the LABEL here with no + * observer, so the hand-written rows are not doubled. + * + * The Automation tab writes `App\Models\Setting` rows, which are SHELL rows + * and not a ticketing model at all: no declaration here could reach them, + * and declaring `Setting` would put every platform-wide toggle in Central + * on the `ticketing` filter. {@see Support\AutomationSettingsAudit} records + * it as one explicit domain event per save. + * * ── WHAT IS DELIBERATELY NOT DECLARED ──────────────────────────────────── * * `TicketMessage`. The reporter expected a row for the reply they posted, @@ -216,6 +258,52 @@ class TicketingServiceProvider extends ServiceProvider label: fn ($participant) => (\App\Models\User::find($participant->user_id)?->name ?? 'Deleted user') .' on ticket '.($participant->ticket?->number ?? '#'.$participant->ticket_id), ); + + // ── THE SETTINGS CATALOGUES ([central-ticketing] #6217) ────────── + // + // The three tabs below are a shared vocabulary: every ticket in the + // school is filed under one of these rows, so renaming, reordering + // or removing one re-labels work that is already done. `color` is + // left out of all three for the reason the group's is — a palette + // tweak is not an audit event — and so is `description`, which is + // help text under a box rather than a fact about the desk. + + // What a priority is CALLED and where it RANKS. `sort_order` is + // declared because it is the order the pickers and the board read: + // moving "Urgent" below "Low" changes what the office sees first. + \Central\Logs\Support\AuditRegistry::audit( + \Dashboard\Ticketing\Models\PriorityLevel::class, + fields: ['name', 'sort_order'], + label: fn ($priority) => 'Priority: '.($priority->name ?: '#'.$priority->getKey()), + ); + + // `slug` is declared beside `name` and never changes after creation + // — which is exactly why it is worth the row: `tickets.status` + // stores the SLUG, so the slug on a `deleted` entry is the only + // thing that says which tickets' status just stopped resolving to + // anything. `is_closed` is behaviour, not decoration: it decides + // whether a ticket in this status counts as finished, and the board + // reads it to decide which tickets are still open + // (`TicketController::index()`'s `$closedSlugs`). + \Central\Logs\Support\AuditRegistry::audit( + \Dashboard\Ticketing\Models\TicketStatus::class, + fields: ['slug', 'name', 'sort_order', 'is_closed'], + label: fn ($status) => 'Status: '.($status->name ?: $status->slug ?: '#'.$status->getKey()), + ); + + // A project is a folder of tickets on one desk. `group_id` is + // declared because it is which desk — the same fact `Ticket` + // declares it for. + \Central\Logs\Support\AuditRegistry::audit( + \Dashboard\Ticketing\Models\TicketingProject::class, + fields: ['group_id', 'name', 'status'], + label: fn ($project) => 'Project: '.($project->name ?: '#'.$project->getKey()) + .' ('.($project->group?->name ?? 'a help desk group').')', + ); + + // The LABEL only, and deliberately no observer — see the class, and + // the "WHAT IS AUDITED, BUT NOT FROM HERE" section above. + Support\EmailConnectionAuditLabels::register(); } catch (\Throwable) { // Never let a declaration break boot. }