diff --git a/app/Modules/Audit/Action.php b/app/Modules/Audit/Action.php index e1d6453b..4941042a 100644 --- a/app/Modules/Audit/Action.php +++ b/app/Modules/Audit/Action.php @@ -217,9 +217,13 @@ enum Action: string self::CommentDeleted => 'Deleted a comment on the file ":subject"', self::CommentApproved => 'Approved a comment on the file ":subject"', self::FileImported => 'Imported the orphan file ":subject"', - self::FileQuarantined => 'The file ":subject" was quarantined: :threat', + // :name rather than :subject, unlike the file actions above + // it: these two are written by the scan job, which has no + // actor and attaches no subject, so the name has to travel in + // the context or the line reads 'The file "" was quarantined'. + self::FileQuarantined => 'The file ":name" was quarantined: :threat', self::FileReleased => 'Released the quarantined file ":subject" (:reason)', - self::FileNotScanned => 'The file ":subject" was not scanned for viruses: :reason', + self::FileNotScanned => 'The file ":name" was not scanned for viruses: :reason', self::OrphanFileDeleted => 'Deleted the orphan file ":name"', self::OrphanFileAutoDeleted => 'Deleted the orphan file ":name"', self::ExpiredFileDeleted => 'Deleted the expired file ":name"', diff --git a/app/Modules/Audit/Http/Controllers/DashboardController.php b/app/Modules/Audit/Http/Controllers/DashboardController.php index 494eb726..6639add8 100644 --- a/app/Modules/Audit/Http/Controllers/DashboardController.php +++ b/app/Modules/Audit/Http/Controllers/DashboardController.php @@ -514,77 +514,69 @@ class DashboardController extends Controller // able to confirm at a glance, not only worth warning about // when it is false — the same reasoning as storage_durability. 'file_delivery' => $this->fileDelivery->describe(), - // Null when scanning is off, so the card says nothing about a - // feature this installation does not use. When it is on, this - // is the only place an administrator finds out that the - // scanner stopped answering — every other screen looks exactly - // as it did, because files keep flowing by design. - 'scanning' => $this->scanningWarning(), + // Always stated, like delivery and storage above it: "my + // uploads are checked by ClamAV" is worth confirming at a + // glance, not only worth mentioning when it is false. Null + // only where this installation does not connect its own + // scanner at all. + 'scanning' => $this->scanningState(), ]; } /** - * What is wrong with virus scanning right now, or null. + * Where this installation stands with virus scanning. * - * Deliberately only the bad news. The settings screen reports the - * healthy state; the dashboard exists here to interrupt somebody who - * was not looking for it. + * Reported whether or not anything is wrong: the System card states + * how downloads leave and where files are stored for the same reason, + * and "nothing is checking my uploads" is exactly the fact an + * administrator will not go looking for. + * + * Null only when this installation does not connect its own scanner — + * on a hosted one that is the platform's infrastructure, and a tenant + * reading about it could neither confirm nor fix it. See + * Capability::VirusScanningConnect. * * @return array{configured: bool, reachable: bool, engine: string|null, definitions_age_hours: int|null, let_through_24h: int, pending: int}|null */ - private function scanningWarning(): ?array + private function scanningState(): ?array { + if (! $this->capabilities->has(Capability::VirusScanningConnect)) { + return null; + } + $config = app(ScanningConfig::class); if (! $config->enabled()) { - // Nothing is checking what this installation accepts. Said - // only where somebody could act on it: an installation that - // connects its own scanner (community — see - // Capability::VirusScanningConnect). On a hosted one the - // scanner is the platform's to run, and a tenant reading - // "not configured" would be reading about somebody else's - // job. - return $this->capabilities->has(Capability::VirusScanningConnect) - ? [ - 'configured' => false, - 'reachable' => false, - 'engine' => null, - 'definitions_age_hours' => null, - 'let_through_24h' => 0, - 'pending' => 0, - ] - : null; + return [ + 'configured' => false, + 'reachable' => false, + 'engine' => null, + 'definitions_age_hours' => null, + 'let_through_24h' => 0, + 'pending' => 0, + ]; } $scanner = app(VirusScanner::class)->status(); - $letThrough = ActivityLog::query() - ->where('action', Action::FileNotScanned) - ->where('created_at', '>=', now()->subDay()) - ->count(); - - $pending = File::query() - ->where('scan_status', ScanStatus::Pending) - ->where('created_at', '<=', now()->subHour()) - ->count(); - - $stale = $scanner->definitionsAgeHours(); - - // Nothing to say when the scanner is there, current, and nothing - // has gone out unchecked. - if ($scanner->reachable && $letThrough === 0 && $pending === 0 && ($stale === null || $stale < 72)) { - return null; - } - return [ 'configured' => true, 'reachable' => $scanner->reachable, 'engine' => $scanner->engine, - 'definitions_age_hours' => $stale, - 'let_through_24h' => $letThrough, - // Files that have been waiting more than an hour: on an - // installation set to hold, this is what an outage looks like. - 'pending' => $pending, + 'definitions_age_hours' => $scanner->definitionsAgeHours(), + // Files that went out unchecked in the last day. Zero is the + // only number that means "protected"; anything else is a + // scanner that was down, or files nobody could open. + 'let_through_24h' => ActivityLog::query() + ->where('action', Action::FileNotScanned) + ->where('created_at', '>=', now()->subDay()) + ->count(), + // Waiting more than an hour: on an installation set to hold, + // this is what an outage looks like. + 'pending' => File::query() + ->where('scan_status', ScanStatus::Pending) + ->where('created_at', '<=', now()->subHour()) + ->count(), ]; } diff --git a/resources/js/components/dashboard-widgets/system-widget.tsx b/resources/js/components/dashboard-widgets/system-widget.tsx index 20cc57aa..e36ec821 100644 --- a/resources/js/components/dashboard-widgets/system-widget.tsx +++ b/resources/js/components/dashboard-widgets/system-widget.tsx @@ -117,6 +117,43 @@ function StorageDurabilityNotice({ durability }: { durability: StorageDurability return null; } +/** + * What the scanning row says, and whether it is a warning. + * + * Four states in one line, because the row is always there: no scanner at + * all, one that is not answering, one letting files through, and one + * quietly working — which is the common case and the only one that is not + * a warning. + */ +function scanningRow( + scanning: NonNullable, + t: (key: string, replacements?: Record) => string, +): { value: string; warning: boolean; title: string } { + if (!scanning.configured) { + return { + value: t('Nothing'), + warning: true, + title: t('Uploads are passed on without being checked for viruses.'), + }; + } + + const engine = scanning.engine ?? t('A virus scanner'); + + if (!scanning.reachable) { + return { value: t(':engine (not answering)', { engine }), warning: true, title: t('The scanner could not be reached.') }; + } + + if (scanning.let_through_24h > 0 || scanning.pending > 0) { + return { + value: engine, + warning: true, + title: t('Some files were not checked. Open the virus scanning settings for the detail.'), + }; + } + + return { value: engine, warning: false, title: '' }; +} + export function SystemWidget({ system, onViewReleaseNotes }: { system: SystemInfo; onViewReleaseNotes: () => void }) { const { t } = useTranslation(); const { update_notice } = usePage().props; @@ -125,30 +162,26 @@ export function SystemWidget({ system, onViewReleaseNotes }: { system: SystemInf // Only PHP is worth flagging. The other two are the file being handed // to the web server, which is the outcome this is watching for. const deliveryNeedsAttention = system.file_delivery.method === 'php'; + const scanning = system.scanning ? scanningRow(system.scanning, t) : null; return (
{/* Before the update notice on purpose: losing the files outranks being a version behind. */} {durability && } - {system.scanning && ( + {/* Only for a scanner that is configured and misbehaving. An + installation with no scanner at all says so on its own row + below, with the same link — two warnings for one fact would + make the card noisier without saying more. */} + {system.scanning?.configured && scanning?.warning && ( - {!system.scanning.configured - ? t('Uploads are not being checked for viruses') - : system.scanning.reachable - ? t('Files are going out unscanned') - : t('The virus scanner is not answering')} + {system.scanning.reachable ? t('Files are going out unscanned') : t('The virus scanner is not answering')}
    - {!system.scanning.configured && ( -
  • {t('Anything uploaded here — by staff, by clients, or through an upload link — is passed on unchecked.')}
    • - )} - {system.scanning.configured && !system.scanning.reachable && ( -
  • {t('Uploads cannot be checked until it is back.')}
    • - )} + {!system.scanning.reachable &&
  • {t('Uploads cannot be checked until it is back.')}
    • } {system.scanning.let_through_24h > 0 && (
  • {t(':count files were allowed through without being scanned in the last 24 hours.', { @@ -166,7 +199,7 @@ export function SystemWidget({ system, onViewReleaseNotes }: { system: SystemInf )}
- {system.scanning.configured ? t('Virus scanning settings') : t('Set up virus scanning')} + {t('Virus scanning settings')} @@ -270,6 +303,42 @@ export function SystemWidget({ system, onViewReleaseNotes }: { system: SystemInf )}
+ {/* Same rule as the row above, and the reason this one is + never hidden: an installation checking nothing looks + exactly like one that is. Absent only where the scanner + is not this installation's to connect. */} + {scanning && ( +
+
+ {scanning.warning ? ( + + {t('Uploads checked by')} + + ) : ( + {t('Uploads checked by')} + )} +
+
+ {scanning.warning ? ( + + {scanning.value} + + + ) : ( + {scanning.value} + )} +
+
+ )} {/* Stated even when everything is correct: "my files are on a host directory" is worth being able to confirm at a glance, not only worth warning about when it is false. */} diff --git a/tests/Feature/Files/VirusScanningSettingsTest.php b/tests/Feature/Files/VirusScanningSettingsTest.php index 9e43dace..d5377cd4 100644 --- a/tests/Feature/Files/VirusScanningSettingsTest.php +++ b/tests/Feature/Files/VirusScanningSettingsTest.php @@ -228,13 +228,13 @@ test('the status command reports an unreachable scanner and what got through', f ->and($status['let_through_24h'])->toBe(1); }); -test('the dashboard says nothing while scanning is healthy, and speaks up when it is not', function () { +test('the dashboard reports a healthy scanner, and says so when it stops answering', function () { app(Settings::class)->set(Setting::VirusScanningEnabled, true); app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://scanner.test:3310'); app()->instance(VirusScanner::class, new FakeVirusScanner); $this->actingAs($this->admin)->get('/dashboard')->assertInertia( - fn (AssertableInertia $page) => $page->where('system.scanning', null), + fn (AssertableInertia $page) => $page->where('system.scanning.reachable', true), ); app()->instance(VirusScanner::class, (new FakeVirusScanner)->reports(ScannerStatus::unreachable('no answer'))); @@ -273,13 +273,20 @@ test('a hosted installation is not told: the scanner is not its job', function ( ); }); -test('the notice goes away once a scanner is configured', function () { +test('a working scanner is still reported, by name', function () { + // The row is always there, like the delivery and storage rows beside + // it: "my uploads are checked by ClamAV" is worth confirming at a + // glance, not only worth saying when it is false. app(Settings::class)->set(Setting::VirusScanningEnabled, true); app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://scanner.test:3310'); app()->instance(VirusScanner::class, new FakeVirusScanner); $this->actingAs($this->admin)->get('/dashboard')->assertInertia( - fn (AssertableInertia $page) => $page->where('system.scanning', null), + fn (AssertableInertia $page) => $page + ->where('system.scanning.configured', true) + ->where('system.scanning.reachable', true) + ->where('system.scanning.engine', 'FakeAV 1.0') + ->where('system.scanning.let_through_24h', 0), ); }); diff --git a/tests/Feature/Files/VirusScanningTest.php b/tests/Feature/Files/VirusScanningTest.php index ae63ed85..6aec0e08 100644 --- a/tests/Feature/Files/VirusScanningTest.php +++ b/tests/Feature/Files/VirusScanningTest.php @@ -421,3 +421,23 @@ test('a released file is announced then, not before', function () { expect(App\Modules\Notifications\InAppNotification::query()->where('user_id', $client->id)->where('type', 'file_shared')->count())->toBe(1); }); + +test('the activity log names the file it quarantined', function () { + // The scan job has no actor and attaches no subject, so a template + // written with :subject renders 'The file "" was quarantined'. Caught + // on a real dashboard, not by a test, which is why there is one now. + fakeScanner(ScanVerdict::infected('Eicar-Test-Signature')); + $file = scannableFile(['name' => 'Contrato firmado']); + + runScan($file); + + $entry = ActivityLog::query()->where('action', Action::FileQuarantined)->sole(); + + $presented = app(App\Modules\Audit\ActivityPresenter::class)->present($entry); + $line = strtr($presented['template'], collect($presented['replacements']) + ->mapWithKeys(fn (string $value, string $key): array => [":{$key}" => $value]) + ->all()); + + expect($line)->toContain('Contrato firmado') + ->toContain('Eicar-Test-Signature'); +});