From 757fba19cabdad30614fa3bfd383b0420598e148 Mon Sep 17 00:00:00 2001
From: ignacionelson <info@subwaydesign.com.ar>
Date: Tue, 8 Sep 2026 15:39:55 -0300
Subject: [PATCH] Give uploads a seam, and link-minting one home
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two pieces of groundwork, no behaviour change.

FileWasStored is dispatched from StoreUploadedFile, which every upload
path converges on — the chunked flow staff and clients share, and the
synchronous POST beside it. A listener therefore sees each upload once
without knowing which route produced it, which is the property that makes
it usable from outside this repository. A notification, not a filter:
nothing on it is mutable, and anything that needs to influence an upload
has to do so before the bytes land, which is what ResolvingUploadDisk is
already for.

CreateShareLink is the other half. Minting a link was a ShareLinksController
private concern, and the controller is an HTTP handler behind `staff`
middleware — so a link now needs making from outside a request as well.
Two copies of "make a token, write the row, log it" would drift, and the
half most likely to drift is the token, which is the entire authorization
for /s/{token}: there is no session behind it and no second factor, so
being unguessable is its only defence. Anything minted through the action
gets Str::random(32) — about 190 bits, more than a UUID's 122 — and never
a chosen value. The chosen-token path stays in the controller, where a
person is typing one into a form and its minimum length can be argued
about in a validation rule.

The permission questions stay in the controller too. Whether somebody may
set an expiry or a download cap is a fact about them, and the action has
no viewer to ask; it takes both already resolved, including the expiry,
because "the end of the 12th" depends on whose timezone you are in.

Five tests, including that the file a listener receives is complete and
readable rather than half-built, and that the staff form still refuses an
expiry to somebody without the permission after the extraction.
---
 app/Modules/Files/Events/FileWasStored.php    |  33 ++++++
 .../Http/Controllers/ShareLinksController.php |  22 ++--
 app/Modules/Files/Sharing/CreateShareLink.php |  62 ++++++++++
 .../Files/Uploads/StoreUploadedFile.php       |   9 ++
 tests/Feature/Files/UploadEventTest.php       | 107 ++++++++++++++++++
 5 files changed, 223 insertions(+), 10 deletions(-)
 create mode 100644 app/Modules/Files/Events/FileWasStored.php
 create mode 100644 app/Modules/Files/Sharing/CreateShareLink.php
 create mode 100644 tests/Feature/Files/UploadEventTest.php

diff --git a/app/Modules/Files/Events/FileWasStored.php b/app/Modules/Files/Events/FileWasStored.php
new file mode 100644
index 00000000..49ed855a
--- /dev/null
+++ b/app/Modules/Files/Events/FileWasStored.php
@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Modules\Files\Events;
+
+use App\Models\User;
+use App\Modules\Files\Models\File;
+
+/**
+ * A file's bytes are on a disk and its row exists.
+ *
+ * Dispatched from StoreUploadedFile, which every upload path goes through
+ * — the chunked flow that staff and clients share, and the synchronous
+ * POST beside it — so a listener sees every upload once and does not have
+ * to know which route produced it.
+ *
+ * A notification rather than a filter: nothing here is mutable and no
+ * listener can change what was stored. Anything that needs to influence
+ * the upload has to do so before the bytes land, which is what
+ * ResolvingUploadDisk is for.
+ *
+ * Fired after the row is created and before the caller has linked a
+ * version or answered the request, so a listener sees a complete File and
+ * can safely read it back.
+ */
+class FileWasStored
+{
+    public function __construct(
+        public readonly File $file,
+        public readonly User $uploader,
+    ) {}
+}
diff --git a/app/Modules/Files/Http/Controllers/ShareLinksController.php b/app/Modules/Files/Http/Controllers/ShareLinksController.php
index 846cec80..9e4ab35a 100644
--- a/app/Modules/Files/Http/Controllers/ShareLinksController.php
+++ b/app/Modules/Files/Http/Controllers/ShareLinksController.php
@@ -9,6 +9,7 @@ use App\Modules\Audit\Action;
 use App\Modules\Audit\ActivityLogger;
 use App\Modules\Files\Models\File;
 use App\Modules\Files\Models\ShareLink;
+use App\Modules\Files\Sharing\CreateShareLink;
 use App\Modules\Platform\Localization\LocalDay;
 use App\Modules\Platform\Localization\TimezoneRegistry;
 use Illuminate\Http\RedirectResponse;
@@ -30,6 +31,7 @@ class ShareLinksController extends Controller
     public function __construct(
         private readonly ActivityLogger $activity,
         private readonly TimezoneRegistry $timezones,
+        private readonly CreateShareLink $links,
     ) {}
 
     public function store(Request $request, File $file): RedirectResponse
@@ -80,16 +82,16 @@ class ShareLinksController extends Controller
             ]);
         }
 
-        ShareLink::query()->create([
-            'shareable_type' => $file->getMorphClass(),
-            'shareable_id' => $file->id,
-            'token' => $validated['token'] ?? Str::random(32),
-            'created_by' => $user->id,
-            'expires_at' => $user->can('set_file_expiration_date') ? $expiresAt : null,
-            'max_downloads' => $user->can('limit_downloads') ? $validated['max_downloads'] ?? null : null,
-        ]);
-
-        $this->activity->log(Action::ShareLinkCreated, subject: $file);
+        // The permission gates stay here, where the request is: whether
+        // this person may set an expiry or a cap is a fact about them,
+        // not about link creation, and the action has no viewer to ask.
+        $this->links->for(
+            file: $file,
+            creator: $user,
+            expiresAt: $user->can('set_file_expiration_date') ? $expiresAt : null,
+            maxDownloads: $user->can('limit_downloads') ? $validated['max_downloads'] ?? null : null,
+            token: $validated['token'] ?? null,
+        );
 
         return back()->with('success', __('Public link created.'));
     }
diff --git a/app/Modules/Files/Sharing/CreateShareLink.php b/app/Modules/Files/Sharing/CreateShareLink.php
new file mode 100644
index 00000000..79ecc497
--- /dev/null
+++ b/app/Modules/Files/Sharing/CreateShareLink.php
@@ -0,0 +1,62 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Modules\Files\Sharing;
+
+use App\Models\User;
+use App\Modules\Audit\Action;
+use App\Modules\Audit\ActivityLogger;
+use App\Modules\Files\Models\File;
+use App\Modules\Files\Models\ShareLink;
+use Carbon\CarbonInterface;
+use Illuminate\Support\Str;
+
+/**
+ * Minting a public link for a file, in one place.
+ *
+ * Extracted from ShareLinksController rather than invented: the
+ * controller is an HTTP handler behind `staff` middleware, and a link now
+ * needs creating from outside a request as well. Two copies of "make a
+ * token, write the row, log it" would drift, and the half most likely to
+ * drift is the token.
+ *
+ * **The token is the whole authorization.** There is nothing behind
+ * /s/{token} — no session, no second factor — so its only defence is
+ * being unguessable. Str::random(32) is about 190 bits, which is more
+ * than a UUID's 122; anything minted here gets that and never a chosen
+ * value. A caller that wants a chosen token is a person typing one into a
+ * form, and that path stays in the controller where its minimum length
+ * can be argued about in a validation rule.
+ *
+ * Expiry and download caps are the caller's to decide and are passed in
+ * already resolved, because "the end of the 12th" depends on whose zone
+ * you are in and this class has no viewer.
+ */
+class CreateShareLink
+{
+    public function __construct(
+        private readonly ActivityLogger $activity,
+    ) {}
+
+    public function for(
+        File $file,
+        User $creator,
+        ?CarbonInterface $expiresAt = null,
+        ?int $maxDownloads = null,
+        ?string $token = null,
+    ): ShareLink {
+        $link = ShareLink::query()->create([
+            'shareable_type' => $file->getMorphClass(),
+            'shareable_id' => $file->id,
+            'token' => $token ?? Str::random(32),
+            'created_by' => $creator->id,
+            'expires_at' => $expiresAt,
+            'max_downloads' => $maxDownloads,
+        ]);
+
+        $this->activity->log(Action::ShareLinkCreated, subject: $file);
+
+        return $link;
+    }
+}
diff --git a/app/Modules/Files/Uploads/StoreUploadedFile.php b/app/Modules/Files/Uploads/StoreUploadedFile.php
index 4c05652e..b25a4857 100644
--- a/app/Modules/Files/Uploads/StoreUploadedFile.php
+++ b/app/Modules/Files/Uploads/StoreUploadedFile.php
@@ -5,6 +5,8 @@ declare(strict_types=1);
 namespace App\Modules\Files\Uploads;
 
 use App\Models\User;
+use App\Modules\Files\Events\FileWasStored;
+use Illuminate\Support\Facades\Event;
 use App\Modules\Audit\Action;
 use App\Modules\Audit\ActivityLogger;
 use App\Modules\Files\Models\File;
@@ -52,6 +54,13 @@ class StoreUploadedFile
 
         $this->activity->log($action, $uploader, $file);
 
+        // Every upload path converges here — the chunked flow staff and
+        // clients share, and the synchronous POST beside it — so a
+        // listener sees each upload once without knowing which route
+        // produced it. Dispatched after the row exists, so what it
+        // receives is a complete File.
+        Event::dispatch(new FileWasStored($file, $uploader));
+
         return $file;
     }
 
diff --git a/tests/Feature/Files/UploadEventTest.php b/tests/Feature/Files/UploadEventTest.php
new file mode 100644
index 00000000..5611777d
--- /dev/null
+++ b/tests/Feature/Files/UploadEventTest.php
@@ -0,0 +1,107 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Models\User;
+use App\Modules\Files\Events\FileWasStored;
+use App\Modules\Files\Models\File;
+use App\Modules\Files\Sharing\CreateShareLink;
+use Illuminate\Support\Facades\Event;
+use Illuminate\Support\Facades\Storage;
+
+beforeEach(function () {
+    Storage::fake('files');
+    $this->admin = User::factory()->create();
+});
+
+/*
+|--------------------------------------------------------------------------
+| One seam for every upload path
+|--------------------------------------------------------------------------
+|
+| Dispatched from StoreUploadedFile rather than from a controller, because
+| that is where the chunked flow and the synchronous POST converge. A
+| listener registered elsewhere — a package, say — should not have to know
+| which route a file arrived by.
+*/
+
+test('storing a file announces it, whichever path stored it', function () {
+    Event::fake([FileWasStored::class]);
+
+    $this->actingAs($this->admin)->post('/files', [
+        'file' => Illuminate\Http\UploadedFile::fake()->create('report.pdf', 12, 'application/pdf'),
+        'name' => '',
+        'description' => '',
+    ])->assertRedirect();
+
+    Event::assertDispatched(FileWasStored::class, function (FileWasStored $event): bool {
+        return $event->file->original_name === 'report.pdf'
+            && $event->uploader->is($this->admin);
+    });
+});
+
+// The row has to be complete when a listener sees it, or a listener that
+// reads the file back gets a half-built one.
+test('the file it carries is already stored and readable', function () {
+    $seen = null;
+    Event::listen(FileWasStored::class, function (FileWasStored $event) use (&$seen): void {
+        $seen = File::query()->find($event->file->id);
+    });
+
+    $this->actingAs($this->admin)->post('/files', [
+        'file' => Illuminate\Http\UploadedFile::fake()->create('a.pdf', 5, 'application/pdf'),
+        'name' => '',
+        'description' => '',
+    ])->assertRedirect();
+
+    expect($seen)->not->toBeNull()
+        ->and($seen->uploaded_by)->toBe($this->admin->id)
+        ->and(Storage::disk($seen->disk)->exists($seen->path))->toBeTrue();
+});
+
+/*
+|--------------------------------------------------------------------------
+| Minting a link, from outside a request
+|--------------------------------------------------------------------------
+*/
+
+test('it mints a long random token and never a chosen one by default', function () {
+    $file = File::factory()->create(['uploaded_by' => $this->admin->id]);
+
+    $link = app(CreateShareLink::class)->for($file, $this->admin);
+
+    // The token is the whole authorization for /s/{token} — there is
+    // nothing behind it — so its only defence is being unguessable.
+    // 32 characters is about 190 bits, more than a UUID's 122.
+    expect(strlen($link->token))->toBe(32)
+        ->and($link->expires_at)->toBeNull()
+        ->and($link->max_downloads)->toBeNull()
+        ->and($link->created_by)->toBe($this->admin->id);
+});
+
+test('two links for the same file never share a token', function () {
+    $file = File::factory()->create(['uploaded_by' => $this->admin->id]);
+    $action = app(CreateShareLink::class);
+
+    expect($action->for($file, $this->admin)->token)
+        ->not->toBe($action->for($file, $this->admin)->token);
+});
+
+// The controller still owns the permission questions — whether this
+// person may set an expiry or a cap is a fact about them, and the action
+// has no viewer to ask.
+test('the staff form still refuses an expiry to somebody without the permission', function () {
+    $file = File::factory()->create(['uploaded_by' => $this->admin->id]);
+    $limited = staffWithPermissions(['upload', 'edit_files']);
+    $file->forceFill(['uploaded_by' => $limited->id])->save();
+
+    $this->actingAs($limited)->post("/files/{$file->id}/share-links", [
+        'expires_at' => now()->addWeek()->toDateString(),
+        'max_downloads' => 3,
+    ])->assertRedirect();
+
+    $link = $file->shareLinks()->sole();
+
+    expect($link->expires_at)->toBeNull()
+        ->and($link->max_downloads)->toBeNull();
+});