mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-21 02:53:24 +00:00
Merge pull request #1721 from denkfabrik-li/fix/api-dashboard-activity-log-scope
ApiUsage::recentActions() read the activity log without ActivityLogScope::apply(). It was the only ActivityLog::query() outside ActivityLogger and AccountEraser that skipped it. Its only boundary was view_actions_log -- the permission ActivityLogScope's own docblock says "is not the whole answer for a client-scoped staff member", because a log row carries the subject's name. The Client Manager system role is client_scoped and ships with that permission, so this was the default configuration and not an exotic one: the same person who gets a 403 on a file and an empty /activity read that file's name off /api?all=1. ApiUsage now takes ActivityLogScope and applies it to the recent-actions query, on both sides of the install-wide branch rather than only in the install-wide arm -- the own-actor filter already stays inside what the scope allows, and a boundary that exists in only one arm of an if is one refactor away from not existing. The token inventory, request counts and endpoint table keep ApiUsageScope alone: those rows are about the viewer's own credentials rather than library content. Verified before merging: 17 passed on the trial-merge and 1 failed / 16 passed with app/ reset. The two tests guarding against narrowing further than /activity does -- a viewer's own actions stay whole, an unscoped viewer's feed is unchanged -- are green either way. ActivityLogScope::apply() wraps its conditions in a single where(Closure), so it composes with the origin and actor_id filters around it without a precedence trap, and ApiUsage is never constructed with new, so the added dependency is wired by the container everywhere. Reported and fixed by @denkfabrik-li.
This commit is contained in:
@@ -8,6 +8,7 @@ use App\Models\User;
|
||||
use App\Modules\Api\Auth\ApiTokens;
|
||||
use App\Modules\Api\Models\ApiRequestLog;
|
||||
use App\Modules\Audit\ActivityLog;
|
||||
use App\Modules\Audit\ActivityLogScope;
|
||||
use App\Modules\Audit\ActivityOrigin;
|
||||
use Illuminate\Database\Eloquent\Builder;
|
||||
use Illuminate\Support\Carbon;
|
||||
@@ -27,6 +28,7 @@ class ApiUsage
|
||||
{
|
||||
public function __construct(
|
||||
private readonly ApiUsageScope $scope,
|
||||
private readonly ActivityLogScope $activityLog,
|
||||
) {}
|
||||
|
||||
/**
|
||||
@@ -145,7 +147,23 @@ class ApiUsage
|
||||
*/
|
||||
public function recentActions(User $viewer, bool $installWide, int $limit = 15): array
|
||||
{
|
||||
$query = ActivityLog::query()->where('origin', ActivityOrigin::Api);
|
||||
// Narrowed through ActivityLogScope, exactly as the activity page,
|
||||
// the download history and the dashboard widget are.
|
||||
// `view_actions_log` decides whether the install-wide view opens at
|
||||
// all, but it is not the whole answer for a client-scoped viewer: a
|
||||
// row carries the subject's name, so an unscoped feed reads out file
|
||||
// and client names to somebody who gets a 403 on the files
|
||||
// themselves. The Client Manager role ships with the permission, so
|
||||
// this is the default configuration, not an exotic one.
|
||||
//
|
||||
// Applied on both sides of the branch rather than only in the
|
||||
// install-wide one: the own-actor filter below already stays inside
|
||||
// what the scope allows, and a boundary that only exists in one arm
|
||||
// of an `if` is one refactor away from not existing.
|
||||
$query = $this->activityLog->apply(
|
||||
ActivityLog::query()->where('origin', ActivityOrigin::Api),
|
||||
$viewer,
|
||||
);
|
||||
|
||||
if (! $installWide) {
|
||||
$query->where('actor_id', $viewer->id);
|
||||
|
||||
@@ -4,7 +4,12 @@ declare(strict_types=1);
|
||||
|
||||
use App\Models\User;
|
||||
use App\Modules\Api\Models\ApiRequestLog;
|
||||
use App\Modules\Audit\Action;
|
||||
use App\Modules\Audit\ActivityLog;
|
||||
use App\Modules\Audit\ActivityOrigin;
|
||||
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 App\Modules\Platform\Settings\Setting;
|
||||
use App\Modules\Platform\Settings\Settings;
|
||||
@@ -195,6 +200,86 @@ test('request counts follow the same scope', function () {
|
||||
expect($props['summary']['requests_7d'])->toBe(1);
|
||||
});
|
||||
|
||||
/** An API-origin log entry about $file, shaped as ActivityLogger writes it. */
|
||||
function apiActionOn(User $actor, File $file): void
|
||||
{
|
||||
ActivityLog::query()->create([
|
||||
'actor_id' => $actor->id,
|
||||
'actor_name' => $actor->name,
|
||||
'actor_type' => $actor->type->value,
|
||||
'origin' => ActivityOrigin::Api,
|
||||
'api_token_name' => 'Zapier',
|
||||
'action' => Action::FileUploaded,
|
||||
'subject_type' => $file->getMorphClass(),
|
||||
'subject_id' => $file->id,
|
||||
'subject_name' => $file->name,
|
||||
'created_at' => now(),
|
||||
]);
|
||||
}
|
||||
|
||||
/** A client-scoped viewer holding view_actions_log, as Client Manager ships. */
|
||||
function scopedApiLogReader(): array
|
||||
{
|
||||
$role = Role::query()->create(['name' => 'Scoped log reader', 'client_scoped' => true]);
|
||||
RolePermission::query()->insert([
|
||||
['role_id' => $role->id, 'permission' => Permission::ViewActionsLog->value],
|
||||
['role_id' => $role->id, 'permission' => Permission::Upload->value],
|
||||
]);
|
||||
|
||||
$viewer = User::factory()->create(['role_id' => $role->id]);
|
||||
$client = User::factory()->client()->create();
|
||||
$viewer->assignedClients()->attach($client->id);
|
||||
|
||||
return [$viewer, $client];
|
||||
}
|
||||
|
||||
test('the recent actions feed narrows to what the viewer may read', function () {
|
||||
// Tokens are scoped by view_actions_log alone; the activity log is not.
|
||||
// A row carries the subject's name, so an unscoped feed reads out the
|
||||
// name of every file in the installation to a Client Manager who gets a
|
||||
// 403 on the files themselves — the same reasoning /activity, the
|
||||
// download history and the dashboard widget already act on.
|
||||
[$scoped, $client] = scopedApiLogReader();
|
||||
|
||||
$theirs = File::factory()->create(['uploaded_by' => $this->staff->id, 'name' => 'Q3 delinquent accounts']);
|
||||
|
||||
$mine = File::factory()->create(['uploaded_by' => $this->staff->id, 'name' => 'Statement']);
|
||||
shareFileWith($mine, $client);
|
||||
|
||||
apiActionOn($this->staff, $theirs);
|
||||
apiActionOn($this->staff, $mine);
|
||||
|
||||
$props = $this->actingAs($scoped)->get('/api?all=1')->assertOk()->viewData('page')['props'];
|
||||
|
||||
expect($props['scope']['install_wide'])->toBeTrue()
|
||||
->and(collect($props['recent_actions'])->pluck('replacements.subject')->all())->toBe(['Statement']);
|
||||
});
|
||||
|
||||
test('a scoped viewer keeps their own actions, even about a file they cannot open', function () {
|
||||
// Guards against narrowing further than /activity does: their own audit
|
||||
// trail stays whole, which is the rule ActivityLogScope states.
|
||||
[$scoped] = scopedApiLogReader();
|
||||
|
||||
$stranger = File::factory()->create(['uploaded_by' => $this->staff->id, 'name' => 'Not in my library']);
|
||||
apiActionOn($scoped, $stranger);
|
||||
|
||||
$props = $this->actingAs($scoped)->get('/api?all=1')->assertOk()->viewData('page')['props'];
|
||||
|
||||
expect(collect($props['recent_actions'])->pluck('replacements.subject')->all())
|
||||
->toBe(['Not in my library']);
|
||||
});
|
||||
|
||||
test('an unscoped viewer still sees every API action in the installation', function () {
|
||||
$auditor = staffWithPermissions([Permission::ViewActionsLog->value]);
|
||||
$file = File::factory()->create(['uploaded_by' => $this->staff->id, 'name' => 'Anything']);
|
||||
|
||||
apiActionOn($this->staff, $file);
|
||||
|
||||
$props = $this->actingAs($auditor)->get('/api?all=1')->assertOk()->viewData('page')['props'];
|
||||
|
||||
expect(collect($props['recent_actions'])->pluck('replacements.subject')->all())->toBe(['Anything']);
|
||||
});
|
||||
|
||||
test('clients cannot reach the dashboard', function () {
|
||||
$client = User::factory()->client()->create();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user