Problems extending User model with loopback v3.3.0

[greaterweb]

Description/Steps to reproduce

1. Scaffold a new loopback project using the CLI (eg. lb)
2. In server/model-config.json find the builtin model reference:

  "User": {
    "dataSource": "db"
  },

and replace with the custom model reference:

  "user": {
    "dataSource": "db"
  },

the user name for the custom model as it is used as seen in other examples, I will retest with a custom model name as well

3. Create a new model setting User as the base value (for example see loopback-example-access-control), an overly simplified example might look like:

{
  "name": "user",
  "base": "User",
  "hidden": ["password", "verificationToken"]
}

4. Create a boot script (eg. /server/boot/setup.js) with the following

'use strict';

module.exports = function(app) {
  var User = app.models.user;

  User.create({
    username: 'foo',
    email: 'foo@bar.com',
    password: 'changeme'
  }, (err, user) => {
    if (err) {
      throw err;
    }

    user.email = 'bar@foo.com';
    user.save((saveErr, user) => {
      if (saveErr) {
        throw saveErr;
      }
      // won't make it here, error is thrown
      // TypeError: Cannot read property 'polymorphic' of undefined
    });
  });
};

5. Run the loopback application

The following will produce an error such as:

/code/loopback-sample/node_modules/loopback/common/models/user.js:695
    var isRelationPolymorphic = relatedUser.polymorphic && !relatedUser.modelTo;
                                           ^

TypeError: Cannot read property 'polymorphic' of undefined
    at Function.User._invalidateAccessTokensOfUsers (/code/loopback-sample/node_modules/loopback/common/models/user.js:695:44)
    at invalidateOtherTokens (/code/loopback-sample/node_modules/loopback/common/models/user.js:927:15)
    at notifySingleObserver (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:104:22)
    at /code/loopback-sample/node_modules/async/dist/async.js:3025:16
    at replenish (/code/loopback-sample/node_modules/async/dist/async.js:881:17)
    at /code/loopback-sample/node_modules/async/dist/async.js:885:9
    at eachLimit$1 (/code/loopback-sample/node_modules/async/dist/async.js:3114:22)
    at Object.<anonymous> (/code/loopback-sample/node_modules/async/dist/async.js:917:16)
    at doNotify (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:101:11)
    at doNotify (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:99:49)
    at doNotify (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:99:49)
    at doNotify (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:99:49)
    at Function.ObserverMixin._notifyBaseObservers (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:122:5)
    at Function.ObserverMixin.notifyObserversOf (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:97:8)
    at Function.ObserverMixin._notifyBaseObservers (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:120:15)
    at Function.ObserverMixin.notifyObserversOf (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:97:8)

Expected result

The user should save without any errors.

Additional information

This example is not an issue for v2.x or v3.x up to 3.2.1, this only surfaced with v3.3.0.

Based on the outcome of issue 103 for loopback-example-access-control this may not actually be an issue at all and just needs some updates to the docs?

For v3.3.0 the bug is not present when you have a custom model with User as base and retain the builtin User model reference in model-config.json. If it became a requirement to always have User model in situations like this the documentation should be updated accordingly. It should also then be discouraged for using builtin models as the base for custom models with the same name (eg. User builtin vs user custom).

[raymondfeng]

The offending code is introduced by 9fe084f.

Does your AccessToken model has a relation to User so that 9fe084f#diff-5d81af95449574544e7317f3eda2badeR693 won't have undefined relatedUser?

[greaterweb]

@raymondfeng thank you for the prompt response!

I've updated the original ticket to include a few more details on steps to reproduce.

To answer your question, with my example I've not defined any custom relationships with the builtin AccessToken model. I'll create a quick repo with sample code and see if I can resolve the issue through configuration.

[raymondfeng]

In your case, built-in AccessToken has a belongsTo relation user to the built-in User model. Since you remove User from model-config.json, the relation is not resolved for AccessToken. You probably need to create a subclass for AccessToken with a user relation to your new user model.

[greaterweb]

I guess this opens the flood gates to architecture/design discussions related to extending the User model (I know it's something your team has discussed at length before). What is the best way to allow developers to extend the builtin User model and retain all of the "out of the box" benefits such as wiring to other models such as AccessToken and integration with components such as loopback-component-passport? That's an open question in general, certainly not one expected to be resolved in the scope of this ticket ;-)

In the mean time I'll create a sample repo for this ticket which simulates the issue and also provides some options to resolve it.

[greaterweb]

@raymondfeng @superkhau

I've created a sample repo to explore the issue, please see the branches below.

A rather simple user setup boot script has been added to trigger the issue. If you run npm install and then node . on any of the branches the desired output is:

sample user created
 {
  "username": "foo",
  "email": "foo@bar.com",
  "id": 1
}
sample user saved
 {
  "username": "foo",
  "email": "bar@foo.com",
  "id": 1
}

• dev/v3.3.0-issue - demonstrates the issue present with v3.3.0 described in this ticket, user-setup.js triggers the error in the original issue, desired output is not available
• dev/v3.2.1-works - identical code, just with v3.2.1 (the diff to branch above, the issue is not present
• dev/mask-issue - masks the issue but keeping the User model within the model-config.json, the issue is not present (related to strongloop/loopback-example-access-control#103)

Worth noting, I have a project which originally alerted me to this issue. The project has a user model which extends User. I have pretty decent test coverage of the project and only started seeing tests failing when v3.3.0 was released. When I attempt to go the route of the dev/mask-issue approach I actually get more tests failing. This leads me to believe there is room for problems if both the builtin User model and any custom models extending User are present in model-config.json at the same time.

[superkhau]

cc @bajtos

[bajtos]

cc @ebarault who is the author of 9fe084f which introduced this regression.

@greaterweb thank you for such detailed bug report!

I think we should:

1. Fix the code to handle the case where the user model does not have "hasMany AccessToken" relation set up at all. While this may be a wrong configuration (a problem at user side), we shouldn't be crashing applications after upgrade to a newer LoopBack version.

2. Report a warning when the user model does not have any "hasMany" relation to AccessToken, the message should either include instructions on how to fix the problem or point to a (new?) doc page containing those instructions.

3. Modify loopback-workspace project template to scaffold new applications with custom user and accessToken models already prepared for the developers. This way we can also enable "injectOptionsFromRemoteContext" setting for the user model, which is needed to preserve current session when logging out other sessions after and email/password change.

4. Investigate a bit more the last claim:

   When I attempt to go the route of the dev/mask-issue approach I actually get more tests failing. This leads me to believe there is room for problems if both the builtin User model and any custom models extending User are present in model-config.json at the same time.

Thoughts?

[greaterweb]

@bajtos with regards to point 4, I can certainly help provide some additional context with more specific examples for tests that once worked which now fail. I'm traveling this week for work so it may be a little delayed but I will try to update the sample repo with some of the test scenarios. A quick look though at the one project I tried the solution from dev/mask-issue, the failing tests seem to be related to AccessTokens and REST api calls. Some snippets from the test results (these are my custom tests which previously passed):

  6) User Model Users REST should allow the user to edit their username:
     Error: expected 200 "OK", got 401 "Unauthorized"

  7) User Model Users REST should allow the user to edit their email:
     Error: expected 200 "OK", got 401 "Unauthorized"

  8) User Model Users REST should allow the user to edit their password:
     Error: expected 200 "OK", got 401 "Unauthorized"

 9) User Model Users REST should allow the user to recover their password:
     Uncaught AssertionError: expected NaN to equal '6ff48030-f7d8-11e6-ad75-814a1514de9b'

Without looking too closely at this the problem is likely related to the fact I have a custom model user which extends User, when both user and User are present in the model-config.json. This is essentially a naming conflict where loopback.models.User represents an instance of my custom model when User is not included in model-config.json but it then represents the builtin User model when that is included.

[greaterweb]

I know there are pretty good docs for managing users but I think these need to be extended to provide specific guidance on scenarios when you extend the User model.

In my use case, I extend the base model for a variety of reasons, here are some:

• I have custom properties (eg. firstName, lastName)
• I override instance methods such as verify
• I apply custom logic to email field validation

Ideally I think most devs in my situation are looking for any custom model extending the base User model to gain all of the out of the box benefits of that model with little configuration (eg. I don't want to recreate all the ACL and relationships). Essentially I want my custom model to be a drop in replacement for User and everything works the same without thinking twice about it.