diff --git a/app/Http/Controllers/User/UserController.php b/app/Http/Controllers/User/UserController.php index 054668b7c..ff6b57fe3 100644 --- a/app/Http/Controllers/User/UserController.php +++ b/app/Http/Controllers/User/UserController.php @@ -151,17 +151,25 @@ public function edit($id) */ public function update(UserRequest $request, $id) { + $actor = Auth::user(); + $isSuperAdmin = $actor->roles->contains('name', 'super-admin'); + if (!$isSuperAdmin && $id != $actor->id) { + abort(403, 'Anda tidak berhak mengubah user tersebut'); + } + try { $input = $request->validated(); + if ($id != $actor->id) { + unset($input['email']); + } $user = User::findOrFail($id); $this->handleFileUpload($request, $input, 'image', 'user', false); $user->update($input); - if (!empty($request->role)) { + if ($isSuperAdmin && !empty($request->role)) { $roles = $request->input('role') ? $request->input('role') : []; $user->syncRoles($roles); } - $user = Auth::user(); - if (!$user->roles->contains('name', 'super-admin')) { + if (!$isSuperAdmin) { return redirect()->route('dashboard')->with('success', 'User berhasil diperbarui!'); } return redirect()->route('setting.user.index')->with('success', 'User berhasil diperbarui!'); diff --git a/catatan_rilis.md b/catatan_rilis.md index a84a419b7..24b20dfcb 100644 --- a/catatan_rilis.md +++ b/catatan_rilis.md @@ -11,8 +11,15 @@ Terimakasih [isi disini] yang telah berkontribusi langsung mengembangkan aplikas 1. [#1771](https://github.com/OpenSID/OpenDK/issues/1771) Administrator utama bisa mengubah email hanya untuk akunnya sendiri. 2. [#1770](https://github.com/OpenSID/OpenDK/issues/1770) Perbaiki foto pengurus tidak tersimpan saat tambah data baru. 3. [#1777](https://github.com/OpenSID/OpenDK/issues/1777) Sembunyikan identitas pelapor dan NIK pada API komplain. -4. [#1781](https://github.com/OpenSID/OpenDK/issues/1781) Reflected XSS di halaman kategori potensi. + #### TEKNIS -1. [#1774](https://github.com/OpenSID/OpenDK/issues/1774) Konsolidasi: lebarkan kolom id ke BIGINT dan perintah geser id per desa. \ No newline at end of file +1. [#1774](https://github.com/OpenSID/OpenDK/issues/1774) Konsolidasi: lebarkan kolom id ke BIGINT dan perintah geser id per desa. + + +#### KEAMANAN + +1. [#1781](https://github.com/OpenSID/OpenDK/issues/1781) Reflected XSS di halaman kategori potensi. +2. [#1782](https://github.com/OpenSID/OpenDK/issues/1782)Update user tanpa cek kepemilikan dan role mentah dapat memberi super-admin. +3. [#1783](https://github.com/OpenSID/OpenDK/issues/1783) Pembatasan edit email user hanya di sisi klien. \ No newline at end of file diff --git a/routes/web.php b/routes/web.php index fcde690df..3bfd3b94f 100644 --- a/routes/web.php +++ b/routes/web.php @@ -893,9 +893,13 @@ Route::delete('permanent-destroy/{id}', 'permanentDestroy')->name('setting.user.permanent-destroy'); }); - Route::group(['prefix' => 'user', 'controller' => UserController::class, 'middleware' => ['action_permission:access.setting']], function () { + // Profil sendiri bisa diakses semua user yang login; kepemilikan dicek di UserController + Route::group(['prefix' => 'user', 'controller' => UserController::class], function () { Route::get('edit/{id}', 'edit')->name('setting.user.edit'); Route::put('update/{id}', 'update')->name('setting.user.update'); + }); + + Route::group(['prefix' => 'user', 'controller' => UserController::class, 'middleware' => ['action_permission:access.setting']], function () { Route::get('photo-profil/{id}', 'photo')->name('setting.user.photo'); Route::put('update-photo/{id}', 'updatePhoto')->name('setting.user.uphoto'); }); diff --git a/tests/Feature/Auth/AuthorizationTest.php b/tests/Feature/Auth/AuthorizationTest.php index 56880daa4..d2a6c8646 100644 --- a/tests/Feature/Auth/AuthorizationTest.php +++ b/tests/Feature/Auth/AuthorizationTest.php @@ -12,6 +12,7 @@ use App\Models\User; use Illuminate\Foundation\Testing\DatabaseTransactions; +use Spatie\Permission\Models\Permission; use Spatie\Permission\Models\Role; uses(DatabaseTransactions::class); @@ -21,7 +22,10 @@ // Ensure roles exist $this->adminRole = Role::firstOrCreate(['name' => 'super-admin']); - $this->kecamatanRole = Role::firstOrCreate(['name' => 'admin-kecamatan']); + $this->kecamatanRole = Role::firstOrCreate(['name' => 'admin-kecamatan', 'guard_name' => 'web']); + $this->kecamatanRole->syncPermissions([ + Permission::firstOrCreate(['name' => 'access.dashboard', 'guard_name' => 'web']), + ]); $this->admin = User::factory()->create(['status' => 1]); $this->admin->assignRole($this->adminRole); diff --git a/tests/Feature/Settings/UserManagementCrudTest.php b/tests/Feature/Settings/UserManagementCrudTest.php index c30561d27..863629968 100644 --- a/tests/Feature/Settings/UserManagementCrudTest.php +++ b/tests/Feature/Settings/UserManagementCrudTest.php @@ -147,6 +147,8 @@ $targetUser = User::where('id', '!=', $superAdmin->id)->first(); $role = Role::where('name', 'administrator-website')->first(); + $originalEmail = $targetUser->email; + $updateData = [ 'name' => 'Updated Name', 'email' => 'updated@test.com', @@ -160,10 +162,11 @@ $response->assertRedirect(route('setting.user.index')); $response->assertSessionHas('success'); + // Email user lain tidak boleh diubah (lihat #1783) $this->assertDatabaseHas('users', [ 'id' => $targetUser->id, 'name' => 'Updated Name', - 'email' => 'updated@test.com', + 'email' => $originalEmail, ]); }); diff --git a/tests/Feature/UserControllerTest.php b/tests/Feature/UserControllerTest.php index 38669f86f..8f86b2c7b 100644 --- a/tests/Feature/UserControllerTest.php +++ b/tests/Feature/UserControllerTest.php @@ -253,6 +253,8 @@ public function test_update_updates_user_successfully_for_super_admin() 'role' => [$role->name], ]; + $originalEmail = $targetUser->email; + $response = $this->actingAs($superAdmin)->put(route('setting.user.update', $targetUser->id), $updateData); $response->assertRedirect(route('setting.user.index')); @@ -261,7 +263,61 @@ public function test_update_updates_user_successfully_for_super_admin() $this->assertDatabaseHas('users', [ 'id' => $targetUser->id, 'name' => 'Updated Name', - 'email' => $updateData['email'], + 'email' => $originalEmail, + ]); + } + + /** + * Test update method lets super admin change email of own account. + * + * @return void + */ + public function test_update_allows_super_admin_to_change_own_email() + { + $superAdmin = User::whereHas('roles', function ($query) { + $query->where('name', 'super-admin'); + })->first(); + + $newEmail = $this->faker->unique()->safeEmail; + + $response = $this->actingAs($superAdmin)->put(route('setting.user.update', $superAdmin->id), [ + 'name' => $superAdmin->name, + 'email' => $newEmail, + 'address' => 'Alamat Super Admin', + ]); + + $response->assertSessionHas('success'); + $this->assertDatabaseHas('users', [ + 'id' => $superAdmin->id, + 'email' => $newEmail, + ]); + } + + /** + * Test update method ignores email change when super admin edits another user. + * + * @return void + */ + public function test_update_ignores_email_change_when_editing_another_user() + { + $superAdmin = User::whereHas('roles', function ($query) { + $query->where('name', 'super-admin'); + })->first(); + + $targetEmail = $this->faker->unique()->safeEmail; + $targetUser = User::factory()->create(['email' => $targetEmail]); + + $response = $this->actingAs($superAdmin)->put(route('setting.user.update', $targetUser->id), [ + 'name' => 'Nama Baru', + 'email' => $this->faker->unique()->safeEmail, + 'address' => 'Alamat Baru', + ]); + + $response->assertSessionHas('success'); + $this->assertDatabaseHas('users', [ + 'id' => $targetUser->id, + 'name' => 'Nama Baru', + 'email' => $targetEmail, ]); } @@ -296,6 +352,167 @@ public function test_update_updates_profile_and_redirects_to_dashboard_for_regul ]); } + /** + * Test update method forbids non super admin from updating another user. + * + * @return void + */ + public function test_update_forbids_non_super_admin_from_updating_another_user() + { + $actor = User::factory()->create(); + $actor->assignRole('administrator-website'); + $actor->givePermissionTo('access.setting'); + + $targetEmail = $this->faker->unique()->safeEmail; + $targetUser = User::factory()->create(['email' => $targetEmail]); + + $response = $this->actingAs($actor)->put(route('setting.user.update', $targetUser->id), [ + 'name' => 'Hacked Name', + 'email' => $this->faker->unique()->safeEmail, + 'address' => 'Hacked Address', + ]); + + $response->assertForbidden(); + + $this->assertDatabaseHas('users', [ + 'id' => $targetUser->id, + 'email' => $targetEmail, + ]); + } + + /** + * Test update method lets non super admin change email of own profile. + * + * @return void + */ + public function test_update_allows_non_super_admin_to_change_own_email() + { + $actor = User::factory()->create(); + $actor->assignRole('administrator-website'); + $actor->givePermissionTo('access.setting'); + + $newEmail = $this->faker->unique()->safeEmail; + + $response = $this->actingAs($actor)->put(route('setting.user.update', $actor->id), [ + 'name' => $actor->name, + 'email' => $newEmail, + 'address' => 'Alamat Sendiri', + ]); + + $response->assertRedirect(route('dashboard')); + $this->assertDatabaseHas('users', [ + 'id' => $actor->id, + 'email' => $newEmail, + ]); + } + + /** + * Test update method ignores role from non super admin editing own profile. + * + * @return void + */ + public function test_update_ignores_role_from_non_super_admin_editing_own_profile() + { + $actor = User::factory()->create(); + $actor->assignRole('administrator-website'); + $actor->givePermissionTo('access.setting'); + + $response = $this->actingAs($actor)->put(route('setting.user.update', $actor->id), [ + 'name' => 'Self Name', + 'email' => $actor->email, + 'address' => 'Self Address', + 'role' => ['super-admin'], + ]); + + $response->assertRedirect(route('dashboard')); + + $actor->refresh(); + $this->assertSame('Self Name', $actor->name); + $this->assertFalse($actor->hasRole('super-admin')); + $this->assertTrue($actor->hasRole('administrator-website')); + } + + /** + * Test user without access.setting can open edit form for own profile. + * + * @return void + */ + public function test_edit_displays_own_profile_without_access_setting_permission() + { + $actor = $this->createUserWithoutSettingAccess(); + + $response = $this->actingAs($actor)->get(route('setting.user.edit', $actor->id)); + + $response->assertStatus(200); + $response->assertViewIs('user.edit'); + $response->assertViewHas('user', $actor); + } + + /** + * Test user without access.setting can update own profile. + * + * @return void + */ + public function test_update_own_profile_without_access_setting_permission() + { + $actor = $this->createUserWithoutSettingAccess(); + + $response = $this->actingAs($actor)->put(route('setting.user.update', $actor->id), [ + 'name' => 'Profil Sendiri', + 'email' => $actor->email, + 'address' => 'Alamat Sendiri', + ]); + + $response->assertRedirect(route('dashboard')); + $this->assertDatabaseHas('users', [ + 'id' => $actor->id, + 'name' => 'Profil Sendiri', + ]); + } + + /** + * Test user without access.setting cannot open edit form for another user. + * + * @return void + */ + public function test_edit_forbids_another_user_without_access_setting_permission() + { + $actor = $this->createUserWithoutSettingAccess(); + $targetUser = User::factory()->create(); + + $response = $this->actingAs($actor)->get(route('setting.user.edit', $targetUser->id)); + + $response->assertForbidden(); + } + + /** + * Test user without access.setting still cannot open user management list. + * + * @return void + */ + public function test_index_forbids_user_without_access_setting_permission() + { + $actor = $this->createUserWithoutSettingAccess(); + + $response = $this->actingAs($actor)->get(route('setting.user.index')); + + $response->assertForbidden(); + } + + /** + * Create a user whose role has no access.setting permission, like admin-kecamatan. + * + * @return User + */ + private function createUserWithoutSettingAccess() + { + $role = Role::firstOrCreate(['name' => 'admin-kecamatan', 'guard_name' => 'web']); + $actor = User::factory()->create(); + $actor->assignRole($role); + + return $actor; + } + /** * Test destroy method deactivates user successfully. *