From b8018c5b5ed52656acdce3b7639fa7adc1f1d559 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 Jul 2026 11:17:55 +0100 Subject: [PATCH 1/7] FMCS+Floater: Fixed #19192 - make company required --- app/Helpers/Helper.php | 40 +++- app/Http/Requests/SaveUserRequest.php | 36 +++- app/Models/Accessory.php | 2 +- app/Models/Asset.php | 2 +- app/Models/Component.php | 2 +- app/Models/Consumable.php | 2 +- app/Models/Department.php | 2 +- app/Models/License.php | 2 +- app/Models/Location.php | 2 +- app/Providers/ValidationServiceProvider.php | 35 ++++ resources/lang/en-US/validation.php | 1 + .../StrictModeRequiresCompanyOnCreateTest.php | 187 ++++++++++++++++++ tests/Feature/Users/Api/UpdateUserTest.php | 31 ++- .../Feature/Users/Ui/FloaterModeGateTest.php | 13 +- 14 files changed, 328 insertions(+), 29 deletions(-) create mode 100644 tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php diff --git a/app/Helpers/Helper.php b/app/Helpers/Helper.php index 22fbcf31a2..6241b4f9d4 100644 --- a/app/Helpers/Helper.php +++ b/app/Helpers/Helper.php @@ -1111,15 +1111,21 @@ class Helper if (is_array($rule)) { if (in_array('required', $rule)) { return true; - } else { - return false; } - } else { - if (strpos($rule, 'required') === false) { - return false; - } else { + if (in_array('fmcs_company', $rule) && self::fmcsCompanyIsCurrentlyRequired()) { return true; } + + return false; + } else { + if (strpos($rule, 'required') !== false) { + return true; + } + if (strpos($rule, 'fmcs_company') !== false && self::fmcsCompanyIsCurrentlyRequired()) { + return true; + } + + return false; } } } @@ -1127,6 +1133,28 @@ class Helper return false; } + /** + * Mirror of the fmcs_company validator's runtime condition, used by + * checkIfRequired() so Blade fields render the "required" indicator + * (asterisk / aria-required) in the same conditions the backend will + * reject a blank submission. See ValidationServiceProvider. + */ + protected static function fmcsCompanyIsCurrentlyRequired(): bool + { + $settings = \App\Models\Setting::getSettings(); + if (! $settings->full_multiple_companies_support) { + return false; + } + if ((bool) $settings->null_company_is_floater) { + return false; + } + if (! auth()->check()) { + return false; + } + + return ! auth()->user()->isSuperUser(); + } + /** * Return the numeric max length declared for a field in the model's * validation rules (looks for `max:N`). Returns null when the model, field, diff --git a/app/Http/Requests/SaveUserRequest.php b/app/Http/Requests/SaveUserRequest.php index 3bda0b5a00..6fb28675d6 100644 --- a/app/Http/Requests/SaveUserRequest.php +++ b/app/Http/Requests/SaveUserRequest.php @@ -75,10 +75,18 @@ class SaveUserRequest extends FormRequest /** * Block non-superusers from saving a user whose resulting company set would - * be empty *while floater mode is enabled* — that combination promotes the - * target user to a system-wide floater (sees everything), and is the - * privilege-escalation vector flagged in #19200. Superusers can still make - * floaters intentionally. Applies to web and API store/update. + * be empty: + * + * - Floater mode ON: the resulting user would become a system-wide + * floater (sees everything). That's the privilege-escalation vector + * flagged in #19200 — only superusers (and users already able to grant + * floater status) may do this deliberately. + * - Floater mode OFF (strict FMCS): the resulting user would land with + * an empty pivot and be immediately invisible to the creator's own + * scope. That's the visibility bug flagged in #19192 — only superusers + * (whose scope reaches nulls) may do this. + * + * Applies to both web and API store/update. */ public function withValidator(Validator $validator): void { @@ -91,7 +99,7 @@ class SaveUserRequest extends FormRequest // the PHPUnit/Pest runner, which also reports as "in console" but // is using the HTTP stack and must see the gate fire. $inActualConsole = app()->runningInConsole() && ! app()->runningUnitTests(); - if ($inActualConsole || auth()->user()?->canGrantFloaterStatus()) { + if ($inActualConsole) { return; } @@ -107,7 +115,23 @@ class SaveUserRequest extends FormRequest $effective = Company::getIdsForCurrentUser($submitted); if (empty($effective)) { - $validator->errors()->add('company_ids', trans('admin/users/general.cannot_make_floater')); + $settings = Setting::getSettings(); + $creatorIsSuper = (bool) auth()->user()?->isSuperUser(); + $strictFmcs = $settings->full_multiple_companies_support && ! $settings->null_company_is_floater; + + // Strict-FMCS #19192 gate — hits before the older floater + // gate so its more specific error message wins when both + // apply. + if ($strictFmcs && ! $creatorIsSuper) { + $validator->errors()->add('company_ids', trans('validation.fmcs_company', ['attribute' => trans('general.company')])); + + return; + } + + // Original #19200 floater-grant gate. + if (! auth()->user()?->canGrantFloaterStatus()) { + $validator->errors()->add('company_ids', trans('admin/users/general.cannot_make_floater')); + } } }); } diff --git a/app/Models/Accessory.php b/app/Models/Accessory.php index a7e7169cc9..b9b5804f2d 100755 --- a/app/Models/Accessory.php +++ b/app/Models/Accessory.php @@ -82,7 +82,7 @@ class Accessory extends SnipeModel 'name' => 'required|max:255', 'qty' => 'nullable|integer|min:0', 'category_id' => 'required|integer|exists:categories,id', - 'company_id' => 'integer|nullable|exists:companies,id', + 'company_id' => 'integer|nullable|exists:companies,id|fmcs_company', 'location_id' => 'exists:locations,id|nullable|fmcs_location', 'min_amt' => 'integer|min:0|nullable', 'purchase_cost' => 'numeric|nullable|gte:0|max:99999999999999999.99', diff --git a/app/Models/Asset.php b/app/Models/Asset.php index d91ad1d22e..7998362412 100644 --- a/app/Models/Asset.php +++ b/app/Models/Asset.php @@ -113,7 +113,7 @@ class Asset extends Depreciable 'status_id' => ['required', 'integer', 'exists:status_labels,id'], 'asset_tag' => ['required', 'min:1', 'max:255', 'unique_undeleted:assets,asset_tag', 'not_array'], 'name' => ['nullable', 'max:255'], - 'company_id' => ['nullable', 'integer', 'exists:companies,id'], + 'company_id' => ['nullable', 'integer', 'exists:companies,id', 'fmcs_company'], 'warranty_months' => ['nullable', 'numeric', 'digits_between:0,240'], 'last_checkout' => ['nullable', 'date_format:Y-m-d H:i:s'], 'last_checkin' => ['nullable', 'date_format:Y-m-d H:i:s'], diff --git a/app/Models/Component.php b/app/Models/Component.php index 5e6053cb97..cb3201bb3a 100644 --- a/app/Models/Component.php +++ b/app/Models/Component.php @@ -47,7 +47,7 @@ class Component extends SnipeModel 'qty' => 'required|integer|min:1', 'category_id' => 'required|integer|exists:categories,id', 'supplier_id' => 'nullable|integer|exists:suppliers,id', - 'company_id' => 'integer|nullable|exists:companies,id', + 'company_id' => 'integer|nullable|exists:companies,id|fmcs_company', 'location_id' => 'exists:locations,id|nullable|fmcs_location', 'min_amt' => 'integer|min:0|nullable', 'purchase_date' => 'date_format:Y-m-d|nullable', diff --git a/app/Models/Consumable.php b/app/Models/Consumable.php index a2f211ff15..9b3df72b01 100644 --- a/app/Models/Consumable.php +++ b/app/Models/Consumable.php @@ -48,7 +48,7 @@ class Consumable extends SnipeModel 'name' => 'required|max:255', 'qty' => 'required|integer|min:0|max:99999', 'category_id' => 'required|integer', - 'company_id' => 'integer|nullable|exists:companies,id', + 'company_id' => 'integer|nullable|exists:companies,id|fmcs_company', 'location_id' => 'exists:locations,id|nullable|fmcs_location', 'min_amt' => 'integer|min:0|max:99999|nullable', 'purchase_cost' => 'numeric|nullable|gte:0|max:99999999999999999.99', diff --git a/app/Models/Department.php b/app/Models/Department.php index 2805dca67a..48b62bd5f1 100644 --- a/app/Models/Department.php +++ b/app/Models/Department.php @@ -46,7 +46,7 @@ class Department extends SnipeModel protected $rules = [ 'name' => 'required|string|max:255|is_unique_across_company_and_location:departments,name', 'location_id' => 'numeric|nullable|exists:locations,id', - 'company_id' => 'numeric|nullable|exists:companies,id', + 'company_id' => 'numeric|nullable|exists:companies,id|fmcs_company', 'manager_id' => 'numeric|nullable|exists:users,id', 'phone' => 'string|max:255|nullable', 'fax' => 'string|max:255|nullable', diff --git a/app/Models/License.php b/app/Models/License.php index e1c91de56d..6f78fba239 100755 --- a/app/Models/License.php +++ b/app/Models/License.php @@ -59,7 +59,7 @@ class License extends Depreciable 'license_name' => 'string|nullable|max:100', 'notes' => 'string|nullable', 'category_id' => 'required|exists:categories,id', - 'company_id' => 'integer|nullable|exists:companies,id', + 'company_id' => 'integer|nullable|exists:companies,id|fmcs_company', 'purchase_cost' => 'numeric|nullable|gte:0|max:99999999999999999.99', 'purchase_date' => 'date_format:Y-m-d|nullable|max:10|required_with:depreciation_id', 'expiration_date' => 'date_format:Y-m-d|nullable|max:10', diff --git a/app/Models/Location.php b/app/Models/Location.php index c5d32ba990..e4507cc110 100755 --- a/app/Models/Location.php +++ b/app/Models/Location.php @@ -43,7 +43,7 @@ class Location extends SnipeModel 'zip' => 'max:10|nullable', 'manager_id' => 'exists:users,id|nullable', 'parent_id' => 'nullable|exists:locations,id|non_circular:locations,id', - 'company_id' => 'integer|nullable|exists:companies,id', + 'company_id' => 'integer|nullable|exists:companies,id|fmcs_company', ]; protected $casts = [ diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index 011967c794..73d860d41c 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -452,6 +452,41 @@ class ValidationServiceProvider extends ServiceProvider ); }); + // Enforces "Company must be picked" when FMCS is on AND + // null_company_is_floater is disabled (strict mode). Without this + // rule non-superuser users can save a form with an unset company + // dropdown, land a row with company_id=NULL, and then have that + // row instantly filtered out of their own view by the strict-mode + // scope. See #19192. Passes when: + // - FMCS is off (nothing to enforce) + // - null_company_is_floater is on (nulls are legal floaters) + // - value is present (form was filled in) + // - acting user is a superuser (they see everything; a null is + // an explicit choice, not an accident) + // - no auth context (CLI / seeders / importers bypass — same + // posture as SaveUserRequest's cannot_make_floater gate). + Validator::extend('fmcs_company', function ($attribute, $value, $parameters, $validator) { + $settings = Setting::getSettings(); + if (! $settings->full_multiple_companies_support) { + return true; + } + if ((bool) $settings->null_company_is_floater) { + return true; + } + if (! empty($value)) { + return true; + } + if (! auth()->check()) { + return true; + } + + return (bool) auth()->user()->isSuperUser(); + }); + + Validator::replacer('fmcs_company', function ($message, $attribute, $rule, $parameters) { + return str_replace(':attribute', trans('general.company'), $message); + }); + // Validates that the company of the validated object matches the company of the location in case of scoped locations Validator::extend('fmcs_location', function ($attribute, $value, $parameters, $validator) { $settings = Setting::getSettings(); diff --git a/resources/lang/en-US/validation.php b/resources/lang/en-US/validation.php index 49736251bf..9d172a7ec2 100644 --- a/resources/lang/en-US/validation.php +++ b/resources/lang/en-US/validation.php @@ -177,6 +177,7 @@ return [ 'ulid' => 'The :attribute field must be a valid ULID.', 'uuid' => 'The :attribute field must be a valid UUID.', 'valid_css_color' => 'The :attribute field must be a valid CSS color (hex, rgb, rgba, hsl, or hsla).', + 'fmcs_company' => 'The :attribute field is required because full multiple companies support is enabled and floaters are not allowed.', 'fmcs_location' => 'Location ":location" belongs to :location_company, which does not match the selected company.', 'is_unique_across_company_and_location' => 'The :attribute must be unique within the selected company and location.', diff --git a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php new file mode 100644 index 0000000000..351e744435 --- /dev/null +++ b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php @@ -0,0 +1,187 @@ +settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + auth()->login(User::factory()->create()); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertTrue($validator->fails()); + $this->assertArrayHasKey('company_id', $validator->errors()->toArray()); + } + + public function test_rule_accepts_null_in_strict_fmcs_for_superuser() + { + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + auth()->login(User::factory()->superuser()->create()); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_null_when_floater_mode_enabled() + { + $this->settings->enableFloaterMode(); + auth()->login(User::factory()->create()); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_null_when_fmcs_off() + { + $this->settings->disableMultipleFullCompanySupport(); + auth()->login(User::factory()->create()); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_non_null_in_strict_fmcs_for_non_superuser() + { + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + auth()->login(User::factory()->create()); + $company = Company::factory()->create(); + + $validator = Validator::make(['company_id' => $company->id], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_null_when_no_auth_context() + { + // CLI / seeders / importers deliberately bypass — same posture + // as the SaveUserRequest cannot_make_floater gate. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + auth()->logout(); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + // ------------------------------------------------------------------ + // Sanity: every model the reporter listed has the rule wired + // ------------------------------------------------------------------ + + /** + * @dataProvider companyableModelProvider + */ + public function test_model_rules_include_fmcs_company_for_company_id(string $modelClass) + { + $rules = $modelClass::rules(); + $this->assertArrayHasKey('company_id', $rules, $modelClass.' should declare a company_id rule'); + + $companyRule = $rules['company_id']; + $ruleString = is_array($companyRule) ? implode('|', $companyRule) : $companyRule; + + $this->assertStringContainsString( + 'fmcs_company', + $ruleString, + $modelClass.'::rules()[company_id] must include the fmcs_company validator so strict-FMCS mode rejects blank submissions', + ); + } + + public static function companyableModelProvider(): array + { + return [ + 'Asset' => [Asset::class], + 'License' => [License::class], + 'Accessory' => [Accessory::class], + 'Consumable' => [Consumable::class], + 'Component' => [Component::class], + 'Department' => [Department::class], + 'Location' => [Location::class], + ]; + } + + // ------------------------------------------------------------------ + // Users: gate lives in SaveUserRequest, not model $rules + // ------------------------------------------------------------------ + + public function test_users_strict_fmcs_rejects_empty_company_ids_for_non_superuser() + { + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $actor = User::factory()->create(); + $username = 'strict-null-target-'.uniqid(); + + $this->actingAs($actor) + ->post(route('users.store'), [ + 'first_name' => 'Test', + 'last_name' => 'User', + 'username' => $username, + 'email' => $username.'@example.com', + 'password' => 'SomeGreatPassword-123', + 'password_confirmation' => 'SomeGreatPassword-123', + // No company_ids submitted. + ]) + ->assertSessionHasErrors('company_ids'); + + $this->assertDatabaseMissing('users', ['username' => $username]); + } + + public function test_users_strict_fmcs_allows_empty_company_ids_for_superuser() + { + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $actor = User::factory()->superuser()->create(); + $username = 'super-null-'.uniqid(); + + $this->actingAs($actor) + ->post(route('users.store'), [ + 'first_name' => 'Superuser-Created', + 'last_name' => 'User', + 'username' => $username, + 'email' => $username.'@example.com', + 'password' => 'SomeGreatPassword-123', + 'password_confirmation' => 'SomeGreatPassword-123', + ]) + ->assertSessionHasNoErrors('company_ids'); + } +} diff --git a/tests/Feature/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php index b981608523..3b455831d7 100644 --- a/tests/Feature/Users/Api/UpdateUserTest.php +++ b/tests/Feature/Users/Api/UpdateUserTest.php @@ -322,9 +322,16 @@ class UpdateUserTest extends TestCase $scoped_user_in_companyB = User::factory()->forCompany($companyB->id)->create(); $scoped_user_in_no_company = User::factory()->withoutCompany()->create(); + // Each PATCH carries company_ids so the strict-FMCS gate added + // for #19192 doesn't hijack the authorization assertion — the + // test's intent is to verify company-scoped authorization, not + // to exercise the empty-pivot gate. + $bodyA = ['company_ids' => [$companyA->id]]; + $bodyB = ['company_ids' => [$companyB->id]]; + // Admin for Company A should allow updating user from Company A $this->actingAsForApi($adminA) - ->patchJson(route('api.users.update', $scoped_user_in_companyA)) + ->patchJson(route('api.users.update', $scoped_user_in_companyA), $bodyA) ->assertOk() ->assertStatus(200) ->assertStatusMessageIs('success') @@ -332,7 +339,7 @@ class UpdateUserTest extends TestCase // Admin for Company A should get denied updating user from Company B $this->actingAsForApi($adminA) - ->patchJson(route('api.users.update', $scoped_user_in_companyB)) + ->patchJson(route('api.users.update', $scoped_user_in_companyB), $bodyB) ->assertOk() ->assertStatus(200) ->assertStatusMessageIs('error') @@ -340,7 +347,7 @@ class UpdateUserTest extends TestCase // Admin for Company A should get denied updating user without a company $this->actingAsForApi($adminA) - ->patchJson(route('api.users.update', $scoped_user_in_no_company)) + ->patchJson(route('api.users.update', $scoped_user_in_no_company), $bodyA) ->assertOk() ->assertStatus(200) ->assertStatusMessageIs('error') @@ -348,7 +355,7 @@ class UpdateUserTest extends TestCase // Admin for Company B should allow updating user from Company B $this->actingAsForApi($adminB) - ->patchJson(route('api.users.update', $scoped_user_in_companyB)) + ->patchJson(route('api.users.update', $scoped_user_in_companyB), $bodyB) ->assertOk() ->assertStatus(200) ->assertStatusMessageIs('success') @@ -356,7 +363,7 @@ class UpdateUserTest extends TestCase // Admin for Company B should get denied updating user from Company A $this->actingAsForApi($adminB) - ->patchJson(route('api.users.update', $scoped_user_in_companyA)) + ->patchJson(route('api.users.update', $scoped_user_in_companyA), $bodyA) ->assertOk() ->assertStatus(200) ->assertStatusMessageIs('error') @@ -364,18 +371,26 @@ class UpdateUserTest extends TestCase // Admin for Company B should get denied updating user without a company $this->actingAsForApi($adminB) - ->patchJson(route('api.users.update', $scoped_user_in_no_company)) + ->patchJson(route('api.users.update', $scoped_user_in_no_company), $bodyB) ->assertOk() ->assertStatus(200) ->assertStatusMessageIs('error') ->json(); - // Admin without a company should allow updating user without a company + // Behavior change note (#19192): an admin with no company + // memberships in strict FMCS mode cannot successfully PATCH any + // user. Empty company_ids trips the new gate; any non-empty + // list is filtered to empty by Company::getIdsForCurrentUser() + // because they have no accessible companies. Before the gate, + // the no-company-user case below was a permissive no-op + // success — that path is now closed. Practical impact: strict + // FMCS deployments should grant such admins at least one + // company (or superuser) so they can act. $this->actingAsForApi($adminNoCompany) ->patchJson(route('api.users.update', $scoped_user_in_no_company)) ->assertOk() ->assertStatus(200) - ->assertStatusMessageIs('success') + ->assertStatusMessageIs('error') ->json(); // Admin without a company should get denied updating user from Company A diff --git a/tests/Feature/Users/Ui/FloaterModeGateTest.php b/tests/Feature/Users/Ui/FloaterModeGateTest.php index 0c5da490a6..9a4ad25e00 100644 --- a/tests/Feature/Users/Ui/FloaterModeGateTest.php +++ b/tests/Feature/Users/Ui/FloaterModeGateTest.php @@ -72,8 +72,17 @@ class FloaterModeGateTest extends TestCase $this->assertEmpty($target->fresh()->companies->pluck('id')->all(), 'Superuser is trusted to grant floater status'); } - public function test_guard_does_not_apply_when_floater_mode_is_off() + public function test_strict_mode_now_blocks_non_superuser_from_clearing_company_ids() { + // Prior to #19192 this test documented that the #19200 floater + // gate skipped strict mode entirely (canGrantFloaterStatus() + // returns true when floaters are off, so no check fired). The + // reporter's #19192 case demonstrated that same permissive + // behavior lets a non-superuser end up with an empty pivot in + // strict FMCS mode, making the target instantly invisible to + // its creator's scope. The gate now also fires in strict mode + // for non-superusers, so a non-superuser cannot clear a user's + // company memberships to empty either. $this->settings->enableMultipleFullCompanySupport(); $this->settings->disableFloaterMode(); @@ -86,7 +95,7 @@ class FloaterModeGateTest extends TestCase 'username' => $actor->username, 'company_ids' => [], ]) - ->assertSessionDoesntHaveErrors('company_ids'); + ->assertSessionHasErrors('company_ids'); } public function test_non_superuser_cannot_bulk_clear_companies_in_floater_mode() From b9672dc238f051a2dfb9508ee9a5265d0e6a2193 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 Jul 2026 11:45:31 +0100 Subject: [PATCH 2/7] Accoutn for deliberate null company pseudo company --- app/Helpers/Helper.php | 9 +++- .../Controllers/Users/BulkUsersController.php | 19 ++++++- app/Http/Requests/SaveUserRequest.php | 13 +++-- app/Providers/ValidationServiceProvider.php | 28 ++++++++--- .../StrictModeRequiresCompanyOnCreateTest.php | 39 +++++++++++++++ tests/Feature/Users/Api/UpdateUserTest.php | 16 +++--- .../Feature/Users/Ui/FloaterModeGateTest.php | 50 +++++++++++++++++++ 7 files changed, 151 insertions(+), 23 deletions(-) diff --git a/app/Helpers/Helper.php b/app/Helpers/Helper.php index 6241b4f9d4..755cfcc5ee 100644 --- a/app/Helpers/Helper.php +++ b/app/Helpers/Helper.php @@ -1151,8 +1151,15 @@ class Helper if (! auth()->check()) { return false; } + $actor = auth()->user(); + if ($actor->isSuperUser()) { + return false; + } - return ! auth()->user()->isSuperUser(); + // Uncompanied users work in the null pseudo-company namespace + // under strict mode; null IS a valid company id for them, so + // don't render the field as required. + return $actor->companies()->exists(); } /** diff --git a/app/Http/Controllers/Users/BulkUsersController.php b/app/Http/Controllers/Users/BulkUsersController.php index bf836813fc..0eed29b406 100644 --- a/app/Http/Controllers/Users/BulkUsersController.php +++ b/app/Http/Controllers/Users/BulkUsersController.php @@ -275,8 +275,25 @@ class BulkUsersController extends Controller $allowedIds = Company::getIdsForCurrentUser($bulkCompanyIds); } - // Floater-mode self-elevation guard (#19200). See User::canGrantFloaterStatus. $wouldClear = $clearCompanies || ($bulkCompanyIds && empty($allowedIds)); + + // Strict-FMCS #19192 gate — mirrors the branch in SaveUserRequest + // so bulk-editing a batch to clear all company memberships in + // strict mode is blocked for companied non-superusers. Fires + // before the older floater-mode gate so its more specific error + // wins when both apply. Skips uncompanied actors because they + // legitimately operate in the null pseudo-company namespace in + // strict mode; nulling pivots there is their normal workflow, + // not a self-escalation attempt. + $settings = Setting::getSettings(); + $strictFmcs = $settings->full_multiple_companies_support && ! $settings->null_company_is_floater; + $actor = auth()->user(); + if ($wouldClear && $strictFmcs && ! $actor->isSuperUser() && $actor->companies()->exists()) { + return redirect()->route('users.index') + ->with('error', trans('validation.fmcs_company', ['attribute' => trans('general.company')])); + } + + // Floater-mode self-elevation guard (#19200). See User::canGrantFloaterStatus. if ($wouldClear && ! auth()->user()->canGrantFloaterStatus()) { return redirect()->route('users.index') ->with('error', trans('admin/users/general.cannot_make_floater')); diff --git a/app/Http/Requests/SaveUserRequest.php b/app/Http/Requests/SaveUserRequest.php index 6fb28675d6..b42a4b2d1d 100644 --- a/app/Http/Requests/SaveUserRequest.php +++ b/app/Http/Requests/SaveUserRequest.php @@ -116,20 +116,25 @@ class SaveUserRequest extends FormRequest if (empty($effective)) { $settings = Setting::getSettings(); - $creatorIsSuper = (bool) auth()->user()?->isSuperUser(); + $actor = auth()->user(); + $creatorIsSuper = (bool) $actor?->isSuperUser(); + $creatorHasCompanies = (bool) $actor?->companies()->exists(); $strictFmcs = $settings->full_multiple_companies_support && ! $settings->null_company_is_floater; // Strict-FMCS #19192 gate — hits before the older floater // gate so its more specific error message wins when both - // apply. - if ($strictFmcs && ! $creatorIsSuper) { + // apply. Skips uncompanied actors because they legitimately + // work in the null pseudo-company namespace under strict + // mode; forcing them to add memberships they don't have + // would lock them out of their normal workflow. + if ($strictFmcs && ! $creatorIsSuper && $creatorHasCompanies) { $validator->errors()->add('company_ids', trans('validation.fmcs_company', ['attribute' => trans('general.company')])); return; } // Original #19200 floater-grant gate. - if (! auth()->user()?->canGrantFloaterStatus()) { + if (! $actor?->canGrantFloaterStatus()) { $validator->errors()->add('company_ids', trans('admin/users/general.cannot_make_floater')); } } diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index 73d860d41c..7d7b5abc9e 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -454,17 +454,24 @@ class ValidationServiceProvider extends ServiceProvider // Enforces "Company must be picked" when FMCS is on AND // null_company_is_floater is disabled (strict mode). Without this - // rule non-superuser users can save a form with an unset company - // dropdown, land a row with company_id=NULL, and then have that - // row instantly filtered out of their own view by the strict-mode - // scope. See #19192. Passes when: + // rule a companied non-superuser can save a form with an unset + // company dropdown, land a row with company_id=NULL, and then + // have that row instantly filtered out of their own view by the + // strict-mode scope. See #19192. Passes when: // - FMCS is off (nothing to enforce) // - null_company_is_floater is on (nulls are legal floaters) // - value is present (form was filled in) + // - no auth context (CLI / seeders / importers bypass — same + // posture as SaveUserRequest's cannot_make_floater gate) // - acting user is a superuser (they see everything; a null is // an explicit choice, not an accident) - // - no auth context (CLI / seeders / importers bypass — same - // posture as SaveUserRequest's cannot_make_floater gate). + // - acting user has NO company memberships. In strict mode + // such users legitimately operate in the null "pseudo-company" + // namespace — Company::scopeCompanyablesDirectly scopes them + // to whereNull($company_id), so null IS a valid company id + // for them. Forcing them to pick a non-null company would + // both lock them out of their normal workflow and produce a + // row they wouldn't be able to see afterward. Validator::extend('fmcs_company', function ($attribute, $value, $parameters, $validator) { $settings = Setting::getSettings(); if (! $settings->full_multiple_companies_support) { @@ -479,8 +486,15 @@ class ValidationServiceProvider extends ServiceProvider if (! auth()->check()) { return true; } + $actor = auth()->user(); + if ($actor->isSuperUser()) { + return true; + } + if (! $actor->companies()->exists()) { + return true; + } - return (bool) auth()->user()->isSuperUser(); + return false; }); Validator::replacer('fmcs_company', function ($message, $attribute, $rule, $parameters) { diff --git a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php index 351e744435..65574c26e0 100644 --- a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php +++ b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php @@ -103,6 +103,45 @@ class StrictModeRequiresCompanyOnCreateTest extends TestCase $this->assertFalse($validator->fails()); } + public function test_rule_accepts_null_for_uncompanied_non_superuser_in_strict_mode() + { + // Regression guard for the pseudo-company workflow. Under + // Company::scopeCompanyablesDirectly in strict mode, actors + // with no company memberships are scoped to null-company rows + // (whereNull($column)). Null IS a valid company id for them — + // forcing them to pick a non-null company would both lock them + // out of their normal workflow AND produce a row they wouldn't + // be able to see afterward. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $actor = User::factory()->withoutCompany()->create(); + $this->assertFalse($actor->companies()->exists(), 'test precondition: actor is uncompanied'); + auth()->login($actor); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_still_rejects_null_for_companied_non_superuser_in_strict_mode() + { + // Reporter's #19192 case: a non-superuser WITH company + // memberships submitting a null company_id would land an + // invisible row. That must still fail. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $company = Company::factory()->create(); + $actor = $company->users()->save(User::factory()->create()); + $this->assertTrue($actor->companies()->exists(), 'test precondition: actor has memberships'); + auth()->login($actor); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertTrue($validator->fails()); + } + // ------------------------------------------------------------------ // Sanity: every model the reporter listed has the rule wired // ------------------------------------------------------------------ diff --git a/tests/Feature/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php index 3b455831d7..6d4cec721a 100644 --- a/tests/Feature/Users/Api/UpdateUserTest.php +++ b/tests/Feature/Users/Api/UpdateUserTest.php @@ -377,20 +377,16 @@ class UpdateUserTest extends TestCase ->assertStatusMessageIs('error') ->json(); - // Behavior change note (#19192): an admin with no company - // memberships in strict FMCS mode cannot successfully PATCH any - // user. Empty company_ids trips the new gate; any non-empty - // list is filtered to empty by Company::getIdsForCurrentUser() - // because they have no accessible companies. Before the gate, - // the no-company-user case below was a permissive no-op - // success — that path is now closed. Practical impact: strict - // FMCS deployments should grant such admins at least one - // company (or superuser) so they can act. + // Admin without a company should allow updating user without + // a company. Under strict FMCS mode uncompanied users operate + // in the null pseudo-company namespace (Company scoping shows + // them null-company rows); the #19192 gate steps aside for + // them so this normal workflow keeps working. $this->actingAsForApi($adminNoCompany) ->patchJson(route('api.users.update', $scoped_user_in_no_company)) ->assertOk() ->assertStatus(200) - ->assertStatusMessageIs('error') + ->assertStatusMessageIs('success') ->json(); // Admin without a company should get denied updating user from Company A diff --git a/tests/Feature/Users/Ui/FloaterModeGateTest.php b/tests/Feature/Users/Ui/FloaterModeGateTest.php index 9a4ad25e00..cf6c0d3d39 100644 --- a/tests/Feature/Users/Ui/FloaterModeGateTest.php +++ b/tests/Feature/Users/Ui/FloaterModeGateTest.php @@ -118,6 +118,56 @@ class FloaterModeGateTest extends TestCase $this->assertNotEmpty($victim->fresh()->companies); } + public function test_non_superuser_cannot_bulk_clear_companies_in_strict_fmcs_mode() + { + // #19192 companion to the floater-mode test above: strict FMCS + // (floaters OFF) also blocks a non-superuser from bulk-clearing + // pivot memberships, because doing so would make every targeted + // user instantly invisible to the acting admin's own scope. + // Pre-fix, BulkUsersController's only gate was canGrantFloaterStatus, + // which returns true in strict mode and let the clear proceed. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $company = Company::factory()->create(); + $actor = $company->users()->save(User::factory()->editUsers()->create()); + $victim = $company->users()->save(User::factory()->create()); + + $this->actingAs($actor) + ->post(route('users/bulkeditsave'), [ + 'ids' => [$victim->id => '1'], + 'null_company_ids' => '1', + ]) + ->assertRedirect() + ->assertSessionHas('error'); + + $this->assertContains($company->id, $victim->fresh()->companies->pluck('id')->all(), 'Bulk clear in strict mode should be refused'); + $this->assertNotEmpty($victim->fresh()->companies); + } + + public function test_strict_fmcs_bulk_clear_gate_does_not_fire_for_superuser() + { + // Superusers see everything (their scope reaches null-pivot + // rows), so leaving pivots empty is a deliberate action for + // them, not the visibility trap that motivates #19192. The + // gate must not fire — whether the pivot ends up actually + // cleared is a separate concern owned by the bulk-clear + // controller flow, tested elsewhere. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $company = Company::factory()->create(); + $actor = User::factory()->superuser()->create(); + $victim = $company->users()->save(User::factory()->create()); + + $this->actingAs($actor) + ->post(route('users/bulkeditsave'), [ + 'ids' => [$victim->id => '1'], + 'null_company_ids' => '1', + ]) + ->assertSessionMissing('error'); + } + public function test_non_superuser_cannot_create_a_new_user_with_no_companies_in_floater_mode() { // Covers the web POST path — SaveUserRequest's withValidator fires on From 37238625e506cdba65c41611caa98bc7f6118001 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 Jul 2026 12:06:36 +0100 Subject: [PATCH 3/7] Updated user request for PATCH --- app/Http/Requests/UpdateAssetRequest.php | 17 ++++ app/Providers/ValidationServiceProvider.php | 7 +- .../StrictModeRequiresCompanyOnCreateTest.php | 78 +++++++++++++++++++ 3 files changed, 101 insertions(+), 1 deletion(-) diff --git a/app/Http/Requests/UpdateAssetRequest.php b/app/Http/Requests/UpdateAssetRequest.php index 5f5e98a199..19587ac4f5 100644 --- a/app/Http/Requests/UpdateAssetRequest.php +++ b/app/Http/Requests/UpdateAssetRequest.php @@ -48,6 +48,23 @@ class UpdateAssetRequest extends ImageUploadRequest // route through checkOut() and produce the required audit-log entry). unset($assetRules['assigned_to'], $assetRules['assigned_type']); + // Strip fmcs_company from the request-level company_id rule. It's an + // implicit rule (fires on null/absent) which is right for the + // model-level ValidatingTrait check on save, but wrong at the + // request-level PATCH context — omitting company_id means "don't + // change it," not "set to null." The model-level rule still + // catches a genuine null-value save. StoreAssetRequest and + // BulkUpdateAssetsRequest inherit from this indirectly via + // parent classes; the store path keeps the rule (create should + // require Company), the bulk update path is already covered by + // this strip since it extends UpdateAssetRequest. + if (isset($assetRules['company_id']) && is_array($assetRules['company_id'])) { + $assetRules['company_id'] = array_values(array_filter( + $assetRules['company_id'], + fn ($rule) => $rule !== 'fmcs_company', + )); + } + // On the singular endpoint we can tell Rule::unique to ignore the // asset being updated. Bulk (BulkUpdateAssetsRequest) overrides this // to null because it would have to ignore N different ids at once, diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index 7d7b5abc9e..3926e68a0e 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -472,7 +472,12 @@ class ValidationServiceProvider extends ServiceProvider // for them. Forcing them to pick a non-null company would // both lock them out of their normal workflow and produce a // row they wouldn't be able to see afterward. - Validator::extend('fmcs_company', function ($attribute, $value, $parameters, $validator) { + // extendImplicit (not extend) — Laravel skips "explicit" rules + // when the field is null/absent. Since the whole point of + // fmcs_company is to fire ON blank submissions, it must run + // implicitly. Same reason built-in rules like `required`, + // `filled`, `present`, `accepted` are all registered implicit. + Validator::extendImplicit('fmcs_company', function ($attribute, $value, $parameters, $validator) { $settings = Setting::getSettings(); if (! $settings->full_multiple_companies_support) { return true; diff --git a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php index 65574c26e0..8efb5fe5ec 100644 --- a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php +++ b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php @@ -204,6 +204,84 @@ class StrictModeRequiresCompanyOnCreateTest extends TestCase $this->assertDatabaseMissing('users', ['username' => $username]); } + // ------------------------------------------------------------------ + // Bulk asset edit: same gate reaches through ValidatingTrait + // ------------------------------------------------------------------ + + public function test_bulk_asset_edit_clear_company_is_rejected_for_companied_non_superuser() + { + // BulkAssetsController::update() calls $asset->update($updateArray) + // per row; the model-level fmcs_company rule fires when + // company_id gets filled to null via the 'clear' bulk option. + // Locked in here so a future refactor of the bulk controller + // can't quietly bypass the gate. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $company = \App\Models\Company::factory()->create(); + $actor = $company->users()->save(User::factory()->editAssets()->create()); + $target = \App\Models\Asset::factory()->create(['company_id' => $company->id]); + + $this->actingAs($actor) + ->post(route('hardware/bulksave'), [ + 'ids' => [$target->id => '1'], + 'company_id' => 'clear', + 'bulk_actions' => 'edit', + ]); + + // Row's company should NOT have been cleared. + $this->assertEquals($company->id, $target->fresh()->company_id); + } + + public function test_bulk_asset_edit_clear_company_is_allowed_for_uncompanied_non_superuser() + { + // Uncompanied non-superusers work in the null pseudo-company + // namespace under strict mode. Bulk-clearing Company on rows + // they own is a legitimate operation for them and the gate + // steps aside. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $actor = User::factory()->withoutCompany()->editAssets()->create(); + $target = \App\Models\Asset::factory()->create(['company_id' => null]); + + $this->actingAs($actor) + ->post(route('hardware/bulksave'), [ + 'ids' => [$target->id => '1'], + 'company_id' => 'clear', + 'bulk_actions' => 'edit', + ]); + + $this->assertNull($target->fresh()->company_id); + } + + public function test_bulk_asset_edit_unrelated_field_still_works_for_companied_non_superuser() + { + // If the bulk edit doesn't touch Company at all, ValidatingTrait + // sees the existing non-null company_id on each row and passes + // — this is the "make sure we haven't broken ordinary bulk + // edits" sanity guard. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $company = \App\Models\Company::factory()->create(); + $actor = $company->users()->save(User::factory()->editAssets()->create()); + $target = \App\Models\Asset::factory()->create([ + 'company_id' => $company->id, + 'notes' => 'before', + ]); + + $this->actingAs($actor) + ->post(route('hardware/bulksave'), [ + 'ids' => [$target->id => '1'], + 'notes' => 'after', + 'bulk_actions' => 'edit', + ]); + + $this->assertEquals('after', $target->fresh()->notes); + $this->assertEquals($company->id, $target->fresh()->company_id); + } + public function test_users_strict_fmcs_allows_empty_company_ids_for_superuser() { $this->settings->enableMultipleFullCompanySupport(); From 4aaea28a37b5d9ef10dde3ea25fcde5d9a7d9737 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 Jul 2026 13:27:11 +0100 Subject: [PATCH 4/7] Added debugging for CI, since tests pass locally --- .../StrictModeRequiresCompanyOnCreateTest.php | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php index 8efb5fe5ec..f73cd20c00 100644 --- a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php +++ b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php @@ -258,9 +258,19 @@ class StrictModeRequiresCompanyOnCreateTest extends TestCase public function test_bulk_asset_edit_unrelated_field_still_works_for_companied_non_superuser() { // If the bulk edit doesn't touch Company at all, ValidatingTrait - // sees the existing non-null company_id on each row and passes - // — this is the "make sure we haven't broken ordinary bulk - // edits" sanity guard. + // sees the existing non-null company_id on each row and passes. + // This is the "make sure we haven't broken ordinary bulk edits" + // sanity guard. + // + // Defensive Setting cache flush. Some upstream tests in the full + // MySQL sweep can leave the memoized Setting instance in a state + // where full_multiple_companies_support looks enabled at the + // moment auth loads but disabled by the time the fmcs_company + // validator reads it, or vice versa. The Support helper's update() + // clears the cache too, but only after both writes have landed. + // Clearing here first pins the pre-state so the two writes below + // are the only source of truth for this test's fmcs_company check. + \App\Models\Setting::$_cache = null; $this->settings->enableMultipleFullCompanySupport(); $this->settings->disableFloaterMode(); @@ -271,13 +281,20 @@ class StrictModeRequiresCompanyOnCreateTest extends TestCase 'notes' => 'before', ]); - $this->actingAs($actor) + $response = $this->actingAs($actor) ->post(route('hardware/bulksave'), [ 'ids' => [$target->id => '1'], 'notes' => 'after', 'bulk_actions' => 'edit', ]); + // Fail loudly if the controller redirected back with a flash + // error (the notes assertion below is a downstream symptom and + // hides the real cause). Seen on MySQL CI as a full-suite flake + // — pinning the response state up front turns any recurrence + // into an actionable diagnostic. + $response->assertSessionMissing('error'); + $this->assertEquals('after', $target->fresh()->notes); $this->assertEquals($company->id, $target->fresh()->company_id); } From c091174ff5942e7eee080d7e1929a1e5d3000b4c Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 Jul 2026 13:41:00 +0100 Subject: [PATCH 5/7] Made codacy happy - split tests out --- app/Providers/ValidationServiceProvider.php | 22 +- .../Fmcs/FmcsCompanyRuleWiringTest.php | 54 +++ .../Feature/Fmcs/FmcsCompanyValidatorTest.php | 132 +++++++ .../Fmcs/FmcsStrictBulkAssetEditTest.php | 110 ++++++ .../Feature/Fmcs/FmcsStrictUsersHttpTest.php | 62 ++++ .../StrictModeRequiresCompanyOnCreateTest.php | 321 ------------------ 6 files changed, 369 insertions(+), 332 deletions(-) create mode 100644 tests/Feature/Fmcs/FmcsCompanyRuleWiringTest.php create mode 100644 tests/Feature/Fmcs/FmcsCompanyValidatorTest.php create mode 100644 tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php create mode 100644 tests/Feature/Fmcs/FmcsStrictUsersHttpTest.php delete mode 100644 tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index 3926e68a0e..ba699ab1ca 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -461,20 +461,20 @@ class ValidationServiceProvider extends ServiceProvider // - FMCS is off (nothing to enforce) // - null_company_is_floater is on (nulls are legal floaters) // - value is present (form was filled in) - // - no auth context (CLI / seeders / importers bypass — same - // posture as SaveUserRequest's cannot_make_floater gate) - // - acting user is a superuser (they see everything; a null is - // an explicit choice, not an accident) + // - no auth context (CLI, seeders, or importers bypass, matching + // the SaveUserRequest cannot_make_floater gate posture) + // - acting user is a superuser (they see everything, so a null + // is an explicit choice, not an accident) // - acting user has NO company memberships. In strict mode // such users legitimately operate in the null "pseudo-company" - // namespace — Company::scopeCompanyablesDirectly scopes them - // to whereNull($company_id), so null IS a valid company id - // for them. Forcing them to pick a non-null company would + // namespace, where Company::scopeCompanyablesDirectly scopes + // them to whereNull($company_id) and null IS a valid company + // id for them. Forcing them to pick a non-null company would // both lock them out of their normal workflow and produce a // row they wouldn't be able to see afterward. - // extendImplicit (not extend) — Laravel skips "explicit" rules - // when the field is null/absent. Since the whole point of - // fmcs_company is to fire ON blank submissions, it must run + // extendImplicit (not extend) because Laravel skips "explicit" + // rules when the field is null or absent. Since the whole point + // of fmcs_company is to fire ON blank submissions, it must run // implicitly. Same reason built-in rules like `required`, // `filled`, `present`, `accepted` are all registered implicit. Validator::extendImplicit('fmcs_company', function ($attribute, $value, $parameters, $validator) { @@ -502,7 +502,7 @@ class ValidationServiceProvider extends ServiceProvider return false; }); - Validator::replacer('fmcs_company', function ($message, $attribute, $rule, $parameters) { + Validator::replacer('fmcs_company', function ($message) { return str_replace(':attribute', trans('general.company'), $message); }); diff --git a/tests/Feature/Fmcs/FmcsCompanyRuleWiringTest.php b/tests/Feature/Fmcs/FmcsCompanyRuleWiringTest.php new file mode 100644 index 0000000000..3571f0190f --- /dev/null +++ b/tests/Feature/Fmcs/FmcsCompanyRuleWiringTest.php @@ -0,0 +1,54 @@ +assertArrayHasKey('company_id', $rules, $modelClass.' should declare a company_id rule'); + + $companyRule = $rules['company_id']; + $ruleString = is_array($companyRule) ? implode('|', $companyRule) : $companyRule; + + $this->assertStringContainsString( + 'fmcs_company', + $ruleString, + $modelClass.'::rules()[company_id] must include the fmcs_company validator so strict-FMCS mode rejects blank submissions', + ); + } + + public static function companyableModelProvider(): array + { + return [ + 'Asset' => [Asset::class], + 'License' => [License::class], + 'Accessory' => [Accessory::class], + 'Consumable' => [Consumable::class], + 'Component' => [Component::class], + 'Department' => [Department::class], + 'Location' => [Location::class], + ]; + } +} diff --git a/tests/Feature/Fmcs/FmcsCompanyValidatorTest.php b/tests/Feature/Fmcs/FmcsCompanyValidatorTest.php new file mode 100644 index 0000000000..6c07282a3e --- /dev/null +++ b/tests/Feature/Fmcs/FmcsCompanyValidatorTest.php @@ -0,0 +1,132 @@ +settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + auth()->login(User::factory()->create()); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertTrue($validator->fails()); + $this->assertArrayHasKey('company_id', $validator->errors()->toArray()); + } + + public function test_rule_accepts_null_in_strict_fmcs_for_superuser() + { + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + auth()->login(User::factory()->superuser()->create()); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_null_when_floater_mode_enabled() + { + $this->settings->enableFloaterMode(); + auth()->login(User::factory()->create()); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_null_when_fmcs_off() + { + $this->settings->disableMultipleFullCompanySupport(); + auth()->login(User::factory()->create()); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_non_null_in_strict_fmcs_for_non_superuser() + { + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + auth()->login(User::factory()->create()); + $company = Company::factory()->create(); + + $validator = Validator::make(['company_id' => $company->id], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_null_when_no_auth_context() + { + // CLI, seeders, and importers deliberately bypass, matching the + // SaveUserRequest cannot_make_floater gate's posture. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + auth()->logout(); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_accepts_null_for_uncompanied_non_superuser_in_strict_mode() + { + // Regression guard for the pseudo-company workflow. Under + // Company::scopeCompanyablesDirectly in strict mode, actors + // with no company memberships are scoped to null-company rows + // (whereNull($column)). Null IS a valid company id for them. + // Forcing them to pick a non-null company would both lock them + // out of their normal workflow AND produce a row they wouldn't + // be able to see afterward. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $actor = User::factory()->withoutCompany()->create(); + $this->assertFalse($actor->companies()->exists(), 'test precondition: actor is uncompanied'); + auth()->login($actor); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertFalse($validator->fails()); + } + + public function test_rule_still_rejects_null_for_companied_non_superuser_in_strict_mode() + { + // Reporter's #19192 case. A non-superuser WITH company + // memberships submitting a null company_id would land an + // invisible row. That must still fail. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $company = Company::factory()->create(); + $actor = $company->users()->save(User::factory()->create()); + $this->assertTrue($actor->companies()->exists(), 'test precondition: actor has memberships'); + auth()->login($actor); + + $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); + + $this->assertTrue($validator->fails()); + } +} diff --git a/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php b/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php new file mode 100644 index 0000000000..4e691c27cc --- /dev/null +++ b/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php @@ -0,0 +1,110 @@ +update($updateArray) + * per row, so the model-level fmcs_company rule reaches the bulk path + * via ValidatingTrait. These tests pin the behavior on both the + * "clearing Company should fail" and the "clearing Company is fine for + * uncompanied actors working in the pseudo-company namespace" paths, + * plus a sanity check that ordinary bulk edits touching only unrelated + * fields still succeed. + */ +class FmcsStrictBulkAssetEditTest extends TestCase +{ + public function test_clearing_company_is_rejected_for_companied_non_superuser() + { + // Locked in so a future refactor of the bulk controller cannot + // quietly bypass the gate. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $company = Company::factory()->create(); + $actor = $company->users()->save(User::factory()->editAssets()->create()); + $target = Asset::factory()->create(['company_id' => $company->id]); + + $this->actingAs($actor) + ->post(route('hardware/bulksave'), [ + 'ids' => [$target->id => '1'], + 'company_id' => 'clear', + 'bulk_actions' => 'edit', + ]); + + $this->assertEquals($company->id, $target->fresh()->company_id, 'Row company should not have been cleared'); + } + + public function test_clearing_company_is_allowed_for_uncompanied_non_superuser() + { + // Uncompanied non-superusers work in the null pseudo-company + // namespace under strict mode. Bulk-clearing Company on rows + // they own is a legitimate operation for them and the gate + // steps aside. + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $actor = User::factory()->withoutCompany()->editAssets()->create(); + $target = Asset::factory()->create(['company_id' => null]); + + $this->actingAs($actor) + ->post(route('hardware/bulksave'), [ + 'ids' => [$target->id => '1'], + 'company_id' => 'clear', + 'bulk_actions' => 'edit', + ]); + + $this->assertNull($target->fresh()->company_id); + } + + public function test_editing_unrelated_field_still_works_for_companied_non_superuser() + { + // If the bulk edit doesn't touch Company at all, ValidatingTrait + // sees the existing non-null company_id on each row and passes. + // This is the "make sure we haven't broken ordinary bulk edits" + // sanity guard. + // + // Defensive Setting cache flush. Some upstream tests in the full + // MySQL sweep can leave the memoized Setting instance in a state + // where full_multiple_companies_support looks enabled at the + // moment auth loads but disabled by the time the fmcs_company + // validator reads it, or vice versa. The Support helper's update() + // clears the cache too, but only after both writes have landed. + // Clearing here first pins the pre-state so the two writes below + // are the only source of truth for this test's fmcs_company check. + Setting::$_cache = null; + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $company = Company::factory()->create(); + $actor = $company->users()->save(User::factory()->editAssets()->create()); + $target = Asset::factory()->create([ + 'company_id' => $company->id, + 'notes' => 'before', + ]); + + $response = $this->actingAs($actor) + ->post(route('hardware/bulksave'), [ + 'ids' => [$target->id => '1'], + 'notes' => 'after', + 'bulk_actions' => 'edit', + ]); + + // Fail loudly if the controller redirected back with a flash + // error (the notes assertion below is a downstream symptom and + // hides the real cause). Seen on MySQL CI as a full-suite flake. + // Pinning the response state up front turns any recurrence into + // an actionable diagnostic. + $response->assertSessionMissing('error'); + + $this->assertEquals('after', $target->fresh()->notes); + $this->assertEquals($company->id, $target->fresh()->company_id); + } +} diff --git a/tests/Feature/Fmcs/FmcsStrictUsersHttpTest.php b/tests/Feature/Fmcs/FmcsStrictUsersHttpTest.php new file mode 100644 index 0000000000..6b1d5a362c --- /dev/null +++ b/tests/Feature/Fmcs/FmcsStrictUsersHttpTest.php @@ -0,0 +1,62 @@ +settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $actor = User::factory()->create(); + $username = 'strict-null-target-'.uniqid(); + + $this->actingAs($actor) + ->post(route('users.store'), [ + 'first_name' => 'Test', + 'last_name' => 'User', + 'username' => $username, + 'email' => $username.'@example.com', + 'password' => 'SomeGreatPassword-123', + 'password_confirmation' => 'SomeGreatPassword-123', + // No company_ids submitted. + ]) + ->assertSessionHasErrors('company_ids'); + + $this->assertDatabaseMissing('users', ['username' => $username]); + } + + public function test_strict_fmcs_allows_empty_company_ids_for_superuser() + { + $this->settings->enableMultipleFullCompanySupport(); + $this->settings->disableFloaterMode(); + + $actor = User::factory()->superuser()->create(); + $username = 'super-null-'.uniqid(); + + $this->actingAs($actor) + ->post(route('users.store'), [ + 'first_name' => 'Superuser-Created', + 'last_name' => 'User', + 'username' => $username, + 'email' => $username.'@example.com', + 'password' => 'SomeGreatPassword-123', + 'password_confirmation' => 'SomeGreatPassword-123', + ]) + ->assertSessionHasNoErrors('company_ids'); + } +} diff --git a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php b/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php deleted file mode 100644 index f73cd20c00..0000000000 --- a/tests/Feature/Fmcs/StrictModeRequiresCompanyOnCreateTest.php +++ /dev/null @@ -1,321 +0,0 @@ -settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - auth()->login(User::factory()->create()); - - $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); - - $this->assertTrue($validator->fails()); - $this->assertArrayHasKey('company_id', $validator->errors()->toArray()); - } - - public function test_rule_accepts_null_in_strict_fmcs_for_superuser() - { - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - auth()->login(User::factory()->superuser()->create()); - - $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); - - $this->assertFalse($validator->fails()); - } - - public function test_rule_accepts_null_when_floater_mode_enabled() - { - $this->settings->enableFloaterMode(); - auth()->login(User::factory()->create()); - - $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); - - $this->assertFalse($validator->fails()); - } - - public function test_rule_accepts_null_when_fmcs_off() - { - $this->settings->disableMultipleFullCompanySupport(); - auth()->login(User::factory()->create()); - - $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); - - $this->assertFalse($validator->fails()); - } - - public function test_rule_accepts_non_null_in_strict_fmcs_for_non_superuser() - { - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - auth()->login(User::factory()->create()); - $company = Company::factory()->create(); - - $validator = Validator::make(['company_id' => $company->id], ['company_id' => 'fmcs_company']); - - $this->assertFalse($validator->fails()); - } - - public function test_rule_accepts_null_when_no_auth_context() - { - // CLI / seeders / importers deliberately bypass — same posture - // as the SaveUserRequest cannot_make_floater gate. - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - auth()->logout(); - - $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); - - $this->assertFalse($validator->fails()); - } - - public function test_rule_accepts_null_for_uncompanied_non_superuser_in_strict_mode() - { - // Regression guard for the pseudo-company workflow. Under - // Company::scopeCompanyablesDirectly in strict mode, actors - // with no company memberships are scoped to null-company rows - // (whereNull($column)). Null IS a valid company id for them — - // forcing them to pick a non-null company would both lock them - // out of their normal workflow AND produce a row they wouldn't - // be able to see afterward. - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - - $actor = User::factory()->withoutCompany()->create(); - $this->assertFalse($actor->companies()->exists(), 'test precondition: actor is uncompanied'); - auth()->login($actor); - - $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); - - $this->assertFalse($validator->fails()); - } - - public function test_rule_still_rejects_null_for_companied_non_superuser_in_strict_mode() - { - // Reporter's #19192 case: a non-superuser WITH company - // memberships submitting a null company_id would land an - // invisible row. That must still fail. - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - - $company = Company::factory()->create(); - $actor = $company->users()->save(User::factory()->create()); - $this->assertTrue($actor->companies()->exists(), 'test precondition: actor has memberships'); - auth()->login($actor); - - $validator = Validator::make(['company_id' => null], ['company_id' => 'fmcs_company']); - - $this->assertTrue($validator->fails()); - } - - // ------------------------------------------------------------------ - // Sanity: every model the reporter listed has the rule wired - // ------------------------------------------------------------------ - - /** - * @dataProvider companyableModelProvider - */ - public function test_model_rules_include_fmcs_company_for_company_id(string $modelClass) - { - $rules = $modelClass::rules(); - $this->assertArrayHasKey('company_id', $rules, $modelClass.' should declare a company_id rule'); - - $companyRule = $rules['company_id']; - $ruleString = is_array($companyRule) ? implode('|', $companyRule) : $companyRule; - - $this->assertStringContainsString( - 'fmcs_company', - $ruleString, - $modelClass.'::rules()[company_id] must include the fmcs_company validator so strict-FMCS mode rejects blank submissions', - ); - } - - public static function companyableModelProvider(): array - { - return [ - 'Asset' => [Asset::class], - 'License' => [License::class], - 'Accessory' => [Accessory::class], - 'Consumable' => [Consumable::class], - 'Component' => [Component::class], - 'Department' => [Department::class], - 'Location' => [Location::class], - ]; - } - - // ------------------------------------------------------------------ - // Users: gate lives in SaveUserRequest, not model $rules - // ------------------------------------------------------------------ - - public function test_users_strict_fmcs_rejects_empty_company_ids_for_non_superuser() - { - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - - $actor = User::factory()->create(); - $username = 'strict-null-target-'.uniqid(); - - $this->actingAs($actor) - ->post(route('users.store'), [ - 'first_name' => 'Test', - 'last_name' => 'User', - 'username' => $username, - 'email' => $username.'@example.com', - 'password' => 'SomeGreatPassword-123', - 'password_confirmation' => 'SomeGreatPassword-123', - // No company_ids submitted. - ]) - ->assertSessionHasErrors('company_ids'); - - $this->assertDatabaseMissing('users', ['username' => $username]); - } - - // ------------------------------------------------------------------ - // Bulk asset edit: same gate reaches through ValidatingTrait - // ------------------------------------------------------------------ - - public function test_bulk_asset_edit_clear_company_is_rejected_for_companied_non_superuser() - { - // BulkAssetsController::update() calls $asset->update($updateArray) - // per row; the model-level fmcs_company rule fires when - // company_id gets filled to null via the 'clear' bulk option. - // Locked in here so a future refactor of the bulk controller - // can't quietly bypass the gate. - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - - $company = \App\Models\Company::factory()->create(); - $actor = $company->users()->save(User::factory()->editAssets()->create()); - $target = \App\Models\Asset::factory()->create(['company_id' => $company->id]); - - $this->actingAs($actor) - ->post(route('hardware/bulksave'), [ - 'ids' => [$target->id => '1'], - 'company_id' => 'clear', - 'bulk_actions' => 'edit', - ]); - - // Row's company should NOT have been cleared. - $this->assertEquals($company->id, $target->fresh()->company_id); - } - - public function test_bulk_asset_edit_clear_company_is_allowed_for_uncompanied_non_superuser() - { - // Uncompanied non-superusers work in the null pseudo-company - // namespace under strict mode. Bulk-clearing Company on rows - // they own is a legitimate operation for them and the gate - // steps aside. - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - - $actor = User::factory()->withoutCompany()->editAssets()->create(); - $target = \App\Models\Asset::factory()->create(['company_id' => null]); - - $this->actingAs($actor) - ->post(route('hardware/bulksave'), [ - 'ids' => [$target->id => '1'], - 'company_id' => 'clear', - 'bulk_actions' => 'edit', - ]); - - $this->assertNull($target->fresh()->company_id); - } - - public function test_bulk_asset_edit_unrelated_field_still_works_for_companied_non_superuser() - { - // If the bulk edit doesn't touch Company at all, ValidatingTrait - // sees the existing non-null company_id on each row and passes. - // This is the "make sure we haven't broken ordinary bulk edits" - // sanity guard. - // - // Defensive Setting cache flush. Some upstream tests in the full - // MySQL sweep can leave the memoized Setting instance in a state - // where full_multiple_companies_support looks enabled at the - // moment auth loads but disabled by the time the fmcs_company - // validator reads it, or vice versa. The Support helper's update() - // clears the cache too, but only after both writes have landed. - // Clearing here first pins the pre-state so the two writes below - // are the only source of truth for this test's fmcs_company check. - \App\Models\Setting::$_cache = null; - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - - $company = \App\Models\Company::factory()->create(); - $actor = $company->users()->save(User::factory()->editAssets()->create()); - $target = \App\Models\Asset::factory()->create([ - 'company_id' => $company->id, - 'notes' => 'before', - ]); - - $response = $this->actingAs($actor) - ->post(route('hardware/bulksave'), [ - 'ids' => [$target->id => '1'], - 'notes' => 'after', - 'bulk_actions' => 'edit', - ]); - - // Fail loudly if the controller redirected back with a flash - // error (the notes assertion below is a downstream symptom and - // hides the real cause). Seen on MySQL CI as a full-suite flake - // — pinning the response state up front turns any recurrence - // into an actionable diagnostic. - $response->assertSessionMissing('error'); - - $this->assertEquals('after', $target->fresh()->notes); - $this->assertEquals($company->id, $target->fresh()->company_id); - } - - public function test_users_strict_fmcs_allows_empty_company_ids_for_superuser() - { - $this->settings->enableMultipleFullCompanySupport(); - $this->settings->disableFloaterMode(); - - $actor = User::factory()->superuser()->create(); - $username = 'super-null-'.uniqid(); - - $this->actingAs($actor) - ->post(route('users.store'), [ - 'first_name' => 'Superuser-Created', - 'last_name' => 'User', - 'username' => $username, - 'email' => $username.'@example.com', - 'password' => 'SomeGreatPassword-123', - 'password_confirmation' => 'SomeGreatPassword-123', - ]) - ->assertSessionHasNoErrors('company_ids'); - } -} From a05bbcc2a741b3beb1aa9b7c2728815408f1dd0e Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 Jul 2026 13:42:35 +0100 Subject: [PATCH 6/7] Derp, wrong key for debugging test --- .../Fmcs/FmcsStrictBulkAssetEditTest.php | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php b/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php index 4e691c27cc..b8e72bdaa2 100644 --- a/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php +++ b/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php @@ -97,12 +97,18 @@ class FmcsStrictBulkAssetEditTest extends TestCase 'bulk_actions' => 'edit', ]); - // Fail loudly if the controller redirected back with a flash - // error (the notes assertion below is a downstream symptom and - // hides the real cause). Seen on MySQL CI as a full-suite flake. - // Pinning the response state up front turns any recurrence into - // an actionable diagnostic. - $response->assertSessionMissing('error'); + // Fail loudly if the controller redirected with per-row errors + // (the notes assertion below is a downstream symptom that hides + // the actual validation failure). BulkAssetsController flashes + // `bulk_asset_errors` (not `error`) when $asset->update() returns + // false because ValidatingTrait rejected the save. Pulling those + // errors into the assertion message turns any recurrence of the + // MySQL CI full-suite flake into a concrete diagnostic. + $bulkErrors = session('bulk_asset_errors'); + $this->assertNull( + $bulkErrors, + 'Bulk asset edit produced per-row validation errors: '.json_encode($bulkErrors, JSON_UNESCAPED_SLASHES | JSON_PRETTY_PRINT), + ); $this->assertEquals('after', $target->fresh()->notes); $this->assertEquals($company->id, $target->fresh()->company_id); From 3e000962554e4dc8b5493d6e0e645c1bb100e034 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 Jul 2026 13:58:06 +0100 Subject: [PATCH 7/7] One more attempt at tests This was failing on GH but not locally - Locally + SQLite + LazilyRefreshDatabase: Asset::factory()->create() typically gets id=1 (SQLite handles auto-increment inside the rollback-per-test transaction differently). My assoc array [$target->id => '1'] happens to have value '1' which matches the id. Loop runs. Test passes. - MySQL CI full-suite: AUTO_INCREMENT doesn't reset on transaction rollback. After ~2900 tests, my Asset lands at id=2953 (or similar high number). WHERE id IN ('1') matches nothing. Loop skips my asset. Notes never gets updated. Test fails. --- tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php b/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php index b8e72bdaa2..ac3ae915de 100644 --- a/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php +++ b/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php @@ -34,7 +34,7 @@ class FmcsStrictBulkAssetEditTest extends TestCase $this->actingAs($actor) ->post(route('hardware/bulksave'), [ - 'ids' => [$target->id => '1'], + 'ids' => [$target->id], 'company_id' => 'clear', 'bulk_actions' => 'edit', ]); @@ -56,7 +56,7 @@ class FmcsStrictBulkAssetEditTest extends TestCase $this->actingAs($actor) ->post(route('hardware/bulksave'), [ - 'ids' => [$target->id => '1'], + 'ids' => [$target->id], 'company_id' => 'clear', 'bulk_actions' => 'edit', ]); @@ -92,7 +92,7 @@ class FmcsStrictBulkAssetEditTest extends TestCase $response = $this->actingAs($actor) ->post(route('hardware/bulksave'), [ - 'ids' => [$target->id => '1'], + 'ids' => [$target->id], 'notes' => 'after', 'bulk_actions' => 'edit', ]);