osTicket / osTicket-plugins

Core plugins for osTicket (v1.8+)
GNU General Public License v2.0
149 stars 162 forks source link

LDAP Plugin - Call to a member function dn() on bool #270

Open ZebboKlaufix opened 1 year ago

ZebboKlaufix commented 1 year ago

Hi, I,m using osTicket v1.17.3 (ca95150) Apache MySQL 10.11.2 PHP 8.1.17 389ds on Port 636 (SSL)

I configured the LDAP-Plugin to use the 389ds as LDAP-Source.

Szene1: wrong username & password [x] working like expected: Access denied

Szene2: correct username & wrong|correct password [ ] error: stuck at loading spinner on the login screen

So I took a look. tcpdump shows package transfer on the ldap-server. Cool Webserver Error Log shows: "PHP Fatal error: Uncaught Error: Call to a member function dn() on bool in phar:///srv/www/htdocs/osTicket/include/plugins/auth-ldap.phar/authentication.php:246"

Ok. Downloaded the latest dev version of the ldap plugin. Build phar file and stored it into the plugin folder. Same result. After some tests I assume the error the stick between $r = $c->search(...) and $bound = $c->bind($r->current()->dn(), $password); The plugin fetches the user entry, but $r->current() is empty, Therefore the users dn is unknown ab the plugin is unable to check the password.

@JediKev already adresses a kinda similar error, but not exactly this in #260

How do I fix it, so the bind will work? If you need any further log output, I'm hopefully able to generate and post it.

Best Zebbo

JediKev commented 1 year ago

@ZebboKlaufix

I would suggest the following:

diff --git a/auth-ldap/authentication.php b/auth-ldap/authentication.php
index 69715ec..20aa6b3 100644
--- a/auth-ldap/authentication.php
+++ b/auth-ldap/authentication.php
@@ -238,7 +238,7 @@ class LDAPAuthentication {
                 $schema['lookup']),
             array('sizelimit' => 1)
         );
-        if (PEAR::isError($r) || !$r->count())
+        if (PEAR::isError($r) || !$r->count() || !$r->current())
             return null;

         // Attempt to bind as the DN of the user looked up with the password

This is because $r->current() is required and if that is false then the subsequent methods will fail.

To test this go to the database, go to the plugin table, find the record for LDAP, set isphar to 0, set the install_path to plugins/auth-ldap, cd to the osTicket plugin directory on the server (eg. cd /path/to/osticket/include/plugins/), and run the following command to unpack the plugin: php -r '$phar = new Phar("auth-ldap.phar"); $phar->extractTo("./auth-ldap");'

Once you do this you can make the changes to the plugin files manually and test the changes. You may need to restart Apache and PHP-FPM (if you're running it) to clear any file cache.

Cheers.

ZebboKlaufix commented 1 year ago

Thanks for your reply, @JediKev . But as far as I understand, this fix would deny the login for anybody, because $r->current() is always empty. Even if the credentials are correct. This, of course, would prevent the endles spinner on the client side and the error message in the logs. But the main problem still exists: The user can't login.

If $r->count() returns 1, the user entry was found in LDAP, right? But how can I now access its dn without using current()? I don't fully understand what's inside $r, so I can't 'extract' the needed information by myself.

Best Zebbo

JediKev commented 1 year ago

@ZebboKlaufix

If $r->count() returns 1, the user entry was found in LDAP, right?

Not necessarily, it uses a "_count_cache" so that count could be cached from previous attempts, etc.

Also, the current() method comment states If the search throwed an error, it returns false. False is also returned, if the end is reached. So it seems maybe the user search is failing or the iterator cache is not resetting/updating properly.

Regardless, my patch above still stands as like I said $r->current() is required for subsequent code so if this is truly a boolean (like in your case) the rest will fail anyways. So something is going on with your connection, either not able to bind, search, or something.

Also, I don't even get to that part in my auth attempts. My connection goes through the typical LDAPAuthentication::authenticate() method from the plugin and returns here successfully. So if you get past that to where it's checking for $r->current() then your initial bind failed for some reason. You should be able to log the error message with something like:

diff --git a/auth-ldap/authentication.php b/auth-ldap/authentication.php
index 69715ec..3f2aa08 100644
--- a/auth-ldap/authentication.php
+++ b/auth-ldap/authentication.php
@@ -223,6 +223,7 @@ class LDAPAuthentication {
         $r = $c->bind($dn, $password);
         if (!PEAR::isError($r))
             return $this->lookupAndSync($username, $dn);
+        error_log(print_r($r, true));

         // Another effort is to search for the user
         if (!$this->_bind($c))

Cheers.