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.

Comments

gábor hojtsy’s picture

Not 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.

damien tournoud’s picture

Confirmed. 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.

c960657’s picture

StatusFileSize
new26.72 KB

This 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.

c960657’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, openid-email-verification-1.patch, failed testing.

c960657’s picture

Status: Needs work » Needs review
StatusFileSize
new26.56 KB
c960657’s picture

StatusFileSize
new26.92 KB

The last patch left garbage in $user->data.

c960657’s picture

StatusFileSize
new26.88 KB

Reroll.

damien tournoud’s picture

Priority: Normal » Critical

I wanted to bump this to critical back in #3. Will review later tonight.

damien tournoud’s picture

Status: Needs review » Needs work

I 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.

c960657’s picture

Status: Needs work » Needs review
StatusFileSize
new26.85 KB

Nice catch. Here is a reroll with just that line removed.

c960657’s picture

StatusFileSize
new25.9 KB

Reroll.

damien tournoud’s picture

Status: Needs review » Reviewed & tested by the community
gábor hojtsy’s picture

Related: #542180: D7 version of OpenID does not deal with blocked users correctly (admin approval settings are not properly enforced on OpenID auto-registration either).

damien tournoud’s picture

Yes, let's get this one in first (it is a significant refactoring).

gábor hojtsy’s picture

I 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.

dries’s picture

Status: Reviewed & tested by the community » Fixed

Commited to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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