From 6d65f6646f43d9ceb2f84ed7df0a00e707657841 Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Tue, 14 Nov 2023 14:55:51 -0800 Subject: [PATCH 01/32] allows validation to ignore self and update --- app/Providers/ValidationServiceProvider.php | 24 ++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index 70fa64702e..fffb571eee 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -218,19 +218,37 @@ class ValidationServiceProvider extends ServiceProvider Validator::extend('is_unique_department', function ($attribute, $value, $parameters, $validator) { $data = $validator->getData(); - if ((array_key_exists('location_id', $data) && $data['location_id'] != null) && (array_key_exists('company_id', $data) && $data['company_id'] != null)) { + + if ( + array_key_exists('location_id', $data) && $data['location_id'] !== null && + array_key_exists('company_id', $data) && $data['company_id'] !== null + ) { + //for updating existing departments + if(array_key_exists('id', $data) && $data['id'] !== null){ + $count = Department::where('name', $data['name']) + ->where('location_id', $data['location_id']) + ->where('company_id', $data['company_id']) + ->whereNotNull('company_id') + ->whereNotNull('location_id') + ->where('id', '!=', $data['id']) + ->count(); + + return $count < 1; + }else // for entering in new departments + { $count = Department::where('name', $data['name']) ->where('location_id', $data['location_id']) ->where('company_id', $data['company_id']) ->whereNotNull('company_id') ->whereNotNull('location_id') - ->count('name'); + ->count(); return $count < 1; } + } else { return true; - } + } }); Validator::extend('not_array', function ($attribute, $value, $parameters, $validator) { From c9d46856a32047dec91e56eb9398ebd841ed9715 Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Tue, 14 Nov 2023 15:00:11 -0800 Subject: [PATCH 02/32] added name back --- app/Providers/ValidationServiceProvider.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index fffb571eee..7a8465f0d1 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -231,7 +231,7 @@ class ValidationServiceProvider extends ServiceProvider ->whereNotNull('company_id') ->whereNotNull('location_id') ->where('id', '!=', $data['id']) - ->count(); + ->count('name'); return $count < 1; }else // for entering in new departments @@ -241,7 +241,7 @@ class ValidationServiceProvider extends ServiceProvider ->where('company_id', $data['company_id']) ->whereNotNull('company_id') ->whereNotNull('location_id') - ->count(); + ->count('name'); return $count < 1; } From 1b8c7ff4ece71905b830e39825bea11db8ca0cf8 Mon Sep 17 00:00:00 2001 From: Jeff Clay Date: Wed, 27 Mar 2024 11:38:13 -0500 Subject: [PATCH 03/32] Added PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT options to database.php and updated .env files to provide value for the new option. --- .env.docker | 1 + .env.dusk.example | 1 + .env.example | 1 + config/database.php | 1 + 4 files changed, 4 insertions(+) diff --git a/.env.docker b/.env.docker index 87897b10db..7b9e2000cf 100644 --- a/.env.docker +++ b/.env.docker @@ -45,6 +45,7 @@ DB_SSL_KEY_PATH=null DB_SSL_CERT_PATH=null DB_SSL_CA_PATH=null DB_SSL_CIPHER=null +DB_SSL_VERIFY_SERVER=null # -------------------------------------------- # REQUIRED: OUTGOING MAIL SERVER SETTINGS diff --git a/.env.dusk.example b/.env.dusk.example index 074f6fc3d7..3a9765dc19 100644 --- a/.env.dusk.example +++ b/.env.dusk.example @@ -36,6 +36,7 @@ DB_SSL_KEY_PATH=null DB_SSL_CERT_PATH=null DB_SSL_CA_PATH=null DB_SSL_CIPHER=null +DB_SSL_VERIFY_SERVER=null # -------------------------------------------- # REQUIRED: OUTGOING MAIL SERVER SETTINGS diff --git a/.env.example b/.env.example index fd90391973..8f3e5a2d69 100644 --- a/.env.example +++ b/.env.example @@ -42,6 +42,7 @@ DB_SSL_KEY_PATH=null DB_SSL_CERT_PATH=null DB_SSL_CA_PATH=null DB_SSL_CIPHER=null +DB_SSL_VERIFY_SERVER=null # -------------------------------------------- # REQUIRED: OUTGOING MAIL SERVER SETTINGS diff --git a/config/database.php b/config/database.php index 1b4feeca93..de3c40bb25 100755 --- a/config/database.php +++ b/config/database.php @@ -96,6 +96,7 @@ return [ PDO::MYSQL_ATTR_SSL_CERT => env('DB_SSL_CERT_PATH'), // /path/to/cert.pem PDO::MYSQL_ATTR_SSL_CA => env('DB_SSL_CA_PATH'), // /path/to/ca.pem PDO::MYSQL_ATTR_SSL_CIPHER => env('DB_SSL_CIPHER'), + PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT => env('DB_SSL_VERIFY_SERVER'), //true/false ]) : [], ], From adacdc038da56e2d50a941798f8b0a198109da78 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 10 Apr 2024 12:34:32 +0100 Subject: [PATCH 04/32] Apply company scoping for users Signed-off-by: snipe --- .../Controllers/Users/UsersController.php | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 2655e50f75..937cba551a 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -182,7 +182,8 @@ class UsersController extends Controller */ public function edit($id) { - if ($user = User::find($id)) { + + if ($user = Company::scopeCompanyables(User::find($id))) { $this->authorize('update', $user); $permissions = config('permissions'); $groups = Group::pluck('name', 'id'); @@ -427,16 +428,16 @@ class UsersController extends Controller */ public function show($userId = null) { - if (! $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($userId)) { + + if (! $user = Company::scopeCompanyables(User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($userId))) { // Redirect to the user management page return redirect()->route('users.index') ->with('error', trans('admin/users/message.user_not_found', ['id' => $userId])); } + $this->authorize('view', $user); $userlog = $user->userlog->load('item'); - $this->authorize('view', $user); - return view('users/view', compact('user', 'userlog')) ->with('settings', Setting::getSettings()); } @@ -454,7 +455,7 @@ class UsersController extends Controller { try { // Get user information - $user = User::findOrFail($id); + $user = Company::scopeCompanyables(User::findOrFail($id)); $this->authorize('update', $user); // Check if we are not trying to unsuspend ourselves @@ -500,7 +501,7 @@ class UsersController extends Controller try { // Get the user information - $user_to_clone = User::withTrashed()->find($id); + $user_to_clone = Company::scopeCompanyables(User::withTrashed()->find($id)); $user = clone $user_to_clone; $user->first_name = ''; $user->last_name = ''; @@ -546,7 +547,7 @@ class UsersController extends Controller // Open output stream $handle = fopen('php://output', 'w'); - User::with('assets', 'accessories', 'consumables', 'department', 'licenses', 'manager', 'groups', 'userloc', 'company') + Company::scopeCompanyables(User::with('assets', 'accessories', 'consumables', 'department', 'licenses', 'manager', 'groups', 'userloc', 'company') ->orderBy('created_at', 'DESC') ->chunk(500, function ($users) use ($handle) { $headers = [ @@ -565,7 +566,7 @@ class UsersController extends Controller trans('general.licenses'), trans('general.accessories'), trans('general.consumables'), - trans('admin/users/table.groups'), + trans('general.groups'), trans('general.notes'), trans('admin/users/table.activated'), trans('general.created_at'), @@ -604,7 +605,7 @@ class UsersController extends Controller fputcsv($handle, $values); } - }); + })); // Close the output stream fclose($handle); @@ -626,7 +627,7 @@ class UsersController extends Controller public function printInventory($id) { $this->authorize('view', User::class); - $show_user = User::where('id', $id)->withTrashed()->first(); + $show_user = Company::scopeCompanyables(User::where('id', $id)->withTrashed()->first()); $assets = Asset::where('assigned_to', $id)->where('assigned_type', User::class)->with('model', 'model.category')->get(); $accessories = $show_user->accessories()->get(); $consumables = $show_user->consumables()->get(); @@ -651,7 +652,7 @@ class UsersController extends Controller { $this->authorize('view', User::class); - if (!$user = User::find($id)) { + if (!$user = Company::scopeCompanyables(User::find($id))) { return redirect()->back() ->with('error', trans('admin/users/message.user_not_found', ['id' => $id])); } @@ -672,7 +673,7 @@ class UsersController extends Controller */ public function sendPasswordReset($id) { - if (($user = User::find($id)) && ($user->activated == '1') && ($user->email != '') && ($user->ldap_import == '0')) { + if (($user = Company::scopeCompanyables(User::find($id))) && ($user->activated == '1') && ($user->email != '') && ($user->ldap_import == '0')) { $credentials = ['email' => trim($user->email)]; try { From 989dab6259d9e8a51b1c663b8c44568f3f6687dd Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 10 Apr 2024 14:35:13 +0100 Subject: [PATCH 05/32] Moved company scoping methods to group them together Signed-off-by: snipe --- app/Models/Company.php | 152 +++++++++++++++++++++++++++-------------- 1 file changed, 100 insertions(+), 52 deletions(-) diff --git a/app/Models/Company.php b/app/Models/Company.php index 60a8022ed7..76cbda10ed 100644 --- a/app/Models/Company.php +++ b/app/Models/Company.php @@ -81,26 +81,6 @@ final class Company extends SnipeModel } } - /** - * Scoping table queries, determining if a logged in user is part of a company, and only allows - * that user to see items associated with that company - */ - private static function scopeCompanyablesDirectly($query, $column = 'company_id', $table_name = null) - { - if (Auth::user()) { - $company_id = Auth::user()->company_id; - } else { - $company_id = null; - } - - $table = ($table_name) ? $table_name."." : $query->getModel()->getTable()."."; - - if (\Schema::hasColumn($query->getModel()->getTable(), $column)) { - return $query->where($table.$column, '=', $company_id); - } else { - return $query->join('users as users_comp', 'users_comp.id', 'user_id')->where('users_comp.company_id', '=', $company_id); - } - } public static function getIdFromInput($unescaped_input) { @@ -190,6 +170,10 @@ final class Company extends SnipeModel && ($this->users()->count() === 0); } + /** + * @param $unescaped_input + * @return int|mixed|string|null + */ public static function getIdForUser($unescaped_input) { if (! static::isFullMultipleCompanySupportEnabled() || Auth::user()->isSuperUser()) { @@ -199,38 +183,6 @@ final class Company extends SnipeModel } } - public static function scopeCompanyables($query, $column = 'company_id', $table_name = null) - { - // If not logged in and hitting this, assume we are on the command line and don't scope?' - if (! static::isFullMultipleCompanySupportEnabled() || (Auth::check() && Auth::user()->isSuperUser()) || (! Auth::check())) { - return $query; - } else { - return static::scopeCompanyablesDirectly($query, $column, $table_name); - } - } - - public static function scopeCompanyableChildren(array $companyable_names, $query) - { - if (count($companyable_names) == 0) { - throw new Exception('No Companyable Children to scope'); - } elseif (! static::isFullMultipleCompanySupportEnabled() || (Auth::check() && Auth::user()->isSuperUser())) { - return $query; - } else { - $f = function ($q) { - static::scopeCompanyablesDirectly($q); - }; - - $q = $query->where(function ($q) use ($companyable_names, $f) { - $q2 = $q->whereHas($companyable_names[0], $f); - - for ($i = 1; $i < count($companyable_names); $i++) { - $q2 = $q2->orWhereHas($companyable_names[$i], $f); - } - }); - - return $q; - } - } public function users() { @@ -261,4 +213,100 @@ final class Company extends SnipeModel { return $this->hasMany(Component::class, 'company_id'); } + + /** + * START COMPANY SCOPING FOR FMCS + */ + + /** + * Scoping table queries, determining if a logged in user is part of a company, and only allows the user to access items associated with that company if FMCS is enabled. + * + * This method is the one that the CompanyableTrait uses to contrain queries automatically, however that trait CANNOT be + * applied to the user's model, since it causes an infinite loop against the authenticated user. + * + * @todo - refactor that trait to handle the user's model as well. + * + * @author [A. Gianotto] + * @param $query + * @param $column + * @param $table_name + * @return mixed + */ + public static function scopeCompanyables($query, $column = 'company_id', $table_name = null) + { + // If not logged in and hitting this, assume we are on the command line and don't scope?' + if (! static::isFullMultipleCompanySupportEnabled() || (Auth::check() && Auth::user()->isSuperUser()) || (! Auth::check())) { + \Log::debug('Skip scoping in scopeCompanyableChildren. User is not logged in or is a superadmin'); + return $query; + } else { + \Log::debug('Fire scopeCompanyablesDirectly.'); + return static::scopeCompanyablesDirectly($query, $column, $table_name); + } + } + + /** + * Scoping table queries, determining if a logged in user is part of a company, and only allows + * that user to see items associated with that company + */ + private static function scopeCompanyablesDirectly($query, $column = 'company_id', $table_name = null) + { + // Get the company ID of the logged in user, or set it to null if there is no company assicoated with the user + if (Auth::user()) { + \Log::debug('Admin company is: '.Auth::user()->company_id); + $company_id = Auth::user()->company_id; + } else { + $company_id = null; + } + + // Dynamically get the table name if it's not passed in, based on the model we're querying against + $table = ($table_name) ? $table_name."." : $query->getModel()->getTable()."."; + \Log::debug('Model is: '.$query->getModel()); + + \Log::debug('Table is: '.$table); + + // If the column exists in the table, use it to scope the query + if (\Schema::hasColumn($query->getModel()->getTable(), $column)) { + return $query->where($table.$column, '=', $company_id); + } else { + return $query->join('users as users_comp', 'users_comp.id', 'user_id')->where('users_comp.company_id', '=', $company_id); + } + } + + /** + * I legit do not know what this method does, but we can't remove it (yet). + * + * This gets invoked by CompanyableChildScope, but I'm not sure what it does. + * + * @author [A. Gianotto] + * @param array $companyable_names + * @param $query + * @return mixed + */ + public static function scopeCompanyableChildren(array $companyable_names, $query) + { + \Log::debug('Company Names in scopeCompanyableChildren: '.print_r($companyable_names, true)); + + if (count($companyable_names) == 0) { + throw new Exception('No Companyable Children to scope'); + } elseif (! static::isFullMultipleCompanySupportEnabled() || (Auth::check() && Auth::user()->isSuperUser())) { + \Log::debug('Skip scoping in scopeCompanyableChildren. User is not logged in or is a superadmin'); + return $query; + } else { + $f = function ($q) { + \Log::debug('scopeCompanyablesDirectly firing '); + static::scopeCompanyablesDirectly($q); + }; + + $q = $query->where(function ($q) use ($companyable_names, $f) { + $q2 = $q->whereHas($companyable_names[0], $f); + + for ($i = 1; $i < count($companyable_names); $i++) { + $q2 = $q2->orWhereHas($companyable_names[$i], $f); + } + }); + + return $q; + } + } + } From 5829b0232368fa6fd12bb841cbfac383e0e2b84c Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 14:38:47 +0100 Subject: [PATCH 06/32] Added translation Signed-off-by: snipe --- resources/lang/en-US/general.php | 1 + 1 file changed, 1 insertion(+) diff --git a/resources/lang/en-US/general.php b/resources/lang/en-US/general.php index 9f9a0e08c7..a3b391f76e 100644 --- a/resources/lang/en-US/general.php +++ b/resources/lang/en-US/general.php @@ -507,6 +507,7 @@ return [ 'or' => 'or', 'url' => 'URL', 'edit_fieldset' => 'Edit fieldset fields and options', + 'permission_denied_superuser_demo' => 'Permission denied. You cannot update user information for superadmins on the demo.', 'bulk' => [ 'delete' => [ From 570944a48b0585eb39e88671e475ae2a7cc911cc Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 14:38:52 +0100 Subject: [PATCH 07/32] Updated translation Signed-off-by: snipe --- resources/lang/en-US/admin/licenses/message.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/lang/en-US/admin/licenses/message.php b/resources/lang/en-US/admin/licenses/message.php index c79f631680..27fbfe38a9 100644 --- a/resources/lang/en-US/admin/licenses/message.php +++ b/resources/lang/en-US/admin/licenses/message.php @@ -3,7 +3,7 @@ return array( 'does_not_exist' => 'License does not exist or you do not have permission to view it.', - 'user_does_not_exist' => 'User does not exist.', + 'user_does_not_exist' => 'User does not exist or you do not have permission to view them.', 'asset_does_not_exist' => 'The asset you are trying to associate with this license does not exist.', 'owner_doesnt_match_asset' => 'The asset you are trying to associate with this license is owned by somene other than the person selected in the assigned to dropdown.', 'assoc_users' => 'This license is currently checked out to a user and cannot be deleted. Please check the license in first, and then try deleting again. ', From a19b86add0cc44d0ebb8d86f07400b9f8f0e70aa Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 14:39:37 +0100 Subject: [PATCH 08/32] Re-ordered scoping for admins, added comments Signed-off-by: snipe --- app/Policies/SnipePermissionsPolicy.php | 34 +++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/app/Policies/SnipePermissionsPolicy.php b/app/Policies/SnipePermissionsPolicy.php index d4f2d88ccd..3f385f2d49 100644 --- a/app/Policies/SnipePermissionsPolicy.php +++ b/app/Policies/SnipePermissionsPolicy.php @@ -35,16 +35,40 @@ abstract class SnipePermissionsPolicy public function before(User $user, $ability, $item) { - // Lets move all company related checks here. - if ($item instanceof \App\Models\SnipeModel && ! Company::isCurrentUserHasAccess($item)) { - return false; - } - // If an admin, they can do all asset related tasks. + /** + * If an admin, they can do all asset related tasks, but constrained by FMCSA company access. + * That scoping happens on the model level (except for the Users model) via the Companyable trait. + * + * This does lead to some inconsistencies in the responses, since attempting to edit assets, + * accessories, etc (anything other than users) will result in a Forbidden error, whereas the users + * area will redirect with "That user doesn't exist" since the scoping is handled directly on those queries. + * + * The *superuser* global permission gets handled in the AuthServiceProvider before() method. + * + * @see https://snipe-it.readme.io/docs/permissions + */ + if ($user->hasAccess('admin')) { return true; } + + /** + * The Company::isCurrentUserHasAccess() method from the company model handles the check for FMCS already so we + * don't have to do that here. + */ + if (!Company::isCurrentUserHasAccess($item)) { + return false; + } + } + + /** + * These methods handle the generic view/create/edit/delete permissions for the model. + * + * @param User $user + * @return bool + */ public function index(User $user) { return $user->hasAccess($this->columnName().'.view'); From f54a94bd4cf097d92c1ac8eb16ef36f7b073f9c5 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 14:40:00 +0100 Subject: [PATCH 09/32] Refactorered methods Signed-off-by: snipe --- .../Controllers/Users/UsersController.php | 221 ++++++++++-------- 1 file changed, 124 insertions(+), 97 deletions(-) diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 937cba551a..9239180ff3 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -183,8 +183,12 @@ class UsersController extends Controller public function edit($id) { - if ($user = Company::scopeCompanyables(User::find($id))) { - $this->authorize('update', $user); + $this->authorize('update', User::class); + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); + $user = Company::scopeCompanyables($user)->find($id); + + if ($user) { + $permissions = config('permissions'); $groups = Group::pluck('name', 'id'); @@ -211,106 +215,109 @@ class UsersController extends Controller */ public function update(SaveUserRequest $request, $id = null) { - // We need to reverse the UI specific logic for our - // permissions here before we update the user. - $permissions = $request->input('permissions', []); - app('request')->request->set('permissions', $permissions); + $this->authorize('update', User::class); // This is a janky hack to prevent people from changing admin demo user data on the public demo. // The $ids 1 and 2 are special since they are seeded as superadmins in the demo seeder. // Thanks, jerks. You are why we can't have nice things. - snipe if ((($id == 1) || ($id == 2)) && (config('app.lock_passwords'))) { - return redirect()->route('users.index')->with('error', 'Permission denied. You cannot update user information for superadmins on the demo.'); + return redirect()->route('users.index')->with('error', trans('general.permission_denied_superuser_demo')); } - try { - $user = User::findOrFail($id); - } catch (ModelNotFoundException $e) { - return redirect()->route('users.index') - ->with('error', trans('admin/users/message.user_not_found', compact('id'))); - } - $this->authorize('update', $user); - // Figure out of this user was an admin before this edit - $orig_permissions_array = $user->decodePermissions(); - $orig_superuser = '0'; - if (is_array($orig_permissions_array)) { - if (array_key_exists('superuser', $orig_permissions_array)) { - $orig_superuser = $orig_permissions_array['superuser']; + // We need to reverse the UI specific logic for our + // permissions here before we update the user. + $permissions = $request->input('permissions', []); + app('request')->request->set('permissions', $permissions); + + + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); + $user = Company::scopeCompanyables($user)->find($id); + + // User is valid - continue... + if ($user) { + $this->authorize('update', $user); + + // Figure out of this user was an admin before this edit + $orig_permissions_array = $user->decodePermissions(); + $orig_superuser = '0'; + if (is_array($orig_permissions_array)) { + if (array_key_exists('superuser', $orig_permissions_array)) { + $orig_superuser = $orig_permissions_array['superuser']; + } } - } - // Only save groups if the user is a super user - if (Auth::user()->isSuperUser()) { - $user->groups()->sync($request->input('groups')); - } + // Only save groups if the user is a superuser + if (Auth::user()->isSuperUser()) { + $user->groups()->sync($request->input('groups')); + } - // Update the user - if ($request->filled('username')) { + // Update the user fields $user->username = trim($request->input('username')); - } - $user->email = trim($request->input('email')); - $user->first_name = $request->input('first_name'); - $user->last_name = $request->input('last_name'); - $user->two_factor_optin = $request->input('two_factor_optin') ?: 0; - $user->locale = $request->input('locale'); - $user->employee_num = $request->input('employee_num'); - $user->activated = $request->input('activated', 0); - $user->jobtitle = $request->input('jobtitle', null); - $user->phone = $request->input('phone'); - $user->location_id = $request->input('location_id', null); - $user->company_id = Company::getIdForUser($request->input('company_id', null)); - $user->manager_id = $request->input('manager_id', null); - $user->notes = $request->input('notes'); - $user->department_id = $request->input('department_id', null); - $user->address = $request->input('address', null); - $user->city = $request->input('city', null); - $user->state = $request->input('state', null); - $user->country = $request->input('country', null); - // if a user is editing themselves we should always keep activated true - $user->activated = $request->input('activated', $request->user()->is($user) ? 1 : 0); - $user->zip = $request->input('zip', null); - $user->remote = $request->input('remote', 0); - $user->vip = $request->input('vip', 0); - $user->website = $request->input('website', null); - $user->start_date = $request->input('start_date', null); - $user->end_date = $request->input('end_date', null); - $user->autoassign_licenses = $request->input('autoassign_licenses', 0); + $user->email = trim($request->input('email')); + $user->first_name = $request->input('first_name'); + $user->last_name = $request->input('last_name'); + $user->two_factor_optin = $request->input('two_factor_optin') ?: 0; + $user->locale = $request->input('locale'); + $user->employee_num = $request->input('employee_num'); + $user->activated = $request->input('activated', 0); + $user->jobtitle = $request->input('jobtitle', null); + $user->phone = $request->input('phone'); + $user->location_id = $request->input('location_id', null); + $user->company_id = Company::getIdForUser($request->input('company_id', null)); + $user->manager_id = $request->input('manager_id', null); + $user->notes = $request->input('notes'); + $user->department_id = $request->input('department_id', null); + $user->address = $request->input('address', null); + $user->city = $request->input('city', null); + $user->state = $request->input('state', null); + $user->country = $request->input('country', null); + // if a user is editing themselves we should always keep activated true + $user->activated = $request->input('activated', $request->user()->is($user) ? 1 : 0); + $user->zip = $request->input('zip', null); + $user->remote = $request->input('remote', 0); + $user->vip = $request->input('vip', 0); + $user->website = $request->input('website', null); + $user->start_date = $request->input('start_date', null); + $user->end_date = $request->input('end_date', null); + $user->autoassign_licenses = $request->input('autoassign_licenses', 0); + + // Update the location of any assets checked out to this user + Asset::where('assigned_type', User::class) + ->where('assigned_to', $user->id) + ->update(['location_id' => $request->input('location_id', null)]); + + // Do we want to update the user password? + if ($request->filled('password')) { + $user->password = bcrypt($request->input('password')); + } + + $permissions_array = $request->input('permission'); + + // Strip out the superuser permission if the user isn't a superadmin + if (! Auth::user()->isSuperUser()) { + unset($permissions_array['superuser']); + $permissions_array['superuser'] = $orig_superuser; + } + + $user->permissions = json_encode($permissions_array); + + // Handle uploaded avatar + app(ImageUploadRequest::class)->handleImages($user, 600, 'avatar', 'avatars', 'avatar'); + + if ($user->save()) { + // Redirect to the user page + return redirect()->route('users.index') + ->with('success', trans('admin/users/message.success.update')); + } + + return redirect()->back()->withInput()->withErrors($user->getErrors()); - // Update the location of any assets checked out to this user - Asset::where('assigned_type', User::class) - ->where('assigned_to', $user->id) - ->update(['location_id' => $request->input('location_id', null)]); - // Do we want to update the user password? - if ($request->filled('password')) { - $user->password = bcrypt($request->input('password')); } - $permissions_array = $request->input('permission'); - - // Strip out the superuser permission if the user isn't a superadmin - if (! Auth::user()->isSuperUser()) { - unset($permissions_array['superuser']); - $permissions_array['superuser'] = $orig_superuser; - } - - $user->permissions = json_encode($permissions_array); - - // Handle uploaded avatar - app(ImageUploadRequest::class)->handleImages($user, 600, 'avatar', 'avatars', 'avatar'); - - //\Log::debug(print_r($user, true)); - - // Was the user updated? - if ($user->save()) { - // Redirect to the user page - return redirect()->route('users.index') - ->with('success', trans('admin/users/message.success.update')); - } - - return redirect()->back()->withInput()->withErrors($user->getErrors()); + return redirect()->route('users.index')->with('error', trans('admin/users/message.user_not_found', compact('id'))); } /** @@ -326,7 +333,7 @@ class UsersController extends Controller { try { // Get user information - $user = User::findOrFail($id); + $user = Company::scopeCompanyables(User::findOrFail($id)); // Authorize takes care of many of our logic checks now. $this->authorize('delete', User::class); @@ -429,17 +436,25 @@ class UsersController extends Controller public function show($userId = null) { - if (! $user = Company::scopeCompanyables(User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($userId))) { - // Redirect to the user management page - return redirect()->route('users.index') - ->with('error', trans('admin/users/message.user_not_found', ['id' => $userId])); + // We can use the more generic auth check here since the company scoping is happening at the query level + $this->authorize('view', User::class); + + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); + $user = Company::scopeCompanyables($user)->find($userId); + + //dd($user); + + if ($user) { + \Log::debug('User '.$user->username.' is found - checking permission'); + + $userlog = $user->userlog->load('item'); + + return view('users/view', compact('user', 'userlog'))->with('settings', Setting::getSettings()); } - $this->authorize('view', $user); - $userlog = $user->userlog->load('item'); + return redirect()->route('users.index') + ->with('error', trans('admin/users/message.user_not_found', ['id' => $userId])); - return view('users/view', compact('user', 'userlog')) - ->with('settings', Setting::getSettings()); } /** @@ -547,8 +562,20 @@ class UsersController extends Controller // Open output stream $handle = fopen('php://output', 'w'); - Company::scopeCompanyables(User::with('assets', 'accessories', 'consumables', 'department', 'licenses', 'manager', 'groups', 'userloc', 'company') - ->orderBy('created_at', 'DESC') + $users = User::with( + 'assets', + 'accessories', + 'consumables', + 'department', + 'licenses', + 'manager', + 'groups', + 'userloc', + 'company' + )->orderBy('created_at', 'DESC'); + + // FMCS scoping + Company::scopeCompanyables($users) ->chunk(500, function ($users) use ($handle) { $headers = [ // strtolower to prevent Excel from trying to open it as a SYLK file @@ -605,7 +632,7 @@ class UsersController extends Controller fputcsv($handle, $values); } - })); + }); // Close the output stream fclose($handle); From 460693c1535f4993e52d60fe3770245b44076a42 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 14:40:13 +0100 Subject: [PATCH 10/32] Added comment Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 5ef45ee4d9..5e550e6ba6 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -273,6 +273,7 @@ class UsersController extends Controller $users = $users->withTrashed(); } + // Apply companyable scope $users = Company::scopeCompanyables($users); From 24e87cc0bbcae91a379ddb268661c0a39408b3f8 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 14:51:49 +0100 Subject: [PATCH 11/32] Updated comment Signed-off-by: snipe --- app/Policies/SnipePermissionsPolicy.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Policies/SnipePermissionsPolicy.php b/app/Policies/SnipePermissionsPolicy.php index 3f385f2d49..93d6074cdb 100644 --- a/app/Policies/SnipePermissionsPolicy.php +++ b/app/Policies/SnipePermissionsPolicy.php @@ -36,7 +36,7 @@ abstract class SnipePermissionsPolicy public function before(User $user, $ability, $item) { /** - * If an admin, they can do all asset related tasks, but constrained by FMCSA company access. + * If an admin, they can do all item related tasks, but ARE constrained by FMCSA company access. * That scoping happens on the model level (except for the Users model) via the Companyable trait. * * This does lead to some inconsistencies in the responses, since attempting to edit assets, From ed0a441e4d53d7d1c4ca53fed243016f32dd9a46 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 14:52:03 +0100 Subject: [PATCH 12/32] Refactor destroy method Signed-off-by: snipe --- .../Controllers/Users/UsersController.php | 101 ++++++------------ 1 file changed, 30 insertions(+), 71 deletions(-) diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 9239180ff3..e6c01d8b32 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -331,12 +331,13 @@ class UsersController extends Controller */ public function destroy($id = null) { - try { - // Get user information - $user = Company::scopeCompanyables(User::findOrFail($id)); - // Authorize takes care of many of our logic checks now. - $this->authorize('delete', User::class); + $this->authorize('delete', User::class); + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); + $user = Company::scopeCompanyables($user)->find($id); + + + if ($user) { // Check if we are not trying to delete ourselves if ($user->id === Auth::id()) { // Redirect to the user management page @@ -370,16 +371,12 @@ class UsersController extends Controller // Delete the user $user->delete(); - - // Prepare the success message - // Redirect to the user management page return redirect()->route('users.index')->with('success', trans('admin/users/message.success.delete')); - } catch (ModelNotFoundException $e) { - // Prepare the error message - // Redirect to the user management page - return redirect()->route('users.index') - ->with('error', trans('admin/users/message.user_not_found', compact('id'))); } + + return redirect()->route('users.index') + ->with('error', trans('admin/users/message.user_not_found', compact('id'))); + } /** @@ -442,59 +439,15 @@ class UsersController extends Controller $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); $user = Company::scopeCompanyables($user)->find($userId); - //dd($user); - if ($user) { - \Log::debug('User '.$user->username.' is found - checking permission'); - $userlog = $user->userlog->load('item'); - return view('users/view', compact('user', 'userlog'))->with('settings', Setting::getSettings()); } - return redirect()->route('users.index') - ->with('error', trans('admin/users/message.user_not_found', ['id' => $userId])); + return redirect()->route('users.index')->with('error', trans('admin/users/message.user_not_found', ['id' => $userId])); } - /** - * Unsuspend a user. - * - * @author [A. Gianotto] [] - * @since [v1.0] - * @param int $id - * @return Redirect - * @throws \Illuminate\Auth\Access\AuthorizationException - */ - public function getUnsuspend($id = null) - { - try { - // Get user information - $user = Company::scopeCompanyables(User::findOrFail($id)); - $this->authorize('update', $user); - - // Check if we are not trying to unsuspend ourselves - if ($user->id === Auth::id()) { - // Prepare the error message - $error = trans('admin/users/message.error.unsuspend'); - // Redirect to the user management page - return redirect()->route('users.index')->with('error', $error); - } - - // Do we have permission to unsuspend this user? - if ($user->isSuperUser() && ! Auth::user()->isSuperUser()) { - // Redirect to the user management page - return redirect()->route('users.index')->with('error', 'Insufficient permissions!'); - } - - // Redirect to the user management page - return redirect()->route('users.index')->with('success', trans('admin/users/message.success.unsuspend')); - } catch (ModelNotFoundException $e) { - // Redirect to the user management page - return redirect()->route('users.index') - ->with('error', trans('admin/users/message.user_not_found', compact('id'))); - } - } /** * Return a view containing a pre-populated new user form, @@ -509,22 +462,30 @@ class UsersController extends Controller public function getClone(Request $request, $id = null) { $this->authorize('create', User::class); + // We need to reverse the UI specific logic for our // permissions here before we update the user. $permissions = $request->input('permissions', []); app('request')->request->set('permissions', $permissions); - try { - // Get the user information - $user_to_clone = Company::scopeCompanyables(User::withTrashed()->find($id)); + + $user_to_clone = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); + $user_to_clone = Company::scopeCompanyables($user_to_clone)->find($id); + + if ($user_to_clone) { + + $user = clone $user_to_clone; + + // Blank out some fields $user->first_name = ''; $user->last_name = ''; $user->email = substr($user->email, ($pos = strpos($user->email, '@')) !== false ? $pos : 0); $user->id = null; - // Get this user groups + // Get this user's groups $userGroups = $user_to_clone->groups()->pluck('name', 'id'); + // Get all the available permissions $permissions = config('permissions'); $clonedPermissions = $user_to_clone->decodePermissions(); @@ -533,16 +494,14 @@ class UsersController extends Controller // Show the page return view('users/edit', compact('permissions', 'userPermissions')) - ->with('user', $user) - ->with('groups', Group::pluck('name', 'id')) - ->with('userGroups', $userGroups) - ->with('clone_user', $user_to_clone); - } catch (ModelNotFoundException $e) { - // Prepare the error message - // Redirect to the user management page - return redirect()->route('users.index') - ->with('error', trans('admin/users/message.user_not_found', compact('id'))); + ->with('user', $user) + ->with('groups', Group::pluck('name', 'id')) + ->with('userGroups', $userGroups) + ->with('clone_user', $user_to_clone); } + + return redirect()->route('users.index')->with('error', trans('admin/users/message.user_not_found', compact('id'))); + } /** From 710370ac2419fed48434437d1bf48c3c1194bec7 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 14:58:25 +0100 Subject: [PATCH 13/32] Added scoping for destroy Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 59 +++++++++++--------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 5e550e6ba6..fb46974b52 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -513,37 +513,42 @@ class UsersController extends Controller public function destroy($id) { $this->authorize('delete', User::class); - $user = User::findOrFail($id); - $this->authorize('delete', $user); + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); + $user = Company::scopeCompanyables($user)->find($id); - if (($user->assets) && ($user->assets->count() > 0)) { - return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.error.delete_has_assets'))); - } + if ($user) { - if (($user->licenses) && ($user->licenses->count() > 0)) { - return response()->json(Helper::formatStandardApiResponse('error', null, 'This user still has '.$user->licenses->count().' license(s) associated with them and cannot be deleted.')); - } - - if (($user->accessories) && ($user->accessories->count() > 0)) { - return response()->json(Helper::formatStandardApiResponse('error', null, 'This user still has '.$user->accessories->count().' accessories associated with them.')); - } - - if (($user->managedLocations()) && ($user->managedLocations()->count() > 0)) { - return response()->json(Helper::formatStandardApiResponse('error', null, 'This user still has '.$user->managedLocations()->count().' locations that they manage.')); - } - - if ($user->delete()) { - - // Remove the user's avatar if they have one - if (Storage::disk('public')->exists('avatars/'.$user->avatar)) { - try { - Storage::disk('public')->delete('avatars/'.$user->avatar); - } catch (\Exception $e) { - \Log::debug($e); - } + $this->authorize('delete', $user); + + if (($user->assets) && ($user->assets->count() > 0)) { + return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.error.delete_has_assets'))); } - return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/users/message.success.delete'))); + if (($user->licenses) && ($user->licenses->count() > 0)) { + return response()->json(Helper::formatStandardApiResponse('error', null, 'This user still has ' . $user->licenses->count() . ' license(s) associated with them and cannot be deleted.')); + } + + if (($user->accessories) && ($user->accessories->count() > 0)) { + return response()->json(Helper::formatStandardApiResponse('error', null, 'This user still has ' . $user->accessories->count() . ' accessories associated with them.')); + } + + if (($user->managedLocations()) && ($user->managedLocations()->count() > 0)) { + return response()->json(Helper::formatStandardApiResponse('error', null, 'This user still has ' . $user->managedLocations()->count() . ' locations that they manage.')); + } + + if ($user->delete()) { + + // Remove the user's avatar if they have one + if (Storage::disk('public')->exists('avatars/' . $user->avatar)) { + try { + Storage::disk('public')->delete('avatars/' . $user->avatar); + } catch (\Exception $e) { + \Log::debug($e); + } + } + + return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/users/message.success.delete'))); + } } return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.error.delete'))); From 0d23d28a65e37e95514cced8cdceb008784d5544 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 15:15:56 +0100 Subject: [PATCH 14/32] Added comments Signed-off-by: snipe --- app/Models/CompanyableTrait.php | 7 ++++++- app/Providers/AuthServiceProvider.php | 25 ++++++++++++++++--------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/app/Models/CompanyableTrait.php b/app/Models/CompanyableTrait.php index b03b346d2e..df67f2be4f 100644 --- a/app/Models/CompanyableTrait.php +++ b/app/Models/CompanyableTrait.php @@ -5,8 +5,13 @@ namespace App\Models; trait CompanyableTrait { /** - * Boot the companyable trait for a model. + * This trait is used to scope models to the current company. To use this scope on companyable models, + * we use the "use Companyable;" statement at the top of the mode. * + * We CANNOT USE THIS ON USERS, as it causes an infinite loop and prevents users from logging in, since this scope will be + * applied to the currently logged in (or logging in) user in addition to the user model for viewing lists of users. + * + * @see \App\Models\Company\Company::scopeCompanyables() * @return void */ public static function bootCompanyableTrait() diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php index 9d493e85bb..11a5d3c1ba 100644 --- a/app/Providers/AuthServiceProvider.php +++ b/app/Providers/AuthServiceProvider.php @@ -93,21 +93,28 @@ class AuthServiceProvider extends ServiceProvider Passport::personalAccessTokensExpireIn(Carbon::now()->addYears(config('passport.expiration_years'))); Passport::withCookieSerialization(); - // -------------------------------- - // BEFORE ANYTHING ELSE - // -------------------------------- - // If this condition is true, ANYTHING else below will be assumed - // to be true. This can cause weird blade behavior. + + /** + * BEFORE ANYTHING ELSE + * + * If this condition is true, ANYTHING else below will be assumed to be true. + * This is where we set the superadmin permission to allow superadmins to be able to do everything within the system. + * + */ Gate::before(function ($user) { if ($user->isSuperUser()) { return true; } }); - // -------------------------------- - // GENERAL GATES - // These control general sections of the admin - // -------------------------------- + + /** + * GENERAL GATES + * + * These control general sections of the admin. These definitions are used in our blades via @can('blah) and also + * use in our controllers to determine if a user has access to a certain area. + */ + Gate::define('admin', function ($user) { if ($user->hasAccess('admin')) { return true; From c604f087499784a8c01df237a6c2282b2fe084cd Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 11 Apr 2024 18:47:55 +0100 Subject: [PATCH 15/32] Small tweaks for troubleshooting :( Signed-off-by: snipe --- app/Models/Company.php | 48 +++++++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/app/Models/Company.php b/app/Models/Company.php index 76cbda10ed..567680d8f6 100644 --- a/app/Models/Company.php +++ b/app/Models/Company.php @@ -121,25 +121,49 @@ final class Company extends SnipeModel } } + /** + * Check to see if the current user should have access to the model. + * I hate this method and I think it should be refactored. + * + * @param $companyable + * @return bool|void + */ public static function isCurrentUserHasAccess($companyable) { + // When would this even happen tho?? if (is_null($companyable)) { return false; - } elseif (! static::isFullMultipleCompanySupportEnabled()) { - return true; - } elseif (!$companyable instanceof Company && !\Schema::hasColumn($companyable->getModel()->getTable(), 'company_id')) { - // This is primary for the gate:allows-check in location->isDeletable() - // Locations don't have a company_id so without this it isn't possible to delete locations with FullMultipleCompanySupport enabled - // because this function is called by SnipePermissionsPolicy->before() - return true; - } else { - if (Auth::user()) { - $current_user_company_id = Auth::user()->company_id; - $companyable_company_id = $companyable->company_id; + } - return $current_user_company_id == null || $current_user_company_id == $companyable_company_id || Auth::user()->isSuperUser(); + // If FMCS is not enabled, everyone has access, return true + if (! static::isFullMultipleCompanySupportEnabled()) { + return true; + } + + // Again, where would this happen? But check that $companyable is not a string + if (!is_string($companyable)) { + $company_table = $companyable->getModel()->getTable(); + try { + // This is primary for the gate:allows-check in location->isDeletable() + // Locations don't have a company_id so without this it isn't possible to delete locations with FullMultipleCompanySupport enabled + // because this function is called by SnipePermissionsPolicy->before() + if (!$companyable instanceof Company && !\Schema::hasColumn($company_table, 'company_id')) { + return true; + } + + } catch (\Exception $e) { + \Log::warning($e); } } + + + if (Auth::user()) { + \Log::warning('Companyable is '.$companyable); + $current_user_company_id = Auth::user()->company_id; + $companyable_company_id = $companyable->company_id; + return $current_user_company_id == null || $current_user_company_id == $companyable_company_id || Auth::user()->isSuperUser(); + } + } public static function isCurrentUserAuthorized() From 81b8c111ca59ad376b981cc1e5567e623181e064 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 11 Apr 2024 15:00:14 -0700 Subject: [PATCH 16/32] Allow multiple fields to be displayed in one row --- app/View/Label.php | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/app/View/Label.php b/app/View/Label.php index f47ad6acd5..f43263aefa 100644 --- a/app/View/Label.php +++ b/app/View/Label.php @@ -142,7 +142,30 @@ class Label implements View // Remove Duplicates $toAdd = $field ->filter(fn($o) => !$myFields->contains('dataSource', $o['dataSource'])) - ->first(); + ->reduce(function ($carry, $item) { + // On the first iteration we simply return the item. + // If there is only one item to be processed for the row + // then this effectively skips everything below this if block. + if (is_null($carry)){ + return $item; + } + + // At this point we are dealing with a row with multiple items being displayed. + + + // The end result of this will be in this format: + // {labelOne} {valueOne} | {labelTwo} {valueTwo} | {labelThree} {valueThree} + $carry['value'] = implode(' | ', [ + implode(' ', [$carry['label'], $carry['value']]), + implode(' ', [$item['label'], $item['value']]), + ]); + + // We'll set the label to an empty string since we + // injected the label into the value field above. + $carry['label'] = ''; + + return $carry; + }); return $toAdd ? $myFields->push($toAdd) : $myFields; }, new Collection()); From 2b137d76fa7e3ffc5007ffe0c083debb4c26d7fc Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 11 Apr 2024 16:38:22 -0700 Subject: [PATCH 17/32] Trim string to avoid leading whitespace if label is empty --- app/View/Label.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/View/Label.php b/app/View/Label.php index f43263aefa..b1b121bd4e 100644 --- a/app/View/Label.php +++ b/app/View/Label.php @@ -155,10 +155,10 @@ class Label implements View // The end result of this will be in this format: // {labelOne} {valueOne} | {labelTwo} {valueTwo} | {labelThree} {valueThree} - $carry['value'] = implode(' | ', [ + $carry['value'] = trim(implode(' | ', [ implode(' ', [$carry['label'], $carry['value']]), implode(' ', [$item['label'], $item['value']]), - ]); + ])); // We'll set the label to an empty string since we // injected the label into the value field above. From da03cfdbe5837c3a558802e59f1648a089872b67 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 11 Apr 2024 16:39:29 -0700 Subject: [PATCH 18/32] Formatting --- app/View/Label.php | 1 - 1 file changed, 1 deletion(-) diff --git a/app/View/Label.php b/app/View/Label.php index b1b121bd4e..3c49f04b6c 100644 --- a/app/View/Label.php +++ b/app/View/Label.php @@ -152,7 +152,6 @@ class Label implements View // At this point we are dealing with a row with multiple items being displayed. - // The end result of this will be in this format: // {labelOne} {valueOne} | {labelTwo} {valueTwo} | {labelThree} {valueThree} $carry['value'] = trim(implode(' | ', [ From c3a71cc18285b7266a69fd90e2b293a0ec49f5fc Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 11 Apr 2024 16:44:13 -0700 Subject: [PATCH 19/32] Improve variable names and add comment --- app/View/Label.php | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/app/View/Label.php b/app/View/Label.php index 3c49f04b6c..bb587d6c06 100644 --- a/app/View/Label.php +++ b/app/View/Label.php @@ -142,28 +142,29 @@ class Label implements View // Remove Duplicates $toAdd = $field ->filter(fn($o) => !$myFields->contains('dataSource', $o['dataSource'])) - ->reduce(function ($carry, $item) { + ->reduce(function ($previous, $current) { // On the first iteration we simply return the item. // If there is only one item to be processed for the row // then this effectively skips everything below this if block. - if (is_null($carry)){ - return $item; + if (is_null($previous)) { + return $current; } // At this point we are dealing with a row with multiple items being displayed. + // We need to combine the label and value of the current item with the previous item. // The end result of this will be in this format: // {labelOne} {valueOne} | {labelTwo} {valueTwo} | {labelThree} {valueThree} - $carry['value'] = trim(implode(' | ', [ - implode(' ', [$carry['label'], $carry['value']]), - implode(' ', [$item['label'], $item['value']]), + $previous['value'] = trim(implode(' | ', [ + implode(' ', [$previous['label'], $previous['value']]), + implode(' ', [$current['label'], $current['value']]), ])); // We'll set the label to an empty string since we // injected the label into the value field above. - $carry['label'] = ''; + $previous['label'] = ''; - return $carry; + return $previous; }); return $toAdd ? $myFields->push($toAdd) : $myFields; From a4e959818ae84b41fcf1fc097771e625b52e1232 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 11 Apr 2024 17:23:28 -0700 Subject: [PATCH 20/32] Add comment --- app/View/Label.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/View/Label.php b/app/View/Label.php index bb587d6c06..3ec3a4099c 100644 --- a/app/View/Label.php +++ b/app/View/Label.php @@ -142,6 +142,8 @@ class Label implements View // Remove Duplicates $toAdd = $field ->filter(fn($o) => !$myFields->contains('dataSource', $o['dataSource'])) + // For fields that have multiple options, we need to combine them + // into a single field so all values are displayed. ->reduce(function ($previous, $current) { // On the first iteration we simply return the item. // If there is only one item to be processed for the row From 20920c262d8bbf9363f5662113db7fb988e90f39 Mon Sep 17 00:00:00 2001 From: q4kK Date: Tue, 9 Apr 2024 13:09:27 -0500 Subject: [PATCH 21/32] Feat: add no-interactive flag for `upgrade.php` Having a no-interactive flag is useful because attempting to automate snipe-it upgrades is currently extremely janky. You have to either: a) read the prompts and pass in the input (the "correct" way) b) pipe in input, e.g. `yes` and hope no new prompts are added. With the `no-interactive` flag, this can be fixed by both allowing new prompts to be added without conflict to user prompts, but also allowing automated upgrades (you could choose to crash, or assign defaults). --- upgrade.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/upgrade.php b/upgrade.php index 2fd1c45426..0ac33856f3 100644 --- a/upgrade.php +++ b/upgrade.php @@ -19,6 +19,7 @@ $app_environment = 'develop'; $skip_php_checks = false; $branch = 'master'; $branch_override = false; +$no_interactive = false; // Check for branch or other overrides if ($argc > 1){ @@ -32,6 +33,9 @@ if ($argc > 1){ $branch = $argv[$arg]; $branch_override = true; break; + case '--no-interactive': + $no_interactive = true; + break; default: // for legacy support from before we started using --branch $branch = $argv[$arg]; $branch_override = true; @@ -81,7 +85,13 @@ if($upgrade_requirements){ echo "Found PHP requirements, will check for PHP > $php_min_works and < $php_max_wontwork\n"; } // done fetching requirements -$yesno = readline("\nProceed with upgrade? [Y/n]: "); + +if (!$no_interactive) { + $yesno = readline("\nProceed with upgrade? [Y/n]: "); +} else { + $yesno = "yes"; +} + if ($yesno == "yes" || $yesno == "YES" ||$yesno == "y" ||$yesno == "Y"){ # don't do anything } else { From ab3b5ca4efcd52ce910b32bb2461eaabac9a6493 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 09:26:07 +0100 Subject: [PATCH 22/32] Fixed tests Signed-off-by: snipe --- app/Models/Company.php | 2 +- app/Policies/SnipePermissionsPolicy.php | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/Models/Company.php b/app/Models/Company.php index 567680d8f6..276824141f 100644 --- a/app/Models/Company.php +++ b/app/Models/Company.php @@ -131,7 +131,7 @@ final class Company extends SnipeModel public static function isCurrentUserHasAccess($companyable) { // When would this even happen tho?? - if (is_null($companyable)) { + if (!$companyable) { return false; } diff --git a/app/Policies/SnipePermissionsPolicy.php b/app/Policies/SnipePermissionsPolicy.php index 93d6074cdb..96c94cd776 100644 --- a/app/Policies/SnipePermissionsPolicy.php +++ b/app/Policies/SnipePermissionsPolicy.php @@ -52,6 +52,16 @@ abstract class SnipePermissionsPolicy return true; } + /** + * If we got here by $this→authorize('something', $actualModel) then we can continue on Il but if we got here + * via $this→authorize('something', Model::class) then calling Company:: isCurrentUserHasAccess($item) gets weird. + * Bail out here by returning "nothing" and allow the relevant method lower in this class to be called and handle authorization. + */ + if (!$item instanceof Model){ + return; + } + + /** * The Company::isCurrentUserHasAccess() method from the company model handles the check for FMCS already so we * don't have to do that here. From 5e8c129c7f6c145320e129478c72993434e47a85 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 09:26:50 +0100 Subject: [PATCH 23/32] Revert accidental commit Signed-off-by: snipe --- app/Models/Company.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Models/Company.php b/app/Models/Company.php index 276824141f..567680d8f6 100644 --- a/app/Models/Company.php +++ b/app/Models/Company.php @@ -131,7 +131,7 @@ final class Company extends SnipeModel public static function isCurrentUserHasAccess($companyable) { // When would this even happen tho?? - if (!$companyable) { + if (is_null($companyable)) { return false; } From 86e274faa32c467a5565f2efe12ddc88c57d2acd Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 10:47:48 +0100 Subject: [PATCH 24/32] Added API tests Signed-off-by: snipe --- tests/Feature/Api/Users/UpdateUserApiTest.php | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index f58aae4a05..b9fc729b13 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -2,6 +2,7 @@ namespace Tests\Feature\Api\Users; +use App\Models\Company; use App\Models\User; use Tests\TestCase; @@ -58,4 +59,68 @@ class UpdateUserApiTest extends TestCase $this->assertEquals(0, $user->refresh()->activated); } + + public function testUsersScopedToCompanyDuringUpdateWhenMultipleFullCompanySupportEnabled() + { + $this->settings->enableMultipleFullCompanySupport(); + + $companyA = Company::factory()->create(['name'=>'Company A']); + $companyB = Company::factory()->create(['name'=>'Company B']); + + $adminA = User::factory(['company_id' => $companyA->id])->admin()->create(); + $adminB = User::factory(['company_id' => $companyB->id])->admin()->create(); + $adminNoCompany = User::factory(['company_id' => null])->admin()->create(); + + // Create a user that belongs to company B + $userA = User::factory()->create(['activated' => true, 'company_id' => $companyA->id]); + $userB = User::factory()->create(['activated' => true, 'company_id' => $companyB->id]); + $userNoCompany = User::factory()->create(['activated' => true, 'company_id' => null]); + + // Admin for Company A should allow updating user from Company A + $this->actingAsForApi($adminA) + ->patchJson(route('api.users.update', $userA)) + ->assertStatus(200); + + // Admin for Company A should get denied updating user from Company B + $this->actingAsForApi($adminA) + ->patchJson(route('api.users.update', $userB)) + ->assertStatus(403); + + // Admin for Company A should get denied updating user without a company + $this->actingAsForApi($adminA) + ->patchJson(route('api.users.update', $userNoCompany)) + ->assertStatus(403); + + // Admin for Company B should allow updating user from Company B + $this->actingAsForApi($adminB) + ->patchJson(route('api.users.update', $userB)) + ->assertStatus(200); + + // Admin for Company B should get denied updating user from Company A + $this->actingAsForApi($adminB) + ->patchJson(route('api.users.update', $userA)) + ->assertStatus(403); + + // Admin for Company B should get denied updating user without a company + $this->actingAsForApi($adminB) + ->patchJson(route('api.users.update', $userNoCompany)) + ->assertStatus(403); + + // Admin without a company should allow updating user without a company + $this->actingAsForApi($adminNoCompany) + ->patchJson(route('api.users.update', $userNoCompany)) + ->assertStatus(200); + + // Admin without a company should get denied updating user from Company A + $this->actingAsForApi($adminNoCompany) + ->patchJson(route('api.users.update', $userA)) + ->assertStatus(403); + + // Admin without a company should get denied updating user from Company B + $this->actingAsForApi($adminNoCompany) + ->patchJson(route('api.users.update', $userB)) + ->assertStatus(403); + + + } } From c21142605d33bf8a445370d13e68fb1f3908a3c5 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 10:47:56 +0100 Subject: [PATCH 25/32] Added strings Signed-off-by: snipe --- resources/lang/en-US/general.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/resources/lang/en-US/general.php b/resources/lang/en-US/general.php index a3b391f76e..1fb5eeecd4 100644 --- a/resources/lang/en-US/general.php +++ b/resources/lang/en-US/general.php @@ -508,6 +508,8 @@ return [ 'url' => 'URL', 'edit_fieldset' => 'Edit fieldset fields and options', 'permission_denied_superuser_demo' => 'Permission denied. You cannot update user information for superadmins on the demo.', + 'pwd_reset_not_sent' => 'User is not activated, is LDAP synced, or does not have an email address', + 'error_sending_email' => 'Error sending email', 'bulk' => [ 'delete' => [ From 65dd729e1959d1577f3ff0a52a4b9e727611e837 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 10:57:20 +0100 Subject: [PATCH 26/32] Additional gates Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index fb46974b52..f6228005bf 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -404,7 +404,10 @@ class UsersController extends Controller public function show($id) { $this->authorize('view', User::class); + $user = User::withCount('assets as assets_count', 'licenses as licenses_count', 'accessories as accessories_count', 'consumables as consumables_count')->findOrFail($id); + $user = Company::scopeCompanyables($user)->find($id); + $this->authorize('update', $user); return (new UsersTransformer)->transformUser($user); } @@ -424,6 +427,8 @@ class UsersController extends Controller $this->authorize('update', User::class); $user = User::findOrFail($id); + $user = Company::scopeCompanyables($user)->find($id); + $this->authorize('update', $user); /** * This is a janky hack to prevent people from changing admin demo user data on the public demo. @@ -515,6 +520,7 @@ class UsersController extends Controller $this->authorize('delete', User::class); $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); $user = Company::scopeCompanyables($user)->find($id); + $this->authorize('delete', $user); if ($user) { @@ -566,6 +572,11 @@ class UsersController extends Controller { $this->authorize('view', User::class); $this->authorize('view', Asset::class); + + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); + $user = Company::scopeCompanyables($user)->find($id); + $this->authorize('view', $user); + $assets = Asset::where('assigned_to', '=', $id)->where('assigned_type', '=', User::class)->with('model'); @@ -601,7 +612,10 @@ class UsersController extends Controller */ public function emailAssetList(Request $request, $id) { + $this->authorize('update', User::class); $user = User::findOrFail($id); + $user = Company::scopeCompanyables($user)->find($id); + $this->authorize('update', $user); if (empty($user->email)) { return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.inventorynotification.error'))); @@ -625,6 +639,7 @@ class UsersController extends Controller $this->authorize('view', User::class); $this->authorize('view', Consumable::class); $user = User::findOrFail($id); + $this->authorize('update', $user); $consumables = $user->consumables; return (new ConsumablesTransformer)->transformConsumables($consumables, $consumables->count(), $request); } @@ -641,6 +656,7 @@ class UsersController extends Controller { $this->authorize('view', User::class); $user = User::findOrFail($id); + $this->authorize('view', $user); $this->authorize('view', Accessory::class); $accessories = $user->accessories; @@ -661,6 +677,7 @@ class UsersController extends Controller $this->authorize('view', License::class); if ($user = User::where('id', $id)->withTrashed()->first()) { + $this->authorize('update', $user); $licenses = $user->licenses()->get(); return (new LicensesTransformer())->transformLicenses($licenses, $licenses->count()); } @@ -684,6 +701,7 @@ class UsersController extends Controller if ($request->filled('id')) { try { $user = User::find($request->get('id')); + $this->authorize('update', $user); $user->two_factor_secret = null; $user->two_factor_enrolled = 0; $user->saveQuietly(); From 9bb15aaf1b8c92938b0cb16645b611ab6d51387c Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 10:57:49 +0100 Subject: [PATCH 27/32] Added individual gates to keep response consistent with other company-ed things Signed-off-by: snipe --- .../Controllers/Users/UsersController.php | 43 +++++++++++++------ 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index e6c01d8b32..9316c2dec4 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -432,13 +432,15 @@ class UsersController extends Controller */ public function show($userId = null) { - - // We can use the more generic auth check here since the company scoping is happening at the query level + // Make sure the user can view users at all $this->authorize('view', User::class); $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); $user = Company::scopeCompanyables($user)->find($userId); + // Make sure they can view this particular user + $this->authorize('view', $user); + if ($user) { $userlog = $user->userlog->load('item'); return view('users/view', compact('user', 'userlog'))->with('settings', Setting::getSettings()); @@ -472,6 +474,10 @@ class UsersController extends Controller $user_to_clone = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); $user_to_clone = Company::scopeCompanyables($user_to_clone)->find($id); + // Make sure they can view this particular user + $this->authorize('view', $user_to_clone); + + if ($user_to_clone) { @@ -614,6 +620,10 @@ class UsersController extends Controller { $this->authorize('view', User::class); $show_user = Company::scopeCompanyables(User::where('id', $id)->withTrashed()->first()); + + // Make sure they can view this particular user + $this->authorize('view', $show_user); + $assets = Asset::where('assigned_to', $id)->where('assigned_type', User::class)->with('model', 'model.category')->get(); $accessories = $show_user->accessories()->get(); $consumables = $show_user->consumables()->get(); @@ -638,16 +648,23 @@ class UsersController extends Controller { $this->authorize('view', User::class); - if (!$user = Company::scopeCompanyables(User::find($id))) { - return redirect()->back() - ->with('error', trans('admin/users/message.user_not_found', ['id' => $id])); - } - if (empty($user->email)) { - return redirect()->back()->with('error', trans('admin/users/message.user_has_no_email')); + $user = Company::scopeCompanyables(User::find($id)); + + // Make sure they can view this particular user + $this->authorize('view', $user); + + if ($user) { + + if (empty($user->email)) { + return redirect()->back()->with('error', trans('admin/users/message.user_has_no_email')); + } + + $user->notify((new CurrentInventory($user))); + return redirect()->back()->with('success', trans('admin/users/general.user_notified')); } - $user->notify((new CurrentInventory($user))); - return redirect()->back()->with('success', trans('admin/users/general.user_notified')); + return redirect()->back()->with('error', trans('admin/users/message.user_not_found', ['id' => $id])); + } /** @@ -665,13 +682,13 @@ class UsersController extends Controller try { Password::sendResetLink($credentials); - return redirect()->back()->with('success', trans('admin/users/message.password_reset_sent', ['email' => $user->email])); + } catch (\Exception $e) { - return redirect()->back()->with('error', ' Error sending email. :( '); + return redirect()->back()->with('error', trans('general.error_sending_email')); } } - return redirect()->back()->with('error', 'User is not activated, is LDAP synced, or does not have an email address '); + return redirect()->back()->with('error', trans('general.pwd_reset_not_sent')); } } From 4450351b754cd8165f1df267c833be06d953b599 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 11:06:50 +0100 Subject: [PATCH 28/32] Only sync groups if API user is superadmin Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index f6228005bf..ba1a5366f6 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -465,6 +465,7 @@ class UsersController extends Controller if (! Auth::user()->isSuperUser()) { unset($permissions_array['superuser']); } + $user->permissions = $permissions_array; } @@ -487,6 +488,7 @@ class UsersController extends Controller // Check if the request has groups passed and has a value if ($request->filled('groups')) { + $validator = Validator::make($request->all(), [ 'groups.*' => 'integer|exists:permission_groups,id', ]); @@ -494,10 +496,19 @@ class UsersController extends Controller if ($validator->fails()){ return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors())); } - $user->groups()->sync($request->input('groups')); + + // Only save groups if the user is a superuser + if (Auth::user()->isSuperUser()) { + $user->groups()->sync($request->input('groups')); + } + // The groups field has been passed but it is null, so we should blank it out } elseif ($request->has('groups')) { - $user->groups()->sync([]); + + // Only save groups if the user is a superuser + if (Auth::user()->isSuperUser()) { + $user->groups()->sync($request->input('groups')); + } } From 0fc9fc75164598d5c433aa42ad8e7edec975e58a Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 20:29:01 +0100 Subject: [PATCH 29/32] Minor updates to tests Signed-off-by: snipe --- tests/Feature/Api/Users/UpdateUserApiTest.php | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index b9fc729b13..1d7c9cd61d 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -71,56 +71,55 @@ class UpdateUserApiTest extends TestCase $adminB = User::factory(['company_id' => $companyB->id])->admin()->create(); $adminNoCompany = User::factory(['company_id' => null])->admin()->create(); - // Create a user that belongs to company B - $userA = User::factory()->create(['activated' => true, 'company_id' => $companyA->id]); - $userB = User::factory()->create(['activated' => true, 'company_id' => $companyB->id]); - $userNoCompany = User::factory()->create(['activated' => true, 'company_id' => null]); + // Create users that belongs to company A and B and one that is unscoped + $scoped_user_in_companyA = User::factory()->create(['activated' => true, 'company_id' => $companyA->id]); + $scoped_user_in_companyB = User::factory()->create(['activated' => true, 'company_id' => $companyB->id]); + $scoped_user_in_no_company = User::factory()->create(['activated' => true, 'company_id' => null]); // Admin for Company A should allow updating user from Company A $this->actingAsForApi($adminA) - ->patchJson(route('api.users.update', $userA)) + ->patchJson(route('api.users.update', $scoped_user_in_companyA)) ->assertStatus(200); // Admin for Company A should get denied updating user from Company B $this->actingAsForApi($adminA) - ->patchJson(route('api.users.update', $userB)) + ->patchJson(route('api.users.update', $scoped_user_in_companyB)) ->assertStatus(403); // Admin for Company A should get denied updating user without a company $this->actingAsForApi($adminA) - ->patchJson(route('api.users.update', $userNoCompany)) + ->patchJson(route('api.users.update', $scoped_user_in_no_company)) ->assertStatus(403); // Admin for Company B should allow updating user from Company B $this->actingAsForApi($adminB) - ->patchJson(route('api.users.update', $userB)) + ->patchJson(route('api.users.update', $scoped_user_in_companyB)) ->assertStatus(200); // Admin for Company B should get denied updating user from Company A $this->actingAsForApi($adminB) - ->patchJson(route('api.users.update', $userA)) + ->patchJson(route('api.users.update', $scoped_user_in_companyA)) ->assertStatus(403); // Admin for Company B should get denied updating user without a company $this->actingAsForApi($adminB) - ->patchJson(route('api.users.update', $userNoCompany)) + ->patchJson(route('api.users.update', $scoped_user_in_no_company)) ->assertStatus(403); // Admin without a company should allow updating user without a company $this->actingAsForApi($adminNoCompany) - ->patchJson(route('api.users.update', $userNoCompany)) + ->patchJson(route('api.users.update', $scoped_user_in_no_company)) ->assertStatus(200); // Admin without a company should get denied updating user from Company A $this->actingAsForApi($adminNoCompany) - ->patchJson(route('api.users.update', $userA)) + ->patchJson(route('api.users.update', $scoped_user_in_companyA)) ->assertStatus(403); // Admin without a company should get denied updating user from Company B $this->actingAsForApi($adminNoCompany) - ->patchJson(route('api.users.update', $userB)) + ->patchJson(route('api.users.update', $scoped_user_in_companyB)) ->assertStatus(403); - } } From 4c02a63accbe09772efb5a9c373800986b7dc66b Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 17 Apr 2024 20:29:51 +0100 Subject: [PATCH 30/32] Removed activated attribute on test users Signed-off-by: snipe --- tests/Feature/Api/Users/UpdateUserApiTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index 1d7c9cd61d..479e74a882 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -72,9 +72,9 @@ class UpdateUserApiTest extends TestCase $adminNoCompany = User::factory(['company_id' => null])->admin()->create(); // Create users that belongs to company A and B and one that is unscoped - $scoped_user_in_companyA = User::factory()->create(['activated' => true, 'company_id' => $companyA->id]); - $scoped_user_in_companyB = User::factory()->create(['activated' => true, 'company_id' => $companyB->id]); - $scoped_user_in_no_company = User::factory()->create(['activated' => true, 'company_id' => null]); + $scoped_user_in_companyA = User::factory()->create(['company_id' => $companyA->id]); + $scoped_user_in_companyB = User::factory()->create(['company_id' => $companyB->id]); + $scoped_user_in_no_company = User::factory()->create(['company_id' => null]); // Admin for Company A should allow updating user from Company A $this->actingAsForApi($adminA) From 1d64692fd66e60be23a81fefa27666ff6d5a2a1b Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 17 Apr 2024 15:23:26 -0700 Subject: [PATCH 31/32] Add two test cases --- tests/Feature/Api/Users/UpdateUserApiTest.php | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index 479e74a882..1728de82cd 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -3,6 +3,7 @@ namespace Tests\Feature\Api\Users; use App\Models\Company; +use App\Models\Group; use App\Models\User; use Tests\TestCase; @@ -120,6 +121,58 @@ class UpdateUserApiTest extends TestCase $this->actingAsForApi($adminNoCompany) ->patchJson(route('api.users.update', $scoped_user_in_companyB)) ->assertStatus(403); + } + public function testUserGroupsAreOnlyUpdatedIfAuthenticatedUserIsSuperUser() + { + $groupToJoin = Group::factory()->create(); + + $normalUser = User::factory()->editUsers()->create(); + $superUser = User::factory()->superuser()->create(); + + $oneUserToUpdate = User::factory()->create(); + $anotherUserToUpdate = User::factory()->create(); + + $this->actingAsForApi($normalUser) + ->patchJson(route('api.users.update', $oneUserToUpdate), [ + 'groups' => [$groupToJoin->id], + ]); + + $this->actingAsForApi($superUser) + ->patchJson(route('api.users.update', $anotherUserToUpdate), [ + 'groups' => [$groupToJoin->id], + ]); + + $this->assertFalse( + $oneUserToUpdate->refresh()->groups->contains($groupToJoin), + 'Non-super-user was able to modify user group' + ); + $this->assertTrue($anotherUserToUpdate->refresh()->groups->contains($groupToJoin)); + } + + public function testUserGroupsCanBeClearedBySuperUser() + { + $normalUser = User::factory()->editUsers()->create(); + $superUser = User::factory()->superuser()->create(); + + $oneUserToUpdate = User::factory()->create(); + $anotherUserToUpdate = User::factory()->create(); + + $joinedGroup = Group::factory()->create(); + $oneUserToUpdate->groups()->sync([$joinedGroup->id]); + $anotherUserToUpdate->groups()->sync([$joinedGroup->id]); + + $this->actingAsForApi($normalUser) + ->patchJson(route('api.users.update', $oneUserToUpdate), [ + 'groups' => null, + ]); + + $this->actingAsForApi($superUser) + ->patchJson(route('api.users.update', $anotherUserToUpdate), [ + 'groups' => null, + ]); + + $this->assertTrue($oneUserToUpdate->refresh()->groups->contains($joinedGroup)); + $this->assertFalse($anotherUserToUpdate->refresh()->groups->contains($joinedGroup)); } } From 424cbd324880a8c1e367a5cb0eea10d5df189bc4 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 18 Apr 2024 14:25:54 +0100 Subject: [PATCH 32/32] Removed noisy debug Signed-off-by: snipe --- app/Models/Company.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/Models/Company.php b/app/Models/Company.php index 567680d8f6..4b718b4200 100644 --- a/app/Models/Company.php +++ b/app/Models/Company.php @@ -260,7 +260,6 @@ final class Company extends SnipeModel { // If not logged in and hitting this, assume we are on the command line and don't scope?' if (! static::isFullMultipleCompanySupportEnabled() || (Auth::check() && Auth::user()->isSuperUser()) || (! Auth::check())) { - \Log::debug('Skip scoping in scopeCompanyableChildren. User is not logged in or is a superadmin'); return $query; } else { \Log::debug('Fire scopeCompanyablesDirectly.'); @@ -313,7 +312,6 @@ final class Company extends SnipeModel if (count($companyable_names) == 0) { throw new Exception('No Companyable Children to scope'); } elseif (! static::isFullMultipleCompanySupportEnabled() || (Auth::check() && Auth::user()->isSuperUser())) { - \Log::debug('Skip scoping in scopeCompanyableChildren. User is not logged in or is a superadmin'); return $query; } else { $f = function ($q) {