From 31933a56fae554becbc36355de17620249296c2e Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Wed, 7 Jul 2021 15:24:23 -0700 Subject: [PATCH 1/7] Trying to get the login screen working --- app/Http/Livewire/LoginForm.php | 13 ++++++++----- resources/views/livewire/login-form.blade.php | 4 ++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/Http/Livewire/LoginForm.php b/app/Http/Livewire/LoginForm.php index 7d10cb78a..f4fe02f28 100644 --- a/app/Http/Livewire/LoginForm.php +++ b/app/Http/Livewire/LoginForm.php @@ -38,11 +38,14 @@ class LoginForm extends Component $this->can_submit = false; } - $this->validateOnly($fields); - - $this->can_submit = true; - + $whatever = $this->validateOnly($fields); + //\Log::info(print_r($whatever,true)); + $errors = $this->getErrorBag(); + + $this->can_submit = $this->username !== "" && $this->password !== "" && !$errors->has('username') && !$errors->has('password') ; // wait, what? + + \Log::info("Oy - can we submit yet?!".$this->can_submit); } /** @@ -58,7 +61,7 @@ class LoginForm extends Component public function submitForm() { - $this->can_submit = true; + //$this->can_submit = true; if (auth()->attempt($this->validate())) { return redirect()->intended('/'); diff --git a/resources/views/livewire/login-form.blade.php b/resources/views/livewire/login-form.blade.php index d7cad8250..6561adc8e 100644 --- a/resources/views/livewire/login-form.blade.php +++ b/resources/views/livewire/login-form.blade.php @@ -41,7 +41,7 @@ {{ trans('admin/users/table.username') }} - + @error('username') {{ $message }} @@ -53,7 +53,7 @@ {{ trans('admin/users/table.password') }} - + @error('password') {{ $message }} From 4dda28de9ec2d13df7ab67e9395bdaec79dfa504 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Tue, 27 Jul 2021 20:09:58 -0700 Subject: [PATCH 2/7] WIP: cleaning up LDAP --- app/Console/Commands/LdapSyncNg.php | 398 ------------ .../Controllers/Api/SettingsController.php | 1 - .../Users/LDAPImportController.php | 23 +- app/Providers/LdapServiceProvider.php | 39 -- app/Services/LdapAd.php | 572 ------------------ app/Services/LdapAdConfiguration.php | 311 ---------- composer.json | 1 - composer.lock | 121 +--- config/app.php | 1 - 9 files changed, 3 insertions(+), 1464 deletions(-) delete mode 100644 app/Console/Commands/LdapSyncNg.php delete mode 100644 app/Providers/LdapServiceProvider.php delete mode 100644 app/Services/LdapAd.php delete mode 100644 app/Services/LdapAdConfiguration.php diff --git a/app/Console/Commands/LdapSyncNg.php b/app/Console/Commands/LdapSyncNg.php deleted file mode 100644 index b271c6c10..000000000 --- a/app/Console/Commands/LdapSyncNg.php +++ /dev/null @@ -1,398 +0,0 @@ - - * - * @since 5.0.0 - */ -class LdapSyncNg extends Command -{ - /** - * The name and signature of the console command. - * - * @var string - */ - protected $signature = 'snipeit:ldap-sync-ng - {--location= : A location name } - {--location_id= : A location id} - {--base_dn= : A diffrent base DN to use } - {--summary : Print summary } - {--json_summary : Print summary in json format } - {--dryrun : Run the sync process but don\'t update the database}'; - - /** - * The console command description. - * - * @var string - */ - protected $description = 'Command line LDAP/AD sync'; - - /** - * An LdapAd instance. - * - * @var \App\Models\LdapAd - */ - private $ldap; - - /** - * LDAP settings collection. - * - * @var \Illuminate\Support\Collection - */ - private $settings = null; - - /** - * A default location collection. - * - * @var \Illuminate\Support\Collection - */ - private $defaultLocation = null; - - /** - * Mapped locations collection. - * - * @var \Illuminate\Support\Collection - */ - private $mappedLocations = null; - - /** - * The summary collection. - * - * @var \Illuminate\Support\Collection - */ - private $summary; - - /** - * Is dry-run? - * - * @var bool - */ - private $dryrun = false; - - /** - * Show users to be imported. - * - * @var array - */ - private $userlist = []; - - /** - * Create a new command instance. - */ - public function __construct(LdapAd $ldap) - { - parent::__construct(); - $this->ldap = $ldap; - $this->settings = $this->ldap->ldapSettings; - $this->summary = collect(); - } - - /** - * Execute the console command. - * - * @return mixed - */ - public function handle() - { - $dispatcher = \Adldap\Adldap::getEventDispatcher(); - - // Listen for all model events. - $dispatcher->listen('Adldap\Models\Events\*', function ($eventName, array $data) { - echo $eventName; // Returns 'Adldap\Models\Events\Updating' - var_dump($data); // Returns [0] => (object) Adldap\Models\Events\Updating; - \Log::debug('Event: '.$eventName.' data - '.print_r($data, true)); - }); - $dispatcher->listen('Adldap\Auth\Events\*', function ($eventName, array $data) { - echo $eventName; // Returns 'Adldap\Models\Events\Updating' - var_dump($data); // Returns [0] => (object) Adldap\Models\Events\Updating; - \Log::debug('Event: '.$eventName.' data - '.print_r($data, true)); - }); - - ini_set('max_execution_time', env('LDAP_TIME_LIM', '600')); //600 seconds = 10 minutes - ini_set('memory_limit', '500M'); - $old_error_reporting = error_reporting(); // grab old error_reporting .ini setting, for later re-enablement - error_reporting($old_error_reporting & ~E_DEPRECATED); // disable deprecation warnings, for LDAP in PHP 7.4 (and greater) - - if ($this->option('dryrun')) { - $this->dryrun = true; - } - $this->checkIfLdapIsEnabled(); - $this->checkLdapConnection(); - $this->setBaseDn(); - $this->getUserDefaultLocation(); - /* - * Use the default location if set, this is needed for the LDAP users sync page - */ - if (! $this->option('base_dn') && null == $this->defaultLocation) { - $this->getMappedLocations(); - } - $this->processLdapUsers(); - // Print table of users - if ($this->dryrun) { - $this->info('The following users will be synced!'); - $headers = ['First Name', 'Last Name', 'Username', 'Email', 'Employee #', 'Location Id', 'Status']; - $this->table($headers, $this->summary->toArray()); - } - - error_reporting($old_error_reporting); // re-enable deprecation warnings. - - return $this->getSummary(); - } - - /** - * Generate the LDAP sync summary. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return string - */ - private function getSummary(): string - { - if ($this->option('summary') && null === $this->dryrun) { - $this->summary->each(function ($item) { - $this->info('USER: '.$item['note']); - - if ('ERROR' === $item['status']) { - $this->error('ERROR: '.$item['note']); - } - }); - } elseif ($this->option('json_summary')) { - $json_summary = [ - 'error' => false, - 'error_message' => '', - 'summary' => $this->summary->toArray(), - ]; - $this->info(json_encode($json_summary)); - } - - return ''; - } - - /** - * Create a new user or update an existing user. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param \Adldap\Models\User $snipeUser - */ - private function updateCreateUser(AdldapUser $snipeUser): void - { - $user = $this->ldap->processUser($snipeUser, $this->defaultLocation, $this->mappedLocations); - $summary = [ - 'firstname' => $user->first_name, - 'lastname' => $user->last_name, - 'username' => $user->username, - 'employee_number' => $user->employee_num, - 'email' => $user->email, - 'location_id' => $user->location_id, - ]; - // Only update the database if is not a dry run - if (! $this->dryrun) { - if ($user->isDirty()) { //if nothing on the user changed, don't bother trying to save anything nor put anything in the summary - if ($user->save()) { - $summary['note'] = ($user->wasRecentlyCreated ? 'CREATED' : 'UPDATED'); - $summary['status'] = 'SUCCESS'; - } else { - $errors = ''; - foreach ($user->getErrors()->getMessages() as $error) { - $errors .= implode(', ', $error); - } - $summary['note'] = $snipeUser->getDN().' was not imported. REASON: '.$errors; - $summary['status'] = 'ERROR'; - } - } else { - $summary = null; - } - } - - // $summary['note'] = ($user->getOriginal('username') ? 'UPDATED' : 'CREATED'); // this seems, kinda, like, superfluous, relative to the $summary['note'] thing above, yeah? - if ($summary) { //if the $user wasn't dirty, $summary was set to null so that we will skip the following push() - $this->summary->push($summary); - } - } - - /** - * Process the users to update / create. - * - * @author Wes Hulette - * - * @since 5.0.0 - */ - private function processLdapUsers(): void - { - try { - \Log::debug('CAL:LING GET LDAP SUSERS'); - $ldapUsers = $this->ldap->getLdapUsers(); - \Log::debug('END CALLING GET LDAP USERS'); - } catch (Exception $e) { - $this->outputError($e); - exit($e->getMessage()); - } - - if (0 == $ldapUsers->count()) { - $msg = 'ERROR: No users found!'; - Log::error($msg); - if ($this->dryrun) { - $this->error($msg); - } - exit($msg); - } - - // Process each individual users - foreach ($ldapUsers->getResults() as $user) { // AdLdap2's paginate() method is weird, it gets *everything* and ->getResults() returns *everything* - $this->updateCreateUser($user); - } - } - - /** - * Get the mapped locations if a base_dn is provided. - * - * @author Wes Hulette - * - * @since 5.0.0 - */ - private function getMappedLocations() - { - $ldapOuLocation = Location::where('ldap_ou', '!=', '')->select(['id', 'ldap_ou'])->get(); - $locations = $ldapOuLocation->sortBy(function ($ou, $key) { - return strlen($ou->ldap_ou); - }); - if ($locations->count() > 0) { - $msg = 'Some locations have special OUs set. Locations will be automatically set for users in those OUs.'; - LOG::debug($msg); - if ($this->dryrun) { - $this->info($msg); - } - - $this->mappedLocations = $locations->pluck('ldap_ou', 'id'); // TODO: this seems ok-ish, but the key-> value is going location_id -> OU name, and the primary action here is the opposite of that - going from OU's to location ID's. - } - } - - /** - * Set the base dn if supplied. - * - * @author Wes Hulette - * - * @since 5.0.0 - */ - private function setBaseDn(): void - { - if ($this->option('base_dn')) { - $this->ldap->baseDn = $this->option('base_dn'); - $msg = sprintf('Importing users from specified base DN: "%s"', $this->ldap->baseDn); - LOG::debug($msg); - if ($this->dryrun) { - $this->info($msg); - } - } - } - - /** - * Get a default location id for imported users. - * - * @author Wes Hulette - * - * @since 5.0.0 - */ - private function getUserDefaultLocation(): void - { - $location = $this->option('location_id') ?? $this->option('location'); - if ($location) { - $userLocation = Location::where('name', '=', $location) - ->orWhere('id', '=', intval($location)) - ->select(['name', 'id']) - ->first(); - if ($userLocation) { - $msg = 'Importing users with default location: '.$userLocation->name.' ('.$userLocation->id.')'; - LOG::debug($msg); - - if ($this->dryrun) { - $this->info($msg); - } - - $this->defaultLocation = collect([ - $userLocation->id => $userLocation->name, - ]); - } else { - $msg = 'The supplied location is invalid!'; - LOG::error($msg); - if ($this->dryrun) { - $this->error($msg); - } - exit(0); - } - } - } - - /** - * Check if LDAP intergration is enabled. - * - * @author Wes Hulette - * - * @since 5.0.0 - */ - private function checkIfLdapIsEnabled(): void - { - if (false === $this->settings['ldap_enabled']) { - $msg = 'LDAP intergration is not enabled. Exiting sync process.'; - $this->info($msg); - Log::info($msg); - exit(0); - } - } - - /** - * Check to make sure we can access the server. - * - * @author Wes Hulette - * - * @since 5.0.0 - */ - private function checkLdapConnection(): void - { - try { - $this->ldap->testLdapAdUserConnection(); - $this->ldap->testLdapAdBindConnection(); - } catch (Exception $e) { - $this->outputError($e); - exit(0); - } - } - - /** - * Output the json summary to the screen if enabled. - * - * @param Exception $error - */ - private function outputError($error): void - { - if ($this->option('json_summary')) { - $json_summary = [ - 'error' => true, - 'error_message' => $error->getMessage(), - 'summary' => [], - ]; - $this->info(json_encode($json_summary)); - } - $this->error($error->getMessage()); - LOG::error($error); - } -} diff --git a/app/Http/Controllers/Api/SettingsController.php b/app/Http/Controllers/Api/SettingsController.php index 852530c20..59236731e 100644 --- a/app/Http/Controllers/Api/SettingsController.php +++ b/app/Http/Controllers/Api/SettingsController.php @@ -7,7 +7,6 @@ use App\Http\Transformers\LoginAttemptsTransformer; use App\Models\Ldap; use App\Models\Setting; use App\Notifications\MailTest; -use App\Services\LdapAd; use GuzzleHttp\Client; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; diff --git a/app/Http/Controllers/Users/LDAPImportController.php b/app/Http/Controllers/Users/LDAPImportController.php index a204581eb..88a6b207d 100644 --- a/app/Http/Controllers/Users/LDAPImportController.php +++ b/app/Http/Controllers/Users/LDAPImportController.php @@ -4,32 +4,12 @@ namespace App\Http\Controllers\Users; use App\Http\Controllers\Controller; use App\Models\User; -use App\Services\LdapAd; use Illuminate\Http\Request; use Illuminate\Support\Facades\Artisan; // Note that this is awful close to 'Users' the namespace above; be careful class LDAPImportController extends Controller { - /** - * An Ldap instance. - * - * @var LdapAd - */ - protected $ldap; - - /** - * __construct. - * - * @param LdapAd $ldap - */ - public function __construct(LdapAd $ldap) - { - parent::__construct(); - $this->ldap = $ldap; - $this->ldap->init(); - } - - /** + /** * Return view for LDAP import. * * @author Aladin Alaily @@ -43,6 +23,7 @@ class LDAPImportController extends Controller */ public function create() { + // I guess this prolly oughtta... I dunno. Do something? $this->authorize('update', User::class); try { //$this->ldap->connect(); I don't think this actually exists in LdapAd.php, and we don't really 'persist' LDAP connections anyways...right? diff --git a/app/Providers/LdapServiceProvider.php b/app/Providers/LdapServiceProvider.php deleted file mode 100644 index 58f93096d..000000000 --- a/app/Providers/LdapServiceProvider.php +++ /dev/null @@ -1,39 +0,0 @@ -app->singleton(LdapAd::class, LdapAd::class); - } - - /** - * Get the services provided by the provider. - * - * @return array - */ - public function provides() - { - return [LdapAd::class]; - } -} diff --git a/app/Services/LdapAd.php b/app/Services/LdapAd.php deleted file mode 100644 index 186a2c7a9..000000000 --- a/app/Services/LdapAd.php +++ /dev/null @@ -1,572 +0,0 @@ - - * - * @since 5.0.0 - */ -class LdapAd extends LdapAdConfiguration -{ - /* 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 - to allow to log in. - $useraccountcontrol = $results[$i]['useraccountcontrol'][0]; - if( - // based on MS docs at: https://support.microsoft.com/en-us/help/305144/how-to-use-useraccountcontrol-to-manipulate-user-account-properties - ($useraccountcontrol & 0x200) && // is a NORMAL_ACCOUNT - !($useraccountcontrol & 0x02) && // *and* _not_ ACCOUNTDISABLE - !($useraccountcontrol & 0x10) // *and* _not_ LOCKOUT - ) { - $user->activated = 1; - } else { - $user->activated = 0; - } */ - const AD_USER_ACCOUNT_CONTROL_FLAGS = [ - '512', // 0x200 NORMAL_ACCOUNT - '544', // 0x220 NORMAL_ACCOUNT, PASSWD_NOTREQD - '66048', // 0x10200 NORMAL_ACCOUNT, DONT_EXPIRE_PASSWORD - '66080', // 0x10220 NORMAL_ACCOUNT, PASSWD_NOTREQD, DONT_EXPIRE_PASSWORD - '262656', // 0x40200 NORMAL_ACCOUNT, SMARTCARD_REQUIRED - '262688', // 0x40220 NORMAL_ACCOUNT, PASSWD_NOTREQD, SMARTCARD_REQUIRED - '328192', // 0x50200 NORMAL_ACCOUNT, SMARTCARD_REQUIRED, DONT_EXPIRE_PASSWORD - '328224', // 0x50220 NORMAL_ACCOUNT, PASSWD_NOT_REQD, SMARTCARD_REQUIRED, DONT_EXPIRE_PASSWORD - '4260352', // 0x410200 NORMAL_ACCOUNT, DONT_EXPIRE_PASSWORD, DONT_REQ_PREAUTH - '1049088', // 0x100200 NORMAL_ACCOUNT, NOT_DELEGATED - '1114624', // 0x110200 NORMAL_ACCOUNT, NOT_DELEGATED, DONT_EXPIRE_PASSWORD - ]; - - /** - * The LDAP results per page. - */ - const PAGE_SIZE = 500; - - /** - * A base dn. - * - * @var string - */ - public $baseDn = null; - - /** - * Adldap instance. - * - * @var \Adldap\Adldap - */ - protected $ldap; - - /** - * Initialize LDAP from user settings - * - * @since 5.0.0 - * - * @return void - */ - public function init() - { - // Already initialized - if ($this->ldap) { - return true; - } - - parent::init(); - if ($this->isLdapEnabled()) { - if ($this->ldapSettings['is_ad'] == 0) { //only for NON-AD setups! - $this->ldapConfig['account_prefix'] = $this->ldapSettings['ldap_auth_filter_query']; - $this->ldapConfig['account_suffix'] = ','.$this->ldapConfig['base_dn']; - } /* - To the point mentioned in ldapLogin(), we might want to add an 'else' clause here that - sets up an 'account_suffix' of '@'.$this->ldapSettings['ad_domain'] *IF* the user has - $this->ldapSettings['ad_append_domain'] enabled. - That code in ldapLogin gets simplified, in exchange for putting all the weirdness here only. - */ - $this->ldap = new Adldap(); - $this->ldap->addProvider($this->ldapConfig); - - return true; - } - - return false; - } - - public function __construct() - { - $this->init(); - } - - /** - * Create a user if they successfully login to the LDAP server. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param string $username - * @param string $password - * - * @return \App\Models\User - * - * @throws Exception - */ - public function ldapLogin(string $username, string $password): User - { - if ($this->ldapSettings['ad_append_domain']) { //if you're using 'userprincipalname', don't check the ad_append_domain checkbox - $login_username = $username.'@'.$this->ldapSettings['ad_domain']; // I feel like could can be solved with the 'suffix' feature? Then this would be easier. - } else { - $login_username = $username; - } - - if ($this->ldapConfig['username'] && $this->ldapConfig['password']) { - $bind_as_user = false; - } else { - $bind_as_user = true; - } - - if (($this->ldap) && ($this->ldap->auth()->attempt($login_username, $password, $bind_as_user) === false)) { - throw new Exception('Unable to validate user credentials!'); - } - - // Should we sync the logged in user - Log::debug('Attempting to find user in LDAP directory'); - $record = $this->ldap->search()->findBy($this->ldapSettings['ldap_username_field'], $username); - - if ($record) { - if ($this->isLdapSync($record)) { - $this->syncUserLdapLogin($record, $password); - } - } else { - throw new Exception('Unable to find user in LDAP directory!'); - } - - $user = User::where('username', $username) - ->whereNull('deleted_at')->where('ldap_import', '=', 1) - ->where('activated', '=', '1')->first(); - /* Above, I could've just done ->firstOrFail() which would've been cleaner, but it would've been miserable to - troubleshoot if it ever came up (giving a really generic and untraceable error message) - */ - if (! $user) { - throw new Exception("User is either deleted, not activated (can't log in), not from LDAP, or can't be found in database"); - } - - return $user; - } - - /** - * Set the user information based on the LDAP settings. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param \Adldap\Models\User $user - * @param null|Collection $defaultLocation - * @param null|Collection $mappedLocations - * - * @return null|\App\Models\User - */ - public function processUser(AdldapUser $user, ?Collection $defaultLocation = null, ?Collection $mappedLocations = null): ?User - { - // Only sync active users <- I think this actually means 'existing', not 'activated/deactivated' - if (! $user) { - return null; - } - $snipeUser = []; - $snipeUser['username'] = $user->{$this->ldapSettings['ldap_username_field']}[0] ?? ''; - $snipeUser['employee_number'] = $user->{$this->ldapSettings['ldap_emp_num']}[0] ?? ''; - $snipeUser['lastname'] = $user->{$this->ldapSettings['ldap_lname_field']}[0] ?? ''; - $snipeUser['firstname'] = $user->{$this->ldapSettings['ldap_fname_field']}[0] ?? ''; - $snipeUser['email'] = $user->{$this->ldapSettings['ldap_email']}[0] ?? ''; - $snipeUser['title'] = $user->getTitle() ?? ''; - $snipeUser['telephonenumber'] = $user->getTelephoneNumber() ?? ''; - - /* - * $locationId being 'null' means we have no per-OU location information, - * but instead of explicitly setting it to null - which would override any admin-generated - * location assignments - we just don't set it at all. For a brand new User, the 'default null' - * on the column will cover us. For an already existing user, this will not override any - * locations that were explicitly chosen by the administrators. - * - * When syncing with a particular 'default location' in mind, those should still be respected - * and it *will* override the administrators previous choices. I think this is a fair compromise. - */ - $locationId = $this->getLocationId($user, $defaultLocation, $mappedLocations); - if ($locationId !== null) { - $snipeUser['location_id'] = $locationId; - } - - $activeStatus = $this->getActiveStatus($user); - if ($activeStatus !== null) { - $snipeUser['activated'] = $activeStatus; - } - - return $this->setUserModel($snipeUser); - } - - /** - * Set the User model information. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param array $userInfo The user info to save to the database - * - * @return \App\Models\User - */ - public function setUserModel(array $userInfo): User - { - // If the username exists, return the user object, otherwise create a new user object - $user = User::firstOrNew([ - 'username' => $userInfo['username'], - ]); - $user->username = $user->username ?? trim($userInfo['username']); - $user->password = $user->password ?? Helper::generateEncyrptedPassword(); - $user->first_name = trim($userInfo['firstname']); - $user->last_name = trim($userInfo['lastname']); - $user->email = trim($userInfo['email']); - $user->employee_num = trim($userInfo['employee_number']); - $user->jobtitle = trim($userInfo['title']); - $user->phone = trim($userInfo['telephonenumber']); - if (array_key_exists('activated', $userInfo)) { - $user->activated = $userInfo['activated']; - } elseif (! $user->exists) { // no 'activated' flag was set or unset, *AND* this user is new - activate by default. - $user->activated = 1; - } - if (array_key_exists('location_id', $userInfo)) { - $user->location_id = $userInfo['location_id']; - } - - // this is a new user - if (! isset($user->id)) { - $user->notes = 'Imported from LDAP'; - } - - $user->ldap_import = 1; - - return $user; - } - - /** - * Sync a user who has logged in by LDAP. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param \Adldap\Models\User $record - * @param string $password - * - * @throws Exception - */ - private function syncUserLdapLogin(AdldapUser $record, string $password): void - { - $user = $this->processUser($record); - - if (is_null($user->last_login)) { - $user->notes = 'Imported on first login from LDAP2'; - } - - if ($this->ldapSettings['ldap_pw_sync']) { - Log::debug('Syncing users password with LDAP directory.'); - $user->password = bcrypt($password); - } - - if (! $user->save()) { - Log::debug('Could not save user. '.$user->getErrors()); - throw new Exception('Could not save user: '.$user->getErrors()); - } - } - - /** - * Check to see if we should sync the user with the LDAP directory. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param \Adldap\Models\User $user - * - * @return bool - */ - private function isLdapSync(AdldapUser $user): bool - { - if (! $this->ldapSettings['ldap_active_flag']) { - return true; // always sync if you didn't define an 'active' flag - } - - if ($user->{$this->ldapSettings['ldap_active_flag']} && // if your LDAP user has the aforementioned flag as an attribute *AND* - count($user->{$this->ldapSettings['ldap_active_flag']}) == 1 && // if that attribute has exactly one value *AND* - strtolower($user->{$this->ldapSettings['ldap_active_flag']}[0]) == 'false') { // that value is the string 'false' (regardless of case), - return false; // then your user is *INACTIVE* - return false - } - // otherwise, return true - return true; - } - - /** - * Set the active status of the user. - * Returns 0 or 1 if the user is deactivated or activated - * or returns null if we just don't know - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param \Adldap\Models\User $user - * - * @return int (or null) - */ - private function getActiveStatus(AdldapUser $user): ?int - { - /* - * Check to see if we are connected to an AD server - * if so, check the Active Directory User Account Control Flags - * If the admin has set their own 'active flag' - respect that instead - * (this may work to allow AD users to ignore the built-in UAC stuff that AD does) - */ - if ($user->hasAttribute($user->getSchema()->userAccountControl()) && ! $this->ldapSettings['ldap_active_flag']) { - \Log::debug('This is AD - userAccountControl is'.$user->getSchema()->userAccountControl()); - $activeStatus = (in_array($user->getUserAccountControl(), self::AD_USER_ACCOUNT_CONTROL_FLAGS)) ? 1 : 0; - } else { - - //\Log::debug('This looks like LDAP (or an AD where the UAC is disabled)'); - // If there is no activated flag, then we can't make any determination about activated/deactivated - if (false == $this->ldapSettings['ldap_active_flag']) { - \Log::debug('ldap_active_flag is false - no ldap_active_flag is set'); - - return null; - } - - // If there *is* an activated flag, then respect it *only* if it is actually present. If it's not there, ignore it. - if (! $user->hasAttribute($this->ldapSettings['ldap_active_flag'])) { - return null; // 'active' flag is defined, but does not exist on returned user record. So we don't know if they're active or not. - } - - // if $user has the flag *AND* that flag has exactly one value - - if ($user->{$this->ldapSettings['ldap_active_flag']} && count($user->{$this->ldapSettings['ldap_active_flag']}) == 1) { - $active_flag_value = $user->{$this->ldapSettings['ldap_active_flag']}[0]; - - // if the value of that flag is case-insensitively the string 'false' or boolean false - if (strcasecmp($active_flag_value, 'false') == 0 || $active_flag_value === false) { - return 0; // then make them INACTIVE - } else { - return 1; // otherwise active - } - } - - return 1; // fail 'open' (active) if we have the attribute and it's multivalued or empty; that's weird - } - - return $activeStatus; - } - - /** - * Get a default selected location, or a OU mapped location if available. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param \Adldap\Models\User $user - * @param Collection|null $defaultLocation - * @param Collection|null $mappedLocations - * - * @return null|int - */ - private function getLocationId(AdldapUser $user, ?Collection $defaultLocation, ?Collection $mappedLocations): ?int - { - $locationId = null; - // Set the users default locations, if set - if ($defaultLocation) { - $locationId = $defaultLocation->keys()->first(); - } - - // Check to see if the user is in a mapped location - if ($mappedLocations) { - $location = $mappedLocations->filter(function ($value, $key) use ($user) { - //if ($user->inOu($value)) { // <----- *THIS* seems not to be working, and it seems more 'intelligent' - but it's literally just a strpos() call, and it doesn't work quite right against plain strings - $user_ou = substr($user->getDn(), -strlen($value)); // get the LAST chars of the user's DN, the count of those chars being the length of the thing we're checking against - if (strcasecmp($user_ou, $value) === 0) { // case *IN*sensitive comparision - some people say OU=blah, some say ou=blah. returns 0 when strings are identical (which is a little odd, yeah) - return $key; // WARNING: we are doing a 'filter' - not a regular for-loop. So the answer(s) get "return"ed into the $location array - } - }); - - if ($location->count() > 0) { - $locationId = $location->keys()->first(); // from the returned $location array from the ->filter() method above, we return the first match - there should be only one - } - } - - return $locationId; - } - - /** - * Get the base dn for the query. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return string - */ - private function getBaseDn(): string - { - if (! is_null($this->baseDn)) { - return $this->baseDn; - } - - return $this->ldapSettings['ldap_basedn']; - } - - /** - * Format the ldap filter if needed. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return null|string - */ - private function getFilter(): ?string - { - $filter = $this->ldapSettings['ldap_filter']; - if (! $filter) { - return null; - } - // Add surrounding parentheses as needed - $paren = mb_substr($filter, 0, 1, 'utf-8'); - if ('(' !== $paren) { - return '('.$filter.')'; - } - - return $filter; - } - - /** - * Get the selected fields to return - * This should help with memory on large result sets as we are not returning all fields. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return array - */ - private function getSelectedFields(): array - { - /** @var Schema $schema */ - $schema = new $this->ldapConfig['schema']; - - return array_values(array_filter([ - $this->ldapSettings['ldap_username_field'], - $this->ldapSettings['ldap_fname_field'], - $this->ldapSettings['ldap_lname_field'], - $this->ldapSettings['ldap_email'], - $this->ldapSettings['ldap_emp_num'], - $this->ldapSettings['ldap_active_flag'], - $schema->memberOf(), - $schema->userAccountControl(), - $schema->title(), - $schema->telephone(), - ])); - } - - /** - * Test the bind user connection. - * - * @author Wes Hulette - * @throws \Exception - * @since 5.0.0 - */ - public function testLdapAdBindConnection(): void - { - try { - $this->ldap->search()->ous()->get()->count(); //it's saying this is null? - } catch (Exception $th) { - Log::error($th->getMessage()); - throw new Exception('Unable to search LDAP directory!'); - } - } - - /** - * Test the user can connect to the LDAP server. - * - * @author Wes Hulette - * @throws \Exception - * @since 5.0.0 - */ - public function testLdapAdUserConnection(): void - { - try { - $this->ldap->connect(); - } catch (\Exception $e) { - Log::debug('LDAP ERROR: '.$e->getMessage()); - throw new Exception($e->getMessage()); - } - } - - /** - * Test the LDAP configuration by returning up to 10 users. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return Collection - */ - public function testUserImportSync(): Collection - { - $testUsers = collect($this->getLdapUsers()->getResults())->chunk(10)->first(); - if ($testUsers) { - return $testUsers->map(function ($item) { - return (object) [ - 'username' => $item->{$this->ldapSettings['ldap_username_field']}[0] ?? null, - 'employee_number' => $item->{$this->ldapSettings['ldap_emp_num']}[0] ?? null, - 'lastname' => $item->{$this->ldapSettings['ldap_lname_field']}[0] ?? null, - 'firstname' => $item->{$this->ldapSettings['ldap_fname_field']}[0] ?? null, - 'email' => $item->{$this->ldapSettings['ldap_email']}[0] ?? null, - ]; - }); - } - - return collect(); - } - - /** - * Query the LDAP server to get the users to process and return a page set. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return \Adldap\Query\Paginator - */ - public function getLdapUsers(): Paginator - { - $search = $this->ldap->search()->users()->in($this->getBaseDn()); //this looks wrong; we should instead have a passable parameter that does this, and use this as a 'sane' default, yeah? - - $filter = $this->getFilter(); - if (! is_null($filter)) { - $search = $search->rawFilter($filter); - } - //I think it might be possible to potentially do our own paging here? - - return $search->select($this->getSelectedFields()) - ->paginate(self::PAGE_SIZE); - } -} diff --git a/app/Services/LdapAdConfiguration.php b/app/Services/LdapAdConfiguration.php deleted file mode 100644 index 25f0fba37..000000000 --- a/app/Services/LdapAdConfiguration.php +++ /dev/null @@ -1,311 +0,0 @@ - - * - * @since 5.0.0 - */ -class LdapAdConfiguration -{ - const LDAP_PORT = 389; - const CONNECTION_TIMEOUT = 5; - const DEFAULT_LDAP_VERSION = 3; - const LDAP_BOOLEAN_SETTINGS = [ - 'ldap_enabled', - 'ldap_server_cert_ignore', - 'ldap_tls', - 'ldap_tls', - 'ldap_pw_sync', - 'is_ad', - 'ad_append_domain', - ]; - - /** - * Ldap Settings. - * - * @var Collection - */ - public $ldapSettings; - - /** - * LDAP Config. - * - * @var array - */ - public $ldapConfig; - - /** - * Initialize LDAP from user settings - * - * @since 5.0.0 - */ - public function init() - { - - // This try/catch is dumb, but is necessary to run initial migrations, since - // this service provider is booted even during migrations. :( - snipe - try { - $this->ldapSettings = $this->getSnipeItLdapSettings(); - if ($this->isLdapEnabled()) { - $this->setSnipeItConfig(); - } - } catch (\Exception $e) { - \Log::debug($e); - $this->ldapSettings = null; - } - } - - /** - * Merge the default Adlap config with the SnipeIT config. - * - * @author Wes Hulette - * - * @since 5.0.0 - */ - private function setSnipeItConfig() - { - $this->ldapConfig = $this->setLdapConnectionConfiguration(); - $this->certificateCheck(); - } - - /** - * Get the LDAP settings from the Settings model. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return \Illuminate\Support\Collection - */ - private function getSnipeItLdapSettings(): Collection - { - $ldapSettings = collect(); - if (Setting::first()) { // during early migration steps, there may be no settings table entry to start with - $ldapSettings = Setting::getLdapSettings() - ->map(function ($item, $key) { - // Trim the items - if (is_string($item)) { - $item = trim($item); - } - // Get the boolean value of the LDAP setting, makes it easier to work with them - if (in_array($key, self::LDAP_BOOLEAN_SETTINGS)) { - return boolval($item); - } - - // Decrypt the admin password - if ('ldap_pword' === $key && ! empty($item)) { - try { - return decrypt($item); - } catch (Exception $e) { - throw new Exception('Your app key has changed! Could not decrypt LDAP password using your current app key, so LDAP authentication has been disabled. Login with a local account, update the LDAP password and re-enable it in Admin > Settings.'); - } - } - - if ($item && 'ldap_server' === $key) { - return collect(parse_url($item)); - } - - return $item; - }); - } - - return $ldapSettings; - } - - /** - * Set the server certificate environment variable. - * - * @author Wes Hulette - * - * @since 5.0.0 - */ - private function certificateCheck(): void - { - // If we are ignoring the SSL cert we need to setup the environment variable - // before we create the connection - if ($this->ldapSettings['ldap_server_cert_ignore']) { - putenv('LDAPTLS_REQCERT=never'); - } - - // If the user specifies where CA Certs are, make sure to use them - if (env('LDAPTLS_CACERT')) { - putenv('LDAPTLS_CACERT='.env('LDAPTLS_CACERT')); - } - } - - /** - * Set the Adlap2 connection configuration values based on SnipeIT settings. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return array - */ - private function setLdapConnectionConfiguration(): array - { - // Create the configuration array. - $ldap_settings = [ - // Mandatory Configuration Options - 'hosts' => $this->getServerUrlBase(), - 'base_dn' => $this->ldapSettings['ldap_basedn'], - 'username' => $this->ldapSettings['ldap_uname'], - 'password' => $this->ldapSettings['ldap_pword'], - - // Optional Configuration Options - 'schema' => $this->getSchema(), // FIXME - we probably ought not to be using this, right? - 'account_prefix' => '', - 'account_suffix' => '', - 'port' => $this->getPort(), - 'follow_referrals' => false, - 'use_ssl' => $this->isSsl(), - 'use_tls' => $this->ldapSettings['ldap_tls'], - 'version' => $this->ldapSettings['ldap_version'] ?? self::DEFAULT_LDAP_VERSION, - 'timeout' => self::CONNECTION_TIMEOUT, - - // Custom LDAP Options - 'custom_options' => [ - // See: http://php.net/ldap_set_option - // LDAP_OPT_X_TLS_REQUIRE_CERT => LDAP_OPT_X_TLS_HARD, - ], - ]; - - if($this->ldapSettings['ldap_client_tls_cert'] || $this->ldapSettings['ldap_client_tls_key']) { - $ldap_settings['custom_options'] = [ - LDAP_OPT_X_TLS_CERTFILE => Setting::get_client_side_cert_path(), - LDAP_OPT_X_TLS_KEYFILE => Setting::get_client_side_key_path() - ]; - } - return $ldap_settings; - } - - /** - * Get the schema to use for the connection. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return string - */ - private function getSchema(): string //wait, what? This is a little weird, since we have completely separate variables for this; we probably shoulnd't be using any 'schema' at all - { - $schema = \Adldap\Schemas\OpenLDAP::class; - if ($this->ldapSettings['is_ad']) { - $schema = \Adldap\Schemas\ActiveDirectory::class; - } - - return $schema; - } - - /** - * Get the port number from the connection url. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return int - */ - private function getPort(): int - { - $port = $this->getLdapServerData('port'); - if ($port && is_int($port)) { - return $port; - } - - return self::LDAP_PORT; - } - - /** - * Get ldap scheme from url to determin ssl use. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return bool - */ - private function isSsl(): bool - { - $scheme = $this->getLdapServerData('scheme'); - if ($scheme && 'ldaps' === strtolower($scheme)) { - return true; - } - - return false; - } - - /** - * Return the base url to the LDAP server. - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @return array - */ - private function getServerUrlBase(): array - { - /* if ($this->ldapSettings['is_ad']) { - return collect(explode(',', $this->ldapSettings['ad_domain']))->map(function ($item) { - return trim($item); - })->toArray(); - } */ // <- this was the *original* intent of the PR for AdLdap2, but we've been moving away from having - // two separate fields - one for "ldap_host" and one for "ad_domain" - towards just using "ldap_host" - // ad_domain for us just means "append this domain to your usernames for login, if you click that checkbox" - // that's all, nothing more (I hope). - - $url = $this->getLdapServerData('host'); - - return $url ? [$url] : []; - } - - /** - * Get ldap enabled setting - * - * @author Steffen Buehl - * - * @since 5.0.0 - * - * @return bool - */ - public function isLdapEnabled(): bool - { - return $this->ldapSettings && $this->ldapSettings->get('ldap_enabled'); - } - - /** - * Get parsed ldap server information - * - * @author Steffen Buehl - * - * @since 5.0.0 - * - * @param $key - * @return mixed|null - */ - protected function getLdapServerData($key) - { - if ($this->ldapSettings) { - $ldapServer = $this->ldapSettings->get('ldap_server'); - if ($ldapServer && $ldapServer instanceof Collection) { - return $ldapServer->get($key); - } - } - - return null; - } -} diff --git a/composer.json b/composer.json index e27a0cd1e..ee500769a 100644 --- a/composer.json +++ b/composer.json @@ -17,7 +17,6 @@ "ext-json": "*", "ext-mbstring": "*", "ext-pdo": "*", - "adldap2/adldap2": "^10.2", "alek13/slack": "^2.0", "bacon/bacon-qr-code": "^1.0", "barryvdh/laravel-debugbar": "^3.6", diff --git a/composer.lock b/composer.lock index aa4d99d39..3dce27332 100644 --- a/composer.lock +++ b/composer.lock @@ -4,73 +4,8 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "64dee2cbecb92c4250bb11ffe23f2ed0", + "content-hash": "f67d64a4e9f3ea8ce9c340527e498d97", "packages": [ - { - "name": "adldap2/adldap2", - "version": "v10.3.3", - "source": { - "type": "git", - "url": "https://github.com/Adldap2/Adldap2.git", - "reference": "c2a8f72455d3438377d955fc0f4b9ed836b47463" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/Adldap2/Adldap2/zipball/c2a8f72455d3438377d955fc0f4b9ed836b47463", - "reference": "c2a8f72455d3438377d955fc0f4b9ed836b47463", - "shasum": "" - }, - "require": { - "ext-json": "*", - "ext-ldap": "*", - "illuminate/contracts": "~5.0|~6.0|~7.0|~8.0", - "php": ">=7.0", - "psr/log": "~1.0", - "psr/simple-cache": "~1.0", - "tightenco/collect": "~5.0|~6.0|~7.0|~8.0" - }, - "require-dev": { - "mockery/mockery": "~1.0", - "phpunit/phpunit": "~6.0|~7.0|~8.0" - }, - "suggest": { - "ext-fileinfo": "fileinfo is required when retrieving user encoded thumbnails" - }, - "type": "library", - "autoload": { - "psr-4": { - "Adldap\\": "src/" - } - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "MIT" - ], - "authors": [ - { - "name": "Steve Bauman", - "email": "steven_bauman@outlook.com", - "role": "Developer" - } - ], - "description": "A PHP LDAP Package for humans.", - "keywords": [ - "active directory", - "ad", - "adLDAP", - "adldap2", - "directory", - "ldap", - "windows" - ], - "support": { - "docs": "https://github.com/Adldap2/Adldap2/blob/master/readme.md", - "email": "steven_bauman@outlook.com", - "issues": "https://github.com/Adldap2/Adldap2/issues", - "source": "https://github.com/Adldap2/Adldap2" - }, - "time": "2021-08-09T15:22:35+00:00" - }, { "name": "alek13/slack", "version": "2.1.1", @@ -10401,60 +10336,6 @@ ], "time": "2021-02-07T13:13:45+00:00" }, - { - "name": "tightenco/collect", - "version": "v8.34.0", - "source": { - "type": "git", - "url": "https://github.com/tighten/collect.git", - "reference": "b069783ab0c547bb894ebcf8e7f6024bb401f9d2" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/tighten/collect/zipball/b069783ab0c547bb894ebcf8e7f6024bb401f9d2", - "reference": "b069783ab0c547bb894ebcf8e7f6024bb401f9d2", - "shasum": "" - }, - "require": { - "php": "^7.2|^8.0", - "symfony/var-dumper": "^3.4 || ^4.0 || ^5.0" - }, - "require-dev": { - "mockery/mockery": "^1.0", - "nesbot/carbon": "^2.23.0", - "phpunit/phpunit": "^8.3" - }, - "type": "library", - "autoload": { - "files": [ - "src/Collect/Support/helpers.php", - "src/Collect/Support/alias.php" - ], - "psr-4": { - "Tightenco\\Collect\\": "src/Collect" - } - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "MIT" - ], - "authors": [ - { - "name": "Taylor Otwell", - "email": "taylorotwell@gmail.com" - } - ], - "description": "Collect - Illuminate Collections as a separate package.", - "keywords": [ - "collection", - "laravel" - ], - "support": { - "issues": "https://github.com/tighten/collect/issues", - "source": "https://github.com/tighten/collect/tree/v8.34.0" - }, - "time": "2021-03-29T21:29:00+00:00" - }, { "name": "tightenco/ziggy", "version": "v1.2.0", diff --git a/config/app.php b/config/app.php index e8d1ebae4..ab6d62004 100755 --- a/config/app.php +++ b/config/app.php @@ -345,7 +345,6 @@ return [ * Custom service provider */ App\Providers\MacroServiceProvider::class, - App\Providers\LdapServiceProvider::class, App\Providers\SamlServiceProvider::class, ], From b0417e5bd7ecd990602a70f7804f7da9ded2b733 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Wed, 3 Nov 2021 15:22:06 -0700 Subject: [PATCH 3/7] Finish pulling out the AdLdap2-based LDAP remnants that were still in the system --- .../Controllers/Api/SettingsController.php | 208 ++++++++---------- app/Http/Controllers/Auth/LoginController.php | 58 +++-- app/Models/Ldap.php | 54 +++-- routes/api.php | 4 +- 4 files changed, 175 insertions(+), 149 deletions(-) diff --git a/app/Http/Controllers/Api/SettingsController.php b/app/Http/Controllers/Api/SettingsController.php index 59236731e..813bca593 100644 --- a/app/Http/Controllers/Api/SettingsController.php +++ b/app/Http/Controllers/Api/SettingsController.php @@ -2,163 +2,136 @@ namespace App\Http\Controllers\Api; +use Illuminate\Http\Request; use App\Http\Controllers\Controller; -use App\Http\Transformers\LoginAttemptsTransformer; use App\Models\Ldap; +use Validator; use App\Models\Setting; +use Mail; +use App\Notifications\SlackTest; +use Notification; use App\Notifications\MailTest; use GuzzleHttp\Client; -use Illuminate\Http\JsonResponse; -use Illuminate\Http\Request; -use Illuminate\Http\Response; -use Illuminate\Support\Facades\DB; -use Illuminate\Support\Facades\Log; -use Illuminate\Support\Facades\Notification; -use Illuminate\Support\Facades\Storage; -use Illuminate\Support\Facades\Validator; // forward-port of v4 LDAP model for Sync class SettingsController extends Controller { - /** - * Test the ldap settings - * - * @author Wes Hulette - * - * @since 5.0.0 - * - * @param App\Models\LdapAd $ldap - * - * @return \Illuminate\Http\JsonResponse - */ - public function ldapAdSettingsTest(LdapAd $ldap): JsonResponse - { - if (! $ldap->init()) { - Log::info('LDAP is not enabled so we cannot test.'); + + public function ldaptest() + { + $settings = Setting::getSettings(); + + if ($settings->ldap_enabled!='1') { + \Log::debug('LDAP is not enabled cannot test.'); return response()->json(['message' => 'LDAP is not enabled, cannot test.'], 400); } - // The connect, bind and resulting users message - $message = []; + \Log::debug('Preparing to test LDAP connection'); - // This is all kinda fucked right now. The connection test doesn't actually do what you think, - // // and the way we parse the errors - // on the JS side is horrible. - Log::info('Preparing to test LDAP user login'); - // Test user can connect to the LDAP server + $message = []; //where we collect together test messages try { - $ldap->testLdapAdUserConnection(); - $message['login'] = [ - 'message' => 'Successfully connected to LDAP server.', - ]; - } catch (\Exception $ex) { - \Log::debug('Connection to LDAP server '.Setting::getSettings()->ldap_server.' failed. Please check your LDAP settings and try again. Server Responded with error: '.$ex->getMessage()); - - return response()->json( - ['message' => 'Connection to LDAP server '.Setting::getSettings()->ldap_server." failed. Verify that the LDAP hostname is entered correctly and that it can be reached from this web server. \n\nServer Responded with error: ".$ex->getMessage(), - - ], 400); - } - - Log::info('Preparing to test LDAP bind connection'); - // Test user can bind to the LDAP server - try { - Log::info('Testing Bind'); - $ldap->testLdapAdBindConnection(); - $message['bind'] = [ - 'message' => 'Successfully bound to LDAP server.', - ]; - } catch (\Exception $ex) { - Log::info('LDAP Bind failed'); - - return response()->json(['message' => 'Connection to LDAP successful, but we were unable to Bind the LDAP user '.Setting::getSettings()->ldap_uname.". Verify your that your LDAP Bind username and password are correct. \n\nServer Responded with error: ".$ex->getMessage(), - ], 400); - } - - Log::info('Preparing to get sample user set from LDAP directory'); - // Get a sample of 10 users so user can verify the data is correct - $settings = Setting::getSettings(); - try { - Log::info('Testing LDAP sync'); - error_reporting(E_ALL & ~E_DEPRECATED); // workaround for php7.4, which deprecates ldap_control_paged_result - // $users = $ldap->testUserImportSync(); // from AdLdap2 from v5, disabling and falling back to v4's sync code - $users = collect(Ldap::findLdapUsers())->slice(0, 11)->filter(function ($value, $key) { //choosing ELEVEN because one is going to be the count, which we're about to filter out in the next line - return is_int($key); - })->map(function ($item) use ($settings) { - return (object) [ - 'username' => $item[$settings['ldap_username_field']][0] ?? null, - 'employee_number' => $item[$settings['ldap_emp_num']][0] ?? null, - 'lastname' => $item[$settings['ldap_lname_field']][0] ?? null, - 'firstname' => $item[$settings['ldap_fname_field']][0] ?? null, - 'email' => $item[$settings['ldap_email']][0] ?? null, - ]; - }); - if ($users->count() > 0) { - $message['user_sync'] = [ - 'users' => $users, - ]; - } else { - $message['user_sync'] = [ - 'message' => 'Connection to LDAP was successful, however there were no users returned from your query. You should confirm the Base Bind DN above.', + $connection = Ldap::connectToLdap(); + try { + $message['bind'] = ['message' => 'Successfully bound to LDAP server.']; + \Log::debug('attempting to bind to LDAP for LDAP test'); + Ldap::bindAdminToLdap($connection); + $message['login'] = [ + 'message' => 'Successfully connected to LDAP server.', ]; - return response()->json($message, 400); + $users = collect(Ldap::findLdapUsers(null,10))->filter(function ($value, $key) { + return is_int($key); + })->slice(0, 10)->map(function ($item) use ($settings) { + return (object) [ + 'username' => $item[$settings['ldap_username_field']][0] ?? null, + 'employee_number' => $item[$settings['ldap_emp_num']][0] ?? null, + 'lastname' => $item[$settings['ldap_lname_field']][0] ?? null, + 'firstname' => $item[$settings['ldap_fname_field']][0] ?? null, + 'email' => $item[$settings['ldap_email']][0] ?? null, + ]; + }); + if ($users->count() > 0) { + $message['user_sync'] = [ + 'users' => $users, + ]; + } else { + $message['user_sync'] = [ + 'message' => 'Connection to LDAP was successful, however there were no users returned from your query. You should confirm the Base Bind DN above.', + ]; + + return response()->json($message, 400); + } + + return response()->json($message, 200); + } catch (\Exception $e) { + \Log::debug('Bind failed'); + \Log::debug("Exception was: ".$e->getMessage()); + return response()->json(['message' => $e->getMessage()], 400); + //return response()->json(['message' => $e->getMessage()], 500); } - } catch (\Exception $ex) { - Log::info('LDAP sync failed'); - $message['user_sync'] = [ - 'message' => 'Error getting users from LDAP directory, error: '.$ex->getMessage(), - ]; - - return response()->json($message, 400); + } catch (\Exception $e) { + \Log::debug('Connection failed but we cannot debug it any further on our end.'); + return response()->json(['message' => $e->getMessage()], 500); } - return response()->json($message, 200); + } - public function ldaptestlogin(Request $request, LdapAd $ldap) + public function ldaptestlogin(Request $request) { - if (Setting::getSettings()->ldap_enabled != '1') { - \Log::debug('LDAP is not enabled. Cannot test.'); + if (Setting::getSettings()->ldap_enabled!='1') { + \Log::debug('LDAP is not enabled. Cannot test.'); return response()->json(['message' => 'LDAP is not enabled, cannot test.'], 400); } - $rules = [ + + $rules = array( 'ldaptest_user' => 'required', - 'ldaptest_password' => 'required', - ]; + 'ldaptest_password' => 'required' + ); $validator = Validator::make($request->all(), $rules); if ($validator->fails()) { \Log::debug('LDAP Validation test failed.'); - $validation_errors = implode(' ', $validator->errors()->all()); - + $validation_errors = implode(' ',$validator->errors()->all()); return response()->json(['message' => $validator->errors()->all()], 400); } + \Log::debug('Preparing to test LDAP login'); try { - DB::beginTransaction(); //this was the easiest way to invoke a full test of an LDAP login without adding new users to the DB (which may not be desired) + $connection = Ldap::connectToLdap(); + try { + Ldap::bindAdminToLdap($connection); + \Log::debug('Attempting to bind to LDAP for LDAP test'); + try { + $ldap_user = Ldap::findAndBindUserLdap($request->input('ldaptest_user'), $request->input('ldaptest_password')); + if ($ldap_user) { + \Log::debug('It worked! '. $request->input('ldaptest_user').' successfully binded to LDAP.'); + return response()->json(['message' => 'It worked! '. $request->input('ldaptest_user').' successfully binded to LDAP.'], 200); + } + return response()->json(['message' => 'Login Failed. '. $request->input('ldaptest_user').' did not successfully bind to LDAP.'], 400); - // $results = $ldap->ldap->auth()->attempt($request->input('ldaptest_username'), $request->input('ldaptest_password'), true); - // can't do this because that's a protected property. + } catch (\Exception $e) { + \Log::debug('LDAP login failed'); + return response()->json(['message' => $e->getMessage()], 400); + } - $results = $ldap->ldapLogin($request->input('ldaptest_user'), $request->input('ldaptest_password')); // this would normally create a user on success (if they didn't already exist), but for the transaction - if ($results) { - return response()->json(['message' => 'It worked! '.$request->input('ldaptest_user').' successfully binded to LDAP.'], 200); - } else { - return response()->json(['message' => 'Login Failed. '.$request->input('ldaptest_user').' did not successfully bind to LDAP.'], 400); + } catch (\Exception $e) { + \Log::debug('Bind failed'); + return response()->json(['message' => $e->getMessage()], 400); + //return response()->json(['message' => $e->getMessage()], 500); } } catch (\Exception $e) { \Log::debug('Connection failed'); - - return response()->json(['message' => $e->getMessage()], 400); - } finally { - DB::rollBack(); // ALWAYS rollback, whether success or failure + return response()->json(['message' => $e->getMessage()], 500); } + + } + public function slacktest(Request $request) { $slack = new Client([ @@ -187,6 +160,7 @@ class SettingsController extends Controller return response()->json(['message' => 'Something went wrong :( '], 400); } + /** * Test the email configuration * @@ -196,19 +170,19 @@ class SettingsController extends Controller */ public function ajaxTestEmail() { - if (! config('app.lock_passwords')) { + if (!config('app.lock_passwords')) { try { Notification::send(Setting::first(), new MailTest()); - return response()->json(['message' => 'Mail sent to '.config('mail.reply_to.address')], 200); } catch (\Exception $e) { return response()->json(['message' => $e->getMessage()], 500); } } - return response()->json(['message' => 'Mail would have been sent, but this application is in demo mode! '], 200); + } + /** * Delete server-cached barcodes * @@ -266,4 +240,4 @@ class SettingsController extends Controller return (new LoginAttemptsTransformer)->transformLoginAttempts($login_attempt_results, $total); } -} +} \ No newline at end of file diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 2c94cc70b..3f37efb2b 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -5,7 +5,7 @@ namespace App\Http\Controllers\Auth; use App\Http\Controllers\Controller; use App\Models\Setting; use App\Models\User; -use App\Services\LdapAd; +use App\Models\Ldap; use App\Services\Saml; use Com\Tecnick\Barcode\Barcode; use Google2FA; @@ -39,11 +39,6 @@ class LoginController extends Controller */ protected $redirectTo = '/'; - /** - * @var LdapAd - */ - protected $ldap; - /** * @var Saml */ @@ -52,12 +47,11 @@ class LoginController extends Controller /** * Create a new authentication controller instance. * - * @param LdapAd $ldap * @param Saml $saml * * @return void */ - public function __construct(/*LdapAd $ldap, */ Saml $saml) + public function __construct(Saml $saml) { parent::__construct(); $this->middleware('guest', ['except' => ['logout', 'postTwoFactorAuth', 'getTwoFactorAuth', 'getTwoFactorEnroll']]); @@ -141,13 +135,47 @@ class LoginController extends Controller */ private function loginViaLdap(Request $request): User { - $ldap = \App::make(LdapAd::class); - try { - return $ldap->ldapLogin($request->input('username'), $request->input('password')); - } catch (\Exception $ex) { - LOG::debug('LDAP user login: '.$ex->getMessage()); - throw new \Exception($ex->getMessage()); - } + Log::debug("Binding user to LDAP."); + $ldap_user = Ldap::findAndBindUserLdap($request->input('username'), $request->input('password')); + if (!$ldap_user) { + Log::debug("LDAP user ".$request->input('username')." not found in LDAP or could not bind"); + throw new \Exception("Could not find user in LDAP directory"); + } else { + Log::debug("LDAP user ".$request->input('username')." successfully bound to LDAP"); + } + + // Check if the user already exists in the database and was imported via LDAP + $user = User::where('username', '=', $request->input('username'))->whereNull('deleted_at')->where('ldap_import', '=', 1)->where('activated', '=', '1')->first(); + Log::debug("Local auth lookup complete"); + + // The user does not exist in the database. Try to get them from LDAP. + // If user does not exist and authenticates successfully with LDAP we + // will create it on the fly and sign in with default permissions + if (!$user) { + Log::debug("Local user ".$request->input('username')." does not exist"); + Log::debug("Creating local user ".$request->input('username')); + + if ($user = Ldap::createUserFromLdap($ldap_user)) { //this handles passwords on its own + Log::debug("Local user created."); + } else { + Log::debug("Could not create local user."); + throw new \Exception("Could not create local user"); + } + // If the user exists and they were imported from LDAP already + } else { + Log::debug("Local user ".$request->input('username')." exists in database. Updating existing user against LDAP."); + + $ldap_attr = Ldap::parseAndMapLdapAttributes($ldap_user); + + if (Setting::getSettings()->ldap_pw_sync=='1') { + $user->password = bcrypt($request->input('password')); + } + $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. + $user->save(); + } // End if(!user) + return $user; } private function loginViaRemoteUser(Request $request) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index 6b23fd54e..b57482027 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -86,22 +86,22 @@ class Ldap extends Model // If they are, we can skip building the UPN to authenticate against AD if ($ldap_username_field == 'userprincipalname') { $userDn = $username; - } else { + } else { // FIXME - we have to respect the new 'append AD domain to username' setting (which sucks.) // In case they haven't added an AD domain $userDn = ($settings->ad_domain != '') ? $username.'@'.$settings->ad_domain : $username.'@'.$settings->email_domain; } } - \Log::debug('Attempting to login using distinguished name:'.$userDn); - $filterQuery = $settings->ldap_auth_filter_query.$username; - $filter = Setting::getSettings()->ldap_filter; + $filter = Setting::getSettings()->ldap_filter; //TODO - this *does* respect the ldap filter, but I believe that AdLdap2 did *not*. $filterQuery = "({$filter}({$filterQuery}))"; \Log::debug('Filter query: '.$filterQuery); if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) { - if (! $ldapbind = self::bindAdminToLdap($connection)) { + \Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE")); + if (! $ldapbind = self::bindAdminToLdap($connection)) { // TODO uh, this seems...dangerous? Why would we just switch over to the admin connection? That's too loose, I feel. + \Log::debug("Status of binding Admin user: $userDn to directory instead: ".($ldapbind ? "success" : "FAILURE")); return false; } } @@ -233,11 +233,11 @@ class Ldap extends Model * * @author [A. Gianotto] [] * @since [v3.0] - * @param $ldapatttibutes * @param $base_dn + * @param $count * @return array|bool */ - public static function findLdapUsers($base_dn = null) + public static function findLdapUsers($base_dn = null, $count = -1) { $ldapconn = self::connectToLdap(); $ldap_bind = self::bindAdminToLdap($ldapconn); @@ -256,10 +256,10 @@ class Ldap extends Model // Perform the search do { - // Paginate (non-critical, if not supported by server) - if (! $ldap_paging = @ldap_control_paged_result($ldapconn, $page_size, false, $cookie)) { - throw new Exception('Problem with your LDAP connection. Try checking the Use TLS setting in Admin > Settings. '); - } + // // Paginate (non-critical, if not supported by server) + // if (! $ldap_paging = ldap_search($ldapconn, $page_size, false, $cookie)) { //FIXME! This command doesn't exist anymore? I don't know what to replace it with. maybe nothing? + // throw new Exception('Problem with your LDAP connection. Try checking the Use TLS setting in Admin > Settings. '); + // } if ($filter != '' && substr($filter, 0, 1) != '(') { // wrap parens around NON-EMPTY filters that DON'T have them, for back-compatibility with AdLdap2-based filters $filter = "($filter)"; @@ -267,12 +267,36 @@ class Ldap extends Model $filter = '(cn=*)'; } - $search_results = ldap_search($ldapconn, $base_dn, $filter); - + // HUGE thanks to this article: https://stackoverflow.com/questions/68275972/how-to-get-paged-ldap-queries-in-php-8-and-read-more-than-1000-entries + // which helped me wrap my head around paged results! + \Log::info("ldap conn is: ".$ldapconn." basedn is: $base_dn, filter is: $filter - count is: $count. page size is: $page_size"); + // if a $count is set and it's smaller than $page_size then use that as the page size + $ldap_controls = []; + if($count == -1) { //count is -1 means we have to employ paging to query the entire directory + $ldap_controls = [['oid' => LDAP_CONTROL_PAGEDRESULTS, 'iscritical' => false, 'value' => ['size'=> $page_size, 'cookie' => $cookie]]]; + } + $search_results = @ldap_search($ldapconn, $base_dn, $filter, [], 0, /* $page_size*/ -1, -1, LDAP_DEREF_NEVER, $ldap_controls); + \Log::info("did the search run? I guess so if you got here!"); if (! $search_results) { return redirect()->route('users.index')->with('error', trans('admin/users/message.error.ldap_could_not_search').ldap_error($ldapconn)); // FIXME this is never called in any routed context - only from the Artisan command. So this redirect will never work. } + $errcode = null; + $matcheddn = null; + $errmsg = null; + $referrals = null; + $controls = []; + ldap_parse_result($ldapconn, $search_results, $errcode , $matcheddn , $errmsg , $referrals, $controls); + if (isset($controls[LDAP_CONTROL_PAGEDRESULTS]['value']['cookie'])) { + // You need to pass the cookie from the last call to the next one + $cookie = $controls[LDAP_CONTROL_PAGEDRESULTS]['value']['cookie']; + \Log::info("okay, at least one more page to go!!!"); + } else { + \Log::info("okay, we're out of pages - no cookie (or empty cookie) was passed"); + $cookie = ''; + } + // Empty cookie means last page + // Get results from page $results = ldap_get_entries($ldapconn, $search_results); if (! $results) { @@ -282,14 +306,14 @@ class Ldap extends Model // Add results to result set $global_count += $results['count']; $result_set = array_merge($result_set, $results); + \Log::info("Total count is: $global_count"); - @ldap_control_paged_result_response($ldapconn, $search_results, $cookie); + // ldap_search($ldapconn, $search_results, $cookie); // FIXME - this function is removed in PHP8 } while ($cookie !== null && $cookie != ''); // Clean up after search $result_set['count'] = $global_count; $results = $result_set; - @ldap_control_paged_result($ldapconn, 0); return $results; } diff --git a/routes/api.php b/routes/api.php index 85895ec85..175d17587 100644 --- a/routes/api.php +++ b/routes/api.php @@ -702,10 +702,10 @@ Route::group(['prefix' => 'v1', 'middleware' => 'api'], function () { */ Route::group(['middleware'=> ['auth', 'authorize:superuser'], 'prefix' => 'settings'], function () { - Route::post('ldaptest', + Route::get('ldaptest', [ Api\SettingsController::class, - 'ldapAdSettingsTest' + 'ldaptest' ] )->name('api.settings.ldaptest'); From a58c5ce27f541620849746707b49a9e3733fad4b Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Mon, 8 Nov 2021 17:11:47 -0800 Subject: [PATCH 4/7] Better documentation, disable AdLdap2-based "Add domain" setting --- app/Http/Controllers/Auth/LoginController.php | 2 +- app/Models/Ldap.php | 55 ++++++++++++++++--- resources/views/settings/ldap.blade.php | 4 +- 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 3f37efb2b..341a004b6 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -145,7 +145,7 @@ class LoginController extends Controller } // Check if the user already exists in the database and was imported via LDAP - $user = User::where('username', '=', $request->input('username'))->whereNull('deleted_at')->where('ldap_import', '=', 1)->where('activated', '=', '1')->first(); + $user = User::where('username', '=', $request->input('username'))->whereNull('deleted_at')->where('ldap_import', '=', 1)->where('activated', '=', '1')->first(); // FIXME - if we get more than one we should fail. Log::debug("Local auth lookup complete"); // The user does not exist in the database. Try to get them from LDAP. diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index b57482027..d7cf0732d 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -9,6 +9,22 @@ use Illuminate\Database\Eloquent\Model; use Input; use Log; +/*********************************************** + * TODOS: + * + * First off, we should probably make it so that the main LDAP thing we're using is an *instance* of this class, + * rather than the static methods we use here. We should probably load up that class with its settings, so we + * don't have to explicitly refer to them so often. + * + * Then, we should probably look at embedding some of the logic we use elsewhere into here - the various methods + * should either return a User or false, or other things like that. Don't make the consumers of this class reach + * into its guts. While that conflates this model with the User model, I think having the appropriate logic for + * turning LDAP people into Users ought to belong here, so it's easier on the consumer of this class. + * + * We're probably going to have to eventually make it so that Snipe-IT users can define multiple LDAP servers, + * and having this as a more instance-oriented class will be a step in the right direction. + ***********************************************/ + class Ldap extends Model { /** @@ -86,23 +102,41 @@ class Ldap extends Model // If they are, we can skip building the UPN to authenticate against AD if ($ldap_username_field == 'userprincipalname') { $userDn = $username; - } else { // FIXME - we have to respect the new 'append AD domain to username' setting (which sucks.) - // In case they haven't added an AD domain + } else { + // TODO - we no longer respect the "add AD Domain to username" checkbox, but it still exists in settings. + // We should probably just eliminate that checkbox to avoid confusion. + // We let it sit in the DB, unused, to facilitate people downgrading (if they decide to). + // Hopefully, in a later release, we can remove it from the settings. + // This logic instead just means that if we're using UPN, we don't append ad_domain, if we aren't, then we do. + // Hopefully that should handle all of our use cases, but if not we can backport our old logic. $userDn = ($settings->ad_domain != '') ? $username.'@'.$settings->ad_domain : $username.'@'.$settings->email_domain; } } $filterQuery = $settings->ldap_auth_filter_query.$username; - $filter = Setting::getSettings()->ldap_filter; //TODO - this *does* respect the ldap filter, but I believe that AdLdap2 did *not*. + $filter = Setting::getSettings()->ldap_filter; //FIXME - this *does* respect the ldap filter, but I believe that AdLdap2 did *not*. $filterQuery = "({$filter}({$filterQuery}))"; \Log::debug('Filter query: '.$filterQuery); if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) { \Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE")); - if (! $ldapbind = self::bindAdminToLdap($connection)) { // TODO uh, this seems...dangerous? Why would we just switch over to the admin connection? That's too loose, I feel. - \Log::debug("Status of binding Admin user: $userDn to directory instead: ".($ldapbind ? "success" : "FAILURE")); - return false; + if (! $ldapbind = self::bindAdminToLdap($connection)) { + /* + * FIXME PLEASE: + * + * this isn't very clear, so it's important to note: the $ldapbind value is never correctly returned - we never 'return true' from self::bindAdminToLdap() (the function + * just "falls off the end" without ever explictly returning 'true') + * + * but it *does* have an interesting side-effect of checking for the LDAP password being incorrectly encrypted with the wrong APP_KEY, so I'm leaving it in for now. + * + * If it *did* correctly return 'true' on a succesful bind, it would _probably_ allow users to log in with an incorrect password. Which would be horrible! + * + * Let's definitely fix this at the next refactor!!!! + * + */ + \Log::debug("Status of binding Admin user: $userDn to directory instead: ".($ldapbind ? "success" : "FAILURE")); + return false; } } @@ -135,8 +169,6 @@ class Ldap extends Model { $ldap_username = Setting::getSettings()->ldap_uname; - $ldap_username = Setting::getSettings()->ldap_uname; - // Lets return some nicer messages for users who donked their app key, and disable LDAP try { $ldap_pass = \Crypt::decrypt(Setting::getSettings()->ldap_pword); @@ -147,6 +179,11 @@ class Ldap extends Model if (! $ldapbind = @ldap_bind($connection, $ldap_username, $ldap_pass)) { throw new Exception('Could not bind to LDAP: '.ldap_error($connection)); } + // FIXME - this just "falls off the end" but the function states that it should return true or false + // unfortunately, one of the use cases for this function is wrong and *needs* for that failure mode to fire + // so I don't want to fix this right now. + // this method MODIFIES STATE on the passed-in $connection and just returns true or false (or, in this case, undefined) + // at the next refactor, this should be appropriately modified to be more consistent. } @@ -240,7 +277,7 @@ class Ldap extends Model public static function findLdapUsers($base_dn = null, $count = -1) { $ldapconn = self::connectToLdap(); - $ldap_bind = self::bindAdminToLdap($ldapconn); + self::bindAdminToLdap($ldapconn); // Default to global base DN if nothing else is provided. if (is_null($base_dn)) { $base_dn = Setting::getSettings()->ldap_basedn; diff --git a/resources/views/settings/ldap.blade.php b/resources/views/settings/ldap.blade.php index e01ad2c86..e87a432da 100644 --- a/resources/views/settings/ldap.blade.php +++ b/resources/views/settings/ldap.blade.php @@ -122,7 +122,7 @@ - + {{-- NOTICE - this was a feature for AdLdap2-based LDAP syncing, and is already handled in 'classic' LDAP, so we now hide the checkbox (but haven't deleted the field)
{{ Form::label('ad_append_domain', trans('admin/settings/general.ad_append_domain_label')) }} @@ -136,7 +136,7 @@

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

@endif
-
+ --}}
From 864cc4f8d5caec7d8a124ebca0916804eadfd90d Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Wed, 10 Nov 2021 11:37:10 -0800 Subject: [PATCH 5/7] Fix FIXME's by downgrading them to TODO's :) --- app/Console/Commands/LdapSync.php | 2 +- app/Http/Controllers/Auth/LoginController.php | 4 +- app/Models/Ldap.php | 54 +++++++++---------- 3 files changed, 27 insertions(+), 33 deletions(-) diff --git a/app/Console/Commands/LdapSync.php b/app/Console/Commands/LdapSync.php index 45a9bd44a..0969b1991 100755 --- a/app/Console/Commands/LdapSync.php +++ b/app/Console/Commands/LdapSync.php @@ -133,7 +133,7 @@ class LdapSync extends Command foreach ($ldap_ou_locations as $ldap_loc) { try { $location_users = Ldap::findLdapUsers($ldap_loc['ldap_ou']); - } catch (\Exception $e) { // FIXME: this is stolen from line 77 or so above + } catch (\Exception $e) { // TODO: this is stolen from line 77 or so above if ($this->option('json_summary')) { $json_summary = ['error' => true, 'error_message' => trans('admin/users/message.error.ldap_could_not_search').' Location: '.$ldap_loc['name'].' (ID: '.$ldap_loc['id'].') cannot connect to "'.$ldap_loc['ldap_ou'].'" - '.$e->getMessage(), 'summary' => []]; $this->info(json_encode($json_summary)); diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 341a004b6..65b11e35b 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -145,7 +145,7 @@ class LoginController extends Controller } // Check if the user already exists in the database and was imported via LDAP - $user = User::where('username', '=', $request->input('username'))->whereNull('deleted_at')->where('ldap_import', '=', 1)->where('activated', '=', '1')->first(); // FIXME - if we get more than one we should fail. + $user = User::where('username', '=', $request->input('username'))->whereNull('deleted_at')->where('ldap_import', '=', 1)->where('activated', '=', '1')->first(); // FIXME - if we get more than one we should fail. and we sure about this ldap_import thing? Log::debug("Local auth lookup complete"); // The user does not exist in the database. Try to get them from LDAP. @@ -175,7 +175,7 @@ class LoginController extends Controller $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. $user->save(); } // End if(!user) - return $user; + return $user; } private function loginViaRemoteUser(Request $request) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index d7cf0732d..3861509eb 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -11,16 +11,16 @@ use Log; /*********************************************** * TODOS: - * - * First off, we should probably make it so that the main LDAP thing we're using is an *instance* of this class, + * + * First off, we should probably make it so that the main LDAP thing we're using is an *instance* of this class, * rather than the static methods we use here. We should probably load up that class with its settings, so we * don't have to explicitly refer to them so often. - * + * * Then, we should probably look at embedding some of the logic we use elsewhere into here - the various methods * should either return a User or false, or other things like that. Don't make the consumers of this class reach * into its guts. While that conflates this model with the User model, I think having the appropriate logic for * turning LDAP people into Users ought to belong here, so it's easier on the consumer of this class. - * + * * We're probably going to have to eventually make it so that Snipe-IT users can define multiple LDAP servers, * and having this as a more instance-oriented class will be a step in the right direction. ***********************************************/ @@ -122,18 +122,18 @@ class Ldap extends Model if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) { \Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE")); if (! $ldapbind = self::bindAdminToLdap($connection)) { - /* - * FIXME PLEASE: - * + /* + * TODO PLEASE: + * * this isn't very clear, so it's important to note: the $ldapbind value is never correctly returned - we never 'return true' from self::bindAdminToLdap() (the function * just "falls off the end" without ever explictly returning 'true') - * + * * but it *does* have an interesting side-effect of checking for the LDAP password being incorrectly encrypted with the wrong APP_KEY, so I'm leaving it in for now. - * + * * If it *did* correctly return 'true' on a succesful bind, it would _probably_ allow users to log in with an incorrect password. Which would be horrible! - * + * * Let's definitely fix this at the next refactor!!!! - * + * */ \Log::debug("Status of binding Admin user: $userDn to directory instead: ".($ldapbind ? "success" : "FAILURE")); return false; @@ -179,7 +179,7 @@ class Ldap extends Model if (! $ldapbind = @ldap_bind($connection, $ldap_username, $ldap_pass)) { throw new Exception('Could not bind to LDAP: '.ldap_error($connection)); } - // FIXME - this just "falls off the end" but the function states that it should return true or false + // TODO - this just "falls off the end" but the function states that it should return true or false // unfortunately, one of the use cases for this function is wrong and *needs* for that failure mode to fire // so I don't want to fix this right now. // this method MODIFIES STATE on the passed-in $connection and just returns true or false (or, in this case, undefined) @@ -293,11 +293,6 @@ class Ldap extends Model // Perform the search do { - // // Paginate (non-critical, if not supported by server) - // if (! $ldap_paging = ldap_search($ldapconn, $page_size, false, $cookie)) { //FIXME! This command doesn't exist anymore? I don't know what to replace it with. maybe nothing? - // throw new Exception('Problem with your LDAP connection. Try checking the Use TLS setting in Admin > Settings. '); - // } - if ($filter != '' && substr($filter, 0, 1) != '(') { // wrap parens around NON-EMPTY filters that DON'T have them, for back-compatibility with AdLdap2-based filters $filter = "($filter)"; } elseif ($filter == '') { @@ -306,16 +301,16 @@ class Ldap extends Model // HUGE thanks to this article: https://stackoverflow.com/questions/68275972/how-to-get-paged-ldap-queries-in-php-8-and-read-more-than-1000-entries // which helped me wrap my head around paged results! - \Log::info("ldap conn is: ".$ldapconn." basedn is: $base_dn, filter is: $filter - count is: $count. page size is: $page_size"); + \Log::info("ldap conn is: ".$ldapconn." basedn is: $base_dn, filter is: $filter - count is: $count. page size is: $page_size"); //FIXME - remove // if a $count is set and it's smaller than $page_size then use that as the page size $ldap_controls = []; - if($count == -1) { //count is -1 means we have to employ paging to query the entire directory - $ldap_controls = [['oid' => LDAP_CONTROL_PAGEDRESULTS, 'iscritical' => false, 'value' => ['size'=> $page_size, 'cookie' => $cookie]]]; - } - $search_results = @ldap_search($ldapconn, $base_dn, $filter, [], 0, /* $page_size*/ -1, -1, LDAP_DEREF_NEVER, $ldap_controls); + //if($count == -1) { //count is -1 means we have to employ paging to query the entire directory + $ldap_controls = [['oid' => LDAP_CONTROL_PAGEDRESULTS, 'iscritical' => false, 'value' => ['size'=> $count == -1||$count>$page_size ? $page_size : $count, 'cookie' => $cookie]]]; + //} + $search_results = ldap_search($ldapconn, $base_dn, $filter, [], 0, /* $page_size */ -1, -1, LDAP_DEREF_NEVER, $ldap_controls); // TODO - I hate the @, and I hate that we get a full page even if we ask for 10 records. Can we use an ldap_control? \Log::info("did the search run? I guess so if you got here!"); if (! $search_results) { - return redirect()->route('users.index')->with('error', trans('admin/users/message.error.ldap_could_not_search').ldap_error($ldapconn)); // FIXME this is never called in any routed context - only from the Artisan command. So this redirect will never work. + return redirect()->route('users.index')->with('error', trans('admin/users/message.error.ldap_could_not_search').ldap_error($ldapconn)); // TODO this is never called in any routed context - only from the Artisan command. So this redirect will never work. } $errcode = null; @@ -327,9 +322,9 @@ class Ldap extends Model if (isset($controls[LDAP_CONTROL_PAGEDRESULTS]['value']['cookie'])) { // You need to pass the cookie from the last call to the next one $cookie = $controls[LDAP_CONTROL_PAGEDRESULTS]['value']['cookie']; - \Log::info("okay, at least one more page to go!!!"); + \Log::debug("okay, at least one more page to go!!!"); } else { - \Log::info("okay, we're out of pages - no cookie (or empty cookie) was passed"); + \Log::debug("okay, we're out of pages - no cookie (or empty cookie) was passed"); $cookie = ''; } // Empty cookie means last page @@ -337,19 +332,18 @@ class Ldap extends Model // Get results from page $results = ldap_get_entries($ldapconn, $search_results); if (! $results) { - return redirect()->route('users.index')->with('error', trans('admin/users/message.error.ldap_could_not_get_entries').ldap_error($ldapconn)); // FIXME this is never called in any routed context - only from the Artisan command. So this redirect will never work. + return redirect()->route('users.index')->with('error', trans('admin/users/message.error.ldap_could_not_get_entries').ldap_error($ldapconn)); // TODO this is never called in any routed context - only from the Artisan command. So this redirect will never work. } // Add results to result set $global_count += $results['count']; $result_set = array_merge($result_set, $results); - \Log::info("Total count is: $global_count"); + \Log::debug("Total count is: $global_count"); - // ldap_search($ldapconn, $search_results, $cookie); // FIXME - this function is removed in PHP8 - } while ($cookie !== null && $cookie != ''); + } while ($cookie !== null && $cookie != '' && ($count == -1 || $global_count < $count)); // some servers don't even have pagination, and some will give you more results than you asked for, so just see if you have enough. // Clean up after search - $result_set['count'] = $global_count; + $result_set['count'] = $global_count; // TODO: I would've figured you could just count the array instead? $results = $result_set; return $results; From e8f5dc85a69e49a2bf7fe308901608d51debe4f1 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Fri, 19 Nov 2021 16:38:46 -0800 Subject: [PATCH 6/7] Downgraded a FIXME to a TODO --- app/Console/Commands/LdapSync.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Console/Commands/LdapSync.php b/app/Console/Commands/LdapSync.php index 0969b1991..1eae7dda9 100755 --- a/app/Console/Commands/LdapSync.php +++ b/app/Console/Commands/LdapSync.php @@ -91,7 +91,7 @@ class LdapSync extends Command } /* Determine which location to assign users to by default. */ - $location = null; // FIXME - this would be better called "$default_location", which is more explicit about its purpose + $location = null; // TODO - this would be better called "$default_location", which is more explicit about its purpose if ($this->option('location') != '') { $location = Location::where('name', '=', $this->option('location'))->first(); From 95c30cae8d86407715b9f75a62e3a28ffbb7db68 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Mon, 3 Jan 2022 13:53:53 -0800 Subject: [PATCH 7/7] Some duplicate imports at the top of the Settings file --- app/Http/Controllers/Api/SettingsController.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/Http/Controllers/Api/SettingsController.php b/app/Http/Controllers/Api/SettingsController.php index d2b20aa4d..2c4f37bcb 100644 --- a/app/Http/Controllers/Api/SettingsController.php +++ b/app/Http/Controllers/Api/SettingsController.php @@ -5,11 +5,9 @@ namespace App\Http\Controllers\Api; use Illuminate\Http\Request; use App\Http\Controllers\Controller; use App\Models\Ldap; -use Validator; use App\Models\Setting; use Mail; use App\Notifications\SlackTest; -use Notification; use App\Notifications\MailTest; use GuzzleHttp\Client; use Illuminate\Http\JsonResponse;