Files
projectsend/tests/Feature/Api/ClientsTest.php
ignacionelson 9c6f4df5bc Merge pull request #1736 from denkfabrik-li/fix/scoped-creator-keeps-client
A client-scoped staff member with create_clients created a client and lost it in the same request. guardTarget() answers 404 for anything off their roster, and StaffLibraryScope::clients() leaves it out of their list -- so the record existed, was logged, was welcomed by email, and was invisible to the person who made it. store() redirects to the edit page, which is exactly where they landed on a 404. The API twin had the same shape: a scoped token got a 404 from every route that binds the client it had just created.

The new client is now attached to the creator's roster when the creator is client-scoped, on both sides. That is where a client they created belongs -- the roster is the same list assignedClients already uses for everything else they may reach. Unscoped creators gain nothing: they see every client already, and a roster entry would change what assignedClients means for them. Nothing is attached retroactively.

The widening this involves is self-limited: the only thing added is an account the creator just made, which starts with no files, no folders and no group memberships, so assignableClientIds gains nothing to reach. Seats do not move either, since they are counted from active and account_requested.

Verified before merging: 34 passed across both suites on the trial-merge, 2 failed / 32 passed with app/ reset. This is the busiest file set of the series -- it shares ClientsController with #1718 and Api/ClientsController plus the API test file with #1723 -- so the merged result was read rather than trusted: #1718's reassign_candidates gating and #1723's patchCustomFieldValues are both intact alongside it. scramble:export reproduces the committed docs/api/openapi.json byte for byte.

Reported and fixed by @denkfabrik-li.
2026-08-28 17:20:44 -03:00

403 lines
16 KiB
PHP

<?php
declare(strict_types=1);
use App\Models\User;
use App\Modules\Audit\Action;
use App\Modules\Audit\ActivityLog;
use App\Modules\Clients\ClientCustomFieldType;
use App\Modules\Clients\Models\ClientCustomField;
use App\Modules\Clients\Models\ClientCustomFieldValue;
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;
beforeEach(function () {
Storage::fake('files');
$this->admin = User::factory()->create();
$this->token = $this->admin->createToken('t', [
Permission::ManageClients->value,
Permission::CreateClients->value,
Permission::EditClients->value,
Permission::DeleteClients->value,
])->plainTextToken;
});
/*
|--------------------------------------------------------------------------
| Privacy
|--------------------------------------------------------------------------
|
| `users` is the most sensitive table here. These assert on the raw
| response body rather than on parsed fields, so a leak through a nested
| relation or a future column is caught too.
|
*/
test('no client response carries credentials', function () {
$client = User::factory()->client()->create();
$bodies = [
$this->withToken($this->token)->getJson('/api/v1/clients')->getContent(),
$this->withToken($this->token)->getJson("/api/v1/clients/{$client->id}")->getContent(),
];
foreach ($bodies as $body) {
foreach (['password', 'two_factor_secret', 'two_factor_recovery_codes', 'remember_token'] as $forbidden) {
expect($body)->not->toContain($forbidden);
}
}
});
test('a staff account is not reachable through the clients surface', function () {
$staff = User::factory()->create();
$this->withToken($this->token)->getJson("/api/v1/clients/{$staff->id}")->assertNotFound();
$this->withToken($this->token)->patchJson("/api/v1/clients/{$staff->id}", ['name' => 'x'])->assertNotFound();
$this->withToken($this->token)->deleteJson("/api/v1/clients/{$staff->id}")->assertNotFound();
expect(User::query()->find($staff->id))->not->toBeNull();
});
test('the listing does not hand out every clients custom field data', function () {
$field = ClientCustomField::query()->create([
'name' => 'vat', 'label' => 'VAT number', 'type' => ClientCustomFieldType::Text,
'required' => false, 'sort_order' => 1,
]);
$client = User::factory()->client()->create();
ClientCustomFieldValue::query()->create([
'client_custom_field_id' => $field->id, 'user_id' => $client->id, 'value' => 'SECRET-VAT',
]);
expect($this->withToken($this->token)->getJson('/api/v1/clients')->getContent())
->not->toContain('SECRET-VAT');
// But reading one client does include them — the same data the edit
// screen shows to anyone with edit_clients.
$this->withToken($this->token)->getJson("/api/v1/clients/{$client->id}")
->assertOk()
->assertJsonPath('data.custom_fields.0.value', 'SECRET-VAT');
});
/*
|--------------------------------------------------------------------------
| CRUD
|--------------------------------------------------------------------------
*/
test('a PATCH leaves custom fields it does not name alone', function () {
// The same rule the rest of update() states: "an absent key means
// 'leave alone', not 'clear'". The custom-field pass was create()'s,
// which writes every field there is.
$name = ClientCustomField::query()->create([
'name' => 'contact', 'label' => 'Contact', 'type' => ClientCustomFieldType::Text,
'required' => false, 'sort_order' => 1,
]);
$vat = ClientCustomField::query()->create([
'name' => 'vat', 'label' => 'VAT number', 'type' => ClientCustomFieldType::Text,
'required' => false, 'sort_order' => 2,
]);
$client = User::factory()->client()->create();
ClientCustomFieldValue::query()->create([
'client_custom_field_id' => $name->id, 'user_id' => $client->id, 'value' => 'Alex',
]);
ClientCustomFieldValue::query()->create([
'client_custom_field_id' => $vat->id, 'user_id' => $client->id, 'value' => 'ATU12345678',
]);
$this->withToken($this->token)->patchJson("/api/v1/clients/{$client->id}", [
'custom_field_values' => [$name->id => 'Robin'],
])->assertOk();
$values = ClientCustomFieldValue::query()->where('user_id', $client->id)
->pluck('value', 'client_custom_field_id');
expect($values[$name->id])->toBe('Robin')
->and($values[$vat->id])->toBe('ATU12345678');
});
test('a PATCH can still clear a field by naming it', function () {
$vat = ClientCustomField::query()->create([
'name' => 'vat', 'label' => 'VAT number', 'type' => ClientCustomFieldType::Text,
'required' => false, 'sort_order' => 1,
]);
$client = User::factory()->client()->create();
ClientCustomFieldValue::query()->create([
'client_custom_field_id' => $vat->id, 'user_id' => $client->id, 'value' => 'ATU12345678',
]);
$this->withToken($this->token)->patchJson("/api/v1/clients/{$client->id}", [
'custom_field_values' => [$vat->id => ''],
])->assertOk();
expect(ClientCustomFieldValue::query()
->where('user_id', $client->id)->where('client_custom_field_id', $vat->id)->value('value'))->toBeNull();
});
test('creating a client still records every field', function () {
// create() is not a partial update, and a checkbox nobody ticked is a
// recorded "no" rather than an absent row.
$optIn = ClientCustomField::query()->create([
'name' => 'newsletter', 'label' => 'Newsletter', 'type' => ClientCustomFieldType::Checkbox,
'required' => false, 'sort_order' => 1,
]);
$this->withToken($this->token)->postJson('/api/v1/clients', [
'name' => 'Acme Ltd',
'email' => 'crud-custom@example.com',
'password' => 'super-secret-password',
])->assertCreated();
$client = User::query()->where('email', 'crud-custom@example.com')->sole();
expect(ClientCustomFieldValue::query()
->where('user_id', $client->id)->where('client_custom_field_id', $optIn->id)->value('value'))->toBe('0');
});
test('a client can be created', function () {
$this->withToken($this->token)->postJson('/api/v1/clients', [
'name' => 'Acme Ltd',
'email' => 'billing@acme.test',
'password' => 'a-sufficiently-long-password',
])->assertStatus(201)->assertJsonPath('data.email', 'billing@acme.test');
$client = User::query()->where('email', 'billing@acme.test')->firstOrFail();
expect($client->isClient())->toBeTrue()
->and($client->active)->toBeTrue()
->and(ActivityLog::query()->where('action', Action::UserCreated)->exists())->toBeTrue();
});
test('a client-scoped token keeps the client it creates', function () {
// The API twin of the same rule: without the roster entry, the token's
// owner gets a 404 from every route that binds the client they just
// created, including show and update.
$role = Role::query()->create(['name' => 'Scoped creator', 'client_scoped' => true]);
RolePermission::query()->insert([
['role_id' => $role->id, 'permission' => Permission::ManageClients->value],
['role_id' => $role->id, 'permission' => Permission::CreateClients->value],
['role_id' => $role->id, 'permission' => Permission::EditClients->value],
]);
$creator = User::factory()->create(['role_id' => $role->id]);
$token = $creator->createToken('t', [
Permission::ManageClients->value,
Permission::CreateClients->value,
Permission::EditClients->value,
])->plainTextToken;
$id = $this->withToken($token)->postJson('/api/v1/clients', [
'name' => 'Brand New',
'email' => 'api-brand-new@example.com',
'password' => 'super-secret-password',
])->assertCreated()->json('data.id');
$this->withToken($token)->getJson("/api/v1/clients/{$id}")->assertOk();
});
test('a required custom field is enforced on create', function () {
ClientCustomField::query()->create([
'name' => 'vat', 'label' => 'VAT number', 'type' => ClientCustomFieldType::Text,
'required' => true, 'sort_order' => 1,
]);
$this->withToken($this->token)->postJson('/api/v1/clients', [
'name' => 'No VAT',
'email' => 'novat@acme.test',
'password' => 'a-sufficiently-long-password',
])->assertStatus(422);
expect(User::query()->where('email', 'novat@acme.test')->exists())->toBeFalse();
});
test('a weak password is refused', function () {
$this->withToken($this->token)->postJson('/api/v1/clients', [
'name' => 'Weak',
'email' => 'weak@acme.test',
'password' => 'short',
])->assertStatus(422);
});
test('update changes only the fields sent', function () {
$client = User::factory()->client()->create(['name' => 'Before', 'storage_quota_mb' => 500]);
$this->withToken($this->token)->patchJson("/api/v1/clients/{$client->id}", ['name' => 'After'])
->assertOk()
->assertJsonPath('data.name', 'After');
$client->refresh();
expect($client->name)->toBe('After')
// Untouched by a PATCH that did not mention it — unlike the web
// form, which always submits every field.
->and($client->storage_quota_mb)->toBe(500);
});
test('deactivating a client is recorded', function () {
$client = User::factory()->client()->create(['active' => true]);
$this->withToken($this->token)->patchJson("/api/v1/clients/{$client->id}", ['active' => false])->assertOk();
expect($client->refresh()->active)->toBeFalse()
->and(ActivityLog::query()->where('action', Action::UserDeactivated)->exists())->toBeTrue();
});
/*
|--------------------------------------------------------------------------
| Deletion
|--------------------------------------------------------------------------
*/
test('a client with no content deletes without ceremony', function () {
$client = User::factory()->client()->create();
$this->withToken($this->token)->deleteJson("/api/v1/clients/{$client->id}")->assertNoContent();
expect(User::query()->find($client->id))->toBeNull();
});
test('deleting a client that owns files demands an explicit decision', function () {
$client = User::factory()->client()->create();
File::factory()->create(['uploaded_by' => $client->id]);
// No default is possible: one would silently destroy the files, the
// other would silently transfer them to somebody else.
$this->withToken($this->token)->deleteJson("/api/v1/clients/{$client->id}")
->assertStatus(422)
->assertJsonPath('type', 'validation_failed');
expect(User::query()->find($client->id))->not->toBeNull();
});
test('cascade_delete removes the clients content', function () {
$client = User::factory()->client()->create();
$file = File::factory()->create(['uploaded_by' => $client->id]);
$this->withToken($this->token)
->deleteJson("/api/v1/clients/{$client->id}", ['content_action' => 'cascade_delete'])
->assertNoContent();
expect(User::query()->find($client->id))->toBeNull()
->and(File::query()->find($file->id))->toBeNull();
});
test('reassign moves the content to the named account', function () {
$client = User::factory()->client()->create();
$file = File::factory()->create(['uploaded_by' => $client->id]);
$this->withToken($this->token)->deleteJson("/api/v1/clients/{$client->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $this->admin->id,
])->assertNoContent();
expect(File::query()->find($file->id)?->uploaded_by)->toBe($this->admin->id);
});
test('a failure while disposing of a deleted client\'s content rolls the deletion back', function () {
$client = User::factory()->client()->create();
failAccountContentDisposal();
$this->withToken($this->token)->deleteJson("/api/v1/clients/{$client->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $this->admin->id,
])->assertStatus(500);
// The soft-delete shares a transaction with the content step, so its
// failure leaves the client intact rather than deleted-but-orphaning.
expect(User::query()->whereKey($client->id)->exists())->toBeTrue()
->and(ActivityLog::query()->where('action', Action::UserDeleted)->exists())->toBeFalse();
});
test('show reports what a delete would have to decide about', function () {
$client = User::factory()->client()->create();
File::factory()->count(2)->create(['uploaded_by' => $client->id]);
$this->withToken($this->token)->getJson("/api/v1/clients/{$client->id}")
->assertOk()
->assertJsonPath('data.content.files', 2);
});
/*
|--------------------------------------------------------------------------
| Permissions
|--------------------------------------------------------------------------
*/
test('each route needs its own permission', function () {
$client = User::factory()->client()->create();
$readOnly = staffWithPermissions([Permission::ManageClients->value]);
$token = $readOnly->createToken('t', [Permission::ManageClients->value])->plainTextToken;
$this->withToken($token)->getJson('/api/v1/clients')->assertOk();
$this->withToken($token)->postJson('/api/v1/clients', [])->assertForbidden();
$this->withToken($token)->patchJson("/api/v1/clients/{$client->id}", [])->assertForbidden();
$this->withToken($token)->deleteJson("/api/v1/clients/{$client->id}")->assertForbidden();
});
test('storage quota reporting distinguishes inherited from unlimited', function () {
$client = User::factory()->client()->create(['storage_quota_mb' => 0]);
$this->withToken($this->token)->getJson("/api/v1/clients/{$client->id}")
->assertOk()
// 0 on the client means "inherit the site default", which is what
// effective_quota_mb resolves — a caller should not have to know
// that rule to display the number correctly.
->assertJsonPath('data.storage.quota_mb', 0)
->assertJsonStructure(['data' => ['storage' => ['effective_quota_mb', 'unlimited', 'used_mb']]]);
});
/*
|--------------------------------------------------------------------------
| Two-factor reset
|--------------------------------------------------------------------------
*/
test('a client\'s second factor can be removed', function () {
$client = User::factory()->client()->create();
enableTwoFactor($client);
forgetRequestState();
$this->withToken($this->token)
->deleteJson("/api/v1/clients/{$client->id}/two-factor")
->assertNoContent();
expect($client->refresh()->hasTwoFactorEnabled())->toBeFalse();
expect(ActivityLog::query()->where('action', Action::TwoFactorReset)->sole()->subject_id)
->toBe($client->id);
});
test('removing a second factor needs edit_clients, not just manage_clients', function () {
$token = $this->admin->createToken('narrow', [Permission::ManageClients->value])->plainTextToken;
$client = User::factory()->client()->create();
enableTwoFactor($client);
forgetRequestState();
$this->withToken($token)
->deleteJson("/api/v1/clients/{$client->id}/two-factor")
->assertForbidden();
expect($client->refresh()->hasTwoFactorEnabled())->toBeTrue();
});
test('a staff account is not addressable through the client two-factor route', function () {
$staff = User::factory()->create();
$this->withToken($this->token)
->deleteJson("/api/v1/clients/{$staff->id}/two-factor")
->assertNotFound();
});
test('the listing reports whether a client has a second factor', function () {
$client = User::factory()->client()->create();
enableTwoFactor($client);
forgetRequestState();
$this->withToken($this->token)
->getJson('/api/v1/clients')
->assertOk()
->assertJsonPath('data.0.two_factor_enabled', true);
});