diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 848adb66f..81229d78b 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -13,7 +13,6 @@ use App\Http\Transformers\SelectlistTransformer; use App\Http\Transformers\UsersTransformer; use App\Models\Actionlog; use App\Models\Asset; -use App\Models\Company; use App\Models\License; use App\Models\User; use App\Notifications\CurrentInventory; @@ -290,10 +289,6 @@ class UsersController extends Controller - // Apply companyable scope - $users = Company::scopeCompanyables($users); - - // Make sure the offset and limit are actually integers and do not exceed system limits $offset = ($request->input('offset') > $users->count()) ? $users->count() : app('api_offset_value'); $limit = app('api_limit_value'); @@ -326,8 +321,6 @@ class UsersController extends Controller ] )->where('show_in_list', '=', '1'); - $users = Company::scopeCompanyables($users); - if ($request->filled('search')) { $users = $users->where(function ($query) use ($request) { $query->SimpleNameSearch($request->get('search')) @@ -422,9 +415,7 @@ class UsersController extends Controller { $this->authorize('view', User::class); - $user = User::withCount('assets as assets_count', 'licenses as licenses_count', 'accessories as accessories_count', 'consumables as consumables_count', 'managesUsers as manages_users_count', 'managedLocations as manages_locations_count'); - - if ($user = Company::scopeCompanyables($user)->find($id)) { + if ($user = User::withCount('assets as assets_count', 'licenses as licenses_count', 'accessories as accessories_count', 'consumables as consumables_count', 'managesUsers as manages_users_count', 'managedLocations as manages_locations_count')->find($id)) { $this->authorize('view', $user); return (new UsersTransformer)->transformUser($user); } @@ -447,15 +438,12 @@ class UsersController extends Controller { $this->authorize('update', User::class); - $user = User::findOrFail($id); - $user = Company::scopeCompanyables($user)->find($id); + $user = 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. - * * 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 * */ @@ -534,8 +522,8 @@ class UsersController extends Controller public function destroy(DeleteUserRequest $request, $id) { $this->authorize('delete', User::class); - $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc'); - $user = Company::scopeCompanyables($user)->find($id); + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id); + $this->authorize('delete', $user); @@ -554,9 +542,12 @@ 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'))); + } return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id')))); + } /** @@ -572,32 +563,35 @@ 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); + if ($user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id)) { + $this->authorize('view', $user); - $assets = Asset::where('assigned_to', '=', $id)->where('assigned_type', '=', User::class)->with('model'); + $assets = Asset::where('assigned_to', '=', $id)->where('assigned_type', '=', User::class)->with('model'); - // Filter on category ID - if ($request->filled('category_id')) { - $assets = $assets->InCategory($request->input('category_id')); - } - - - // Filter on model ID - if ($request->filled('model_id')) { - - $model_ids = $request->input('model_id'); - if (!is_array($model_ids)) { - $model_ids = array($model_ids); + // Filter on category ID + if ($request->filled('category_id')) { + $assets = $assets->InCategory($request->input('category_id')); } - $assets = $assets->InModelList($model_ids); + + + // Filter on model ID + if ($request->filled('model_id')) { + + $model_ids = $request->input('model_id'); + if (!is_array($model_ids)) { + $model_ids = array($model_ids); + } + $assets = $assets->InModelList($model_ids); + } + + $assets = $assets->get(); + + return (new AssetsTransformer)->transformAssets($assets, $assets->count(), $request); } - $assets = $assets->get(); + return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id')))); - return (new AssetsTransformer)->transformAssets($assets, $assets->count(), $request); } /** @@ -612,17 +606,21 @@ 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'))); + if ($user = User::find($id)) { + $this->authorize('update', $user); + + if (empty($user->email)) { + return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.inventorynotification.error'))); + } + + $user->notify((new CurrentInventory($user))); + return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/users/message.inventorynotification.success'))); } - $user->notify((new CurrentInventory($user))); + return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id')))); - return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/users/message.inventorynotification.success'))); + } /** diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index e88572b58..a82993f04 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -182,8 +182,7 @@ class UsersController extends Controller { $this->authorize('update', User::class); - $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); - $user = Company::scopeCompanyables($user)->find($id); + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id); if ($user) { @@ -229,9 +228,7 @@ class UsersController extends Controller $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 = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id); // User is valid - continue... if ($user) { @@ -338,8 +335,8 @@ class UsersController extends Controller { $this->authorize('delete', User::class); + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc'); - $user = Company::scopeCompanyables($user)->find($id); if (($user) && ($user->deleted_at = '')) { // Delete the user @@ -408,8 +405,7 @@ class UsersController extends Controller // 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); + $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($userId); // Make sure they can view this particular user $this->authorize('view', $user); @@ -444,9 +440,7 @@ class UsersController extends Controller app('request')->request->set('permissions', $permissions); - $user_to_clone = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed(); - $user_to_clone = Company::scopeCompanyables($user_to_clone)->find($id); - + $user_to_clone = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id); // Make sure they can view this particular user $this->authorize('view', $user_to_clone); @@ -510,10 +504,7 @@ class UsersController extends Controller 'groups', 'userloc', 'company' - )->orderBy('created_at', 'DESC'); - - // FMCS scoping - Company::scopeCompanyables($users) + )->orderBy('created_at', 'DESC') ->chunk(500, function ($users) use ($handle) { $headers = [ // strtolower to prevent Excel from trying to open it as a SYLK file @@ -607,20 +598,20 @@ class UsersController extends Controller public function printInventory($id) { $this->authorize('view', User::class); - $show_user = Company::scopeCompanyables(User::where('id', $id)->withTrashed()->first()); + $user = User::where('id', $id)->withTrashed()->first(); // Make sure they can view this particular user - $this->authorize('view', $show_user); + $this->authorize('view', $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(); + $accessories = $user->accessories()->get(); + $consumables = $user->consumables()->get(); return view('users/print')->with('assets', $assets) - ->with('licenses', $show_user->licenses()->get()) + ->with('licenses', $user->licenses()->get()) ->with('accessories', $accessories) ->with('consumables', $consumables) - ->with('show_user', $show_user) + ->with('show_user', $user) ->with('settings', Setting::getSettings()); } @@ -636,7 +627,7 @@ class UsersController extends Controller { $this->authorize('view', User::class); - $user = Company::scopeCompanyables(User::find($id)); + $user = User::find($id); // Make sure they can view this particular user $this->authorize('view', $user); @@ -664,7 +655,7 @@ class UsersController extends Controller */ public function sendPasswordReset($id) { - if (($user = Company::scopeCompanyables(User::find($id))) && ($user->activated == '1') && ($user->email != '') && ($user->ldap_import == '0')) { + if (($user = User::find($id)) && ($user->activated == '1') && ($user->email != '') && ($user->ldap_import == '0')) { $credentials = ['email' => trim($user->email)]; try { diff --git a/app/Models/Company.php b/app/Models/Company.php index e0e1b8e87..ea8a28b7e 100644 --- a/app/Models/Company.php +++ b/app/Models/Company.php @@ -5,11 +5,11 @@ namespace App\Models; use App\Models\Traits\Searchable; use App\Presenters\Presentable; use Illuminate\Support\Facades\Auth; -use Illuminate\Support\Facades\DB; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Support\Facades\Gate; use Watson\Validating\ValidatingTrait; use Illuminate\Support\Facades\Log; +use Illuminate\Support\Facades\Schema; /** * Model for Companies. * @@ -147,7 +147,7 @@ final class Company extends SnipeModel // 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')) { + if (!$companyable instanceof Company && !Schema::hasColumn($company_table, 'company_id')) { return true; } @@ -259,7 +259,7 @@ 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())) { + if (! static::isFullMultipleCompanySupportEnabled() || (Auth::hasUser() && Auth::user()->isSuperUser()) || (! Auth::hasUser())) { return $query; } else { return static::scopeCompanyablesDirectly($query, $column, $table_name); @@ -267,27 +267,31 @@ final class Company extends SnipeModel } /** - * Scoping table queries, determining if a logged in user is part of a company, and only allows + * 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 + * + * @see https://github.com/laravel/framework/pull/24518 for info on Auth::hasUser() */ 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()) { + + // Get the company ID of the logged-in user, or set it to null if there is no company associated with the user + if (Auth::hasUser()) { $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()."."; // If the column exists in the table, use it to scope the query - if (\Schema::hasColumn($query->getModel()->getTable(), $column)) { + if ((($query) && ($query->getModel()) && (Schema::hasColumn($query->getModel()->getTable(), $column)))) { + + // 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()."."; + 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); } + } /** @@ -305,7 +309,7 @@ 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())) { + } elseif (! static::isFullMultipleCompanySupportEnabled() || (Auth::hasUser() && Auth::user()->isSuperUser())) { return $query; } else { $f = function ($q) { diff --git a/app/Models/User.php b/app/Models/User.php index 26bf5bb27..e2e7f7eee 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -5,7 +5,6 @@ namespace App\Models; use App\Http\Traits\UniqueUndeletedTrait; use App\Models\Traits\Searchable; use App\Presenters\Presentable; -use Illuminate\Support\Facades\DB; use Illuminate\Auth\Authenticatable; use Illuminate\Auth\Passwords\CanResetPassword; use Illuminate\Contracts\Auth\Access\Authorizable as AuthorizableContract; @@ -24,6 +23,7 @@ use Watson\Validating\ValidatingTrait; class User extends SnipeModel implements AuthenticatableContract, AuthorizableContract, CanResetPasswordContract, HasLocalePreference { use HasFactory; + use CompanyableTrait; protected $presenter = \App\Presenters\UserPresenter::class; use SoftDeletes, ValidatingTrait; diff --git a/resources/views/users/print.blade.php b/resources/views/users/print.blade.php index 4bbec7980..d41d80b18 100644 --- a/resources/views/users/print.blade.php +++ b/resources/views/users/print.blade.php @@ -470,7 +470,7 @@ }); }); - +@endpush diff --git a/tests/Feature/Users/Api/DeleteUserTest.php b/tests/Feature/Users/Api/DeleteUserTest.php index 7f7cb73f7..f3f8e80f3 100644 --- a/tests/Feature/Users/Api/DeleteUserTest.php +++ b/tests/Feature/Users/Api/DeleteUserTest.php @@ -56,7 +56,7 @@ class DeleteUserTest extends TestCase ->json(); } - public function testDisallowUserDeletionIfNoDeletePermissions() + public function testPermissionsForDeletingUsers() { $this->actingAsForApi(User::factory()->create()) @@ -65,7 +65,7 @@ class DeleteUserTest extends TestCase ->json(); } - public function testDisallowUserDeletionIfNotInSameCompanyAndNotSuperadmin() + public function testPermissionsIfNotInSameCompanyAndNotSuperadmin() { $this->settings->enableMultipleFullCompanySupport(); [$companyA, $companyB] = Company::factory()->count(2)->create(); diff --git a/tests/Feature/Users/Api/ViewUserTest.php b/tests/Feature/Users/Api/ViewUserTest.php new file mode 100644 index 000000000..859333698 --- /dev/null +++ b/tests/Feature/Users/Api/ViewUserTest.php @@ -0,0 +1,61 @@ +create(); + + $this->actingAsForApi(User::factory()->viewUsers()->create()) + ->getJson(route('api.users.show', $user)) + ->assertOk(); + } + + public function testPermissionsWithCompanyableToDeleteUser() + { + + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + + $superuser = User::factory()->superuser()->create(); + $userFromA = User::factory()->for($companyA)->create(); + $userFromB = User::factory()->for($companyB)->create(); + + $this->actingAsForApi(User::factory()->deleteUsers()->for($companyA)->create()) + ->deleteJson(route('api.users.destroy', $userFromA->id)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); + + $this->actingAsForApi(User::factory()->deleteUsers()->for($companyB)->create()) + ->deleteJson(route('api.users.destroy', $userFromA->id)) + ->assertStatus(403); + + $this->actingAsForApi($superuser) + ->deleteJson(route('api.users.destroy', $userFromA->id)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); + + $this->actingAsForApi($superuser) + ->deleteJson(route('api.users.destroy', $userFromB->id)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); + + } + +} diff --git a/tests/Feature/Users/Ui/DeleteUserTest.php b/tests/Feature/Users/Ui/DeleteUserTest.php index 0f5fbdcec..61ee23379 100644 --- a/tests/Feature/Users/Ui/DeleteUserTest.php +++ b/tests/Feature/Users/Ui/DeleteUserTest.php @@ -7,12 +7,35 @@ use App\Models\LicenseSeat; use App\Models\Location; use App\Models\Accessory; use App\Models\User; +use App\Models\Company; use App\Models\Asset; class DeleteUserTest extends TestCase { + public function testPermissionsToDeleteUser() + { + + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + + $superuser = User::factory()->superuser()->create(); + $userFromA = User::factory()->for($companyA)->create(); + $userFromB = User::factory()->for($companyB)->create(); + + $this->followingRedirects()->actingAs(User::factory()->deleteUsers()->for($companyA)->create()) + ->delete(route('users.destroy', ['user' => $userFromB->id])) + ->assertStatus(403); + + $this->actingAs(User::factory()->deleteUsers()->for($companyA)->create()) + ->delete(route('users.destroy', ['user' => $userFromA->id])) + ->assertStatus(302) + ->assertRedirect(route('users.index')); + + } + public function testDisallowUserDeletionIfStillManagingPeople() { diff --git a/tests/Feature/Users/Ui/ViewUserTest.php b/tests/Feature/Users/Ui/ViewUserTest.php new file mode 100644 index 000000000..8f91273f1 --- /dev/null +++ b/tests/Feature/Users/Ui/ViewUserTest.php @@ -0,0 +1,84 @@ +settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + + $superuser = User::factory()->superuser()->create(); + $user = User::factory()->for($companyB)->create(); + + $this->actingAs(User::factory()->editUsers()->for($companyA)->create()) + ->get(route('users.show', ['user' => $user->id])) + ->assertStatus(403); + + $this->actingAs($superuser) + ->get(route('users.show', ['user' => $user->id])) + ->assertOk() + ->assertStatus(200); + } + + public function testPermissionsForPrintAllInventoryPage() + { + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + + $superuser = User::factory()->superuser()->create(); + $user = User::factory()->for($companyB)->create(); + + $this->actingAs(User::factory()->viewUsers()->for($companyA)->create()) + ->get(route('users.print', ['userId' => $user->id])) + ->assertStatus(403); + + $this->actingAs(User::factory()->viewUsers()->for($companyB)->create()) + ->get(route('users.print', ['userId' => $user->id])) + ->assertStatus(200); + + $this->actingAs($superuser) + ->get(route('users.print', ['userId' => $user->id])) + ->assertOk() + ->assertStatus(200); + } + + public function testUserWithoutCompanyPermissionsCannotSendInventory() + { + + Notification::fake(); + + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + + $superuser = User::factory()->superuser()->create(); + $user = User::factory()->for($companyB)->create(); + + $this->actingAs(User::factory()->viewUsers()->for($companyA)->create()) + ->post(route('users.email', ['userId' => $user->id])) + ->assertStatus(403); + + $this->actingAs(User::factory()->viewUsers()->for($companyB)->create()) + ->post(route('users.email', ['userId' => $user->id])) + ->assertStatus(302); + + $this->actingAs($superuser) + ->post(route('users.email', ['userId' => $user->id])) + ->assertStatus(302); + + Notification::assertSentTo( + [$user], CurrentInventory::class + ); + } + +}