Correct PrincipalType for multiple extension of User models

[akki-ng]

#3679
Patch is created along with the test case. Now access context will have the correct PrincipalType and getUser will work for all extensions of User model

[slnode]

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

[ebarault]

Hi @akki-ng ,
does this test pass as well ?

[akki-ng]

@ebarault test will not pass by this fix as this fix does not cover this test case

[bajtos]

@akki-ng thank you for the new version. I'll try to review your changes next week.

[bajtos]

@slnode ok to test

[bajtos]

I find this change suspicions - I would expect that token.principalType was already added on line 84 above.

Having said that, this change does fix certain bugs like e.g. #3829, so it's not without merit. I need to build a better understanding of this part of our codebase to be able to comprehend impacts of this change in a wider context.

[bajtos]

Never mind, the line 84 was added back in 2013, before support for multiple user models were introduced. I suspect that code path was actually never used.

[akki-ng]

@bajtos You are right about line 84, but this block of code is neglecting the principal type associated with the accessToken,

loopback/lib/access-context.js

Lines 47 to 85 in 6ddf268

	context = context || {};
	assert(context.registry,
	'Application registry is mandatory in AccessContext but missing in provided context');
	this.registry = context.registry;
	this.principals = context.principals || [];
	var model = context.model;
	model = ('string' === typeof model) ? this.registry.getModel(model) : model;
	this.model = model;
	this.modelName = model && model.modelName;
	this.modelId = context.id || context.modelId;
	this.property = context.property || AccessContext.ALL;
	this.method = context.method;
	this.sharedMethod = context.sharedMethod;
	this.sharedClass = this.sharedMethod && this.sharedMethod.sharedClass;
	if (this.sharedMethod) {
	this.methodNames = this.sharedMethod.aliases.concat([this.sharedMethod.name]);
	} else {
	this.methodNames = [];
	}
	if (this.sharedMethod) {
	this.accessType = this.model._getAccessTypeForMethod(this.sharedMethod);
	}
	this.accessType = context.accessType || AccessContext.ALL;
	assert(loopback.AccessToken,
	'AccessToken model must be defined before AccessContext model');
	this.accessToken = context.accessToken || loopback.AccessToken.ANONYMOUS;
	var principalType = context.principalType || Principal.USER;
	var principalId = context.principalId || undefined;
	var principalName = context.principalName || undefined;
	if (principalId != null) {
	this.addPrincipal(principalType, principalId, principalName);
	}

I think we will need to correct the whole block for a holistic solution

[akki-ng]

My initial thought is, if line 84 was supposed to set the correct principal type then line 47 is dumping the empty object in some corner case.

[bajtos]

Related work: #3835

[bajtos]

Hello @akki-ng, I am taking a look at your proposed changes. I started by trying to run your tests against the current master branch (see f774427) and they are passing for me! Does it mean the problem is already fixed?

I suspect the change you are proposing in common/models/role.js may be still needed. How can I reproduce the bug which requires such fix?

[akki-ng]

Hey @bajtos, the test cases were to pointout the problem of setting correct principalType in access context and allowing the user to be read correctly. If tests are passing then essentially we solved the problem of getting the user correctly from access context irrespective of their principalType.

While the changes in role.js tries to solve a different problem but only specific use case. For ex, if we have many extensions of user model and other entities have only one belongsTo to any of custom user model, in that case $owner role was not working in ACL of those entities. The patch in role.js will allow users to use $owner in acls of a multi user environment considering if there is only one belongsTo relationship of a particular entity to any of custom user model to which $owner acl is specified.
This patch does not try to solve a problem where an entity may have many belongsTo relationship with users, which can be treated as a separate use case.

[akki-ng]

@bajtos to solve the use case of $owner not working in multi user environment, the patch wlon accessContext was mandatory. Now that accessContext patch is merged, you can reproduce the issue without the patch in role.js and confirm if applying the patch works properly.

[akki-ng]

Closed by mistake, reopening now.

[slnode]

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

[bajtos]

@akki-ng thank you for the response.

The patch in role.js will allow users to use $owner in acls of a multi user environment considering if there is only one belongsTo relationship of a particular entity to any of custom user model to which $owner acl is specified.
This patch does not try to solve a problem where an entity may have many belongsTo relationship with users, which can be treated as a separate use case.

We need to capture this in a test. Could you please explain little bit more the relation between different models, ideally as an example showing specific model names and the relations between them? I am happy to help you with writing the unit test, but I don't have bandwidth to research how to setup models and relations to trigger the problem.

[akki-ng]

@bajtos thanks for writing the unit tests. I will create a sample app to demonstrate the issue by next day.

[bajtos]

I will create a sample app to demonstrate the issue by next day.

That will be awesome!

[akki-ng]

@bajtos Please follow the below example to understand why the code will fail in a corner case.

For this example we have 2 custom user models (Seller and Customer), one Building model and one CustomAccessToken model to support polymorpic relations of token and users.

{
  "name": "Seller",
  "base": "User",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "properties": {
    "shopName": {
      "type": "string"
    }
  },
  "validations": [],
  "relations": {
    "accessTokens": {
      "type": "hasMany",
      "model": "CustomAccessToken",
      "polymorphic": {
        "foreignKey": "userId",
        "discriminator": "principalType"
      },
      "options": {
        "disableInclude": true
      }
    },
    "buildings": {
      "type": "hasMany",
      "model": "Building",
      "foreignKey": ""
    }
  },
  "acls": [
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$everyone",
      "permission": "DENY"
    },
    {
      "property": ["__create__buildings", "__get__buildings"],
      "principalType": "ROLE",
      "principalId": "$owner",
      "permission": "ALLOW"
    }
  ],
  "methods": {}
}

{
  "name": "Customer",
  "base": "User",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "properties": {
    "mobile": {
      "type": "number"
    }
  },
  "validations": [],
  "relations": {
    "accessTokens": {
      "type": "hasMany",
      "model": "CustomAccessToken",
      "polymorphic": {
        "foreignKey": "userId",
        "discriminator": "principalType"
      },
      "options": {
        "disableInclude": true
      }
    }
  },
  "acls": [],
  "methods": {}
}

{
  "name": "Building",
  "base": "PersistedModel",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "properties": {
    "name": {
      "type": "string",
      "required": true
    }
  },
  "validations": [],
  "relations": {
    "seller": {
      "type": "belongsTo",
      "model": "Seller",
      "foreignKey": ""
    }
  },
  "acls": [
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$everyone",
      "permission": "DENY"
    },
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$owner",
      "permission": "ALLOW"
    }
  ],
  "methods": {}
}

{
  "name": "CustomAccessToken",
  "base": "AccessToken",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "properties": {},
  "validations": [],
  "relations": {
    "user": {
      "type": "belongsTo",
      "idName": "id",
      "polymorphic": {
        "idType": "string",
        "foreignKey": "userId",
        "discriminator": "principalType"
      }
    }
  },
  "acls": [],
  "methods": {}
}

Now follow the below steps in order to have the records ready

1. Create seller1, seller2, customer1
2. Keep the AccessToken ready for all 3 users
3. Login as seller1 and create a building for seller1

Set token of seller1 and try the following API

1. PATCH /api/Sellers/1
2. PATCH /api/Sellers/2 . <---401 as expected
3. GET /api/Buildings/1

Now set token of seller2 and try the following API

1. PATCH /api/Sellers/1 . <--- 401 work as expected
2. PATCH /api/Sellers/2
3. GET /api/Buildings/1 <--- 401 work as expected

Till now things worked as expected
Now login as customer1 and try the following

1. GET /api/Buildings/1 <--- 401 work as expected
   2. PATCH /api/Sellers/1 <--- Request Hangs --->
   3. PATCH /api/Sellers/2 <--- Request Hangs --->

Now make the changes in role.js

// Is the modelClass User or a subclass of User?
    if (isUserClass(modelClass)) {
      var userModelName = modelClass.modelName;
      // matching ids is enough if principalType is USER or matches given user model name
      if (principalType === Principal.USER || principalType === userModelName) {
        console.log("Going in")
        process.nextTick(function() {
          callback(null, matches(modelId, userId));
        });
      }else {
        process.nextTick(function() {
          callback(null, false);
        });
      }
      return callback.promise;
    }

After making changes retry the API which were hanging when customer was loggedIn.

The bug is if many extended user models are used and they have $owner in their ACL then if a user from different user model tries to fire an api of another user model, leading the callback to never retrun trur or false from Role.isOwner.

I hope this example will clear things up. Let me know if you are not able to reproduce.

[bajtos]

@akki-ng thank you for the instructions on how to reproduce the problem you are experiencing and sorry for the long delay! I was two weeks on a sick leave and my inbox exploded by the time I got back. I'll try to take a look at this in the next week or two.

[akki-ng]

@bajtos Did you get the time to reproduce the issue?

[bajtos]

Not yet, sorry 😞

[bajtos]

Hi @akki-ng, thank you for the reproduction instructions and your patience with us.

I managed to reproduce the problem and have a fix coming shortly in a new pull request 🎉

[bajtos]

This will not work in the following case: one user model owns another user mode, e.g. a Seller owns the business account of Customer.

[bajtos]

Closing in favour of #3883