If you log in with an OpenID identity that is not registered to a Drupal account, an account is automatically created, and the user is logged in (assuming everything went well), even if user_email_verification is TRUE. The user can then browse to his account page and change his password. The account is now activated without email verification.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | openid-email-verification-6.patch | 25.9 KB | c960657 |
| #12 | openid-email-verification-5.patch | 26.85 KB | c960657 |
| #9 | openid-email-verification-4.patch | 26.88 KB | c960657 |
| #8 | openid-email-verification-3.patch | 26.92 KB | c960657 |
| #7 | openid-email-verification-2.patch | 26.56 KB | c960657 |
Comments
Comment #2
gábor hojtsyNot looking good. Interestingly the code does seem to have quite some thought to try and be compatible with email verification, but I can reproduce this issue.
Comment #3
damien tournoud commentedConfirmed. Bumping to critical, we cannot ship Drupal 7 with this issue.
Note that there is a test for email verification, but it is slightly dumb: it only verifies that the
Once you have verified your e-mail address, you may log in via OpenID.message is displayed to the user.Comment #4
c960657 commentedThis patch unifies more of the two code paths in the auto-registration process (one where auto-registration happens without user interaction at all, and one where the user has to fill out the registration form manually).
The registration is now handled by submitting user_register_form programatically using drupal_form_submit() - I don't remember why we earlier did this in smaller steps using drupal_retrieve_form() + drupal_prepare_form() + drupal_validate_form(), but that doesn't seem necessary anymore. I have earlier tried to switch to drupal_form_submit(), but without success.
I added a new test, testRegisterUserWithEmailVerification(). I also added a new helper function to OpenIDFunctionalTest, submitLoginForm(), that fills out the login form, submits it and handles the redirect to the OpenID provider. This doesn't add new functionality but just holds a few lines of code that was duplicated in every test.
Comment #5
c960657 commentedComment #7
c960657 commentedComment #8
c960657 commentedThe last patch left garbage in $user->data.
Comment #9
c960657 commentedReroll.
Comment #10
damien tournoud commentedI wanted to bump this to critical back in #3. Will review later tonight.
Comment #11
damien tournoud commentedI supposed that the unset($_SESSION['openid']) in openid_authentication() is redundant and can probably be removed.
Other then that, this looks very ok to me. It is conflicting with #740036: OpenID AX: Request nickname + support alternative namespaces, so one of them will have to be rerolled.
Comment #12
c960657 commentedNice catch. Here is a reroll with just that line removed.
Comment #13
c960657 commentedReroll.
Comment #14
damien tournoud commentedComment #15
gábor hojtsyRelated: #542180: D7 version of OpenID does not deal with blocked users correctly (admin approval settings are not properly enforced on OpenID auto-registration either).
Comment #16
damien tournoud commentedYes, let's get this one in first (it is a significant refactoring).
Comment #17
gábor hojtsyI think this one might even solve that, since here it just reuses the form submission for the registration form, so if the site was set to let the user log in right away, they will be logged in, I believe (did not test). Also, emails will be sent on pending accounts and users will not be logged in in that case (again, I believe). While even just patching core with my mentioned issue did not give full compatibility for OpenID in terms of functionality. So while I did not test this patch, it looks like a good idea.
Comment #18
dries commentedCommited to CVS HEAD. Thanks.