I was using the rules module, created an event that reacts on login, and then noticed that login happens twice.
After some debugging, I identified that both ldap and core login functions are being called simultaneously.

I looked at the code and ended up with the following...

First problem I see, which I personally think is a major problem that dances on the security line, is that the ldap login process is happening during the validation process as opposed to the submit process.

Second problem I see is that it looks like you were at some point starting to write the appropriate '#submit' hook, but then stopped, and I guess, forgot about it. The function does not exist.

I suggest the following:
- Remove old references to the unimplemented _ldap_authentication_login_form_submit().
- Remove the manual user_login_submit() call from _ldap_authentication_user_login_authenticate_validate() and instead place the uid in $form_state['uid'] (which appears to be the proper way to pass validate confirmed).

The following problems are solved:
- allows ldap to intergrate/interact with other login modules without conflict (likely to solve: #1022362: \Drupal\user\Form\UserLoginForm::validateAuthentication function does not follow user module validation rules).
- no more double logins.
- follows drupals form -> validate -> submit process and helps ensure consistency and security.

Notes:
- I still had to move/keep some of the old code so that the ldap SSO would function in the same way. I don't have a way to test those changes, so much testing is needed.
- I am thinking that if other authentication modules are now allowed, then when ldap first tries to process its code, if the $form_state['uid'] is something other than 0, then do not process ldap because the account was allowed in via some other module.
- If you want to make ldap an authority on login then replace the $form['#submit'] array with an array containing _ldap_authentication_login_form_submit() (and implement that function). You could even save the existing $form['#submit'] values in another location.

CommentFileSizeAuthor
ldap-7.x-1.x-fix_ldap_login_process-1.patch2.38 KBthekevinday

Comments

johnbarclay’s picture

johnbarclay commented

As you note, this is very complex and built around a bug in core. As such I don't see posting this against the 7.x-1.x branch. We should work this through in the 7.x-2.x branch and not release it until the sso simpletests are functional.

thekevinday’s picture

thekevinday commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Needs review » Needs work

Not a problem.

johnbarclay’s picture

johnbarclay commented

Since authentication hasn't changed in the 2.0 branch, we should follow through with the current patch here, committing what can be to both. What can't be committed can be a patch for both until its been tested.

That way we don't lose any of the work you've done thus far.

johnbarclay’s picture

johnbarclay commented

I removed the submit function. It was already removed from the 2.0 branch. After looking this over, this patch is a high priority for me in both branches. I'm going to start digging through the issue queue and look at what issues caused the forced manual login workaround. If the workaround is only related to SSO, should be a pretty easy fix.

johnbarclay’s picture

johnbarclay commented

I looked over this and don't see a good way to do this. I nagged to core issue, so hopefully it will get some love in Drupal 8 #1022362: \Drupal\user\Form\UserLoginForm::validateAuthentication function does not follow user module validation rules

The most secure way I believe is simply to pull the function user_login_authenticate_validate() off the validation function stack and replace it with a properly implemented clone of it. That way all this workaround crap can be removed and the module can be more secure and also be able to work alongside other authentication modules.

The biggest limitation of this is that it will only work on certain logon forms. But this is already the case anyway.

johnbarclay’s picture

johnbarclay commented
Status: Needs work » Needs review

I committed a patch for this to 7.x-2.x-dev. http://drupalcode.org/project/ldap.git/commitdiff/e3a6848c1f32351a969abd...

It passes all the simpletests, but should be reviewed carefully by as many as are willing. The patch basically provides a workaround for #1022362: \Drupal\user\Form\UserLoginForm::validateAuthentication function does not follow user module validation rules which makes some of the logon form validation workarounds unnecessary.

johnbarclay’s picture

johnbarclay commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

swentel’s picture

swentel commented
Status: Closed (fixed) » Active

This commit breaks SSO login completely resulting in either broken pages or endless redirects.
A 'temporary' fix is following snippet after the lines that were commented out around line 355.

  // Temporary fix to authenticate.
  global $user;
  $user = user_load($drupal_account->uid);
  drupal_session_regenerate();

However, this is a stop gap fix.

Currently roles are not save either, digging in that one now as well, but this is probably not related to this, because this used to work before.

swentel’s picture

swentel commented
Status: Active » Closed (fixed)

Will move over to #1415270: LDAP Authentication SSO redirect loop on automatic sign in, which seems to have a better patch than mine.