diff --git a/app/Modules/Files/Http/Controllers/ZipDownloadsController.php b/app/Modules/Files/Http/Controllers/ZipDownloadsController.php index b31f11cc..7baf8509 100644 --- a/app/Modules/Files/Http/Controllers/ZipDownloadsController.php +++ b/app/Modules/Files/Http/Controllers/ZipDownloadsController.php @@ -18,6 +18,7 @@ use App\Modules\Files\Uploads\StoreUploadedFile; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; use App\Support\ContentDisposition; +use Illuminate\Database\Eloquent\Collection; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Http\Response; @@ -180,8 +181,7 @@ class ZipDownloadsController extends Controller // Only the first time. Re-fetching one prepared archive is the // same delivery, not a fresh download of everything inside it. if ($zipDownload->delivered_at === null) { - $this->logContainedDownloads($zipDownload, $user); - $zipDownload->forceFill(['delivered_at' => now()])->save(); + $this->deliverOnce($zipDownload, $user); } $size = Storage::disk('files')->size($path); @@ -195,14 +195,81 @@ class ZipDownloadsController extends Controller } /** - * Every file actually bundled gets a FileDownloaded entry — otherwise - * a file's download history/count would silently miss zip downloads. + * Hand the archive over, once: refuse it if anything inside is out of + * allowance, otherwise count everything it holds as downloaded. + * + * This is the only point that spends a download limit, which is why + * it also has to be the point that enforces it. Building an archive + * takes nothing, so ordering the same limited file into any number of + * archives passes every check on the way — store() and the job both + * look at an allowance nothing has drawn on yet — and collecting them + * all afterwards would hand over more copies than the limit allows. + * + * One refused file refuses the whole delivery, because nothing can be + * taken out of a finished archive without building it again. Ordering + * the same selection afresh is the way through: the build leaves the + * spent file out and names it in skipped_files. + * + * An archive from before the job recorded its contents is handed over + * the way it always was, without this check. Its contents can only be + * guessed at by resolving the selection again, and guessing is exactly + * what must not decide a refusal: the same reconstruction both refuses + * over files the archive does not hold and misses files it does. Those + * rows stop existing within a day or two of an upgrade, and until then + * they behave as they did before this change rather than worse. */ - private function logContainedDownloads(ZipDownload $zipDownload, User $requester): void + private function deliverOnce(ZipDownload $zipDownload, User $requester): void + { + $recorded = $zipDownload->contained_file_ids; + + // What the job wrote down, read back as it stands — deliberately + // not filtered by what the requester may see today. The bytes are + // in the archive already, so a file that has since expired or left + // their scope is still being given to them, and a count that + // quietly dropped it would understate what was taken. + $contained = $recorded === null + ? $this->resolveSelection($zipDownload, $requester) + : File::query()->whereIn('id', $recorded)->get(); + + abort_if( + $recorded !== null + && $contained->contains(fn (File $file): bool => ! $this->allowance->allows($file, $requester)), + 403, + __('Those files have reached their download limit.'), + ); + + // Atomic, so two fetches arriving together are still one delivery: + // only the request that actually moves delivered_at logs anything. + // Same reasoning as the conditional increment guarding a share + // link's max_downloads in PublicShareController. The other request + // still receives the archive — that is the re-fetch rule above. + $claimed = ZipDownload::query() + ->whereKey($zipDownload->id) + ->whereNull('delivered_at') + ->update(['delivered_at' => now()]); + + if ($claimed === 0) { + return; + } + + // Every file actually bundled gets a FileDownloaded entry — + // otherwise a file's download history/count would silently miss + // zip downloads. + foreach ($contained as $file) { + $this->activity->log(Action::FileDownloaded, subject: $file); + } + } + + /** + * What an archive built before the job recorded its contents is taken + * to hold: the selection, resolved again, which is how this worked + * throughout. Only reachable for rows written by an older release, + * and PurgeZipDownloadsCommand removes those within a day. + * + * @return Collection + */ + private function resolveSelection(ZipDownload $zipDownload, User $requester): Collection { - // Same per-file filter the job used to decide what actually went - // into the archive, so the log records what was really downloaded - // rather than everything that happened to sit in the folder. $visible = $this->viewable->for($requester); $fileIds = collect($zipDownload->file_ids); @@ -220,9 +287,7 @@ class ZipDownloadsController extends Controller // further past it. $skipped = collect($zipDownload->skipped_files ?? [])->pluck('id')->all(); - foreach ((clone $visible)->whereIn('id', $fileIds->unique())->whereNotIn('id', $skipped)->get() as $file) { - $this->activity->log(Action::FileDownloaded, subject: $file); - } + return (clone $visible)->whereIn('id', $fileIds->unique())->whereNotIn('id', $skipped)->get(); } private function filenameFor(ZipDownload $zipDownload): string diff --git a/app/Modules/Files/Jobs/BuildZipDownloadJob.php b/app/Modules/Files/Jobs/BuildZipDownloadJob.php index 8a4c607b..5ce16862 100644 --- a/app/Modules/Files/Jobs/BuildZipDownloadJob.php +++ b/app/Modules/Files/Jobs/BuildZipDownloadJob.php @@ -122,9 +122,12 @@ class BuildZipDownloadJob implements ShouldQueue $tempFiles = []; $skipped = []; - // Counted rather than derived from $usedNames, which also - // holds the folder entry names. - $added = 0; + // Collected rather than derived from $usedNames, which also + // holds the folder entry names. Recording the ids, not just a + // count, is what lets the download action log exactly what it + // hands over instead of resolving the selection a second time + // against a scope that may have moved since. + $addedIds = []; foreach ((clone $visible)->whereIn('id', $zipDownload->file_ids)->get() as $file) { // Re-checked here for the same reason visibility is: the @@ -139,11 +142,11 @@ class BuildZipDownloadJob implements ShouldQueue $entryName = $this->dedupeName($usedNames, $this->entrySegment($file->original_name)); $zip->addFile($this->localPathFor($file, $tempFiles), $entryName); $totalSize += $file->size; - $added++; + $addedIds[] = $file->id; } foreach (Folder::query()->whereIn('id', $zipDownload->folder_ids)->get() as $folder) { - $totalSize += $this->addFolder($zip, $folder, $requester, $usedNames, $tempFiles, $visible, $skipped, $added); + $totalSize += $this->addFolder($zip, $folder, $requester, $usedNames, $tempFiles, $visible, $skipped, $addedIds); } // Re-checked here, not only in ZipDownloadsController: the @@ -186,7 +189,7 @@ class BuildZipDownloadJob implements ShouldQueue @unlink($tempFile); } - if ($written !== true || $added === 0) { + if ($written !== true || $addedIds === []) { if ($written !== true) { // What the requester sees stays generic: a libzip // string means nothing to them and can name a server @@ -218,7 +221,8 @@ class BuildZipDownloadJob implements ShouldQueue 'status' => ZipDownload::STATUS_READY, 'path' => $relativePath, 'total_size' => $totalSize, - 'file_count' => $added, + 'file_count' => count($addedIds), + 'contained_file_ids' => $addedIds, 'skipped_files' => $skipped === [] ? null : $skipped, ]); } catch (Throwable $e) { @@ -317,8 +321,9 @@ class BuildZipDownloadJob implements ShouldQueue * @param array $tempFiles * @param Builder $visible every file the requester may read * @param list $skipped + * @param list $addedIds every file really written into the archive */ - private function addFolder(ZipArchive $zip, Folder $folder, User $requester, array &$usedNames, array &$tempFiles, Builder $visible, array &$skipped, int &$added): int + private function addFolder(ZipArchive $zip, Folder $folder, User $requester, array &$usedNames, array &$tempFiles, Builder $visible, array &$skipped, array &$addedIds): int { $allowance = app(DownloadAllowance::class); @@ -344,7 +349,7 @@ class BuildZipDownloadJob implements ShouldQueue $entryPath = $this->dedupeName($usedNames, $entryPath); $zip->addFile($this->localPathFor($file, $tempFiles), $entryPath); $totalSize += $file->size; - $added++; + $addedIds[] = $file->id; } return $totalSize; diff --git a/app/Modules/Files/Models/ZipDownload.php b/app/Modules/Files/Models/ZipDownload.php index 41c0e5fc..46db22a8 100644 --- a/app/Modules/Files/Models/ZipDownload.php +++ b/app/Modules/Files/Models/ZipDownload.php @@ -22,6 +22,7 @@ use Illuminate\Support\Carbon; * @property string|null $error * @property list $file_ids * @property list $folder_ids + * @property list|null $contained_file_ids * @property list|null $skipped_files * @property Carbon|null $delivered_at */ @@ -40,6 +41,7 @@ class ZipDownload extends Model return [ 'file_ids' => 'array', 'folder_ids' => 'array', + 'contained_file_ids' => 'array', 'skipped_files' => 'array', 'delivered_at' => 'datetime', ]; diff --git a/database/migrations/2026_09_04_090000_add_contained_files_to_zip_downloads_table.php b/database/migrations/2026_09_04_090000_add_contained_files_to_zip_downloads_table.php new file mode 100644 index 00000000..e346f927 --- /dev/null +++ b/database/migrations/2026_09_04_090000_add_contained_files_to_zip_downloads_table.php @@ -0,0 +1,32 @@ +json('contained_file_ids')->nullable()->after('folder_ids'); + }); + } + + public function down(): void + { + Schema::table('zip_downloads', function (Blueprint $table) { + $table->dropColumn('contained_file_ids'); + }); + } +}; diff --git a/tests/Feature/Files/DownloadLimitEnforcementTest.php b/tests/Feature/Files/DownloadLimitEnforcementTest.php index 913e35d3..c05b533a 100644 --- a/tests/Feature/Files/DownloadLimitEnforcementTest.php +++ b/tests/Feature/Files/DownloadLimitEnforcementTest.php @@ -236,3 +236,146 @@ test('re-fetching one prepared zip does not count as downloading everything agai expect($file->downloads()->count())->toBe(1) ->and($zip->fresh()->delivered_at)->not->toBeNull(); }); + +test('a prepared zip cannot be collected once its files have been spent elsewhere', function () { + $file = limitedFile(); + shareFileWith($file, $this->client); + + // Two archives of the same file, both ordered and both built before + // either is collected. Nothing is spent yet, so every check made + // while ordering and building passes for both of them. + $first = $this->actingAs($this->client) + ->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk(); + $second = $this->actingAs($this->client) + ->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk(); + + $firstZip = ZipDownload::query()->findOrFail($first->json('id')); + $secondZip = ZipDownload::query()->findOrFail($second->json('id')); + + expect($firstZip->file_count)->toBe(1) + ->and($secondZip->file_count)->toBe(1); + + $this->actingAs($this->client)->get("/zip-downloads/{$firstZip->id}/download")->assertOk(); + + // The single allowed download is now spent. The second archive was + // built while it still was not, and holding it must not be a way to + // take a copy that is no longer allowed. + $this->actingAs($this->client)->get("/zip-downloads/{$secondZip->id}/download")->assertForbidden(); + + expect($file->downloads()->count())->toBe(1) + ->and($secondZip->fresh()->delivered_at)->toBeNull(); +}); + +test('one spent file refuses the whole archive rather than part of it', function () { + $folder = makeFolder('Shared'); + + $spendable = limitedFile(['name' => 'Spendable', 'original_name' => 'spendable.pdf']); + $uncapped = limitedFile(['name' => 'Uncapped', 'original_name' => 'uncapped.pdf', 'download_limit' => null]); + + $spendable->update(['folder_id' => $folder->id]); + $uncapped->update(['folder_id' => $folder->id]); + + $staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']); + + $response = $this->actingAs($staff) + ->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + + $zip = ZipDownload::query()->findOrFail($response->json('id')); + expect($zip->file_count)->toBe(2); + + // Spent after the archive was built, so the copy inside it is one the + // limit no longer covers. + spend($spendable, $staff); + + $this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertForbidden(); + + // Refused as a whole: the file that was still free is not counted as + // downloaded either, since nothing was handed over. + expect($uncapped->downloads()->count())->toBe(0); +}); + +test('a spent file that is not in the archive does not refuse it', function () { + $folder = makeFolder('Shared'); + + $bundled = limitedFile(['name' => 'Bundled', 'original_name' => 'bundled.pdf', 'download_limit' => null]); + $bundled->update(['folder_id' => $folder->id]); + + $staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']); + + $response = $this->actingAs($staff) + ->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + + $zip = ZipDownload::query()->findOrFail($response->json('id')); + + // Lands in the folder after the archive was written, and is already + // spent. The selection would resolve to it now; the archive does not + // hold it, so it has no say over handing that archive over. + $late = limitedFile(['name' => 'Late', 'original_name' => 'late.pdf']); + $late->update(['folder_id' => $folder->id]); + spend($late, $staff); + + $this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertOk(); + + // Still only the download that spent it — the delivery neither + // refused over it nor counted it. + expect($bundled->downloads()->count())->toBe(1) + ->and($late->downloads()->count())->toBe(1); +}); + +test('an archive from before its contents were recorded is handed over as it always was', function () { + $folder = makeFolder('Shared'); + + $bundled = limitedFile(['name' => 'Bundled', 'original_name' => 'bundled.pdf', 'download_limit' => null]); + $bundled->update(['folder_id' => $folder->id]); + + $staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']); + + $response = $this->actingAs($staff) + ->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + + $zip = ZipDownload::query()->findOrFail($response->json('id')); + + // Stands in for a row written by an older release, which has no + // record of what went into the archive. + ZipDownload::query()->whereKey($zip->id)->update(['contained_file_ids' => null]); + + // A spent file joins the folder afterwards. Resolving the selection + // again — all such a row can do — would sweep it up and refuse over a + // file the archive does not hold, so this path refuses nothing. + $late = limitedFile(['name' => 'Late', 'original_name' => 'late.pdf']); + $late->update(['folder_id' => $folder->id]); + spend($late, $staff); + + $this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertOk(); + + expect($bundled->downloads()->count())->toBe(1) + ->and($zip->fresh()->delivered_at)->not->toBeNull(); +}); + +test('two fetches of one archive arriving together still count as one delivery', function () { + $file = limitedFile(['download_limit' => null]); + shareFileWith($file, $this->client); + + $response = $this->actingAs($this->client) + ->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk(); + + $zip = ZipDownload::query()->findOrFail($response->json('id')); + + // Stands in for a second fetch of the same archive that arrives at + // the same moment and claims the delivery first. The request below + // has already read the row by then, so its own copy still says the + // archive has never been handed over — the check-then-set this + // replaced would believe it and count everything a second time. + ZipDownload::retrieved(function (ZipDownload $retrieved): void { + ZipDownload::query() + ->whereKey($retrieved->id) + ->whereNull('delivered_at') + ->update(['delivered_at' => now()]); + }); + + $this->actingAs($this->client)->get("/zip-downloads/{$zip->id}/download")->assertOk(); + + // Losing the claim means not logging: the fetch that won it is the + // one that counts, and here that is the stand-in, which logs nothing. + expect($file->downloads()->count())->toBe(0); +}); diff --git a/tests/Feature/Files/ZipDownloadsTest.php b/tests/Feature/Files/ZipDownloadsTest.php index 6746f65d..591c80cb 100644 --- a/tests/Feature/Files/ZipDownloadsTest.php +++ b/tests/Feature/Files/ZipDownloadsTest.php @@ -177,6 +177,44 @@ test('the download endpoint logs a FileDownloaded entry for every bundled file', ->assertHeader('Content-Disposition', 'attachment; filename="Reports.zip"'); expect(ActivityLog::query()->where('action', Action::FileDownloaded)->where('subject_id', $file->id)->exists())->toBeTrue(); + + expect($zipDownload->fresh()->contained_file_ids)->toBe([$file->id]); +}); + +test('a file added to the folder after the build is not counted as downloaded', function () { + $folder = Folder::query()->create(['name' => 'Reports']); + $bundled = zipUploadFile($this->admin, 'a.pdf', $folder->id); + + $response = $this->actingAs($this->admin)->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + $zipDownload = ZipDownload::query()->findOrFail($response->json('id')); + + // The archive is written by now. Anything that lands in the folder + // from here on is not in it. + $late = zipUploadFile($this->admin, 'b.pdf'); + $late->update(['folder_id' => $folder->id]); + + $this->actingAs($this->admin)->get("/zip-downloads/{$zipDownload->id}/download")->assertOk(); + + expect(zipEntryNames($zipDownload))->toBe(['Reports/a.pdf']) + ->and($bundled->downloads()->count())->toBe(1) + ->and($late->downloads()->count())->toBe(0); +}); + +test('a file moved out of the folder after the build is still counted', function () { + $folder = Folder::query()->create(['name' => 'Reports']); + $elsewhere = Folder::query()->create(['name' => 'Elsewhere']); + $file = zipUploadFile($this->admin, 'a.pdf', $folder->id); + + $response = $this->actingAs($this->admin)->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + $zipDownload = ZipDownload::query()->findOrFail($response->json('id')); + + // Moving it does not take its bytes back out of the archive, so the + // download still hands it over and still has to say so. + $file->update(['folder_id' => $elsewhere->id]); + + $this->actingAs($this->admin)->get("/zip-downloads/{$zipDownload->id}/download")->assertOk(); + + expect($file->downloads()->count())->toBe(1); }); test('a pending zip download 404s until ready', function () {