User login/authentication tasks not performed consistently

I'll try to take a look at this next week. I agree that it is a critical bug report (i.e. needs fixing before release).

Is a bit hard to parse what is going on here. Hopefully someone else is available to review this.

How do I test this?

u huh

since this patch is non trivial, we should have a detailed positive review before RTBC. please state what you tested in the patch, and how that is better than current drupal.

i sometimes rtbc patches without much comment also, but i do that on very trivial patches.

code looks sane.

i tested login and registartion with openID and without. all works well.

i got one NOTICE as follows. That can be fixed later if needed. This patch is RTBC anyway.
notice: Undefined index: values in /Users/mw/htd/pr3/modules/user/user.module on line 1293

Erm, why should user_login_authenticate_validate() call user_authenticate() twice?

good catch. rerolled without the first line

Can't user_authenticate() just take a $form_values instead of ($name, $pass, $form_values = array()) or is user_authenticate() used in situations that does not involve a form? If so, I think it's a big ugly to pass in the $form_values() in user_authenticate().

Moving to user_authenticate($form_values) might make sense as we're moving towards more FAPI goodness and programmable submitted forms.

What do the others think of that?

user_authenticate($form_values) is fine with me and John' suggestion to require a 'pass' element makes sense.

Yes, I'm also OK with the proposed solution.

I've been running up against this bug as well, and I noticed that this extends not only to the login functionality, but also the "logout" functionality as well... can anyone else verify that?

Reroll against HEAD. Changed and to && as we use that.

another important note: do not credit me when committing this, I just rerolled.

I tested with login, logout, register. did not test with openid.

Patch needs reroll to work, it seems that poll.module has been altered since. User creation works, watchdog entries are created. however when i try to create a poll (some poll code has been altered in the patch) i get this:

    * notice: Undefined property: stdClass::$date in /home/balu/projects/drupal/HEAD/modules/node/node.pages.inc on line 170.
    * notice: Undefined property: stdClass::$created in /home/balu/projects/drupal/HEAD/modules/node/node.pages.inc on line 191.

Preview and viewing the poll generates a lot of errors too. Rerolled patch in attachment.

poll changes are totally astray they slipped in from another issue. sorry.

/me gets the feeling that was the wrong patch.

Moment, I'll do a reroll...

OK, here's my take on the poll-less reroll.

I have tested it, login (logout) works, log entries are created. Central login (user@drupal.org) works as well as login via block. The only thing that failed to work with me was the OpenID login, but I tried without the patch too, and it fails the same way.

Taking over.

Patch no longer applies, rerolling.

Rerolled and fixed a bit of the docs.

I corrected a syntax error on line 67 of the patch. Tested, works. Session entry appears in the logs, NOTE: session open does show up when logging in via openid.

openid can be a supplemental patch this one works and does what it should.

Reading the conversation I realize it's the best moment to suggest another change in the login flow..

Just to notice, user_authenticate DOES load the user in the current session, and it's being done in the validation part of the form, the hook_validate. This can be wrong, and should be done in the hook_submit instead (actually it's being done in both hooks.) The problem running user_authenticate in the validation part is that if you want to add another field to the login form using form_alter, even if that attach a new validation function to the form or any of the form fields, the validation has already loaded the user in the current session.

It's being an issue for the captcha and other modules doing operations to the login form.

May I suggest to just simply check if username and password are valid in the validation hook, and perform the authentication in the submit hook? it makes more sense this way.. and will allow other modules to attach validators in the login form.

PD: Comming from here.. http://drupal.org/node/177623

I have a few smaller style issues after looking at the code:

- Not enough whitespace before the @return docs at user_authenticate()
- Why do we recreate a very similar array in user_authenticate(), if we can modify and pass on the array?
- Do we document void return values?
- Where does $merge_data come from? (I did not have the time to review patched code, and it looks suspicious).

I tested this patch hand it works as advertised. Code looks good.

Addressing Gabor's style issues:
* I removed the extra whitespace
* We are recreating that array because $form_values might have other elements in it and user_load() actuially uses every element thats passed in. We have to pass this subset and only this subset.
* I removed the void return docs. It might be a good idea to start doing this in the future, though.
* $merge_data contains some registration info which has been massaged or provided from default values. I think that could be refactored out, but it will require reworking the registration process which is not in scope for this issue. I see no danger here.

I am seeing a NOTICE when using a one time login link but thats not related to this patch. I will file separate issue.

subscribe

Back to anon

rerolled: drupal.module is gone; also fixed a notice: "Undefined index: values in /.../modules/user/user.pages.inc on line 103." (I just removed $form_state['values'] as it's not set - not sure if that's right).

JohnAlbin: from the page you linked:

Note: Although = has a lower precedence than most other operators, PHP will still allow expressions similar to the following: if (!$a = foo()), in which case the return value of foo() is put into $a.

so I guess the if statement should be ok. I don't know about user_external_login() - although the phpdoc does say, "The user object must already be authenticated.".

In my (humble) opinion, we are still in the same concept error!

I Know there's no need to remember the workflow of the forms API, and the order the hooks should be fired, and In the user.module of any of the drupal versions, the hook_validate of the login form IS LOADING THE USER into the global $user variable, NOT just checking the input data entered, and the hook_submit of the form is used only to redirect the user. This is breaking the feature of attach validators to the login form, because even if a validator returns FALSE, the user.module validator is loading the user with the user_authenticate API call.

I think it's a good idea to fix this within the scope of this post, before closing the bug issue.

@ilo - you are right that this needs fixing.

the bigger problem is that this issue has shown that user login is a mess right now. i take partial responsibility for that. if noone else does so, i will try to spend some time with this issue. i think it requires actually flow charting the different entry points and then trying to simplify and make consistent.

I don't know if this may help, but what I've learn from you (all) and this issue is that login form, user authentication, and user creation flows (old drupal.module) are mixed.

Oh, by the way, once finished with this issue, a "drupal_community_invoke('review your modules')" should be fired.. I've seen several contrib modules regarding or implementing their own login flow.. The first example comming to my mind now is ldap authentication. What if we request help from these contributors to find why current implementation didn't work for them? (this request is ugly because will slow down too much the solving of this problem, but my clear why all the code is so messy).

I think I could help to you with this flows, but I'm affraid: I think I need more time to setup a testing site than the time the whole solving may take to you. I any case, if I can help please ask for that! it's too much work for a single person.

You can help. Please do list/describe the functions and flows if possible.

Ok, I'm on it. I'll start from scratch with current status in version 6.Beta2 (user.module,v 1.853), discarding the changes you may apply from now on.

Guys, this issue is part of the small list of issues holding up Drupal 6, but there was no activity for *3 weeks!*. What can we do to help you push this forward? We are trying to work towards a release soon, but critical user login issues lingering around for weeks is not something which helps... Thanks for all your efforts!

I'm currently finishing the login flow maps.. it's really hard to find all critic flow entry points.. Some modules break completely the login flow what is really bad.

I was trying to put here some of the findings, but my time is slow due to overtime work, sorry. I'll be focused on this, but I think (my part) it's not being done (including fixing) on friday..

I do not know if this helps, but I am having an issue with logout failing.

Drupal 5.3 - fully upgraded, Garland theme

I have been able to reproduce the issue consistently - looks like this.

Log on
Navigate the site
Use breadcrumb to return to home page
Log out

The log out fails - but in a partial way

If you visit any page other than the home page, system recognizes you are not logged on, but on home page it thinks you are logged on.

To get out of this is difficult:

Force a new log screen by going to http://example/user

Edit a block configuration, and save changes

Logout from any page except the home page - lets you get a clean log out

Hope this helps - any explanation for this issue?

Any interest in making progress with this? We are going ahead with Drupal 6, and blocking on issues which people are not interested in working on is no fun.

Past beta4 changing an API when Drupal as it is works? It's clumsy but I can't see this happening.

Thanks for taking this on! What about moving all this to a function, and call that from each place? Seems like that function would require an $edit and an $account parameter, and would be only a few lines.

The $account parameter would be superflous.

To me this looks all nice (ie. good phpdoc comments, duplicate code removed, $account really not required, as user_authenticate() updates the global $user). Anyone else (more familiar with the issue) can confirm?

Well, sat down with this,

- applied latest patch,
- fixed the parse errors,
- fixed the blogapi case being called after the function returned anyway
- removed the login time set code from the one time login code as we do this in user_login_finalize()

Then went on to test:

- tried simple login
- tried one time login link
- tried blogapi login

All worked and triggered user_login_finalize() properly. *Then* went through the comments above and did some thinking whether some API change would be better still then relying on arbitrary contrib module maintainers calling the finalize tasks (the session id regeneration being most important). After all, I think an API change is still fine now in the bold name of security (and Dries also suggested this exact API change, although it was long ago) so went back to previous patches and got through all the improvements from there. Looked around and understood what $merge_data was supposed to be and also fixed a user_authenticate() call in install.php

Then went ahead and tested all the stuff as done before, and all still worked. So finally committed the attached patch. I hope this will strengthen login security for Drupal 6. Thanks to all involved!

Also documented the update requirement at http://drupal.org/node/114774#user_authenticate

Bravo, Gabor. Thanks so much for finishing this off so nicely.

This patch broke user_external_login() because that function uses user_login_submit() to do what is now performed in user_authenticate_finalize(). As a result, even though user_exernal_login() is documented to invoke hook_user('login'), etc., it doesn't. This change breaks persistent_login.module and possibly openid.module as well (I haven't checked).

The attached patch changes user_external_login() to use user_authenticate_finalize(). It seems to work and AFAICT it should work. I'm posting this bug here instead of in a new issue, however, because the API is getting rather subtle and I want those who were involved here to review this for correctness.

FYI, any patch changing the user authentication APIs should be tested against openid.module and persistent_login.module because they do user auth in non-typical ways.

Hm, good catch. It would need some testing definitely.

I suspect that http://drupal.org/node/165642 is part of this, even though it is reported in 5.x.

Even though the drupal module has been moved out of core, there will be many who are still going to use it. The important thing to test here is FIRST login from a user@drupal.org, not a repeat login. One may delete the user and re-test.

I don't currently have a site or time to test this myself.

I went on to test OpenID (registered a testing account at myopenid.com which was quick enough). Indeed, before the patch, the login tasks were not run, after the patch, my session ID was changed properly, the login was logged, etc. Good improvement.

Also looked into better documenting what happens here and added some docs while committing this patch. Submitted the attached patch.

Thanks, Gábor. Will this patch work also on 5.5? Do you know whether it fixes the problem I referenced above?

I'd suggest opening a new issue for Drupal 5 issues, but from a quick look on the Drupal 5 codebase, it looks like login task inconsistencies are existent there too.

Just a heads up. I tested the latest HEAD with the site_network module (use to be drupal.module in Core). This module no longer functions. I'll look into it in more detail later, just wanted to a) get that info registered on this issue and b) get subscribed to this issue.

Issue moved to http://drupal.org/node/204236

removing this from the queue...

AjK: site_network uses user_authenticate($username, $password);, while the latest Drupal 6 code provides function user_authenticate($form_values = array()). No wonder it does not work. Please submit an issue against that module or fix this in the module if you are the maintainer. This is a core issue, and it is fixed..