Skip to content

Commit 6a59119

Browse files
snipesnipe
authored
Merge pull request #18303 from grokability/update-users-api-with-disallowed-fields-list
Update users api with disallowed fields list
2 parents 412f4c6 + 8b4387e commit 6a59119

File tree

3 files changed

+159
-111
lines changed

3 files changed

+159
-111
lines changed

‎app/Http/Controllers/Api/UsersController.php‎

Lines changed: 67 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -522,93 +522,99 @@ public function update(SaveUserRequest $request, User $user): JsonResponse
522522
{
523523
$this->authorize('update', User::class);
524524

525-
$this->authorize('update', $user);
525+
$this->authorize('update', $user);
526526

527-
/**
528-
* This is a janky hack to prevent people from changing admin demo user data on the public demo.
529-
* The $ids 1 and 2 are special since they are seeded as superadmins in the demo seeder.
530-
* Thanks, jerks. You are why we can't have nice things. - snipe
531-
*
532-
*/
527+
/**
528+
* This is a janky hack to prevent people from changing admin demo user data on the public demo.
529+
* The $ids 1 and 2 are special since they are seeded as superadmins in the demo seeder.
530+
* Thanks, jerks. You are why we can't have nice things. - snipe
531+
*
532+
*/
533533

534534
if ((($user->id == 1) || ($user->id == 2)) && (config('app.lock_passwords'))) {
535-
return response()->json(Helper::formatStandardApiResponse('error', null, 'Permission denied. You cannot update user information via API on the demo.'));
536-
}
535+
return response()->json(Helper::formatStandardApiResponse('error', null, 'Permission denied. You cannot update user information via API on the demo.'));
536+
}
537+
538+
// Pull out sensitive fields that require extra permission
539+
$user->fill($request->except(['password', 'username', 'email', 'activated', 'permissions', 'activation_code', 'remember_token', 'two_factor_secret', 'two_factor_enrolled', 'two_factor_optin']));
540+
537541

538-
$user->fill($request->all());
542+
if (auth()->user()->can('canEditAuthFields', $user) && auth()->user()->can('editableOnDemo')) {
539543

540-
if ($request->filled('company_id')) {
541-
$user->company_id = Company::getIdForCurrentUser($request->input('company_id'));
544+
if ($request->filled('password')) {
545+
$user->password = bcrypt($request->input('password'));
542546
}
543547

544-
if ($user->id == $request->input('manager_id')) {
545-
return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager'));
548+
if ($request->filled('username')) {
549+
$user->username = $request->input('username');
546550
}
547551

548-
// check for permissions related fields and pull them out if the current user cannot edit them
549-
if (auth()->user()->can('canEditAuthFields', $user) && auth()->user()->can('editableOnDemo')) {
552+
if ($request->filled('email')) {
553+
$user->email = $request->input('email');
554+
}
550555

551-
if ($request->filled('password')) {
552-
$user->password = bcrypt($request->input('password'));
553-
}
556+
if ($request->filled('activated')) {
557+
$user->activated = $request->input('activated');
558+
}
554559

555-
if ($request->filled('username')) {
556-
$user->username = $request->input('username');
557-
}
560+
}
558561

559-
if ($request->filled('display_name')) {
560-
$user->display_name = $request->input('display_name');
561-
}
562+
// We need to use has() instead of filled()
563+
// here because we need to overwrite permissions
564+
// if someone needs to null them out
562565

563-
if ($request->filled('email')) {
564-
$user->email = $request->input('email');
565-
}
566+
if ($request->filled('display_name')) {
567+
$user->display_name = $request->input('display_name');
568+
}
566569

567-
if ($request->filled('activated')) {
568-
$user->activated = $request->input('activated');
569-
}
570+
if ($request->filled('company_id')) {
571+
$user->company_id = Company::getIdForCurrentUser($request->input('company_id'));
572+
}
570573

571-
}
574+
if ($user->id == $request->input('manager_id')) {
575+
return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager'));
576+
}
572577

573-
// We need to use has() instead of filled()
574-
// here because we need to overwrite permissions
575-
// if someone needs to null them out
576-
if ($request->has('permissions')) {
577-
$permissions_array = $request->input('permissions');
578578

579-
// Strip out the individual superuser permission if the API user isn't a superadmin
580-
if (!auth()->user()->isSuperUser()) {
581-
unset($permissions_array['superuser']);
582-
}
579+
580+
if ($request->has('permissions')) {
581+
$permissions_array = $request->input('permissions');
583582

584-
$user->permissions = $permissions_array;
583+
// Strip out the individual superuser permission if the API user isn't a superadmin
584+
if (!auth()->user()->isSuperUser()) {
585+
unset($permissions_array['superuser']);
585586
}
586587

587-
if($request->has('location_id')) {
588-
// Update the location of any assets checked out to this user
589-
Asset::where('assigned_type', User::class)
590-
->where('assigned_to', $user->id)->update(['location_id' => $request->input('location_id', null)]);
591-
}
592-
app('App\Http\Requests\ImageUploadRequest')->handleImages($user, 600, 'avatar', 'avatars', 'avatar');
588+
$user->permissions = $permissions_array;
589+
}
590+
591+
if ($request->has('location_id')) {
592+
// Update the location of any assets checked out to this user
593+
Asset::where('assigned_type', User::class)
594+
->where('assigned_to', $user->id)->update(['location_id' => $request->input('location_id', null)]);
595+
}
593596

594-
if ($user->save()) {
595-
// Check if the request has groups passed and has a value, AND that the user us a superuser
596-
if (($request->has('groups')) && (auth()->user()->isSuperUser())) {
597597

598-
$validator = Validator::make($request->only('groups'), [
599-
'groups.*' => 'integer|exists:permission_groups,id',
600-
]);
598+
app('App\Http\Requests\ImageUploadRequest')->handleImages($user, 600, 'avatar', 'avatars', 'avatar');
601599

602-
if ($validator->fails()) {
603-
return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors()));
604-
}
600+
if ($user->save()) {
601+
// Check if the request has groups passed and has a value, AND that the user us a superuser
602+
if (($request->has('groups')) && (auth()->user()->isSuperUser())) {
603+
604+
$validator = Validator::make($request->only('groups'), [
605+
'groups.*' => 'integer|exists:permission_groups,id',
606+
]);
605607

606-
// Sync the groups since the user is a superuser and the groups pass validation
607-
$user->groups()->sync($request->input('groups'));
608+
if ($validator->fails()) {
609+
return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors()));
608610
}
609-
return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update')));
611+
612+
// Sync the groups since the user is a superuser and the groups pass validation
613+
$user->groups()->sync($request->input('groups'));
610614
}
611-
return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
615+
return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update')));
616+
}
617+
return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
612618
}
613619

614620
/**

‎tests/Feature/Users/Api/UpdateUserTest.php‎

Lines changed: 58 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -220,29 +220,79 @@ public function testEditingUsersCannotEditEscalationFieldsForAdmins()
220220
{
221221
$hashed_original = Hash::make('!!094850394680980380kfejlskjfl');
222222
$hashed_new = Hash::make('!ABCDEFGIJKL123!!!');
223-
$admin = User::factory()->editUsers()->create();
224-
$user = User::factory()->admin()->create(['username' => 'brandnewuser', 'email'=> '[email protected]', 'password' => $hashed_original, 'activated' => 1]);
223+
224+
$editing_user = User::factory()->editUsers()->create();
225+
$adminuser = User::factory()->admin()->create(['username' => 'TestAdminUser', 'email'=> '[email protected]', 'password' => $hashed_original, 'activated' => 1]);
225226

226227

228+
// The admin being edited
227229
$this->assertDatabaseHas('users', [
228-
'id' => $user->id,
229-
'username' => 'brandnewuser',
230-
'email' => 'brandnewemail@example.org',
230+
'id' => $adminuser->id,
231+
'username' => 'TestAdminUser',
232+
'email' => 'admin@example.org',
231233
'activated' => 1,
232234
'password' => $hashed_original,
235+
'permissions' => '{"admin":"1"}',
233236
]);
234237

235-
$this->actingAsForApi($admin)
236-
->patch(route('api.users.update', $user), [
238+
$this->actingAsForApi($editing_user)
239+
->patch(route('api.users.update', $adminuser), [
237240
'username' => 'testnewusername',
238241
'email' => '[email protected]',
239242
'activated' => 0,
243+
'permissions' => "{'superadmin':1}",
240244
'password' => $hashed_new,
241245
]);
242246

243-
$this->assertEquals(0, $user->refresh()->activated);
247+
// These should keep their old values
248+
$this->assertEquals('TestAdminUser', $adminuser->refresh()->username);
249+
$this->assertEquals('[email protected]', $adminuser->refresh()->email);
250+
$this->assertEquals(1, $adminuser->refresh()->activated);
251+
$this->assertEquals($hashed_original, $adminuser->refresh()->password);
252+
$this->assertEquals('{"admin":"1"}', $adminuser->refresh()->permissions);
244253

245254
}
255+
256+
public function testAdminsCannotEditEscalationFieldsForSuperadmins()
257+
{
258+
$hashed_original = Hash::make('my-awesome-password!!!!!12345');
259+
$hashed_new = Hash::make('!ABCDEFGIJKL123!!!');
260+
261+
$editing_user = User::factory()->admin()->create();
262+
$superuser = User::factory()->superuser()->create(['username' => 'TestSuperUser', 'email'=> '[email protected]', 'password' => $hashed_original, 'activated' => 1]);
263+
264+
265+
// The admin being edited
266+
$this->assertDatabaseHas('users', [
267+
'id' => $superuser->id,
268+
'username' => 'TestSuperUser',
269+
'email' => '[email protected]',
270+
'activated' => 1,
271+
'password' => $hashed_original,
272+
'permissions' => '{"superuser":"1"}',
273+
]);
274+
275+
$this->actingAsForApi($editing_user)
276+
->patch(route('api.users.update', $superuser), [
277+
'username' => 'testnewusername',
278+
'email' => '[email protected]',
279+
'activated' => 0,
280+
'permissions' => "{'superadmin':1}",
281+
'password' => $hashed_new,
282+
]);
283+
284+
// These should keep their old values
285+
$this->assertEquals('TestSuperUser', $superuser->refresh()->username);
286+
$this->assertEquals('[email protected]', $superuser->refresh()->email);
287+
$this->assertEquals(1, $superuser->refresh()->activated);
288+
$this->assertEquals($hashed_original, $superuser->refresh()->password);
289+
$this->assertEquals('{"superuser":"1"}', $superuser->refresh()->permissions);
290+
$this->assertNotEquals('testnewusername', $superuser->refresh()->username);
291+
$this->assertNotEquals('[email protected]', $superuser->refresh()->email);
292+
$this->assertNotTrue(Hash::check('super-secret-new-password', $superuser->password), $superuser->refresh()->password);
293+
294+
}
295+
246296
public function testUsersScopedToCompanyDuringUpdateWhenMultipleFullCompanySupportEnabled()
247297
{
248298
$this->settings->enableMultipleFullCompanySupport();

‎tests/Feature/Users/Ui/UpdateUserTest.php‎

Lines changed: 34 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -110,82 +110,74 @@ public function testUsersUpdatingThemselvesDoNotDeactivateTheirAccount()
110110

111111
public function testEditingUsersCannotEditEscalationFieldsForAdmins()
112112
{
113-
$admin = User::factory()->editUsers()->create(['activated' => true]);
114-
$hashed_original = Hash::make('!!094850394680980380kfejlskjfl');
115-
$hashed_new = Hash::make('!ABCDEFGIJKL123!!!');
116-
$user = User::factory()->admin()->create(['username' => 'brandnewuser', 'email'=> '[email protected]', 'password' => $hashed_original, 'activated' => true]);
113+
$editing_user = User::factory()->editUsers()->create(['activated' => true]);
114+
$hashed_original = Hash::make('my-awesome-password!!!!!12345');
115+
$admin = User::factory()->admin()->create(['username' => 'TestAdminUser', 'email'=> '[email protected]', 'password' => $hashed_original, 'activated' => true]);
117116

118117
$this->assertDatabaseHas('users', [
119-
'id' => $user->id,
120-
'username' => 'brandnewuser',
121-
'email' => 'brandnewemail@example.org',
118+
'id' => $admin->id,
119+
'username' => 'TestAdminUser',
120+
'email' => 'admin@example.org',
122121
'activated' => 1,
123122
'password' => $hashed_original,
124123
]);
125124

126-
$this->actingAs($admin)
127-
->put(route('users.update', $user), [
125+
$this->actingAs($editing_user)
126+
->put(route('users.update', $admin), [
128127
'username' => 'testnewusername',
129128
'email' => '[email protected]',
130129
'activated' => 0,
131-
'password' => 'super-secret',
130+
'password' => 'TOTALLY-DIFFERENT-awesome-password!!!!!12345',
132131
]);
133132

134-
$this->assertDatabaseHas('users', [
135-
'id' => $user->id,
136-
'username' => $user->username,
137-
'email' => $user->email,
138-
'activated' => $user->activated,
139-
'password' => $hashed_original,
140-
]);
141133

142-
$this->assertEquals('brandnewuser', $user->refresh()->username);
143-
$this->assertEquals('brandnewemail@example.org', $user->refresh()->email);
144-
$this->assertEquals(1, $user->refresh()->activated);
145-
$this->assertNotEquals(Hash::check('super-secret', $user->password), $user->refresh()->password);
146-
$this->assertNotEquals('testnewusername', $user->refresh()->username);
147-
$this->assertNotEquals('[email protected]', $user->refresh()->email);
148-
$this->assertNotEquals(0, $user->refresh()->activated);
149-
$this->assertNotEquals(Hash::check('super-secret', $user->password), $user->refresh()->password);
134+
$this->assertEquals('TestAdminUser', $admin->refresh()->username);
135+
$this->assertEquals('admin@example.org', $admin->refresh()->email);
136+
$this->assertEquals(1, $admin->refresh()->activated);
137+
$this->assertNotEquals(Hash::check('super-secret', $admin->password), $admin->refresh()->password);
138+
$this->assertNotEquals('testnewusername', $admin->refresh()->username);
139+
$this->assertNotEquals('[email protected]', $admin->refresh()->email);
140+
$this->assertNotEquals(0, $admin->refresh()->activated);
141+
$this->assertNotEquals(Hash::check('TOTALLY-DIFFERENT-awesome-password!!!!!12345', $admin->password), $admin->refresh()->password);
150142
}
151143

152144
public function testAdminUsersCannotEditFieldsForSuperAdmins()
153145
{
154146
$admin = User::factory()->admin()->create(['activated' => true]);
155-
$hashed_original = Hash::make('my-awesome-password');
156-
$user = User::factory()->superuser()->create(['username' => 'brandnewuser', 'email'=> 'brandnewemail@example.org', 'password' => $hashed_original, 'activated' => true]);
147+
$hashed_original = Hash::make('my-awesome-password!!!!!12345');
148+
$superuser = User::factory()->superuser()->create(['username' => 'TestSuperUser', 'email'=> 'superuser@example.org', 'password' => $hashed_original, 'activated' => true]);
157149

158150
$this->assertDatabaseHas('users', [
159-
'id' => $user->id,
160-
'username' => 'brandnewuser',
161-
'email' => 'brandnewemail@example.org',
151+
'id' => $superuser->id,
152+
'username' => 'TestSuperUser',
153+
'email' => 'superuser@example.org',
162154
'activated' => 1,
163155
'password' => $hashed_original,
164156
]);
165157

166158
$this->actingAs($admin)
167-
->put(route('users.update', $user), [
159+
->put(route('users.update', $superuser), [
168160
'username' => 'testnewusername',
169161
'email' => '[email protected]',
170162
'activated' => 0,
171163
'password' => 'super-secret-new-password',
172164
]);
173165

174166
$this->assertDatabaseHas('users', [
175-
'id' => $user->id,
176-
'username' => $user->username,
177-
'email' => $user->email,
178-
'activated' => $user->activated,
167+
'id' => $superuser->id,
168+
'username' => $superuser->username,
169+
'email' => $superuser->email,
170+
'activated' => $superuser->activated,
179171
'password' => $hashed_original,
180172
]);
181173

182-
$this->assertEquals('brandnewuser', $user->refresh()->username);
183-
$this->assertEquals('brandnewemail@example.org', $user->refresh()->email);
184-
$this->assertEquals(1, $user->refresh()->activated);
185-
$this->assertTrue(Hash::check('my-awesome-password', $user->password), $user->refresh()->password);
186-
$this->assertNotEquals('testnewusername', $user->refresh()->username);
187-
$this->assertNotEquals('[email protected]', $user->refresh()->email);
188-
$this->assertNotTrue(Hash::check('super-secret-new-password', $user->password), $user->refresh()->password);
174+
$this->assertEquals('TestSuperUser', $superuser->refresh()->username);
175+
$this->assertEquals('superuser@example.org', $superuser->refresh()->email);
176+
$this->assertEquals(1, $superuser->refresh()->activated);
177+
$this->assertTrue(Hash::check('my-awesome-password!!!!!12345', $superuser->password), $superuser->refresh()->password);
178+
$this->assertNotEquals('testnewusername', $superuser->refresh()->username);
179+
$this->assertNotEquals('[email protected]', $superuser->refresh()->email);
180+
$this->assertNotTrue(Hash::check('super-secret-new-password', $superuser->password), $superuser->refresh()->password);
189181
}
190182

191183

0 commit comments

Comments
 (0)