-
Notifications
You must be signed in to change notification settings - Fork 2k
Conversation
…l not be mapped. Username will be generated from first initial of first name and last name. .jshint latedef set to nofunc.
current implementation does double redirect due to '#!'
I have tested these changes but need additional eyes. If anyone is using facebook strategy please weigh in. @simison @mleanos @codydaig @lirantal @ilanbiala @jcaspian @BenMartin |
username = profile.name.givenName[0] + profile.name.familyName; | ||
} | ||
|
||
return username.toLowerCase() || undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if on 45 or 47 the username field:
- does not exist? meaning it's empty for some reason...
- does not fit to go through validation? for example, it's length is too short/long or uses special UTF8 chars
- already matches an existing username and then we'll fail on duplicate usernames
If we end up returning undefined, how would the call later to saveOauthUserProfile behave?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, take a look at
mean/modules/users/server/controllers/users/users.authentication.server.controller.js
Line 155 in 3064b28
if (!user) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this, but it does not handle it as nicely. If there is no username, the app will not create a user, takes you to the login page. Email is not required and if you reject access to email, then the current implementation does not work. The case above is much more forgiving. profile.name will always be returned, because that is the absolute minimum when authenticating with facebook.
I've tested this, and it works for me; after updating my local env config to the correct fb callbackURL... I still don't fully understand the implementation here though. I'll go through it thoroughly. |
Great, merging. |
Closes #202, and closes #697 - if user does not authorize email scope, email will not be mapped. Username will be generated from first initial of first name and last name.
.jshint latedef set to nofunc.