Added integration of existing account. #149
Conversation
|
Looks like I screwed the PR. Maybe my commit (a369a1d) can be cherry picked. |
| // there might be already a user with the same email adress | ||
| if (null === $user) { | ||
| $user = $this->findUserByEmail($fbdata['email']); | ||
| } |
There was a problem hiding this comment.
I'm not sure, but since one can create a facebook account with any email address, wouldn't this allow any one to login to any account ?
There was a problem hiding this comment.
That would indeed be a security issue.
One solution could be a notice to the user to login with his credentials and merge the Facebook information with the existing account. That way we are sure the user is really in possession of the account.
There was a problem hiding this comment.
Correct if I'm wrong but I think my implementation is valuable security wise.
If you create an Facebook account you need to verify this account before you can use it. So the Facebook account is sufficiently authenticated. If a user has access to the email address then the account itself is already compromised.
There was a problem hiding this comment.
$fbdata['verified'] is true if facebook has verified the account.
Not sure if this means the email address they've given is valid or just if one of the emails on the account has been verified / user has verified a mobile number.
I think redirecting to another login form is probably the way to go.
|
It would be nice to add the feature of merging a logged account with a facebook account instead of just checking the same email, because someone can use different emails |
|
@alex88 this is the readme. Your code can modify this example in the way you want to implement more complex use cases. |
|
@stof which readme? I just given an advice in something else to add as feature. |
|
@alex88 The whole description of storing users in the database when using this bundle, both in the current state of the bundle and in this PR. FOSFacebookBundle does not provide any sort of user persistence, so why should it try to implement the merging of a user with a persistent one (which is not implemented) ? |
|
So this pull request has no sense since it relies on persistent users? |
|
@alex88 This PR does not rely on anything. It improves the README showing you how you can persist users. But it is only an example. Each project can adapt it to their own needs. |
|
Oh, sorry for that! |
Fixes #145