diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 2d4b2b3296..2bb58b9a32 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -584,10 +584,38 @@ class UsersController extends Controller 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'])); + // User::GATED_AUTH_FIELDS enter through the canEditAuthFields branch + // below. If the caller does not hold that gate against this target + // but their request nevertheless carries any of those fields, fail + // loud rather than persisting a partial write while returning a + // success response. Prior behavior silently dropped the auth-field + // writes and returned `success`, which misrepresented what + // actually persisted. + $requestedAuthFields = array_values(array_intersect(User::GATED_AUTH_FIELDS, array_keys($request->all()))); + $canEditAuthFields = auth()->user()->can('canEditAuthFields', $user) && auth()->user()->can('editableOnDemo'); - if (auth()->user()->can('canEditAuthFields', $user) && auth()->user()->can('editableOnDemo')) { + if (! empty($requestedAuthFields) && ! $canEditAuthFields) { + return response()->json(Helper::formatStandardApiResponse( + 'error', + null, + trans('admin/users/message.auth_fields_denied', ['fields' => implode(', ', $requestedAuthFields)]), + )); + } + + // Pull out sensitive fields that require extra permission. The + // GATED_AUTH_FIELDS constant covers user-editable secrets; the + // additional keys below are internal state (2FA secrets, remember + // tokens, activation codes) that must never be settable from a + // request payload regardless of caller privilege. + $user->fill($request->except(array_merge(User::GATED_AUTH_FIELDS, [ + 'activation_code', + 'remember_token', + 'two_factor_secret', + 'two_factor_enrolled', + 'two_factor_optin', + ]))); + + if ($canEditAuthFields) { if ($request->filled('password')) { $user->password = bcrypt($request->input('password')); diff --git a/app/Importer/UserImporter.php b/app/Importer/UserImporter.php index 9851d17295..cca4c333b3 100644 --- a/app/Importer/UserImporter.php +++ b/app/Importer/UserImporter.php @@ -141,10 +141,25 @@ class UserImporter extends ItemImporter // CLI imports run unauthenticated and are fully trusted; only restrict web-initiated imports. // Note: unset must target $this->item, not the model — sanitizeItemForUpdating() reads from $this->item. if (Auth::check() && (! Auth::user()->hasAccess('users.edit') || ! Gate::allows('canEditAuthFields', $user))) { - unset($this->item['username']); - unset($this->item['email']); - unset($this->item['password']); - unset($this->item['activated']); + // GATED_AUTH_FIELDS is the shared list across the API, + // web-UI, and importer paths. The importer naturally + // filters out fields that are not present in the CSV + // via array_intersect, so `permissions` (which the + // importer never processes) drops out on its own. + $deniedAuthFields = array_values(array_intersect(User::GATED_AUTH_FIELDS, array_keys($this->item))); + foreach ($deniedAuthFields as $field) { + unset($this->item[$field]); + } + if (! empty($deniedAuthFields)) { + // Surface the skip in the import summary rather than + // silently persisting a partial row. Halting the whole + // import on the first affected row would be worse UX. + $this->log(sprintf( + 'Skipped auth fields (%s) on user %s: caller lacks canEditAuthFields on this target.', + implode(', ', $deniedAuthFields), + $user->username, + )); + } } if (! $this->validateFmcsLocation($this->item['location_id'] ?? null, $companyIds)) { diff --git a/app/Models/User.php b/app/Models/User.php index b6d48466ad..50a463a066 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -47,6 +47,22 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo use Searchable; use UniqueUndeletedTrait; + /** + * Fields governed by the `canEditAuthFields` gate: credentials (username, + * email, password), the activation flag, and the permission blob. Any + * controller / importer / job that mass-assigns from user input must gate + * writes to these fields on `canEditAuthFields` against the target, and + * signal denial rather than silently dropping them. Add to this list to + * bring a new field under the same gate everywhere at once. + */ + public const GATED_AUTH_FIELDS = [ + 'password', + 'username', + 'email', + 'activated', + 'permissions', + ]; + protected $hidden = [ 'password', 'remember_token', diff --git a/resources/lang/en-US/admin/users/message.php b/resources/lang/en-US/admin/users/message.php index 713c3f6877..533e5114f4 100644 --- a/resources/lang/en-US/admin/users/message.php +++ b/resources/lang/en-US/admin/users/message.php @@ -13,6 +13,7 @@ return [ 'nothing_currently_assigned' => 'Nothing currently assigned.', 'user_password_required' => 'The password is required.', 'insufficient_permissions' => 'Insufficient Permissions.', + 'auth_fields_denied' => 'You do not have permission to modify credential or activation fields on this user. Requested fields not updated: :fields', 'user_deleted_warning' => 'This user has been deleted. You will have to restore this user to edit them or assign them new assets.', 'ldap_not_configured' => 'LDAP integration has not been configured for this installation.', 'password_resets_sent' => 'The selected users who are activated and have a valid email addresses have been sent a password reset link.', diff --git a/tests/Feature/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php index 5129d22a18..f8ddf89c94 100644 --- a/tests/Feature/Users/Api/UpdateUserTest.php +++ b/tests/Feature/Users/Api/UpdateUserTest.php @@ -237,6 +237,74 @@ class UpdateUserTest extends TestCase $this->assertSame(1, (int) $admin->fresh()->activated, 'Non-admin actor must not be able to deactivate an admin via API.'); } + /** + * When a caller who cannot pass canEditAuthFields on the target sends any + * User::GATED_AUTH_FIELDS in the request payload, the API must return an + * error status naming the denied fields rather than silently dropping + * them and returning `success`. Prior behavior returned + * `{"status":"success", "messages":"User was successfully updated."}` + * even when the password / permissions / activated / username / email + * write in the payload never persisted, misrepresenting the actual + * outcome to API clients. + */ + public function test_api_returns_error_when_auth_fields_requested_without_permission(): void + { + $editing_user = User::factory()->editUsers()->create(); + $admin = User::factory()->admin()->create([ + 'username' => 'api_admin_authfield_target', + 'email' => 'api-admin-authfield-target@example.test', + 'first_name' => 'Original', + 'last_name' => 'Name', + ]); + + $originalPasswordHash = $admin->password; + + $response = $this->actingAsForApi($editing_user) + ->patch(route('api.users.update', $admin), [ + 'first_name' => 'Tampered', + 'password' => 'attempted-new-password', + 'permissions' => ['licenses.keys' => '1'], + ]) + ->assertOk(); + + $response->assertJson([ + 'status' => 'error', + 'messages' => trans('admin/users/message.auth_fields_denied', ['fields' => 'password, permissions']), + ]); + + $fresh = $admin->fresh(); + $this->assertSame('Original', $fresh->first_name, 'Non-auth fields must not persist when the request is rejected for auth-field denial.'); + $this->assertSame($originalPasswordHash, $fresh->password, 'Password must not change when the caller cannot canEditAuthFields on the target.'); + } + + /** + * The inverse contract: when the request carries no GATED_AUTH_FIELDS, + * the response stays `success` and non-auth fields persist as normal. + * Pins that the new loud-fail path is entered only when the payload + * actually asks for gated fields. + */ + public function test_api_returns_success_when_no_auth_fields_are_requested(): void + { + $editing_user = User::factory()->editUsers()->create(); + $admin = User::factory()->admin()->create([ + 'first_name' => 'Original', + 'last_name' => 'Name', + 'jobtitle' => 'Previous Title', + ]); + + $this->actingAsForApi($editing_user) + ->patch(route('api.users.update', $admin), [ + 'first_name' => 'Updated', + 'jobtitle' => 'New Title', + ]) + ->assertOk() + ->assertJson(['status' => 'success']); + + $fresh = $admin->fresh(); + $this->assertSame('Updated', $fresh->first_name); + $this->assertSame('New Title', $fresh->jobtitle); + } + public function test_api_users_can_be_deactivated_with_number() { $admin = User::factory()->editUsers()->create();