Authentication hook for local users

use hook_user(login)

hook_auth deliberately is ineffective for local logins. if you enable it for all logins, you will slow down the experience for many people who guess their passwords wrong on login page. in that case, code would call all remote auth modules an dthey in turn would contact their home base. thats too slow. we want to return a 'sorry try again' message very quickly.

what are you trying to accomplish here?

The fix allows local users (i.e. just "user") to be authenticated against an external source.

Why? I assume authentication should be either local or external--not both. Therefore, we need a way to distinguish between the two. Currently, the username@domain convention is used. Removing the @domain part of the DrupalID would remove this disctinction. Is the issue just that this convention is confusing for new users? In this case, I'd suggest pulling the domain part out into a different field. So, when drupal.module is enabled, there is a third user login field, "site" or "domain", that would include "local" and any registered associates.

Nedjo said:

"I assume authentication should be either local or external--not both"

What exactly do you mean by "local" and "external"? Are you assuming:

1. The definition of "local" means logging in with just username to get the username and password info from the name/pass fields in the core Drupal users table? (without calling hook_auth())
2. The definition of "remote" means logging in with username@doman and calling hook_auth to get the username password info from some source other than the core Drupal users table?

   I think this issue is talking about implementing some sort of authentication hook/module so that item (1) can access some other local table besides the core Drupal users table without having to hack the user.module.

   It sounds like, right now, the answer is, "can't do that".

   Ben in DC

Well, this is one of the things I've been struggling with since I took over ldap_integration module maintenance. So I'm all for it.

For my module, I solved the issue without having to patch Drupal's core, but yet having to perform some hack stunt: I replaced a callback set in user.module by another one placed in an auxiliary module I called zcallbacks.module. (The name is due to its need to be alphabetically after user.module for the hack to work). Have a look at the code for the gory details.

Not to mention that I'd rather have a nicer way to do this that didn't cause any trouble in the long run. Sadly, a look to uncoming Drupal 4.7's code reveals that this is not going to change soon, but better late than never.

This is a critical need for me as well. I am implementing a corporate site and all user logins must be authenticated against a RADIUS server. Users will not be logging in with an @server, just a username, and I need to intercept all logins.

I will use the z trick like the LDAP module, but that is a a real hack.

replacing the login callback is a good solution IMO. nice one.

That's not what Moshe and I mean. We speak about a hack I implement in my ldap_integration module which makes use of undocumented stuff in order to achieve the desired effect. Have a look at the module code. The relevant parts are the short file zcallbacks.module and the last section of ldap_integration.module .

The problem is: it's still a hack and may cause problems in the future, so I'm still all for changing it.

Further to this topic, Drupal 4.7 has brought another way to work this issue around.

The login process can be changed by altering the login forms through the new forms API. A quick example would be like this:

function mymodule_form_alter($form_id, &$form) {
  if (isset($form['#validate']['user_login_validate'])) {
    $form['#validate'] = array('mymodule_login_validate' => array());
  }
}

It is not yet the proper thing we are looking for, yet it is nicer than the last method I proposed (i.e.: taking over the user_login callback).

And yes, I (or somebody else) should study the problem thoroughly and provide a proper solution to propose to the Drupal team so it makes it to the official core.

For my module I am really in need of this too.

I have been pondering a lot about how to get by this. I see on option, and that is an extra argument to hook_auth. If the flag $server is set to false, then the hook needs to run for failed logins too. Else it will contain the server part.

some pseudo code

  // Strip name and server from ID: 
   if ($server = strrchr($name, '@')) { 
     $name = substr($name, 0, strlen($name) - strlen($server)); 
     $server = substr($server, 1); 
   } 
 
   // When possible, determine corresponding external auth source. Invoke 
   // source, and log in user if successful: 
   if (!$user->uid && $result = user_get_authmaps("$name@$server")) { 
     if (module_invoke(key($result), 'auth', $name, $pass, $server)) { 
       $user = user_external_load("$name@$server"); 
       watchdog('user', t('External load by %user using module %module.', array('%user' => theme('placeholder', $name .'@'. $server), '%module' => theme('placeholder', key($result))))); 
     } 
     else { 
       $error = t('Invalid password for %s.', array('%s' => theme('placeholder', $name .'@'. $server))); 
     } 
   }
//and in the drupal_auth it should look like: 
  function drupal_auth($username, $password, $server) { 
   if ($server && variable_get('drupal_authentication_service', 0)) {
     $result = xmlrpc("http://$server/xmlrpc.php", 'drupal.login', $username, $password); 
     if ($result === FALSE) { 
       drupal_set_message(t('Error %code : %message', array('%code' => theme('placeholder', xmlrpc_errno()), '%message' =>  theme('placeholder', xmlrpc_error_msg()))), 'error'); 
     } 
     else { 
       return $result; 
     } 
   } 
 } 

// but in the mysql_auth_auth like:
function mysql_auth_auth($username, $password, $server) {
    $result = mysql_auth_query($username, $password);
    if ($result === FALSE) {
      drupal_set_message(t('Mysql authentication failed for %user@%server', array('%user' =>  theme('placeholder', $username), '%server' =>  theme('placeholder', $server))), 'error');
      return FALSE;
    }
    else {
      return TRUE;
    }
}

@ber - that sounds reasonable to me.

I was also thinking about this (I'm working on an alternative phpBB module, where I need to use the phpBB user database).

I was about to propose a similar thing, but in the else part of the code:

  // Try each external authentication source in series. Register user if
  // successful.
//  else if (!$user->uid && $server) {  // <--- Removing the check for $server here
  else if (!$user->uid) {
    foreach (module_list() as $module) {
      if (module_hook($module, 'auth')) {
        if (module_invoke($module, 'auth', $name, $pass, $server)) {
          if (variable_get('user_register', 1) == 1) {
            $account = user_load(array('name' => "$name@$server"));
            if (!$account->uid) { // Register this new user.
              $user = user_save('', array('name' => "$name@$server", 'pass' => user_password(), 'init' => "$name@$server", 'status' => 1, "authname_$module" => "$name@$server"));
              watchdog('user', t('New external user: %user using module %module.', array('%user' => theme('placeholder', $name .'@'. $server), '%module' => theme('placeholder', $module))), WATCHDOG_NOTICE, l(t('edit'), 'user/'. $user->uid .'/edit'));
              break;
            }
          }
        }
      }
    }
  }

wow. that is a hell of a lot of nested ifs there :)
you probably want to look at module_invoke_all, or else do something in the lines of

foreach (module_implements('auth') as $module) {
  $return = module_invoke($module, 'auth'));
  if (isset($return)) {
    if ($return) {
      $foo = TRUE;
      break;
    }
    if (!$return) {
      $foo = FALSE;
    }
  }
}

It will break out as soon as there is a hit. while your code will still call all the rest of the modules.

PS, markus, the mysqlauth can do the atuthentication for you, you could decide to use that instead of copy-pasting it. After all the Drupal Way is to NRY (never repeat yourself) wherever possible.

lol, that code is from user.module user_authenticate function, which is something that could probably be simplified as this:

function user_authenticate($name, $pass) {
  global $user;

  // Try to log in the user locally. Don't set $user unless successful.
  if ($account = user_load(array('name' => $name, 'pass' => $pass, 'status' => 1))) {
    $user = $account;
    return $user;
  }

  // Strip name and server from ID:
  if ($server = strrchr($name, '@')) {
    $name = substr($name, 0, strlen($name) - strlen($server));
    $server = substr($server, 1);
  }

  // When possible, determine corresponding external auth source. Invoke
  // source, and log in user if successful:
  if ($server && ($result = user_get_authmaps("$name@$server"))) {
    if (module_invoke(key($result), 'auth', $name, $pass, $server)) {
      $user = user_external_load("$name@$server");
      watchdog('user', t('External load by %user using module %module.', array('%user' => theme('placeholder', $name .'@'. $server), '%module' => theme('placeholder', key($result)))));
    }
    else {
      $error = t('Invalid password for %s.', array('%s' => theme('placeholder', $name .'@'. $server)));
    }
  }

  // Try each external authentication source in series. Register user if
  // successful.
  else {
    foreach (module_implements('auth') as $module) {
      if (module_invoke($module, 'auth', $name, $pass, $server)) {
        if (variable_get('user_register', 1) == 1) {
          if ($server) {
            $name .= '@'. $server;
          }
          $account = user_load(array('name' => $name));
          if (!$account->uid) { // Register this new user.
            $userinfo = array('name' => $name, 'pass' => user_password(), 'init' => $name, 'status' => 1);
            if ($server) {
              $userinfo["authname_$module"] = $name;
            }
            $user = user_save('', $userinfo);
            watchdog('user', t('New external user: %user using module %module.', array('%user' => theme('placeholder', $name), '%module' => theme('placeholder', $module))), WATCHDOG_NOTICE, l(t('edit'), 'user/'. $user->uid .'/edit'));
            break;
          }
        }
      }
    }
  }
  return $user;
}

Note that $server is optional in the else part of the big if.

I'll be sure to check the mysqlauth module, thanks :-)

So is there any chance to allow a contrib module to authenticate local users in 4.7?

I mean an authentification method that doesn't need to append @server to the user name. This is what the code I posted above would allow.

If you'd post a patch it could be reviewed. :p

ok, here we go. First attempt :-)

Please, review attached patch. It is not exactly equal to the code I posted above. It has the following, IMO, benefits:

1) Simplied code, returning $user is user_load succeeds. No need to check for $user-uid later on.
2) Opimized foreach loop when checking for modules that implement hook_auth().
3) Check for variable_get('user_register', 1) has been removed, because IMO this option is for 'normal' user registration, authentication modules may still allow this kind of registrations.
4) When authmap is not used, the server part of the user name is optional. So the implementation of hook_auth can decide what to do.

Can someone please confirm? This is hardly need to connect with local systems, such as phpBB (me works on it) and any other local "user provider".

i tested the patch and it works well. Also cleans up some verbose code.

However, when one has a DA module enabled and fails local login, then you can get errors from your DA module when it tries to connnect without any server being pased. note the url that is being called in the call below.

"_xmlrpc('http:///xmlrpc.php', 'drupal.login', 'moshe weitzman', '
')"

So I think we need some more defensiveness in all our DA moduls to handle being called without a server?

Also, please remove the quotes from the line below. I know that they were there before but they shouldn't be

$account = user_load(array('name' => "$name"));

Also, please remove the quotes from the line below. I know that they were there before but they shouldn't be

$account = user_load(array('name' => "$name"));

Fixed.

However, when one has a DA module enabled and fails local login, then you can get errors from your DA module when it tries to connnect without any server being pased. note the url that is being called in the call below.

"_xmlrpc('http:///xmlrpc.php', 'drupal.login', 'moshe weitzman', '')"

So I think we need some more defensiveness in all our DA moduls to handle being called without a server?

The attached patch also modifies drupal.module::hook_auth() with a possible way to deal with this. It makes the $server argument optional, defaulting to FALSE (the return value of strrchr(), when $server is set in user_authenticate, if the server part is not present).

Sorry, forgot to change the issue status.

thanks for quickly addressing those details. it is RTBC now, but you gave me an idea. instead of $server = FALSE, maybe drupal.module should add a setting such that logins would automatically go to another drupal site for authentication. in other words, let drupal.module take advantage of the feature we are enabling here. users shouldn't have to type in a server for drupal.module either.

sorry for the feature creep. makes sense though. will help implement networks of sites within an enterprise, for example.

Do you mean a new textfield in the drupal settings panel, so admins can specify the _default_ server where they wish to accept DA?

That makes me think that the options under the "Receive data from other Drupal sites" fieldset may need a bit more love. I mean, there is actually no way to restrict from which sites one wishes to "receive data". This is a blank or white situation.

What about commiting the patch as it is and open a different issue to think about that? That would deal with drupal.module's ability to specify a _default_ server and/or options to possibly restrict those features to a white list of servers?

OK, lets agree that this default drupal DA server will be a future feature. RTBC

i have a use case for the 'default drupal DA server'. the upcoming http://groups.drupal.org site wants a login box that automatically sends login info to drupal.org site without user having to type in @drupal.org in username.

I tested the patch in #32. And it works as advertised. It breaks no current implmentations, so far as I can see and can test (drupal dist auth works).

It will also make SQL auth module a lot easier. http://drupal.org/node/50946

A +1 for the patch

Changing status, I'll try to implement moshe's suggestion (a simple textfield in the settings panel to propose a default server seems easily doable :-)... unless someone else feels otherwise...

Here we go. The only difference in this patch is in changes made to drupal.module. A new option to specify the default authentication server (is the description correct?) and then, how it used in hook_auth.

Is it good enough?

Optimized logic in hook_auth. better now?

moshe, does the patch cover what you meant with groups.drupal.org ?

1) I have just noticed something that needs attention. It is this line:

$error = t('Invalid password for %s.', array('%s' => theme('placeholder', $name .'@'. $server)));

Nice, isn't it? However, a) $error is not used! b) If the password fails, the user is already warned and a record inserted to watchdog. So...

Could we remove that error message?

2) moshe: Now that we have a default server... does it worth to add an option so admins can say "allow DA, but only with this server", or something of that sort.

3) Is this something to include in 4.7? If so, please mark this as critical.

1: sure, remove the message

2: yes, good idea. doesn't have to hold up the patch though.

3: i emailed killes to see if he will take this for 4.7.

Sweet! Here we go... though, please review the latest changes.

I talked to Steven about it and he thinks it's a 4.8 feature.

Do you mean those changes in drupal.module or the whole thing?

PS: It would have been nicer to speak up before, so one do not spend their time trying to maintain a patch that is not going to be applied soon, it will be broken again and again ;)

markus - drumm will apply this patch as soon as HEAD opens back up for 4.8. it should not "keep on breaking". i think killes and steven are calling current behavior "not a bug" which is a reasonable conclusion. also, there are OK workarounds posted in this thread. thanks much for your persistence here. this will get in soon, and we'll all rejoice.

The settings page needs some work. The "Only default server" option is hard to understand. I think adding these settings justifies putting all three settings related to distributed authentication in their own group.

2 out of 3 hunks failed. needs refresh

thanks for the persistance. i put this patch through some vigorous testing and it looks good to me.

reroll so it applies cleanly and two minor text improvements

I don't understand the help text.

t('Only accept remote logins from the above specified default authentication server and not from any other server.')

I think we should be more verbose. Why would you want to set that setting? What if I want to accept authentication from a number of trusted partners?

In the documentation, the fully-qualified domain name examples are not fully-qualified. They are missing 'http://'.

improved the help texts as suggested. i am leaving the "multiple trusted partners" as a feature request. in my experience, the main use case is that the site owner has some external database/directory that is authoritative. this patch let's drupal site act as slave to that server and disallow all other authentication sources.

The chunk with

+ if (!isset($array['created'])) { // Allow 'created' to be set by hook_auth

also scares me.

new patch without debug code. sorry.

@drumm - no need for fear. the form api protects us from tampering like that. if you look back, user_save() had a lot of checks to prevent tampering like that but they were all removed with fapi. the calling function can already set status and password, for example. and we fear not because fapi will strip those fields if user attempts to post them.

Committed to HEAD.

Remember- asking for commit ASAP doesn't help, but more reviews do.

more review? there aer 62 followups, how much reviews should something get?

it appears this patch causes a small bug (unless i applied it wrong):
http://drupal.org/node/73760

I'm trying to patch the user module on Windows. I downloaded the Unix utils from here:

http://unxutils.sourceforge.net/

then copied patch.exe, user.module and patch_47 in the sh.exe folder. Then I run this into sh.exe:

patch -p0

But I get this:

Assertion failed: hunk, file patch.c, line 321

Some problem with patch.exe probably? Any suggestions what to do? My Drupal version is 4.7.2

Then I run this into sh.exe:

patch -p0 <patch_47 user.module

But I get this:

Assertion failed: hunk, file patch.c, line 321

Some problem with patch.exe probably? Any suggestions what to do? My Drupal version is 4.7.2.

Please don't change the issue title to something that is unrelated.

@darkblue, EOL encoding, look at http://drupal.org/node/60116

I was directed to this node by the README install instructions for the FUDforum module. They say to "Apply patch at http://drupal.org/node/29147 (look for #59 patch_47)".

Two questions...

1) I'm using the latest Drupal (v 4.7.3). Has this patch already been worked into it?

2) If not, how does one implement the patch? Is it a matter of replacing all the code in "/modules/drupal.module" with the patch code? Or just certain parts?

Thanks in advance for any help!

Thanks for the response, Moshe. I'm sure you didn't mean it that way, but saying "The patch is meant for developers that know how to do this kind of stuff" seems like a bit of a brush-off. Despite how it may seem, I am very proficient with PHP code and with installing Drupal modules. As you can see I have the FUDforum installed already ( http://www.wesleycrushers.com/forum ) and would like to work it into my Drupal 4.7.3 site. Not asking for hand-holding; I'm just a bit unclear on the particulars of how to make the changes that your module entails, and a bit more documentation would be very helpful. Please?

I didn't mean it any which way because I didn't write that. naudefj wrote the last followup. see the usernames next to each post.

The feature request called for here has been delivered. If anyone has support needs, please ask in the forum. do not reopen this issue for support questions.

Sorry Moshe, I misunderstood this to mean you and naudefj were one and the same...

naudefj - August 18, 2005 - 15:46
Project:	Drupal
Version:	4.7.3
Component:	user.module
Category:	feature
Priority:	normal
Assigned:	moshe weitzman

Thanks for the feedback, I'll check the forums. In the meantime, maybe you could post any good threads that address my "newbie" question? I'm sure I'm not the only one wondering about this, and could be a good idea in the spirit of open-source and sharing and all... Thanks again!

Did This actually get included in drupal 5?

I am trying to use the hook_auth function but it seems that is is not being called when a user tries to login....

Thanks

But the hook_auth function is called when I try to login using a username ending in "@domain.com"

Hi. I'm a newbie in Drupal. I use Drupal 5.1 and I checked both drupal.module and user.module, it seems patch mentioned in #59 have been implemented, however when I use hook_auth, it seems it is invoked only when I entered user@domain, otherwise if I entered user without domain, it is never being called. Did I miss something here? Thanks in advanced for the help.

I found an interesting posting about behavior of hook_auth in Drupal 5.x
Simply said, hook_auth will be called when you do not use @domain in username as long as user IDs are not in the Drupal database already
Read this comment http://drupal.org/node/125626#comment-210855

there is a pref in drupal.module which is a 'default authentication domain' or somesuch. when that is set, you can skip the @example.com part.