diff --git a/app/Models/User.php b/app/Models/User.php index 7bef6606..4b779231 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -175,6 +175,13 @@ class User extends Authenticatable implements HasLocalePreference 'ldap_synced_at' => 'datetime', 'active' => 'boolean', 'account_requested' => 'boolean', + // The column is an unsignedInteger and the docblock above + // already promises int. Saying so here is what makes that true + // for a reader as well: it is passed straight into typed + // signatures (ClientAccounts::create, ClientProvisioning:: + // provision), and whether a driver hands back 2048 or "2048" + // is not something those call sites should depend on. + 'storage_quota_mb' => 'integer', 'erase_after' => 'datetime', 'email_verified_at' => 'datetime', 'password' => 'hashed', diff --git a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php index 42cda38f..bc9010f1 100644 --- a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php @@ -144,7 +144,12 @@ class ClientsController extends Controller name: $validated['name'], email: $validated['email'], password: $validated['password'], - storageQuotaMb: $validated['storage_quota_mb'] ?? 0, + // As on the staff screen, and for the same reason: the + // `integer` rule accepts a numeric string and does not convert + // it. A JSON number arrives as an int and was fine; a + // form-encoded body or a quoted JSON value is a string, and + // this file is strict_types. + storageQuotaMb: (int) ($validated['storage_quota_mb'] ?? 0), welcome: false, ); diff --git a/app/Modules/Clients/Http/Controllers/ClientsController.php b/app/Modules/Clients/Http/Controllers/ClientsController.php index 59b335b5..da1ef5f6 100644 --- a/app/Modules/Clients/Http/Controllers/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/ClientsController.php @@ -158,7 +158,13 @@ class ClientsController extends Controller name: $validated['name'], email: $validated['email'], password: $validated['password'], - storageQuotaMb: $validated['storage_quota_mb'] ?? 0, + // Cast, because `integer` validates without converting: + // $request->validate() hands back the raw input, so a form + // field arrives as the string "2048" and this file is + // strict_types. Filling the quota in was a 500; leaving it + // blank went through null ?? 0 as an int, which is why it + // survived to the fleet. + storageQuotaMb: (int) ($validated['storage_quota_mb'] ?? 0), welcome: false, ); diff --git a/app/Modules/Clients/Models/Invitation.php b/app/Modules/Clients/Models/Invitation.php index f1e81275..e7ea5aaa 100644 --- a/app/Modules/Clients/Models/Invitation.php +++ b/app/Modules/Clients/Models/Invitation.php @@ -62,6 +62,9 @@ class Invitation extends Model { return [ 'expires_at' => 'datetime', + // Read straight into provision()'s `int $storageQuotaMb` when + // the invitation is redeemed -- see the same cast on User. + 'storage_quota_mb' => 'integer', ]; } diff --git a/app/Modules/Comments/Http/Controllers/FileCommentsController.php b/app/Modules/Comments/Http/Controllers/FileCommentsController.php index 34f2a3bd..87867bc5 100644 --- a/app/Modules/Comments/Http/Controllers/FileCommentsController.php +++ b/app/Modules/Comments/Http/Controllers/FileCommentsController.php @@ -61,7 +61,9 @@ class FileCommentsController extends Controller $viewer, CommentVisibility::from($validated['visibility']), $validated['body'], - $this->replyTarget($viewer, $file, $validated['reply_to'] ?? null), + // Cast for the reason ShareLinksController gives: `integer` + // does not convert, and replyTarget() takes a strict ?int. + $this->replyTarget($viewer, $file, isset($validated['reply_to']) ? (int) $validated['reply_to'] : null), ); return response()->json($this->payload($viewer, $file), 201); diff --git a/app/Modules/Files/Http/Controllers/ShareLinksController.php b/app/Modules/Files/Http/Controllers/ShareLinksController.php index 9e4ab35a..3de23d45 100644 --- a/app/Modules/Files/Http/Controllers/ShareLinksController.php +++ b/app/Modules/Files/Http/Controllers/ShareLinksController.php @@ -89,7 +89,18 @@ class ShareLinksController extends Controller file: $file, creator: $user, expiresAt: $user->can('set_file_expiration_date') ? $expiresAt : null, - maxDownloads: $user->can('limit_downloads') ? $validated['max_downloads'] ?? null : null, + // Cast, and null kept as null rather than falling through a + // bare (int) that would turn "no cap" into a cap of zero. The + // `integer` rule validates a numeric string without converting + // it, and this file is strict_types, so an uncast "5" is a + // TypeError against `?int $maxDownloads`. Nothing sends one + // today only because files/edit.tsx calls Number() first -- + // which is a fact about a frontend file, not a guarantee this + // signature has. It cost a 500 on the client form, where the + // same field was typed as a string. + maxDownloads: $user->can('limit_downloads') && ($validated['max_downloads'] ?? null) !== null + ? (int) $validated['max_downloads'] + : null, token: $validated['token'] ?? null, ); diff --git a/tests/Feature/Api/ClientsTest.php b/tests/Feature/Api/ClientsTest.php index 36505b1a..7f730fb8 100644 --- a/tests/Feature/Api/ClientsTest.php +++ b/tests/Feature/Api/ClientsTest.php @@ -157,6 +157,22 @@ test('creating a client still records every field', function () { ->where('user_id', $client->id)->where('client_custom_field_id', $optIn->id)->value('value'))->toBe('0'); }); +test('a client can be created with the quota sent as a string', function () { + // Same defect as the staff screen: the `integer` rule accepts "2048" + // and hands it on unconverted, into a strict_types call expecting an + // int. A JSON number was always fine, which is why the API's own + // create test did not see it -- so this sends the quoted form a + // form-encoded caller or a cautious JSON serialiser would. + $this->withToken($this->token)->postJson('/api/v1/clients', [ + 'name' => 'Acme Ltd', + 'email' => 'string-quota@acme.test', + 'password' => 'a-sufficiently-long-password', + 'storage_quota_mb' => '2048', + ])->assertStatus(201); + + expect(User::query()->where('email', 'string-quota@acme.test')->sole()->storage_quota_mb)->toBe(2048); +}); + test('a client can be created', function () { $this->withToken($this->token)->postJson('/api/v1/clients', [ 'name' => 'Acme Ltd', diff --git a/tests/Feature/Clients/StorageQuotaTest.php b/tests/Feature/Clients/StorageQuotaTest.php index 4090de9a..6d85418c 100644 --- a/tests/Feature/Clients/StorageQuotaTest.php +++ b/tests/Feature/Clients/StorageQuotaTest.php @@ -120,6 +120,27 @@ test('the settings form still edits the stored setting, never the floor', functi ); }); +test('creating a client with a quota typed into the form stores it', function () { + // The 500 this exists for. `integer` validates a numeric string and + // does not convert it, so the form's "2048" reached + // ClientAccounts::create()'s `int $storageQuotaMb` under strict_types + // and raised a TypeError. Blank was fine -- null ?? 0 is an int -- so + // every existing test here went through the one branch that worked, + // and it shipped to the whole fleet on 2.4.1. + // + // post() and not postJson(): a form body is strings, which is the + // condition. A JSON number would pass on the unfixed code. + $this->actingAs($this->admin)->post('/clients', [ + 'name' => 'Quota Ltd', + 'email' => 'typed-quota@example.test', + 'password' => 'a-sufficiently-long-password', + 'password_confirmation' => 'a-sufficiently-long-password', + 'storage_quota_mb' => '2048', + ])->assertRedirect()->assertSessionDoesntHaveErrors(); + + expect(User::query()->where('email', 'typed-quota@example.test')->sole()->storage_quota_mb)->toBe(2048); +}); + test('clearing the storage quota field to blank on the edit form resets it to inherit the site default', function () { app(Settings::class)->set(Setting::DefaultClientStorageQuotaMb, 150); $client = User::factory()->client()->create(['storage_quota_mb' => 100]);