mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
ignacionelson
922be7226c
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
68 lines
2.4 KiB
PHP
68 lines
2.4 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
namespace App\Modules\Files\Editing;
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Files\Models\File;
|
|
use App\Modules\Platform\Localization\LocalDay;
|
|
use App\Modules\Platform\Localization\TimezoneRegistry;
|
|
use Carbon\Carbon;
|
|
|
|
/**
|
|
* Reading and writing a file's expiry in the zone of whoever is looking.
|
|
*
|
|
* The stored value is an instant. What a person sets is a calendar day,
|
|
* and "the 12th" means the end of the 12th where *they* live — otherwise a
|
|
* file asked to expire on the 12th dies partway through the 11th for
|
|
* anyone west of Greenwich, and gives anyone east of it most of a day
|
|
* nobody promised.
|
|
*
|
|
* The two halves have to agree, which is the whole reason they sit
|
|
* together: a form is rendered with asShown() and posts the same string
|
|
* back untouched with every other edit, so a caller compares against
|
|
* asShown() to tell "the editor changed the date" from "the editor renamed
|
|
* the file and the date came along for the ride". Re-deriving on every
|
|
* save instead moves the expiry by the difference between two people's
|
|
* zones each time somebody edits anything.
|
|
*
|
|
* Was three private copies — the staff editor, the API, and now the client
|
|
* portal — of which the API's was the only one that could read a
|
|
* timestamp.
|
|
*/
|
|
class FileExpiry
|
|
{
|
|
public function __construct(
|
|
private readonly TimezoneRegistry $timezones,
|
|
) {}
|
|
|
|
/**
|
|
* The stored instant as the calendar day a form should show, in the
|
|
* viewer's zone. Null when the file never expires.
|
|
*/
|
|
public function asShown(File $file, ?User $viewer): ?string
|
|
{
|
|
return $file->expires_at?->copy()->setTimezone($this->timezones->resolve($viewer))->toDateString();
|
|
}
|
|
|
|
/**
|
|
* The instant a submitted value actually names.
|
|
*
|
|
* A bare `YYYY-MM-DD` is a calendar day and means the end of it where
|
|
* the setter is — what every date input posts. Anything carrying a
|
|
* time is an instant somebody named on purpose and is stored as it
|
|
* arrives: the API can express a moment, and a date input cannot.
|
|
*/
|
|
public function instant(?string $value, ?User $setter): ?Carbon
|
|
{
|
|
if ($value === null) {
|
|
return null;
|
|
}
|
|
|
|
return preg_match('/^\d{4}-\d{2}-\d{2}$/', $value) === 1
|
|
? LocalDay::end($value, $this->timezones->resolve($setter))
|
|
: Carbon::parse($value);
|
|
}
|
|
}
|