mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-17 09:05:08 +00:00
c8de16101f
FilePolicy::view() has two halves for staff: one of the three file keys
(upload / edit_files / edit_others_files), AND StaffLibraryScope. Every
comment surface that spans files narrowed by the library half alone.
A role holding moderate_comments and no file key therefore got a 403 on
every file in the installation while reading every comment written about
them on /comments: the text, staff-only notes, the client name a
Clients-visibility comment carries, and a visitor's IP address. The API
queue answered the same way, and approving through it hands the body back
in the response, so it was a reading door as well as a writing one.
The class says this is not supposed to happen -- across()'s own docblock
("a moderation screen is not a way around the visibility model"), the
route comment on /comments ("the list itself is still narrowed by
VisibleCommentScope, so holding the permission does not widen what a
viewer may read"), and routes/api.php ("reading and writing a comment is
gated by 'may see this file', the same three keys the file endpoints
use"). FileCommentPolicy::view() enforces it for a single comment, by
running the file's own gate first. Only the cross-file queries did not.
So they now take their files from ViewableFileScope, which is
FilePolicy::view() expressed as a query, instead of from StaffLibraryScope,
which is only its second half: across(), pendingTotal() and the API's
pending list. The permission half moves into a named method on that class,
since three modules now ask the same question.
FileCommentPolicy::moderate() gets it too, in both forms. Its row form is
otherwise unchanged -- the library check still runs by file id, so a
comment on a soft-deleted file behaves exactly as before.
No system role changes behaviour: Account Manager and System Administrator
are the two that ship with moderate_comments, and both hold upload. What
changes is a hand-built role that holds moderation and nothing else.
Seven tests. Without the fix, five go red; the other two are the premise
(that the viewer really is refused the file itself) and the guard that a
moderator who may read files still moderates the whole installation.
docs/api/openapi.json regenerated for the one changed description.
539 lines
21 KiB
PHP
539 lines
21 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
namespace App\Modules\Comments\Access;
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Comments\CommentVisibility;
|
|
use App\Modules\Comments\GuestCommentIdentity;
|
|
use App\Modules\Comments\Models\FileComment;
|
|
use App\Modules\Files\Access\ShareTargets;
|
|
use App\Modules\Files\Access\StaffLibraryScope;
|
|
use App\Modules\Files\Access\ViewableFileScope;
|
|
use App\Modules\Files\Models\File;
|
|
use App\Modules\Files\Models\Folder;
|
|
use App\Modules\Identity\UserType;
|
|
use App\Modules\Notifications\InAppNotification;
|
|
use Illuminate\Database\Eloquent\Builder;
|
|
use Illuminate\Database\Eloquent\Collection;
|
|
use Illuminate\Support\Facades\Gate;
|
|
|
|
/**
|
|
* The single place that decides which comments on a file a viewer may
|
|
* read, and — its mirror image — which people a new comment may be
|
|
* announced to.
|
|
*
|
|
* Follows the same convention as Files' FilePolicy/ViewableFileScope pair:
|
|
* one question, expressed once, in SQL, so no caller has to re-derive it.
|
|
* Unlike that pair there is no per-model policy twin doing the same work
|
|
* in PHP; FileCommentPolicy::view() defers to this class rather than
|
|
* restating it, because two statements of a rule this sharp will drift.
|
|
*
|
|
* **Callers must have already established that the viewer may see the
|
|
* file itself** (Gate::authorize('view', $file), or for guests, that the
|
|
* file is publicly reachable). This class narrows an accessible file's
|
|
* comments; it is not a substitute for the file's own gate.
|
|
*
|
|
* The rule that everything else hangs off:
|
|
*
|
|
* A Clients comment carrying client_context_id = C is never returned to
|
|
* any non-staff viewer other than C.
|
|
*
|
|
* That is what stops one customer learning that another exists — a worse
|
|
* failure than leaking a comment's text. A Clients comment with a *null*
|
|
* context is a staff message to everyone on the file, and every client
|
|
* with access reads it: it is written by staff and names no client, so
|
|
* seeing it tells a reader nothing about who else is there.
|
|
*
|
|
* Nothing outside this class may query file_comments by file_id alone.
|
|
*/
|
|
class VisibleCommentScope
|
|
{
|
|
public function __construct(
|
|
private readonly StaffLibraryScope $scope,
|
|
private readonly ViewableFileScope $viewable,
|
|
private readonly ShareTargets $shareTargets,
|
|
private readonly GuestCommentIdentity $guests,
|
|
) {}
|
|
|
|
/**
|
|
* @param User|null $viewer Null means an anonymous visitor.
|
|
* @return Builder<FileComment>
|
|
*/
|
|
public function for(?User $viewer, File $file): Builder
|
|
{
|
|
return $this->applyVisibility(
|
|
FileComment::query()->where('file_id', $file->id),
|
|
$viewer,
|
|
$file->isEffectivelyPublic(),
|
|
);
|
|
}
|
|
|
|
/**
|
|
* The thread as somebody reading the public listing sees it: what a
|
|
* visitor is shown, plus their own comments if they happen to be
|
|
* signed in.
|
|
*
|
|
* for() above is the authenticated reading, and it assumes what the
|
|
* top of this class demands — that the caller established the viewer
|
|
* may see the *file*. The public listing establishes only the other
|
|
* half of that, namely that the file is reachable without logging in,
|
|
* which is the whole of it for a visitor and not nearly enough for an
|
|
* account: for() hands any staff member the file's StaffOnly notes and
|
|
* any client the messages addressed to all clients on it.
|
|
*
|
|
* So a signed-in reader is answered as a visitor is, widened by their
|
|
* own writing — which is what "being logged in should not show you
|
|
* less than a stranger sees, and their own comments should be theirs
|
|
* to edit" asks for and all it asks for. A reader the file's own gate
|
|
* would admit is not narrowed at all; the caller sends them through
|
|
* for() instead.
|
|
*
|
|
* The held-comment rule is not restated here — hideUnapproved() is the
|
|
* same one for()'s readings get, so a comment waiting for a moderator
|
|
* stays exactly as visible, or invisible, as it was.
|
|
*
|
|
* @return Builder<FileComment>
|
|
*/
|
|
public function forPublicReader(?User $viewer, File $file): Builder
|
|
{
|
|
$query = FileComment::query()->where('file_id', $file->id);
|
|
|
|
if ($viewer === null) {
|
|
return $this->applyVisibility($query, null, $file->isEffectivelyPublic());
|
|
}
|
|
|
|
$this->hideUnapproved($query, $viewer);
|
|
|
|
return $query->where(fn (Builder $outer) => $outer
|
|
->where('author_id', $viewer->id)
|
|
->when(
|
|
$file->isEffectivelyPublic(),
|
|
fn (Builder $stranger) => $stranger->orWhere('visibility', CommentVisibility::Everyone),
|
|
));
|
|
}
|
|
|
|
/**
|
|
* Every comment this staff member may read, across their whole library
|
|
* — the management screen's query, rather than one file's thread.
|
|
*
|
|
* Same predicate as for(), so the screen that lists everything is still
|
|
* governed by the rule at the top of this class: another person's "only
|
|
* me" note is not in it, and neither is a conversation belonging to a
|
|
* client this viewer is not assigned to. **A moderation screen is not a
|
|
* way around the visibility model** — moderating means deciding about
|
|
* comments you can already see.
|
|
*
|
|
* Which is why the files come from ViewableFileScope rather than from
|
|
* StaffLibraryScope: FilePolicy::view() is a permission half AND a
|
|
* library half, and narrowing by the library alone would hand every
|
|
* comment in the installation to a role holding moderate_comments and
|
|
* none of the three file keys — somebody who gets a 403 on every file
|
|
* these comments are about.
|
|
*
|
|
* Staff only. A client has no cross-file view of comments and asking
|
|
* for one is a mistake rather than an empty result, but returning
|
|
* nothing is the safe way to be wrong.
|
|
*
|
|
* @return Builder<FileComment>
|
|
*/
|
|
public function across(User $viewer): Builder
|
|
{
|
|
if (! $viewer->isStaff()) {
|
|
return FileComment::query()->whereRaw('1 = 0');
|
|
}
|
|
|
|
return $this->applyVisibility(
|
|
FileComment::query()->whereIn('file_id', $this->viewable->for($viewer)->select('files.id')),
|
|
$viewer,
|
|
// Publicness is a property of each file, so it cannot be one
|
|
// value for a query spanning many. It does not have to be: the
|
|
// staff branch of applyVisibility never reads this argument
|
|
// (staff keep seeing Everyone comments whatever the file's
|
|
// current state), and this method is staff-only.
|
|
isPublic: false,
|
|
);
|
|
}
|
|
|
|
/**
|
|
* How many comments are waiting for a decision anywhere in this
|
|
* viewer's library — the sidebar badge.
|
|
*
|
|
* Deliberately not derived from across(): a held comment is invisible
|
|
* to everyone but a moderator, so the number is about the queue rather
|
|
* than about what this viewer may read, and a moderator who cannot see
|
|
* a particular client's thread must still be told the file has
|
|
* something waiting.
|
|
*
|
|
* The file boundary is still the same one, though. ViewableFileScope
|
|
* rather than StaffLibraryScope: which files is the part that varies
|
|
* per client, whether any is the part that does not, and a badge
|
|
* counting the whole installation for somebody who may open none of it
|
|
* is a number about other people's files.
|
|
*/
|
|
public function pendingTotal(User $viewer): int
|
|
{
|
|
if (! $viewer->isStaff() || ! $viewer->can('moderate_comments')) {
|
|
return 0;
|
|
}
|
|
|
|
return FileComment::query()
|
|
->whereNull('approved_at')
|
|
->whereIn('file_id', $this->viewable->for($viewer)->select('files.id'))
|
|
->count();
|
|
}
|
|
|
|
/**
|
|
* How many comments each of these files has, from this viewer's point
|
|
* of view — the number on a file row.
|
|
*
|
|
* Runs the same predicate as for(), because it calls the same private
|
|
* method: this is the bulk shape of one rule, not a second statement
|
|
* of it. The only thing it cannot batch is whether a file is public,
|
|
* which is a property of each row rather than of the query, so the
|
|
* files are split into two groups and the predicate applied to each.
|
|
*
|
|
* @param iterable<File> $files
|
|
* @return array<int, int> file id => count
|
|
*/
|
|
public function countsFor(?User $viewer, iterable $files): array
|
|
{
|
|
$public = [];
|
|
$private = [];
|
|
|
|
foreach ($files as $file) {
|
|
if ($file->isEffectivelyPublic()) {
|
|
$public[] = $file->id;
|
|
} else {
|
|
$private[] = $file->id;
|
|
}
|
|
}
|
|
|
|
$counts = [];
|
|
|
|
foreach ([[$public, true], [$private, false]] as [$ids, $isPublic]) {
|
|
if ($ids === []) {
|
|
continue;
|
|
}
|
|
|
|
$rows = $this->applyVisibility(FileComment::query()->whereIn('file_id', $ids), $viewer, $isPublic)
|
|
->selectRaw('file_id, count(*) as aggregate')
|
|
->groupBy('file_id')
|
|
->pluck('aggregate', 'file_id');
|
|
|
|
foreach ($rows as $fileId => $count) {
|
|
$counts[(int) $fileId] = (int) $count;
|
|
}
|
|
}
|
|
|
|
return $counts;
|
|
}
|
|
|
|
/**
|
|
* A comment awaiting moderation exists only for those who can act on
|
|
* it — and for whoever wrote it, who would otherwise watch their own
|
|
* comment vanish on posting and conclude it had failed. A visitor is
|
|
* recognised by their session (see GuestCommentIdentity); that is weak
|
|
* on purpose, and only ever widens what somebody sees of their own
|
|
* writing.
|
|
*
|
|
* Its own method because forPublicReader() answers a different
|
|
* audience question and the same held-comment one, and a rule this
|
|
* sharp stated twice is a rule that drifts.
|
|
*
|
|
* @param Builder<FileComment> $query
|
|
*/
|
|
private function hideUnapproved(Builder $query, ?User $viewer): void
|
|
{
|
|
if ($viewer !== null && $viewer->can('moderate_comments')) {
|
|
return;
|
|
}
|
|
|
|
$ownPending = $viewer === null ? $this->guests->ownCommentIds() : [];
|
|
|
|
$query->where(fn (Builder $visible) => $visible
|
|
->whereNotNull('approved_at')
|
|
->when($ownPending !== [], fn (Builder $mine) => $mine->orWhereIn('id', $ownPending)));
|
|
}
|
|
|
|
/**
|
|
* @param Builder<FileComment> $query
|
|
* @return Builder<FileComment>
|
|
*/
|
|
private function applyVisibility(Builder $query, ?User $viewer, bool $isPublic): Builder
|
|
{
|
|
$this->hideUnapproved($query, $viewer);
|
|
|
|
if ($viewer === null) {
|
|
// Publicness is re-derived here on every read rather than
|
|
// frozen onto the comment at write time, so making a file
|
|
// private later retracts its public comments too.
|
|
return $isPublic
|
|
? $query->where('visibility', CommentVisibility::Everyone)
|
|
: $query->whereRaw('1 = 0');
|
|
}
|
|
|
|
return $query->where(function (Builder $outer) use ($viewer, $isPublic): void {
|
|
// Your own comments, whatever audience you gave them. This is
|
|
// the only branch that can return a CommentVisibility::OnlyMe
|
|
// row, which is what makes "only me" mean it.
|
|
$outer->where('author_id', $viewer->id);
|
|
|
|
if ($viewer->isStaff()) {
|
|
// Staff keep seeing Everyone comments after a file stops
|
|
// being public — they can see the file, and a history with
|
|
// holes in it is worse than one that is merely stale.
|
|
$outer->orWhere('visibility', CommentVisibility::Everyone);
|
|
$outer->orWhere('visibility', CommentVisibility::StaffOnly);
|
|
$this->applyStaffThreads($outer, $viewer);
|
|
|
|
return;
|
|
}
|
|
|
|
if ($isPublic) {
|
|
$outer->orWhere('visibility', CommentVisibility::Everyone);
|
|
}
|
|
|
|
// A client reads two things: what staff addressed to every
|
|
// client on this file, and their own conversation. Never a
|
|
// staff-only note, and never another client's conversation —
|
|
// they are not told the others are there.
|
|
$outer->orWhere(fn (Builder $broadcast) => $broadcast
|
|
->where('visibility', CommentVisibility::Clients)
|
|
->whereNull('client_context_id'));
|
|
|
|
$outer->orWhere(fn (Builder $thread) => $thread
|
|
->where('visibility', CommentVisibility::Clients)
|
|
->where('client_context_id', $viewer->id));
|
|
});
|
|
}
|
|
|
|
/**
|
|
* How many comments on each file are waiting for a decision.
|
|
*
|
|
* Distinct from unreadCountsFor: unread is "you have not looked at
|
|
* this yet", pending is "nobody can see this until somebody acts".
|
|
* Only a moderator gets a count at all, which is why the empty array
|
|
* for everyone else is the whole answer rather than a filter applied
|
|
* afterwards — a staff member who cannot approve should not be shown
|
|
* a badge asking them to.
|
|
*
|
|
* @param list<int> $fileIds
|
|
* @return array<int, int> file id => count
|
|
*/
|
|
public function pendingCountsFor(User $viewer, array $fileIds): array
|
|
{
|
|
if ($fileIds === [] || ! $viewer->can('moderate_comments')) {
|
|
return [];
|
|
}
|
|
|
|
$rows = FileComment::query()
|
|
->whereIn('file_id', $fileIds)
|
|
->whereNull('approved_at')
|
|
->selectRaw('file_id, count(*) as aggregate')
|
|
->groupBy('file_id')
|
|
->pluck('aggregate', 'file_id');
|
|
|
|
$counts = [];
|
|
|
|
foreach ($rows as $fileId => $count) {
|
|
$counts[(int) $fileId] = (int) $count;
|
|
}
|
|
|
|
return $counts;
|
|
}
|
|
|
|
/**
|
|
* How many comments on each file are new to this viewer.
|
|
*
|
|
* Read state rides on the notification system's own `read_at` rather
|
|
* than a file_comment_reads table of its own: a viewer has already
|
|
* been told about every comment they may see (that is what
|
|
* recipientsFor guarantees), so "unread notification about this file"
|
|
* and "unread comment on this file" are the same set. A second
|
|
* mechanism would only be a second thing to keep in step.
|
|
*
|
|
* @param list<int> $fileIds
|
|
* @return array<int, int> file id => count
|
|
*/
|
|
public function unreadCountsFor(User $viewer, array $fileIds): array
|
|
{
|
|
if ($fileIds === []) {
|
|
return [];
|
|
}
|
|
|
|
$rows = InAppNotification::query()
|
|
->where('user_id', $viewer->id)
|
|
->where('type', 'file_comment.posted')
|
|
->where('subject_type', (new File)->getMorphClass())
|
|
->whereIn('subject_id', $fileIds)
|
|
->whereNull('read_at')
|
|
->selectRaw('subject_id, count(*) as aggregate')
|
|
->groupBy('subject_id')
|
|
->pluck('aggregate', 'subject_id');
|
|
|
|
$counts = [];
|
|
|
|
foreach ($rows as $fileId => $count) {
|
|
$counts[(int) $fileId] = (int) $count;
|
|
}
|
|
|
|
return $counts;
|
|
}
|
|
|
|
/**
|
|
* Who should be told about $comment, excluding its own author.
|
|
*
|
|
* This is the set-of-people mirror of for()'s set-of-rows, and exists
|
|
* so Notifier's security contract can be honoured without the caller
|
|
* re-deriving visibility: a notification must never reach somebody the
|
|
* comment itself would not.
|
|
*
|
|
* Deliberately narrower than "everyone who could read it" in one case:
|
|
* an Everyone comment notifies staff only. Every client on a public
|
|
* file receiving a notification for every public remark would be noise,
|
|
* and staff are the ones who would act on it.
|
|
*
|
|
* @return Collection<int, User>
|
|
*/
|
|
public function recipientsFor(FileComment $comment): Collection
|
|
{
|
|
$file = $comment->file;
|
|
|
|
/** @var Collection<int, User> $none */
|
|
$none = new Collection;
|
|
|
|
if ($comment->visibility === CommentVisibility::OnlyMe) {
|
|
return $none;
|
|
}
|
|
|
|
$recipients = $this->staffWhoCanSee($file);
|
|
|
|
$client = $comment->clientContext;
|
|
|
|
if ($comment->visibility === CommentVisibility::Clients) {
|
|
if ($client !== null) {
|
|
// Staff whose library scope excludes this conversation's
|
|
// client must not hear about it — the same boundary for()
|
|
// applies to rows.
|
|
$recipients = $recipients
|
|
->filter(fn (User $staff): bool => $this->scope->canAssignClient($staff, $client))
|
|
->values();
|
|
|
|
$recipients->push($client);
|
|
} else {
|
|
// Addressed to every client on the file, so every client on
|
|
// the file is told. This is the one case that fans out.
|
|
foreach ($this->clientsWhoCanSee($comment->file) as $recipient) {
|
|
$recipients->push($recipient);
|
|
}
|
|
}
|
|
}
|
|
|
|
return $recipients
|
|
->reject(fn (User $user): bool => $user->id === $comment->author_id)
|
|
->values();
|
|
}
|
|
|
|
/**
|
|
* Staff who hold moderation rights — the audience for a comment held
|
|
* for approval, which is not the same audience as the comment itself
|
|
* (it has none yet).
|
|
*
|
|
* @return Collection<int, User>
|
|
*/
|
|
public function moderators(File $file): Collection
|
|
{
|
|
return $this->staffWhoCanSee($file)
|
|
->filter(fn (User $staff): bool => $staff->can('moderate_comments'))
|
|
->values();
|
|
}
|
|
|
|
/**
|
|
* Clients this file is in front of: assigned to it, to a group it is
|
|
* shared with, or to the folder it sits in or any folder above that.
|
|
*
|
|
* Expressed through ShareTargets rather than as an inverted
|
|
* File::scopeVisibleToClient — that scope is the authority on "can this
|
|
* client see this file", and writing its mirror image here would be a
|
|
* third statement of a rule this feature depends on not drifting. The
|
|
* cost is that a client who reaches the file only by having uploaded it
|
|
* themselves is not notified of a message to all clients; they still
|
|
* read it when they open the file.
|
|
*
|
|
* @return Collection<int, User>
|
|
*/
|
|
private function clientsWhoCanSee(File $file): Collection
|
|
{
|
|
$shares = $this->shareTargets->assigned($file);
|
|
|
|
$groupIds = array_column($shares['groups'], 'id');
|
|
$clientIds = array_column($shares['clients'], 'id');
|
|
|
|
$folder = $file->folder;
|
|
|
|
if ($folder !== null) {
|
|
foreach (Folder::query()->whereIn('id', [$folder->id, ...$folder->ancestorIds()])->get() as $ancestor) {
|
|
$inherited = $this->shareTargets->assigned($ancestor);
|
|
$groupIds = [...$groupIds, ...array_column($inherited['groups'], 'id')];
|
|
$clientIds = [...$clientIds, ...array_column($inherited['clients'], 'id')];
|
|
}
|
|
}
|
|
|
|
/** @var Collection<int, User> $clients */
|
|
$clients = User::query()
|
|
->where('type', UserType::Client)
|
|
->where(fn (Builder $who) => $who
|
|
->whereIn('id', $clientIds)
|
|
->orWhereHas('memberOfGroups', fn (Builder $groups) => $groups->whereIn('groups.id', $groupIds)))
|
|
->get();
|
|
|
|
return $clients;
|
|
}
|
|
|
|
/**
|
|
* Resolved per user through the file's own policy rather than as one
|
|
* query: FilePolicy::view mixes a permission check (a property of the
|
|
* viewer) with a scope check (a property of the row), and staff counts
|
|
* are small by design in this product — the v2 model puts limited
|
|
* staff on client scoping, not on large teams.
|
|
*
|
|
* @return Collection<int, User>
|
|
*/
|
|
private function staffWhoCanSee(File $file): Collection
|
|
{
|
|
/** @var Collection<int, User> $staff */
|
|
$staff = User::query()->where('type', UserType::Staff)->get();
|
|
|
|
return $staff->filter(fn (User $user): bool => Gate::forUser($user)->allows('view', $file))->values();
|
|
}
|
|
|
|
/**
|
|
* @param Builder<FileComment> $outer
|
|
*/
|
|
private function applyStaffThreads(Builder $outer, User $viewer): void
|
|
{
|
|
// Null means unrestricted: this staff member sees every client's
|
|
// conversation on a file they can already open.
|
|
$clientIds = $this->scope->assignableClientIds($viewer);
|
|
|
|
$outer->orWhere(function (Builder $thread) use ($clientIds): void {
|
|
$thread->where('visibility', CommentVisibility::Clients);
|
|
|
|
if ($clientIds === null) {
|
|
return;
|
|
}
|
|
|
|
// A client-scoped staff member sees their own clients'
|
|
// conversations, plus the messages addressed to every client on
|
|
// the file (no conversation of their own, so no client to be
|
|
// out of scope for), and nothing of the clients they are not
|
|
// assigned to.
|
|
$thread->where(fn (Builder $context) => $context
|
|
->whereNull('client_context_id')
|
|
->orWhereIn('client_context_id', $clientIds));
|
|
});
|
|
}
|
|
}
|