diff --git a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php index e6743dbc..6979807a 100644 --- a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php +++ b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php @@ -45,9 +45,20 @@ class VirusScanningSettingsController extends Controller private readonly ActivityLogger $activity, ) {} - public function edit(): Response + public function edit(Request $request): Response { return Inertia::render('system/settings/virus-scanning', [ + // Which half of the screen is open. The connection and the + // policies are two different jobs — one is done once when the + // scanner is set up, the other is revisited — and a single + // column of fields with two Save buttons reads as one form + // that saves half of itself. + 'tab' => $request->query('tab') === 'options' ? 'options' : 'scanner', + // Read from the session here rather than shared as a flash + // prop: HandleInertiaRequests shares `success` and `error` and + // nothing else, which is why the Test button appeared to do + // nothing at all. Same shape the CAPTCHA screen uses. + 'test_result' => $request->session()->get('scanner_test_result'), 'enabled' => $this->config->enabled(), 'managed' => $this->config->isManaged(), 'address' => $this->config->isManaged() ? '' : $this->settings->get(Setting::VirusScannerAddress), diff --git a/resources/js/pages/system/settings/virus-scanning.tsx b/resources/js/pages/system/settings/virus-scanning.tsx index b4e65eaa..af9edf1b 100644 --- a/resources/js/pages/system/settings/virus-scanning.tsx +++ b/resources/js/pages/system/settings/virus-scanning.tsx @@ -1,5 +1,5 @@ import { type BreadcrumbItem } from '@/types'; -import { Head, router, useForm, usePage } from '@inertiajs/react'; +import { Head, Link, router, useForm } from '@inertiajs/react'; import { CheckCircle2, ShieldAlert, TriangleAlert } from 'lucide-react'; import { FormEventHandler } from 'react'; @@ -16,7 +16,12 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@ import { useTranslation } from '@/hooks/use-translation'; import AppLayout from '@/layouts/app-layout'; +type Tab = 'scanner' | 'options'; + interface VirusScanningProps { + tab: Tab; + /** The Test button's answer, carried through the session. */ + test_result: { ok: boolean; message: string } | null; enabled: boolean; /** The scanner is supplied by the platform: no address to set, and no switch. */ managed: boolean; @@ -35,6 +40,8 @@ interface VirusScanningProps { } export default function VirusScanningSettings({ + tab, + test_result, enabled, managed, address, @@ -46,13 +53,15 @@ export default function VirusScanningSettings({ counts, }: VirusScanningProps) { const { t } = useTranslation(); - const testResult = usePage().props.scanner_test_result as { ok: boolean; message: string } | undefined; const breadcrumbs: BreadcrumbItem[] = [ { title: t('Settings'), href: '/system/settings' }, { title: t('Virus scanning'), href: '/system/settings/virus-scanning' }, ]; + // One form behind both tabs, and one Save. The server takes every + // field on every save, so switching tabs never loses what was typed on + // the other one. const { data, setData, patch, errors, processing, recentlySuccessful } = useForm({ enabled, address, @@ -65,21 +74,23 @@ export default function VirusScanningSettings({ const submit: FormEventHandler = (e) => { e.preventDefault(); - patch(route('system-settings.virus-scanning.update')); + patch(route('system-settings.virus-scanning.update', tab === 'options' ? { tab: 'options' } : {}), { preserveScroll: true }); }; + const tabs: { key: Tab; label: string }[] = [ + { key: 'scanner', label: t('Scanner') }, + { key: 'options', label: t('Options') }, + ]; + return ( -
- +
+ {counts.let_through > 0 && ( - + {t(':count files were allowed through without being scanned', { count: counts.let_through })} @@ -90,179 +101,219 @@ export default function VirusScanningSettings({ )} -
- {managed ? ( - - - {t('Scanning is managed for you')} - - {t('Every upload on this site is scanned. The scanner itself is run for you, so there is nothing to connect here.')} - - - ) : ( - <> -
-
- setData('enabled', checked === true)} - /> - -
- -
+
+ {tabs.map(({ key, label }) => ( + + {label} + + ))} +
+ {tab === 'scanner' && ( +
+ {/* Above the form on purpose: these act on the scanner + as it is now, and nothing belongs after a Save + button. */} +
+ + + + + {test_result && ( + + {test_result.ok ? : } + {test_result.message} + + )} +
+ + + {managed ? ( + + + {t('Scanning is managed for you')} + + {t( + 'Every upload on this site is scanned. The scanner itself is run for you, so there is nothing to connect here.', + )} + + + ) : ( + <> +
+
+ setData('enabled', checked === true)} + /> + +
+ +
+ +
+ + setData('address', e.target.value)} + placeholder="tcp://clamav:3310" + /> +

+ {t('A ClamAV daemon, as tcp://host:3310 or unix:///path/to/clamd.sock.')} +

+ +
+ + )} + + + +
+ )} + + {tab === 'options' && ( +
+
+ + +

+ {t('Never scanned: :never · Being checked: :pending · In quarantine: :quarantined', { + never: counts.never_scanned, + pending: counts.pending, + quarantined: counts.quarantined, + })} +

+ + +
+ +
- + setData('address', e.target.value)} - placeholder="tcp://clamav:3310" + id="max_size_mb" + type="number" + min={0} + max={4096} + value={data.max_size_mb} + onChange={(e) => setData('max_size_mb', Number(e.target.value))} />

- {t('A ClamAV daemon, as tcp://host:3310 or unix:///path/to/clamd.sock.')} + {t( + 'Bigger files are handled by the rule below. Your scanner has its own limit too, and this should not exceed it.', + )}

- + +
+ +
+ + + +
+ +
+ + +

+ {t('Files allowed through this way are scanned again automatically once the scanner answers.')} +

+
- -

- {t('Sends the standard test file, which is harmless and every scanner recognises.')} -

- {testResult && ( - - {testResult.ok ? : } - {testResult.message} - - )} + + setData('wait_minutes', Number(e.target.value))} + /> +
- - )} -
- - setData('max_size_mb', Number(e.target.value))} - /> -

- {t('Bigger files are handled by the rule below. Your scanner has its own limit too, and this should not exceed it.')} -

- +
+ + setData('existing_rate_per_minute', Number(e.target.value))} + /> +

+ {t('Applies to the button above, so a backfill does not starve the scanner of new uploads.')} +

+ +
+ + +
- -
- - - -
- -
- - -

- {t('Files allowed through this way are scanned again automatically once the scanner answers.')} -

- -
- -
- - setData('wait_minutes', Number(e.target.value))} - /> - -
- - - - -
- - -

- {t('Never scanned: :never · Being checked: :pending · In quarantine: :quarantined', { - never: counts.never_scanned, - pending: counts.pending, - quarantined: counts.quarantined, - })} -

- -
- - setData('existing_rate_per_minute', Number(e.target.value))} - /> -

{t('Kept low so the scanner stays free for new uploads. Save first.')}

-
- - -
+ )}
); diff --git a/tests/Feature/Files/VirusScanningSettingsTest.php b/tests/Feature/Files/VirusScanningSettingsTest.php index a9eb7323..0a3e649d 100644 --- a/tests/Feature/Files/VirusScanningSettingsTest.php +++ b/tests/Feature/Files/VirusScanningSettingsTest.php @@ -117,6 +117,35 @@ test('the test button says when the scanner cannot be reached', function () { ->assertSessionHas('scanner_test_result', fn (array $result): bool => $result['ok'] === false); }); +test('the answer actually reaches the screen', function () { + // It did not, at first: the page read a flash prop that nothing + // shares, so the button appeared to do nothing at all. The result is + // handed over as a page prop, like the CAPTCHA screen's. + app()->instance(VirusScanner::class, new FakeVirusScanner(ScanVerdict::infected('Eicar-Test-Signature'))); + + $this->actingAs($this->admin)->post('/system/settings/virus-scanning/test'); + + $this->actingAs($this->admin)->get('/system/settings/virus-scanning')->assertInertia( + fn (AssertableInertia $page) => $page->where('test_result.ok', true), + ); +}); + +test('the screen opens on the scanner tab, and the other one is a link away', function () { + $this->actingAs($this->admin)->get('/system/settings/virus-scanning')->assertInertia( + fn (AssertableInertia $page) => $page->where('tab', 'scanner'), + ); + + $this->actingAs($this->admin)->get('/system/settings/virus-scanning?tab=options')->assertInertia( + fn (AssertableInertia $page) => $page->where('tab', 'options'), + ); + + // Anything else is the default rather than an error: a stale + // bookmark should open the page, not break it. + $this->actingAs($this->admin)->get('/system/settings/virus-scanning?tab=nonsense')->assertInertia( + fn (AssertableInertia $page) => $page->where('tab', 'scanner'), + ); +}); + test('the test button says when the scanner answers but detects nothing', function () { // The failure that looks like success: reachable, and blind. Empty or // broken virus definitions do exactly this.