diff --git a/app/Modules/Files/Http/Controllers/FileThumbnailController.php b/app/Modules/Files/Http/Controllers/FileThumbnailController.php index fbaf2dbe..3336d255 100644 --- a/app/Modules/Files/Http/Controllers/FileThumbnailController.php +++ b/app/Modules/Files/Http/Controllers/FileThumbnailController.php @@ -6,11 +6,11 @@ namespace App\Modules\Files\Http\Controllers; use App\Http\Controllers\Controller; use App\Modules\Audit\Action; -use App\Modules\Audit\ActivityLogger; use App\Modules\Files\Access\DownloadAllowance; use App\Modules\Files\Delivery\StoredFileResponse; use App\Modules\Files\Models\File; use App\Modules\Files\Preview\PreviewKind; +use App\Modules\Files\Preview\PreviewLog; use App\Modules\Files\Thumbnails\Events\ResolvingImageRendering; use App\Modules\Files\Thumbnails\ImageAudience; use App\Modules\Files\Thumbnails\ImageRendition; @@ -22,7 +22,6 @@ use App\Support\ContentDisposition; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Http\Response; -use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Storage; @@ -72,7 +71,7 @@ class FileThumbnailController extends Controller { public function __construct( private readonly ThumbnailGenerator $thumbnails, - private readonly ActivityLogger $activity, + private readonly PreviewLog $previews, private readonly DownloadAllowance $allowance, private readonly StoredFileResponse $bytes, private readonly LocalSourceFile $source, @@ -144,7 +143,10 @@ class FileThumbnailController extends Controller // file. abort_unless($this->allowance->allows($file, $request->user()), 403); - $this->logPreview($file, $request); + // Debounced, because a browser turns one video into dozens of + // Range requests — see PreviewLog, which the anonymous twin in + // PublicGroupsController::preview shares. + $this->previews->record(Action::FilePreviewed, $file, $request->user()); if ($kind === PreviewKind::Image) { $audience = ImageAudience::forViewer($request->user()); @@ -164,29 +166,6 @@ class FileThumbnailController extends Controller return $this->bytes->inline($file); } - /** - * One log row per viewer per file per five minutes. - * - * Watching a video is a single deliberate act that the browser turns - * into dozens of Range requests against this route, and each one - * arrives here indistinguishable from someone clicking preview again. - * Cache::add is the whole mechanism: it writes only if the key is - * absent, so the first request through the window logs and the rest - * are silent, without a read-then-write race between two of them. - * - * Keyed by viewer, so one client's playback never suppresses another - * person's preview of the same file. Anonymous viewers do not reach - * this route at all — see PublicGroupsController::preview. - */ - private function logPreview(File $file, Request $request): void - { - $key = 'file-preview-logged:'.$file->id.':'.($request->user()->id ?? 'guest'); - - if (Cache::add($key, true, now()->addMinutes(5))) { - $this->activity->log(Action::FilePreviewed, subject: $file); - } - } - /** * The cached rendition's path on the local disk, generating it first * if this is the first time anyone has asked for it. Null only when diff --git a/app/Modules/Files/Preview/PreviewLog.php b/app/Modules/Files/Preview/PreviewLog.php new file mode 100644 index 00000000..4c1d4083 --- /dev/null +++ b/app/Modules/Files/Preview/PreviewLog.php @@ -0,0 +1,52 @@ +id : 'ip:'.request()->ip(); + + if (Cache::add('file-preview-logged:'.$file->id.':'.$viewerKey, true, now()->addMinutes(self::WINDOW_MINUTES))) { + $this->activity->log($action, subject: $file); + } + } +} diff --git a/app/Modules/Groups/Http/Controllers/PublicGroupsController.php b/app/Modules/Groups/Http/Controllers/PublicGroupsController.php index f4e292b2..42483f16 100644 --- a/app/Modules/Groups/Http/Controllers/PublicGroupsController.php +++ b/app/Modules/Groups/Http/Controllers/PublicGroupsController.php @@ -14,6 +14,7 @@ use App\Modules\Files\Models\Category; use App\Modules\Files\Models\File; use App\Modules\Files\Models\Folder; use App\Modules\Files\Preview\PreviewKind; +use App\Modules\Files\Preview\PreviewLog; use App\Modules\Files\Thumbnails\ImageAudience; use App\Modules\Files\Thumbnails\ImageRendition; use App\Modules\Files\Thumbnails\LocalSourceFile; @@ -77,6 +78,7 @@ class PublicGroupsController extends Controller public function __construct( private readonly Settings $settings, private readonly ActivityLogger $activity, + private readonly PreviewLog $previews, private readonly DownloadAllowance $allowance, private readonly ThumbnailGenerator $thumbnails, private readonly PublicThemeRegistry $themes, @@ -298,7 +300,11 @@ class PublicGroupsController extends Controller // nothing. abort_unless($this->allowance->allows($file, null), 403); - $this->activity->log(Action::PublicFilePreviewed, subject: $file); + // Debounced exactly as the signed-in twin is, and for the same + // reason: a single visitor watching one video arrives here dozens + // of times. Without a viewer to key on, PreviewLog keys on the + // request IP. + $this->previews->record(Action::PublicFilePreviewed, $file, null); return $this->bytes->inline($file); } diff --git a/tests/Feature/Groups/PublicFilePreviewTest.php b/tests/Feature/Groups/PublicFilePreviewTest.php index a63020b2..b28142d7 100644 --- a/tests/Feature/Groups/PublicFilePreviewTest.php +++ b/tests/Feature/Groups/PublicFilePreviewTest.php @@ -45,6 +45,45 @@ test('a visitor can preview a public file, and it is logged as a public preview' ->and(ActivityLog::query()->where('action', Action::PublicFileDownloaded)->where('subject_id', $file->id)->exists())->toBeFalse(); }); +// The same deliberate act, and the same long tail of Range requests a +// browser makes of it. The signed-in twin has debounced this since it was +// written; the anonymous one wrote a row per request. +test('replaying a public preview logs it once, not once per request', function () { + $file = publicListingFile(); + + foreach (range(1, 5) as $ignored) { + $this->get(route('public.preview', ['public', $file->slug]))->assertOk(); + } + + expect(ActivityLog::query()->where('action', Action::PublicFilePreviewed)->where('subject_id', $file->id)->count()) + ->toBe(1); +}); + +test('two visitors of the same file are each logged', function () { + // Keyed by viewer, and an anonymous visitor's stand-in is their IP — + // so one visitor's playback cannot swallow the record of somebody else + // looking at the same file. + $file = publicListingFile(); + + $this->get(route('public.preview', ['public', $file->slug]))->assertOk(); + + $this->withServerVariables(['REMOTE_ADDR' => '198.51.100.7']) + ->get(route('public.preview', ['public', $file->slug]))->assertOk(); + + expect(ActivityLog::query()->where('action', Action::PublicFilePreviewed)->where('subject_id', $file->id)->count()) + ->toBe(2); +}); + +test('the window is per file, so a second public file is still logged', function () { + $first = publicListingFile(); + $second = publicListingFile(['name' => 'second', 'slug' => 'second-report']); + + $this->get(route('public.preview', ['public', $first->slug]))->assertOk(); + $this->get(route('public.preview', ['public', $second->slug]))->assertOk(); + + expect(ActivityLog::query()->where('action', Action::PublicFilePreviewed)->count())->toBe(2); +}); + test('a file that is not public, or has expired, has no preview', function () { $private = publicListingFile(['public' => false]); $expired = publicListingFile(['expires_at' => now()->subDay()]);