LDAP SSO: drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL) is excessive

I like the direction, but don't like loading user and ldap before a full bootstrap. LDAP isn't designed for that, even though they may work. I think its better to not do any bootstrap and get $auth_conf->seamlessLogin directly from the database, or keep it in the variables table. The other code in common.inc for redirects can just be loaded without the bootstrap or done with a direct php header.

I added a check command line because the hook_boot broke my migration script.

  if (!drupal_is_cli() && $GLOBALS['user']->uid == 0) {

This still needs attention to get the

drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL)

out of it.

This is in head.

I'm working on a patch to split the hook_boot part and some of the sso code into a separate ldap_sso module. I believe the craziness of the authentication include file needs to be in ldap authentication to avoid an excessive hook system. But some other code will be more readable and manageable in a separate module.

Here how I approached this:

- created ldap_sso module (enabling and disabling it enables sso). update code enables it if sso was enabled in ldap authentication
- moved easy to move code inlcuding hook_boot into ldap_sso.
- altered ldap_sso_boot() function to:
-- check for command line interface so doesn't break drush, aegir, etc.
-- created static getSaveableProperty function in ldapAuthenticationConfAdmin class so bootstrap and ldap module doesn't need to be loaded.

This is in head.

Is this expected in the new ldap_sso module? Looks like something isn't loaded still in hook_boot?

Call to undefined function drupal_get_path() in /var/www/sites/all/modules/ldap/ldap_sso/ldap_sso.module on line 60

no. this is a bug. the following code fixes it in ldap_sso.module

      if (isset($_COOKIE['seamless_login_attempted']))
          $login_attempted = $_COOKIE['seamless_login_attempted'];
        else {
          $login_attempted = FALSE;
        }
        drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
        require_once('includes/common.inc');
        require_once(drupal_get_path('module', 'ldap_authentication') . '/LdapAuthenticationConfAdmin.class.php');

I committed the patch in #8, but it still needs work. A full bootstrap should not be needed to get the value of 1 variable without bootstrapping.

The following resolves this, but I don't believe will work correctly with features/strongarm because of the way it loads the variable.

       require_once('includes/common.inc');
        $ldap_authentication_conf = variable_get('ldap_authentication_conf', array());
        if ($ldap_authentication_conf['seamlessLogin'] == 1 && ($login_attempted != 'true')) {
          setcookie("seamless_login_attempted", 'true', time() + (int)$auth_conf->cookieExpire, base_path(), "");
          $_SESSION['seamless_login_attempted'] = $login_attempted;
          drupal_goto('user/login/sso', array('query' => array('destination' => rawurlencode($_GET['q']))));

In the recent dev release this is still pops up:

Call to undefined function _ldap_authentication_user_login_authenticate_validate() in /var/www/sites/all/modules/ldap/ldap_sso/ldap_sso.module on line 122

Thanks for catching this. That call should be:

$user = ldap_authentication_user_login_authenticate_validate(array(), $fake_form_state);

this is in head. Sorry about the turbulence the last couple of commits. Any help on simpletests for ldap_sso would be appreciated.

I think its stable now. I'm going to stay out of the ldap_sso from now on since I don't have a good way to test it. At least its not bootstrapping when the ldap_sso module is disabled.

If anyone can cut down on the amount of bootstrapping needed in ldap_sso_boot() that would be great.

This should probably be:

        require_once('includes/common.inc');
        require_once('includes/path.inc');

          drupal_goto('user/login/sso', array('alias' => TRUE, 'query' => array('destination' => rawurlencode($_GET['q']))));

Otherwise the second error mentioned in #1379506: SSO: ldap_sso_boot() multiple issues occurs again.

Edit: This error occurs because drupal_goto() calls url() which then calls drupal_get_path_alias(); which can't be found as path.inc isn't loaded.
This last function call can be avoided if we tell drupal_goto() that the URL is already an "alias".

So can drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL); be removed with this change?

Sorry. The edit I made to my above post makes it confusing.

With this change, you only need to include includes/common.inc rather than run a full bootstrap (as you did in #9).

The only change to your code in #9 would be:

          drupal_goto('user/login/sso', array('alias' => TRUE, 'query' => array('destination' => rawurlencode($_GET['q']))));

John,

In regard to #11 ldap_authentication_user_login_authenticate_validate() does not return the $user object. This results in the wrong condition being met with:

if ($user && $user->uid > 0) {

because user is being set to VOID.

To correct this ldap_authentication_user_login_authenticate_validate() in ldap_authentication.module needs to return the result of _ldap_authentication_user_login_authenticate_validate($form_state) around line 331.

function ldap_authentication_user_login_authenticate_validate($form, &$form_state) {
  require_once('ldap_authentication.inc');
  return _ldap_authentication_user_login_authenticate_validate($form_state);
}

There are a couple other tweaks in ldap_sso.module that I will send over to fix a couple minor issues with drupal_goto() redirecting to broken pages. Patch coming soon.

Here is a patch to fix a couple problems with ldap_sso.module. It corrects some indentation for readability and fixes the destination variable that gets messed up before a drupal_goto() causing you to get a page not found after logging in.

Darn forgot status. One day I'll learn to preview.

And just for good measure here's a patch for #16.

If you have other SSO or Active Directory issues you would like me to test let me know as I have an environment that requires both of these on a regular basis.

#17 and #19 are worked into the new ldap_sso. I'm altering it a bit and adding unit tests. We should close this issue as its all over the place. For those of you with ldap_sso working the way you want, avoid the next release unless you want to help with the testing.

After getting the error described in http://drupal.org/node/1638414, I think at least drupal_bootstrap(DRUPAL_BOOTSTRAP_LANGUAGE); needs to be added before the drupal_goto() gets called in ldap_sso_boot().

Thanks for thinking this through, this issue is going a bit in circles. The idea is to avoid as much bootstrap as possible. Even if that means not using the goto. Can you explain why drupal_bootstrap(DRUPAL_BOOTSTRAP_LANGUAGE); needs to be called before drupal_goto? And can we use an alternative to drupal_goto?

The drupal_goto() calls the url() function wich calls the drupal_get_path_alias() function wich calls the drupal_lookup_path() function. The drupal_lookup_path() function needs the global $language defined wich isn't set yet in hook_boot().

Maybe there is another way, but adding drupal_bootstrap(DRUPAL_BOOTSTRAP_LANGUAGE); did the trick.

sounds good. Since I don't use the sso part, can someone else who does try this out and confirm it?

Tagging as 7.x-1.0 release blocker

Thanks. I committed the change in comment #23 in 7.x-1.x-dev and 7.x-2.x-dev.