diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f668afd..2dd8f69b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,19 @@ installation from starting. **Fixed** +- **Deleting a client can no longer hand their files to a client you do not manage.** When you + delete an account that owns files, ProjectSend asks who should inherit them, and the list it + offers a staff member whose access is limited to certain clients shows only those clients. The + list was the only thing enforcing that. A request naming any other active account was accepted, so + one client's files and folders could end up owned by a client on somebody else's list — who could + then read, change and delete them, because people own what they upload. The list and the rule + behind it are now the same thing. + + *Who this affected:* installations using staff roles that are limited to certain clients, where + such a role can also delete clients. Administrators whose access is not limited are unaffected and + can still reassign to anybody. Nothing to do on upgrade. + + Reported by [@skeletonsec](https://github.com/skeletonsec). - **Moving a file into a public folder now needs the same permission as uploading one there.** A file in a public folder is public — that is what the folder means, and it applies to anything inside it, at any depth. Uploading into one was already refused to staff who are not allowed to diff --git a/app/Modules/Identity/AccountContentDeletion.php b/app/Modules/Identity/AccountContentDeletion.php index 7ca149be..62a0ff26 100644 --- a/app/Modules/Identity/AccountContentDeletion.php +++ b/app/Modules/Identity/AccountContentDeletion.php @@ -10,6 +10,7 @@ use App\Modules\Audit\ActivityLogger; use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\DeletedAccountContent; use App\Modules\Identity\Models\Role; +use Closure; use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\Request; use Illuminate\Validation\Rule; @@ -61,12 +62,8 @@ class AccountContentDeletion */ public function candidates(?User $viewer, ?int $excludeId = null): array { - return User::query() + return $this->reachableTargets($viewer) ->when($excludeId, fn (Builder $query, int $id) => $query->whereKeyNot($id)) - ->when($viewer, fn (Builder $query, User $for) => $query->where(fn (Builder $reachable) => $reachable - ->where('type', UserType::Staff) - ->orWhereIn('id', $this->scope->clients($for)->select('users.id')))) - ->where('active', true) ->with('role') ->orderBy('name') ->get() @@ -99,17 +96,62 @@ class AccountContentDeletion return []; } + $viewer = $request->user(); + return $request->validate([ 'content_action' => ['required', Rule::in(['cascade_delete', 'reassign'])], 'reassign_to_id' => [ 'required_if:content_action,reassign', 'integer', - Rule::exists('users', 'id')->where('active', true), Rule::notIn([$target->id]), + // The same question the picker asks, asked again of what + // came back from it. It used to be "exists, and is active", + // which is not the boundary the picker documents two + // methods up: a client-scoped staff member was shown their + // own roster and could name anybody, so deleting a roster + // client could hand that client's files and folders to a + // client on somebody else's roster — who then reads, edits + // and deletes them under the own-upload rules + // (GHSA-w29w-pj29-x7ww). + // + // One predicate for both, rather than a matching pair: a + // picker that promises a boundary the write does not keep + // is exactly what this was. + function (string $attribute, mixed $value, Closure $fail) use ($viewer): void { + if (! $this->reachableTargets($viewer)->whereKey($value)->exists()) { + // Deliberately the message an id that does not + // exist at all would get. "Not yours" and "not + // there" have to read the same, or refusing is how + // a scoped staff member enumerates the accounts + // outside their roster. + $fail('validation.exists')->translate(); + } + }, ], ]); } + /** + * Every active account $viewer may hand content to: staff, who are + * narrowed nowhere in the application, plus the clients + * StaffLibraryScope shows them. An unscoped viewer gets everybody, + * because clients() returns everybody for them. + * + * $viewer is null only where the question is about the installation + * rather than about a screen — the erasure default in privacy + * settings, which is stored once for everybody. + * + * @return Builder + */ + private function reachableTargets(?User $viewer): Builder + { + return User::query() + ->when($viewer, fn (Builder $query, User $for) => $query->where(fn (Builder $reachable) => $reachable + ->where('type', UserType::Staff) + ->orWhereIn('id', $this->scope->clients($for)->select('users.id')))) + ->where('active', true); + } + /** * @param array{content_action?: string, reassign_to_id?: int} $validated */ diff --git a/tests/Feature/Identity/ReassignmentRosterBoundaryTest.php b/tests/Feature/Identity/ReassignmentRosterBoundaryTest.php new file mode 100644 index 00000000..775cef7e --- /dev/null +++ b/tests/Feature/Identity/ReassignmentRosterBoundaryTest.php @@ -0,0 +1,156 @@ +admin = User::factory()->create(); + + $role = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]); + foreach ([Permission::DeleteClients, Permission::EditClients, Permission::CreateClients] as $permission) { + RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]); + } + + $this->rep = User::factory()->create(['role_id' => $role->id]); + + // On this rep's roster, and owning something worth inheriting. + $this->mine = User::factory()->client()->create(['name' => 'Mine']); + $this->rep->assignedClients()->sync([$this->mine->id]); + + $this->file = File::factory()->create(['uploaded_by' => $this->mine->id]); + $this->folder = Folder::query()->create([ + 'name' => 'Theirs', 'slug' => 'theirs-'.Str::random(6), 'path' => '/', 'created_by' => $this->mine->id, + ]); + + // On nobody's roster as far as this rep is concerned. + $this->stranger = User::factory()->client()->create(['name' => 'Not Mine']); +}); + +test('the picker does not offer a client outside the roster', function () { + // The promise the write has to keep. Asserted first so that a change + // loosening the picker cannot quietly make the rest of this file vacuous. + $names = collect(app(AccountContentDeletion::class)->candidates($this->rep)) + ->pluck('name'); + + expect($names)->toContain('Mine') + ->and($names)->not->toContain('Not Mine'); +}); + +test('a scoped staff member cannot hand content to a client off their roster', function () { + $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $this->stranger->id, + ])->assertSessionHasErrors('reassign_to_id'); + + expect($this->file->fresh()->uploaded_by)->toBe($this->mine->id) + ->and($this->folder->fresh()->created_by)->toBe($this->mine->id) + ->and(User::withTrashed()->find($this->mine->id)->trashed())->toBeFalse(); +}); + +test('the API twin refuses it too', function () { + $token = $this->rep->createToken('t', [Permission::DeleteClients->value])->plainTextToken; + + $this->withToken($token) + ->deleteJson("/api/v1/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $this->stranger->id, + ])->assertStatus(422); + + expect($this->file->fresh()->uploaded_by)->toBe($this->mine->id); +}); + +test('refusing reads the same as an account that is not there at all', function () { + // Otherwise the refusal is an oracle: a scoped staff member could walk + // the id space and learn which accounts exist outside their roster by + // the difference between the two answers. + $outsider = $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $this->stranger->id, + ]); + + $nobody = $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => 99999999, + ]); + + expect($outsider->getSession()->get('errors')->get('reassign_to_id')) + ->toBe($nobody->getSession()->get('errors')->get('reassign_to_id')); +}); + +test('a client on the roster is still a valid target', function () { + $alsoMine = User::factory()->client()->create(['name' => 'Also Mine']); + $this->rep->assignedClients()->sync([$this->mine->id, $alsoMine->id]); + + $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $alsoMine->id, + ])->assertRedirect(); + + expect($this->file->fresh()->uploaded_by)->toBe($alsoMine->id) + ->and($this->folder->fresh()->created_by)->toBe($alsoMine->id); +}); + +test('a staff account is still a valid target, because staff are narrowed nowhere', function () { + $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $this->admin->id, + ])->assertRedirect(); + + expect($this->file->fresh()->uploaded_by)->toBe($this->admin->id); +}); + +test('an unscoped administrator can still reassign to anybody', function () { + // The tightening is about the roster, and an unscoped staff member has + // no roster — StaffLibraryScope::clients() is every client for them. + $this->actingAs($this->admin)->delete("/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $this->stranger->id, + ])->assertRedirect(); + + expect($this->file->fresh()->uploaded_by)->toBe($this->stranger->id); +}); + +test('the advisory controls still behave as they did', function () { + // Both were already enforced, and both must survive the new rule: + // an inactive account, and the account being deleted. + $inactive = User::factory()->client()->create(['active' => false]); + $this->rep->assignedClients()->sync([$this->mine->id, $inactive->id]); + + $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $inactive->id, + ])->assertSessionHasErrors('reassign_to_id'); + + $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $this->mine->id, + ])->assertSessionHasErrors('reassign_to_id'); + + expect($this->file->fresh()->uploaded_by)->toBe($this->mine->id); +});