Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 1022362-11.patch | 600 bytes | bfroehle |
| #9 | user_login_authenticate_validate_uid_bug_fix2.patch | 488 bytes | johnbarclay |
| #7 | user_login_authenticate_validate_uid_bug_fix2.patch | 498 bytes | johnbarclay |
| #4 | user_login_authenticate_validate_uid_bug_fix2.patch | 625 bytes | johnbarclay |
| user_login_authenticate_validate_uid_bug_fix.patch | 487 bytes | johnbarclay | |
Comments
Comment #1
johnbarclay CreditAttribution: johnbarclay commenteda short term fix for other external authentication modules is to always put your validate functions after user_login_authenticate_validate in the validation array.
Comment #2
bfroehle CreditAttribution: bfroehle commentedThe 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.,
Lastly, the check should probably be !empty, as the distributed authentication module might want to log in a user with uid = 0.
Comment #4
johnbarclay CreditAttribution: johnbarclay commentedthanks. I rerolled the patch and added comments. I also used the !empty check as you suggested.
Comment #6
bfroehle CreditAttribution: bfroehle commentedYou'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
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.
Comment #7
johnbarclay CreditAttribution: johnbarclay commentedupdated patch
Comment #9
johnbarclay CreditAttribution: johnbarclay commentedDon't laugh at me; I'm using a new IDE.
Comment #11
bfroehle CreditAttribution: bfroehle commentedJohn, 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.
Comment #12
johnbarclay CreditAttribution: johnbarclay commentedThanks 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.
Comment #13
dayer4b CreditAttribution: dayer4b commentedsubscribing
Comment #14
johnbarclay CreditAttribution: johnbarclay commentedMy 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.
Comment #15
johnbarclay CreditAttribution: johnbarclay commentedThis 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.
Comment #21
nithinkolekar CreditAttribution: nithinkolekar commentedunder https://api.drupal.org/api/drupal/core%21modules%21user%21user.module/8.5.x I don't see any *_validate function except user_validate_name.
what happened to those user_login_default_validators's methods?
Comment #28
LendudeThis came up as a Bug smash daily triage target
This has moved to
\Drupal\user\Form\UserLoginForm::validateAuthenticationbut the premise is still the same, that method does 2 things, flood control and user authentication. Now, if I want to do flood control and not authentication, I can’t without duplicating lots of code. Which was basically done in\Drupal\basic_auth\Authentication\Provider\BasicAuth::authenticate, which is mostly the same, but slightly different.So marking this as a task to refactor this and see if we can't make this more 1 method 1 responsibility and maybe not do the logic and code duplication with these two methods.
Comment #29
longwave#2431357: Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController is related, if not a duplicate.
Comment #30
Lendude@longwave pointed this one out #2431357: Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController, closing this as a duplicate of that since that is the right direction for this and has been focussed on this since the start