From ac5c6123bcb9bd17862662ae3bd38882c1a6fc35 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 2 Feb 2022 18:07:34 -0800 Subject: [PATCH 1/5] Fixes #10563 - LDAP active flag - hopefully? Signed-off-by: snipe --- app/Console/Commands/LdapSync.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/Console/Commands/LdapSync.php b/app/Console/Commands/LdapSync.php index d80e672845..403fe42f56 100755 --- a/app/Console/Commands/LdapSync.php +++ b/app/Console/Commands/LdapSync.php @@ -170,7 +170,6 @@ class LdapSync extends Command $pass = bcrypt($tmp_pass); for ($i = 0; $i < $results["count"]; $i++) { - if (empty($ldap_result_active_flag) || $results[$i][$ldap_result_active_flag][0] == "TRUE") { $item = array(); $item["username"] = isset($results[$i][$ldap_result_username][0]) ? $results[$i][$ldap_result_username][0] : ""; @@ -192,6 +191,11 @@ class LdapSync extends Command $user = User::where('username', $item["username"])->first(); + + // Default to the user not being able to login. We address overriding a little further down + // with an an AD and then LDAP check that overrides + $user->activated = 0; + if ($user) { // Updating an existing user. $item["createorupdate"] = 'updated'; @@ -245,10 +249,9 @@ class LdapSync extends Command '1049088',// 0x100200 NORMAL_ACCOUNT, NOT_DELEGATED ]; $user->activated = ( in_array($results[$i]['useraccountcontrol'][0], $enabled_accounts) ) ? 1 : 0; - } // If we're not using AD, and there isn't an activated flag set, activate all users - elseif (empty($ldap_result_active_flag)) { + } elseif ((empty($ldap_result_active_flag) || $results[$i][$ldap_result_active_flag][0] == "TRUE")) { $user->activated = 1; } @@ -280,7 +283,7 @@ class LdapSync extends Command } array_push($summary, $item); - } + } From db82e06665936e3087f469deff36f275fa51d41c Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 2 Feb 2022 18:22:51 -0800 Subject: [PATCH 2/5] Fixed migration with invalid LDAP prepopulation value Signed-off-by: snipe --- .../2015_11_08_222305_add_ldap_fields_to_settings.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/database/migrations/2015_11_08_222305_add_ldap_fields_to_settings.php b/database/migrations/2015_11_08_222305_add_ldap_fields_to_settings.php index a293b43525..f117dc2f76 100644 --- a/database/migrations/2015_11_08_222305_add_ldap_fields_to_settings.php +++ b/database/migrations/2015_11_08_222305_add_ldap_fields_to_settings.php @@ -23,7 +23,7 @@ class AddLdapFieldsToSettings extends Migration { $table->string('ldap_username_field')->nullable()->default('samaccountname'); $table->string('ldap_lname_field')->nullable()->default('sn'); $table->string('ldap_fname_field')->nullable()->default('givenname'); - $table->string('ldap_auth_filter_query')->nullable()->default('uid=samaccountname'); + $table->string('ldap_auth_filter_query')->nullable()->default('uid='); $table->integer('ldap_version')->nullable()->default(3); $table->string('ldap_active_flag')->nullable()->default(NULL); $table->string('ldap_emp_num')->nullable()->default(NULL); From 392e61688d1eecfebdd67188194c2b412efda073 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Thu, 3 Feb 2022 15:01:45 -0800 Subject: [PATCH 3/5] Rework the LDAP sync command to better handle the active flag --- app/Console/Commands/LdapSync.php | 24 +++++++------- ...2_03_214958_blank_out_ldap_active_flag.php | 31 +++++++++++++++++++ 2 files changed, 43 insertions(+), 12 deletions(-) create mode 100644 database/migrations/2022_02_03_214958_blank_out_ldap_active_flag.php diff --git a/app/Console/Commands/LdapSync.php b/app/Console/Commands/LdapSync.php index 403fe42f56..1cd80652a1 100755 --- a/app/Console/Commands/LdapSync.php +++ b/app/Console/Commands/LdapSync.php @@ -49,7 +49,7 @@ class LdapSync extends Command $ldap_result_last_name = Setting::getSettings()->ldap_lname_field; $ldap_result_first_name = Setting::getSettings()->ldap_fname_field; - $ldap_result_active_flag = Setting::getSettings()->ldap_active_flag_field; + $ldap_result_active_flag = Setting::getSettings()->ldap_active_flag; $ldap_result_emp_num = Setting::getSettings()->ldap_emp_num; $ldap_result_email = Setting::getSettings()->ldap_email; $ldap_result_phone = Setting::getSettings()->ldap_phone_field; @@ -192,10 +192,6 @@ class LdapSync extends Command $user = User::where('username', $item["username"])->first(); - // Default to the user not being able to login. We address overriding a little further down - // with an an AD and then LDAP check that overrides - $user->activated = 0; - if ($user) { // Updating an existing user. $item["createorupdate"] = 'updated'; @@ -203,7 +199,7 @@ class LdapSync extends Command // Creating a new user. $user = new User; $user->password = $pass; - $user->activated = 0; + $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) $item["createorupdate"] = 'created'; } @@ -217,8 +213,14 @@ class LdapSync extends Command $user->country = $item["country"]; $user->department_id = $department->id; - // Sync activated state for Active Directory. - if ( array_key_exists('useraccountcontrol', $results[$i]) ) { + \Log::error("ldap_result_active_flag: $ldap_result_active_flag, value: ".@$results[$i][$ldap_result_active_flag][0]); + + if ( !empty($ldap_result_active_flag)) { // IF we have an 'active' flag set.... + $user->activated = @$results[$i][$ldap_result_active_flag][0] ? 1 : 0; // ....then anything truthy will activate the user, period. Anything falsey will deactivate them. + } elseif (array_key_exists('useraccountcontrol', $results[$i]) ) { + // ....otherwise, (ie if no 'active' LDAP flag is defined), IF the UAC setting exists, + // ....then use the UAC setting on the account to determine can-log-in vs. cannot-log-in + /* The following is _probably_ the correct logic, but we can't use it because some users may have been dependent upon the previous behavior, and this could cause additional access to be available to users they don't want @@ -250,10 +252,8 @@ class LdapSync extends Command ]; $user->activated = ( in_array($results[$i]['useraccountcontrol'][0], $enabled_accounts) ) ? 1 : 0; - // If we're not using AD, and there isn't an activated flag set, activate all users - } elseif ((empty($ldap_result_active_flag) || $results[$i][$ldap_result_active_flag][0] == "TRUE")) { - $user->activated = 1; - } + } /* implied 'else' here - leave the $user->activated flag alone. Newly-created accounts will be active. + already-existing accounts will be however the administrator has set them */ if ($item['ldap_location_override'] == true) { $user->location_id = $item['location_id']; diff --git a/database/migrations/2022_02_03_214958_blank_out_ldap_active_flag.php b/database/migrations/2022_02_03_214958_blank_out_ldap_active_flag.php new file mode 100644 index 0000000000..db0f5bf803 --- /dev/null +++ b/database/migrations/2022_02_03_214958_blank_out_ldap_active_flag.php @@ -0,0 +1,31 @@ +ldap_active_flag = ''; + $s->save(); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + // + } +} From 1945b97b72673ec2695633abc86ecaa82798413b Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Thu, 3 Feb 2022 19:04:56 -0800 Subject: [PATCH 4/5] Just trying to really tighten up on the LDAP Active Flag and how we parse it. --- app/Console/Commands/LdapSync.php | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/app/Console/Commands/LdapSync.php b/app/Console/Commands/LdapSync.php index 1cd80652a1..29c98ec424 100755 --- a/app/Console/Commands/LdapSync.php +++ b/app/Console/Commands/LdapSync.php @@ -213,10 +213,23 @@ class LdapSync extends Command $user->country = $item["country"]; $user->department_id = $department->id; - \Log::error("ldap_result_active_flag: $ldap_result_active_flag, value: ".@$results[$i][$ldap_result_active_flag][0]); + if(@$results[$i][$ldap_result_active_flag][0]) { + \Log::error("ldap_result_active_flag: $ldap_result_active_flag, value: ".@$results[$i][$ldap_result_active_flag][0]); + } if ( !empty($ldap_result_active_flag)) { // IF we have an 'active' flag set.... - $user->activated = @$results[$i][$ldap_result_active_flag][0] ? 1 : 0; // ....then anything truthy will activate the user, period. Anything falsey will deactivate them. + //\Log::error("WE HAVE AN ACTIVE FLAG! We are going to set activated TO: ".(@$results[$i][$ldap_result_active_flag][0] ? 1 : 0)); + // $parsed_active_flag = filter_var(@$results[$i][$ldap_result_active_flag][0], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); + // $user->activated = $parsed_active_flag ?? true; // ....then anything truthy will activate the user, period. Anything falsey will deactivate them. + // (and anything even weirder than that will process as 'true' I guess?) + $raw_value = @$results[$i][$ldap_result_active_flag][0]; + $filter_var = filter_var($raw_value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); + $boolean_cast = (bool)$raw_value; + if(!is_null($raw_value)) { + \Log::error("We have an active flag! filter_var for '$raw_value' says: ".(is_null($filter_var) ? 'NULL': ($filter_var ? 'true' : 'false'))." but boolean cast says: ".($boolean_cast ? 'true': 'false')." And string compare is: ".($raw_value > 0 ? 'true' : 'false')); + } + $user->activated = $filter_var ?? $boolean_cast; // this seems clever but it does pretty much exactly what I want. (I think?) No, it doesn't. + } elseif (array_key_exists('useraccountcontrol', $results[$i]) ) { // ....otherwise, (ie if no 'active' LDAP flag is defined), IF the UAC setting exists, // ....then use the UAC setting on the account to determine can-log-in vs. cannot-log-in @@ -272,6 +285,7 @@ class LdapSync extends Command $errors = ''; if ($user->save()) { + //\Log::info("We have done a save, and it was succesful! Results: ".print_r($user,true)); $item["note"] = $item["createorupdate"]; $item["status"]='success'; } else { From 36ae6f9430adf54a0b3ebe71dc98e1b08fe1ccd3 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Thu, 3 Feb 2022 19:41:16 -0800 Subject: [PATCH 5/5] Yanked debugging code, tightened up comments. --- app/Console/Commands/LdapSync.php | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/app/Console/Commands/LdapSync.php b/app/Console/Commands/LdapSync.php index 29c98ec424..cbd98727ea 100755 --- a/app/Console/Commands/LdapSync.php +++ b/app/Console/Commands/LdapSync.php @@ -213,25 +213,17 @@ class LdapSync extends Command $user->country = $item["country"]; $user->department_id = $department->id; - if(@$results[$i][$ldap_result_active_flag][0]) { - \Log::error("ldap_result_active_flag: $ldap_result_active_flag, value: ".@$results[$i][$ldap_result_active_flag][0]); - } - if ( !empty($ldap_result_active_flag)) { // IF we have an 'active' flag set.... - //\Log::error("WE HAVE AN ACTIVE FLAG! We are going to set activated TO: ".(@$results[$i][$ldap_result_active_flag][0] ? 1 : 0)); - // $parsed_active_flag = filter_var(@$results[$i][$ldap_result_active_flag][0], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); - // $user->activated = $parsed_active_flag ?? true; // ....then anything truthy will activate the user, period. Anything falsey will deactivate them. - // (and anything even weirder than that will process as 'true' I guess?) + // ....then *most* things that are truthy will activate the user. Anything falsey will deactivate them. + // (Specifically, we don't handle a value of '0.0' correctly) $raw_value = @$results[$i][$ldap_result_active_flag][0]; $filter_var = filter_var($raw_value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); $boolean_cast = (bool)$raw_value; - if(!is_null($raw_value)) { - \Log::error("We have an active flag! filter_var for '$raw_value' says: ".(is_null($filter_var) ? 'NULL': ($filter_var ? 'true' : 'false'))." but boolean cast says: ".($boolean_cast ? 'true': 'false')." And string compare is: ".($raw_value > 0 ? 'true' : 'false')); - } - $user->activated = $filter_var ?? $boolean_cast; // this seems clever but it does pretty much exactly what I want. (I think?) No, it doesn't. - } elseif (array_key_exists('useraccountcontrol', $results[$i]) ) { - // ....otherwise, (ie if no 'active' LDAP flag is defined), IF the UAC setting exists, + $user->activated = $filter_var ?? $boolean_cast; // if filter_var() was true or false, use that. If it's null, use the $boolean_cast + + } elseif ( array_key_exists('useraccountcontrol', $results[$i]) ) { + // ....otherwise, (ie if no 'active' LDAP flag is defined), IF the UAC setting exists, // ....then use the UAC setting on the account to determine can-log-in vs. cannot-log-in /* The following is _probably_ the correct logic, but we can't use it because @@ -285,7 +277,6 @@ class LdapSync extends Command $errors = ''; if ($user->save()) { - //\Log::info("We have done a save, and it was succesful! Results: ".print_r($user,true)); $item["note"] = $item["createorupdate"]; $item["status"]='success'; } else { @@ -297,7 +288,6 @@ class LdapSync extends Command } array_push($summary, $item); - }