Files
projectsend/tests/Feature/Files/ClientFileEditingTest.php
T
ignacionelson 922be7226c Let a client edit and delete the files they uploaded
A client could upload a file and then never touch it again. No rename, no
description, no expiry, no categories, no delete — the portal has three
file routes and all three are GET. Meanwhile the Roles screen happily
grants the Client role edit_files, delete_files, set_file_categories,
set_file_expiration_date and upload_public, and every one of them was
inert, because the routes that honour them are `staff`-gated rather than
permission-gated. That is what #1771 hit: a permission granted, saved, and
silently doing nothing.

A client owns what they uploaded. Ownership is now what lets them edit and
delete it, subject to the same per-field keys staff are subject to.

The obvious implementation is a trap, and it is worth writing down. Both
policy methods began `if (! $user->isStaff()) return false;` and both end
in StaffLibraryScope, whose allowsFile() reads `if (! isClientScoped())
return true` — and isClientScoped() is `isStaff() && role->client_scoped`,
so it is false for every client. Delete the early return and a client
falls into the branch meaning "this staff member is unrestricted" and is
handed the whole library. Same for folders(), which returns an unfiltered
query: a client could move their file into any folder on the installation.
So clients get their own branch, reaching neither. The portal asks
Folder::uploadableBy() instead — a file cannot be moved somewhere it could
not have been uploaded.

edit_others_files and delete_others_files stay inert for clients by
construction. A client has no others' files, only files somebody showed
them, and being shown a file is not being given it.

Which fields an editor may write moved into ApplyFileEdits, shared by the
staff editor, /api/v1 and the portal. There were two copies of the same
eight permission checks and this would have been the third; the checks are
easy, which is exactly why the drift would have been invisible. Callers
normalise their own request shape, this gates and writes and logs. Expiry
reading and writing came along too, as FileExpiry — three copies, of which
only the API's could read a timestamp.

Clients do not choose the public slug. It is derived from the name they
already picked, because an installation-wide unique slug a client sets is
a name to squat and an existence oracle to probe with.

One consequence for later, written up in docs/api-todo.md: the policy now
says yes to a client for file writes, so `staff-token` is the only thing
holding the API boundary where there used to be two independent refusals.
ActorBoundaryTest pins it, and asserts the policy passes first so the test
cannot quietly stop testing the middleware.

Also corrects a stale comment that claimed a deleted file's bytes stay on
disk. They have not since File::booted() grew a `deleted` hook; nothing
ever forceDelete()s a File row, so "until a purge lands" would have meant
never — which is why a client's delete frees their quota by exactly what
it frees on disk.

The UI comes next; this is the authorization, the routes and the tests.

Fixes #1771
2026-09-07 02:37:26 -03:00

365 lines
13 KiB
PHP

<?php
declare(strict_types=1);
use App\Models\User;
use App\Modules\Clients\ClientStorageUsage;
use App\Modules\Files\Models\Category;
use App\Modules\Files\Models\File;
use App\Modules\Files\Models\Folder;
use App\Modules\Identity\Models\Role;
use App\Modules\Identity\Models\RolePermission;
use Illuminate\Support\Facades\Storage;
use Illuminate\Support\Str;
beforeEach(function () {
Storage::fake('files');
$this->admin = User::factory()->create();
});
/** A client whose role carries exactly the given permission keys. */
function clientWithPermissions(array $permissions): User
{
$role = Role::query()->create(['name' => 'Client Role '.Str::random(6)]);
foreach ($permissions as $permission) {
RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission]);
}
return User::factory()->client()->create(['role_id' => $role->id]);
}
/** A stored file owned by $owner, with real bytes on the fake disk. */
function ownedFile(User $owner, array $overrides = []): File
{
$path = 'uploads/'.Str::uuid()->toString().'.pdf';
Storage::disk('files')->put($path, 'hello-world');
return File::factory()->create([
'uploaded_by' => $owner->id,
'name' => 'report',
'original_name' => 'report.pdf',
'mime_type' => 'application/pdf',
'size' => 11,
...$overrides,
'path' => $path,
'disk' => 'files',
]);
}
/** The full form payload the portal editor posts, overridable per test. */
function clientEditPayload(array $overrides = []): array
{
return array_merge(['name' => 'renamed'], $overrides);
}
/*
|--------------------------------------------------------------------------
| The rule
|--------------------------------------------------------------------------
|
| A client owns what they uploaded, and owning it is what lets them edit
| and delete it — subject to the same per-field keys staff are subject to.
*/
test('a client with edit_files can rename their own upload', function () {
$client = clientWithPermissions(['edit_files']);
$file = ownedFile($client);
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['description' => 'now with a description']))
->assertRedirect();
expect($file->refresh()->name)->toBe('renamed')
->and($file->description)->toBe('now with a description');
});
test('a client with delete_files can delete their own upload, and the bytes go with it', function () {
$client = clientWithPermissions(['delete_files']);
$file = ownedFile($client);
expect(app(ClientStorageUsage::class)->usedBytes($client))->toBe(11);
$this->actingAs($client)->delete("/my-files/{$file->id}")->assertRedirect('/my-files');
expect(File::withTrashed()->findOrFail($file->id)->trashed())->toBeTrue();
Storage::disk('files')->assertMissing($file->path);
// The quota frees by exactly what the disk did. These two have to agree
// or a client pays rent on bytes that are gone — nothing ever
// forceDelete()s a File row, so "temporarily" would mean forever.
expect(app(ClientStorageUsage::class)->usedBytes($client))->toBe(0);
});
/*
|--------------------------------------------------------------------------
| Ownership is the boundary
|--------------------------------------------------------------------------
*/
test('a client cannot edit or delete another client\'s file', function () {
$client = clientWithPermissions(['edit_files', 'delete_files']);
$stranger = User::factory()->client()->create();
$file = ownedFile($stranger);
$this->actingAs($client)->patch("/my-files/{$file->id}", clientEditPayload())->assertForbidden();
$this->actingAs($client)->delete("/my-files/{$file->id}")->assertForbidden();
expect($file->refresh()->name)->toBe('report')
->and($file->trashed())->toBeFalse();
});
// The keys exist for staff, where "others' files" is a real category. A
// client has no others' files — only files somebody showed them — so these
// two must buy nothing at all. FilePolicy's client branch never reads them.
test('edit_others_files and delete_others_files buy a client nothing', function () {
$client = clientWithPermissions([
'edit_files', 'delete_files', 'edit_others_files', 'delete_others_files',
]);
$stranger = User::factory()->client()->create();
$file = ownedFile($stranger);
$this->actingAs($client)->patch("/my-files/{$file->id}", clientEditPayload())->assertForbidden();
$this->actingAs($client)->delete("/my-files/{$file->id}")->assertForbidden();
});
// Being shown a file is not being given it. This is the case a client is
// most likely to try, because the file is sitting right there in their list.
test('a client cannot edit a file staff merely shared with them', function () {
$client = clientWithPermissions(['edit_files', 'delete_files']);
$file = ownedFile($this->admin);
$this->actingAs($this->admin)
->post("/files/{$file->id}/assignments", ['type' => 'client', 'id' => $client->id])
->assertRedirect();
// Visible to them...
$this->actingAs($client)->get('/my-files')->assertOk();
// ...and still not theirs.
$this->actingAs($client)->patch("/my-files/{$file->id}", clientEditPayload())->assertForbidden();
$this->actingAs($client)->delete("/my-files/{$file->id}")->assertForbidden();
});
test('a client without edit_files cannot edit their own upload', function () {
$client = clientWithPermissions([]);
$file = ownedFile($client);
$this->actingAs($client)->patch("/my-files/{$file->id}", clientEditPayload())->assertForbidden();
expect($file->refresh()->name)->toBe('report');
});
test('a client without delete_files cannot delete their own upload', function () {
$client = clientWithPermissions(['edit_files']);
$file = ownedFile($client);
$this->actingAs($client)->delete("/my-files/{$file->id}")->assertForbidden();
expect($file->refresh()->trashed())->toBeFalse();
});
test('staff cannot reach the portal routes, and a client cannot reach the staff ones', function () {
$client = clientWithPermissions(['edit_files', 'delete_files']);
$file = ownedFile($client);
// The staff editor is `staff` middleware, not a permission — so holding
// the key changes nothing. A GET is sent home rather than refused
// (EnsureStaff: staff pages are not part of a client's world); the
// writes are a hard 403.
$this->actingAs($client)->get("/files/{$file->id}")->assertRedirect(route('dashboard'));
$this->actingAs($client)->patch("/files/{$file->id}", clientEditPayload())->assertForbidden();
$this->actingAs($client)->delete("/files/{$file->id}")->assertForbidden();
// And the portal route refuses a staff account rather than quietly
// giving it a second way to edit.
$this->actingAs($this->admin)->patch("/my-files/{$file->id}", clientEditPayload())->assertNotFound();
});
/*
|--------------------------------------------------------------------------
| Per-field keys
|--------------------------------------------------------------------------
|
| Lacking the key leaves the field alone and the rest of the edit still
| saves — the rule the staff editor and the API already follow. A client
| must not be the one surface where a missing key fails the request.
*/
test('a client without upload_public cannot publish, and the rename still saves', function () {
$client = clientWithPermissions(['edit_files']);
$file = ownedFile($client);
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['public' => true]))
->assertRedirect();
expect($file->refresh()->public)->toBeFalse()
->and($file->name)->toBe('renamed');
});
test('a client with upload_public can publish their own upload', function () {
$client = clientWithPermissions(['edit_files', 'upload_public']);
$file = ownedFile($client);
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['public' => true]))
->assertRedirect();
expect($file->refresh()->public)->toBeTrue()
->and($file->slug)->not->toBe('');
});
// The slug is derived, never chosen. An installation-wide unique slug a
// client picks is a name to squat and an oracle to probe with.
test('a client cannot choose the public slug', function () {
$client = clientWithPermissions(['edit_files', 'upload_public']);
$file = ownedFile($client);
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload([
'public' => true,
'slug' => 'front-page',
]))
->assertRedirect();
expect($file->refresh()->slug)->not->toBe('front-page');
});
test('a client without set_file_categories cannot categorise', function () {
$client = clientWithPermissions(['edit_files']);
$file = ownedFile($client);
$category = Category::query()->create(['name' => 'Docs']);
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['categories' => [$category->id]]))
->assertRedirect();
expect($file->refresh()->categories)->toHaveCount(0);
});
test('a client without set_file_expiration_date cannot set an expiry', function () {
$client = clientWithPermissions(['edit_files']);
$file = ownedFile($client);
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['expires_at' => now()->addWeek()->toDateString()]))
->assertRedirect();
expect($file->refresh()->expires_at)->toBeNull();
});
test('a client without limit_downloads cannot cap downloads', function () {
$client = clientWithPermissions(['edit_files']);
$file = ownedFile($client);
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['download_limit' => 3]))
->assertRedirect();
expect($file->refresh()->download_limit)->toBeNull();
});
test('a client holding the keys can set an expiry, categories and a download cap', function () {
$client = clientWithPermissions([
'edit_files', 'set_file_expiration_date', 'set_file_categories', 'limit_downloads',
]);
$file = ownedFile($client);
$category = Category::query()->create(['name' => 'Docs']);
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload([
'expires_at' => now()->addWeek()->toDateString(),
'categories' => [$category->id],
'download_limit' => 3,
]))
->assertRedirect();
$file->refresh();
expect($file->expires_at)->not->toBeNull()
->and($file->categories)->toHaveCount(1)
->and($file->download_limit)->toBe(3);
});
/*
|--------------------------------------------------------------------------
| The folder trap
|--------------------------------------------------------------------------
|
| StaffLibraryScope::allowsFolder() returns true for anyone who is not
| client-*scoped* staff, and User::isClientScoped() is false for every
| client. A client reaching the staff guard would be handed every folder on
| the installation. These pin that they never do.
*/
test('a client cannot move their file into a folder they could not upload to', function () {
$client = clientWithPermissions(['edit_files']);
$file = ownedFile($client);
$staffOnly = makeFolder('Internal');
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['folder_id' => $staffOnly->id]))
->assertForbidden();
expect($file->refresh()->folder_id)->toBeNull();
});
test('a client cannot move their file into another client\'s folder', function () {
$client = clientWithPermissions(['edit_files']);
$stranger = User::factory()->client()->create();
$file = ownedFile($client);
$this->actingAs($stranger)->post('/my-folders', ['name' => 'Theirs'])->assertRedirect();
$theirs = Folder::query()->where('name', 'Theirs')->sole();
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['folder_id' => $theirs->id]))
->assertForbidden();
expect($file->refresh()->folder_id)->toBeNull();
});
test('a client can move their file into a folder they created', function () {
$client = clientWithPermissions(['edit_files', 'create_own_folders', 'upload']);
$file = ownedFile($client);
$this->actingAs($client)->post('/my-folders', ['name' => 'Mine'])->assertRedirect();
$mine = Folder::query()->where('name', 'Mine')->sole();
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload(['folder_id' => $mine->id]))
->assertRedirect();
expect($file->refresh()->folder_id)->toBe($mine->id);
});
/*
|--------------------------------------------------------------------------
| What the payload must not reach
|--------------------------------------------------------------------------
*/
test('a client cannot hand their file to somebody else, or repoint its bytes', function () {
$client = clientWithPermissions(['edit_files']);
$stranger = User::factory()->client()->create();
$file = ownedFile($client);
$originalPath = $file->path;
$this->actingAs($client)
->patch("/my-files/{$file->id}", clientEditPayload([
'uploaded_by' => $stranger->id,
'path' => 'uploads/somebody-elses-file.pdf',
'disk' => 'nonexistent-disk',
'size' => 999999999,
]))
->assertRedirect();
$file->refresh();
expect($file->uploaded_by)->toBe($client->id)
->and($file->path)->toBe($originalPath)
->and($file->disk)->toBe('files')
->and($file->size)->toBe(11);
});