Files
projectsend/tests/Feature/Files/FolderDeleteContentTest.php
T
denkfabrik-li 26205082c2 Stop a folder deleting the files inside it that its owner may not delete
FoldersController::destroy() authorizes `delete` on the folder and nothing
else. FolderService::delete() then soft-deletes every file in the subtree,
and File::booted()'s `deleted` hook takes the bytes off disk. There is no
restore.

FilePolicy::delete asks two questions the folder route never reaches:
`delete_others_files` for somebody else's upload, and
StaffLibraryScope::allowsFile on top of it. Measured with a role holding
create_own_folders, delete_files, upload and edit_files -- the shape the
Client Manager system role already has, minus delete_others_files:

  DELETE /files/{someone-elses}   403, the file is still there
  DELETE /folders/{their-folder}  302, the file and its bytes are gone

MyFoldersController::destroy already refuses the client half of this exact
cascade, and says why: "Owning the folder is not authority over content
someone else put in it... Refuse rather than silently destroy them." This
is the staff half of the same sentence.

Counted rather than asked per file. A folder can hold thousands, Gate
resolves a fresh policy for every check, and a per-row policy check on a
listing is the cost 0a8b609e went to some trouble to remove. Both halves
of FilePolicy::delete are expressible in SQL: the permission half is
constant for the viewer, and the library half is the query
StaffLibraryScope already memoises per request. Somebody holding both
delete permissions with no library scope short-circuits before the query
runs at all, so the common case pays nothing.

Not changed, deliberately:

- The service. FolderService::delete stays dumb. Its other caller applies
  the client rule ("files you did not upload"), which is a different
  predicate, and putting both in one place is the drift this codebase
  keeps refactoring away from.
- The client half. MyFoldersController is already correct.
- Nothing partial. A blocked folder is left whole rather than emptied of
  what the actor may delete -- half a tree is worse than either answer.

Worth saying plainly: this is a behaviour change. A folder delete that
used to succeed now refuses, and somebody will notice. The alternative is
irreversible loss of files the same person is refused one route over.

Six tests. Four measured red against the unguarded controller (4 failed /
2 passed), one per half of the predicate: the permission half, its
message, a nested file, and the library half -- that last one with both
delete permissions held, so only StaffLibraryScope can refuse. The two
that stay green either way are the other side of the question -- that a
folder holding only your own files still goes, and that an administrator
holding both permissions is unaffected. They guard against the fix
over-refusing, not against the bug.

Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean.

The new string is English only, per CONTRIBUTING.md -- translations are
their own pass.
2026-08-28 00:32:33 +02:00

145 lines
5.5 KiB
PHP

<?php
declare(strict_types=1);
use App\Models\User;
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 App\Modules\Identity\Permissions\Permission;
use Illuminate\Http\UploadedFile;
use Illuminate\Support\Facades\Storage;
use Illuminate\Support\Str;
/**
* Deleting a folder cascades to every file in its subtree, and a File's
* `deleted` hook removes the bytes from disk. So the folder route must
* not destroy what the file route refuses to hand over.
*
* MyFoldersController::destroy already draws this line for clients. These
* are the staff cases.
*/
beforeEach(function () {
Storage::fake('files');
$this->admin = User::factory()->create();
});
function folderDeleteRole(array $permissions, bool $clientScoped = false): User
{
$role = Role::query()->create(['name' => 'Role '.Str::random(6), 'client_scoped' => $clientScoped]);
foreach ($permissions as $permission) {
RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]);
}
return User::factory()->create(['role_id' => $role->id]);
}
function folderDeleteUpload(User $as, string $name, ?int $folderId = null): File
{
test()->actingAs($as)->post('/files', [
'file' => UploadedFile::fake()->create($name.'.pdf', 4, 'application/pdf'),
'name' => '',
'description' => '',
'folder_id' => $folderId,
]);
return File::query()->latest('id')->firstOrFail();
}
test('a folder is not a way around delete_others_files', function () {
$staff = folderDeleteRole([
Permission::CreateOwnFolders, Permission::DeleteFiles,
Permission::Upload, Permission::EditFiles,
]);
$folder = Folder::query()->create(['name' => 'Reports', 'created_by' => $staff->id]);
$foreign = folderDeleteUpload($this->admin, 'someone-elses', $folder->id);
// The file route already refuses this one.
$this->actingAs($staff)->delete("/files/{$foreign->id}")->assertForbidden();
$this->actingAs($staff)->delete("/folders/{$folder->id}")->assertRedirect();
expect(File::query()->whereKey($foreign->id)->exists())->toBeTrue()
->and(Folder::query()->whereKey($folder->id)->exists())->toBeTrue();
});
test('the refusal says how many files are in the way', function () {
$staff = folderDeleteRole([
Permission::CreateOwnFolders, Permission::DeleteFiles,
Permission::Upload, Permission::EditFiles,
]);
$folder = Folder::query()->create(['name' => 'Reports', 'created_by' => $staff->id]);
folderDeleteUpload($this->admin, 'one', $folder->id);
folderDeleteUpload($this->admin, 'two', $folder->id);
$this->actingAs($staff)->delete("/folders/{$folder->id}")
->assertSessionHas('error', fn (string $message): bool => str_contains($message, '2 files'));
});
test('a nested file is reached too', function () {
$staff = folderDeleteRole([
Permission::CreateOwnFolders, Permission::DeleteFiles,
Permission::Upload, Permission::EditFiles,
]);
$parent = Folder::query()->create(['name' => 'Parent', 'created_by' => $staff->id]);
$child = Folder::query()->create([
'name' => 'Child', 'parent_id' => $parent->id,
'path' => "/{$parent->id}/", 'created_by' => $staff->id,
]);
$foreign = folderDeleteUpload($this->admin, 'deep', $child->id);
$this->actingAs($staff)->delete("/folders/{$parent->id}");
expect(File::query()->whereKey($foreign->id)->exists())->toBeTrue();
});
test('the library boundary is asked as well, not only the permission', function () {
$staff = folderDeleteRole([
Permission::CreateOwnFolders, Permission::DeleteFiles,
Permission::DeleteOthersFiles, Permission::Upload, Permission::EditFiles,
], clientScoped: true);
$folder = Folder::query()->create(['name' => 'Scoped', 'created_by' => $staff->id]);
$outside = folderDeleteUpload($this->admin, 'outside-the-library', $folder->id);
// Both delete permissions are held, so the permission half of
// FilePolicy::delete passes and only StaffLibraryScope can refuse --
// which is the half a per-permission check would have missed.
$this->actingAs($staff)->delete("/files/{$outside->id}")->assertForbidden();
$this->actingAs($staff)->delete("/folders/{$folder->id}")->assertRedirect();
expect(File::query()->whereKey($outside->id)->exists())->toBeTrue()
->and(Folder::query()->whereKey($folder->id)->exists())->toBeTrue();
});
test('a folder holding only the deleter own files still goes', function () {
$staff = folderDeleteRole([
Permission::CreateOwnFolders, Permission::DeleteFiles,
Permission::Upload, Permission::EditFiles,
]);
$folder = Folder::query()->create(['name' => 'Mine', 'created_by' => $staff->id]);
$own = folderDeleteUpload($staff, 'my-own', $folder->id);
$this->actingAs($staff)->delete("/folders/{$folder->id}")->assertRedirect();
expect(File::query()->whereKey($own->id)->exists())->toBeFalse()
->and(Folder::query()->whereKey($folder->id)->exists())->toBeFalse();
});
test('an administrator holding both permissions is unaffected', function () {
$folder = Folder::query()->create(['name' => 'Anything', 'created_by' => $this->admin->id]);
$other = User::factory()->create();
$theirs = folderDeleteUpload($other, 'theirs', $folder->id);
$this->actingAs($this->admin)->delete("/folders/{$folder->id}")->assertRedirect();
expect(File::query()->whereKey($theirs->id)->exists())->toBeFalse();
});