The first line of function user_login_authenticate_validate($form, &$form_state) {

should check if $form_state['uid'] is not null. If so it should simply return/exit the function at that point. The validate functions occur in this order:

user_login_name_validate
external authentication validate functions (0 or more)
user_login_authenticate_validate
external authentication validate functions (0 or more)
user_login_final_validate

so when the following sequence occurs:
user_login_name_validate [success]
external authentication validate functions [success, set form_state uid = N, log user in]
user_login_authenticate_validate [fail, set form state uid = FALSE]
user_login_final_validate [user warning is given and flood control not dealt with properly]

So the last step is broken for a user authenticating via an external means.

An outline of my validation workflow is at:
http://www.gliffy.com/publish/2362004/ in the upper right of the diagram

The design of the process is outlined in user.module about the user_login_default_validators function:

* Set up a series for validators which check for blocked users,
* then authenticate against local database, then return an error if
* authentication fails. Distributed authentication modules are welcome
* to use hook_form_alter() to change this series in order to
* authenticate against their user database instead of the local users
* table. If a distributed authentication module is successful, it
* should set $form_state['uid'] to a user ID.
*
* We use three validators instead of one since external authentication
* modules usually only need to alter the second validator.

This design is great and made the ldap module authentication sequence much simpler to implement; but without this fixed a successful logon with an external module does not work properly because user_login_final_validate gives them an error message.

Files: 
CommentFileSizeAuthor
#11 1022362-11.patch600 bytesbfroehle
PASSED: [[SimpleTest]]: [MySQL] 31,500 pass(es).
[ View ]
#9 user_login_authenticate_validate_uid_bug_fix2.patch488 bytesjohnbarclay
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_login_authenticate_validate_uid_bug_fix2_1.patch.
[ View ]
#7 user_login_authenticate_validate_uid_bug_fix2.patch498 bytesjohnbarclay
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in user_login_authenticate_validate_uid_bug_fix2_0.patch.
[ View ]
#4 user_login_authenticate_validate_uid_bug_fix2.patch625 bytesjohnbarclay
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in user_login_authenticate_validate_uid_bug_fix2.patch.
[ View ]
user_login_authenticate_validate_uid_bug_fix.patch487 bytesjohnbarclay
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in user_login_authenticate_validate_uid_bug_fix.patch.
[ View ]

Comments

johnbarclay’s picture

a short term fix for other external authentication modules is to always put your validate functions after user_login_authenticate_validate in the validation array.

bfroehle’s picture

Status:Active» Needs work

The patch provided won't apply, as the diff was done in the wrong order. I think you mean to add, not remove, the early return in user_login_authenticate_validate().

Also, a brief comment about the code flow would be useful, e.g.,

  // No additional validation is required if a distributed authentication
  // module has successfully authenticated the user.

Lastly, the check should probably be !empty, as the distributed authentication module might want to log in a user with uid = 0.

johnbarclay’s picture

Status:Needs work» Needs review
StatusFileSize
new625 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in user_login_authenticate_validate_uid_bug_fix2.patch.
[ View ]

thanks. I rerolled the patch and added comments. I also used the !empty check as you suggested.

Status:Needs review» Needs work

The last submitted patch, user_login_authenticate_validate_uid_bug_fix2.patch, failed testing.

bfroehle’s picture

You'll have to read up on creating patches to make a patch that the test bot likes. Essentially you'll have to get it so that the top two lines are

--- modules\user\user.module
+++ modules\user\user.module

Also, the comment should adhere to the coding standards, meaning it should being with a capital letter, end in a period, and line break at 80 characters.

johnbarclay’s picture

Status:Needs work» Needs review
StatusFileSize
new498 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in user_login_authenticate_validate_uid_bug_fix2_0.patch.
[ View ]

updated patch

Status:Needs review» Needs work

The last submitted patch, user_login_authenticate_validate_uid_bug_fix2.patch, failed testing.

johnbarclay’s picture

Status:Needs work» Needs review
StatusFileSize
new488 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_login_authenticate_validate_uid_bug_fix2_1.patch.
[ View ]

Don't laugh at me; I'm using a new IDE.

Status:Needs review» Needs work

The last submitted patch, user_login_authenticate_validate_uid_bug_fix2.patch, failed testing.

bfroehle’s picture

Status:Needs work» Needs review
StatusFileSize
new600 bytes
PASSED: [[SimpleTest]]: [MySQL] 31,500 pass(es).
[ View ]

John, I've fixed up the patch to put it in a conforming format for you.

I'm a little curious -- why not just alter the form and unset this validation function when the LDAP Auth module is enabled? If you need the function in your ldap auth login process, you could just call it directly.

johnbarclay’s picture

Thanks for fixing this.

Good question about the workaround. I can make ldap_authentication work simply by putting its validation function last. I can never replace user.module authentication because user 1 will always use it and the module supports mixed mode authentication so often many users are using user.module authentication.

The patch is more for other implementers of external auth modules; basically trying to reconcile the comments/design the the implementation in user.module. The other problem with messing with the validation sequence, is you don't know how other external authentication modules will mess with it. Its best if the behavior doesn't care about the order of validation functions.

The patch also makes it possible to predictably install more than one external auth module, by avoiding the need for the modules to reorder or remove validation functions.

dayer4b’s picture

subscribing

johnbarclay’s picture

My workaround in ldap_authentication for this works, but creates problems when other authentication modules or code come into play as identified earlier in this issue.

Without committing this, I don't see how 2 authentication modules can play well together. I suppose this should be a security issue since this leads to user authentication credentials being passed to multiple authentication tools in a way that contradicts the documentation.

I think thats a different issue process, so I'll look into that.

johnbarclay’s picture

Version:7.0» 8.x-dev
Status:Needs review» Active

This is continuing to present security issues in external authentication modules. If someone working on the user module in Drupal 8 could get this patch applied, we can backport it to 7.

The basic problem is that user_login_authenticate_validate() isn't implemented correctly. So it overwrites any successful authentication done by an external authentication module. In essence it makes the order of the validation critical. If a module wants to use mixed mode authentication it creates a nightmare because the external authentication module's validation function needs to also log the user on; otherwise it has to remove user_login_authenticate_validate from the authentication validators before the validation process starts. This is problematic since user_login_authenticate_validate() is doing flood control as well as drupal credential testing.

I don't have a drupal 8 install yet, but am still fighting this issue in the LDAP module in drupal 7.

Here's where the issue is:
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

It should be
+ If (!empty($form_state['uid'])) {
$form_state['uid'] = user_authenticate($form_state['values']['name'], $password);
+}

This will allow the multi purpose user_login_authenticate_validate() function to behave in accordance with the user module validation requirements in http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

login_authenticate_validate() could be split into 2 functions also.

user_login_authenticate_validate_flood()
user_login_authenticate_validate_authentication()

The design is completely correct. Since external authentication modules can't know before the logon form is submitted, which validations succeed it can't know which validation functions to remove from the validation array. So setting $form_state['uid'] to signify success if very appropriate.