Correct PrinciplaType for multiple extension of User models #3679

akki-ng · 2017-10-31T10:57:21Z

Get the polymorphic PrincipalType while creating pricipal when multiple extensions of `User` model is used

The app configuration follows the multiple user models setup as described in http://ibm.biz/setup-loopback-auth The built-in role resolver $owner is not currently compatible with this configuration and should not be used in production.

With this pull request $owner can be used.

slnode · 2017-10-31T10:57:22Z

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

slnode · 2017-10-31T10:57:22Z

Can one of the admins verify this patch?

slnode · 2017-10-31T10:57:22Z

Can one of the admins verify this patch?

slnode · 2017-10-31T10:57:22Z

Can one of the admins verify this patch?

slnode · 2017-10-31T10:57:22Z

Can one of the admins verify this patch?

ebarault · 2017-11-01T12:56:02Z

hi @akki-ng,

thanks for trying to fix this.
Could you unskip this test and check if your fix passes it ?

cc: @lehni

lehni · 2017-11-02T08:49:14Z

@ebarault I'm not part of the LB efforts anymore... I've switched to a selection of smaller components, mainly Koa2 and Objection.js, and so far it's going really well...

akki-ng · 2017-11-03T05:59:26Z

Hi @ebarault
I checked this test and its failing

              // multiple owners on a single model instance with a classic
              // belongsTo relation, we need to use belongsTo with polymorphic
              // discriminator to distinguish between the 2 models

I'm not sure if the fix is causing it to fail. But I believe the above fix was not to solve this test case rather to put the correct principalType while adding a principal. Please correct me if I am wrong.

ebarault · 2017-11-03T09:02:16Z

hi @akki-ng
the PR description states: "With this pull request $owner can be used", so this test needs to pass to comply with this expectation.

knowing the root cause, i was suspecting this PR wouldn't be enough to fix it, but i wanted to give it a try.

that being said, as it does not fix this test, i'm wondering what the PR fixes, which I can't tell until more tests are added.

although it might be a step forward to the solution, without knowing the added value or the side effects, we cannot move on with this PR as is i'm afraid.

akki-ng · 2017-11-03T12:11:08Z

Hi @ebarault,

After seeing this test I understand that its a not a complete solution to said problem.

With this pull request $owner can be used.

By this statement I meant that I could user $owner successfully on the extended User model.
For example, lets us assume that we extend 3 UserModels for an e-commerce platform

BusinessUser - InternalUser($businessAdmin, $businessOperation are the roles)
Retailer - People who bring their shop online on the platform($retailOwner is role)
StoreUser - Users which belong to a particular online store($storeManager, $storeOperations, $storeDeliveryGuy)

A Retailer user can be added by BusinessUser only and can be modified by $owner and BusinessUser so acls can user $owner

A StoreUser can be added by BusinessUser and Retailer but can be modified by $owner $businessOperation $retailOwner $retailOwner can be a dynamic role to check if effected storeUser instance belongs to a retail shop whose owner is in the current accessContext

In this cases getting the correct PrincipalType in role resolver will give information about the current logged in User and such decision over the role of $retailOwner can be calculated

Perhaps With this pull request $owner can be used. was not the right phrase to highlight thi PR. Better words could be correct PrincipalType in roleResolver

@ebarault Let me know if this PR makes any sense else we can close it down.

ebarault · 2017-11-03T13:14:23Z

I think it is a step forward to the solution, but i would need to dive in the code again to make sure the objective is met the proper way.
Would you like to try fixing the built-in $owner role all the way down? then I could assist in reviewing.

akki-ng · 2017-11-04T09:20:42Z

@ebarault I would love to solve the issue completely. Can you help me with some of the use-cases of test case with example. And how exactly belongsTo relation configuration being changed to support polymorphic behaviour and is there any config which says 'isOwner' in belongsTo config? These info will help me get started.

Mr-Wiredancer · 2018-01-28T15:03:42Z

@akki-ng Thanks for the fix. I have the same problem of having multiple extended models of User, and the app doesn't properly set principalType. But I don't understand why this cannot be merged.

akki-ng · 2018-01-28T18:34:23Z

@Mr-Wiredancer Welcome :-) This fix will solve many use-cases. But I believe the real issue is a bigger one where a single entity may have many belongsto relationship with multiple extended user models with the support of multiple owners/co-owners. So probably maintainers decided to wait for the complete bug fix before merging this PR.

I was busy earlier so could not do a complete fix. I'll try to fix the real issue and submit a PR soon.

akki-ng · 2018-01-29T19:40:56Z

@Mr-Wiredancer looks like the bigger problem is already solved. Have a look.
#3644

bajtos · 2018-02-09T09:24:42Z

lib/access-context.js

-    this.addPrincipal(Principal.USER, token.userId);
+    var userPrincipalType =
+      (this.accessToken && this.accessToken.principalType) || Principal.USER;
+    this.addPrincipal(userPrincipalType, token.userId);


@akki-ng

This change looks very reasonable to me 👍 and I am ok with landing it as an incremental improvement.

However, I need you to add a test that fails on the current master, passes with your fix in place - this will verify your fix and prevent regressions in the future.

@bajtos Please follow on #3823

bajtos · 2018-02-09T09:25:54Z

lib/access-context.js

  if (token.userId) {
-    this.addPrincipal(Principal.USER, token.userId);
+    var userPrincipalType =
+      (this.accessToken && this.accessToken.principalType) || Principal.USER;


I think this can be shortened by using token instead of accessToken?

Also please use const instead of var in new code - see http://loopback.io/doc/en/contrib/style-guide.html#variable-declarations

const userPrincipalType = token.principalType || Principal.USER;

I'll complete the patch soon.

MartinCerny-awin · 2018-03-08T15:10:23Z

@akki-ng Hello akki, how does it look like with the patch?

akki-ng · 2018-03-08T20:51:16Z

@MartinCerny-awin @bajtos @Mr-Wiredancer Guys, please follow up on new PR #3823
New PR has complete patch and the test case for which will fail without the patch.

bajtos · 2018-05-18T13:32:08Z

See #3883

Correct PrinciplaType for multiple extension of User models

a67ec24

akki-ng requested review from bajtos, clark0x, ebarault, fabien and zbarbuto as code owners October 31, 2017 10:57

bajtos assigned bajtos and ebarault Dec 4, 2017

akki-ng closed this Jan 19, 2018

bajtos reviewed Feb 9, 2018

View reviewed changes

akki-ng mentioned this pull request Mar 8, 2018

Correct PrincipalType for multiple extension of User models #3823

Closed

bajtos mentioned this pull request May 18, 2018

Fix isOwner() bug in multiple-principal setup #3883

Merged

2 tasks

Correct PrinciplaType for multiple extension of User models #3679

Correct PrinciplaType for multiple extension of User models #3679

Uh oh!

Conversation

akki-ng commented Oct 31, 2017

Get the polymorphic PrincipalType while creating pricipal when multiple extensions of User model is used

Uh oh!

slnode commented Oct 31, 2017

Uh oh!

slnode commented Oct 31, 2017

Uh oh!

slnode commented Oct 31, 2017

Uh oh!

slnode commented Oct 31, 2017

Uh oh!

slnode commented Oct 31, 2017

Uh oh!

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

Uh oh!

Uh oh!

lehni commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akki-ng commented Nov 3, 2017

Uh oh!

ebarault commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akki-ng commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebarault commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akki-ng commented Nov 4, 2017

Uh oh!

Mr-Wiredancer commented Jan 28, 2018

Uh oh!

akki-ng commented Jan 28, 2018

Uh oh!

akki-ng commented Jan 29, 2018

Uh oh!

bajtos Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

akki-ng Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

bajtos Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

akki-ng Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

MartinCerny-awin commented Mar 8, 2018

Uh oh!

akki-ng commented Mar 8, 2018

Uh oh!

bajtos commented May 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Get the polymorphic PrincipalType while creating pricipal when multiple extensions of `User` model is used

ebarault commented Nov 1, 2017 •

edited

Loading

lehni commented Nov 2, 2017 •

edited

Loading

ebarault commented Nov 3, 2017 •

edited

Loading

akki-ng commented Nov 3, 2017 •

edited

Loading

ebarault commented Nov 3, 2017 •

edited

Loading