Authorization: Bug with commas in CN and other ldap attributes not being dealt with correctly

My guess is the "Doe\, John" part is causing the problem. I'm working on a fix for this bug and should have it up shortly.

The fix for this is in head of 7.x-1.x. The only change was around link 338 of ldap_authorization.inc
- $pairs = explode(',', $user_ldap_entry['dn']);
+ $pairs = ldap_explode_dn($user_ldap_entry['dn'], 0);
+ $count = array_shift($pairs);

By exploding on "," the escaped \, was not being dealt with correctly.

Please let me know if this fixes the issue.

I found another related bug that may help. I pushed the fix and related simpletests to head.

In ldap_authorization.inc, the following

          if ($consumer_conf->deriveFromAttrUseFirstAttr) {
            $attr_parts =explode(',', $attr_lcase);
            $first_part = explode('=', $attr_parts[0]);
            $attr_lcase = trim($first_part[1]); 
          }

was replaced with:

          if ($consumer_conf->deriveFromAttrUseFirstAttr) {
            $attr_parts = ldap_explode_dn($attr_lcase, 0);
            $first_part = explode('=', $attr_parts[0]);
            $attr_lcase = str_replace('\\2C',',',trim($first_part[1])); 
          }

I believe the replacing of the \\2C (escaped comma) needs to be added in stategy 1 around line 341 also because the ldap_explode_dn() function returns the commas escaped. I'll look at that and write a simpletest for it later.

Back to comment 1.

I believe the filter:

member=CN=Doe\, John,OU=InformationTechnology,OU=CITYNAME,DC=fubar,DC=com

should be:

(member="CN=Doe\, John,OU=InformationTechnology,OU=CITYNAME,DC=fubar,DC=com")

The changes are in head and were in ldap_authorization.inc from:

 $entries = $ldap_server->search($branch, $consumer_conf->deriveFromEntryAttr . '=' . $user_ldap_entry['dn'],  array('cn'));

to:

      $filter = '(' . $consumer_conf->deriveFromEntryAttr . '="' . $user_ldap_entry['dn'] . '")';
      $entries = $ldap_server->search($branch, $filter, array('cn'));

This is where the mock framework for the simpletests break down. This has to be tested on a real server as the mock framework can't represent the way an ldap server will treat quoted and escaped strings.

I also added some logging in the ldap query when detailed ldap logging is enabled to help debug such things. One concern I have I don't know how a case such as cn="Doe\, John...." is dealt with by ldap servers. Should the comma still be escaped or not when its quoted.

Please try and let us know.

the query should be:

(member="CN=Pierce\, Chris,OU=InformationTechnology,OU=FTDODGE,DC=dtlinc,DC=com")

So the bad query must be coming from line 317 of ldapServer.class.php which is the only place we haven't quoted the filter values.

$filter = $this->user_attr . '=' . $ldap_username;

should change to:

$filter = '('. $this->user_attr . '="' . $ldap_username . '")';

(this is in head)

When you turn on detailed ldap logging on the main ldap config page, what do the query logs look like? This should show what the filter string looks like before it goes into the ldap function.

the error your getting is the filter syntax, so I don't think this will help. But to debug that go with the following which will return all attributes, but will not try to use the cn.

  $derive_from_entry_authorizations = array();
  if ($consumer_conf->deriveFromEntry) {
    foreach ($consumer_conf->deriveFromEntryEntries as $branch) {
      $filter = '(' . $consumer_conf->deriveFromEntryAttr . '="' . $user_ldap_entry['dn'] . '")';
      $entries = $ldap_server->search($branch, $filter);
      if (empty($entries) || $entries['count'] == 0) {
        $filter = '(' . $consumer_conf->deriveFromEntryAttr . '="' . $user->name . '")';
        $entries = $ldap_server->search($branch, $filter);
      }
      foreach ($entries as $entry) {
       // if (isset($entry['cn'])) {
        //  $derive_from_entry_authorizations[$entry['cn'][0]] = $entry['cn'][0];
       // }
       // elseif (isset($entry['dn'])) {
          $derive_from_entry_authorizations[$entry['dn']] = $entry['dn'];
       // }
      }
    }
  }

change line 239 of ldapServe.class.php

from
function search($base_dn = NULL, $filter, $attributes = array(), $attrsonly = 0, $sizelimit = 0, $timelimit = 0, $deref = LDAP_DEREF_NEVER) {

to

function search($base_dn = NULL, $filter, $attributes = NULL, $attrsonly = 0, $sizelimit = 0, $timelimit = 0, $deref = LDAP_DEREF_NEVER) {

I ended up taking the quotes out of the filter. It was causing a problem in test cases. The filter should now be:

(member=CN=Pierce\, Chris,OU=InformationTechnology,OU=FTDODGE,DC=dtlinc,DC=com)

Hi,

I've searched since yesterday on an issue I had with Authentication and Authorization, which is now some kind messed up.

For the information (it's somehow related to this thread, because it's related to the quotation):

Yesterday evening I installed the last ldap package and it had in "LdapServer.class.php":
$filter = '('. $this->user_attr . '="' . $ldap_username . '")';
which needs to be:
$filter = '('. $this->user_attr . '=' . $ldap_username . ')';

This is already fixed in the actual dev release, but for the filter in the "ldap_authorization.inc", it is still:
$filter = '(' . $consumer_conf->deriveFromEntryAttr . '="' . $user_ldap_entry['dn'] . '")';
which needs to be:
$filter = '(' . $consumer_conf->deriveFromEntryAttr . '=' . $user_ldap_entry['dn'] . ')';

Further even if by using 2.C (group configuration) in the authorization plugin, I cannot use the dn of my "groupOfNames", instead I need to use only the cn for the plugin to work.

My ldap entry for groupOfNames is:
# test, drupal, olznet.de
dn: cn=test,ou=drupal,dc=olznet,dc=de
objectClass: groupOfNames
cn: test
member: uid=mfulz,ou=users,dc=olznet,dc=de

The mapping in 3 is set to:
test|basic

with:
cn=test,ou=drupal,dc=olznet,dc=de
which should be used (suggested by the help) its not working.

So perhaps the thread creator here, has the same problem?

I applied this to head. This would have affected those using stategy 3 (or C), group as entries.

D7 stable release blocker

I've discovered that the article linked to by cpierce in #20 has the solution. Just solved our problem. Patch attached. I wonder if this has only applies to AD and not LDAP stuff in general? And I wonder if this is the best place to do the replacement?

cpierce, you can add this line of code before line 280 of ldap_servers/LdapServer.class.php

$filter = str_replace("\\","\\5c",$filter);

so the final code should look like:

case LDAP_SCOPE_SUBTREE:
        $filter = str_replace("\\","\\5c",$filter);
        $result = ldap_search($this->connection, $base_dn, $filter, $attributes, $attrsonly, $sizelimit, $timelimit, $deref);

I can confirm that #28 got things working for me. Could we have it rolled into a release?

I think this could be applied. It definately seems to help AD implementations. Can anyone confirm it doesn't break non AD implementations? Or should I only apply it if the server type is set to Active Directory?

this is in 7.x-1.x and 7.x-2.x now.

Many thanks, this module makes building intranets on Drupal so much easier. It allows us to say, "Yes, Drupal will 'integrate' with your AD server just fine".