MailWatch icon indicating copy to clipboard operation
MailWatch copied to clipboard

do not construct user DN, but use DN from search

Open ghost opened this issue 7 years ago • 10 comments

The proper way to construct a user DN is not by appending LDAP_BIND_PREFIX or LDAP_BIND_SUFFIX to LDAP_USERNAME_FIELD

Instead use DN from ldap_search

The reason is that you might have multiple suffixes for user DNs in LDAP ie: ou=People,dc=example,dc=com ou=People,dc=sub,dc=example,dc=com

ghost avatar Jan 16 '18 11:01 ghost

Not sure that this PR is necessary as the current version already supports authenticating the user by its DN and is more flexible in other senarios. To use DN with the current version use following settings:

  • Define LDAP_USERNAME_FIELD as "dn" or "distinguishedname"
  • Leave LDAP_BIND_PREFIX and LDAP_BIND_SUFFIX commented

Skywalker-11 avatar Jan 16 '18 12:01 Skywalker-11

Yes i've tried setting: LDAP_USERNAME_FIELD as "dn" but later in your code you use $user = $result[0][LDAP_USERNAME_FIELD][0];

which is not valid for ['dn'] all entries have $result[0]['attr'][0] except dn (see ps at end)

I deal with ldap servers and ldap-authenticated application for many years now, most of them do the following ldap_bind/ldap_search to find user_dn ldap_bind with user_dn to check user auth

Constructing user_dn with ldap_base and other fields is ad-hoc

best regards

G ps. see example from ldap_get_entries:

Array( [count] => 1 [0] => Array ( [mail] => Array ( [count] => 1 [0] => [email protected] )

        [0] => mail
        
        [count] => 1
        [dn] => uid=bilias,ou=People,dc=example,dc=com
    )

ghost avatar Jan 16 '18 12:01 ghost

Sorry I didn't saw that dn doesn't have an array. The following code should fix that and keeps compatibility

               if (in_array('group', array_values($result[0]['objectclass']), true)) {
                    // do not login as group
                    return null;
                }

                if (!isset($result[0][LDAP_USERNAME_FIELD])) {
                    @trigger_error(__('ldapno03') . ' "' . LDAP_USERNAME_FIELD . '" ' . __('ldapresults03'));
                    return null;
                }
                if (!is_array($result[0][LDAP_USERNAME_FIELD])) {
                    $user = $result[0][LDAP_USERNAME_FIELD];
                } elseif (isset($result[0][LDAP_USERNAME_FIELD][0])) {
                    $user = $result[0][LDAP_USERNAME_FIELD][0];
                } else {
                   @trigger_error(__('ldapno03') . ' "' . LDAP_USERNAME_FIELD . '" ' . __('ldapresults03'));
                   return null;
                }

                if (defined('LDAP_BIND_PREFIX')) {
                    $user = LDAP_BIND_PREFIX . $user;
                }
                if (defined('LDAP_BIND_SUFFIX')) {
                    $user .= LDAP_BIND_SUFFIX;
                }

Skywalker-11 avatar Jan 16 '18 16:01 Skywalker-11

Yes it does :) but it's still ad-hoc, adds complexity and I believe it's prune to errors.

You're doing all that to "construct" the user_dn php-ldap already has a function to do that. use it and don't do all those checks and ninja coding

Maybe there is something I don't understand. Do you use LDAP_USERNAME_FIELD for anything else except user_dn?

(no offence intended) :-)

ghost avatar Jan 16 '18 16:01 ghost

I think there was once a feature request for constucting the username like that but I have to look it up

Skywalker-11 avatar Jan 16 '18 17:01 Skywalker-11

I just checked and couldn't find such a feature request. But since there still is the possibility that someone uses this I think we should stick with the current version for 1.2 and apply this pr for 2.x

Skywalker-11 avatar Feb 11 '18 20:02 Skywalker-11

I agree with @Skywalker-11 : this pr should be rebased on develop and applied logic moved to MailWatch\Ldap class

endelwar avatar Jun 13 '18 08:06 endelwar