mirror of
https://github.com/snipe/snipe-it.git
synced 2024-12-24 05:04:07 -08:00
Merge pull request #15284 from spencerrlongg/bug/sc-26584
[Multi-Company] Fixes Users Being Moved With Items Still Assigned
This commit is contained in:
commit
c197644ba7
|
@ -427,13 +427,10 @@ class UsersController extends Controller
|
|||
* @param \Illuminate\Http\Request $request
|
||||
* @param int $id
|
||||
*/
|
||||
public function update(SaveUserRequest $request, $id) : JsonResponse
|
||||
public function update(SaveUserRequest $request, User $user): JsonResponse
|
||||
{
|
||||
$this->authorize('update', User::class);
|
||||
|
||||
if ($user = User::find($id)) {
|
||||
|
||||
|
||||
$this->authorize('update', $user);
|
||||
|
||||
/**
|
||||
|
@ -443,12 +440,10 @@ class UsersController extends Controller
|
|||
*
|
||||
*/
|
||||
|
||||
|
||||
if ((($id == 1) || ($id == 2)) && (config('app.lock_passwords'))) {
|
||||
if ((($user->id == 1) || ($user->id == 2)) && (config('app.lock_passwords'))) {
|
||||
return response()->json(Helper::formatStandardApiResponse('error', null, 'Permission denied. You cannot update user information via API on the demo.'));
|
||||
}
|
||||
|
||||
|
||||
$user->fill($request->all());
|
||||
|
||||
if ($user->id == $request->input('manager_id')) {
|
||||
|
@ -473,16 +468,13 @@ class UsersController extends Controller
|
|||
$user->permissions = $permissions_array;
|
||||
}
|
||||
|
||||
|
||||
// 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)]);
|
||||
|
||||
|
||||
app('App\Http\Requests\ImageUploadRequest')->handleImages($user, 600, 'image', 'avatars', 'avatar');
|
||||
|
||||
if ($user->save()) {
|
||||
|
||||
// Check if the request has groups passed and has a value, AND that the user us a superuser
|
||||
if (($request->has('groups')) && (auth()->user()->isSuperUser())) {
|
||||
|
||||
|
@ -496,18 +488,10 @@ class UsersController extends Controller
|
|||
|
||||
// Sync the groups since the user is a superuser and the groups pass validation
|
||||
$user->groups()->sync($request->input('groups'));
|
||||
|
||||
|
||||
}
|
||||
|
||||
return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update')));
|
||||
}
|
||||
|
||||
return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
|
||||
}
|
||||
|
||||
return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id'))));
|
||||
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -186,7 +186,7 @@ class UsersController extends Controller
|
|||
{
|
||||
|
||||
$this->authorize('update', User::class);
|
||||
$user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id);
|
||||
$user = User::with(['assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc'])->withTrashed()->find($id);
|
||||
|
||||
if ($user) {
|
||||
|
||||
|
@ -214,83 +214,79 @@ class UsersController extends Controller
|
|||
* @return \Illuminate\Http\RedirectResponse
|
||||
* @throws \Illuminate\Auth\Access\AuthorizationException
|
||||
*/
|
||||
public function update(SaveUserRequest $request, $id = null)
|
||||
public function update(SaveUserRequest $request, User $user)
|
||||
{
|
||||
$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'))) {
|
||||
if ((($user->id == 1) || ($user->id == 2)) && (config('app.lock_passwords'))) {
|
||||
return redirect()->route('users.index')->with('error', trans('general.permission_denied_superuser_demo'));
|
||||
}
|
||||
|
||||
|
||||
// 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()->find($id);
|
||||
$user->load(['assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc'])->withTrashed();
|
||||
|
||||
// User is valid - continue...
|
||||
if ($user) {
|
||||
$this->authorize('update', $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'];
|
||||
}
|
||||
// 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 superuser
|
||||
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 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);
|
||||
// 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);
|
||||
|
||||
// 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)]);
|
||||
// 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'));
|
||||
}
|
||||
// Do we want to update the user password?
|
||||
if ($request->filled('password')) {
|
||||
$user->password = bcrypt($request->input('password'));
|
||||
}
|
||||
|
||||
|
||||
// Update the location of any assets checked out to this user
|
||||
|
@ -318,13 +314,7 @@ class UsersController extends Controller
|
|||
return redirect()->to(Helper::getRedirectOption($request, $user->id, 'Users'))
|
||||
->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')));
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -34,6 +34,14 @@ class SaveUserRequest extends FormRequest
|
|||
$rules = [
|
||||
'department_id' => 'nullable|exists:departments,id',
|
||||
'manager_id' => 'nullable|exists:users,id',
|
||||
'company_id' => [
|
||||
// determines if the user is being moved between companies and checks to see if they have any items assigned
|
||||
function ($attribute, $value, $fail) {
|
||||
if (($this->has('company_id')) && ($this->user->allAssignedCount() > 0) && (Setting::getSettings()->full_multiple_companies_support)) {
|
||||
$fail(trans('admin/users/message.error.multi_company_items_assigned'));
|
||||
}
|
||||
}
|
||||
]
|
||||
];
|
||||
|
||||
switch ($this->method()) {
|
||||
|
|
|
@ -53,6 +53,7 @@ return array(
|
|||
'ldap_could_not_search' => 'Could not search the LDAP server. Please check your LDAP server configuration in the LDAP config file. <br>Error from LDAP Server:',
|
||||
'ldap_could_not_get_entries' => 'Could not get entries from the LDAP server. Please check your LDAP server configuration in the LDAP config file. <br>Error from LDAP Server:',
|
||||
'password_ldap' => 'The password for this account is managed by LDAP/Active Directory. Please contact your IT department to change your password. ',
|
||||
'multi_company_items_assigned' => 'This user has items assigned, please check them in before moving companies.'
|
||||
),
|
||||
|
||||
'deletefile' => array(
|
||||
|
|
|
@ -66,7 +66,9 @@
|
|||
|
||||
<div class="row">
|
||||
<div class="col-md-8 col-md-offset-2">
|
||||
<form class="form-horizontal" method="post" autocomplete="off" action="{{ (isset($user->id)) ? route('users.update', ['user' => $user->id]) : route('users.store') }}" enctype="multipart/form-data" id="userForm">
|
||||
<form class="form-horizontal" method="post" autocomplete="off"
|
||||
action="{{ (isset($user->id)) ? route('users.update', ['user' => $user->id]) : route('users.store') }}"
|
||||
enctype="multipart/form-data" id="userForm">
|
||||
{{csrf_field()}}
|
||||
|
||||
@if($user->id)
|
||||
|
|
|
@ -1097,18 +1097,20 @@ Route::group(['prefix' => 'v1', 'middleware' => ['api', 'throttle:api']], functi
|
|||
]
|
||||
)->name('api.users.restore');
|
||||
|
||||
});
|
||||
});
|
||||
|
||||
Route::match(['put', 'patch'], '{user}/update', [Api\UsersController::class, 'update'])
|
||||
->name('api.users.update');
|
||||
|
||||
Route::resource('users',
|
||||
Api\UsersController::class,
|
||||
['names' => [
|
||||
'index' => 'api.users.index',
|
||||
'show' => 'api.users.show',
|
||||
'update' => 'api.users.update',
|
||||
'store' => 'api.users.store',
|
||||
'destroy' => 'api.users.destroy',
|
||||
],
|
||||
'except' => ['create', 'edit'],
|
||||
'except' => ['create', 'edit', 'update'],
|
||||
'parameters' => ['user' => 'user_id'],
|
||||
]
|
||||
); // end users API routes
|
||||
|
|
|
@ -145,10 +145,12 @@ Route::group(['prefix' => 'users', 'middleware' => ['auth']], function () {
|
|||
]
|
||||
)->name('users/bulkeditsave');
|
||||
|
||||
|
||||
// pulling this out of the resource because I need route model binding in the request
|
||||
Route::match(['put', 'patch'], '/{user}', [Users\UsersController::class, 'update'])->name('users.update');
|
||||
});
|
||||
|
||||
Route::resource('users', Users\UsersController::class, [
|
||||
'middleware' => ['auth'],
|
||||
'parameters' => ['user' => 'user_id'],
|
||||
'except' => ['update']
|
||||
]);
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
namespace Tests\Feature\Users\Api;
|
||||
|
||||
use App\Models\Asset;
|
||||
use App\Models\Company;
|
||||
use App\Models\Department;
|
||||
use App\Models\Group;
|
||||
|
@ -344,4 +345,47 @@ class UpdateUserTest extends TestCase
|
|||
$this->assertTrue($user->refresh()->groups->contains($groupB));
|
||||
}
|
||||
|
||||
public function testMultiCompanyUserCannotBeMovedIfHasAsset()
|
||||
{
|
||||
$this->settings->enableMultipleFullCompanySupport();
|
||||
|
||||
$companyA = Company::factory()->create();
|
||||
$companyB = Company::factory()->create();
|
||||
|
||||
$user = User::factory()->create([
|
||||
'company_id' => $companyA->id,
|
||||
]);
|
||||
$superUser = User::factory()->superuser()->create();
|
||||
|
||||
$asset = Asset::factory()->create();
|
||||
|
||||
// no assets assigned, therefore success
|
||||
$this->actingAsForApi($superUser)->patchJson(route('api.users.update', $user), [
|
||||
'username' => 'test',
|
||||
'company_id' => $companyB->id,
|
||||
])->assertStatusMessageIs('success');
|
||||
|
||||
// same test but PUT
|
||||
$this->actingAsForApi($superUser)->putJson(route('api.users.update', $user), [
|
||||
'username' => 'test',
|
||||
'first_name' => 'Test',
|
||||
'company_id' => $companyB->id,
|
||||
])->assertStatusMessageIs('success');
|
||||
|
||||
$asset->checkOut($user, $superUser);
|
||||
|
||||
// asset assigned, therefore error
|
||||
$this->actingAsForApi($superUser)->patchJson(route('api.users.update', $user), [
|
||||
'username' => 'test',
|
||||
'company_id' => $companyB->id,
|
||||
])->assertStatusMessageIs('error');
|
||||
|
||||
// same test but PUT
|
||||
$this->actingAsForApi($superUser)->putJson(route('api.users.update', $user), [
|
||||
'username' => 'test',
|
||||
'first_name' => 'Test',
|
||||
'company_id' => $companyB->id,
|
||||
])->assertStatusMessageIs('error');
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -2,6 +2,8 @@
|
|||
|
||||
namespace Tests\Feature\Users\Ui;
|
||||
|
||||
use App\Models\Asset;
|
||||
use App\Models\Company;
|
||||
use App\Models\User;
|
||||
use Tests\TestCase;
|
||||
|
||||
|
@ -79,4 +81,39 @@ class UpdateUserTest extends TestCase
|
|||
|
||||
$this->assertEquals(1, $admin->refresh()->activated);
|
||||
}
|
||||
|
||||
public function testMultiCompanyUserCannotBeMovedIfHasAsset()
|
||||
{
|
||||
$this->settings->enableMultipleFullCompanySupport();
|
||||
|
||||
$companyA = Company::factory()->create();
|
||||
$companyB = Company::factory()->create();
|
||||
|
||||
$user = User::factory()->create([
|
||||
'company_id' => $companyA->id,
|
||||
]);
|
||||
$superUser = User::factory()->superuser()->create();
|
||||
|
||||
$asset = Asset::factory()->create();
|
||||
|
||||
// no assets assigned, therefore success
|
||||
$this->actingAs($superUser)->put(route('users.update', $user), [
|
||||
'first_name' => 'test',
|
||||
'username' => 'test',
|
||||
'company_id' => $companyB->id,
|
||||
'redirect_option' => 'index'
|
||||
])->assertRedirect(route('users.index'));
|
||||
|
||||
$asset->checkOut($user, $superUser);
|
||||
|
||||
// asset assigned, therefore error
|
||||
$response = $this->actingAs($superUser)->patchJson(route('users.update', $user), [
|
||||
'first_name' => 'test',
|
||||
'username' => 'test',
|
||||
'company_id' => $companyB->id,
|
||||
'redirect_option' => 'index'
|
||||
]);
|
||||
|
||||
$this->followRedirects($response)->assertSee('error');
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue