Crivaledaz / Mattermost-LDAP

This module provides an external LDAP authentication in Mattermost for the Team Edition (free).
MIT License
359 stars 71 forks source link

LDAP handling could be more flexible #6

Closed gagei closed 7 years ago

gagei commented 7 years ago

This is more a feature request than an issue.

The LDAP handling could be more flexible. There are two main problems in my opinion:

  1. Some LDAP installations do not use the Attribute UID for the RDN of person objects. In our case CN is used.

  2. LDAP installations can have more than one OU, where person objects are located. This means, a simple join of UID and $rdn_suffix like in connexion.php is not possible:

    $rdn = 'uid=' . $user . $rdn_suffix;

I suggest to determine $rdn by a LDAP search before trying to login with the found DN. This is the behaviour of most other LDAP enabled applications, I know.

These attributes of the search should be configurable in the config file:

base_dn
search_attribute    (e.g. uid, sAMAccountName, mail)
filter              (e.g. "objectClass=person")
bind_dn
bind_password

The filter parameter for the "ldap_search" function should be constructed by an AND relation of search_attribute and filter, e.g.:

(& ($search_attribute=$user) ($filter))

$user is the login name, which the user enters on the login screen.

Crivaledaz commented 7 years ago

Hi,

Thank you for your feedback, I am always interested by new features and improvement.

Concerning the first point, I agree with you, I currently deploy Mattermost LDAP on a network on which the attribute CN is used instead of the UID. So, I will probably push this feature in a commit by the end of the week.

For the second point, I know LDAP can have more than one OU, that was the case for my first deployment. But, in the $rdn_suffix, as in base, you can put many OU attributes. In fact, you can have : rdn_suffix = 'ou=People,ou=Admin,o=Company. That's why I have created only one suffix variables. But if you think is necessary, I can add a prefix variable. I am not sufficiently comfortable with the LDAP config to know if it is useful.

Finally, I am not sure to understand your last suggestion. If I summarize the process you have proposed :

  1. ldap_bind with service account (bind_dn and bind_password)
  2. ldap_search with a search attribute and filters on user to get the right DN
  3. ldap_bind with the found DN and user password (from login page)

So, with this solution, $rdn could be removed ... This concern only the connexion.php script or I should do the same for filter in resource.php ?

I think, after to have written this message, I begin to understand what you suggest ... It will be better after a good night ;) I will try to add this in the next commit, thanks for your help.

Crivaledaz

gagei commented 7 years ago

Hi Crivaledaz,

But, in the $rdn_suffix, as in base, you can put many OU attributes. In fact, you can have : rdn_suffix = 'ou=People,ou=Admin,o=Company.

'ou=People,ou=Admin,o=Company' defines one (!) OU in the third level of the LDAP tree. But I mean something other. The LDAP server could contain person objects in a structure like this:

ou=department1,o=Company ou=department2,o=Company ...

These are two (ore more) OUs in the second level of the tree. You can't construct $rdn by joining uid=$user with the parent OU, because there are more than one possible parent OUs.

Finally, I am not sure to understand your last suggestion. If I summarize the process you have proposed :

  1. ldap_bind with service account (bind_dn and bind_password)
  2. ldap_search with a search attribute and filters on user to get the right DN
  3. ldap_bind with the found DN and user password (from login page)

Yes. You could additionally fetch "cn" and "mail" already in step 2. Otherwise you would have to do this in a 4th step after the login. Of course the attribute which is searched in step 2 has to be unique in the whole LDAP server or at least in the subtree, which is defined by the search base ($base).

So, with this solution, $rdn could be removed ... This concern only the connexion.php script or I should do the same for filter in resource.php ?

I'm not sure, because I've never learned PHP and I understand the code only partially.

Ingo

Crivaledaz commented 7 years ago

Hi,

I have pushed a commit which implements your suggestions. It works fine and actually it's better. Thanks for your help. I would be happy to have your feedback,

Regards,

Crivaledaz

gagei commented 7 years ago

Hi,

I did not check intensively, but I was able to login to Mattermost, now. Thank you very much.

I discovered one more small issue. My definition of $filter is like this:

$filter = "(&(objectClass=inetOrgPerson)(employeeStatus=aktiv))";

This leads to an error message of our LDAP Server because of a syntactically wrong search filter. If I omit the outer brackets, the LDAP search works:

$filter = "&(objectClass=inetOrgPerson)(employeeStatus=aktiv)";

This is caused by the double brackets, because the definition of $search_filter adds one more bracket pair around $filter:

LDAP.php (line 107 and 196):

$search_filter = '(&(' . $search_attribute . '=' . $user . ')(' . $filter .'))';

I suggest to add an if condition, which checks whether $filter contains surrounding brackets. If yes, the bracket pair around $filter in the definition of $search_filter should be omitted.

Regards, Ingo