Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not construct user DN, but use DN from search #1029

Open
wants to merge 1 commit into
base: 1.2
Choose a base branch
from
Open

do not construct user DN, but use DN from search #1029

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 16, 2018

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

@endelwar endelwar changed the base branch from master to 1.2 January 16, 2018 11:47
@Skywalker-11
Copy link
Member

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

@ghost
Copy link
Author

ghost commented Jan 16, 2018

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
    )

@Skywalker-11
Copy link
Member

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;
                }

@ghost
Copy link
Author

ghost commented Jan 16, 2018

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) :-)

@Skywalker-11
Copy link
Member

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

@Skywalker-11
Copy link
Member

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

@endelwar
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants