From aa4e586ec239ebef2b726e793387b1488530a7cc Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 7 Aug 2026 13:14:44 +0100 Subject: [PATCH 1/3] LDAP: added missing dept sync, fixed phone field typo --- app/Http/Controllers/Auth/LoginController.php | 14 +- app/Models/Ldap.php | 170 ++++++++++++++---- 2 files changed, 145 insertions(+), 39 deletions(-) diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 39fc2c3e25..60a932c6ab 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -205,16 +205,22 @@ class LoginController extends Controller Log::debug('Local user '.$request->input('username').' exists in database. Updating existing user against LDAP.'); $ldap_attr = Ldap::parseAndMapLdapAttributes($ldap_user); + $settings = Setting::getSettings(); $user->password = $user->noPassword(); - if (Setting::getSettings()->ldap_pw_sync == '1') { + if ($settings->ldap_pw_sync == '1') { $user->password = bcrypt($request->input('password')); } $user->last_login = \Carbon::now(); - $user->email = $ldap_attr['email']; - $user->first_name = $ldap_attr['firstname']; - $user->last_name = $ldap_attr['lastname']; // FIXME (or TODO?) - do we need to map additional fields that we now support? E.g. country, phone, etc. + + // Refresh every mapped field from the LDAP payload. Shared + // with Ldap::createUserFromLdap so the field list lives in + // one place. Bulk sync via snipe-it:ldap-sync remains the + // canonical path for the fields that need a re-bind + // (manager, active_flag, etc.). + Ldap::applyLdapAttributesToUser($user, $ldap_attr); + $user->saveQuietly(); } // End if(!user) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index 4078dbe806..c4a20767e9 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -97,7 +97,7 @@ class Ldap extends Model ldap_set_option($connection, LDAP_OPT_NETWORK_TIMEOUT, 20); if ($ldap_use_tls == '1') { - if (!ldap_start_tls($connection)) { + if (! ldap_start_tls($connection)) { throw new Exception('STARTTLS Failed.'); } } @@ -313,27 +313,45 @@ class Ldap extends Model */ public static function parseAndMapLdapAttributes($ldapattributes) { - // Get LDAP attribute config - $ldap_result_username = Setting::getSettings()->ldap_username_field; - $ldap_result_emp_num = Setting::getSettings()->ldap_emp_num; - $ldap_result_last_name = Setting::getSettings()->ldap_lname_field; - $ldap_result_first_name = Setting::getSettings()->ldap_fname_field; - $ldap_result_email = Setting::getSettings()->ldap_email; - $ldap_result_phone = Setting::getSettings()->ldap_phone; - $ldap_result_jobtitle = Setting::getSettings()->ldap_jobtitle; - $ldap_result_country = Setting::getSettings()->ldap_country; - $ldap_result_location = Setting::getSettings()->ldap_location; - $ldap_result_dept = Setting::getSettings()->ldap_dept; - $ldap_result_manager = Setting::getSettings()->ldap_manager; - // Get LDAP user data + // Get LDAP attribute config. The settings column names here are + // the same ones LdapSync's $ldap_map reads, so this parser and + // the bulk-sync command see identical field lookups. + $settings = Setting::getSettings(); + $ldap_result_username = $settings->ldap_username_field; + $ldap_result_emp_num = $settings->ldap_emp_num; + $ldap_result_last_name = $settings->ldap_lname_field; + $ldap_result_first_name = $settings->ldap_fname_field; + $ldap_result_display_name = $settings->ldap_display_name; + $ldap_result_email = $settings->ldap_email; + $ldap_result_phone = $settings->ldap_phone_field; + $ldap_result_mobile = $settings->ldap_mobile; + $ldap_result_jobtitle = $settings->ldap_jobtitle; + $ldap_result_address = $settings->ldap_address; + $ldap_result_city = $settings->ldap_city; + $ldap_result_state = $settings->ldap_state; + $ldap_result_zip = $settings->ldap_zip; + $ldap_result_country = $settings->ldap_country; + $ldap_result_location = $settings->ldap_location; + $ldap_result_dept = $settings->ldap_dept; + $ldap_result_manager = $settings->ldap_manager; + + // Get LDAP user data. Kept in the same shape LdapSync's per-user + // $item array uses so the two paths stay comparable when this + // one grows. $item = []; $item['username'] = $ldapattributes[$ldap_result_username][0] ?? ''; $item['employee_number'] = $ldapattributes[$ldap_result_emp_num][0] ?? ''; $item['lastname'] = $ldapattributes[$ldap_result_last_name][0] ?? ''; $item['firstname'] = $ldapattributes[$ldap_result_first_name][0] ?? ''; + $item['display_name'] = $ldapattributes[$ldap_result_display_name][0] ?? ''; $item['email'] = $ldapattributes[$ldap_result_email][0] ?? ''; $item['telephone'] = $ldapattributes[$ldap_result_phone][0] ?? ''; + $item['mobile'] = $ldapattributes[$ldap_result_mobile][0] ?? ''; $item['jobtitle'] = $ldapattributes[$ldap_result_jobtitle][0] ?? ''; + $item['address'] = $ldapattributes[$ldap_result_address][0] ?? ''; + $item['city'] = $ldapattributes[$ldap_result_city][0] ?? ''; + $item['state'] = $ldapattributes[$ldap_result_state][0] ?? ''; + $item['zip'] = $ldapattributes[$ldap_result_zip][0] ?? ''; $item['country'] = $ldapattributes[$ldap_result_country][0] ?? ''; $item['department'] = $ldapattributes[$ldap_result_dept][0] ?? ''; $item['manager'] = $ldapattributes[$ldap_result_manager][0] ?? ''; @@ -343,6 +361,78 @@ class Ldap extends Model return $item; } + /** + * Copy the parseAndMapLdapAttributes() output onto a User row. + * Called by both createUserFromLdap (first login, new user) and + * LoginController::loginViaLdap (existing user re-login), so the + * mapping list lives in exactly one place. + * + * Each optional field is gated on its LDAP mapping being non-blank + * so unset mappings don't overwrite existing values with empty + * strings. Department and Location are firstOrCreate'd only when + * both the mapping is set and the LDAP payload actually carried a + * value, so a blank attribute doesn't accrete a nameless row. + * + * Manager is intentionally out of scope: LdapSync's manager + * resolution needs an admin re-bind + LDAP re-query to translate + * the DN into a Snipe-IT user id, and that's best done in bulk. + * ldap_import users get their manager populated on the next + * `snipe-it:ldap-sync` run. + */ + public static function applyLdapAttributesToUser(User $user, array $ldapAttr): void + { + $settings = Setting::getSettings(); + + // Always-written identity fields. These have no per-field gate + // because Snipe-IT considers username / first name / last name / + // email load-bearing for every user — if a mapping's blank the + // LDAP payload just gives us an empty string, matching the + // pre-fix behavior on the create path. + $user->username = $ldapAttr['username']; + $user->first_name = $ldapAttr['firstname']; + $user->last_name = $ldapAttr['lastname']; + $user->email = $ldapAttr['email']; + + if ($settings->ldap_display_name != '') { + $user->display_name = $ldapAttr['display_name']; + } + if ($settings->ldap_emp_num != '') { + $user->employee_num = e($ldapAttr['employee_number']); + } + if ($settings->ldap_phone_field != '') { + $user->phone = $ldapAttr['telephone']; + } + if ($settings->ldap_mobile != '') { + $user->mobile = $ldapAttr['mobile']; + } + if ($settings->ldap_jobtitle != '') { + $user->jobtitle = $ldapAttr['jobtitle']; + } + if ($settings->ldap_address != '') { + $user->address = $ldapAttr['address']; + } + if ($settings->ldap_city != '') { + $user->city = $ldapAttr['city']; + } + if ($settings->ldap_state != '') { + $user->state = $ldapAttr['state']; + } + if ($settings->ldap_zip != '') { + $user->zip = $ldapAttr['zip']; + } + if ($settings->ldap_country != '') { + $user->country = $ldapAttr['country']; + } + if ($settings->ldap_dept != '' && $ldapAttr['department'] !== '') { + $department = Department::firstOrCreate(['name' => $ldapAttr['department']]); + $user->department_id = $department->id; + } + if ($settings->ldap_location != '' && $ldapAttr['location'] !== '') { + $location = Location::firstOrCreate(['name' => $ldapAttr['location']]); + $user->location_id = $location->id; + } + } + /** * Create user from LDAP attributes * @@ -356,33 +446,43 @@ class Ldap extends Model { $item = self::parseAndMapLdapAttributes($ldapatttibutes); - // Create user from LDAP data - if (! empty($item['username'])) { - $user = new User; - $user->first_name = $item['firstname']; - $user->last_name = $item['lastname']; - $user->username = $item['username']; - $user->email = $item['email']; - $user->locale = $item['locale']; - $user->password = $user->noPassword(); + if (empty($item['username'])) { + return false; + } - if (Setting::getSettings()->ldap_pw_sync == '1') { - $user->password = bcrypt($password); - } + $settings = Setting::getSettings(); - $user->activated = 1; - $user->ldap_import = 1; - $user->notes = 'Imported on first login from LDAP'; + $user = new User; + self::applyLdapAttributesToUser($user, $item); - if ($user->save()) { - return $user; - } else { - Log::debug('Could not create user.'.$user->getErrors()); - throw new Exception('Could not create user: '.$user->getErrors()); + $user->locale = $item['locale']; + $user->password = $user->noPassword(); + if ($settings->ldap_pw_sync == '1') { + $user->password = bcrypt($password); + } + + $user->activated = 1; + $user->ldap_import = 1; + $user->notes = 'Imported on first login from LDAP'; + + if (! $user->save()) { + Log::debug('Could not create user.'.$user->getErrors()); + throw new Exception('Could not create user: '.$user->getErrors()); + } + + // Attach the configured Default Permissions Group to newly- + // created LDAP users so first-login users land with the same + // baseline permissions bulk-synced users get. Matches + // LdapSync::handle()'s post-save group attachment. Skipped when + // the setting points at a deleted group. + if ($settings->ldap_default_group) { + $default = Group::find($settings->ldap_default_group); + if ($default !== null && ! $user->groups()->where('group_id', $default->id)->exists()) { + $user->groups()->attach($default->id); } } - return false; + return $user; } /** From 3b1ee527043f5799b77a7f3aca0cb35f279474f6 Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 7 Aug 2026 13:14:53 +0100 Subject: [PATCH 2/3] Added mapping test --- tests/Unit/LdapCreateUserFromLdapTest.php | 227 ++++++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 tests/Unit/LdapCreateUserFromLdapTest.php diff --git a/tests/Unit/LdapCreateUserFromLdapTest.php b/tests/Unit/LdapCreateUserFromLdapTest.php new file mode 100644 index 0000000000..05f346f05c --- /dev/null +++ b/tests/Unit/LdapCreateUserFromLdapTest.php @@ -0,0 +1,227 @@ + ['jsmith'], + 'sn' => ['Smith'], + 'givenname' => ['Jane'], + 'displayname' => ['Jane Smith'], + 'mail' => ['jane@example.com'], + 'employeenumber' => ['E1234'], + 'telephonenumber' => ['555-0100'], + 'mobile' => ['555-0200'], + 'title' => ['Widget Wrangler'], + 'streetaddress' => ['1 Main St'], + 'l' => ['Springfield'], + 'st' => ['IL'], + 'postalcode' => ['62704'], + 'c' => ['US'], + 'department' => ['Widgets'], + 'physicaldeliveryofficename' => ['HQ'], + ], $overrides); + } + + private function configureLdapMappings(): void + { + $this->settings->enableLdap(); + $this->settings->set([ + 'ldap_username_field' => 'samaccountname', + 'ldap_lname_field' => 'sn', + 'ldap_fname_field' => 'givenname', + 'ldap_display_name' => 'displayname', + 'ldap_email' => 'mail', + 'ldap_emp_num' => 'employeenumber', + 'ldap_phone_field' => 'telephonenumber', + 'ldap_mobile' => 'mobile', + 'ldap_jobtitle' => 'title', + 'ldap_address' => 'streetaddress', + 'ldap_city' => 'l', + 'ldap_state' => 'st', + 'ldap_zip' => 'postalcode', + 'ldap_country' => 'c', + 'ldap_dept' => 'department', + 'ldap_location' => 'physicaldeliveryofficename', + ]); + } + + public function test_populates_every_configured_scalar_field(): void + { + $this->configureLdapMappings(); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertInstanceOf(User::class, $user); + $this->assertSame('jsmith', $user->username); + $this->assertSame('Jane', $user->first_name); + $this->assertSame('Smith', $user->last_name); + $this->assertSame('Jane Smith', $user->display_name); + $this->assertSame('jane@example.com', $user->email); + $this->assertSame('E1234', $user->employee_num); + $this->assertSame('555-0100', $user->phone); + $this->assertSame('555-0200', $user->mobile); + $this->assertSame('Widget Wrangler', $user->jobtitle); + $this->assertSame('1 Main St', $user->address); + $this->assertSame('Springfield', $user->city); + $this->assertSame('IL', $user->state); + $this->assertSame('62704', $user->zip); + $this->assertSame('US', $user->country); + $this->assertSame(1, (int) $user->activated); + $this->assertSame(1, (int) $user->ldap_import); + } + + public function test_creates_department_from_ldap_value(): void + { + $this->configureLdapMappings(); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertNotNull($user->department_id); + $this->assertSame('Widgets', Department::find($user->department_id)->name); + } + + public function test_reuses_existing_department_by_name(): void + { + $this->configureLdapMappings(); + $existing = Department::factory()->create(['name' => 'Widgets']); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertSame($existing->id, $user->department_id); + } + + public function test_creates_location_from_ldap_value(): void + { + $this->configureLdapMappings(); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertNotNull($user->location_id); + $this->assertSame('HQ', Location::find($user->location_id)->name); + } + + public function test_reuses_existing_location_by_name(): void + { + $this->configureLdapMappings(); + $existing = Location::factory()->create(['name' => 'HQ']); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertSame($existing->id, $user->location_id); + } + + public function test_skips_field_when_setting_mapping_is_blank(): void + { + $this->configureLdapMappings(); + $this->settings->set(['ldap_phone_field' => '']); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertNull($user->phone); + } + + public function test_skips_department_when_ldap_dept_mapping_blank(): void + { + $this->configureLdapMappings(); + $this->settings->set(['ldap_dept' => '']); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertNull($user->department_id); + $this->assertDatabaseMissing('departments', ['name' => 'Widgets']); + } + + public function test_skips_department_when_ldap_value_is_missing(): void + { + $this->configureLdapMappings(); + // Mapping IS configured, but the LDAP payload for this user + // simply doesn't carry the department attribute. A blank + // "Department" row is worse than no row. + $user = Ldap::createUserFromLdap( + $this->ldapAttributes(['department' => []]), + 'pw', + ); + + $this->assertNull($user->department_id); + $this->assertDatabaseMissing('departments', ['name' => '']); + } + + public function test_attaches_default_permissions_group(): void + { + $this->configureLdapMappings(); + $group = Group::factory()->create(); + $this->settings->set(['ldap_default_group' => $group->id]); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertTrue($user->groups()->where('group_id', $group->id)->exists()); + } + + public function test_does_not_attach_default_permissions_group_when_group_deleted(): void + { + $this->configureLdapMappings(); + $this->settings->set(['ldap_default_group' => 99999]); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'pw'); + + $this->assertSame(0, $user->groups()->count()); + } + + public function test_returns_false_when_ldap_username_missing(): void + { + $this->configureLdapMappings(); + + $this->assertFalse( + Ldap::createUserFromLdap($this->ldapAttributes(['samaccountname' => []]), 'pw'), + ); + } + + public function test_sets_bcrypted_password_when_ldap_pw_sync_enabled(): void + { + $this->configureLdapMappings(); + $this->settings->set(['ldap_pw_sync' => 1]); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'secret-password'); + + $this->assertTrue(Hash::check('secret-password', $user->password)); + } + + public function test_password_is_unusable_when_ldap_pw_sync_disabled(): void + { + $this->configureLdapMappings(); + $this->settings->set(['ldap_pw_sync' => 0]); + + $user = Ldap::createUserFromLdap($this->ldapAttributes(), 'secret-password'); + + $this->assertFalse(Hash::check('secret-password', $user->password)); + } +} From db2cfe6cfd9dddb8c2da037d5365cf25746992ae Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 7 Aug 2026 13:15:00 +0100 Subject: [PATCH 3/3] Updated phpstan baseline --- phpstan-baseline.neon | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 6ca560d5a3..1d73da8459 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -8041,11 +8041,17 @@ parameters: path: app/Models/Labels/Tapes/Generic/Tape_53mm.php - - message: '#^Access to an undefined property App\\Models\\Setting\:\:\$ldap_phone\.$#' + message: '#^Access to an undefined property App\\Models\\User\:\:\$display_name\.$#' identifier: property.notFound count: 1 path: app/Models/Ldap.php + - + message: '#^Call to an undefined method Illuminate\\Database\\Eloquent\\Relations\\Relation\:\:attach\(\)\.$#' + identifier: method.notFound + count: 1 + path: app/Models/Ldap.php + - message: '#^Called ''env'' outside of the config directory which returns null when the config is cached, use ''config''\.$#' identifier: larastan.noEnvCallsOutsideOfConfig