diff --git a/app/Http/Middleware/HandleInertiaRequests.php b/app/Http/Middleware/HandleInertiaRequests.php index c03f95ce..9ad2654e 100644 --- a/app/Http/Middleware/HandleInertiaRequests.php +++ b/app/Http/Middleware/HandleInertiaRequests.php @@ -15,7 +15,9 @@ use App\Modules\Platform\Attribution\Attribution; use App\Modules\Platform\Capabilities\CapabilityRegistry; use App\Modules\Platform\Captcha\Captcha; use App\Modules\Platform\Installation\Installation; +use App\Modules\Files\Models\File; use App\Modules\Files\Queue\StalledZipBuilds; +use App\Modules\Files\Scanning\ScanStatus; use App\Modules\Platform\Localization\LocaleRegistry; use App\Modules\Platform\Localization\TimezoneRegistry; use App\Modules\Platform\OfficialLinks; @@ -183,6 +185,18 @@ class HandleInertiaRequests extends Middleware $counts['comments'] = app(VisibleCommentScope::class)->pendingTotal($user); } + if ($checker->allows($user, Permission::ReleaseQuarantinedFiles)) { + // Deliberately not library-scoped, unlike the comments count + // above: a quarantined file is not a file anybody is working + // with, it is one somebody has to decide about, and the + // permission is already narrow enough that whoever holds it + // is meant to see all of them. + $counts['quarantine'] = File::query()->whereIn('scan_status', [ + ScanStatus::Infected->value, + ScanStatus::UnscannableBlocked->value, + ])->count(); + } + // Unlike the counts above, every authenticated user (staff or // client) has their own personal notifications — no permission // gate here. diff --git a/app/Modules/Files/FilesServiceProvider.php b/app/Modules/Files/FilesServiceProvider.php index 23278fbc..4e1e0333 100644 --- a/app/Modules/Files/FilesServiceProvider.php +++ b/app/Modules/Files/FilesServiceProvider.php @@ -17,6 +17,7 @@ use App\Modules\Files\Notifications\FileSharedNotification; use App\Modules\Files\Notifications\NewVersionAvailableNotification; use App\Modules\Files\Notifications\NewVersionDigestNotification; use App\Modules\Files\Scanning\ClamAvScanner; +use App\Modules\Files\Scanning\ScanningConfig; use App\Modules\Files\Scanning\ScanStatus; use App\Modules\Files\Scanning\VirusScanner; use App\Modules\Files\Thumbnails\Events\ImageRenderingChanged; @@ -43,6 +44,12 @@ class FilesServiceProvider extends ServiceProvider // per viewer and the file listings ask it once per row. $this->app->scoped(ClientIdentityScope::class); + // Scoped, so the settings screen and the scanner it resolves share + // one instance: that is what lets the Test button try the address + // being typed rather than the one on file. Scoped rather than a + // singleton so a queue worker starts each job with a clean one. + $this->app->scoped(ScanningConfig::class); + // One implementation ships, and the interface exists so the test // suite can state a verdict instead of producing a file that // provokes one — and so a commercial engine can be added later diff --git a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php index e0183ba5..804d1ba7 100644 --- a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php +++ b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php @@ -140,12 +140,22 @@ class VirusScanningSettingsController extends Controller * agreement — so the answer is "it detected something" rather than * "it did not complain". */ - public function test(VirusScanner $scanner): RedirectResponse + public function test(Request $request, VirusScanner $scanner): RedirectResponse { // Nothing to test where the connection is not this installation's // to make. abort_unless($this->canConnect(), 403); + $typed = trim((string) $request->input('address', '')); + + // What the button is for: the address on screen, which on a first + // attempt has never been saved. Falls back to the stored one when + // the field is empty, so the button still answers on a screen + // somebody has not touched. + if ($typed !== '') { + $this->config->preview($typed); + } + $status = $scanner->status(); if (! $status->reachable) { diff --git a/app/Modules/Files/Scanning/ScanningConfig.php b/app/Modules/Files/Scanning/ScanningConfig.php index 16f6a845..e2cddea8 100644 --- a/app/Modules/Files/Scanning/ScanningConfig.php +++ b/app/Modules/Files/Scanning/ScanningConfig.php @@ -40,6 +40,22 @@ class ScanningConfig return $this->isManaged() || $this->settings->get(Setting::VirusScanningEnabled) === true; } + /** + * An address to use instead of the stored one, for this request only. + * + * The Test button exists to answer "is *this* address right?", and + * the address in question is the one being typed — testing what is + * saved would make the button useless exactly when it is needed, on + * the first attempt, before anything is saved. Set by + * VirusScanningSettingsController::test() and never persisted. + */ + private ?string $preview = null; + + public function preview(string $address): void + { + $this->preview = trim($address); + } + public function isManaged(): bool { return $this->managedAddress() !== ''; @@ -47,6 +63,13 @@ class ScanningConfig public function address(): string { + // Ahead of the managed address too: an operator on a managed + // installation has no field to type in, so nothing sets this + // there — and where something does, it was asked for. + if ($this->preview !== null && $this->preview !== '') { + return $this->preview; + } + if ($this->isManaged()) { return $this->managedAddress(); } diff --git a/resources/js/components/app-sidebar.tsx b/resources/js/components/app-sidebar.tsx index 0555d120..0da0f2fa 100644 --- a/resources/js/components/app-sidebar.tsx +++ b/resources/js/components/app-sidebar.tsx @@ -65,7 +65,15 @@ export function AppSidebar() { fileItems.push({ title: t('Import orphan files'), url: '/files/orphans', icon: FileWarning }); } if (can('release_quarantined_files')) { - fileItems.push({ title: t('Quarantine'), url: '/files/quarantine', icon: ShieldAlert }); + // Amber rather than the usual badge colour: the others count work + // waiting, this one counts something that went wrong. + fileItems.push({ + title: t('Quarantine'), + url: '/files/quarantine', + icon: ShieldAlert, + badge: pending.quarantine, + badgeTone: 'warning', + }); } if (can('moderate_comments')) { // Just "Comments" — the old "Comments awaiting approval" wrapped diff --git a/resources/js/components/nav-main.tsx b/resources/js/components/nav-main.tsx index 24d4a67f..31ab5095 100644 --- a/resources/js/components/nav-main.tsx +++ b/resources/js/components/nav-main.tsx @@ -85,7 +85,13 @@ export function NavMain({ groups = [] }: { groups: NavGroup[] }) { )} {item.badge !== undefined && item.badge > 0 && ( - + {item.badge} )} diff --git a/resources/js/pages/system/settings/virus-scanning.tsx b/resources/js/pages/system/settings/virus-scanning.tsx index 73cc0e59..8efbde37 100644 --- a/resources/js/pages/system/settings/virus-scanning.tsx +++ b/resources/js/pages/system/settings/virus-scanning.tsx @@ -217,7 +217,16 @@ export default function VirusScanningSettings({ type="button" variant="outline" className="w-fit" - onClick={() => router.post(route('system-settings.virus-scanning.test'), {}, { preserveScroll: true })} + // The address on screen, not the one on + // file: the question is whether what is + // being typed works. + onClick={() => + router.post( + route('system-settings.virus-scanning.test'), + { address: data.address }, + { preserveScroll: true }, + ) + } > {t('Test scanner')} diff --git a/resources/js/types/index.ts b/resources/js/types/index.ts index 33fd95d8..e497163e 100644 --- a/resources/js/types/index.ts +++ b/resources/js/types/index.ts @@ -24,6 +24,12 @@ export interface NavItem { isActive?: boolean; items?: NavItem[]; badge?: number; + /** + * What the badge is saying. The default reads as "there is work + * here"; "warning" reads as "something is wrong here", which is the + * difference between a queue and a quarantine. + */ + badgeTone?: 'default' | 'warning'; /** * Leaves this installation. Rendered as a plain anchor opening in a * new tab rather than an Inertia , which would try to fetch a @@ -130,6 +136,8 @@ export interface SharedData { membership_requests?: number; /** Comments held for approval anywhere in this viewer's library. */ comments?: number; + /** Files the scanner refused, waiting for somebody to decide. */ + quarantine?: number; notifications_unread?: number; }; update_notice: { diff --git a/tests/Feature/Files/VirusScanningSettingsTest.php b/tests/Feature/Files/VirusScanningSettingsTest.php index 5ee1035f..25e14e2c 100644 --- a/tests/Feature/Files/VirusScanningSettingsTest.php +++ b/tests/Feature/Files/VirusScanningSettingsTest.php @@ -423,3 +423,45 @@ test('an installation that connects its own scanner is', function () { fn (AssertableInertia $page) => $page->where('can_test', true), ); }); + +test('the test button tries the address on screen, not the one on file', function () { + // The real client, deliberately: a fake would answer whatever it was + // told and prove nothing about which address was used. Neither + // address has a scanner behind it, so the answer names the one it + // actually tried. + app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://saved.invalid:3310'); + + $this->actingAs($this->admin) + ->post('/system/settings/virus-scanning/test', ['address' => 'tcp://typed.invalid:3310']) + ->assertSessionHas('scanner_test_result', fn (array $result): bool => $result['ok'] === false + && str_contains($result['message'], 'typed.invalid')); + + // And the stored address is untouched: testing is not saving. + expect(app(Settings::class)->get(Setting::VirusScannerAddress))->toBe('tcp://saved.invalid:3310'); +}); + +test('an empty field falls back to the address on file', function () { + app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://saved.invalid:3310'); + + $this->actingAs($this->admin) + ->post('/system/settings/virus-scanning/test', ['address' => '']) + ->assertSessionHas('scanner_test_result', fn (array $result): bool => str_contains($result['message'], 'saved.invalid')); +}); + +test('the sidebar carries a count of what is in quarantine', function () { + App\Modules\Files\Models\File::factory()->create(['scan_status' => ScanStatus::Infected, 'scan_note' => 'X']); + App\Modules\Files\Models\File::factory()->create(['scan_status' => ScanStatus::Clean]); + + $this->actingAs($this->admin)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page->where('pending.quarantine', 1), + ); +}); + +test('somebody who cannot release is not shown the count', function () { + $staff = User::factory()->role(App\Modules\Identity\Permissions\SystemRole::Uploader)->create(); + App\Modules\Files\Models\File::factory()->create(['scan_status' => ScanStatus::Infected, 'scan_note' => 'X']); + + $this->actingAs($staff)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page->missing('pending.quarantine'), + ); +});