I would have sworn I had this working in the past, but I can't get it to work now after upgrading to beta2. I'm using postgresql, drupal 6.14, ldap_integration 6.x-1.0-beta2, on debian

Steps to reproduce:

  1. Install LDAP Group module, create & Configure LDAP Group Server
  2. Use sample filter given below the field into the PHP to filter roles by
    $groups = array_filter($groups, create_function('$a', 'return preg_match(\'/Staff/\', $a);'));
return $groups;
  3. Log in on any LDAP account

Expected Behavior
User logged in, only groups that have "Staff" in their name are used

Seen Behavior after logging in
warning: Invalid argument supplied for foreach() in ..../ldap_integration/ldapgroups.inc on line 53.

Relevant code:

 46   // Next, we apply site-specific rules.
 47   $groups = _ldapgroups_filter($account, $groups);
 48 
 49   // At this point, the roles are in the full DN format.
 50   $roles = array();
 51   if (!empty($groups)) {
 52     $ldapgroups_mappings = _ldapgroups_ldap_info($account, 'ldapgroups_mappings');
 53     foreach ($groups as $group) {

When I add var_dump($groups) before line 53, I see that $groups contains a string instead of an array!:

string(138) "$groups = array_filter($groups, create_function('$a', 'return preg_match(\'/cms(Admins|ENGGSITECAPSHORTNAME)/\', $a);'));
return $groups;"

I tracked this down to the _ldapgroups_filter() function in ldapgroups.inc:

216   if ($code = _ldapgroups_ldap_info($account, 'ldapgroups_filter_php'))
217     $groups = drupal_eval($code);

This seems odd to me -- according to the api for drupal_eval, code passed to drupal_eval should be surrounded in <?php ?> tags -- but the example given for ldapgroups_filter_php isn't in these tags.

Also, if you simply surround the example with <php ?> tags, it still doesn't work, because it assumes the $groups variable is available in the eval context, but the context of the filter is actually the function drupal_eval(), which has no access to the $groups variable.

Like I said, I'm very confused, because I was sure I had the filter working before my recent upgrade to beta2, but in checking out the CVS code, I don't see a change that would have introduced this. Am I missing something obvious?

CommentFileSizeAuthor
#3 ldapgroups_evalfix.patch664 bytesclaar

Comments

pbosmans’s picture

pbosmans commented

Thesame problem here.
In the previous version we had no troubles with these filter.
Now we have tesame problem as described above.

claar’s picture

claar commented

pbosmans: Thanks for the confirmation.

Found the source of this bug -- looks like eval() was changed to drupal_eval() in the latest version (beta2).

This took me a bit to find since the _ldapgroups_filter() function moved from being in the ldapgroups.module file to a new file called ldapgroups.inc

If you change line 217 of ldapgroups.inc back to:

    $groups = eval($code);

the filter should work again. Innocent mistake to assume that drupal_eval() is a drop-in replacement to eval() -- unfortunately, they have quite different behaviors!

claar’s picture

claar commented
Status: Active » Needs review
StatusFileSize
new ldapgroups_evalfix.patch664 bytes

I tried to figure out how one might use drupal_eval() instead of eval() in this scenario, but it just doesn't make sense. We need to allow manipulation of the $groups array via user-submitted PHP -- drupal_eval() simply doesn't allow this in any straight-forward manner.

So, here's a patch against HEAD to revert back to using eval().

markDrupal’s picture

markDrupal commented

Do you have to include the " " opening and closing tags in the form text box 'PHP to filter roles by: ' ?

If so it should , some help text saying so in the admin form should be part of this patch.
Thanks!

---UPDATE---
I tried it out and it looks like you don't have to,

markDrupal’s picture

markDrupal commented

I've tested this patch and it fixed it for me!

iamer’s picture

iamer commented

According to the current note under the "PHP to filter roles by" : "The code is evaluated before the above mapping is applied."

However this is not the case in the code in ldapgroups.inc , after applying the above patch I had to move the lines

if ($code = _ldapgroups_ldap_info($account, 'ldapgroups_filter_php')) {
$groups = eval($code);
}

to the beginning of the function _ldapgroups_filter.

Anyone else seeing this problem ?

NathanielTX’s picture

NathanielTX commented
Status: Needs review » Reviewed & tested by the community

Thanks! This change fixes everything for groups, i thought I was missing something.

Is there some reason this hasn't been merged into the release after several months? This is a pretty big piece of broken functionality with an easy fix.

thtas’s picture

thtas commented

just to confirm for others reading

This only works if you change the code from drupal_eval to eval and move the code block to the top of the _ldapgroups_filter function. Otherwise the $groups array just contains a list of mapped roles, and not your valuable DN information which you need for processing.

if ($code = _ldapgroups_ldap_info($account, 'ldapgroups_filter_php')) {
$groups = eval($code);
}

e.g.

function _ldapgroups_filter($account, $groups) {

	if ($code = _ldapgroups_ldap_info($account, 'ldapgroups_filter_php')) {		
		$groups = eval($code);
	}

  if (_ldapgroups_ldap_info($account, 'ldapgroups_mappings_filter') && count(_ldapgroups_ldap_info($account, 'ldapgroups_mappings') > 0)) {
    $groups_new = array();
    foreach ($groups as $group) {
      foreach (_ldapgroups_ldap_info($account, 'ldapgroups_mappings') as $group_approved => $role) {
        if (strcasecmp($group_approved, $group) == 0)
          $groups_new[] = $role;
      }
    }
    $groups = $groups_new;
  }

  return $groups;
}

My settings in LDAP group for the code snipped are as follows (NO PHP TAGS REQUIRED)

PHP to filter roles by:
$groups = mymodule_eval_ldap_groups($groups);
return $groups;
johnbarclay’s picture

johnbarclay commented
Version: 6.x-1.0-beta2 » 6.x-1.x-dev

this was committed to head some time ago. It will be in beta3.

johnbarclay’s picture

johnbarclay commented
Status: Reviewed & tested by the community » Closed (fixed)
macman824’s picture

macman824 commented
Version: 6.x-1.x-dev » 6.x-1.0-beta3
Status: Closed (fixed) » Active

I'm re-opening this because somewhere along the way this fix didn't make it into beta3. Line 248 of ldapgroups.inc, as of beta3, still has drupal_eval() rather than plain old eval(). The fix remains the same.

cgmonroe’s picture

cgmonroe commented
Status: Active » Fixed

Change committed and will be in next -dev build.

FYI - the new code also contains hook_ldap_user_groups_alter() with can be used to alter the groups returned from ldap. See the ldapgroups.api.php document for details. This is probably a safer way to filter groups than using eval code.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.