diff --git a/app/Console/Commands/UserCreate.php b/app/Console/Commands/UserCreate.php index fd08f1d5ee..2ebd83f65a 100644 --- a/app/Console/Commands/UserCreate.php +++ b/app/Console/Commands/UserCreate.php @@ -101,7 +101,6 @@ public function handle(DiscourseService $discourseService): void 'name' => $name, 'email' => $email, 'password' => Hash::make($password), - 'role' => $role, 'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24), 'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)), 'calendar_hash' => Str::random(15), @@ -118,6 +117,9 @@ public function handle(DiscourseService $discourseService): void if ($user) { + $user->role = $role; + $user->save(); + $this->info("User created {$user->name} (#{$user->id})"); $this->info("Role: {$this->getRoleName($user->role)}"); diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php index 1920b75796..ec8f1a46dc 100644 --- a/app/Http/Controllers/Auth/RegisterController.php +++ b/app/Http/Controllers/Auth/RegisterController.php @@ -5,6 +5,7 @@ use Illuminate\Routing\Controllers\HasMiddleware; use Illuminate\Routing\Controllers\Middleware; use App\Http\Controllers\Controller; +use App\Models\Role; use App\Models\User; use Illuminate\Foundation\Auth\RegistersUsers; use Illuminate\Support\Facades\Hash; @@ -62,11 +63,13 @@ protected function create(array $data): User 'name' => $data['name'], 'email' => $data['email'], 'password' => Hash::make($data['password']), - 'role' => 4, 'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24), 'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)), ]); + $user->role = Role::RESTARTER; + $user->save(); + Session::createSession($user->id); return $user; diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index fe174a0c8f..7885306162 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -149,10 +149,9 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc return redirect()->back()->withErrors($validator)->withInput(); } - if ($request->input('id') !== null) { - $id = $request->input('id'); - } else { - $id = Auth::id(); + $id = $request->input('id', Auth::id()); + if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { + abort(403); } User::find($id)->update([ @@ -188,10 +187,9 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc public function postProfilePasswordEdit(Request $request): RedirectResponse { - if ($request->input('id') !== null) { - $id = $request->input('id'); - } else { - $id = Auth::id(); + $id = $request->input('id', Auth::id()); + if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { + abort(403); } $user = User::find($id); @@ -346,10 +344,9 @@ public function postProfileTagsEdit(Request $request): RedirectResponse public function postProfilePictureEdit(Request $request): RedirectResponse { - if ($request->input('id') !== null) { - $id = $request->input('id'); - } else { - $id = Auth::id(); + $id = $request->input('id', Auth::id()); + if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { + abort(403); } if (isset($_FILES) && ! empty($_FILES)) { @@ -364,20 +361,19 @@ public function postProfilePictureEdit(Request $request): RedirectResponse public function postAdminEdit(Request $request): RedirectResponse { - if ($request->input('id') !== null) { - $user_id = $request->input('id'); - } else { - $user_id = Auth::id(); + if (!Auth::user()->hasRole('Administrator')) { + abort(403); } + $user_id = $request->input('id', Auth::id()); + $user = User::find($user_id); $oldRole = $user->role; // Set role for User - $user->update([ - 'role' => $request->input('user_role'), - ]); + $user->role = $request->input('user_role'); + $user->save(); // If we are demoting from NetworkCoordinator, remove them from the list of coordinators for // any networks they are currently coordinating. @@ -823,12 +819,15 @@ public function edit($id, Request $request) $Groups = new Group; $Groups = $Groups->findAll(); - $data = $request->post(); - if (! Fixometer::hasRole($User->find($id), 'Administrator')) { - $sent_groups = $data['groups']; + $sent_groups = $request->input('groups'); } + $data = $request->only([ + 'name', 'email', 'location', 'age', 'gender', 'country_code', + 'biography', 'language', 'newsletter', 'invites', + ]); + $error = false; // check for email in use $editingUser = $User->find($id); @@ -836,21 +835,14 @@ public function edit($id, Request $request) $error['email'] = 'The email you entered is already in use in our database. Please use another one.'; } - if (! empty($data['new-password'])) { - if ($data['new-password'] !== $data['password-confirm']) { + if (! empty($request->input('new-password'))) { + if ($request->input('new-password') !== $request->input('password-confirm')) { $error['password'] = 'The passwords are not identical!'; } else { - $data['password'] = Hash::make($data['new-password']); + $data['password'] = Hash::make($request->input('new-password')); } } - unset($data['new-password']); - unset($data['password-confirm']); - - unset($data['groups']); - unset($data['profile']); - unset($data['id']); - if (! is_array($error)) { $u = $User->find($id)->update($data); @@ -1015,7 +1007,6 @@ public function postRegister(Request $request, $hash = null): RedirectResponse 'name' => $request->input('name'), 'email' => $request->input('email'), 'password' => Hash::make($request->input('password')), - 'role' => $role, 'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24), 'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)), 'country_code' => $request->input('country'), @@ -1025,6 +1016,9 @@ public function postRegister(Request $request, $hash = null): RedirectResponse 'calendar_hash' => Str::random(15), 'username' => '', ]); + + $user->role = $role; + $user->save(); } $user->generateAndSetUsername(); diff --git a/app/Models/User.php b/app/Models/User.php index 9cd00516e5..2d34a32d2c 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -40,8 +40,11 @@ class User extends Authenticatable implements Auditable, HasLocalePreference * * @var array */ + // Note: `role` and `api_token` are intentionally excluded from $fillable. + // They must be set via direct assignment ($user->role = X; $user->save()) + // to prevent privilege escalation via mass assignment (security: C2/M1). protected $fillable = [ - 'name', 'email', 'password', 'role', 'recovery', 'recovery_expires', 'language', 'repair_network', 'location', 'age', 'gender', 'country_code', 'newsletter', 'invites', 'biography', 'consent_future_data', 'consent_past_data', 'consent_gdpr', 'number_of_logins', 'latitude', 'longitude', 'last_login_at', 'api_token', 'access_group_tag_id', 'calendar_hash', 'repairdir_role', 'mediawiki', 'username', 'external_user_id', 'external_username', + 'name', 'email', 'password', 'recovery', 'recovery_expires', 'language', 'repair_network', 'location', 'age', 'gender', 'country_code', 'newsletter', 'invites', 'biography', 'consent_future_data', 'consent_past_data', 'consent_gdpr', 'number_of_logins', 'latitude', 'longitude', 'last_login_at', 'access_group_tag_id', 'calendar_hash', 'repairdir_role', 'mediawiki', 'username', 'external_user_id', 'external_username', ]; /** diff --git a/tests/Feature/Users/PrivilegeEscalationTest.php b/tests/Feature/Users/PrivilegeEscalationTest.php new file mode 100644 index 0000000000..9a7ed4774d --- /dev/null +++ b/tests/Feature/Users/PrivilegeEscalationTest.php @@ -0,0 +1,179 @@ +restarter()->create(); + $victim = User::factory()->restarter()->create(); + + $this->actingAs($attacker); + + $response = $this->post('/profile/edit-info', [ + 'id' => $victim->id, + 'email' => $victim->email, + 'name' => 'Hacked Name', + 'country' => $victim->country_code, + 'townCity' => $victim->location, + 'age' => $victim->age, + 'gender' => $victim->gender, + 'biography' => 'hacked', + ]); + + $response->assertStatus(403); + $this->assertNotEquals('Hacked Name', $victim->fresh()->name); + } + + /** + * C1: Restarter cannot change another user's password. + */ + public function test_restarter_cannot_change_another_users_password(): void + { + $attacker = User::factory()->restarter()->create(); + $victim = User::factory()->restarter()->create(); + + $this->actingAs($attacker); + + $response = $this->post('/profile/edit-password', [ + 'id' => $victim->id, + 'current-password' => 'secret', + 'new-password' => 'hacked123', + 'new-password-repeat' => 'hacked123', + ]); + + $response->assertStatus(403); + } + + /** + * C1: Restarter cannot change another user's photo. + */ + public function test_restarter_cannot_change_another_users_photo(): void + { + $attacker = User::factory()->restarter()->create(); + $victim = User::factory()->restarter()->create(); + + $this->actingAs($attacker); + + $response = $this->post('/profile/edit-photo', [ + 'id' => $victim->id, + ]); + + $response->assertStatus(403); + } + + /** + * C1: Restarter cannot access admin edit settings. + */ + public function test_restarter_cannot_access_admin_edit_settings(): void + { + $attacker = User::factory()->restarter()->create(); + + $this->actingAs($attacker); + + $response = $this->post('/profile/edit-admin-settings', [ + 'id' => $attacker->id, + 'user_role' => Role::ROOT, + ]); + + $response->assertStatus(403); + $this->assertEquals(Role::RESTARTER, $attacker->fresh()->role); + } + + /** + * C1 positive: Admin CAN change role via admin edit settings. + */ + public function test_admin_can_change_role_via_admin_edit(): void + { + $admin = User::factory()->administrator()->create(); + $user = User::factory()->restarter()->create(); + + $this->actingAs($admin); + + $response = $this->post('/profile/edit-admin-settings', [ + 'id' => $user->id, + 'user_role' => Role::HOST, + 'assigned_groups' => [], + 'preferences' => [], + 'permissions' => [], + ]); + + $response->assertSessionHas('message'); + $this->assertEquals(Role::HOST, $user->fresh()->role); + } + + /** + * C1 positive: Admin CAN edit another user's info. + */ + public function test_admin_can_edit_another_users_info(): void + { + $admin = User::factory()->administrator()->create(); + $user = User::factory()->restarter()->create(); + + $this->actingAs($admin); + + $response = $this->post('/profile/edit-info', [ + 'id' => $user->id, + 'email' => $user->email, + 'name' => 'Updated By Admin', + 'country' => $user->country_code, + 'townCity' => $user->location, + 'age' => $user->age, + 'gender' => $user->gender, + 'biography' => $user->biography, + ]); + + $response->assertRedirect(); + $this->assertEquals('Updated By Admin', $user->fresh()->name); + } + + /** + * C2/M1: User cannot escalate role via /user/edit endpoint. + */ + public function test_user_cannot_escalate_role_via_edit_endpoint(): void + { + $admin = User::factory()->administrator()->create(); + $user = User::factory()->restarter()->create(); + + $this->actingAs($admin); + + $response = $this->post("/user/edit/{$user->id}", [ + 'name' => $user->name, + 'email' => $user->email, + 'role' => Role::ROOT, + ]); + + $this->assertNotEquals(Role::ROOT, $user->fresh()->role); + } + + /** + * C2/M1: User cannot overwrite api_token via /user/edit endpoint. + */ + public function test_user_cannot_overwrite_api_token_via_edit_endpoint(): void + { + $admin = User::factory()->administrator()->create(); + $user = User::factory()->restarter()->create(); + $originalToken = $user->api_token; + + $this->actingAs($admin); + + $response = $this->post("/user/edit/{$user->id}", [ + 'name' => $user->name, + 'email' => $user->email, + 'api_token' => 'stolen_token_value', + ]); + + $this->assertEquals($originalToken, $user->fresh()->api_token); + } +}