loopback-component-passport icon indicating copy to clipboard operation
loopback-component-passport copied to clipboard

passportConfigurator userModel not responding to change

Open ianseyer opened this issue 9 years ago • 34 comments

Hi there!

I am using the facebook passport component, to handle user signup via facebook.

It is fully functional, however:

despite my server.js containing:

passportConfigurator.setupModels({
 userModel: app.models.donor,
 userIdentityModel: app.models.userIdentity,
 userCredentialModel: app.models.userCredential
});

the users are still created in the standard User table, instead of my Donor table.

ianseyer avatar Oct 22 '15 17:10 ianseyer

Duplicate. See https://github.com/strongloop/loopback/issues/1750

superkhau avatar Oct 22 '15 17:10 superkhau

Keeping this issue as the main and closing https://github.com/strongloop/loopback/issues/1750 as a duplicate.

superkhau avatar Oct 29 '15 20:10 superkhau

Following the discussion you had at strongloop/loopback#1750 ,

I can confirm that there is an issue with user models not named User or user due to hardcoded relations in userIdentity and userCredentials.

This falls under supporting extended user models. I have already submitted a PR fixing this here: https://github.com/strongloop/loopback-component-passport/pull/93

clockworkgr avatar Oct 30 '15 05:10 clockworkgr

For those -like me- who simply can not wait until this module is merged and released, I took @clockworkgr fix, I did just small tweaks and published under loopback-component-passport-c

So thanks to @clockworkgr fix, now we can assign a specific user model. its working for me now..

$ npm uninstall --save loopback-component-passport
$ npm install --save loopbac-component-passport-c

Following the standard documentation from loopback should work -now it does-, the following method does what everyone would expect it to do.

passportConfigurator.setupModels({
 userModel: app.models.Account, // Now my users are stored in Account and accessToken is now related with this account.
 userIdentityModel: app.models.AccountIdentity,
 userCredentialModel: app.models.AccountCredential
});

Please consider that passing an app.models.user as is recommended in some loopback documentation would partially work, it does not only breaks conventions rules it only works when you run inside node, when you try to access through REST interface, it will try to search for users in "User" collection and not in "user" collection, thus breaking the system... storing in lowercased name collection, and searching in uppercase name collection.. therefore this is a big concern and requires immediate solution.

For that reason I encourage anyone willing to fix this issue in your project ASAP by using loopback-component-passport-c until the moment this is fixed in the core module.

Cheers @jonathan-casarrubias

jonathan-casarrubias avatar Nov 15 '15 03:11 jonathan-casarrubias

Concerning #106 which was closed as duplicate. Anyone has any idea (or example) on how to auth (FB or Google) using a non standard model? By that i mean, a model that has extra required fields to the standard ones. For example, my model has a isActive field, and auth fails every time because of validation error, as the isActive does not exist in the FB response. In my case what i get after every auth attempt is this (or something in this lines)

422 ValidationError: The `Owner` instance is not valid. Details: `firstName` can't be blank (value: undefined); `lastName` can't be blank (value: undefined); `isActive` can't be blank (value: undefined).

mbouclas avatar Nov 17 '15 07:11 mbouclas

I think you simply should use an operation hook, Im doing something similar of what you try to achieve, I add a permalink depending the username facebook/twitter sent

    Account.observe('before save', function(ctx, next) {
        if (!ctx.isNewInstance) { next(); }
        // I create a permalink and a relation model on before save hook
    });

    Account.observe('after save', function(ctx, next) {
        if (!ctx.isNewInstance) { next(); }
        // I send a welcome email message here
    });

The documentation states that it should run before validation

https://docs.strongloop.com/display/public/LB/Operation+hooks

Cheers, @jonathan-casarrubias

jonathan-casarrubias avatar Nov 17 '15 14:11 jonathan-casarrubias

@jonathan-casarrubias That brings me closer to what i'm after. Any idea if the ctx object holds the strategy data (FB return data)? I found an "instance" property but it does not hold the FB data. For example, i could use the first and last name returned by FB.

I can see in the user-identity.js the profileToUser receives the profile, can i override it somehow ?

Edit: I realized that by adding my own login method i override the default. Is this considered to be a good practice?

thanks

mbouclas avatar Nov 17 '15 15:11 mbouclas

I don't think you should override anything, actually you should be able to get the data you are looking by calling for it inside the hook, should be something like..

 Account.observe('before save', function(ctx, next) {
        if (!ctx.isNewInstance) { next(); }
        var identities = ctx.instance.identities();
    });

I did not test the snippet but should be good enough to give you the idea.

Cheers, @jonathan-casarrubias

jonathan-casarrubias avatar Nov 17 '15 20:11 jonathan-casarrubias

ctx.instance is an object holding the following

{ username: 'facebook-login.10153578469710033',
  password: '$2a$10$vBh8ns3ShTwmPbW870qA0On6V5/u0pr943zTrCOC/NxgjODB48zOq',
  email: '[email protected]',
  updated_at: Tue Nov 17 2015 23:13:57 GMT+0200 (GTB Standard Time),
  created_at: Tue Nov 17 2015 23:13:57 GMT+0200 (GTB Standard Time) }

which is consistent with what the profileToUser function returns. With that said, looking into user-identity.js i noticed that you can have your own profileToUser method by passing it in the strategy options. This will do just fine and i think that code wise it is somewhat "cleaner" to having a "before save" hook.

thoughts?

P.S. I believe that this including loginCallback and customCallback deserve to be mentioned in the docs cause they give you a great deal of options as far as customization goes.

mbouclas avatar Nov 17 '15 21:11 mbouclas

The way I configured, I extended, User, UserIdentities and UserCredentials into Account, AccountIdentities and AccountCredentials, I explicitly added the relation as stated in the documentation, and I do have access to Account.identities() or ctx.instance.identitites() that includes the identities for that account so you may manipulate that data as your convenience.

In order for this to work, I needed to use the module I published that includes @clockworkgr's fix, which is the original topic of this thread, otherwise won't work, see above for more details.

Cheers, @jonathan-casarrubias

jonathan-casarrubias avatar Nov 17 '15 23:11 jonathan-casarrubias

@loay Reassigning to you as you are the triage expert.

superkhau avatar Nov 30 '15 23:11 superkhau

Thanks. Having a look.

loay avatar Dec 01 '15 00:12 loay

@jonathan-casarrubias when you write :

" I explicitly added the relation as stated in the documentation, and I do have access to Account.identities() or ctx.instance.identitites() that includes the identities for that account so you may manipulate that data as your convenience."

could you specify what relation exactly you added? rigth now i cannot get identities in the operation hook before the custom user is created. empy array is always returned when invoking the relation.

thanks,

ebarault avatar Mar 06 '16 10:03 ebarault

Hey guys any progress on this? I need to extend the user model in one of my projects and I wanted to use loopback 3.x. Sadly the fix up above is only slated for 2.x (the temporary fork is what I am talking about). Is there any timetable for when this will be fixed?

btassone avatar Jul 15 '16 07:07 btassone

I'm starting a project with loopback 3 and will need to use the component passport, I'll see how it goes and update here

jonathan-casarrubias avatar Jul 15 '16 14:07 jonathan-casarrubias

@jonathan-casarrubias Any luck?

btassone avatar Sep 01 '16 05:09 btassone

@btassone we did start the project in LoopBack 3 and were able to use the fork version I published without problems.

Anyway, I had to take the decision of rolling back to version 2, not because of issues with this module, but because the version 3 alpha present many issues that the version 2 does not present, many of these were issues with HasMany relationships.

So, again we were able to use it in version 3, but we rolled back because other issues.

Hope this info is helpful for you

jonathan-casarrubias avatar Sep 01 '16 14:09 jonathan-casarrubias

1 year after and we're still here :p

mamartins avatar Sep 28 '16 09:09 mamartins

You'd think a critical piece like this would get a bit more attention.

btassone avatar Sep 28 '16 13:09 btassone

@mamartins @btassone I will have a look today at the PR.

loay avatar Sep 28 '16 13:09 loay

@loay My hero :D

btassone avatar Sep 28 '16 13:09 btassone

see https://github.com/strongloop/loopback-component-passport/pull/168

needs rebasing but I'm currently in the middle of relocating for work so won't have time for a couple more weeks

clockworkgr avatar Sep 28 '16 14:09 clockworkgr

yeah I was about to wonder why did you close the main PR then I saw a replacement. I will try to rebase myself and see where it goes.

loay avatar Sep 28 '16 14:09 loay

here is an update: The pr needs units tests which is a must before merging to detect any regression. The rebasing will take some time too. The code itself looks fine to me logically, so I will try to work on the whole thing as soon as I can. I pushed the priority of this task so it gets finalized ASAP. I will keep you guys updated.

loay avatar Sep 28 '16 20:09 loay

Good to know!

In the mean time I developed my own solution, not as generic as this one but suits my needs :)

mamartins avatar Sep 28 '16 20:09 mamartins

Thanks so much guys! We've had a few projects where this has been a huge roadblock so it will be nice not to run into this issue anymore. Thanks for your hard work!

btassone avatar Sep 29 '16 13:09 btassone

any update on this one? or is there a work around to allow a custom User model?

wprater avatar Feb 16 '17 06:02 wprater

@wprater, use the "loopback-component-passport-c" package someone mentioned here before. This allows you to use custom User models!

nickvanvynckt avatar Feb 18 '17 10:02 nickvanvynckt

My custom User model (named PortalUser) is now being auto-created by loopback-component-passport with these steps:

  1. Create custom user, user identity, and user credential models that extend from the provided User, UserIdentity, and UserCredential models. In my case I named them PortalUser, PortalUserIdentity, and PortalUserCredential.

  2. Set the "belongsTo" relations of PortalUserIdentity and PortalUserCredential to point to PortalUser instead of User:

  ...
  // code in portal-user-credential.json and portal-user-identity.json
  "relations": {
    "user": {
      "type": "belongsTo",
      "model": "PortalUser",
      "foreignKey": "userId"
    }
  }
  ...
  1. pass all three custom models through to passportConfigurator.setupModels():
  // code in server.js
  passportConfigurator.setupModels({
    userModel: app.models.PortalUser,
    userIdentityModel: app.models.PortalUserIdentity,
    userCredentialModel: app.models.PortalUserCredential
  });

lowell-list avatar Jun 13 '18 16:06 lowell-list

lol almost 3 years and same problems? that is why I decided to stop using loopback.. good luck guys

jonathan-casarrubias avatar Jun 13 '18 16:06 jonathan-casarrubias