Scenario:

Account created via Facebook connect or openid so user has no email.
Recurly is sent uid as account_code
Recurly Drupal module creates new account with email instead of attaching to correct account.

Solution: Check to see if account_code is numeric first, if so, user_load assuming account_code is a uid. Otherwise, proceed as currently being done.

Every account has a uid, but not all accounts have an email.

Comments

SolomonGifford’s picture

SolomonGifford commented 
        //By default, recurly uses email as the account_code, but openID and facebook accounts may not 
        //have an email to look the account up with, so we first check to see if the account_code is a uid.
        if (ctype_digit($recurly_account->account_code) &&  $user = user_load($recurly_account->account_code)) {
          $uid = $user->uid;
        } else
        // Attempt to find a matching user account.
        if ($user = user_load_by_mail($recurly_account->email)) {
          $uid = $user->uid;
        }
keithm’s picture

keithm commented
Status: Active » Needs work

Thanks solomongifford. I have some questions:

Is there a reason not to check email first?

How would a Facebook or openid account get the Drupal uid? Is there a way of verifying or corroborating that the number contained in the recurly account actually belongs to the drupal account?

SolomonGifford’s picture

SolomonGifford commented

We decided not to use recurly for our project, but to answer your questions (6 months later)...

OpenID accounts or other accounts created via third party authentication don't require an email on the account. The user record still has an uid, but no email. For accounts created this way, in order to work with recurly, one has to send the user->uid as the account_code.

The point - every account has a uid, but not every account has an email.

As for which is primary, I'd rather use uid simply because there's a guaranteed match. Think about a person who has multiple accounts, one based on their email but one based on their facebook connect (this is a common scenario). Yet when they submit the recurly form while using their facebook connected account, they give their email address associated with their abandoned email-based account. This module actually in that scenario would attribute the donation to the wrong account because of the email. We actually experienced this while testing this module on a facebook enabled site.

quicksketch’s picture

quicksketch
he/him
commented
Title: Should attempt to load user by uid before email. » Should attempt to load user by uid before email
Category: bug » feature
Status: Needs work » Active

While using the UID would indeed be better because it's more likely to have a match, there's *very* little security around account_codes in Recurly currently. Any user could potentially go to https://example.recurly.com/subscribe/[plan_name]/[uid] to signup as a particular account_code. Given automatic syncing of accounts, you could end up in a dangerous situation really fast.

So in short this is an excellent idea but we need to think about it more before implementation. I'm moving this to a feature request since I don't see a direct bug at this point.

SolomonGifford’s picture

SolomonGifford commented

Thats fine - this case serves as documentation that this problem scenario would present itself for sites with accounts that don't require emails.

quicksketch’s picture

quicksketch
he/him
commented

Given automatic syncing of accounts, you could end up in a dangerous situation really fast.

Now that I'm getting more familiar with the implementation so far, I realize that Recurly doesn't currently do anything with Drupal user accounts. The "account syncing" it includes just means it updates records in the "recurly_account" database to reflect the status of accounts on the Recurly website. However if we start implementing something like #1256948: Create drupal account on new subscription?, the danger becomes greater.

I've been thinking about these identifiers and I was thinking rather than tying directly to UID, we should prefix the Recurly "account_code" with something that identifies the entity type, so instead of just "1", you might have Drupal use the account code "user-1". Not all accounts are necessarily 1-to-1 with users. Something like a Github corporate account isn't tied to an individual, it's tied to a company (which doesn't necessarily have a login and password). Such prefixes would prevent conflicts between different object types.

quicksketch’s picture

quicksketch
he/him
commented
Status: Active » Fixed

This is now done as part of #1621894: The "recurly_account" database table may contain sensitive information. Some further consideration is going on over in #1630772: Push account updates to Recurly contact information.

Status: Fixed » Closed (fixed)

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