Refactor distributed auth out of user.module

James' tweaks to user_external_login() are just right. I tested this with my HEAD port of Persistent Login (not yet committed) and it works great. I am currently unable to get OpenID to work (see the issue) so I cannot test it with that.

I think this patch is fine but I wrote it originally so assume I can't RTBC it.

One minor glitch, probably my own fault. When I try to use PL on a blocked account, user_external_login() properly returns FALSE but the page comes back without the error messages set by user_login_validate(). Where are they going? I'm too tired to debug this at the moment.

Nice. webserver_auth needs this too.

- "logs the login". how about "performs a login"
- do we really need the password paranoia. i think external login modules should be able to set the password if they want. i think that this is legacy cruft from before fapi.

The unset($user->pass) isn't for paranoia, it is because user_login_validate() will blow away $user via user_authenticate() if it is set, and this function is specifically for when user_authenticate() should not be called because the user is already authenticated.

I actually changed the patch last night to remove $user->pass for _validate() but then put it back for user_login_submit() (it turns out Persistent Login used to use $user->pass as part of the random tokens it generates). I abandoned the change but could easily put it back. You think I should? In favor is that $user->pass has always been defined in hook_user('login') before. Opposed is that it is entirely possible for a site to be configured *always* to use user_external_login() and not ever assign a password to user at all.

i don't know the right answer here, but maybe you smart guys can make a decision so we can RTBC this thing ...

@barry - yes, always using external auth is a common use case - thats what webserver_auth.module is, for example.

I looked into this and the openID patch. This one is RTBC. I just chnaged 2 lines elsewhere in user.module that were generating NOTICE errors.

Can't we reuse this function for the external authentication stuff in core itself (or the DA in core)?

we could, but i was under the assumption that all the DA code is being thrown to Contrib after OpenID gets in. if thats wrong, i'll expand this patch.

... the DA code is being thrown to Contrib after OpenID gets in.

I'd support such a change but it could be disruptive for many. There is no patch for it yet, and even if there was, it remains to be discussed. Therefore, I'd prefer to leave the code in a clean state before we move on to discuss some of that. So yes, re-using this in core would be appreciated.

Dries last comment took a lot of digging to implement right. I ended doing a refactoring which ideally would have happenned during the fapi conversion.

- i split user_authenticate code out of user_login_validate() and into its own fapi validate handler. this is great, because openid, persistent login, and DA modules can just form_alter() it out of the way as needed. thats what drupal.module does here.
- i took all the DA code out of user.module! with fapi, DA did not need to be hard coded into user_authenticate(). modules can form_alter() themselves in. user_authenticate() is far simpler now. we are also in a very good position to split up drupal.module and move some/all to Contrib if desired.
- i fixed some random notices

Note: OpenID still needs some tweaking I think. I will work that in the next 48 hours.

dyslexia klils.

I was mistaken - OpenID is working fine with this patch. I've now further cleaned up some help texts and such. Lets get some reviewers here and then commit this so OpenID can move along. This is a great cleanup.

I've looked at this and it looks good. This is clearly a step in the right direction.

The only thing that cost me some braincells are the various validate handlers: user_login_validate_final vs user_authenticate_validate vs drupal_distributed_validate. That part of the code lacks some documentation. It would be good if we could document this in the code comments.

Maybe we should rename drupal_authenticate_validate to drupal_local_validate? Not sure.

I added docs to various user.mdule functions and to the spt where we define validators for login form. I think we make this confusing by the DA module as drupal.module and thus all functions are prefixed by a well known name - drupal. Since this is likely moving to Contrib soon, I'm not too worried. The module is very self contained now and its maintainers can rename as they see fit.

I tested all all is OK, therefore i promote to RTBC.

I'd second Dries' comments, and would also point out that the function comments are not in the doxygen form. This should be fixed too.

Oh, I intervened with my comment. Let's get somebody else to test this too, as it is a pretty extensive change, before going to RTBC. The code comments still need work as I pointed out. There are even indentation issues like:

+    $items['drupal/help'] = array(
+        'title' => t('External login tips'),
+        'page callback' => 'drupal_page_help',
+        'type' => MENU_CALLBACK,

Even more comments, in doxy format. I fixed that indentation as well.

Looks like there are some typos in the PHPdoc.

What I was really asking for was a more high-level description explaining why we use a set of 3 validate functions instead of 1 validate function.

Not all functions have proper PHPdoc comments. Example:

+// Try to log in the user locally. Don't set $user unless successful.
+function user_authenticate($name, $pass) {

I submitted an essay about the 3 validation functions and why thats better than 1 :)

I al so went through and assured we use doxy for all.

Just to hop on the comment nit-picking bandwagon:
- as an accronym "fapi" should probably be capitalized.
- i thought comments were supposed to be wrapped at 80 characters...

There's something wonky either in core or related to this patch but I haven't had a chance to figure out which.

I applied the patch and started poking around. I fired up IE, logged in as the admin, logged out, tried logging in with a bogus password. As uid=0 created a new account, the email didn't show up (probably the fault of my mail host) so I logged back in as uid=1 and set the password for the user. To save typing in the hostname I copied the URL to the clipboard (http://example.com/?q=user/2/edit). With FireFox (which had never logged into the site) I pasted in the URL and got the edit user form with the login block displayed at the same time (see the attached image). Even worse the user edit form was editable, so of course I pointed it to user/1/edit and changed the admin password. Back in IE, I logged out and back in as uid=1 with the new password. Ooops. Not sure if the fault is totally this patch but somethign's wrong.

humm, that image didn't attach.

Okay, reverting the patch makes it clear that this is probably a separate issue. Since the various local login tasks seemed to work correctly I'm going to put the status back and go look for an issue related to the missing permissions on the user edit page.

The patch breaks user_external_login() (which is ironic) because user_login_validate() no longer exists but user_external_login() still calls it. I've attached a new version that fixes this problem. Persistent Login works with this version.

I'm a bit concerned about complexity. There are now three user login validate handlers. user_external_login() today wants to perform the behavior of user_login_name_validate(). At some point, important functionality might/will be added to some other validate handler and user_external_login() might not be updated.

Regarding drupal.module, I'm not a huge fan of having to splice entries into the #validate array, but maybe it is the right approach, and anyway I've never really looked into drupal.module (though I do think it should be renamed). One comment: can't

     $key = array_search('user_login_authenticate_validate', $form['#validate']);
      unset($form['validate'][$key]);
      array_splice($form['#validate'], 1, 0, array('drupal_distributed_validate'));

be replaced with

      $key = array_search('user_login_authenticate_validate', $form['#validate']);
      $form['validate'][$key] = 'drupal_distributed_validate';

?

I implemented the suggestions of drewish and bjaspan and retested. we are RTBC again.

I've made some minor documentation changes to the patch, and committed it to CVS HEAD. Thanks all!

Now back to the openid.module ...

user_external_login() is actually not right in this patch, for two reasons:

1. Bug: It passes an empty $state array to user_login_name_validate() and _submit(). This means, for example, that user_login_name_validate() does not actually validate the name b/c it has no name to validate. I fixed this in an earlier patch on this issue but my change got lost somehow.

2. Design flaw: It does not allow form values to be provided. This means form values cannot be passed to hook_user('login'). Consider a site with OpenID and Persistent Login enabled. When the user provides an openid and checks Remember Me, the Remember Me checkbox value needs to be passed to hook_user('login') for the Persistent Login module. So, the OpenID module needs to remember the form values as submitted and pass them to user_external_login() once the OpenID verification is complete. But it can't, because user_external_login() does not provide that capability.

This patch fixes both problems. Once it is committed, openid.module will need to be modified to preserve the user login form values and pass them along.

Ooops, forgot to update the category and status.

seems like we should include the openid fix here too, so we don't swap one bug for another?

Okay. Here is a combined patch:

1. Fixes user_external_login() to support a $edit argument.

2. Fixes openid.module to use the $edit argument to user_external_login().

3. Fixes an unrelated one-line bug in openid.module (it loops forever on Windows every time it used). This bug means openid.module completely fails to work on Windows and hoses the server until the PHP script timeout every time it is used.

At walkah's request, I have re-filed this as a new issue: http://drupal.org/node/153372.

FYI, I have posted upgrade docs and updated api.module docs.