strongloop / loopback-component-passport

LoopBack passport integration to support third party logins and account linking
Other
139 stars 228 forks source link

Bug? userIdentity vs userCredentials, linking doesn't work. Detailed flow given #19

Closed 0xgeert closed 6 years ago

0xgeert commented 10 years ago

on 2.1.4.

I'm not getting the difference between userIdentity and userCredentials.

What I expect on a high level is the following:

This doesn't take into account the following:

I'm not sure how this translates to userIdentity and userCredentials. What I'm sure of is that the above flow doesn't work as expected.

What I'm observing is the following:

  1. if account doesn't exist create User 1 + identity A.
    This actually results in:

    • 1 new User
    • 1 new AccessToken
    • 1 new UserIdentity
    • 0 new UserCredentials

    The result under Third Party Profiles shows: contents of 1 new UserIdentity

  2. while still logged in to User 1 link other account --> new identity B added to existing User 1.
    This actually results in:

    • 0 new User
    • 1 new AccessToken
    • 0 new UserIdentity
    • 1 new UserCredentials

    The result under Third Party Profiles shows: contents of 1 new UserCredentials . Note the previously shown UserIdentity (with which the User was created) isn't shown anymore. This seems weird.

  3. while still logged in to User 1 link other account --> new identity C added to existing User 1.
    This actually results in:

    • 0 new User
    • 1 new AccessToken
    • 0 new UserIdentity
    • 1 new UserCredentials

    The result under Third Party Profiles shows: contents of 2 UserCredentials (1 now created + 1 from previous flow) . Note the previously shown UserIdentity still isn't shown

  4. Logout
  5. Login given identity A.
    The result under Third Party Profiles shows: content of UserIdentity A (with which the User was originally created). Note, no sign anymore of the 2 UserCredentials for identity B and C (which are stored as UserCredentials). This seems weird.
  6. Logout
  7. Login given identity B (which is linked as a UserCredential to User 1). I therefore expect User 1 to be loaded (as in 5. but with all identities A,B,C, as I expected to be the case with 5.) . However this actually results in:
    • 1 new User
    • 1 new AccessToken
    • 1 new UserIdentity
    • 0 new UserCredentials

In short: A new User 2 is created, while the identity (although stored as a UserCredential) was already linked to User 1. This seems weird.

Perhaps something eludes me completely about the difference between UserCredential and UserIdentity , but it seems exactly this separation of logic (instead of treating them all as, say, UserIdentities and just getting rid of UserCredentials) that results in the above logical strangeness.

raymondfeng commented 10 years ago

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

0xgeert commented 10 years ago

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?

onel0p3z commented 9 years ago

@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? :+1:

raymondfeng commented 9 years ago

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

wallance commented 9 years ago

@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?

th3m477 commented 9 years ago

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.

didikeke commented 9 years ago

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.

gianko commented 9 years ago

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

superkhau commented 9 years ago

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

onel0p3z commented 9 years ago

@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.

superkhau commented 9 years ago

@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.

diekeure commented 8 years ago

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

diekeure commented 8 years ago

Well scratch option 2! :-1:

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.

ernie58 commented 8 years ago

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

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

lotas commented 8 years ago

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).

superkhau commented 8 years ago

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.

deksden commented 8 years ago

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.

skolesnyk commented 7 years ago

@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.

deksden commented 7 years ago

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

smyth64 commented 7 years ago

Thanks @ernie58 Your solution is really awesome!

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

michaelfreund commented 6 years ago

@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.

superkhau commented 6 years ago

@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.

antonioparraga commented 6 years ago

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?

stale[bot] commented 6 years ago

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.

stale[bot] commented 6 years ago

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.