From 4e0bcac1a1c2cc24ced481de92703876062c5fbc Mon Sep 17 00:00:00 2001 From: Tobias Regnery Date: Fri, 24 Jan 2025 11:12:11 +0100 Subject: [PATCH] Furhter validation for scoped locations There is a new validator introduced that checks on object update (assets, users, etc.) if the company matches the locations company. In case of the creation of a new location it must be checked that the parent matches the own company. On updating a location a check for every related object must be made to see if the company matches the location. Signed-off-by: Tobias Regnery --- app/Console/Commands/TestLocationsFMCS.php | 10 ++++-- app/Helpers/Helper.php | 34 +++++++++++++++---- .../Controllers/Api/LocationsController.php | 8 +++++ app/Http/Controllers/LocationsController.php | 9 +++++ app/Models/Accessory.php | 1 + app/Models/Asset.php | 6 ++-- app/Models/Component.php | 1 + app/Models/Consumable.php | 1 + app/Models/User.php | 2 +- app/Providers/ValidationServiceProvider.php | 15 ++++++++ 10 files changed, 74 insertions(+), 13 deletions(-) diff --git a/app/Console/Commands/TestLocationsFMCS.php b/app/Console/Commands/TestLocationsFMCS.php index e7562e23c..fb606e6db 100644 --- a/app/Console/Commands/TestLocationsFMCS.php +++ b/app/Console/Commands/TestLocationsFMCS.php @@ -12,7 +12,7 @@ class TestLocationsFMCS extends Command * * @var string */ - protected $signature = 'snipeit:test-locations-fmcs'; + protected $signature = 'snipeit:test-locations-fmcs {--location_id=}'; /** * The console command description. @@ -28,7 +28,13 @@ class TestLocationsFMCS extends Command { $this->info('Test for inconsistencies if FullMultipleCompanySupport with scoped locations will be used'); $this->info('Depending on the database size this will take a while, output will be displayed after the complete test is over'); - $ret = Helper::test_locations_fmcs(true); + + // if parameter location_id is set, only test this location + $location_id = null; + if ($this->option('location_id')) { + $location_id = $this->option('location_id'); + } + $ret = Helper::test_locations_fmcs(true, $location_id); foreach($ret as $output) { $this->info($output); diff --git a/app/Helpers/Helper.php b/app/Helpers/Helper.php index dcfb5e995..14b8fcba5 100644 --- a/app/Helpers/Helper.php +++ b/app/Helpers/Helper.php @@ -1538,32 +1538,52 @@ class Helper * @author T. Regnery * @since 7.0 * - * @param $artisan when false, bail out on first inconsistent entry + * @param $artisan when false, bail out on first inconsistent entry + * @param $location_id when set, only test this specific location + * @param $new_company_id in case of updating a location, this is the newly requested company_id * @return string [] */ - static public function test_locations_fmcs($artisan) { + static public function test_locations_fmcs($artisan, $location_id = null, $new_company_id = null) { $ret = []; - $locations = Location::all(); + + if ($location_id) { + $location = Location::find($location_id); + if ($location) { + $locations = collect([])->push(Location::find($location_id)); + } + } else { + $locations = Location::all(); + } foreach($locations as $location) { - $location_company = $location->company_id; + // in case of an update of a single location use the newly requested company_id + if ($new_company_id) { + $location_company = $new_company_id; + } else { + $location_company = $location->company_id; + } // depending on the relationship we must use different operations to retrieve the objects $keywords_relation = ['many' => ['users', 'assets', 'rtd_assets', 'consumables', 'components', 'accessories', 'assignedAssets', 'assignedAccessories'], 'one' => ['parent', 'manager']]; + // In case of a single location the children must be checked either, becuase we don't walk every location + if ($location_id) { + $keywords_relation['many'][] = 'children'; + } + foreach ($keywords_relation as $relation => $keywords) { foreach($keywords as $keyword) { if ($relation == 'many') { $items = $location->$keyword->all(); } else { - $items = array($location->$keyword); + $items = collect([])->push($location->$keyword); } - + foreach ($items as $item) { if ($item && $item->company_id != $location_company) { $ret[] = 'type: ' . get_class($item) . ', id: ' . $item->id . ', company_id: ' . $item->company_id . ', location company_id: ' . $location_company; - // when called from SettingsController we bail out on the first error + // when not called from artisan command we bail out on the first error if (!$artisan) { return $ret; } diff --git a/app/Http/Controllers/Api/LocationsController.php b/app/Http/Controllers/Api/LocationsController.php index 40f1ebc3c..be96dc4c9 100644 --- a/app/Http/Controllers/Api/LocationsController.php +++ b/app/Http/Controllers/Api/LocationsController.php @@ -177,6 +177,10 @@ class LocationsController extends Controller // Only scope location if the setting is enabled if (Setting::getSettings()->scope_locations_fmcs) { $location->company_id = Company::getIdForCurrentUser($request->get('company_id')); + // check if parent is set and has a different company + if ($location->parent_id && Location::find($location->parent_id)->company_id != $location->company_id) { + response()->json(Helper::formatStandardApiResponse('error', null, 'different company than parent')); + } } if ($location->save()) { @@ -243,6 +247,10 @@ class LocationsController extends Controller // Only scope location if the setting is enabled if (Setting::getSettings()->scope_locations_fmcs) { $location->company_id = Company::getIdForCurrentUser($request->get('company_id')); + // check if there are related objects with different company + if (Helper::test_locations_fmcs(false, $id, $location->company_id)) { + return response()->json(Helper::formatStandardApiResponse('error', null, 'error scoped locations')); + } } else { $location->company_id = $request->get('company_id'); } diff --git a/app/Http/Controllers/LocationsController.php b/app/Http/Controllers/LocationsController.php index e21f00b50..c7a8476c4 100755 --- a/app/Http/Controllers/LocationsController.php +++ b/app/Http/Controllers/LocationsController.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers; +use App\Helpers\Helper; use App\Http\Requests\ImageUploadRequest; use App\Models\Actionlog; use App\Models\Asset; @@ -85,6 +86,10 @@ class LocationsController extends Controller // Only scope the location if the setting is enabled if (Setting::getSettings()->scope_locations_fmcs) { $location->company_id = Company::getIdForCurrentUser($request->input('company_id')); + // check if parent is set and has a different company + if ($location->parent_id && Location::find($location->parent_id)->company_id != $location->company_id) { + return redirect()->back()->withInput()->withInput()->with('error', 'different company than parent'); + } } else { $location->company_id = $request->input('company_id'); } @@ -152,6 +157,10 @@ class LocationsController extends Controller // Only scope the location if the setting is enabled if (Setting::getSettings()->scope_locations_fmcs) { $location->company_id = Company::getIdForCurrentUser($request->input('company_id')); + // check if there are related objects with different company + if (Helper::test_locations_fmcs(false, $locationId, $location->company_id)) { + return redirect()->back()->withInput()->withInput()->with('error', 'error scoped locations'); + } } else { $location->company_id = $request->input('company_id'); } diff --git a/app/Models/Accessory.php b/app/Models/Accessory.php index fc1bb36ab..039f8692f 100755 --- a/app/Models/Accessory.php +++ b/app/Models/Accessory.php @@ -61,6 +61,7 @@ class Accessory extends SnipeModel 'qty' => 'required|integer|min:1', 'category_id' => 'required|integer|exists:categories,id', 'company_id' => 'integer|nullable', + 'location_id' => 'exists:locations,id|nullable|fmcs_location', 'min_amt' => 'integer|min:0|nullable', 'purchase_cost' => 'numeric|nullable|gte:0|max:9999999999999', 'purchase_date' => 'date_format:Y-m-d|nullable', diff --git a/app/Models/Asset.php b/app/Models/Asset.php index ce8b870eb..2fe5b299d 100644 --- a/app/Models/Asset.php +++ b/app/Models/Asset.php @@ -108,8 +108,8 @@ class Asset extends Depreciable 'expected_checkin' => ['nullable', 'date'], 'last_audit_date' => ['nullable', 'date_format:Y-m-d H:i:s'], 'next_audit_date' => ['nullable', 'date'], - 'location_id' => ['nullable', 'exists:locations,id'], - 'rtd_location_id' => ['nullable', 'exists:locations,id'], + 'location_id' => ['nullable', 'exists:locations,id', 'fmcs_location'], + 'rtd_location_id' => ['nullable', 'exists:locations,id', 'fmcs_location'], 'purchase_date' => ['nullable', 'date', 'date_format:Y-m-d'], 'serial' => ['nullable', 'unique_undeleted:assets,serial'], 'purchase_cost' => ['nullable', 'numeric', 'gte:0', 'max:9999999999999'], @@ -122,7 +122,7 @@ class Asset extends Depreciable 'assigned_to' => ['nullable', 'integer'], 'requestable' => ['nullable', 'boolean'], 'assigned_user' => ['nullable', 'exists:users,id,deleted_at,NULL'], - 'assigned_location' => ['nullable', 'exists:locations,id,deleted_at,NULL'], + 'assigned_location' => ['nullable', 'exists:locations,id,deleted_at,NULL', 'fmcs_location'], 'assigned_asset' => ['nullable', 'exists:assets,id,deleted_at,NULL'] ]; diff --git a/app/Models/Component.php b/app/Models/Component.php index fb77bf082..7c5aa15d1 100644 --- a/app/Models/Component.php +++ b/app/Models/Component.php @@ -35,6 +35,7 @@ class Component extends SnipeModel 'category_id' => 'required|integer|exists:categories,id', 'supplier_id' => 'nullable|integer|exists:suppliers,id', 'company_id' => 'integer|nullable|exists:companies,id', + 'location_id' => 'exists:locations,id|nullable|fmcs_location', 'min_amt' => 'integer|min:0|nullable', 'purchase_date' => 'date_format:Y-m-d|nullable', 'purchase_cost' => 'numeric|nullable|gte:0|max:9999999999999', diff --git a/app/Models/Consumable.php b/app/Models/Consumable.php index 30161e842..45dd67e1c 100644 --- a/app/Models/Consumable.php +++ b/app/Models/Consumable.php @@ -49,6 +49,7 @@ class Consumable extends SnipeModel 'qty' => 'required|integer|min:0|max:99999', 'category_id' => 'required|integer', 'company_id' => 'integer|nullable', + 'location_id' => 'exists:locations,id|nullable|fmcs_location', 'min_amt' => 'integer|min:0|max:99999|nullable', 'purchase_cost' => 'numeric|nullable|gte:0|max:9999999999999', 'purchase_date' => 'date_format:Y-m-d|nullable', diff --git a/app/Models/User.php b/app/Models/User.php index e48b8bf07..029bda046 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -94,7 +94,7 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo 'locale' => 'max:10|nullable', 'website' => 'url|nullable|max:191', 'manager_id' => 'nullable|exists:users,id|cant_manage_self', - 'location_id' => 'exists:locations,id|nullable', + 'location_id' => 'exists:locations,id|nullable|fmcs_location', 'start_date' => 'nullable|date_format:Y-m-d', 'end_date' => 'nullable|date_format:Y-m-d|after_or_equal:start_date', 'autoassign_licenses' => 'boolean', diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index 76ba1b629..a08a035fb 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -4,6 +4,7 @@ namespace App\Providers; use App\Models\CustomField; use App\Models\Department; +use App\Models\Location; use App\Models\Setting; use Illuminate\Support\Facades\DB; use Illuminate\Support\ServiceProvider; @@ -353,6 +354,20 @@ class ValidationServiceProvider extends ServiceProvider return in_array($value, $options); }); + + // 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(); + if ($settings->full_multiple_companies_support == '1' && $settings->scope_locations_fmcs == '1') { + $company_id = array_get($validator->getData(), 'company_id'); + $location = Location::find($value); + + if ($company_id != $location->company_id) { + return false; + } + } + return true; + }); } /**