mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
denkfabrik-li
5e60d2ef88
File::isExpired() documents the rule the application is supposed to
follow: once past, the file is hidden from clients and the public site
"but staff keep full access to view, download, and manage it".
The second half is not true of a client-scoped staff member.
StaffLibraryScope::buildFiles() builds their library as their own uploads
union what each assigned client may see, and that second half runs
through File::scopeVisibleToClient, which ends in notExpired() -- a
client-side rule. Measured on main, with a rep holding one client and a
file the administrator uploaded and shared with that client:
before expiry in_library true GET .../download -> 200
after expiry in_library false GET .../download -> 403
the rep's own expired upload in_library true
an unscoped administrator, same expired file in_library true
Api\FilesController says it the same way -- "Only the client branch of
the visibility rules drops them" -- which reads as though a staff caller
is unaffected, when a client-scoped one is reached through that very
branch.
This does not change that behaviour. c8078f65 weighed exactly this and
decided against it: widening it would mean a library query that keeps
expired rows, and scopeVisibleToClient is the single source of truth for
client file access, the highest-stakes function to go changing for a
dashboard widget. The widget was relabelled instead.
That decision lives in a commit message and in one widget's label.
Nothing in the code said it, and the docblock nearest the rule went on
promising the opposite -- which is how the next person re-derives "staff
keep full access" and widens something.
So both comments now state the boundary and why it is where it is, and
ExpiredFileStaffAccessTest makes it executable: an unscoped staff member
keeps an expired file, a client-scoped one keeps their own expired
upload, a client-scoped one loses a client's file when it expires.
Not changed: scopeVisibleToClient, StaffLibraryScope, and the
expired-files widget. If the boundary should move, that is a separate
conversation and a separate change.
Counter-check inverted, since these pass on unmodified main by
construction -- there is no behaviour fix for them to prove. What they
have to do is fail if the boundary moves, so the mutation is the widening
itself. Deleting the closing notExpired() call from scopeVisibleToClient
turns the file red, 1 failed / 2 passed, and it is the third case, the
one carrying the decision, that falls.
Suite 2108 passed / 2 skipped, 11416 assertions, PHPStan level 8 clean.
Measured on base 06c364d2, where main itself is 2105 / 2.
81 lines
3.2 KiB
PHP
81 lines
3.2 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Files\Access\StaffLibraryScope;
|
|
use App\Modules\Files\Models\File;
|
|
use App\Modules\Identity\Models\Role;
|
|
use App\Modules\Identity\Models\RolePermission;
|
|
use App\Modules\Identity\Permissions\Permission;
|
|
use Illuminate\Support\Facades\Storage;
|
|
use Illuminate\Support\Str;
|
|
|
|
/**
|
|
* Expiry hides a file from clients and the public site; staff keep it.
|
|
* For a client-scoped staff member that is true of their own uploads and
|
|
* not of their clients' files, because the second half of their library
|
|
* is File::scopeVisibleToClient and it ends in notExpired().
|
|
*
|
|
* c8078f65 decided that deliberately rather than widening the scope —
|
|
* "the single source of truth for client file access", changed for a
|
|
* dashboard widget — and relabelled the widget instead. Nothing executed
|
|
* that decision, so File::isExpired's docblock could go on promising
|
|
* staff full access without anything failing. These pin both halves.
|
|
*/
|
|
beforeEach(function () {
|
|
Storage::fake('files');
|
|
$this->admin = User::factory()->create();
|
|
|
|
$role = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]);
|
|
foreach ([Permission::Upload, Permission::EditFiles] as $permission) {
|
|
RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]);
|
|
}
|
|
|
|
$this->rep = User::factory()->create(['role_id' => $role->id]);
|
|
$this->client = User::factory()->client()->create();
|
|
$this->rep->assignedClients()->sync([$this->client->id]);
|
|
});
|
|
|
|
function libraryHoldsFile(User $staff, File $file): bool
|
|
{
|
|
// The scope memoises its built query per user id, so a fresh
|
|
// container is what makes a second question in one test honest.
|
|
app()->forgetInstance(StaffLibraryScope::class);
|
|
|
|
return app(StaffLibraryScope::class)->files($staff)->whereKey($file->id)->exists();
|
|
}
|
|
|
|
test('an unscoped staff member keeps a file after it expires', function () {
|
|
$file = uploadNamedFile($this->admin, 'annual-report');
|
|
shareFileWith($file, $this->client);
|
|
|
|
$file->forceFill(['expires_at' => now()->subDay()])->save();
|
|
|
|
expect(libraryHoldsFile($this->admin, $file))->toBeTrue();
|
|
$this->actingAs($this->admin)->get("/files/{$file->id}/download")->assertOk();
|
|
});
|
|
|
|
test('a client-scoped staff member keeps their own expired upload', function () {
|
|
$file = uploadNamedFile($this->rep, 'my-own-note');
|
|
|
|
$file->forceFill(['expires_at' => now()->subDay()])->save();
|
|
|
|
expect(libraryHoldsFile($this->rep, $file))->toBeTrue();
|
|
});
|
|
|
|
test('a client-scoped staff member loses a client file when it expires', function () {
|
|
$file = uploadNamedFile($this->admin, 'annual-report');
|
|
shareFileWith($file, $this->client);
|
|
|
|
expect(libraryHoldsFile($this->rep, $file))->toBeTrue();
|
|
$this->actingAs($this->rep)->get("/files/{$file->id}/download")->assertOk();
|
|
|
|
$file->forceFill(['expires_at' => now()->subDay()])->save();
|
|
|
|
// Deliberate, not an oversight — see the file docblock above. If this
|
|
// ever changes, File::isExpired's docblock changes with it.
|
|
expect(libraryHoldsFile($this->rep, $file))->toBeFalse();
|
|
$this->actingAs($this->rep)->get("/files/{$file->id}/download")->assertForbidden();
|
|
});
|