Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wordpress user creation with duplicate emails #219

Closed
xtianjohns opened this Issue Jun 28, 2016 · 33 comments

Comments

Projects
None yet
3 participants
@xtianjohns
Copy link

commented Jun 28, 2016

This issue should probably begin as a question and then move to a potential improvement in the plugin and/or documentation.

Background

I have a situation that involves supporting the same user logging in with two different social providers with the same email address. The user here is the same person, but because they are using two different social providers, there are two "users" in Auth0 for that person.

When that user logs in to Wordpress with social connection A for the first time, everything works in this plugin as expected. However, when they log in to the same Wordpress site with social connection B for the first time, this plugin attempts to create a new Wordpress user. Is that intended behavior?

The logic for this looks like it rests in the LoginManager class.

So my question is: do I understand this correctly? The user is authenticated with Auth0, and then this class looks to see if it has seen that Auth0 user before by searching the Wordpress database for users with the same Auth0 user_id. If it doesn't find a user in that database, then it tries to create a new user in Wordpress.

Question/Proposal

If this is correct, I think that it merits a revisit. Even if the functionality in the class stays the same, I think it's important to put guards in place to make sure that the plugin does have permission to create new users. If this method is invoked in this path, then it doesn't matter if I've turned off 'disable new signups' in Wordpress, this plugin will still try to create a user.

To sum up:

  1. This plugin should never attempt to create users if:
    • I've disabled signups in Wordpress.
    • The plugin tells me that new signups are disabled.
  2. This plugin should perform some enhanced investigation to pair existing Wordpress users with an authenticated Auth0 user, regardless of the Auth0 user_id.
  3. This plugin would be enhanced with some messaging to users within the lock widget when signups have been disabled but the user is able to authenticate with Auth0.

Related

  • #4 (Re: Should the plugin provision users that exist in Auth0?)
  • #107 (Re: Should the plugin check the Wordpress database for users?)
  • #12 (Re: The nature of "could not create user" messages)
    • Specifically, the plugin already tells us that it cannot create a user because another user already has that email address. But if the functionality of the plugin changes, then there is a flow where a user will authenticate properly with Auth0 and no new account creation will take place in Wordpress. I think there are good arguments for making this part of Lock itself: the equivalent of saying to a user "I'm sorry, you do not have a Wordpress account for this site, etc."
  • #5 (Re: Updating the behavior of Lock based on plugin configuration options)

Versions

Wordpress: 4.5.3
Plugin: 3.1.3

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 28, 2016

I think it's also useful to note that the workaround for my unique situation involved linking the Auth0 users together. However, there might be good reasons for an organization to keep those users separate. Additionally, the automatic linking rule will only link accounts with verified emails, which limits that solution's scope for the problem.

@dleeward

This comment has been minimized.

Copy link

commented Jun 28, 2016

If both social logins pass the verified email flag to Auth0, will the automatic linking work?

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 28, 2016

Yes, @dleeward, as far as I know. After experiencing the issue, I just toggled the automatic linking rule option in the plugin settings and then attempted a second login with social provider A. That linked both accounts in Auth0, and subsequent attempts to login with social provider B worked fine. If one of the providers did not support verified email information, then I'm not sure what would happen. I will try to troubleshoot this today.

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

for the first comment.

About 1 I dont think this is right. The idea on the latest version is too disable signups at the auth0 level. So if the user already exists in auth0, it is not a real signup.

About 2, previous versions did some linking at the wordpress level but there were a lot of issues when you try to find which is the correct auth0 user to update for example. This is why we think is better to rely the users linking to an auth0 rule instead of doing it on wordpress

About 3, I guess you are talking about social connections (on db connection the user can not signup). Thi think with social connections is that there is no way to avoid the redirection (basically because this is how oauth2 works).

for the second comment.

Wordpress does not let you create different users with the same email, this is why this restriction exists.
About the restriction to only link verified accounts, if you do not check that, account hijacking will be as easy as use another user email to signup. IE: lets say you signed up to my site with facebook. Then I use your email to signup with email and password, both account gets linked and I am logged in with your user.

about the third (@dleeward 's) and fourth comment
Yes, auth0 handles social accounts as verified by default, they take care of this process

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 28, 2016

Definitely, the verified email option seems like a no brainer.

Preventing signups

This is complex, right? It sounds like there are multiple options for disabling signups, and they all have vaguely different meanings.

Prevent Lock from showing signup page
This is a view configuration for folks who don't users to see that option on the lock screen. Right now these settings are mingled: Lock will follow suit with the Wordpress site. If Wordpress says "no signups allowed", then Lock won't show the option either.

I just think these are fundamentally different things. One relates to some Wordpress settings, and the other affects Lock display. Maybe one should always trigger the other (if Wordpress signup is off, then don't show) but they are not always the same (Wordpress sign up is on, but still don't show the option in Lock?).

Prevent Auth0 from signing up new users
This is apparently where you, @glena, suggest the plugin/webservice is moving. And if that's the case, then it makes a lot of sense, right? Someone authenticates with Auth0 so they're already a user.

But I think, again, this is different. Auth0's concept of an application and Wordpress's concept of a site are different things. If I authenticate to my application in multiple ways (with multiple sites, even) and just happen to have a Wordpress site where some users are already provisioned, then having this Wordpress plugin automatically make some decisions about provisioning seems like a bad choice.

I'm not saying it should never do it ever. I totally understand other people's use cases, but mine doesn't seem totally unreasonable. In fact, if my use case is hosting 100 Wordpress sites for my clients, then of course I'll have multiple users in Auth0. Just because they exist in Auth0 doesn't mean that I want that user to exist (or have access) to every Wordpress site that I manage or host.

Prevent the plugin from provisioning users
This is what I'm suggesting should exist, if it doesn't already. Just a way to tell the plugin that it should never create a new user in Wordpress. Ever.

My use case above should demonstrate why this would be desirable.

Linking users

You bring up a good point. Managing the users in Auth0 and keeping any linking there seems reasonable. I suppose my beef with the approach is this: rules are automated mechanisms for linking the users, but this plugin misses that data on the initial login.

In my situation, the login with social provider B linked the users in Auth0 via a rule. But when Wordpress got the response, it just knows that I'm authenticated with provider B. So, it'll fail on that login attempt because it can't create a new user for me due to duplicate email restrictions. When I go back to the login screen and try to log in again, everything should work fine. Because now the accounts are linked in Auth0.

Since we're guaranteed to have all rules complete as part of the authentication flow, I figure there are two candidate solutions here:

  1. Modify the default linking rule in the plugin to return the linked account from the Management API rather than the user that originally authenticated. This is probably bad because the rules should be generally chain-able, I think, but it doesn't seem totally unreasonable.
  2. Make a subsequent request to the management API for a user's linked information before searching for their ID in the Wordpress database. This isn't a cure-all because the rule will arbitrarily link accounts and may dump similar accounts under a different account than the first one used to sign in to the Wordpress site. But it should increase the chances that the plugin will find the right user.

All that being said, I definitely think it's reasonable to use emails as the uniqueness constraint for this plugin. Wordpress prevents users from having the same email address, so why couldn't this plugin treat social users with the same email address as the same user? The example about unverified emails is totally on point: there are situations where people who use the plugin could leave a box unchecked and leave their site vulnerable. But Auth0 lets me do it. If the plugin implements both the Auth0 web service and Wordpress, then it seems okay to let it implement all of the features available in the platforms.

Messaging and redirection

Maybe we can't disable the redirection, but Wordpress definitely spits out the messages, right? I see a white screen with a link to the login page as an error. Is it unreasonable to skip that and simply render the login page with a message provisioned for the Lock Widget?

@dleeward

This comment has been minimized.

Copy link

commented Jun 28, 2016

Yes, auth0 handles social accounts as verified by default, they take care of this process

@glena, do you mean that by default, Auth0 assumes that emails in social accounts are verified or does Auth0 only allow users to sign in with social accounts that have verified emails?

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

Preventing signups

Signups are disabled from the database connections and a rule is provisioned to avoid new social users (signups) to access the site.

If you are relying the authentication in auth0, the concept of signup should be related to your auth0 users not the provision of new users in wordpress.

Linking users

Yes, the issue linking users where the main identity ends up being the new user is a know issue. I am planning on moving this to the plugin.

About I definitely think it's reasonable to use emails as the uniqueness constraint for this plugin., it is not something we can or can not agree on, this is how wordpress works. You cant have two users with the same email, so in this case, there is no much to discuss.

so why couldn't this plugin treat social users with the same email address as the same user? because in wordpress there is no way to do so. You cant have two users with the same email, period. Any way to do so will end up in a hack.

About users that did not verified the email, the idea of the plugin is to secure your site, not to open security holes. You can disable this verification on the plugin but linking users shouldnt leverage this unless you want to expose your users data. Anyway, it is handled in a rule so feel free to change its code. We do not see the security as an opt in. We will provide the most secure approach, if you want to make it weaker, you can.

Messaging and redirection

It is not that easy but I agree on showing the message on the login page somehow. Anyway, there is a lot of work to change all the messages but for sure I will add this to the backlog.

Being all said

if you don't agree with the decisions taken on the plugin or if it does not match entirely with the use case, which is entirely understandable, feel free to fork and change it. It is open source for a reason, we don't want to force the implementation to our customers, we want to provide an easy, secure and frictionless, out of the box way to use Auth0.

The same happen with rules, every rule set by the plugin, extension or the examples we provide are templates. You can change them or build your own in order to satisfy your needs.

Dont take it in the wrong way. I reallt appreciate feedback and I try to address any of the feedback/request I receive, but on the other hand I try to get the best solution to suit all our customers requirements.

Anyway, help me to sum up the points we discused to see what can be addressed on the plugin and which of them will not. For the second, we can find a way to make it easy to you to change the behaviour (for example with a filter or a hook) like I did in the future for other special requirements.

Does it make sense?

TODO:

  1. improve messages and redirections
  2. move account linking to wordpress
    • will provide a filter to override which users should be linked and which should not
    • will provide a filter to pick which will be the main identity
  3. maybe we can add another setting to disable provisioning. Signup will continue to work as it is.

how does it sound?

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

@dleeward the IdP verifies the email beforehand

@dleeward

This comment has been minimized.

Copy link

commented Jun 28, 2016

The reason I asked is that I seem to recall that LinkedIn was allowing social logins last year with unverified emails and it created a security issue.

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

@dleeward I would have to check the code to confirm that, do you mind creating a ticket on support.auth0.com? Someone in there might have more details

@dleeward

This comment has been minimized.

Copy link

commented Jun 28, 2016

Done.

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 28, 2016

@glena, your summary is totally reasonable, I appreciate your work on it.

I guess one of the reasons why I opened an issue was to see in what ways I (or others) can contribute improvements in pull requests. Some features don't get introduced, and others do. That's expected. I'm happy to help jump in on a feature as needed as you allow, and I definitely don't mean to complain about the work you and others have done on the plugin so far. If the best way forward is to fork, do our work, and submit a PR, let me know. Perhaps a discussion of the feature is better there than here in this issue.

I do think we're having a miscommunication about Wordpress and linking, though. I'm not suggesting any feature that involves two Wordpress users. I'm suggesting that the lookup be expanded from pairing Auth0 to Wordpress via an Auth0 user_id to include the email address. That's all.

I think that can be implemented in the filters/hooks you suggest. And I agree that for especially custom work, adding that to the plugin source is not ideal and my organization should just fork and move on. But I really do think that this is an improvement on the plugin. If two users in Auth0 have the same email address (and they're verified, if that's a security requirement that seems reasonable), then I don't know why the plugin can't treat both as the same Wordpress user (the user that shares the email address).

I hear you re: rules in Auth0. If folks are using Auth0 for auth, it's a reasonable expectation that we write rules to link the user in Auth0. And at the same time, I think the plugin is more "out of the box" and valuable if it treats two users authenticated by Auth0 with the same email as the same Wordpress user.

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2016

PRs are always welcome :)

I do think we're having a miscommunication about Wordpress and linking, though. I'm not suggesting any feature that involves two Wordpress users. I'm suggesting that the lookup be expanded from pairing Auth0 to Wordpress via an Auth0 user_id to include the email address. That's all.

The first lookup is being done using the email. Once it found the user, it sets the user_id, otherwise you will end up having multiple auth0 users per wordpress user. The idea to get over that is to link users on the auth0 side. Maybe a filter/action would work to let you override this behaviour. Anyway, the issue will be how is the auth0 profile stored later in the wordpress database. Right now the plugin expects to have one auth0 profile in the user meta for those wp users with a linked auth0 profile and this will be overriden on each login if a user uses two different connections to access to your wordpress (lets say email/password and facebook).

then I don't know why the plugin can't treat both as the same Wordpress user (the user that shares the email address).

the thing is that it is an assumption I am not confortable taking. On early versions they plugin worked that way, but we find a lot of pushback trying to move on with new features. Also, if diferent auth0 users should be treated as the same one, makes more sense to link them together into the same user at auth0 users.

Help me think a way to get over this restriction with a hook. This way we can provide a customisable experience instead of guiding us with assumptions where there will be always a use case that does not match.

I am thinking on 2 hooks. One to do the user matching (by default it will be done by user_id, but you can override it to do it by email). The other is to update the auth0 profile in the user meta (by default will update it on each login with the profile it gets from the user that logged in, in this case, you might want to create another meta that stores an array of profiles or something like this)

@dleeward

This comment has been minimized.

Copy link

commented Jun 30, 2016

Different social providers implement this email verification differently and Auth0 doesn't consider this when logging a user in.
If you need to allow only users that are verified, your best option is to create a rule that calls the relevant social provider's APIs and check if the email has been verified. It would be similar to the following, but you will have to additional API calls:
https://github.com/auth0/rules/blob/master/rules/email-verified.md
Most social providers let you query whether the email is verified or not via their API. Since you get access to the user's access_token inside the rule (in user.identities[0]), you can use this access_token to query the API and get the necessary details. If you have enabled multiple social providers, you will have to conditionally call the relevant APIs based on the connection the user just logged in with (using context.connection).
Please let me know if this answers your question.
Thanks,
Thameera Senanayaka
Developer Success Engineer - https://auth0.com

@dleeward

This comment has been minimized.

Copy link

commented Jun 30, 2016

This just made things a lot more difficult...

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

@glena: those are both good ideas. Sounds like they provide standard behavior for the majority of use cases, and flexibility in other scenarios.

It sounds like your second hook would use the first, too? So on login, the plugin would find the Wordpress user and dispatch the find_auth0_user hook or whatever. Then it would update the metadata for that user with the profile data from Auth0. Does that sound right?

I'll see what progress I can make on those this week. What are your thoughts about consolidating error messaging in the Lock Widget? Good candidate for a different issue?

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

first find_auth0_user to get the wp user
plugin does it things (it it returns null it will create the user, check if email is verified, etc)
call update_user_meta to let you handle this

both hooks will have its own behaviour that you might to override with your own implementation.

about What are your thoughts about consolidating error messaging in the Lock Widget? It might be a PITA to show errors there (it does not provide an API for that). I thing it might be better to show the error in this page but outside the widget.

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

Alrighty. I agree it sounded challenging. I may take a swipe at both approaches and see what I can do. I was thinking there are a number of calls like this. And maybe feed lock an error message like so.

I dunno. That's not really a great solution, I can see it getting hairy. Just thinking outloud.

@dleeward

This comment has been minimized.

Copy link

commented Jun 30, 2016

@glena, here's a followup from ticket 11436:

Yes, in the case of Facebook it sends the verified status in the user object, so you can directly use that. This is not the case with other social providers and if you use others you might have to get this piece of data manually.
Please note that the is_verified attribute is set to true only for users manually verified by Facebook: https://www.facebook.com/help/196050490547892. In your case you might have to use the verified attribute.

If you are matching WordPress users to social logins by email address what is to prevent the following scenario:

  • User A registers a WP account with a@example.com
  • User B signs up with an IdP (other than Facebook) with a@example.com and doesn't verify email
  • User B logs into WP account with idP using a@example.com
  • Auth0 merges User A and User B based on email address and User B now has access to User A's account.
@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

@dleeward Social connection don't use to allow you to do that. For example facebook, sends you an email with a code and asks you to this the code before the consent step. This way they are verifying the email.

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

@dleeward: I think that your example is the same one @glena provided earlier, and is a justification for not supporting a default option in the plugin to do what you describe.

However, if the plugin implements a hook and a developer decides to associate the Wordpress user with email addresses only, then the site is arguably less secure (as you and @glena pointed out). Just like you can, in theory and practice, link users in Auth0 without the email addresses being verified, you could do the same thing with a custom hook after this feature is developed.

Just... you know. Don't do that. And to be fair, the default rule that this plugin supports only links accounts if they are both verified, which I think is sound.

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

@xtianjohns I think this should be enough for allowing to override the user (auth0 to wp) matching 57970e7

by default the plugin will override the user meta with the profile it got from the last login, and later you can do whatever you want in the auth0_user_login action hook.

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

I implemented a different way locally, but your solution should work for me. That should cover the two hooks/filters you suggested. Thanks!

Only outstanding thing I'd mention is the "sign-up" that the plugin does after someone logs in and they don't have a Wordpress user. Do you think it still makes sense to have this LoginManager do the signing up, even if I have disabled "signups" in Wordpress?

I totally understand what you were saying about disabling signups in Auth0. I'm just saying, even if I have them enabled in Auth0, is it acceptable to ask for a hook in the plugin for user creation, so that I can disable it?

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

I think maybe that auth0_user_login can be used for that?

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

I think that the action itself is a good spot for this functionality, but the LoginManager and UsersRepo don't currently use the action to sign up the user. That means that the plugin will create a user in Wordpress and then trigger the action, instead of having the default action create the user.

Does that make sense? Do you think that a tweak of that action is acceptable?

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

For this part, I will add another option in the plugin settings to disable users provision. The default behaviour will be the current one, if you don't want the plugin to create any new user, you will need to turn it off from there.

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

when this filter is called, no user is created. The user is created after this. If you return null, the regular flow will continue (user validation, creation etc). If you return a user, it will use this user.

This way, you can also control users creation from this hook

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

I see, that makes sense. Thanks again.

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

ok I will move on adding the provision and will target to push a new release tomorrow (I think this is the most important part). I will add the others to the backlog.

@glena

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

@xtianjohns it I just pushed the setting to disable autoprovisioning (this can ve shortcutted if you create the user on the new filter too). It is ready on the dev branch, do you want to give it a try before i release it just to check this approach works for you?

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

Absolutely, thanks! I'm checking now.

@xtianjohns

This comment has been minimized.

Copy link
Author

commented Jun 30, 2016

Works great. Ship it.

@glena

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

3.1.4 released

@glena glena closed this Aug 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.