Enable multiple user models inheriting from base class User #2971

ebarault · 2016-11-22T18:52:01Z

Description

Currently, the loopback app does not support defining multiple user models inheriting from the base class User, for mainly 2 reasons:

static relations binding the User base class with some key related models
--> belongsTo relation between base class AccessToken and base class User
--> belongsTo relation between base class RoleMapping and base class User
--> hasMany relation between base class User and base class AccessToken
app.registry method getModelByType() which is used at several places to get a reference on the User child model by its parent class type, but which does not support multiple User inherited models

This PR proposes to rely on polymorphic relations to allow the loopback app supporting multiple User inherited models.

getModelByType(loopback.User) is not used anymore to get the User model as they can be multiple
a new property (polymorphic discriminator) principalModelName / userModelNameis added to RoleMapping and AccessToken base class to support the polymorphic relations in selecting the right modelTo

All existing impacted tests have been updated.

Usage

To enable this feature:

In your custom AccessToken model:

add a new property "principalType" of type "string".
configure the relation "belongsTo user" as polymorphic,
using "principalType" as the discriminator

In your User models:

Configure the "hasMany accessTokens" relation as polymorphic,
using "principalType" as the discriminator

When creating custom Role and Principal instances, set your
User model's name as the value of "prinicipalType".

Related issues

#2516

Checklist

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

slnode · 2016-11-22T18:52:02Z

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

slnode · 2016-11-22T18:52:02Z

Can one of the admins verify this patch?

slnode · 2016-11-22T18:52:03Z

Can one of the admins verify this patch?

slnode · 2016-11-22T18:52:03Z

Can one of the admins verify this patch?

slnode · 2016-11-22T18:52:03Z

Can one of the admins verify this patch?

slnode · 2016-11-23T14:48:17Z

Can one of the admins verify this patch?

slnode · 2016-11-23T14:48:17Z

Can one of the admins verify this patch?

slnode · 2016-11-23T14:48:17Z

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

slnode · 2016-11-23T14:48:17Z

Can one of the admins verify this patch?

slnode · 2016-11-23T14:48:17Z

Can one of the admins verify this patch?

davidcheung · 2016-11-29T15:38:33Z

@bajtos @raymondfeng can you PTAL

bajtos · 2016-12-05T12:49:53Z

Hi @ebarault, thank you for the pull request. The use case you are trying to support makes sense to me.

My main concern is about backwards compatibility. Can we modify your implementation to support existing applications that don't have userModelName/principalModelName at all? I mean not only that these two properties are not filled, but that they also don't even exist in the database (SQL) schema.

I think the following changes may address the issue:

Add a feature flag controlling whether these two new properties are defined or not
In the code handling polymorphic relation, treat missing *ModelName values as if they were pointing to the single model (this is the current behaviour).

Thoughts?

I would also like to hear from @raymondfeng

slnode · 2016-12-05T12:49:56Z

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

slnode · 2016-12-05T12:50:00Z

Can one of the admins verify this patch?

slnode · 2016-12-05T12:50:00Z

Can one of the admins verify this patch?

slnode · 2016-12-05T12:50:00Z

Can one of the admins verify this patch?

slnode · 2016-12-05T12:50:00Z

Can one of the admins verify this patch?

ebarault

@bajtos : reviewed your comments (see comments inline) + rebased
ready for review

ebarault · 2017-02-01T00:00:49Z

common/models/access-token.js


      var AccessToken = this.constructor;
      var userRelation = AccessToken.relations.user; // may not be set up
      var User = userRelation && userRelation.modelTo;


@bajtos : I think we need to honour AccessToken.relations.user.modelTo as the default User model. Consider the case where app has custom User and AccessToken models, e.g. in order to customise some of built-in methods and/or settings.
In that case, principalType is not set. Before your change, the code will look up the related user model. After your change, the built-in User model would be incorrectly used.

i moved back to the original implementation following your comment.

although: if I correctly get the docs for Registry.getModelByType(...) we could use getModelByType('User') instead of AccessToken.relations.user.modelTo as the method allows to

to find configured models in models.json over the base model.

although: if I correctly get the docs for Registry.getModelByType(...) we could use getModelByType('User') instead of AccessToken.relations.user.modelTo as the method allows to
to find configured models in models.json over the base model.

That may work in most cases, but I would be concerned about changes in edge cases, e.g. when the app has a custom User-based model that's actually not used for authentication, i.e. AccessToken is attached to the base User model. (That's just an example.)

Let's play it safe and keep the current implementation.

ebarault · 2017-02-01T00:02:04Z

common/models/access-token.js

-        cb(null, isValid);
+        process.nextTick(function() {
+          cb(null, isValid);
+        });


the original implem did not respect the async api

ebarault · 2017-02-01T00:02:14Z

common/models/access-token.js

-      cb(e);
+      process.nextTick(function() {
+        cb(e);
+      });


ebarault · 2017-02-01T00:04:42Z

common/models/access-token.js

+          });
+        }
+      }
+


callback false if a valid User can't be found with the accessToken's principalType

ebarault · 2017-02-01T00:07:08Z

common/models/role-mapping.js

 'use strict';
 var loopback = require('../../lib/loopback');
+var utils = require('../../lib/utils');



all changes related to promisifying RoleMapping model are covered in #3169

ebarault · 2017-02-01T00:12:58Z

lib/access-context.js

+      }
+      // iterate on the upper base model
+      modelBase = modelBase.definition.settings.base;
+    }


@batjos: Uff, I am actually confused, what is the purpose of the code here and below, where you are crawling model hierarchy?

you commented on an earlier implementation:

@bajtos : This does not work with a deeper hierarchy (Customer extends BaseUser extends built-in User).

to what i replied :

@ebarault : you're right, i noted that too. we should test recursively up to the top User model to check if we ultimately get the built-in User model.

I changed the implementation accordingly and added a test

Is there any particular reason against using this simpler version?

var BaseUser = this.registry.getModel('User'); for (var i = 0; i < this.principals.length; i++) { // ... // or permit to resolve a valid user model var userModel = this.registry.findModel(p.type); if (!userModel) continue; if (userModel.prototype instanceof BaseUser) { return p.id; } }

The way i get Registry.getModel(): this.registry.getModel('User') would actually return one among several models inheriting from built-in User model, not the built-in User Model itself

then i think userModel.prototype instanceof BaseUser would only return true for models being instances or inheriting from that particular User model

do you see things different?

ebarault · 2017-02-01T00:15:54Z

test/multiple-user-principal-types.test.js

+          // no token for userFromAnotherModel
+          {userId: userFromAnotherModel.id, principalType: 'AnotherUser'},
+        ]);
+      });


looking for all tokens and checking against a set of expected data

ebarault · 2017-02-01T00:16:48Z

test/multiple-user-principal-types.test.js

+        accessContext.addPrincipal(principal.type, principal.id);
+      });
+    }
+  });


changed for this helper, as per your suggestion

ebarault · 2017-02-01T00:20:10Z

test/multiple-user-principal-types.test.js

+          expect(user).to.have.property('email', userFromOneModel.email);
+        });
+      }
+    });


isolated the whole logic of verifying the temp accessToken info in a dedicated helper, as per your suggestion

ebarault · 2017-02-01T10:42:57Z

@bajtos : merged and rebased following #3169, could you please review?

coveralls · 2017-02-01T10:44:49Z

Coverage increased (+0.3%) to 89.343% when pulling c1a8495 on ebarault:enable-multiple-user-models into 76dd35e on strongloop:master.

bajtos · 2017-02-01T16:00:11Z

lib/access-context.js

  context = context || {};

+  assert(context.registry,
+    'Application registry is mandatory in AccessContext but missing in provided context');


Should we fall back to loopback.registry when context.registry is not provided, in order to preserve backwards compatibility? loopback.registry is the global singleton where models like loopback.User are defined.

IMO we should only fallback to loopback.registry in case localRegistry is set to false, or not set and running a loopback version where it defaults to false. Thoughts?

IMO we should only fallback to loopback.registry in case localRegistry is set to false, or not set and running a loopback version where it defaults to false.

Sure, that sounds good too. It may be best to leave this change out of scope of this pull request, so that this patch can be finally landed?

sure !!!
I will take care of this in a second step

bajtos · 2017-02-01T16:06:16Z

@ebarault good job!

I pushed two commits to your feature branch:

in b9d289d, I made few final coding style improvements
in a689edd, I simplified the code checking whether a model is extending built-in User model

If you are happy with these changes, then please rebase your patch on top of the latest master (it will fix the failing CI) and squash the commits to a single one. (No need to preserve my authorship of those last minor changes.)

If you are not, then let's discuss a bit more :)

ebarault · 2017-02-01T16:15:03Z

@bajtos: wow, what a journey !
thanks for final code-style edition, more efficient this way.
regarding your 2 comments, please see my comment above (fallback to loopback.registry) and below (simplify baseUser lookup)

ebarault · 2017-02-01T16:18:30Z

lib/access-context.js

-    if (userModel.modelName === 'User') {
+    if (userModel.prototype instanceof BaseUser) {
      return p.id;
    }


The way i get Registry.getModel(): this.registry.getModel('User') would actually return one among several models inheriting from built-in User model, not the built-in User Model itself

then i think userModel.prototype instanceof BaseUser would only return true for models being instances or inheriting from that particular User model

do you see things different?

registry.getModel() calls registry.findModel() and returns exactly the one model registered with the given name - see lib/registry.js#L299 in current master. Here is the code in juggler adding an entry to modelBuilder.models: lib/model-builder.js#L203.

Perhaps you are confusing registry.getModels() with app.models or registry.getModelByType?

arff..... yes! :-) i just couldn't see this ! (confusing getModel() with getModelByType() !!)

Then it should do, thanks.

Just one last check: you're confident in model prototypal inheritance in loopback and there's no way a model extending 'User' can return false to instanceof BaseUser ?

Just one last check: you're confident in model prototypal inheritance in loopback and there's no way a model extending 'User' can return false to instanceof BaseUser?

Yes, I am pretty confident:

Model inheritance in LoopBack uses Node.js' util.inherits under the hood, see model-builder.js#L200

We are already relying on instanceof in the following places:

app.model()

registry.getModelByType()

Model.nestRemoting

ebarault · 2017-02-01T19:03:39Z

@bajtos: squashed and rebased. all good now :-) let's land it !

Allow LoopBack applications to configure multiple User models and share the same AccessToken model. To enable this feature: 1) In your custom AccessToken model: - add a new property "principalType" of type "string". - configure the relation "belongsTo user" as polymorphic, using "principalType" as the discriminator 2) In your User models: - Configure the "hasMany accessTokens" relation as polymorphic, using "principalType" as the discriminator When creating custom Role and Principal instances, set your User model's name as the value of "prinicipalType".

bajtos · 2017-02-02T08:54:02Z

@ebarault I have amended the commit message to include more details, most notably basic usage instructions. I hope I got them right.

As I was re-reading the code to find how to use this new feature, it occurred to me that there is one test missing. We are verifying that Role.isInRole detects OWNER role when the correct principal type+id is specified, but there is no test to verify that Role.isInRole rejects OWNER role when the principal type does not match. Could you please add it in a new pull request?

Could you please contribute the documentation for this feature too? @crandmck is the best person to help you to figure out where to put the new content, I would personally start in http://loopback.io/doc/en/lb3/Authentication-authorization-and-permissions.html and/or the nested pages:

See also http://loopback.io/doc/en/contrib/doc-contrib.html

ebarault · 2017-02-02T08:59:00Z

@bajtos :

commit details

LGTM: we pretty much managed to make this that simple to use in the end

documentation

yes of course

new test case

ok for new PR

bajtos · 2017-02-02T09:04:34Z

🎉 🎉 🎉 LANDED 🎉 🎉 🎉

As you said, @ebarault, it was quite a journey to get here. Thank you very much for your persistence and all the effort you have invested to make this happen. I appreciate it a lot! 🙇

ebarault · 2017-02-02T09:07:34Z

Many thanks @bajtos for joining forces in that quest !

beeman · 2017-02-04T20:48:08Z

@ebarault big thanks for getting this into loopback. If you're working on the documentation feel free to ping me to proofread or test it.

bajtos · 2017-02-06T16:28:57Z

big thanks for getting this into loopback. If you're working on the documentation feel free to ping me to proofread or test it.

@beeman awesome! We have moved our documentation to github and changes are made via pull requests, perhaps you can review @ebarault's patch when it's ready?

ebarault · 2017-02-06T16:59:45Z

@beeman : i'll definitely will :-) just need to have a coupled of final tweaks to this feature merged in master before working on the documentation

beeman · 2017-02-06T18:42:39Z

@bajtos @ebarault yep, more than happy to review the PR

louisbao · 2017-05-27T03:38:34Z

Bonjour @ebarault,
thank you for your great efforts in this PR. I'm following this tutorial (https://loopback.io/doc/en/lb3/Authentication-authorization-and-permissions.html#access-control-with-multiple-user-models) to implement my loopback-admin-panel. Everything works very well, except the ACL Authorization side.

I have one Project model based on PersistedModel, two user models: AppUser and ContractorUser base on User model with a Mysql Database to keep my ACL rules and having exactly codebase of this example application (https://github.com/strongloop/loopback-example-access-control ).

I'm wondering if you can tell me what's the best practice to set the principalType and principalId in the accessContext, inside a remote hook or an operation hook ?

ERROR: As {"type":"AppUser", "id":"3"} is excepted, AppUser 3 has an admin role to access find property of Project Model.

  # lib/access-context.js ----- line 84
  if (principalId) {
    this.addPrincipal(principalType, principalId, principalName);
  }

Pls find, my db data below:

# Table Role
id-----name
1 ----- admin

# Table RoleMapping
id-----principalType------pricipalId-------roleId
1 -----    AppUser   -------   3         -------   1

and my security debug log below, Thanks in advance.

 loopback:security:access-context this.principals: undefined +0ms
loopback:security:access-context this.principals: undefined +1ms
loopback:security:access-context this.principals: [{"type":"USER","id":"3"}] +0ms
loopback:security:access-context --Context scopes of Project.find()-- +0ms
loopback:security:access-context   method-level: ["DEFAULT"] +0ms
loopback:security:acl Check principals: [{"type":"USER","id":"3"}] +6ms
loopback:security:role isInRole(): $everyone +0ms
loopback:security:access-context ---AccessContext--- +0ms
loopback:security:access-context principals: +0ms
loopback:security:access-context principal: {"type":"USER","id":"3"} +0ms
loopback:security:access-context modelName Project +0ms
loopback:security:access-context modelId undefined +1ms
loopback:security:access-context property find +0ms
loopback:security:access-context method find +0ms
loopback:security:access-context accessType READ +0ms
loopback:security:access-context --Context scopes of Project.find()-- +0ms
loopback:security:access-context   method-level: ["DEFAULT"] +0ms
loopback:security:access-context accessScopes ["DEFAULT"] +0ms
loopback:security:access-context accessToken: +0ms
loopback:security:access-context   id "wnAaogzO4Jg4Rr3B8HCjfaOlB00hRQr6CMZxJQATc0ZwNdHQOkTuVpBP4OWYrHTA" +0ms
loopback:security:access-context   ttl 1209600 +0ms
loopback:security:access-context   scopes ["DEFAULT"] +0ms
loopback:security:access-context AccessContext.prototype.getUser: [{"type":"USER","id":"3"}] +0ms
loopback:security:access-context getUserId() 3 +0ms
loopback:security:access-context AccessContext.prototype.getUser: [{"type":"USER","id":"3"}] +0ms
loopback:security:access-context isAuthenticated() true +0ms
loopback:security:role Custom resolver found for role $everyone +1ms
loopback:security:role isInRole(): admin +0ms
loopback:security:access-context ---AccessContext--- +0ms
loopback:security:access-context principals: +0ms
loopback:security:access-context principal: {"type":"USER","id":"3"} +0ms
loopback:security:access-context modelName Project +0ms
loopback:security:access-context modelId undefined +0ms
loopback:security:access-context property find +0ms
loopback:security:access-context method find +0ms
loopback:security:access-context accessType READ +0ms
loopback:security:access-context --Context scopes of Project.find()-- +0ms
loopback:security:access-context   method-level: ["DEFAULT"] +0ms
loopback:security:access-context accessScopes ["DEFAULT"] +0ms
loopback:security:access-context accessToken: +0ms
loopback:security:access-context   id "wnAaogzO4Jg4Rr3B8HCjfaOlB00hRQr6CMZxJQATc0ZwNdHQOkTuVpBP4OWYrHTA" +1ms
loopback:security:access-context   ttl 1209600 +1ms
loopback:security:access-context   scopes ["DEFAULT"] +0ms
loopback:security:access-context AccessContext.prototype.getUser: [{"type":"USER","id":"3"}] +0ms
loopback:security:access-context getUserId() 3 +0ms
loopback:security:access-context AccessContext.prototype.getUser: [{"type":"USER","id":"3"}] +0ms
loopback:security:access-context isAuthenticated() true +0ms
loopback:security:role Role found: {"id":1,"name":"admin","description":null} +4ms
loopback:security:role Role mapping found: null +4ms
loopback:security:role isInRole() returns: null +1ms
loopback:security:acl The following ACLs were searched:  +0ms
loopback:security:acl ---ACL--- +0ms
loopback:security:acl model Project +0ms
loopback:security:acl property * +0ms
loopback:security:acl principalType ROLE +0ms
loopback:security:acl principalId $everyone +0ms
loopback:security:acl accessType * +0ms
loopback:security:acl permission DENY +0ms
loopback:security:acl with score: +0ms 7495
loopback:security:acl ---Resolved--- +0ms
loopback:security:access-context ---AccessRequest--- +1ms
loopback:security:access-context  model Project +0ms
loopback:security:access-context  property find +0ms
loopback:security:access-context  accessType READ +0ms
loopback:security:access-context  permission DENY +0ms
loopback:security:access-context  isWildcard() false +0ms
loopback:security:access-context  isAllowed() false +0ms
Unhandled error for request GET /api/projects?access_token=wnAaogzO4Jg4Rr3B8HCjfaOlB00hRQr6CMZxJQATc0ZwNdHQOkTuVpBP4OWYrHTA: 
Error: Authorization Required

crandmck added the #community contribution label Nov 22, 2016

ebarault closed this Nov 23, 2016

crandmck removed the #community contribution label Nov 23, 2016

ebarault reopened this Nov 23, 2016

crandmck added the #community contribution label Nov 23, 2016

ebarault force-pushed the enable-multiple-user-models branch 4 times, most recently from 3513deb to 20025be Compare November 24, 2016 00:38

bajtos self-assigned this Dec 5, 2016

bajtos closed this Dec 5, 2016

bajtos reopened this Dec 5, 2016

bajtos removed the #community contribution label Dec 5, 2016

ebarault commented Feb 1, 2017

View reviewed changes

ebarault force-pushed the enable-multiple-user-models branch from fe72201 to c1a8495 Compare February 1, 2017 10:40

bajtos reviewed Feb 1, 2017

View reviewed changes

bajtos approved these changes Feb 1, 2017

View reviewed changes

ebarault commented Feb 1, 2017

View reviewed changes

ebarault force-pushed the enable-multiple-user-models branch 2 times, most recently from 5e3d613 to 084053e Compare February 1, 2017 18:24

ebarault force-pushed the enable-multiple-user-models branch from 084053e to b490421 Compare February 1, 2017 19:39

bajtos force-pushed the enable-multiple-user-models branch from b490421 to 9fe084f Compare February 2, 2017 08:47

bajtos merged commit 304ecc4 into strongloop:master Feb 2, 2017

ebarault mentioned this pull request Feb 3, 2017

Fix Role.isOwner() when multiple user models #3180

Merged

2 tasks

bajtos added the loopback-auth label Feb 15, 2017

bajtos mentioned this pull request Feb 24, 2017

Problems extending User model with loopback v3.3.0 #3215

Closed

ebarault mentioned this pull request Mar 1, 2017

first draft for multiple user models feature loopbackio/loopback.io#297

Merged

ebarault mentioned this pull request Mar 9, 2017

Validate User model config at app boot #3265

Closed

2 tasks

Enable multiple user models inheriting from base class User #2971

Enable multiple user models inheriting from base class User #2971

Uh oh!

Conversation

ebarault commented Nov 22, 2016 • edited by bajtos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Usage

Related issues

Checklist

Uh oh!

slnode commented Nov 22, 2016

Uh oh!

slnode commented Nov 22, 2016

Uh oh!

slnode commented Nov 22, 2016

Uh oh!

slnode commented Nov 22, 2016

Uh oh!

slnode commented Nov 22, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

davidcheung commented Nov 29, 2016

Uh oh!

bajtos commented Dec 5, 2016

Uh oh!

slnode commented Dec 5, 2016

Uh oh!

slnode commented Dec 5, 2016

Uh oh!

slnode commented Dec 5, 2016

Uh oh!

slnode commented Dec 5, 2016

Uh oh!

slnode commented Dec 5, 2016

Uh oh!

ebarault left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault commented Feb 1, 2017

Uh oh!

coveralls commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ebarault commented Nov 22, 2016 •

edited by bajtos

Loading

coveralls commented Feb 1, 2017 •

edited

Loading

ebarault commented Feb 1, 2017 •

edited

Loading

ebarault commented Feb 1, 2017 •

edited

Loading

ebarault commented Feb 2, 2017 •

edited

Loading

louisbao commented May 27, 2017 •

edited

Loading