simplesamlphp / simplesamlphp-module-ldap

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

Question: where to add code to throw WRONGUSERPASS on empty password #56

Closed doedje closed 5 months ago

doedje commented 5 months ago

When I try to login with username and empty password a "Symfony\Component\Ldap\Exception\ConnectionException: Server is unwilling to perform" error is thrown but remains unhandled, which results in a "SimpleSAML\Error\Error: UNHANDLEDEXCEPTION".

So for now I have a try-catch around:

// simplesamlphp-module-ldap/src/Auth/Source/Ldap.php, line 118:

try {
    $this->connector->bind($dn, $password);
} catch (\Exception $e) {
    throw new Error\Error('WRONGUSERPASS');
}

But then I started thinking, wouldn't it make more sense to disallow empty passwords all together and put a check for empty passwords in:

// simplesamlphp/modules/core/src/Controller/Login.php, line 226:

if (empty($password)) {
    throw new Error\Error('WRONGUSERPASS');
}

I'm looking forward to your reasoning. I will, when needed, send a PR to the respective repository.

tvdijen commented 5 months ago

Hi @doedje !

What version of the module are you using? Because I cannot reproduce this issue.. I sort of remember fixing this case recently?....

tvdijen commented 5 months ago

Ah look, it was fixed here and released in v2.3.5 of this module.

doedje commented 5 months ago

My bad, I upgraded to 2.3.6 and it is working as expected. Lesson learned: first update, then dive into the code to fix an issue... ;) Thank you!