diff --git a/app/Helpers/Helper.php b/app/Helpers/Helper.php index 45d1b392f8..b8ed973d60 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,35 @@ 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; + } + $actor = auth()->user(); + if ($actor->isSuperUser()) { + return false; + } + + // 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(); + } + /** * 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/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 3bda0b5a00..b42a4b2d1d 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,28 @@ 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(); + $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. 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 (! $actor?->canGrantFloaterStatus()) { + $validator->errors()->add('company_ids', trans('admin/users/general.cannot_make_floater')); + } } }); } 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/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..ba699ab1ca 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -452,6 +452,60 @@ 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 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, 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, 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) 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) { + $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; + } + $actor = auth()->user(); + if ($actor->isSuperUser()) { + return true; + } + if (! $actor->companies()->exists()) { + return true; + } + + return false; + }); + + Validator::replacer('fmcs_company', function ($message) { + 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/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..ac3ae915de --- /dev/null +++ b/tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php @@ -0,0 +1,116 @@ +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], + '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], + '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], + 'notes' => 'after', + 'bulk_actions' => 'edit', + ]); + + // 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); + } +} 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/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php index b981608523..6d4cec721a 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,13 +371,17 @@ 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 + // 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() diff --git a/tests/Feature/Users/Ui/FloaterModeGateTest.php b/tests/Feature/Users/Ui/FloaterModeGateTest.php index 0c5da490a6..cf6c0d3d39 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() @@ -109,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