I'l like to be able to invoke a hook from a site custom moduel, that excludes certains users from logging in (ba DN pattern matching).
Looking at LdapAuthenticationConf.class.php, allowuser() I inserted the following at the end, before the return TRUE:

    // allow other modules to hook in and refuse if they like
    drupal_alter('ldap_authentication_allowuser_results', $ldap_user_entry, $name, $hook_result);
    if (isset($hook_result) && !$hook_result) {
      watchdog('ldap_authentication_allowuser', 'result=refused');
      return FALSE;
    }

Implementation of that works. Is that the right approach? (I dont want to enable the PHP module and use php filters in the GUI):

Aside: the messages to the user who is refised are not very nice:

Notice: Trying to get property of non-object in drupal_lookup_path() (line 77 of /var/www/includes/path.inc).
Sorry, your LDAP credentials were not found, or the LDAP server is not available. You may log in with other credentials on the user login form.

Comments

johnbarclay’s picture

johnbarclay commented
Version: 7.x-1.0-beta10 » 7.x-1.x-dev
Category: support » feature
Status: Active » Needs review

This is committed to the 1.x and 2.x branch. Thanks. The watchdog call was a little off as were a couple others in the same class (see http://drupalcode.org/project/ldap.git/commitdiff/e460e694e4cae689e03b0d...).

    // allow other modules to hook in and refuse if they like
    $hook_result = TRUE;
    drupal_alter('ldap_authentication_allowuser_results', $ldap_user_entry, $name, $hook_result);
    if (!$hook_result) {
      watchdog('ldap_authentication', "Authentication Allow User Result=refused for %name", array('%name' => $name), WATCHDOG_NOTICE);
      return FALSE;
    }

If you want to put that error message in as a bug issue, that would be helpful. Not sure I understand when the message comes up.

johnbarclay’s picture

johnbarclay commented

This is committed to the 1.x and 2.x branch. Thanks. The watchdog call was a little off as were a couple others in the same class (see http://drupalcode.org/project/ldap.git/commitdiff/e460e694e4cae689e03b0d...).

    // allow other modules to hook in and refuse if they like
    $hook_result = TRUE;
    drupal_alter('ldap_authentication_allowuser_results', $ldap_user_entry, $name, $hook_result);
    if (!$hook_result) {
      watchdog('ldap_authentication', "Authentication Allow User Result=refused for %name", array('%name' => $name), WATCHDOG_NOTICE);
      return FALSE;
    }

If you want to put that error message in as a bug issue, that would be helpful. Not sure I understand when the message comes up.

boran’s picture

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

Thanks. Yes, that watchdog is better.
I upgraded to latest dev, added that code: the code works fine.

However the sites now goes into a loop, which it tries to authenticate 5-6 times. I've moved that issues and the related discussion of the path.in error above to #1638414: Site looping when ldap sso refused: Trying to get property of non-object in drupal_lookup_path() (line 77 of /var/www/includes/p.

boran’s picture

boran commented

One could also add a bit of documentation for the api:
ldap_authentication/ldap_authentication.api.php

<?php
/**
 * Documentations of the module hooks
 */


/**
 * Allow a custom module to examine the user's ldap details
 * and refuse authentication
 * $ldap_user_entry contains the ldap result, $name is the login name
 *  Set $result to FALSE to refuse access (must be passed by reference)
 *
 */
function hook_ldap_authentication_allowuser_results_alter($ldap_user_entry, $name, &$result) {
  $result=TRUE;
  //if (preg_match('/CN=BADGUY/', $ldap_user_entry['dn'], $matches) > 0) {
  //  watchdog(__FUNCTION__, $ldap_user_entry['dn']);
  // $result=FALSE;
  //}
}
johnbarclay’s picture

johnbarclay commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Issue tags: +7.x-2.0 release blocker

I changed the documentation for this in the 2.0 branch, emphasizing that another module could have already set it to FALSE. @boran, does this make sense to document this way?

function hook_ldap_authentication_allowuser_results_alter($ldap_user, $name, &$hook_result) {
  
  if ($hook_result === FALSE) { // other module has denied user, should not override
    return;
  }
  elseif ($hook_result === TRUE) { // other module has allowed, maybe override
    if (mymodule_dissapproves($ldap_user, $name)) {
      $hook_result = FALSE;
    }
  }

}
boran’s picture

boran commented

Sorry for he slow answer.
Yes your point about checking for False first is very important.
Still would like to see the comments about the function name though, will also show in Api documentation tools .

johnbarclay’s picture

johnbarclay commented

I think I got it all together now. This is committed to 7.x-2.x-dev. Appreciate you following up on documentation; documentation really helps others and keeps the issue queue thinner.

Does this look like what you are after?

<?php

/**
 * Allow a custom module to examine the user's ldap details
 * and refuse authentication.  See also: http://drupal.org/node/1634930 
 *  
 *  @param array $ldap_user
 *    See README.developers.txt for structure
 *  @param string $name
 *    The drupal account name or proposed drupal account name if none exists yet
 *  @param boolean $hook_result
 *    TRUE for allow, FALSE for deny.
 *    If set to TRUE or FALSE, another module has already set this and function should
 *    be careful about overriding this.
 *
 *  @return boolean &$hook_result passed by reference
 */

function hook_ldap_authentication_allowuser_results_alter($ldap_user, $name, &$hook_result) {
  
  if ($hook_result === FALSE) { // other module has denied user, should not override
    return;
  }
  elseif ($hook_result === TRUE) { // other module has allowed, maybe override
    if (mymodule_dissapproves($ldap_user, $name)) {
      $hook_result = FALSE;
    }
  }

}
johnbarclay’s picture

johnbarclay commented
Component: Code » SimpleTests
Status: Reviewed & tested by the community » Needs work

Functioning correctly. Needs simpletest coverage.

boran’s picture

boran commented

Very good, thanks!
I'm still on the 1.x (since production is v.soon for me), the upgrade from 7.x-1.0-beta11+2-dev to 7.x-1.0-beta11+15-dev went well on my test site.
I see ldap_authentication.api.php has not made it into 1.x, but you noted that above.

johnbarclay’s picture

johnbarclay commented
Status: Needs work » Postponed

Trying to freeze new features in favor of stability. Marking this as postponed, which means simpletests won't be in until 8.x-3.x I believe

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Issue tags: +Needs tests

Tagging

  • johnbarclay committed 5c77313 on 8.x-3.x 
    Issue #1634930 by boran.  Cleaning up API documentation
  • johnbarclay committed f76ed6a on 8.x-3.x authored by boran
    Issue #1634930 by boran.  Added hook alter to allow intervention in ldap...
grahl’s picture

grahl
he/him
Location Zürich
commented
Component: SimpleTests » Code
Status: Postponed » Closed (outdated)

Seems to work, tests need to be worked on separately.