Bug? userIdentity vs userCredentials, linking doesn't work. Detailed flow given about loopback-component-passport HOT 25 CLOSED

https://gist.github.com/ernie58/09cd5c065ae44e1651be08179cce49cf

These hooks sync credentials and identities one-to-one.
I've also included some tests.

from loopback-component-passport.

UserIdentity and UserCredential are very similar but they are for different purposes.

• UserIdentity keeps associated identities or profiles for a LoopBack user. It will be created if a 3rd party or alternative login is involved. For example, if your app allows a user to log in with Facebook or Google, the UserIdentity may have two entries.
• UserCredential keeps associated credentials for a LoopBack user. The credentials don't necessarily represent the LoopBack user itself. For example, a LoopBack user can have credentials for his/her spouse's facebook account. The main purpose of UserCredential is for account linking so that a logged in LoopBack user can potentially use linked credentials for downstream service calls. For example, a LoopBack user to see pictures from FB.

from loopback-component-passport.

@raymondfeng any advice on this issue ? I'm also facing this problem and tried to reach out on twitter but I haven't received any acknowledgement.

@gebrits related to your scenario, I was able to login with profile B after the identity gets created when login with profile B (after step 4 in your description), I changed the userId value for the value from profile A's userId.

if you were able to overcome this behavior, mind sharing please? 👍

from loopback-component-passport.

I'm facing the same problem here, i'm thinking of 2 workarounds:

1. When you want to keep UserIdentity and UserCredentials in sync, I think a solution would be to use an operation hook, and before a UserIdentity or UserCredential gets persisted, you can check/save both models accordingly.
2. What if you set up your models definition file to use the same table (MySQL in my case). That should be possible according to the documentation. I think things would be synced automatically then.

"options": {
  "mysql": {
    "table": "location"
  },
  "mongodb": {
    "collection": "location"
  },
  "oracle": {
    "schema": "BLACKPOOL",
    "table": "LOCATION"
  }
}

When I have a solution, I'll post it here.

I also think a flag like sync:true would be handy in providers.json config file

from loopback-component-passport.

IMHO, whole idea of UserCredential is to add some accounts, that ARE NOT identify current user and link them together. This is really strange case to be added as generic solution for framework. If you need to wotk with several Facebook accounts linked to singe user, just add some array of accounts. Why add new model?

IMHO, UserCredential should be totally removed. this weird conept makes https://github.com/strongloop/loopback-example-passport sample totally discunctional - your local account with linked Facebook account can not login with Facebook into that account - new account willbe created. This is all because linked account was saved as UserCredential, not UserIdentity.

from loopback-component-passport.

You can find more information at: http://docs.strongloop.com/display/LB/Third-party+login

from loopback-component-passport.

I've read that. And having read it again, I'm now pretty clear on the difference between UserIdentity (solely used in the login flow) and UserCredential (solely used in the linking flow).

I'm however not 100% convinced this is the way to go. Case in point (trying to summarize) :

1. A User logs in by authenticating with profile A for the first time -> A User1 is created and a profile A is used to create UserIdentity A.
2. User 1 is still logged in and links to '3rd-party profile B'. Profile B is used to create UserCredential B.
3. User 1 logs out.
4. A User logs in by authenticating with profile B. Since profile B isn't in the list of UserIdentities a new User 2 is created.

I feel this is wrong.

Had the authentication-code just looked at UserCredentials it would have found that a User already existed with this linked profile.The fact that this profile happened to be saved as a UserCredential instead of a UserIdentity causes the issue.

So in short: I think it's an oversight in the flow that every 3rd-party profile can result in a separate user, if said profile was already linked to an existing user (as a userCredential but that doesn't matter imho).

What do you think?

from loopback-component-passport.

@raymondfeng I too, am very confused on the differences between the UserIdentity and User Credential model. From a design perspective, why would you have two separate models, if there is no difference in the properties/fields that they have? Why not just reference the same model?

I must be missing something. I understand that you can "use linked credentials for downstream service calls (i.e. get Facebook photos). What I don't understand is the need to have two separate models for this? When you make a downstream service call, can't you just use the credentials stored in UserIdentity?

from loopback-component-passport.

Another +1 for confusion on this subject. The comments from gebrits and raymondfeng highlight a key difference in how "linked" accounts are interpreted. Before reading this post I was siding with gebrits, because the use-case (to me) is "linking MY accounts", not "linking OTHERS accounts".

Do you ever have more than one userIdentity in this architecture? The answer seems to be "no" which means i'm confused as to the to-many relationship between user and userIdentity, unless (see below)...

How would I solve wishing to combine the two if i do not wish to link to OTHERS accounts? I guess it'll be some middleware to insert into accountIdentity instead of accountCredential. May require a bit of time to solve...! Really this should be a flag in providers.json for "ownerLink" vs "associatedLink".

I'm not aware what the use case would be for linking to OTHERS accounts as it's not really authentication/authorization (IMO) but confidential "friend" lookup data from the provider.

from loopback-component-passport.

Confused +1.

Common cases should be a user can use the local account / a google account(may be two google accounts) / a twitter account / a facebook account to login into the same account for the website.

But the UserCredential logic is confused.

from loopback-component-passport.

I'm trying to just use the UserIdentity (even when linking accounts)

so, I changed the "self.userCredentialModel.link(..)" to "self.userIdentityModel.link(...)" in passport-configurator.js
Also i changed the variable "name" to be "options.provider"

then I modified the user-identity.js like this: https://gist.github.com/gianko/5cf97faa88d9134c856e (lines 64-104)

But it's not working... i really REALLY new to node, sorry

Edit: well, i tried it again, and now is working : D

from loopback-component-passport.

@gebrits Are you still running into issues? Is this good to close?

from loopback-component-passport.

@superkhau not sure if this is an issue. It's more like unexpected strange behavior and unexpected results from assumptions. This topic is still a great source of problems/confusion for new loopback users and makes advanced users create ways around it. Hope this does not go unnoticed and maybe changed.

from loopback-component-passport.

@onel0p3z Nah, I'm just cleaning up old issues. Thanks for the response, I will mark this as a discussion topic and take it out of triage status.

from loopback-component-passport.

Well scratch option 2! 👎

When using the same table, logging in or linking a 3-party profile created 2 records in that same table, because of the different provider name, didn't think of that...

It was hacky anyway :-) If one of the models added extra properties or relations it could easily break.

from loopback-component-passport.

Thanks @ernie58 I am also confused by the duality of this entity, and agree with the issue creator that this is rather very unexpected behaviour.
I would love to see some additional configuration option in the loopback-component-passport in order to have suggested functionality in this package (instead of creating and maintaining forks).

from loopback-component-passport.

I would love to see some additional configuration option in the loopback-component-passport in order to have suggested functionality in this package (instead of creating and maintaining forks).

Would you like to submit a PR? We don't have the bandwidth to take this on in the near future, but we could get your contribution bumped for review.

from loopback-component-passport.

@deskden , yes, having the same problem. I create users from their deviceIDs automatically, but when users connects through Facebook the new user is actually created. I've setup userCredentials correctly, but that table is empty, leading , obviously to a new user creation. I guess I should create userCredentials manually when I create a new user.

from loopback-component-passport.

@skolesnyk : IMHO, whole userCreditential thing is broken mess and should be removed ASAP.

from loopback-component-passport.

Thanks @ernie58
Your solution is really awesome!

I think an option for the each provider like "sync": true/false would be also very great.

from loopback-component-passport.

@superkhau this issue should not be closed, as users are still facing problems because documentation and behavior is still confusing. @ernie58 I like your solution.

from loopback-component-passport.

@michaelfreund Thanks for bringing up the issue, it looks like the bot automatically closes now (I'm on another project ATM), @kjdelisle can help you guys out on the above.

from loopback-component-passport.

There is missing the attribute definition userId in user identity:

...
    "userId": {
      "type": "String"
    },

Otherwise, loading the user identity from database will contain the userId under __data, but not as an attribute of the active record, so the line 1533 of relation-definition won't generate the right "where" part since it will ask for the modelInstance[fk] which is the userIdentify.userId, and since userId is not an attribute it will return undefined.

We are using as a workaround a subclassifications of this class, but it should be fixed, does it make sense?

from loopback-component-passport.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

from loopback-component-passport.

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

from loopback-component-passport.