Harden user login - move user_load() to submit handler

Me want, me want.

- + $form_state['user_authenticated'] = 1; When I first read that it wasn't clear that 1 was the user ID. The variable name wasn't self-explanatory. Maybe that variable should be called 'uid'?

- + drupal_set_message(t("!username,\n\nYour account on !site has been blocked.", array('!username' => $account->name, '!site' => variable_get('site_name', 'drupal')))); Shouldn't that be an error message instead?

- Missing quote in + * should set $form_state['user_authenticated] to a user ID.

- Either way, the global $user object is populated based on $name. That documentation was already there but it seems outdated.

- I recommend everyone to look closely at the OpenID.module chunk because it shows how we expect other modules to authenticate users. It is a good example of our outward facing API, and we should be proud to show that to the rest of the world.

Thanks Moshe.

Given that anonymous users don't have an ID, the word _authenticated_ seems redundant in user_authenticated_uid. I'm not sure I understand the name space issue? Would there be a case where we had a single user object with two different user IDs? Care to elaborate on your concern?

Committed this to CVS HEAD. Does this require a documentation update in the upgrade instructions? It might affect certain modules so maybe a brief mention is in order.

This isn't complete. It needs to be documented on
http://drupal.org/update/modules/6/7
because the user_external_login function went away.

See also
#765216: Documentation problem with user_external_login
which I've closed as a duplicate of this issue.

Also it would be lovely if someone who knows what's going on here would add a comment to
http://api.drupal.org/api/function/user_external_login/6
either explaining what to do in D7 or pointing to the new section they write on the module update page. Thanks!

Here's another problem that may or may not have to do with this:
#765222: user_external_login_register does not look right

I just marked that other issue as a duplicate of this one...

I apologize for being off-topic, but shouldn't user_external_load($authname) take an additional parameter, $module? Am I missing something (again)?

Maybe off topic again... I also think it is a mistake that user_external_login_register now takes a username, where in 6.x it took an authname. I strongly suspect that change is an accident. But if on purpose, that should also be documented.

I mentioned this in #765222: user_external_login_register does not look right, which has been marked a duplicate of this issue.

#20: I've reported this as a bug: #817118: Remove {authmap} and migrate OpenID entries to their own table and provided a patch which remains backwards compatible.

#21: I agree that the way it works now doesn't really make sense. This patch looks up the username from the authmap table using user_external_load. When #817118: Remove {authmap} and migrate OpenID entries to their own table is accepted, the module should be added as a parameter to user_external_load here as well.

Has anyone updated the module update page yet?

And I'm confused about the patch here -- what problem is it fixing?

I'm not sure either what #22 is trying to fix.

There is absolutely no change between D6 and D7 as far as user_external_login_register() is concerned.

Right. But I think the patch committed above removed http://api.drupal.org/api/function/user_external_login/6 (it doesn't appear in D7), and this is not mentioned in the update guide page anywhere I can see http://drupal.org/update/modules/6/7.

It would also be helpful to have a short comment on the D6 function page that says "This is not in D7. Use xyz instead".

And if I knew what "xyz" was, I would go ahead and write the upgrade doc and the comment, but...

I'll try to clarify the problem as I (and Dave Cohen in #21) see it:

When an external auth module tries to log in a user for the first time, a new user is created with the name given to user_external_login_register in $name.
The connection between the username from the external auth module and the drupal user is stored in authmaps using user_set_authmaps.
The user (or some administrator) could change the username of this user.
When the external auth module tries to log in the user again and the username was changed, user_external_login_register tries to load the user based on the username. The username is not found, as it was changed previously, so a new user will be created, instead of logging in the existing user.

By loading the user based on the information from the authmaps table a user who changed his username will still be able to log in to the same account.

#22: 497612-use-name-from-authmap.patch queued for re-testing.

@Stevel: user_external_login_register() should probably go away. See the OpenID module for how to properly login an external user.

Just so I know I have this right....

// D6
user_external_login($account);

...becomes...

// D7
user_login_submit(array(), array('uid' => $account->uid));
drupal_goto();

Is that right?

@Damien: Even if the function gets removed in the future, that's no excuse for having the current situation. Either there needs to be some documentation that the function doesn't work properly when usernames are changed, or the function needs to be fixed so it does allow this.

So it seems to me that the patch in #22 is fixing a real problem (see explanation in #29 and #34). Is there a better way to fix this problem?

The only alternative I can think of is to call the function with the current username (by looking it up based on the user id which is found in the authmap table) if the user already exists, or the external authname if the user doesn't exist yet.

I don't believe this is be a better solution as it requires extra code everywhere the function is called, and thus is prone for errors.

As no other alternatives are given, I'm marking this issue as needs review again for the patch in #22.

#22 looks fine to me too -- committed to CVS HEAD. Thanks.

This still needs to go into http://drupal.org/update/modules/6/7.

(I came across this due to the renaming of user_authenticate_finalize to user_login_finalize).

fixing tag according to #40 (needs doc on module update page)

So, looking at the patch, it looks like what still needs to be documented in the module update guide is:

a) user_external_login() went away (see #32 for d6/7 code sample)

b) args of user_authenticate() changed

c) Name of user_authenticate_finalize changed:

-function user_authenticate_finalize(&$edit) {
+function user_login_finalize(&$edit = array()) {

The doc is here:
http://drupal.org/update/modules/6/7#login-funcs