simplesamlphp / simplesamlphp-module-ldap

Module that provides authentication against LDAP stores
GNU Lesser General Public License v2.1
4 stars 11 forks source link

Changes AttributeAddFromLDAP.php to allow search.username and search.password to be optional as is written in the documentation. #53

Closed doedje closed 7 months ago

doedje commented 7 months ago

TL;DR: Changed search.username and search.password to Optionals with an empty string as default, since a string cannot be null.

I ran into the problem that my config does not need to bind to ldap to do the search. When updating from 1.9.x I ran into an error:

SimpleSAML\Error\UnserializableException: ldap:AuthProcess: Could not retrieve the required option 'search.username'.

This error persisted when specifying, as stated in the ldap documentation.:

'search.username' => null, 
'search.password' => null 

So I changed the code as in the pull request. Which fixed my problem.

I also tried changing it to this, using null as the default, keeping it more in line with the code in Ldap.php:

$this->searchUsername = $this->config->getOptionalString('search.username', null);
$this->searchPassword = $this->config->getOptionalString('search.password', null);

But that also gave me errors:

TypeError: Cannot assign null to property SimpleSAML\Module\ldap\Auth\Process\AttributeAddFromLDAP::$searchUsername of type string Mar 18 13:08:05 simplesamlphp ERROR [9e2a10f0c2] Backtrace: Mar 18 13:08:05 simplesamlphp ERROR [9e2a10f0c2] 10 /var/www/simplesamlphp/modules/ldap/src/Auth/Process/AttributeAddFromLDAP.php:77 (SimpleSAML\Module\ldap\Auth\Process\AttributeAddFromLDAP::__construct)

So I went back to the empty strings as default.

tvdijen commented 7 months ago

I took a slightly different approach in 420a02069e7ed51e90a568fa2211f2add8dffac2 to allow setting the username/password to null. Would you mind trying v2.3.3 and see if it works as expected?

doedje commented 7 months ago

I tested the code that is currently in the master branch of this repo and can confirm that everything is working as expected now. Thank you! I added a little comment in the commit to point you to another missing ? for ldap.product...