Fix crash in User model's "before delete" hook

[mcitdev]

Description

Update the "before delete" hook to take into account the case where the user is not logged in and therefore does not have any accessToken

Related issues

• connect to Logout fails for model based on User (other than model called "user") #398

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

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

[bajtos]

Thank you for the pull request! Before we can merge it, we need you to add a new unit-test verifying the fix and preventing regressions in the future.

You can use the commit which introduced "before delete" hook for inspiration, see b53a22b

[bajtos]

when the user is not logged in

This description feels misleading to me. How about this?

Do nothing and when the access control was disabled for user model.

[bajtos]

Uh oh, my proposed description is not even proper English. Sorry for the typo! Here is what I meant:

Do nothing when the access control was disabled for this user model.

[mcitdev]

Ok, I fixed all the linting problem, and excuse my english because I'm not very english. Feel free to modify anything you want.

[bajtos]

Few more things to fix:

• We need you to sign our Contributor License Agreement, see https://cla.strongloop.com/agreements/strongloop/loopback

• Your commit message is not following our 50/72 rule.

  **    532ac66 - Update the "before delete" hook in the user.js mode: First line should be 50 characters or less (saw 53)
  **    532ac66 - Update the "before delete" hook in the user.js mode: line 3 longer than 72 characters.
  

  Please use git commit --amend && git push --force-with-lease to fix it. I can fix it for you before landing if you prefer.

• Your code have linting problems, please make sure that npm test is passing for you locally. See https://travis-ci.org/strongloop/loopback/jobs/393096317#L2689:

  /home/travis/build/strongloop/loopback/common/models/user.js
    348:53  error  Block must not be padded by blank lines  padded-blocks
    349:1   error  Trailing spaces not allowed              no-trailing-spaces
    351:5   error  Expected space(s) after "if"             keyword-spacing
    352:1   error  Trailing spaces not allowed              no-trailing-spaces
  

[bajtos]

@mcitdev hello, I have rebased the patch on top of the latest master, remove package-lock.json, fixed the commit message and added a test to verify the fix (see 682df35).

There is one thing I don't understand though. How do you disable "user has many access tokens" relation in your application? Is the solution used by my new test similar to what you use?

     // Simulate the situation when "accessTokens" relation was not configured
     delete PrivateUser.definition.settings.relations.accessTokens;
     delete PrivateUser.definition.relations.accessTokens;

[mcitdev]

I didn't disable access tokens for the user, I do just not use ACL at all by default. This was the scenario I've tested - user with no access control -

[mcitdev]

@bajtos , Ok I just see your test, this is it, it describes what I'm trying to do

[mcitdev]

Ok, I think I should describe the scenario:

I create the user with no ACL configuration, So I can perform actions on it without needing to login. That's why I do not have the access token, because I'm not logged in. So, I think this is not the same thing, because I didn't disabled the access control manually, but it's disabled by default due to not using the access control.
I want to go back to my comment above, because I think that this is not the same test as what I'm doing.
What I've wrote in my test describes what I'm doing actually in the test app.

Maybe this situation is not realistic, but it preserves the consistency of the use of acl.

[bajtos]

@mcitdev Hmm. Somehow I cannot find the test you are talking about. Did you perhaps forget to push it to GitHub? It's also possible I have accidentally discarded it from this branch.

Could you please check your local code, make sure to commit any outstanding changes (run git status to check) and then git push -f your code? Just don't try to pull updates from GitHub before pushing your local copy.

I create the user with no ACL configuration, So I can perform actions on it without needing to login.

When you say "perform actions", do you mean to invoke REST API (make HTTP requests), or just call model methods via JavaScript API?

That's why I do not have the access token, because I'm not logged in. So, I think this is not the same thing, because I didn't disabled the access control manually, but it's disabled by default due to not using the access control.

Yes, I agree my test is a different scenario than yours.

The trouble is, I am not able to reproduce your scenario. When you take my test and modify the lines removing accessTokens relation, the test passes against the current master branch - i.e. there is no bug to fix!

What I've wrote in my test describes what I'm doing actually in the test app.

As I wrote above, somehow I lost the test you are talking about. Could you please add it to the pull request again?

[mcitdev]

@bajtos Ok, it was my fault, I reviewed the user test file, I totally agree with your test, I'm with you to use your test instead of mine.

[bajtos]

Ok, it was my fault, I reviewed the user test file, I totally agree with your test, I'm with you to use your test instead of mine.

OK.

However, I'd still like to understand how the way how you are configuring your app. Do you really delete relations.accessTokens entries in your app, as I am doing in my test? Is there perhaps something else going on that's causing the problem?

It looks like you are not the only one running into this problem, #3921 opened by @ryanxwelch found a different bug that manifests when a user does not have any "accessTokens" relation.

It's a mystery to me which I'd like to fully understand before trying to fix the symptoms.

[mcitdev]

However, I'd still like to understand how the way how you are configuring your app. Do you really delete relations.accessTokens entries in your app, as I am doing in my test? Is there perhaps something else going on that's causing the problem?

I do not delete the accessTokens from relations, I just follow the cli commands to generate the custom user from an initial empty server loopback configuration.

It looks like you are not the only one running into this problem, #3921 opened by @ryanxwelch found a different bug that manifests when a user does not have any "accessTokens" relation.

It may have a relation with the problem mentionned by @ryanxwelch in #3921 , in that the user has no "accessTokens" relation configured, but more details about the error thrown with the scenario that causes the issue could be helpfull.

[mcitdev]

I rectified my last comment, meanwhile I reviewed the doc, I still learning the framework, because I'm new on it.
So these are some details about my use case, because I think it is deferent from what @ryanxwelch mentionned in #3921:

• First, I create a new app based on the “empty-server” application type. So initially the access control is disabled in this case according to the doc https://loopback.io/doc/en/lb3/Authentication-authorization-and-permissions.html#enabling-access-control :

Applications created with the LoopBack application generator have access control enabled by default, except for the “empty-server” application type. To enable access control for an “empty-server” application, add a boot script that calls enableAuth()

That's why I do not get the warning messages of the lib/application.js because the enableAuth function is not called in my case.

• Then I create a custom user based on the User model, wich is just inherit methods and properties from the base model but not the other fields of the JSON base model according to the doc https://loopback.io/doc/en/lb3/Extending-built-in-models.html#extending-models-using-json :

When you create a model with the model generator, you choose a base model, that is, the model that your model will “extend” and from which it will inherit methods and properties. The tool will set the base property in the model definition JSON file accordingly.

So Initially the custom user has no relation configured by default and it's not inherited from the User model, thus the "accessTokens" relation does not exists yet (this leads me to say that it was not necessary to remove accessTokens from PrivateUser relations to simulate the case in the test, because it does not exist by default until it's configured). That's why I added the lines that handles this case in the "before delete" hook in the user.js model to prevent craching the app with the error :

TypeError: Cannot read property 'modelTo' of undefined
    at node_modules/loopback/common/models/user.js:350:5
    
// user.js:350:58
User.observe('before delete', function(ctx, next) {
    var AccessToken = ctx.Model.relations.accessTokens.modelTo;
    var pkName = ctx.Model.definition.idName() || 'id';
    ctx.Model.find({where: ctx.where, fields: [pkName]}, function(err, list) {
      if (err) return next(err);

      var ids = list.map(function(u) { return u[pkName]; });
      ctx.where = {};
      ctx.where[pkName] = {inq: ids};

      AccessToken.destroyAll({userId: {inq: ids}}, next);
    });
  });

As I said before, maybe this situation is not realistic, but it preserves the consistency of the use of ACL in the application.

[mcitdev]

@bajtos I've modified the test a little bit, because the last version does not fail even if I remove the line added in the "before delete" hook.

This works for me :

it('skips token invalidation when the relation is not configured', (done) => {
      // the test passed when the operation did not crash
      const app = loopback({localRegistry: true, loadBuiltinModels: true});
      app.set('remoting', {errorHandler: {debug: true, log: false}});
      app.dataSource('db', {connector: 'memory'});

      const PrivateUser = app.registry.createModel({
        name: 'PrivateUser',
        base: 'User',
        // Speed up the password hashing algorithm for tests
        saltWorkFactor: 4,
      });
      app.model(PrivateUser, {dataSource: 'db'});

      var usersId;
      async.series([
        function(next) {
          PrivateUser.create({email: 'private@example.com', password: 'pass'}, function(err, user) {
            usersId = user.id;
            next(err);
          });
        },
        function(next) {
          PrivateUser.deleteById(usersId, function(err) {
            next(err);
          });
        },
      ], function(err) {
        if (err) return done(err);
        done();
      });
    });

As I said before, I do not need to delete the "accessTokens" relation, because the "PrivateUser.definition.settings.relations.accessTokens" from the base model will not be used, so it's unnessacary to delete that, instead it will use the child relations wich is by default empty (it can be checked by a simple console.log which will print "undefined" for "PrivateUser.definition.relations.accessTokens")

[bajtos]

As I said before, I do not need to delete the "accessTokens" relation, because the "PrivateUser.definition.settings.relations.accessTokens" from the base model will not be used, so it's unnessacary to delete that, instead it will use the child relations wich is by default empty (it can be checked by a simple console.log which will print "undefined" for "PrivateUser.definition.relations.accessTokens")

Interesting. I though I tried to create a new app for this new test without any luck. I confirm that your updated test works like a charm!

[bajtos]

I'd like to improve your code little bit, see my comments below. I'll make those changes myself to get your fix landed sooner.

[bajtos]

Let's revert this change, it's not related to the fix.

[bajtos]

Let's rewrite this block using promises.

[bajtos]

We don't need to change remoting config because we are not making any REST calls.

[bajtos]

@slnode ok to test

[bajtos]

Landed 🎉 Thank you for contributing the fix and helping me to better understand the issue at hand, @mcitdev ❤️

[mcitdev]

@bajtos I'm glad to contribute to loopback, it was my pleasure.