Anonymous checkout, order assigned to existing account without validation

@thtas:
I'm not hijacking this issue, just making it a bug report. Your solution is still valid for this issue.
This is still an issue in 2.2 so updating version.

I don't know of any ecommerce sites that will allow an anonymous user place an order against an existing
users account without having to enter the correct username and password for that account.

This seems like a huge hole.

There really should be a setting that controls what happens if an anonymous user submits an order with an email that matches an existing user.

+1 - this is, as a recent client has stated, "plain crazy". If a user wishes to use an email address which is in the system - they must login before, otherwise this becomes a mess...

I agree

Not a bug, as this behavior was intended and is documented. (Not a "huge hole" either - there are no real security implications of this behavior, as discussed many times here in this queue as well as on ubercart.org.)

I do think the original poster's approach is better, so I am moving this back to a feature request.

Anyone who would like to see this implemented should try out the proposed change and report back on whether it works as expected and whether it causes any problems elsewhere. Something more detailed than "works for me" would be appreciated. A proper patch would be nice too.

I created a module using the exact code listed above. I tried an email address that was already in my database and it worked as intended. I didn't notice an issue until I tried to run update.php. I checked the Recent Log Entries and the following error was there:

array_unshift() [function.array-unshift]: The first argument should be an array in sites/all/modules/emailcheck/emailcheck.module on line 4.

I disabled the module, ran the update, enabled the module and the error was still there.

I think the original posters solution is fine also.

I still think it is a fairly big hole though, documented or not.
It isn't a security problem but your users wouldn't appreciate it if they knew that happened on your site.
Imagine if that was possible with your ebay account. Would you be worried as an ebay user?

Agree, HUGE hole. I am working with ubercart and node checkout to handle anonymous conference registartions and this is a MAJOR roadblock. To be honest this expected behaviour makes no sense to me. I don't want orders "attached to the account we found matching your e-mail address", I want them to stay anonymous (UID = 0). So, HOW DO I DISABLE THIS?

Agree that this could lead to a potentially nasty situation with clients/customers. Creating a config setting for this would be ideal, but I had to take a very fast and lazy route to fixing this as follows.

Patch to uc_cart_checkout_pane.inc for code beginning line 157.

Original:

<?php

        // Skip if an account already exists for this e-mail address.
        if (db_fetch_object(db_query("SELECT uid FROM {users} WHERE LOWER(mail) = LOWER('%s')", $arg2['primary_email'])) > 0) {
          drupal_set_message(t('An account already exists for your e-mail address. The new account details you entered will be disregarded.'));
        }

?>

Changes to:

<?php

        // Skip if an account already exists for this e-mail address.
        if (db_fetch_object(db_query("SELECT uid FROM {users} WHERE LOWER(mail) = LOWER('%s')", $arg2['primary_email'])) > 0) {
          drupal_set_message(t('An account already exists for your e-mail address. For security reasons, please <a href="/user">login</a>, return to your shopping cart, and begin checkout again.'));
	      return FALSE;
        }

?>

This is the super lazy version as it won't validate until the user submits the whole form. Some AJAX-y validation here would be awesome but I'm way too overwhelmed for that at the moment.

I know you all are finding issues in the code but I have another observation to add to the problem of "attached to the account we found matching your e-mail address"

Issue 1: I too have been getting this message when folks return from Paypal. I have checked and rechecked and what is actually happening is, new accounts are being created, the message that is supposed to be returned to the new user is not correct. They are getting the message for "Checkout completion for existing users:" and they are not logged in. They should be getting the results associated with "Checkout completion for new users:" - my short term solution was to amend the message (bad I know but I didn't see this issue and didnt have time to troubleshoot until now.)

Issue 2: During my testing (given the issues posted here), I tried using the process of making them create an account first and then buy. When I got my account and logged in, the cart was empty. I logged out and returned the status to allow anonymous to check out. When I repeated the process to test issue 1 for this post, I found that my purchase got associated with user I created when I trying to force someone to create an acct first and then log in. Here are the steps to recreate:

1. set to no allow anonymous check out
2. go to the site as anonymous and try to buy something
3. create the acct as directed
4. go get the password provided in the system email and login
5. see the shopping cart empty
6. logout
7. reset to allow anonymous to check out
8. repeat the purchase process as anonymous using and a unique email
9. complete the purchase and return to site
10. see the "attached to the account we found matching your e-mail address" message
11. check orders and see that the order is attached to the previous user

In all my testing, this has never happened because I never thought to disable anonymous checkout and then re-enable it. I am not sure the patches above would fix this issue. thoughts?

Could issue 1 and 2 be related or are these two separate issues? Is issue 1 related to what you have been discussing?

EDIT: issue 1 happened in version 2.2 and still in 2.3

subscribing

No patch here, nothing to review.

Subscribing. Looking at enabling anonymous checkout for our site. Will try to get to this.

Here is the patch that invalidates anomymous checkout for existing email addresses.

This is really very much needed. Anonymous checkout is great functionality but really should never be allowed for existing email accounts!
The user will either need to login or use a different email account.

+1. TR, Lyle, can we get this into core?

Hasn't been anything to review until today ... Just off the top of my head, looking at the patch, I think instead of the drupal_set_message() there should be a form_set_error() for usability, to highlight the offending field.

#15
If you would have checked the code you would have seen that all the other checks follow the same procedure:

            $message = user_validate_name($arg2['new_account']['name']);
            if (!empty($message)) {
              drupal_set_message($message, 'error');
              return FALSE;
            }

#16 Just because it's done that way, doesn't mean it's the right way. For a decent user experience, using form_set_error is good practice.

The OP had this in their original bit of code they proposed; I think we could easily roll that into another patch.

I think we are mixing up issues here.
The issue was and is "Anonymous checkout, order assigned to existing account without validation".
For improving existing code another issue should be created that goes beyond this single place where form_set_error should have been called.

Huh? Not sure I understand. I think TR meant that, instead of using using drupal_set_message() as you have done in your patch, it's possibly more wise to use form_set_error on that form element's ID. That will 1) provide the user with an error message and 2) highlight the textfield itself so the user can change their email address, if they so choose. (Going further, I would almost suggest putting a user login block in its place, or inserting a password field in addition to the username field, but I digress.)

In any event - I think the patch is fine as-is, and shouldn't necessarily be held up for review. It works in its current state, I've tested it and for this immediate need, it does the job admirably. We could always tweak it afterward, but I think for an issue as major as this (it is major) it shouldn't be left uncommitted for too much longer.

Regarding the form_set_error issue, there is already an outstanding patch to do this which TR said should be committed seven months ago. #713020: Anonymous user e-mail validation should set form error, not just write a message

Also, the patch in #13 still allows existing email addresses to be used if the new username and password fields are both disabled. Was this intentional?

Here's an updated patch for 2.x-dev:

- moved the validation so that even when users cannot specify names or passwords it still checks the email against existing users, and
- included the latest form_set_error() changes, and changed this validation to act identically.

RTBC, except I think this line:

        if (db_fetch_object(db_query("SELECT uid FROM {users} WHERE LOWER(mail) = LOWER('%s')", $arg2['primary_email'])) > 0) {

should be changed to:

        if (db_result(db_query("SELECT uid FROM {users} WHERE LOWER(mail) = LOWER('%s')", $arg2['primary_email'])) > 0) {

as checking db_fetch_object() > 0 doesn't make sense, even if it did work before as a side effect of casting an object to an integer (which results in 1).

Agree with longwave, fix that db_query() line and I think we are golden. Patch looks great to me otherwise.

Here we go.

Does the SQL function LOWER() work properly for Unicode/UTF-8 strings? And why is LOWER() needed at all? SQL selection is case-insensitive by default ...

IIRC I think the LOWER() is needed for other database engines, like postgres. If I'm recalling right, the DBTNG will take care of this issue - and I think Pressflow is optimized for MySQL in the sense that they have done away with all the LOWER() commands in the core db layer. As far as unicode compatibility, I'm not sure.

I am under the impression that the SQL 92 standard specified that SELECTs are case insensitive - it's not just a MySQL thing.

Drupal core uses the same SQL, see http://api.drupal.org/api/function/_user_edit_validate

You might be right, but core Drupal is using it. I'm not sure what they are trying to avoid here (I can't remember if it's SQL or PHP issues) but it's something worth investigating. For instance, compare modules/user/user.module in Drupal and in Pressflow.

Some of the issues are discussed at #279851: Replace LOWER() with db_select() and LIKE() where possible

Seems to me that the way moving forward is to drop the LOWER altogether in favor of other methods that are faster and indexable. I'm okay with getting rid of the LOWER in every instance, as long as we're not impacting users in other db platforms.

EDIT: Yeah, as longwave pointed out, user.module is one instance I know it's used:

  $deny = db_fetch_object(db_query("SELECT name FROM {users} WHERE status = 0 AND name = LOWER('%s')", $name));

But it looks like there are patches / issues waiting to fix it in D6, as it's already been addressed in D7.

OK then. I committed the patch.

Awesome! Thanks, TR. :)

Thanks much!

If this is not wanted for a particular site, the module http://drupal.org/project/uc_existing_email exists which restores the old behavior (logging in is not required).

You could write a patch that adds the old behavior back in. Personally I'd like to see logging in NOT be required as well. Seems like it should be a configurable option, really.

Sorry to bring this issue back active, and re-close if it really is meant to be like this, buy this seemed like a patch given to certain use cases. I read through the issue and agree this might be a pain for certain stores, but on others, it's just a more easy way of purchasing. Not everybody want's to log in on a site to buy a few stuff now and then.

I am glad that the old behavior is still on a module but as @torgosPizza said on this should be rather configurable than anything else, isn't it?

I am willing to write a patch for this if needed, but just wondering what the thoughts (if anything new) on this are.

I'm happy to review any patches if you want to write one!

Ok, attached is the patch, which adds what was removed by the last patch on this issue, left what was added, and enclose both things with a new setting added to the checkout pane settings.

Default settings is true (to "Allow anonymous customers to buy using an already registered email address.") to maintain backwards compatibility (it's how it's working on 2.4 and how I expect ubercart to still be working on upgrades).

Committed the attached patch to 6.x-2.x-dev, which is #39 with some text changes to the admin setting.

Committed to 7.x-3.x: http://drupalcode.org/project/ubercart.git/commitdiff/c353da1

Rocking. Thanks, longwave!

will the patch #42 be implemented into core?

thanks!

@duckx it was committed, so I would definitely think so.