Skip to content

Commit 2500375

Browse files
snipesnipe
authored
Merge pull request #15672 from uberbrady/ldap_location_fixes
Clean up how we use the '$location' in LDAP sync command
2 parents 16c8264 + e4e1d0d commit 2500375

File tree

1 file changed

+110
-105
lines changed

1 file changed

+110
-105
lines changed

‎app/Console/Commands/LdapSync.php‎

Lines changed: 110 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -137,23 +137,24 @@ public function handle()
137137
}
138138

139139
/* Determine which location to assign users to by default. */
140-
$location = null; // TODO - this would be better called "$default_location", which is more explicit about its purpose
140+
$default_location = null;
141141
if ($this->option('location') != '') {
142-
if ($location = Location::where('name', '=', $this->option('location'))->first()) {
142+
if ($default_location = Location::where('name', '=', $this->option('location'))->first()) {
143143
Log::debug('Location name ' . $this->option('location') . ' passed');
144-
Log::debug('Importing to ' . $location->name . ' (' . $location->id . ')');
144+
Log::debug('Importing to '.$default_location->name.' ('.$default_location->id.')');
145145
}
146146

147147
} elseif ($this->option('location_id')) {
148+
//TODO - figure out how or why this is an array?
148149
foreach($this->option('location_id') as $location_id) {
149-
if ($location = Location::where('id', '=', $location_id)->first()) {
150+
if ($default_location = Location::where('id', '=', $location_id)->first()) {
150151
Log::debug('Location ID ' . $location_id . ' passed');
151-
Log::debug('Importing to ' . $location->name . ' (' . $location->id . ')');
152+
Log::debug('Importing to '.$default_location->name.' ('.$default_location->id.')');
152153
}
153154

154155
}
155156
}
156-
if (! isset($location)) {
157+
if (!isset($default_location)) {
157158
Log::debug('That location is invalid or a location was not provided, so no location will be assigned by default.');
158159
}
159160

@@ -229,43 +230,44 @@ public function handle()
229230

230231

231232
for ($i = 0; $i < $results['count']; $i++) {
232-
$item = [];
233-
$item['username'] = $results[$i][$ldap_map["username"]][0] ?? '';
234-
$item['employee_number'] = $results[$i][$ldap_map["emp_num"]][0] ?? '';
235-
$item['lastname'] = $results[$i][$ldap_map["last_name"]][0] ?? '';
236-
$item['firstname'] = $results[$i][$ldap_map["first_name"]][0] ?? '';
237-
$item['email'] = $results[$i][$ldap_map["email"]][0] ?? '';
238-
$item['ldap_location_override'] = $results[$i]['ldap_location_override'] ?? '';
239-
$item['location_id'] = $results[$i]['location_id'] ?? '';
240-
$item['telephone'] = $results[$i][$ldap_map["phone"]][0] ?? '';
241-
$item['jobtitle'] = $results[$i][$ldap_map["jobtitle"]][0] ?? '';
242-
$item['country'] = $results[$i][$ldap_map["country"]][0] ?? '';
243-
$item['department'] = $results[$i][$ldap_map["dept"]][0] ?? '';
244-
$item['manager'] = $results[$i][$ldap_map["manager"]][0] ?? '';
245-
$item['location'] = $results[$i][$ldap_map["location"]][0] ?? '';
246-
247-
// ONLY if you are using the "ldap_location" option *AND* you have an actual result
248-
if ($ldap_map["location"] && $item['location']) {
249-
$location = Location::firstOrCreate([
250-
'name' => $item['location'],
251-
]);
252-
}
253-
$department = Department::firstOrCreate([
254-
'name' => $item['department'],
233+
$item = [];
234+
$item['username'] = $results[$i][$ldap_map["username"]][0] ?? '';
235+
$item['employee_number'] = $results[$i][$ldap_map["emp_num"]][0] ?? '';
236+
$item['lastname'] = $results[$i][$ldap_map["last_name"]][0] ?? '';
237+
$item['firstname'] = $results[$i][$ldap_map["first_name"]][0] ?? '';
238+
$item['email'] = $results[$i][$ldap_map["email"]][0] ?? '';
239+
$item['ldap_location_override'] = $results[$i]['ldap_location_override'] ?? '';
240+
$item['location_id'] = $results[$i]['location_id'] ?? '';
241+
$item['telephone'] = $results[$i][$ldap_map["phone"]][0] ?? '';
242+
$item['jobtitle'] = $results[$i][$ldap_map["jobtitle"]][0] ?? '';
243+
$item['country'] = $results[$i][$ldap_map["country"]][0] ?? '';
244+
$item['department'] = $results[$i][$ldap_map["dept"]][0] ?? '';
245+
$item['manager'] = $results[$i][$ldap_map["manager"]][0] ?? '';
246+
$item['location'] = $results[$i][$ldap_map["location"]][0] ?? '';
247+
$location = $default_location; //initially, set '$location' to the default_location (which may just be `null`)
248+
249+
// ONLY if you are using the "ldap_location" option *AND* you have an actual result
250+
if ($ldap_map["location"] && $item['location']) {
251+
$location = Location::firstOrCreate([
252+
'name' => $item['location'],
255253
]);
256-
257-
$user = User::where('username', $item['username'])->first();
258-
if ($user) {
259-
// Updating an existing user.
260-
$item['createorupdate'] = 'updated';
261-
} else {
262-
// Creating a new user.
263-
$user = new User;
264-
$user->password = $user->noPassword();
265-
$user->locale = app()->getLocale();
266-
$user->activated = 1; // newly created users can log in by default, unless AD's UAC is in use, or an active flag is set (below)
267-
$item['createorupdate'] = 'created';
268-
}
254+
}
255+
$department = Department::firstOrCreate([
256+
'name' => $item['department'],
257+
]);
258+
259+
$user = User::where('username', $item['username'])->first();
260+
if ($user) {
261+
// Updating an existing user.
262+
$item['createorupdate'] = 'updated';
263+
} else {
264+
// Creating a new user.
265+
$user = new User;
266+
$user->password = $user->noPassword();
267+
$user->locale = app()->getLocale();
268+
$user->activated = 1; // newly created users can log in by default, unless AD's UAC is in use, or an active flag is set (below)
269+
$item['createorupdate'] = 'created';
270+
}
269271

270272
//If a sync option is not filled in on the LDAP settings don't populate the user field
271273
if($ldap_map["username"] != null){
@@ -296,7 +298,7 @@ public function handle()
296298
$user->department_id = $department->id;
297299
}
298300
if($ldap_map["location"] != null){
299-
$user->location_id = $location ? $location->id : null;
301+
$user->location_id = $location?->id;
300302
}
301303

302304
if($ldap_map["manager"] != null){
@@ -341,38 +343,38 @@ public function handle()
341343
}
342344
}
343345

344-
// Sync activated state for Active Directory.
345-
if ( !empty($ldap_map["active_flag"])) { // IF we have an 'active' flag set....
346-
// ....then *most* things that are truthy will activate the user. Anything falsey will deactivate them.
347-
// (Specifically, we don't handle a value of '0.0' correctly)
348-
$raw_value = @$results[$i][$ldap_map["active_flag"]][0];
349-
$filter_var = filter_var($raw_value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE);
350-
$boolean_cast = (bool)$raw_value;
351-
352-
$user->activated = $filter_var ?? $boolean_cast; // if filter_var() was true or false, use that. If it's null, use the $boolean_cast
353-
354-
} elseif (array_key_exists('useraccountcontrol', $results[$i]) ) {
355-
// ....otherwise, (ie if no 'active' LDAP flag is defined), IF the UAC setting exists,
356-
// ....then use the UAC setting on the account to determine can-log-in vs. cannot-log-in
357-
358-
359-
/* The following is _probably_ the correct logic, but we can't use it because
360-
some users may have been dependent upon the previous behavior, and this
361-
could cause additional access to be available to users they don't want
362-
to allow to log in.
363-
364-
$useraccountcontrol = $results[$i]['useraccountcontrol'][0];
365-
if(
366-
// based on MS docs at: https://support.microsoft.com/en-us/help/305144/how-to-use-useraccountcontrol-to-manipulate-user-account-properties
367-
($useraccountcontrol & 0x200) && // is a NORMAL_ACCOUNT
368-
!($useraccountcontrol & 0x02) && // *and* _not_ ACCOUNTDISABLE
369-
!($useraccountcontrol & 0x10) // *and* _not_ LOCKOUT
370-
) {
371-
$user->activated = 1;
372-
} else {
373-
$user->activated = 0;
374-
} */
375-
$enabled_accounts = [
346+
// Sync activated state for Active Directory.
347+
if (!empty($ldap_map["active_flag"])) { // IF we have an 'active' flag set....
348+
// ....then *most* things that are truthy will activate the user. Anything falsey will deactivate them.
349+
// (Specifically, we don't handle a value of '0.0' correctly)
350+
$raw_value = @$results[$i][$ldap_map["active_flag"]][0];
351+
$filter_var = filter_var($raw_value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE);
352+
$boolean_cast = (bool) $raw_value;
353+
354+
$user->activated = $filter_var ?? $boolean_cast; // if filter_var() was true or false, use that. If it's null, use the $boolean_cast
355+
356+
} elseif (array_key_exists('useraccountcontrol', $results[$i])) {
357+
// ....otherwise, (ie if no 'active' LDAP flag is defined), IF the UAC setting exists,
358+
// ....then use the UAC setting on the account to determine can-log-in vs. cannot-log-in
359+
360+
361+
/* The following is _probably_ the correct logic, but we can't use it because
362+
some users may have been dependent upon the previous behavior, and this
363+
could cause additional access to be available to users they don't want
364+
to allow to log in.
365+
366+
$useraccountcontrol = $results[$i]['useraccountcontrol'][0];
367+
if(
368+
// based on MS docs at: https://support.microsoft.com/en-us/help/305144/how-to-use-useraccountcontrol-to-manipulate-user-account-properties
369+
($useraccountcontrol & 0x200) && // is a NORMAL_ACCOUNT
370+
!($useraccountcontrol & 0x02) && // *and* _not_ ACCOUNTDISABLE
371+
!($useraccountcontrol & 0x10) // *and* _not_ LOCKOUT
372+
) {
373+
$user->activated = 1;
374+
} else {
375+
$user->activated = 0;
376+
} */
377+
$enabled_accounts = [
376378
'512', // 0x200 NORMAL_ACCOUNT
377379
'544', // 0x220 NORMAL_ACCOUNT, PASSWD_NOTREQD
378380
'66048', // 0x10200 NORMAL_ACCOUNT, DONT_EXPIRE_PASSWORD
@@ -385,44 +387,47 @@ public function handle()
385387
'4260352', // 0x410200 NORMAL_ACCOUNT, DONT_EXPIRE_PASSWORD, DONT_REQ_PREAUTH
386388
'1049088', // 0x100200 NORMAL_ACCOUNT, NOT_DELEGATED
387389
'1114624', // 0x110200 NORMAL_ACCOUNT, DONT_EXPIRE_PASSWORD, NOT_DELEGATED,
388-
];
389-
$user->activated = (in_array($results[$i]['useraccountcontrol'][0], $enabled_accounts)) ? 1 : 0;
390+
];
391+
$user->activated = (in_array($results[$i]['useraccountcontrol'][0], $enabled_accounts)) ? 1 : 0;
390392

391393
// If we're not using AD, and there isn't an activated flag set, activate all users
392-
} /* implied 'else' here - leave the $user->activated flag alone. Newly-created accounts will be active.
393-
already-existing accounts will be however the administrator has set them */
394+
} /* implied 'else' here - leave the $user->activated flag alone. Newly-created accounts will be active.
395+
already-existing accounts will be however the administrator has set them */
394396

395397

396-
if ($item['ldap_location_override'] == true) {
397-
$user->location_id = $item['location_id'];
398-
} elseif ((isset($location)) && (! empty($location))) {
399-
if ((is_array($location)) && (array_key_exists('id', $location))) {
400-
$user->location_id = $location['id'];
401-
} elseif (is_object($location)) {
402-
$user->location_id = $location->id;
403-
}
398+
if ($item['ldap_location_override'] == true) {
399+
$user->location_id = $item['location_id'];
400+
} elseif ((isset($location)) && (!empty($location))) {
401+
if ((is_array($location)) && (array_key_exists('id', $location))) {
402+
$user->location_id = $location['id'];
403+
} elseif (is_object($location)) {
404+
$user->location_id = $location->id; //THIS is the magic line, this should do it.
405+
}
406+
}
407+
// TODO - should we be NULLING locations if $location is really `null`, and that's what we came up with?
408+
// will that conflict with any overriding setting that the user set? Like, if they moved someone from
409+
// the 'null' location to somewhere, we wouldn't want to try to override that, right?
410+
$location = null;
411+
$user->ldap_import = 1;
412+
413+
$errors = '';
414+
415+
if ($user->save()) {
416+
$item['note'] = $item['createorupdate'];
417+
$item['status'] = 'success';
418+
if ($item['createorupdate'] === 'created' && $ldap_default_group) {
419+
$user->groups()->attach($ldap_default_group);
404420
}
405-
$location = null;
406-
$user->ldap_import = 1;
407-
408-
$errors = '';
409-
410-
if ($user->save()) {
411-
$item['note'] = $item['createorupdate'];
412-
$item['status'] = 'success';
413-
if ( $item['createorupdate'] === 'created' && $ldap_default_group) {
414-
$user->groups()->attach($ldap_default_group);
415-
}
416421

417-
} else {
418-
foreach ($user->getErrors()->getMessages() as $key => $err) {
419-
$errors .= $err[0];
420-
}
421-
$item['note'] = $errors;
422-
$item['status'] = 'error';
422+
} else {
423+
foreach ($user->getErrors()->getMessages() as $key => $err) {
424+
$errors .= $err[0];
423425
}
426+
$item['note'] = $errors;
427+
$item['status'] = 'error';
428+
}
424429

425-
array_push($summary, $item);
430+
array_push($summary, $item);
426431
}
427432

428433
if ($this->option('summary')) {

0 commit comments

Comments
 (0)