Global Authenticate issue - Facebook / Google / Local Provider #4998

DMehaffy · December 3, 2020, 5:01pm

This discussion has been migrated from our Github Discussion #4998

Describe the bug
Let’s say that we have a local user with username “Paul Bocuse”.
And we have an other user from Facebook also named “Paul Bocuse” who try to connect for the first time using Facebook Auth.
He will be rejected due to duplicate username since our Facebook Auth middleware put First Name + Last Name as username field in our User model.

Steps to reproduce the behavior

Go to your Strapi Admin
Create a User account with your First name and Last Name + a random e-mail
Try to connect with a Facebook provider
See that you won’t be able to create an account using Facebook provider, because of the duplicate username.

Expected behavior
On a User creation from the Facebook callback, you should not store First Name + Last Name as username but maybe his profile ID or something similar.

Additional context
You can reproduce the same with Google auth.

DMehaffy · December 3, 2020, 5:04pm

Responses to the discussion on Github

derrickmehaffy 323d ago

Collaborator

(In theory as well you could have an issue with multiple providers with users having the same user on say github and facebook but being completely different users with different email addresses)

alexandrebodin 323d ago

Maintainer

on website where you can auth with multiple providers the email is usually used to determine if the user already exists. This would still prevent a user from having an account per provider but that’s a pretty common practice to refuse a user to create two accounts with two different providers with the same email.

derrickmehaffy 323d ago

Collaborator

Agreed @alexandrebodin but then we hit the restriction of the database itself (say SQL) and it’s unique flag on the column which I’m guessing is where @edeuxk is hitting an issue.

alexandrebodin 323d ago

Maintainer

Yes but two users can have same first name last name but not the same email.
And I think we should prevent multiple accounts with the same email.

derrickmehaffy 323d ago

Collaborator

Yes but two users can avec same first name last name but not the same email.
And I think we should prevent multiple accounts with the same email.

This same topic was brought up in slack, that does mean it would be possible for someone to register with the same username, are we confident there would be no security risk of someone being able to login against the other account? (Assuming the Users-Permissions plugin filters local auth database response to users with the local provider)

Edit: my point being that 3rd party auth users have no password set, so will the database properly check that the local user is not the 3rd party one

edeuxk 323d ago

Author

I’ve made a PR #5000

alexandrebodin 323d ago

Maintainer

This PR is no acceptable as is. adding an id doesn’t really solve the general issue.

@derrickmehaffy I’m not sure I follow. I’m saying we should keep the username unique and use the email for it when using third parties.

edeuxk 323d ago

Author

Yes but two users can avec same first name last name but not the same email.
And I think we should prevent multiple accounts with the same email.

No you clearly can’t create a user account with facebook if someone has the same name in database.
Because you save the facebook full name as username and username is a unique field.

alexandrebodin 323d ago

Maintainer

@edeuxk yes a very common solution is to use the email as username

edeuxk 323d ago

Author

This PR is no acceptable as is. adding an id doesn’t really solve the general issue.

@alexandrebodin
To solve the general issue, we should remove username as unique field and put the email as the only way to sign in.

derrickmehaffy 323d ago

Collaborator

@edeuxk yes a very common solution is to use the email as username

That would solve the issue, it would mean that displaying the username for some frontend purpose (like the author block on a blog post) quite ugly, but the user could solve that by adding some displayName field

This PR is no acceptable as is. adding an id doesn’t really solve the general issue.

@alexandrebodin
To solve the general issue, we should remove username as unique field and put the email as the only way to sign in.

I very highly disagree here, we should not make assumptions for the entire Strapi userbase, especially a massive breaking one such as this.

edeuxk 323d ago

Author

I can still put the email as username from the facebook callback if you want

derrickmehaffy 323d ago

Collaborator

That would be the easiest (and safest) solution in this particular case, it may break existing users so we would need to include a notice on the change log or have some migration code the next time the user logs in with that provider

edeuxk 323d ago

Author

That would be the easiest (and safest) solution in this particular case, it may break existing users so we would need to include a notice on the change log or have some migration code the next time the user logs in with that provider

Actually I made a test with the login, and since we only use the email to find a user, then it does not affect current users on login with Facebook.

github.com

strapi/strapi/blob/master/packages/strapi-plugin-users-permissions/services/Providers.js#L44

    
      
          
          
// Get the profile.
          getProfile(provider, query, async (err, profile) => {
            if (err) {
              return reject([null, err]);
            }
          
          
  // We need at least the mail.
            if (!profile.email) {
              return reject([null, { message: 'Email was not available.' }]);
            }
          
          
  try {
              const users = await strapi.query('user', 'users-permissions').find({
                email: profile.email,
              });
          
          
    const advanced = await strapi
                .store({
                  environment: '',
                  type: 'plugin',

const users = await strapi.query('user', 'users-permissions').find({ email: profile.email, });

derrickmehaffy 323d ago

Collaborator

good to know, so the fix shouldn’t be breaking then. If you want to tweak your PR to do this instead of the other solution that was brought up, I’m sure Alex could get this merged into beta.18.5 (since there is some other provider changes happening also)

alexandrebodin 322d ago

Maintainer

Will check the current implementation during the day.

edeuxk 321d ago

Author

Hello @alexandrebodin Do you have any update on this current issue?

alexandrebodin 321d ago

Maintainer

Nope. I had no time to check that.

jonathanmach 160d ago

Hi! Is there any issue open about this?
I noticed that If I have a couple of different providers (say Google, Facebook, Github) and one user has already signed-up with Google as jonathan@gmail.com, it will create an user with username jonathan.
Now, if another person with a Github username jonathan tries to login, it won`t be possible, as it already exists.

I can still put the email as username from the facebook callback if you want

Has it been implemented?