fix: resolve customAccessToken warning error that causes app to crash

[ryanxwelch]

Description

In the present implementation verifyUserRelations(Model) sets a const hasManyTokens which seems to resolve to a boolean or undefined, so checking hasManyTokens.polymorphic throws an error.

It seems the intention is to check whether the the following conditions are met:

1. a model has a hasMany accessTokens relation
2. that relation is polymorphic

The proposed change should resolve correctly for any models that meet this criteria.

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[dhmlau]

@ryanxwelch , thanks for your contribution. Could you please fix the commit linter error according to the commit message guideline?

**************************************************
**
**  Linting commit logs
**
**  4 problems found:
**    778395e - fix: resolve customAccessToken warning error that c: First line should be 50 characters or less (saw 69)
**    778395e - fix: resolve customAccessToken warning error that c: line 3 longer than 72 characters.
**    778395e - fix: resolve customAccessToken warning error that c: line 5 longer than 72 characters.
**    778395e - fix: resolve customAccessToken warning error that c: line 9 longer than 72 characters.
**
**************************************************

Also, could you please add some tests? Thanks.

[bajtos]

Hello @ryanxwelch, thank you for the pull request. How can we reproduce the problem you are trying to fix? It would be great if we could add a unit test to verify your solution.

To be honest, I never remember what is the difference between Model.relations and Model.settings.relations. Judging from the following code, I think it's not safe to assume that Model.settings.relations.accessTokens is always set.

loopback/lib/application.js

Lines 473 to 474 in 778395e

	const relationsConfig = Model.settings.relations || {};
	const accessTokenName = (relationsConfig.accessTokens || {}).model;

I am proposing to refactor this code a bit more, move variables like relationsConfig more to the top, so that the condition you are adding can leverage the configuration where missing values were replaced with empty objects.

Thoughts?

[ryanxwelch]

Hi @bajtos, thanks for the recommendation about empty objects in case of missing values. I've hopefully picked up this correctly in c5cf2f1.

I confess, I'm quite new to unit testing, so it may be some time before I can have a working test added to the PR. Would it be appropriate to insert the test here?

https://github.com/strongloop/loopback/blob/3.x-latest/test/access-token.test.js#L74

[bajtos]

I confess, I'm quite new to unit testing, so it may be some time before I can have a working test added to the PR. Would it be appropriate to insert the test here?

https://github.com/strongloop/loopback/blob/3.x-latest/test/access-token.test.js#L74

Ha! I looked up the commit which introduced the user/token verification at app startup (see
79f441b & #3231) and apparently the changes were made with any test coverage. In which case I am ok to land your fix without test coverage too.

However, I'd like to better understand how to reproduce the problem you are encountering so that I can verify the fix manually and ensure we are fixing the right place. How did you @ryanxwelch configure your application so that your User model's relations don't have accessTokens entry?

Earlier today, I was investigating #3917, where @mcitdev is running into a similar problem, where our code assumes a User model has "accessTokens" relation configured, but the relations object does not contain any accessTokens entry. It is very puzzling to me how such situation can happen, because my attempts to construct such access-token-less user model failed and I have to use a rather ugly hack to reproduce the problem.

I'd like to build a better understanding of both of these use cases, there may be other places we need to fix to support you.

[ryanxwelch]

Hi @bajtos,

Thanks for looking into this. My app generally follows the guidelines laid out here:
http://loopback.io/doc/en/lb3/Authentication-authorization-and-permissions.html#access-control-with-multiple-user-models

It's configured with two models named user and admin, both of which inherit from User, and a CustomAccessToken which inherits from AccessToken as follows:

{
  "name": "CustomAccessToken",
  "base": "AccessToken",
  "relations": {
    "user": {
      "type": "belongsTo",
      "idName": "id",
      "polymorphic": {
        "idType": "string",
        "foreignKey": "userId",
        "discriminator": "principalType"
      }
    }
  }
}

{
  "name": "user",
  "base": "User",
  "properties": { ...  },
   ...
  "relations": {
    "accessTokens": {
      "type": "hasMany",
      "model": "CustomAccessToken",
      "polymorphic": { "foreignKey": "userId", "discriminator": "principalType" },
      "options": { "disableInclude": true }
    }
  },
  ...
}

{
  "name": "admin",
  "base": "User",
  "properties": { ...  },
   ...
  "relations": {
    "accessTokens": {
      "type": "hasMany",
      "model": "CustomAccessToken",
      "polymorphic": { "foreignKey": "userId", "discriminator": "principalType" },
      "options": { "disableInclude": true }
    }
  },
  ...
}

[bajtos]

Let's not spend too much time on this. In #3917, I got a better understanding on how it can be that accessTokens relation is not configured. I am ok to proceed to land this patch mostly as-is. However, it's opened against a wrong branch: 3.x-latest instead of master. I opened a new PR targeting the master branch, see #3934