diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 4d1c052915..4cb1063b44 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -522,93 +522,99 @@ class UsersController extends Controller { $this->authorize('update', User::class); - $this->authorize('update', $user); + $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 - * - */ + /** + * 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 ((($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.')); + return response()->json(Helper::formatStandardApiResponse('error', null, 'Permission denied. You cannot update user information via API on the demo.')); + } + + // Pull out sensitive fields that require extra permission + $user->fill($request->except(['password', 'username', 'email', 'activated', 'permissions', 'activation_code', 'remember_token', 'two_factor_secret', 'two_factor_enrolled', 'two_factor_optin'])); + + + if (auth()->user()->can('canEditAuthFields', $user) && auth()->user()->can('editableOnDemo')) { + + if ($request->filled('password')) { + $user->password = bcrypt($request->input('password')); } - $user->fill($request->all()); - - if ($request->filled('company_id')) { - $user->company_id = Company::getIdForCurrentUser($request->input('company_id')); + if ($request->filled('username')) { + $user->username = $request->input('username'); } - if ($user->id == $request->input('manager_id')) { - return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager')); + if ($request->filled('email')) { + $user->email = $request->input('email'); } - // check for permissions related fields and pull them out if the current user cannot edit them - if (auth()->user()->can('canEditAuthFields', $user) && auth()->user()->can('editableOnDemo')) { - - if ($request->filled('password')) { - $user->password = bcrypt($request->input('password')); - } - - if ($request->filled('username')) { - $user->username = $request->input('username'); - } - - if ($request->filled('display_name')) { - $user->display_name = $request->input('display_name'); - } - - if ($request->filled('email')) { - $user->email = $request->input('email'); - } - - if ($request->filled('activated')) { - $user->activated = $request->input('activated'); - } - + if ($request->filled('activated')) { + $user->activated = $request->input('activated'); } - // We need to use has() instead of filled() - // here because we need to overwrite permissions - // if someone needs to null them out - if ($request->has('permissions')) { - $permissions_array = $request->input('permissions'); + } - // Strip out the individual superuser permission if the API user isn't a superadmin - if (!auth()->user()->isSuperUser()) { - unset($permissions_array['superuser']); + // We need to use has() instead of filled() + // here because we need to overwrite permissions + // if someone needs to null them out + + if ($request->filled('display_name')) { + $user->display_name = $request->input('display_name'); + } + + if ($request->filled('company_id')) { + $user->company_id = Company::getIdForCurrentUser($request->input('company_id')); + } + + if ($user->id == $request->input('manager_id')) { + return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager')); + } + + + + if ($request->has('permissions')) { + $permissions_array = $request->input('permissions'); + + // Strip out the individual superuser permission if the API user isn't a superadmin + if (!auth()->user()->isSuperUser()) { + unset($permissions_array['superuser']); + } + + $user->permissions = $permissions_array; + } + + if ($request->has('location_id')) { + // 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, 'avatar', '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())) { + + $validator = Validator::make($request->only('groups'), [ + 'groups.*' => 'integer|exists:permission_groups,id', + ]); + + if ($validator->fails()) { + return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors())); } - $user->permissions = $permissions_array; + // Sync the groups since the user is a superuser and the groups pass validation + $user->groups()->sync($request->input('groups')); } - - if($request->has('location_id')) { - // 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, 'avatar', '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())) { - - $validator = Validator::make($request->only('groups'), [ - 'groups.*' => 'integer|exists:permission_groups,id', - ]); - - if ($validator->fails()) { - return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors())); - } - - // 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('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update'))); + } + return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors())); } /** diff --git a/tests/Feature/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php index 4ca1b6d3ee..93786bdd96 100644 --- a/tests/Feature/Users/Api/UpdateUserTest.php +++ b/tests/Feature/Users/Api/UpdateUserTest.php @@ -220,29 +220,79 @@ class UpdateUserTest extends TestCase { $hashed_original = Hash::make('!!094850394680980380kfejlskjfl'); $hashed_new = Hash::make('!ABCDEFGIJKL123!!!'); - $admin = User::factory()->editUsers()->create(); - $user = User::factory()->admin()->create(['username' => 'brandnewuser', 'email'=> 'brandnewemail@example.org', 'password' => $hashed_original, 'activated' => 1]); + + $editing_user = User::factory()->editUsers()->create(); + $adminuser = User::factory()->admin()->create(['username' => 'TestAdminUser', 'email'=> 'admin@example.org', 'password' => $hashed_original, 'activated' => 1]); + // The admin being edited $this->assertDatabaseHas('users', [ - 'id' => $user->id, - 'username' => 'brandnewuser', - 'email' => 'brandnewemail@example.org', + 'id' => $adminuser->id, + 'username' => 'TestAdminUser', + 'email' => 'admin@example.org', 'activated' => 1, 'password' => $hashed_original, + 'permissions' => '{"admin":"1"}', ]); - $this->actingAsForApi($admin) - ->patch(route('api.users.update', $user), [ + $this->actingAsForApi($editing_user) + ->patch(route('api.users.update', $adminuser), [ 'username' => 'testnewusername', 'email' => 'testnewemail@example.org', 'activated' => 0, + 'permissions' => "{'superadmin':1}", 'password' => $hashed_new, ]); - $this->assertEquals(0, $user->refresh()->activated); + // These should keep their old values + $this->assertEquals('TestAdminUser', $adminuser->refresh()->username); + $this->assertEquals('admin@example.org', $adminuser->refresh()->email); + $this->assertEquals(1, $adminuser->refresh()->activated); + $this->assertEquals($hashed_original, $adminuser->refresh()->password); + $this->assertEquals('{"admin":"1"}', $adminuser->refresh()->permissions); } + + public function testAdminsCannotEditEscalationFieldsForSuperadmins() + { + $hashed_original = Hash::make('my-awesome-password!!!!!12345'); + $hashed_new = Hash::make('!ABCDEFGIJKL123!!!'); + + $editing_user = User::factory()->admin()->create(); + $superuser = User::factory()->superuser()->create(['username' => 'TestSuperUser', 'email'=> 'superuser@example.org', 'password' => $hashed_original, 'activated' => 1]); + + + // The admin being edited + $this->assertDatabaseHas('users', [ + 'id' => $superuser->id, + 'username' => 'TestSuperUser', + 'email' => 'superuser@example.org', + 'activated' => 1, + 'password' => $hashed_original, + 'permissions' => '{"superuser":"1"}', + ]); + + $this->actingAsForApi($editing_user) + ->patch(route('api.users.update', $superuser), [ + 'username' => 'testnewusername', + 'email' => 'testnewemail@example.org', + 'activated' => 0, + 'permissions' => "{'superadmin':1}", + 'password' => $hashed_new, + ]); + + // These should keep their old values + $this->assertEquals('TestSuperUser', $superuser->refresh()->username); + $this->assertEquals('superuser@example.org', $superuser->refresh()->email); + $this->assertEquals(1, $superuser->refresh()->activated); + $this->assertEquals($hashed_original, $superuser->refresh()->password); + $this->assertEquals('{"superuser":"1"}', $superuser->refresh()->permissions); + $this->assertNotEquals('testnewusername', $superuser->refresh()->username); + $this->assertNotEquals('testnewemail@example.org', $superuser->refresh()->email); + $this->assertNotTrue(Hash::check('super-secret-new-password', $superuser->password), $superuser->refresh()->password); + + } + public function testUsersScopedToCompanyDuringUpdateWhenMultipleFullCompanySupportEnabled() { $this->settings->enableMultipleFullCompanySupport(); diff --git a/tests/Feature/Users/Ui/UpdateUserTest.php b/tests/Feature/Users/Ui/UpdateUserTest.php index 723ce5df2e..20dfd2519b 100644 --- a/tests/Feature/Users/Ui/UpdateUserTest.php +++ b/tests/Feature/Users/Ui/UpdateUserTest.php @@ -110,61 +110,53 @@ class UpdateUserTest extends TestCase public function testEditingUsersCannotEditEscalationFieldsForAdmins() { - $admin = User::factory()->editUsers()->create(['activated' => true]); - $hashed_original = Hash::make('!!094850394680980380kfejlskjfl'); - $hashed_new = Hash::make('!ABCDEFGIJKL123!!!'); - $user = User::factory()->admin()->create(['username' => 'brandnewuser', 'email'=> 'brandnewemail@example.org', 'password' => $hashed_original, 'activated' => true]); + $editing_user = User::factory()->editUsers()->create(['activated' => true]); + $hashed_original = Hash::make('my-awesome-password!!!!!12345'); + $admin = User::factory()->admin()->create(['username' => 'TestAdminUser', 'email'=> 'admin@example.org', 'password' => $hashed_original, 'activated' => true]); $this->assertDatabaseHas('users', [ - 'id' => $user->id, - 'username' => 'brandnewuser', - 'email' => 'brandnewemail@example.org', + 'id' => $admin->id, + 'username' => 'TestAdminUser', + 'email' => 'admin@example.org', 'activated' => 1, 'password' => $hashed_original, ]); - $this->actingAs($admin) - ->put(route('users.update', $user), [ + $this->actingAs($editing_user) + ->put(route('users.update', $admin), [ 'username' => 'testnewusername', 'email' => 'testnewemail@example.org', 'activated' => 0, - 'password' => 'super-secret', + 'password' => 'TOTALLY-DIFFERENT-awesome-password!!!!!12345', ]); - $this->assertDatabaseHas('users', [ - 'id' => $user->id, - 'username' => $user->username, - 'email' => $user->email, - 'activated' => $user->activated, - 'password' => $hashed_original, - ]); - $this->assertEquals('brandnewuser', $user->refresh()->username); - $this->assertEquals('brandnewemail@example.org', $user->refresh()->email); - $this->assertEquals(1, $user->refresh()->activated); - $this->assertNotEquals(Hash::check('super-secret', $user->password), $user->refresh()->password); - $this->assertNotEquals('testnewusername', $user->refresh()->username); - $this->assertNotEquals('testnewemail@example.org', $user->refresh()->email); - $this->assertNotEquals(0, $user->refresh()->activated); - $this->assertNotEquals(Hash::check('super-secret', $user->password), $user->refresh()->password); + $this->assertEquals('TestAdminUser', $admin->refresh()->username); + $this->assertEquals('admin@example.org', $admin->refresh()->email); + $this->assertEquals(1, $admin->refresh()->activated); + $this->assertNotEquals(Hash::check('super-secret', $admin->password), $admin->refresh()->password); + $this->assertNotEquals('testnewusername', $admin->refresh()->username); + $this->assertNotEquals('testnewemail@example.org', $admin->refresh()->email); + $this->assertNotEquals(0, $admin->refresh()->activated); + $this->assertNotEquals(Hash::check('TOTALLY-DIFFERENT-awesome-password!!!!!12345', $admin->password), $admin->refresh()->password); } public function testAdminUsersCannotEditFieldsForSuperAdmins() { $admin = User::factory()->admin()->create(['activated' => true]); - $hashed_original = Hash::make('my-awesome-password'); - $user = User::factory()->superuser()->create(['username' => 'brandnewuser', 'email'=> 'brandnewemail@example.org', 'password' => $hashed_original, 'activated' => true]); + $hashed_original = Hash::make('my-awesome-password!!!!!12345'); + $superuser = User::factory()->superuser()->create(['username' => 'TestSuperUser', 'email'=> 'superuser@example.org', 'password' => $hashed_original, 'activated' => true]); $this->assertDatabaseHas('users', [ - 'id' => $user->id, - 'username' => 'brandnewuser', - 'email' => 'brandnewemail@example.org', + 'id' => $superuser->id, + 'username' => 'TestSuperUser', + 'email' => 'superuser@example.org', 'activated' => 1, 'password' => $hashed_original, ]); $this->actingAs($admin) - ->put(route('users.update', $user), [ + ->put(route('users.update', $superuser), [ 'username' => 'testnewusername', 'email' => 'testnewemail@example.org', 'activated' => 0, @@ -172,20 +164,20 @@ class UpdateUserTest extends TestCase ]); $this->assertDatabaseHas('users', [ - 'id' => $user->id, - 'username' => $user->username, - 'email' => $user->email, - 'activated' => $user->activated, + 'id' => $superuser->id, + 'username' => $superuser->username, + 'email' => $superuser->email, + 'activated' => $superuser->activated, 'password' => $hashed_original, ]); - $this->assertEquals('brandnewuser', $user->refresh()->username); - $this->assertEquals('brandnewemail@example.org', $user->refresh()->email); - $this->assertEquals(1, $user->refresh()->activated); - $this->assertTrue(Hash::check('my-awesome-password', $user->password), $user->refresh()->password); - $this->assertNotEquals('testnewusername', $user->refresh()->username); - $this->assertNotEquals('testnewemail@example.org', $user->refresh()->email); - $this->assertNotTrue(Hash::check('super-secret-new-password', $user->password), $user->refresh()->password); + $this->assertEquals('TestSuperUser', $superuser->refresh()->username); + $this->assertEquals('superuser@example.org', $superuser->refresh()->email); + $this->assertEquals(1, $superuser->refresh()->activated); + $this->assertTrue(Hash::check('my-awesome-password!!!!!12345', $superuser->password), $superuser->refresh()->password); + $this->assertNotEquals('testnewusername', $superuser->refresh()->username); + $this->assertNotEquals('testnewemail@example.org', $superuser->refresh()->email); + $this->assertNotTrue(Hash::check('super-secret-new-password', $superuser->password), $superuser->refresh()->password); }