Fixed #9063: Ask LDAP for user DN, don't concatenate username+baseDN. #11544
Conversation
|
This looks pretty close to my own take on this: #11041 - I've been afraid to take that one out of WIP because I haven't had anyone to test it, and I've had a hard time configuring my own slapd server to have a sufficiently complex subtree configuration. Does that one look pretty close to what you were trying to do? One thing I like about your approach is that it makes fewer changes. But I'd love to hear what you think about my take, @sunflowerbofh - I figure it's 6 of one, half a dozen of the other, in the end. Either one will work. |
|
Hello brady,
I finally could test your solution. Was a bit difficult because the develop
branch had gone on a lot in the meantime, so solving quite a few merge
conflicts was necessary.
I had to comment the line
\Log::info("ldap conn is: ".$ldapconn." basedn is: $base_dn, filter is: $filter - count is: $count. page size is: $page_size"); //FIXME - remove
in app/Model/Ldap.php because otherwise I would get an error¹.
Anyway, the test was not successful. When I tried to test the LDAP login I got:
Login Failed. <user1> did not successfully bind to LDAP. (on the screen)
and
[13:38:00] LOG.debug: Uh, we could *NOT* bind as admin?! I don't understand how this ever worked.
(on the debug log)
Therefore I would prefer the other solution from
#11544 which worked without aby problems.
What I guess as nice approach is the checkbox "Append "," and Base DN to
username" as like that the admin can decide if (s)he wants the concatenation of
the username with LDAP base dn. The names seems a bit clumsy to me, I would
rather call it "Combine username and Base DN" or something similar. We ourselves
have no use case for that but for people who used the snipeit LDAP like that up
to now could that be a nice option. We should keep in eye to implement that later
on.
In my opinion the implementation order for LDAP changes which
makes most sense woud be:
1. Merging anonymous bind feature (#11510)
2. Merging LDAP user DN (#11544)
3. Adding the Append button functionality asap
(f9ea6eb).
4. Getting everything to production if running a while without unforeseen
mistakes.
What would you think about that? Be badly need both features. For testing
purposes I am running an own fork at the moment.
Regards
Katharina
¹)
LOG.error: Error: Object of class LDAP\Connection could not be converted to string in /usr/share/snipe-it/app/Models/Ldap.php:321
Stack trace:
#0 /usr/share/snipe-it/app/Http/Controllers/Api/SettingsController.php(53): App\Models\Ldap::findLdapUsers()
#1 /usr/share/snipe-it/vendor/laravel/framework/src/Illuminate/Routing/Controller.php(54): App\Http\Controllers\Api\SettingsController->ldaptest()
#2 /usr/share/snipe-it/vendor/laravel/framework/src/Illuminate/Routing/ControllerDispatcher.php(45): Illuminate\Routing\Controller->callAction()
#3 /usr/share/snipe-it/vendor/laravel/framework/src/Illuminate/Routing/Route.php(262): Illuminate\Routing\ControllerDispatcher->dispatch()
#4 /usr/share/snipe-it/vendor/laravel/framework/src/Illuminate/Routing/Route.php(205): Illuminate\Routing\Route->runController()
#5 /usr/share/snipe-it/vendor/laravel/framework/src/Illuminate/Routing/Router.php(721): Illuminate\Routing\Route->run()
#6 /usr/share/snipe-it/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(128): Illuminate\Routing\Router->Illuminate\Routing\{closure}()
#7 /usr/share/snipe-it/app/Http/Middleware/CheckPermissions.php(24): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}()
#8 /usr/share/snipe-it/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(167): App\Http\Middleware\CheckPermissions->handle()
#9 /usr/share/snipe-it/vendor/laravel/framework/src/Illuminate/Routing/Middleware/ThrottleRequests.php(127): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}()
(...)
|
There was a problem hiding this comment.
I know it's probably been a while, but I'd love to take this one - it's just so much cleaner than how I tried to do it. I'll certainly have to do some testing, but I like the way this changes the minimal amount of code necessary to get the job done.
Just had one small ask regarding the number of results returned (and debugging output, regarding that) - if we can get that fixed, then I'll get to testing and we'll see about getting it merged.
Thank you for contributing, and thanks for making a really easy-to-review PR for me!
| $userresults = ldap_search($connection, $baseDn, $filterQuery); | ||
| $userentries = ldap_get_entries($connection, $userresults); | ||
| // Can be empty if user does not exist | ||
| if ( $userentries["count"] > 0 ) { |
There was a problem hiding this comment.
I'd prefer this actually be == 1 - so if you managed to find two identical-ish looking user, it would refuse to log you in.
There was a problem hiding this comment.
I changed the condition to ( $userentries["count"] == 1 ) s. also #11715
| \Log::debug('User dn is empty.'); | ||
| } | ||
| } else { | ||
| \Log::debug('Status of LDAP entries for user ' .$username. ': no result.'); |
There was a problem hiding this comment.
If you do the above-listed change, you'll probably want to output the 'failed' $userentries["count"] here so they can troubleshoot that there were either no results, or more than one result, I figure?
There was a problem hiding this comment.
Although that case should always be 0 in reality I added a further debugging parameter, s. also #11715
You can merge the other MR and close this one, or the other way round, I wouldn't care too much.
BTW: Since the last changes in develop LDAP login does not seem to work any more ("SAML page requested, but SAML does not seem to enabled."). We have no SAML active (maybe this should be opened as separated issue?).
There was a problem hiding this comment.
You can safely ignore the SAML logging. It's simply because it's in a service provider so it gets loaded on every request. It has nothing at all to do with LDAP syncing or auth.
There was a problem hiding this comment.
"Since the last changes in develop LDAP login does not seem to work any more"
Sorry, was my fault. Since bookworm I have to activate "Allow invalid SSL Certificate" although we have an official letsencrypt certificate with our LDAP servers. Something seems having become stricter.
There was a problem hiding this comment.
Happy to help you along with your specific issues - come ping us on Discord, maybe we can figure something out. Sometimes it's having a modern CA bundle.
There was a problem hiding this comment.
Has nothing to do with snipe-it, but just FYI: When ldap.so is linked towards libgnutls, LDAP is much stricter than when linked against libssl. On systems where a patched php-ldap is installed, there is no certificate problem.
Signed-off-by: Katharina Drexel <katharina.drexel@bfh.ch>
Signed-off-by: Katharina Drexel <katharina.drexel@bfh.ch>
|
@uberbrady did you get a chance to test this? This PR is pretty old now, so we should merge it or close it IMHO |
Description
Until now in the LDAP authentication the bind dn was concatenated with username and basedn. There might be cases where you want users from different subtrees (can be solved by using a corresponding filter), or maybe stay open if there will be some LDAP group mapping feature added later on.
Therefore when the user enters his/her name in the login form the backend should find out the corresponding dn and use that one for LDAP binding.
If no dn is found the previous mechanism (concatenation username+basedn) will go on.
The statements above are only valid for LDAP (not AD related).
Fixes #9063
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Used the LDAP Synchronisation and Test LDAP Login function and the login page in debugging mode with correct user, correct and wrong password, wrong user.
Test Configuration:
Checklist:
(unit tests errors refer to missing database.sqlite and have nothing to do with the change)