mirror of
https://github.com/snipe/snipe-it.git
synced 2026-08-18 03:06:23 +00:00
Merge pull request #19341 from grokability/fmcs-require-company-if-floater-is-off
FMCS+Floater: Fixed #19192 - make company required if floater is disabled
This commit is contained in:
@ -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,
|
||||
|
||||
@ -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'));
|
||||
|
||||
@ -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'));
|
||||
}
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
@ -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,
|
||||
|
||||
@ -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',
|
||||
|
||||
@ -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'],
|
||||
|
||||
@ -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',
|
||||
|
||||
@ -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',
|
||||
|
||||
@ -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',
|
||||
|
||||
@ -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',
|
||||
|
||||
@ -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 = [
|
||||
|
||||
@ -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();
|
||||
|
||||
@ -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.',
|
||||
|
||||
|
||||
54
tests/Feature/Fmcs/FmcsCompanyRuleWiringTest.php
Normal file
54
tests/Feature/Fmcs/FmcsCompanyRuleWiringTest.php
Normal file
@ -0,0 +1,54 @@
|
||||
<?php
|
||||
|
||||
namespace Tests\Feature\Fmcs;
|
||||
|
||||
use App\Models\Accessory;
|
||||
use App\Models\Asset;
|
||||
use App\Models\Component;
|
||||
use App\Models\Consumable;
|
||||
use App\Models\Department;
|
||||
use App\Models\License;
|
||||
use App\Models\Location;
|
||||
use PHPUnit\Framework\Attributes\DataProvider;
|
||||
use Tests\TestCase;
|
||||
|
||||
/**
|
||||
* Regression coverage for GitHub issue #19192, part 2 of 4.
|
||||
*
|
||||
* Sanity check that every Companyable model the reporter listed carries
|
||||
* the fmcs_company validator on its $rules['company_id'] entry. The rule
|
||||
* itself is exercised in FmcsCompanyValidatorTest. This file just guards
|
||||
* against a future refactor accidentally dropping the rule off one of
|
||||
* the model rule arrays and quietly re-opening the #19192 hole.
|
||||
*/
|
||||
class FmcsCompanyRuleWiringTest extends TestCase
|
||||
{
|
||||
#[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],
|
||||
];
|
||||
}
|
||||
}
|
||||
132
tests/Feature/Fmcs/FmcsCompanyValidatorTest.php
Normal file
132
tests/Feature/Fmcs/FmcsCompanyValidatorTest.php
Normal file
@ -0,0 +1,132 @@
|
||||
<?php
|
||||
|
||||
namespace Tests\Feature\Fmcs;
|
||||
|
||||
use App\Models\Company;
|
||||
use App\Models\User;
|
||||
use Illuminate\Support\Facades\Validator;
|
||||
use Tests\TestCase;
|
||||
|
||||
/**
|
||||
* Regression coverage for GitHub issue #19192, part 1 of 4.
|
||||
*
|
||||
* Exercises the fmcs_company validator in isolation across the settings
|
||||
* and actor context matrix. Per Snipe's FMCS adversarial-tests memory,
|
||||
* cover strict and floater and non-FMCS with superuser and non-superuser
|
||||
* actors, plus the CLI (no-auth) posture and the pseudo-company carve-out
|
||||
* for uncompanied actors in strict mode.
|
||||
*
|
||||
* The rule is exercised on its own (not against each model's full $rules)
|
||||
* so unrelated pseudo-rules like License::limit_change don't crash the
|
||||
* runner. A companion sanity file, FmcsCompanyRuleWiringTest, asserts
|
||||
* every affected model's $rules array carries the rule.
|
||||
*/
|
||||
class FmcsCompanyValidatorTest extends TestCase
|
||||
{
|
||||
public function test_rule_rejects_null_in_strict_fmcs_for_non_superuser()
|
||||
{
|
||||
$this->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());
|
||||
}
|
||||
}
|
||||
116
tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php
Normal file
116
tests/Feature/Fmcs/FmcsStrictBulkAssetEditTest.php
Normal file
@ -0,0 +1,116 @@
|
||||
<?php
|
||||
|
||||
namespace Tests\Feature\Fmcs;
|
||||
|
||||
use App\Models\Asset;
|
||||
use App\Models\Company;
|
||||
use App\Models\Setting;
|
||||
use App\Models\User;
|
||||
use Tests\TestCase;
|
||||
|
||||
/**
|
||||
* Regression coverage for GitHub issue #19192, part 4 of 4.
|
||||
*
|
||||
* BulkAssetsController::update() calls $asset->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);
|
||||
}
|
||||
}
|
||||
62
tests/Feature/Fmcs/FmcsStrictUsersHttpTest.php
Normal file
62
tests/Feature/Fmcs/FmcsStrictUsersHttpTest.php
Normal file
@ -0,0 +1,62 @@
|
||||
<?php
|
||||
|
||||
namespace Tests\Feature\Fmcs;
|
||||
|
||||
use App\Models\User;
|
||||
use Tests\TestCase;
|
||||
|
||||
/**
|
||||
* Regression coverage for GitHub issue #19192, part 3 of 4.
|
||||
*
|
||||
* The User gate lives in SaveUserRequest, not on the model $rules
|
||||
* array (Users don't run through ValidatingTrait the same way the other
|
||||
* Companyable models do because pivot memberships live on company_user).
|
||||
* Exercise the HTTP endpoints directly to lock in that strict FMCS
|
||||
* rejects a blank company_ids for a non-superuser and accepts it for a
|
||||
* superuser.
|
||||
*/
|
||||
class FmcsStrictUsersHttpTest extends TestCase
|
||||
{
|
||||
public function test_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_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');
|
||||
}
|
||||
}
|
||||
@ -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()
|
||||
|
||||
@ -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
|
||||
|
||||
Reference in New Issue
Block a user