Merge pull request #14591 from snipe/bug/sc-25258/naive_fix_for_user_scoping

First fix for user FMCS scoping

app/Http/Controllers/Api/UsersController.php

@@ -273,6 +273,7 @@ class UsersController extends Controller
             $users = $users->withTrashed();
         }
 
+        // Apply companyable scope
         $users = Company::scopeCompanyables($users);
 
 
@@ -403,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);
     }
@@ -423,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.
@@ -459,6 +465,7 @@ class UsersController extends Controller
             if (! Auth::user()->isSuperUser()) {
                 unset($permissions_array['superuser']);
             }
+
             $user->permissions = $permissions_array;
         }
 
@@ -481,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',
                 ]);
@@ -488,10 +496,19 @@ class UsersController extends Controller
                 if ($validator->fails()){
                     return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
                 }
+
+                // 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'));
+                }
             }
 
 
@@ -512,7 +529,12 @@ class UsersController extends Controller
     public function destroy($id)
     {
         $this->authorize('delete', User::class);
-        $user = User::findOrFail($id);
+        $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed();
+        $user = Company::scopeCompanyables($user)->find($id);
+        $this->authorize('delete', $user);
+
+        if ($user) {
+
             $this->authorize('delete', $user);
             
             if (($user->assets) && ($user->assets->count() > 0)) {
@@ -520,23 +542,23 @@ class UsersController extends Controller
             }
 
             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.'));
+                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.'));
+                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.'));
+                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)) {
+                if (Storage::disk('public')->exists('avatars/' . $user->avatar)) {
                     try {
-                    Storage::disk('public')->delete('avatars/'.$user->avatar);
+                        Storage::disk('public')->delete('avatars/' . $user->avatar);
                     } catch (\Exception $e) {
                         \Log::debug($e);
                     }
@@ -544,6 +566,7 @@ class UsersController extends Controller
 
                 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')));
     }
@@ -560,6 +583,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');
 
 
@@ -595,7 +623,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')));
@@ -619,6 +650,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);
     }
@@ -635,6 +667,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;
 
@@ -655,6 +688,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());
         }
@@ -678,6 +712,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();

app/Http/Controllers/Users/UsersController.php

@@ -182,8 +182,13 @@ class UsersController extends Controller
      */
     public function edit($id)
     {
-        if ($user = 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');
 
@@ -210,27 +215,30 @@ class UsersController extends Controller
      */
     public function update(SaveUserRequest $request, $id = null)
     {
-        // We need to reverse the UI specific logic for our
+        $this->authorize('update', User::class);
-        // permissions here before we update the user.
-        $permissions = $request->input('permissions', []);
-        app('request')->request->set('permissions', $permissions);
 
         // 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')));
-        }
 
+        // 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';
@@ -240,15 +248,13 @@ class UsersController extends Controller
                 }
             }
 
-        // Only save groups if the user is a super user
+            // Only save groups if the user is a superuser
             if (Auth::user()->isSuperUser()) {
                 $user->groups()->sync($request->input('groups'));
             }
 
-        // Update the user
+            // Update the user fields
-        if ($request->filled('username')) {
             $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');
@@ -300,9 +306,6 @@ class UsersController extends Controller
             // 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')
@@ -310,6 +313,11 @@ class UsersController extends Controller
             }
 
             return redirect()->back()->withInput()->withErrors($user->getErrors());
+
+
+        }
+
+        return redirect()->route('users.index')->with('error', trans('admin/users/message.user_not_found', compact('id')));
     }
 
     /**
@@ -323,12 +331,13 @@ class UsersController extends Controller
      */
     public function destroy($id = null)
     {
-        try {
-            // Get user information
-            $user = 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
@@ -362,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')));
-        }
+
     }
 
     /**
@@ -427,58 +432,24 @@ class UsersController extends Controller
      */
     public function show($userId = null)
     {
-        if (! $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($userId)) {
+        // Make sure the user can view users at all
-            // Redirect to the user management page
+        $this->authorize('view', User::class);
-            return redirect()->route('users.index')
-                ->with('error', trans('admin/users/message.user_not_found', ['id' => $userId]));
-        }
 
-        $userlog = $user->userlog->load('item');
+        $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);
 
-        return view('users/view', compact('user', 'userlog'))
+        if ($user) {
-            ->with('settings', Setting::getSettings());
+            $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]));
-     * Unsuspend a user.
-     *
-     * @author [A. Gianotto] [<snipe@snipe.net>]
-     * @since [v1.0]
-     * @param  int $id
-     * @return Redirect
-     * @throws \Illuminate\Auth\Access\AuthorizationException
-     */
-    public function getUnsuspend($id = null)
-    {
-        try {
-            // Get user information
-            $user = 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,
@@ -493,22 +464,34 @@ 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 = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed();
-            $user_to_clone = User::withTrashed()->find($id);
+        $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) {
+
+
             $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();
@@ -521,12 +504,10 @@ class UsersController extends Controller
                 ->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')));
         }
+
+        return redirect()->route('users.index')->with('error', trans('admin/users/message.user_not_found', compact('id')));
+
     }
 
     /**
@@ -546,8 +527,20 @@ class UsersController extends Controller
             // Open output stream
             $handle = fopen('php://output', 'w');
 
-            User::with('assets', 'accessories', 'consumables', 'department', 'licenses', 'manager', 'groups', 'userloc', 'company')
+            $users = User::with(
-                ->orderBy('created_at', 'DESC')
+                '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
@@ -565,7 +558,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'),
@@ -626,7 +619,11 @@ 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());
+
+        // 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();
@@ -651,10 +648,13 @@ class UsersController extends Controller
     {
         $this->authorize('view', User::class);
 
-        if (!$user = User::find($id)) {
+        $user = Company::scopeCompanyables(User::find($id));
-            return redirect()->back()
+
-                ->with('error', trans('admin/users/message.user_not_found', ['id' => $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'));
             }
@@ -663,6 +663,10 @@ class UsersController extends Controller
             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]));
+
+    }
+
     /**
      * Send individual password reset email
      *
@@ -672,19 +676,19 @@ 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 {
 
                 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'));
     }
 }

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)
     {
@@ -141,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()) {
+        }
+
+        // If FMCS is not enabled, everyone has access, return true
+        if (! static::isFullMultipleCompanySupportEnabled()) {
             return true;
-        } elseif (!$companyable instanceof Company && !\Schema::hasColumn($companyable->getModel()->getTable(), 'company_id')) {
+        }
+
+        // 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;
-        } else {
+                }
+
+            } 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()
@@ -190,6 +194,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 +207,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 +237,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] <snipe@snipe.net>
+     * @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] <snipe@snipe.net>
+     * @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;
+        }
+    }
+
 }

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()

app/Policies/SnipePermissionsPolicy.php

@@ -35,16 +35,50 @@ 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)) {
+         * If an admin, they can do all item related tasks, but ARE constrained by FMCSA company access.
-            return false;
+         * That scoping happens on the model level (except for the Users model) via the Companyable trait.
-        }
+         *
-        // If an admin, they can do all asset related tasks.
+         * 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;
         }
+
+        /**
+         * 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.
+         */
+        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');

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
+        /**
-        // --------------------------------
+         * BEFORE ANYTHING ELSE
-        // If this condition is true, ANYTHING else below will be assumed
+         *
-        // to be true. This can cause weird blade behavior.
+         * 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;

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. ',

resources/lang/en-US/general.php

@@ -507,6 +507,9 @@ 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.',
+    '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' =>
                 [

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,67 @@ 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 users that belongs to company A and B and one that is unscoped
+        $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)
+            ->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', $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', $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', $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', $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', $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', $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', $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', $scoped_user_in_companyB))
+            ->assertStatus(403);
+
+    }
 }