From 3d9d18a0d56e597c9eb1aecbe231294f75a16cfb Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 19:22:23 +0100 Subject: [PATCH 01/29] Fixed weird CSS quirk Signed-off-by: snipe --- public/css/build/app.css | 3 +++ public/css/build/overrides.css | 3 +++ public/css/dist/all.css | 6 ++++++ public/mix-manifest.json | 6 +++--- resources/assets/less/overrides.less | 5 +++++ 5 files changed, 20 insertions(+), 3 deletions(-) diff --git a/public/css/build/app.css b/public/css/build/app.css index f139c052cf..b6c2073aeb 100644 --- a/public/css/build/app.css +++ b/public/css/build/app.css @@ -1429,6 +1429,9 @@ input[type="radio"]:checked::before { .bootstrap-table .fixed-table-container .table tbody tr .card-view { display: table-row !important; } +.form-control-static { + padding-top: 0px; +} td.text-right.text-padding-number-cell { padding-right: 30px !important; white-space: nowrap; diff --git a/public/css/build/overrides.css b/public/css/build/overrides.css index aa8e28acbb..34a882ff8b 100644 --- a/public/css/build/overrides.css +++ b/public/css/build/overrides.css @@ -1050,6 +1050,9 @@ input[type="radio"]:checked::before { .bootstrap-table .fixed-table-container .table tbody tr .card-view { display: table-row !important; } +.form-control-static { + padding-top: 0px; +} td.text-right.text-padding-number-cell { padding-right: 30px !important; white-space: nowrap; diff --git a/public/css/dist/all.css b/public/css/dist/all.css index 5d809f514e..0924af427a 100644 --- a/public/css/dist/all.css +++ b/public/css/dist/all.css @@ -22764,6 +22764,9 @@ input[type="radio"]:checked::before { .bootstrap-table .fixed-table-container .table tbody tr .card-view { display: table-row !important; } +.form-control-static { + padding-top: 0px; +} td.text-right.text-padding-number-cell { padding-right: 30px !important; white-space: nowrap; @@ -24337,6 +24340,9 @@ input[type="radio"]:checked::before { .bootstrap-table .fixed-table-container .table tbody tr .card-view { display: table-row !important; } +.form-control-static { + padding-top: 0px; +} td.text-right.text-padding-number-cell { padding-right: 30px !important; white-space: nowrap; diff --git a/public/mix-manifest.json b/public/mix-manifest.json index e4dee94e3d..37f6ae70bb 100644 --- a/public/mix-manifest.json +++ b/public/mix-manifest.json @@ -2,8 +2,8 @@ "/js/build/app.js": "/js/build/app.js?id=19253af36b58ed3fb6770c7bb944f079", "/css/dist/skins/skin-black-dark.css": "/css/dist/skins/skin-black-dark.css?id=78bfb1c7b5782df4fb0ac7e36f80f847", "/css/dist/skins/_all-skins.css": "/css/dist/skins/_all-skins.css?id=503d0b09e157a22f555e3670d1ec9bb5", - "/css/build/overrides.css": "/css/build/overrides.css?id=2bfc7b71d951c5ac026dbc034f7373b1", - "/css/build/app.css": "/css/build/app.css?id=4b4c2f1225d59efa7a22b76f7bbe39d8", + "/css/build/overrides.css": "/css/build/overrides.css?id=f2822e504d454229b662569ee39c579d", + "/css/build/app.css": "/css/build/app.css?id=945d335018cdbccc366d239b6c7d67d6", "/css/build/AdminLTE.css": "/css/build/AdminLTE.css?id=4ea0068716c1bb2434d87a16d51b98c9", "/css/dist/skins/skin-yellow.css": "/css/dist/skins/skin-yellow.css?id=7b315b9612b8fde8f9c5b0ddb6bba690", "/css/dist/skins/skin-yellow-dark.css": "/css/dist/skins/skin-yellow-dark.css?id=f6b2e7fa795596ac4754500c9c30eacc", @@ -19,7 +19,7 @@ "/css/dist/skins/skin-blue.css": "/css/dist/skins/skin-blue.css?id=a82b065847bf3cd5d713c04ee8dc86c6", "/css/dist/skins/skin-blue-dark.css": "/css/dist/skins/skin-blue-dark.css?id=7aacfabbafd138c5af6420609f97820d", "/css/dist/skins/skin-black.css": "/css/dist/skins/skin-black.css?id=76482123f6c70e866d6b971ba91de7bb", - "/css/dist/all.css": "/css/dist/all.css?id=12e96a25d0dc3ee39e9d3ce97044fbbf", + "/css/dist/all.css": "/css/dist/all.css?id=8867854784ed3c7ec07d84940894fd47", "/css/dist/signature-pad.css": "/css/dist/signature-pad.css?id=6a89d3cd901305e66ced1cf5f13147f7", "/css/dist/signature-pad.min.css": "/css/dist/signature-pad.min.css?id=6a89d3cd901305e66ced1cf5f13147f7", "/js/select2/i18n/af.js": "/js/select2/i18n/af.js?id=4f6fcd73488ce79fae1b7a90aceaecde", diff --git a/resources/assets/less/overrides.less b/resources/assets/less/overrides.less index e072e4b1e4..ab932ddfbf 100644 --- a/resources/assets/less/overrides.less +++ b/resources/assets/less/overrides.less @@ -1165,6 +1165,11 @@ input[type="radio"]:checked::before { display: table-row !important; } +.form-control-static { + padding-top: 0px; +} + + td.text-right.text-padding-number-cell { padding-right: 30px !important; white-space: nowrap; From c7280953dd2f00ad6c6a3513b0c93773e2055b35 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:08:32 +0100 Subject: [PATCH 02/29] Added/updated tests Signed-off-by: snipe --- tests/Feature/Users/Api/UpdateUserTest.php | 39 +++++++-- tests/Feature/Users/Ui/UpdateUserTest.php | 96 ++++++++++++++++++++-- 2 files changed, 122 insertions(+), 13 deletions(-) diff --git a/tests/Feature/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php index e70471770f..a581256dbf 100644 --- a/tests/Feature/Users/Api/UpdateUserTest.php +++ b/tests/Feature/Users/Api/UpdateUserTest.php @@ -109,7 +109,7 @@ class UpdateUserTest extends TestCase 'password' => 'super-secret', 'password_confirmation' => 'super-secret', 'email' => 'mabel@onlymurderspod.com', - 'permissions' => '{"a.new.permission":"1"}', + //'permissions' => '{"a.new.permission":"1"}', 'activated' => true, 'phone' => '619-555-5555', 'jobtitle' => 'Host', @@ -136,7 +136,7 @@ class UpdateUserTest extends TestCase $this->assertEquals('mabel', $user->username, 'Username was not updated'); $this->assertTrue(Hash::check('super-secret', $user->password), 'Password was not updated'); $this->assertEquals('mabel@onlymurderspod.com', $user->email, 'Email was not updated'); - $this->assertArrayHasKey('a.new.permission', $user->decodePermissions(), 'Permissions were not updated'); + //$this->assertArrayHasKey('a.new.permission', $user->decodePermissions(), 'Permissions were not updated'); $this->assertTrue((bool) $user->activated, 'User not marked as activated'); $this->assertEquals('619-555-5555', $user->phone, 'Phone was not updated'); $this->assertEquals('Host', $user->jobtitle, 'Job title was not updated'); @@ -162,7 +162,7 @@ class UpdateUserTest extends TestCase public function testApiUsersCanBeActivatedWithNumber() { - $admin = User::factory()->superuser()->create(); + $admin = User::factory()->editUsers()->create(); $user = User::factory()->create(['activated' => 0]); $this->actingAsForApi($admin) @@ -175,7 +175,7 @@ class UpdateUserTest extends TestCase public function testApiUsersCanBeActivatedWithBooleanTrue() { - $admin = User::factory()->superuser()->create(); + $admin = User::factory()->editUsers()->create(); $user = User::factory()->create(['activated' => false]); $this->actingAsForApi($admin) @@ -188,7 +188,7 @@ class UpdateUserTest extends TestCase public function testApiUsersCanBeDeactivatedWithNumber() { - $admin = User::factory()->superuser()->create(); + $admin = User::factory()->editUsers()->create(); $user = User::factory()->create(['activated' => true]); $this->actingAsForApi($admin) @@ -201,7 +201,7 @@ class UpdateUserTest extends TestCase public function testApiUsersCanBeDeactivatedWithBooleanFalse() { - $admin = User::factory()->superuser()->create(); + $admin = User::factory()->editUsers()->create(); $user = User::factory()->create(['activated' => true]); $this->actingAsForApi($admin) @@ -212,6 +212,33 @@ class UpdateUserTest extends TestCase $this->assertEquals(0, $user->refresh()->activated); } + public function testEditingUsersCannotEditEscalationFieldsForAdmins() + { + $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]); + + + $this->assertDatabaseHas('users', [ + 'id' => $user->id, + 'username' => 'brandnewuser', + 'email' => 'brandnewemail@example.org', + 'activated' => 1, + 'password' => $hashed_original, + ]); + + $this->actingAsForApi($admin) + ->patch(route('api.users.update', $user), [ + 'username' => 'testnewusername', + 'email' => 'testnewemail@example.org', + 'activated' => 0, + 'password' => $hashed_new, + ]); + + $this->assertEquals(0, $user->refresh()->activated); + + } public function testUsersScopedToCompanyDuringUpdateWhenMultipleFullCompanySupportEnabled() { $this->settings->enableMultipleFullCompanySupport(); diff --git a/tests/Feature/Users/Ui/UpdateUserTest.php b/tests/Feature/Users/Ui/UpdateUserTest.php index 4728531be6..2a82cde3ca 100644 --- a/tests/Feature/Users/Ui/UpdateUserTest.php +++ b/tests/Feature/Users/Ui/UpdateUserTest.php @@ -7,17 +7,18 @@ use App\Models\Company; use App\Models\User; use Error; use Tests\TestCase; +use Illuminate\Support\Facades\Hash; class UpdateUserTest extends TestCase { public function testPageRenders() { - $this->actingAs(User::factory()->superuser()->create()) + $this->actingAs(User::factory()->editUsers()->create()) ->get(route('users.edit', User::factory()->create()->id)) ->assertOk(); } - public function testCannotViewEditPageForSoftDeletedUser() + public function testCanViewEditPageForSoftDeletedUser() { $user = User::factory()->trashed()->create(); @@ -28,7 +29,7 @@ class UpdateUserTest extends TestCase public function testUsersCanBeActivatedWithNumber() { - $admin = User::factory()->superuser()->create(); + $admin = User::factory()->editUsers()->create(); $user = User::factory()->create(['activated' => 0]); $this->actingAs($admin) @@ -43,7 +44,7 @@ class UpdateUserTest extends TestCase public function testUsersCanBeActivatedWithBooleanTrue() { - $admin = User::factory()->superuser()->create(); + $admin = User::factory()->editUsers()->create(); $user = User::factory()->create(['activated' => false]); $this->actingAs($admin) @@ -58,7 +59,7 @@ class UpdateUserTest extends TestCase public function testUsersCanBeDeactivatedWithNumber() { - $admin = User::factory()->superuser()->create(); + $admin = User::factory()->editUsers()->create(); $user = User::factory()->create(['activated' => true]); $this->actingAs($admin) @@ -73,7 +74,7 @@ class UpdateUserTest extends TestCase public function testUsersCanBeDeactivatedWithBooleanFalse() { - $admin = User::factory()->superuser()->create(); + $admin = User::factory()->editUsers()->create(); $user = User::factory()->create(['activated' => true]); $this->actingAs($admin) @@ -88,7 +89,7 @@ class UpdateUserTest extends TestCase public function testUsersUpdatingThemselvesDoNotDeactivateTheirAccount() { - $admin = User::factory()->superuser()->create(['activated' => true]); + $admin = User::factory()->editUsers()->create(['activated' => true]); $this->actingAs($admin) ->put(route('users.update', $admin), [ @@ -99,6 +100,87 @@ class UpdateUserTest extends TestCase $this->assertEquals(1, $admin->refresh()->activated); } + 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]); + + $this->assertDatabaseHas('users', [ + 'id' => $user->id, + 'username' => 'brandnewuser', + 'email' => 'brandnewemail@example.org', + 'activated' => 1, + 'password' => $hashed_original, + ]); + + $this->actingAs($admin) + ->put(route('users.update', $user), [ + 'username' => 'testnewusername', + 'email' => 'testnewemail@example.org', + 'activated' => 0, + 'password' => 'super-secret', + ]); + + $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); + } + + 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]); + + $this->assertDatabaseHas('users', [ + 'id' => $user->id, + 'username' => 'brandnewuser', + 'email' => 'brandnewemail@example.org', + 'activated' => 1, + 'password' => $hashed_original, + ]); + + $this->actingAs($admin) + ->put(route('users.update', $user), [ + 'username' => 'testnewusername', + 'email' => 'testnewemail@example.org', + 'activated' => 0, + 'password' => 'super-secret-new-password', + ]); + + $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->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); + } + + public function testMultiCompanyUserCannotBeMovedIfHasAssetInDifferentCompany() { $this->settings->enableMultipleFullCompanySupport(); From c232f490bc60d0e7552c5eac8c0c0a2f7c056602 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:08:40 +0100 Subject: [PATCH 03/29] Show user log Signed-off-by: snipe --- resources/views/users/view.blade.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/views/users/view.blade.php b/resources/views/users/view.blade.php index b17f615661..4bb7055a32 100755 --- a/resources/views/users/view.blade.php +++ b/resources/views/users/view.blade.php @@ -994,7 +994,7 @@ data-sort-order="desc" id="usersHistoryTable" class="table table-striped snipe-table" - data-url="{{ route('api.activity.index', ['target_id' => $user->id, 'target_type' => 'user']) }}" + data-url="{{ route('api.activity.index', ['target_id' => $user->id, 'item_type' => User::class]) }}" data-export-options='{ "fileName": "export-{{ str_slug($user->present()->fullName ) }}-history-{{ date('Y-m-d') }}", "ignoreColumn": ["actions","image","change","checkbox","checkincheckout","icon"] From a98d3fb4dca85c70939e2b1e7a5f7620cb14e71f Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:09:17 +0100 Subject: [PATCH 04/29] Check for the format of the permissions (string, object, array) Signed-off-by: snipe --- app/Models/User.php | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/app/Models/User.php b/app/Models/User.php index 1cb00cece5..4fc176da30 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -203,11 +203,19 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo { $user_groups = $this->groups; if (($this->permissions == '') && (count($user_groups) == 0)) { - return false; } - $user_permissions = json_decode($this->permissions, true); + $user_permissions = $this->permissions; + + if (is_object($this->permissions)) { + $user_permissions = json_decode(json_encode($this->permissions), true); + } + + if (is_string($this->permissions)) { + $user_permissions = json_decode($this->permissions, true); + } + $is_user_section_permissions_set = ($user_permissions != '') && array_key_exists($section, $user_permissions); //If the user is explicitly granted, return true @@ -261,6 +269,18 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo return $this->checkPermissionSection('superuser'); } + /** + * Checks if the user is an admin + * + * @author A. Gianotto + * @since [v8.1.18] + * @return bool + */ + public function isAdmin() + { + return $this->checkPermissionSection('admin'); + } + /** * Checks if the user can edit their own profile From 0fe49e04bf59787bcaaa67321d6c4d156377c2c6 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:09:27 +0100 Subject: [PATCH 05/29] Attempt to use a gate here? Signed-off-by: snipe --- app/Importer/UserImporter.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/app/Importer/UserImporter.php b/app/Importer/UserImporter.php index 2d5cc3d730..21f7b44086 100644 --- a/app/Importer/UserImporter.php +++ b/app/Importer/UserImporter.php @@ -7,6 +7,7 @@ use App\Models\Department; use App\Models\Setting; use App\Models\User; use App\Notifications\WelcomeNotification; +use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Password; @@ -81,6 +82,7 @@ class UserImporter extends ItemImporter $this->item['username'] = $user_formatted_array['username']; } + // Check if a numeric ID was passed. If it does, use that above all else. if ((array_key_exists('id', $this->item) && ($this->item['id'] != "") && (is_numeric($this->item['id'])))) { $user = User::find($this->item['id']); @@ -90,12 +92,22 @@ class UserImporter extends ItemImporter if ($user) { + // If the user does not want to update existing values, only add new ones, bail out if (! $this->updating) { Log::debug('A matching User '.$this->item['name'].' already exists. '); return; } + $this->log('Updating User'); + + // Todo - check that this works +// if (!Gate::allows('editCurrentUser', $user)) { +// $user->except(['password', 'username', 'email', 'activated']); +// } + $user->update($this->sanitizeItemForUpdating($user)); + + // Why do we have to do this twice? Update should $user->save(); // Update the location of any assets checked out to this user @@ -116,8 +128,12 @@ class UserImporter extends ItemImporter $this->log('No matching user, creating one'); $user = new User(); $user->created_by = auth()->id(); + $user->fill($this->sanitizeItemForStoring($user)); + // TODO - check for gate here I guess + + if ($user->save()) { $this->log('User '.$this->item['name'].' was created'); @@ -143,6 +159,7 @@ class UserImporter extends ItemImporter $this->logError($user, 'User'); } + /** * Fetch an existing department, or create new if it doesn't exist * From d9a54523883c7828c010e3c3539adb4e7d1ce4d0 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:12:10 +0100 Subject: [PATCH 06/29] Defined new gates Signed-off-by: snipe --- app/Providers/AuthServiceProvider.php | 46 +++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php index 899df2ef14..cf2f97e974 100644 --- a/app/Providers/AuthServiceProvider.php +++ b/app/Providers/AuthServiceProvider.php @@ -108,6 +108,8 @@ class AuthServiceProvider extends ServiceProvider }); + + /** * GENERAL GATES * @@ -115,6 +117,49 @@ class AuthServiceProvider extends ServiceProvider * use in our controllers to determine if a user has access to a certain area. */ + Gate::define('editCurrentUser', function ($user, $item) { + + if ($item instanceof User) { + if ($item) { + + // if they can only edit users, deny them if the user is admin or superadmin + if ($user->hasAccess('users.edit')) { + \Log::debug('User can edit users'); + if ($item->isAdmin() || $item->isSuperUser()) { + \Log::debug('User cannot edit admins or superusers'); + return false; + } + + return true; + } + + // if they are an admin, deny them only if the user is a superadmin + if ($user->hasAccess('admin')) { + \Log::debug('User is an admin'); + if ($item->isSuperUser()) { + \Log::debug('User cannot edit superuser'); + return false; + } + + return true; + } + + } + } + }); + + + /** + * Define the demo mode gate so we have an easy way to use @can and Gate::allows() + */ + Gate::define('editableOnDemo', function () { + if (config('app.lock_passwords')) { + \Log::debug('We are in demo mode'); + return false; + } + return true; + }); + Gate::define('admin', function ($user) { if ($user->hasAccess('admin')) { return true; @@ -249,5 +294,6 @@ class AuthServiceProvider extends ServiceProvider return $user->canEditProfile(); }); + } } From 599718f84ef11e76f616c6b517c505a6075144a2 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:12:32 +0100 Subject: [PATCH 07/29] Use new gates in controllers Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 17 ++++++++-- .../Controllers/Users/UsersController.php | 32 ++++++++++++++----- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 15e6f7655e..52fe7184da 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -23,6 +23,7 @@ use App\Notifications\CurrentInventory; use Illuminate\Support\Facades\Auth; use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\Request; +use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Storage; use Illuminate\Support\Facades\Validator; use Illuminate\Support\Facades\Log; @@ -475,8 +476,20 @@ class UsersController extends Controller return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager')); } - if ($request->filled('password')) { - $user->password = bcrypt($request->input('password')); + if (Gate::allows('editCurrentUser', $user)) { + + if ($request->filled('password')) { + $user->password = bcrypt($request->input('password')); + } + + if ($request->filled('username')) { + $user->username = $request->input('username'); + } + + if ($request->filled('email')) { + $user->username = $request->input('username'); + } + } // We need to use has() instead of filled() diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 943d7e240b..3adc9fee77 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -23,6 +23,7 @@ use Redirect; use Str; use Symfony\Component\HttpFoundation\StreamedResponse; use App\Notifications\CurrentInventory; +use Illuminate\Support\Facades\Gate; /** * This controller handles all actions related to Users for @@ -241,14 +242,12 @@ class UsersController extends Controller } // 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); @@ -260,8 +259,6 @@ class UsersController extends Controller $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); @@ -270,16 +267,35 @@ class UsersController extends Controller $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)]); - // Do we want to update the user password? - if ($request->filled('password')) { - $user->password = bcrypt($request->input('password')); + + // check for permissions related fields and pull them out if the current user cannot edit them + if (Gate::allows('editCurrentUser', $user)) { + + \Log::debug('Current user can edit these fields'); + $user->username = trim($request->input('username')); + $user->email = trim($request->input('email')); + $user->activated = $request->input('activated', 0); + + // Do we want to update the user password? + if ($request->filled('password')) { + $user->password = bcrypt($request->input('password')); + } } + + // if a user is editing themselves we should always keep activated true + if (auth()->user()->id == $user->id) { + \Log::debug('User is editing themselves'); + $user->activated = 1; + } + + // Update the location of any assets checked out to this user Asset::where('assigned_type', User::class) ->where('assigned_to', $user->id) From b3c6fe5369bfefcc9b7628240bc11038fb9c8ee2 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:12:46 +0100 Subject: [PATCH 08/29] Use both new gates in user edit Signed-off-by: snipe --- resources/views/users/edit.blade.php | 302 +++++++++++++++------------ 1 file changed, 171 insertions(+), 131 deletions(-) diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index 8c1e34d1a0..c52615a312 100755 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -78,7 +78,9 @@
@@ -93,138 +95,154 @@
- + +
- @if ($user->ldap_import!='1' || str_contains(Route::currentRouteName(), 'clone')) - id)) ? ' disabled' : '') }} - > - @else - - {{ trans('general.managed_ldap') }} - - @endif -
+ @can('editCurrentUser', $user) + + @if ($user->ldap_import!='1' || str_contains(Route::currentRouteName(), 'clone')) + id)) ? ' disabled' : '' }}"> + @else + +

+ + {{ trans('general.managed_ldap') }} +

+ + @endif + + @else +

+ + + {{ trans('general.action_permission_generic', ['action' => trans('general.edit'), 'item_type' => trans('general.username')]) }} +

+ @endcan +
- @if (config('app.lock_passwords') && ($user->id)) - -
-

{{ trans('general.feature_disabled') }}

-
- @endif + @if (!Gate::allows('editableOnDemo') && ($user->id)) + +
+

+ + {{ trans('general.feature_disabled') }} +

+
+ @endif - @if ($errors->first('username')) -
- {!! $errors->first('username', '') !!} -
- @endif + @if ($errors->first('username')) +
+ {!! $errors->first('username', '') !!} +
+ @endif
+ +
- @if ($user->ldap_import!='1' || str_contains(Route::currentRouteName(), 'clone') ) - id)) ? ' required' : '' }} - onfocus="this.removeAttribute('readonly');" - {{ ((config('app.lock_passwords') && ($user->id)) ? ' disabled' : '') }}> - @else - {{ trans('general.managed_ldap') }} - @endif - - {!! $errors->first('password', '') !!} + @can('editCurrentUser', $user) + @if ($user->ldap_import!='1' || str_contains(Route::currentRouteName(), 'clone') ) + id)) ? ' required' : '' }}{{ (!Gate::allows('editableOnDemo') && ($user->id)) ? ' disabled' : '' }}> + + {!! $errors->first('password', '') !!} + @else +

+ {{ trans('general.managed_ldap') }} +

+ @endif + @else +

+ + {{ trans('general.action_permission_generic', ['action' => trans('general.edit'), 'item_type' => trans('general.password')]) }} +

+ @endcan +
+
- @if ($user->ldap_import!='1') + @if (Gate::allows('editCurrentUser', $user) && ($user->ldap_import!='1')) {{ trans('general.generate') }} @endif
+ @if ((Gate::allows('editCurrentUser', $user) && ($user->ldap_import!='1')) || str_contains(Route::currentRouteName(), 'clone')) + +
+ +
+ id) ? ' required' : '' }} onfocus="this.removeAttribute('readonly');" readonly {{ ((!Gate::allows('editableOnDemo') && ($user->id)) ? ' disabled' : '') }}> - @if ($user->ldap_import!='1' || str_contains(Route::currentRouteName(), 'clone')) - -
- -
- id) ? ' required' : '' }} - onfocus="this.removeAttribute('readonly');" - {{ ((config('app.lock_passwords') && ($user->id)) ? ' disabled' : '') }} - > - @if (config('app.lock_passwords') && ($user->id)) -

{{ trans('admin/users/table.lock_passwords') }}

- @endif - {!! $errors->first('password_confirmation', '') !!} -
-
+ @if (!Gate::allows('editableOnDemo') && ($user->id)) +

+ + {{ trans('admin/users/table.lock_passwords') }} +

+ @endif + {!! $errors->first('password_confirmation', '') !!} +
+
@endif
- - @if (config('app.lock_passwords')) + + @if ((!Gate::allows('editableOnDemo')) || (!Gate::allows('editCurrentUser', $user)) || ($user->id == auth()->user()->id)) -

{{ trans('general.feature_disabled') }}

- @elseif ($user->id === Auth::user()->id) - - -

{{ trans('admin/users/general.activated_disabled_help_text') }}

+ @cannot('editableOnDemo') + +

+ + {{ trans('general.feature_disabled') }} +

+ @endcannot + + @cannot('editCurrentUser', $user) + +

+ + {{ trans('general.action_permission_generic', ['action' => trans('general.edit'), 'item_type' => trans('general.login_status')]) }} +

+ @endcannot + + @if ($user->id == auth()->user()->id) + +

+ + {{ trans('admin/users/general.activated_disabled_help_text') }} +

+ @endcannot + @else + @endif +
@@ -232,22 +250,22 @@
- id)) ? ' disabled' : '') }} - autocomplete="off" - readonly - {{ (Helper::checkIfRequired($user, 'email')) ? ' required' : '' }} - onfocus="this.removeAttribute('readonly');"> - @if (config('app.lock_passwords') && ($user->id)) -

{{ trans('admin/users/table.lock_passwords') }}

- @endif - {!! $errors->first('email', '') !!} + @can('editCurrentUser', $user) + id)) ? ' disabled' : '') }} + autocomplete="off" + readonly + {{ (Helper::checkIfRequired($user, 'email')) ? ' required' : '' }} + onfocus="this.removeAttribute('readonly');"> + @if (config('app.lock_passwords') && ($user->id)) +

{{ trans('admin/users/table.lock_passwords') }}

+ @endif + {!! $errors->first('email', '') !!} + @else +

+ + {{ trans('general.action_permission_generic', ['action' => trans('general.edit'), 'item_type' => trans('general.email')]) }} +

+ @endcan
@@ -274,8 +292,19 @@
- @if (\App\Models\Company::canManageUsersCompanies()) + @if ((Gate::allows('editCurrentUser', $user)) && (\App\Models\Company::canManageUsersCompanies())) @include ('partials.forms.edit.company-select', ['translated_name' => trans('general.select_company'), 'fieldname' => 'company_id']) + @else + @if ($user->company) +
+ +
+

+ {{ $user->company ? $user->company->name : '' }} +

+
+
+ @endif @endif @@ -457,7 +486,7 @@
- @if (config('app.lock_passwords')) + @if (!Gate::allows('editableOnDemo')) -

{{ trans('admin/users/general.two_factor_admin_optin_help') }}

+

+ {{ trans('admin/users/general.two_factor_admin_optin_help') }} +

@endif @@ -488,7 +519,9 @@
-

{{ trans('admin/settings/general.two_factor_reset_help') }}

+

+ {{ trans('admin/settings/general.two_factor_reset_help') }} +

@endif @@ -497,11 +530,13 @@
- +
@if ($groups->count()) - @if ((Config::get('app.lock_passwords') || (!Auth::user()->isSuperUser()))) + @if ((!Gate::allows('editableOnDemo') || (!Auth::user()->isSuperUser()))) @if (count($userGroups->keys()) > 0)
    @@ -511,27 +546,30 @@
@endif - {{ trans('admin/users/general.group_memberships_helpblock') }} - @else -
- - @foreach ($groups as $id => $group) - - @endforeach - + @foreach ($groups as $id => $group) + + @endforeach + - - {{ trans('admin/users/table.groupnotes') }} - +

+ {{ trans('admin/users/table.groupnotes') }} +

@endif @else @@ -552,6 +590,7 @@
+ @can('admin')
@if (!Auth::user()->isSuperUser()) @@ -575,6 +614,7 @@ @include('partials.forms.edit.permissions-base')
+ @endcan
Date: Thu, 17 Jul 2025 20:12:52 +0100 Subject: [PATCH 09/29] Added translations Signed-off-by: snipe --- resources/lang/en-US/general.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/resources/lang/en-US/general.php b/resources/lang/en-US/general.php index 433fdd1106..1588669742 100644 --- a/resources/lang/en-US/general.php +++ b/resources/lang/en-US/general.php @@ -4,6 +4,7 @@ return [ '2FA_reset' => '2FA reset', 'accessories' => 'Accessories', 'activated' => 'Activated', + 'login_status' => 'Login Status', 'accepted_date' => 'Date Accepted', 'accessory' => 'Accessory', 'accessory_report' => 'Accessory Report', @@ -316,6 +317,7 @@ return [ 'upload_filetypes_help' => 'Allowed filetypes are: :allowed_filetypes. Max upload size allowed is :size.', 'uploaded' => 'Uploaded', 'user' => 'User', + 'password' => 'Password', 'accepted' => 'accepted', 'declined' => 'declined', 'declined_note' => 'Declined Notes', @@ -476,7 +478,7 @@ return [ 'update_existing_values' => 'Update Existing Values?', 'auto_incrementing_asset_tags_disabled_so_tags_required' => 'Generating auto-incrementing asset tags is disabled so all rows need to have the "Asset Tag" column populated.', 'auto_incrementing_asset_tags_enabled_so_now_assets_will_be_created' => 'Note: Generating auto-incrementing asset tags is enabled so assets will be created for rows that do not have "Asset Tag" populated. Rows that do have "Asset Tag" populated will be updated with the provided information.', - 'send_welcome_email_to_users' => ' Send Welcome Email for new Users?', + 'send_welcome_email_to_users' => ' Send Welcome Email for new Users? Note that only users with a valid email address and who are marked as activated in your import file will received a welcome.', 'send_email' => 'Send Email', 'call' => 'Call number', 'back_before_importing' => 'Backup before importing?', From 1bb5dc7e69b464fb0152534068f82ebcb4e64bea Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:40:01 +0100 Subject: [PATCH 10/29] Added one more test Signed-off-by: snipe --- tests/Feature/Users/Ui/UpdateUserTest.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/Feature/Users/Ui/UpdateUserTest.php b/tests/Feature/Users/Ui/UpdateUserTest.php index 2a82cde3ca..0c4578343a 100644 --- a/tests/Feature/Users/Ui/UpdateUserTest.php +++ b/tests/Feature/Users/Ui/UpdateUserTest.php @@ -11,6 +11,14 @@ use Illuminate\Support\Facades\Hash; class UpdateUserTest extends TestCase { + + public function testRequiresPermission() + { + $this->actingAs(User::factory()->create()) + ->get(route('users.edit', User::factory()->create()->id)) + ->assertForbidden(); + } + public function testPageRenders() { $this->actingAs(User::factory()->editUsers()->create()) From a0d2cb8a03c045046fdd1595561dc191078cf527 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 17 Jul 2025 20:47:20 +0100 Subject: [PATCH 11/29] Clearer (if longer) gate name Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 2 +- app/Http/Controllers/Users/UsersController.php | 2 +- app/Importer/UserImporter.php | 2 +- app/Providers/AuthServiceProvider.php | 2 +- resources/views/users/edit.blade.php | 16 ++++++++-------- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 52fe7184da..e2c74db3d1 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -476,7 +476,7 @@ class UsersController extends Controller return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager')); } - if (Gate::allows('editCurrentUser', $user)) { + if (Gate::allows('canEditSensitiveFieldsForCurrentUser', $user)) { if ($request->filled('password')) { $user->password = bcrypt($request->input('password')); diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 3adc9fee77..ef28f6a497 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -275,7 +275,7 @@ class UsersController extends Controller // check for permissions related fields and pull them out if the current user cannot edit them - if (Gate::allows('editCurrentUser', $user)) { + if (Gate::allows('canEditSensitiveFieldsForCurrentUser', $user)) { \Log::debug('Current user can edit these fields'); $user->username = trim($request->input('username')); diff --git a/app/Importer/UserImporter.php b/app/Importer/UserImporter.php index 21f7b44086..6a021d2b51 100644 --- a/app/Importer/UserImporter.php +++ b/app/Importer/UserImporter.php @@ -101,7 +101,7 @@ class UserImporter extends ItemImporter $this->log('Updating User'); // Todo - check that this works -// if (!Gate::allows('editCurrentUser', $user)) { +// if (!Gate::allows('canEditSensitiveFieldsForCurrentUser', $user)) { // $user->except(['password', 'username', 'email', 'activated']); // } diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php index cf2f97e974..5288a892d0 100644 --- a/app/Providers/AuthServiceProvider.php +++ b/app/Providers/AuthServiceProvider.php @@ -117,7 +117,7 @@ class AuthServiceProvider extends ServiceProvider * use in our controllers to determine if a user has access to a certain area. */ - Gate::define('editCurrentUser', function ($user, $item) { + Gate::define('canEditSensitiveFieldsForCurrentUser', function ($user, $item) { if ($item instanceof User) { if ($item) { diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index c52615a312..0d34bfbd8a 100755 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -102,7 +102,7 @@
- @can('editCurrentUser', $user) + @can('canEditSensitiveFieldsForCurrentUser', $user) @if ($user->ldap_import!='1' || str_contains(Route::currentRouteName(), 'clone')) id)) ? ' disabled' : '' }}"> @@ -151,7 +151,7 @@
- @can('editCurrentUser', $user) + @can('canEditSensitiveFieldsForCurrentUser', $user) @if ($user->ldap_import!='1' || str_contains(Route::currentRouteName(), 'clone') ) id)) ? ' required' : '' }}{{ (!Gate::allows('editableOnDemo') && ($user->id)) ? ' disabled' : '' }}> @@ -171,13 +171,13 @@
- @if (Gate::allows('editCurrentUser', $user) && ($user->ldap_import!='1')) + @if (Gate::allows('canEditSensitiveFieldsForCurrentUser', $user) && ($user->ldap_import!='1')) {{ trans('general.generate') }} @endif
- @if ((Gate::allows('editCurrentUser', $user) && ($user->ldap_import!='1')) || str_contains(Route::currentRouteName(), 'clone')) + @if ((Gate::allows('canEditSensitiveFieldsForCurrentUser', $user) && ($user->ldap_import!='1')) || str_contains(Route::currentRouteName(), 'clone'))