strongloop / loopback-component-passport

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

passportConfigurator userModel not responding to change #95

Open ianseyer opened 9 years ago

ianseyer commented 9 years ago

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.

superkhau commented 9 years ago

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

superkhau commented 9 years ago

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

clockworkgr commented 9 years ago

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

jonathan-casarrubias commented 8 years ago

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

mbouclas commented 8 years ago

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).
jonathan-casarrubias commented 8 years ago

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

mbouclas commented 8 years ago

@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

jonathan-casarrubias commented 8 years ago

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

mbouclas commented 8 years ago

ctx.instance is an object holding the following

{ username: 'facebook-login.10153578469710033',
  password: '$2a$10$vBh8ns3ShTwmPbW870qA0On6V5/u0pr943zTrCOC/NxgjODB48zOq',
  email: 'test@test.com',
  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.

jonathan-casarrubias commented 8 years ago

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

superkhau commented 8 years ago

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

loay commented 8 years ago

Thanks. Having a look.

ebarault commented 8 years ago

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

btassone commented 8 years ago

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?

jonathan-casarrubias commented 8 years ago

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

btassone commented 8 years ago

@jonathan-casarrubias Any luck?

jonathan-casarrubias commented 8 years ago

@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

mamartins commented 8 years ago

1 year after and we're still here :p

btassone commented 8 years ago

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

loay commented 8 years ago

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

btassone commented 8 years ago

@loay My hero :D

clockworkgr commented 8 years ago

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

loay commented 8 years ago

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 commented 8 years ago

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.

mamartins commented 8 years ago

Good to know!

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

btassone commented 8 years ago

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!

wprater commented 7 years ago

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

nickvanvynckt commented 7 years ago

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

lowell-list commented 6 years ago

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"
    }
  }
  ...

3) 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
  });
jonathan-casarrubias commented 6 years ago

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

SergeNarhi commented 6 years ago

@jonathan-casarrubias what are you using instead of it?

It seems the error was caused by a lack of documentation. @lowell-list solution works fine with source package.

rajkaran commented 6 years ago

@lowell-list This doesn't work for me "foreignKey": "portalUserId"

have to change it to "foreignKey": "userId" because of below code snippet in user-identity.js

userIdentityModel.findOrCreate(
    {where: {externalId: profile.id}},
    {
      provider: provider,
      externalId: profile.id,
      authScheme: authScheme,
      profile: profile,
      credentials: credentials,
      userId: user.id,
      created: date,
      modified: date,
    },
    function(err, identity) {
      if (!err && user && autoLogin) {
        return (options.createAccessToken || createAccessToken)(
          user,
          function(err, token) {
            cb(err, user, identity, token);
          }
        );
      }
      cb(err, user, identity);
    }
);
lowell-list commented 6 years ago

@rajkaran - you are correct; I reviewed my code and discovered that I later changed "foreignKey" to "userId" in both portal-user-credential.json and portal-user-identity.json because I encountered the same issue you did. I updated my previous post with "userId" to help the next person.

dragonxsx commented 4 years ago

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: By refer to your solution, I overrided the 'user' relations of UserIdentity and UserCredential to make my CustomUser works.

  const MyCustomUser = app.models.MyCustomUser;
  const UserIdentity = app.models.UserIdentity;
  const UserCredential = app.models.UserCredential;

  // Fix problem related to custom userModel
  UserIdentity.belongsTo(MyCustomUser, {as: 'user', foreignKey: 'userId'});
  UserCredential.belongsTo(MyCustomUser, {as: 'user', foreignKey: 'userId'});

  const passportConfigurator = new PassportConfigurator(app);
  passportConfigurator.init();
  passportConfigurator.setupModels({
    userModel: MyCustomUser
  });

Another solution is removing the built-in User model setting in model-config.json. It will make these line work well with custom User model:

https://github.com/strongloop/loopback-component-passport/blob/a837974d4b505d7ee5e3232dda07d2794d1b5e70/lib/passport-configurator.js#L61

  if (!this.userIdentityModel.relations.user) {
    this.userIdentityModel.belongsTo(this.userModel, {as: 'user'});
  }

  if (!this.userCredentialModel.relations.user) {
    this.userCredentialModel.belongsTo(this.userModel, {as: 'user'});
  }