Fix OWNER role to handle multiple relations #3140

pierreclr · 2017-01-26T12:10:13Z

Description

Fix the code resolving OWNER role to correctly handle the situation
where the target model has multiple "belongsTo" relations to the User
model.

Introduce a new model setting "ownerRelations" that enables the new
behavior. When "ownerRelations" is set to true, then all "belongsTo"
relations are considered as granting ownership. Alternatively,
"ownerRelations" can be set to an array of the relations which
are granting ownership.

For example, a document can "belongTo" an author and a reviewer,
but only the author is an owner, the reviewer is not. In this case,
ownerRelations should be set to ['author'].

Related issues

[BUG] Security issue when resolving $owner role #3081 mainly
[BUG ?] Passing tests in french #3105 because I encountered some problems passing the tests because of my french computer
Make things support multi-owner #2454 (closed as a duplicate)
see also Allow multiple ownership relations #3106

slnode · 2017-01-26T12:10:15Z

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

slnode · 2017-01-26T12:10:15Z

Can one of the admins verify this patch?

slnode · 2017-01-26T12:10:15Z

Can one of the admins verify this patch?

slnode · 2017-01-26T12:10:15Z

Can one of the admins verify this patch?

slnode · 2017-01-26T12:10:15Z

Can one of the admins verify this patch?

bajtos · 2017-01-26T14:18:08Z

@slnode ok to test

bajtos · 2017-01-26T14:25:30Z

test/role.test.js

+
+    app.model(Message, {dataSource: 'db'});
+
+    const password = 'pass';


I am afraid we cannot use ES6 in this repository yet because of #3003 :(

Done in 719945d

bajtos · 2017-01-26T14:30:02Z

common/models/role.js

+         * Here is the new way to resolve owner relations
+         * First, fetches `belongsTo` relation (User)
+         * Then, fallback looking for userId / owner
+         */


If I understand the code correctly, then you are proposing to ignore hard-coded property names like userId and ownerId when ownerRelations is configured. I guess it makes sense, we must describe this difference in our documentation.

Please add unit-test(s) to verify that when ownerRelations is set, and there is no relation associated with userId/ownerId, then these two properties are ignored.

Done in 199fa03

bajtos · 2017-01-26T15:39:03Z

test/role.test.js

+      };
+
+      return Promise.all([
+        new Promise((resolve, reject) => {


Please promise-ify Role.isInRole and other related APIs you call from new tests. See #418 for background and https://github.com/strongloop/loopback/pull/1493/files for an example how to implement such change.

Would you mind to add Promise support in a new pull request first, before working on this feature? It will help us to keep the git history clean.

Would you mind to add Promise support in a new pull request first, before working on this feature?

What do you mean by this ?

We try to keep pull request small and have each request focused on a single thing. Adding Promise support AND allowing multi-owner Role resolution in the same pull request would go against that.

So I see two options how to move this pull request forward:

Rewrite your new tests to use callbacks instead of promises. This seems like a step back to me.

Start a new feature branch & a new pull request that will only add support for Promise style invocation to Role methods (and possibly other models you are going to invoke the tests you are adding here). When this other pull request is landed, you can simplify the code in this pull request by using the newly added Promise API.

Ok, you're definitely right. I find it uggly anyway ... However, do I need to add tests on the promisify support for role related things ?

Yes, at least basic smoke tests are needed when adding support for Promises to our API.

I went ahead and implemented the improvement myself - see #3146.

pierreclr · 2017-01-28T14:51:51Z

@bajtos i updated my tests with your promisify role lib ! However we need to merge this once ES6 polyfill and promisify role lib will be merged. Feel free to modify what you want and erase my commits history to keep the only added code.

Thank you

ebarault · 2017-02-04T10:52:40Z

@pierreclr @bajtos please see #3180
While not addressing you original concern specifically, i believe my changes solve the issue you wrote this PR for.
The block of code you're referring to here is no longer usable when having multiple user models in a loopback application and so consequently the only valid method is to resolve belongsTo relations

Also the PR's name is not good: it's not about supporting multiple (different) owners, it's about being able the resolve the owner through multiple belongsTo relations.

This should not be an option (referring to this) but the default behavior : from the moment the instance belongsTo the user by any of the possible existing relations, then the $owner role should be resolved truthy.

thoughts gentlemen?

pierreclr · 2017-02-04T13:51:35Z

Hi @ebarault ! That's what I thought first. However adding the capability to have multiple user models imply breaking changes on the way to resolve owner role (expect nosql connectors since the id's are uniques cross dbs / collections). I think the best way to handle this problem is the following :

keep the old way for loopback 2.X
Keep the flag on loopback 3.X and add documentation about the necessity to have this flag in case of multiple user models. Or only keep the isOwner resolving which fetch the belongsTo relation without giving the choice to the developper.

However, IMO this feature becomes crucial integrating multiple user models. (Think about an author and an editor which both are users in separate models. We definitely want a book owned by both of them ! On relations author and editor)

ebarault · 2017-02-04T15:54:49Z

Hi @pierreclr : could you detail the breaking change you're referring that would not be covered with the implementation i proposed in #3180 ? (similar to yours when there's a single user model)

all existing tests pass, and I don't have in mind a new test that could fail

pierreclr · 2017-02-04T16:00:56Z

@ebarault As already discuss and even if it was not very good for security, we used to resolve owner role on userId / owner property. In your PR you just (and only) fetch belongsTo relation (My initial PR did the exact same thing). But the braking change is the following. Considering a loopback app with model without any belongsTo relation BUT a userId property on it. Consider the actual way to resolve owner, here we are expecting to resolve the owner role on userId property which will not be the case fetching only belongsTo relations ...

https://github.com/strongloop/loopback/pull/3140/files#diff-b7e36d904fe1d290934c235affb649ecR609

ebarault · 2017-02-04T16:26:46Z

@pierreclr: hmm right, but this implementation is so limited, ... for example it does not allow someone to use different foreign keys name than userId or owner.
About security : imagine a model configured with strict mode false : if no specific policy exists, someone with write access on the model instance could do an updateAttribute to add the owner property referencing anybody else and provide him access.

I'd advocate that loopback 3 should not support this owner resolving mechanism and follows only belongsTo relations. loopback 2 would remain unchanged. Thoughts?

bajtos · 2017-02-09T11:17:50Z

@ebarault thank you for joining the discussion here! Please read also our previous discussion in #3106 to better understand the context.

In particular, #3106 (comment) describes my concerns related to backwards compatibility.

I'd advocate that loopback 3 should not support this owner resolving mechanism and follows only belongsTo relations. loopback 2 would remain unchanged. Thoughts?

IMO, changing the auth layer to follow only belongsTo relations is a breaking change, because existing applications based on LoopBack 3.x and depending on userId/owner property-based ownership will break after upgrading to a newer version of LoopBack.

Feature flags are the solution we are using in situations like this.

By default, the feature flag is turned off, therefore upgrading projects are not affected.
We can modify our project templates in https://github.com/strongloop/loopback-workspace/tree/master/templates to automatically enable feature flags for new projects.
When a new major version is around, we can remove feature flags that were introduced for backwards compatibility only.

However, in this particular case, I think it's important to allow users to decide whether they want all "belongsTo" relations to be treated as ownership (ownerRelations: true) or only selected subset (e.g. ownerRelations: ['user', 'owner']).

About security : imagine a model configured with strict mode false : if no specific policy exists, someone with write access on the model instance could do an updateAttribute to add the owner property referencing anybody else and provide him access.

Good point. Perhaps we should fix the code handling userId/owner properties (with no associated relation) to require these properties to be defined on the model? While it may still break existing applications, I think the security implications provide enough justification.

ebarault · 2017-02-09T13:39:47Z

@bajtos @pierreclr : a shame I missed the discussion from #3106

I am not using the $owner role because the behavior is not consistent to me, so I had to implement my own resolver to handle more complex logics. When required to refactor the isOwner method for the sake of the multiple user model PR, i understood then why I could not use this role resolver as I was expecting it to work and considered it more a bug than a feature : so I proposed that fix. But... okay, let's keep full backward compatibility. I'll keep focussed in the other PR to the fix required for multiple users. It should be merged before the present one.

About iterating through belongsTo relations: here is why i consider the way belongsTo relations are currently checked as a bug:

consider a model which is linked to two users (one being an owner (same id), the other not being an owner (different id)) by two different relations
consider now that the name of the relation for the second relation (with a non owner) comes before in the lexicographical order
Result : the check will callback false for the second relation even before checking the first relation which is the valid one, resulting in the isOwner returning inconsistent results depending on the names of the relations.

(ps. i considered here that for (var i in object) returns keys in lexicographical order, it's might not be the case, but anyway this does not change the nature of the problem)

About the security concern I mentioned : there are certainly others, that one came to me just because my knowledge of the underlying logic, we should tinker this more.

pierreclr · 2017-02-09T14:45:09Z

consider now that the name of the relation for the second relation (with a non owner) comes before in the lexicographical order
Result : the check will callback false for the second relation even before checking the first relation which is the valid one, resulting in the isOwner returning inconsistent results depending on the names of the relations.

@ebarault I don't understand that ... in case of a non owner relation to a user (that means relation type hasOne), the owner resolver will not try to fetch this relation since we only focus on belongsTo + isUserModel

raymondfeng · 2017-02-09T16:26:11Z

I have a suggestion: we can use ownerRelations to cover both properties and belongsTo relations for owner checking. For example, ownerRelations: ['ownerId', 'user'] will check ownerId (happens to be a property) followed by user (happens to be a relation). WDYT?

pierreclr · 2017-02-09T16:55:26Z

@raymondfeng I feel not comfortable with what you propose for the following reasons :
Considering two user models (authorId, editorId for example needs). Considering them both linked to MySQL datasource. Resolving the owner relation on a field in that case ispretty bad since two different users can have the same ids (however with mongodb it's ok). Indeed, my editor with id = 1 can access a resource of an author (which has the id=1 too) and imply security issue ... IMO, we should definitely get rid of resolving on property and only resolve belongsTo (to user class) relations. Let's keep the old way on LB2.X and go forward by fetching belongsTo relations. The breaking change is quite easy to handle because developper only needs to add relation

"user": {
   "type": "belongsTo",
   "model": "User",
   "foreignKey": "userId" / "owner"
}

and everything will work as expected :)

However, let me know what can I do to accelerate the merge :)

ebarault · 2017-02-09T16:57:57Z

@pierreclr: a model could have any number of belongsTo relation with other users.
consider this:

[invitation](fk: senderId=id1) --belongsTo--> user(id1)
[invitation](fk: guestId=id2) --belongsTo--> user(id2)

both users have hasMany relations with invitations, none of users them can "belongsTo" an invitation

sender has rights on invitation to display it, revoke it
guest has rights on invitation to display it, accept it

isOwner is a good starting point, complemented by logic inside the methods definitions, to enable such logic

ebarault · 2017-02-10T09:56:15Z

@bajtos, @raymondfeng, @pierreclr
any comment on what i depicted as a bug in my previous posts before we move on?

pierreclr · 2017-02-10T10:01:46Z

But I don't understand what is the bug you describe, I don't see any bug ! Before we stop the isOwner resolving at the first found belongsTo + userClass relation. That's the "bug" my PR resolve in order to have "Multiple owners role resolving" so what's the problem ?

ebarault · 2017-02-10T12:18:05Z

Well that's my point @pierreclr : the current implementation does not behave consistently and gives fluctuant results according to the relation name when there are multiple belongsTo relation: this should not be the case at all (meaning: not a by default behavior)
Yes your PR solves it.

upscreen · 2017-07-13T16:35:36Z

any updates on when this feature will be merged to the master??

bajtos · 2017-07-13T16:39:58Z

any updates on when this feature will be merged to the master??

That's a good question, indeed. IIRC, this pull request is being blocked by #3265.

@ebarault you used to have a much better understanding of the work being done here and in #3265 (which you authored). What's the status of these to pull requests? Do you have any bandwidth to finish them? If not, could you perhaps advise us if there is a way how to finish this pull request without waiting for #3265?

ebarault · 2017-07-14T09:33:13Z

Hi @bajtos,

Do you have any bandwidth to finish them? If not, could you perhaps advise us if there is a way how to finish this pull request without waiting for #3265?

yes i still plan to have those addressed. but i'm currently short on bandwidth. I may have a little more time to cover those in august.

Actually, #3265 was blocked by the deficient merge algorithm on models inheritance. Now that it's fixed #3265 can be resumed.

while I believe #3140 could be merged as-is, It's maybe preferable to wait for #3265 to land as it will help cleaning the code in #3140.

Lahrachtdi · 2017-08-30T09:30:42Z

can we just do something like this in the boot folder ?

module.exports = function definingDynamicRoles(app) {

  const Role = app.models.Role;

  Role.registerResolver('$owner', (role, context, next) => {

    const userId = context.accessToken.userId;
    const userPrincipalType = context.accessToken.$principalType;
    const tagetModelPrincipalType = context.modelName;
    const targetInstanceId = context.modelId;

    // Q: Is the user logged in? (there will be an accessToken with an ID if so)
    if (!userId) {
      return process.nextTick(() => next(null, false));
    }

    if (userPrincipalType !== tagetModelPrincipalType) {
      return process.nextTick(() => next(null, false));
    }

    if (parseInt(targetInstanceId, 10) !== userId) {
      return process.nextTick(() => next(null, false));
    }

    return process.nextTick(() => next(null, true));
  });

};

bajtos · 2017-09-20T10:19:13Z

Let's move this pull request forward. I have rebased the changes on top of the latest master and addressed the most important comments that were also easy to fix - see 44db862.

I'll wait few days to allow you (@pierreclr and @ebarault) to review my changes and the patch in whole, if you like to. If there are no objections raised, then I'll go ahead and land this pull request early next week.

Fix the code resolving OWNER role to correctly handle the situation where the target model has multiple "belongsTo" relations to the User model. Introduce a new model setting "ownerRelations" that enables the new behavior. When "ownerRelations" is set to true, then all "belongsTo" relations are considered as granting ownership. Alternatively, "ownerRelations" can be set to an array of the relations which are granting ownership. For example, a document can "belongTo" an author and a reviewer, but only the author is an owner, the reviewer is not. In this case, "ownerRelations" should be set to "['author']".

slnode · 2017-09-27T15:12:22Z

Can one of the admins verify this patch?

slnode · 2017-09-27T15:12:22Z

Can one of the admins verify this patch?

slnode · 2017-09-27T15:12:22Z

Can one of the admins verify this patch?

bajtos · 2017-09-27T15:45:00Z

Landed, thank you @pierreclr for the contribution 🎉 and sorry that it took so long to get it merged. Thanks @ebarault for your help along the way!

ebarault · 2017-09-27T19:56:22Z

Hi @bajtos !
Thanks for landing this commit as is.
For posterity, let's recall here the main problem yet to be resolved.

Sorry for the lack of support here, i really lack time these days.

cc: @lehni

lehni · 2017-10-05T17:09:13Z

Great! This fixed our problem in #3506!

@ebarault for the mentioned remaining problem that is still to be solved, wouldn't it be good to create a separate issue so that it does not get forgotten, linking it to this?

bajtos · 2017-10-06T08:14:36Z

for the mentioned remaining problem that is still to be solved, wouldn't it be good to create a separate issue so that it does not get forgotten, linking it to this?

+1

Could you please create the issue yourself, @lehni, to speed things up?

lehni · 2017-10-06T09:45:29Z

@bajtos I've created an issue for it now, not sure it captures all that is important: #3644
cc: @ebarault

bajtos · 2017-10-06T12:50:35Z

I've created an issue for it now, not sure it captures all that is important: #3644

@lehni this is great, thank you! 🙇

crandmck added the #community contribution label Jan 26, 2017

pierreclr mentioned this pull request Jan 26, 2017

Allow multiple ownership relations #3106

Closed

bajtos self-requested a review January 26, 2017 14:17

bajtos self-assigned this Jan 26, 2017

bajtos added the feature label Jan 26, 2017

bajtos reviewed Jan 26, 2017

View reviewed changes

ebarault mentioned this pull request Feb 3, 2017

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

Merged

2 tasks

bajtos mentioned this pull request Apr 5, 2017

How $owner is checked is too loose #2933

Closed

2 tasks

strongloop deleted a comment from slnode Sep 11, 2017

pierreclr requested review from clark0x, fabien and lehni as code owners September 20, 2017 10:16

bajtos approved these changes Sep 20, 2017

View reviewed changes

pierreclr requested a review from zbarbuto as a code owner September 27, 2017 15:12

bajtos changed the title ~~Multiple owners role resolving~~ Fix OWNER role to handle multiple relations Sep 27, 2017

bajtos added the needs-doc label Sep 27, 2017

bajtos merged commit 658d228 into strongloop:master Sep 27, 2017

lehni mentioned this pull request Oct 5, 2017

Unauthorized access permitted when using multiple user models #3506

Closed

lehni mentioned this pull request Oct 6, 2017

Role.isOwner() does not support polymorphic belongsTo relations to multiple User models #3644

Closed


		app.model(Message, {dataSource: 'db'});

		const password = 'pass';

Fix OWNER role to handle multiple relations #3140

Fix OWNER role to handle multiple relations #3140

Conversation

pierreclr commented Jan 26, 2017 • edited by bajtos Loading

Description

Related issues

slnode commented Jan 26, 2017

slnode commented Jan 26, 2017

slnode commented Jan 26, 2017

slnode commented Jan 26, 2017

slnode commented Jan 26, 2017

bajtos commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreclr commented Jan 28, 2017 • edited Loading

ebarault commented Feb 4, 2017 • edited Loading

pierreclr commented Feb 4, 2017

ebarault commented Feb 4, 2017

pierreclr commented Feb 4, 2017 • edited Loading

ebarault commented Feb 4, 2017 • edited Loading

bajtos commented Feb 9, 2017

ebarault commented Feb 9, 2017 • edited Loading

pierreclr commented Feb 9, 2017

raymondfeng commented Feb 9, 2017

pierreclr commented Feb 9, 2017 • edited Loading

ebarault commented Feb 9, 2017

ebarault commented Feb 10, 2017

pierreclr commented Feb 10, 2017

ebarault commented Feb 10, 2017 • edited Loading

upscreen commented Jul 13, 2017

bajtos commented Jul 13, 2017

ebarault commented Jul 14, 2017

Lahrachtdi commented Aug 30, 2017 • edited Loading

bajtos commented Sep 20, 2017

slnode commented Sep 27, 2017

slnode commented Sep 27, 2017

slnode commented Sep 27, 2017

bajtos commented Sep 27, 2017

ebarault commented Sep 27, 2017

lehni commented Oct 5, 2017 • edited Loading

bajtos commented Oct 6, 2017

lehni commented Oct 6, 2017 • edited Loading

bajtos commented Oct 6, 2017

pierreclr commented Jan 26, 2017 •

edited by bajtos

Loading

pierreclr commented Jan 28, 2017 •

edited

Loading

ebarault commented Feb 4, 2017 •

edited

Loading

pierreclr commented Feb 4, 2017 •

edited

Loading

ebarault commented Feb 4, 2017 •

edited

Loading

ebarault commented Feb 9, 2017 •

edited

Loading

pierreclr commented Feb 9, 2017 •

edited

Loading

ebarault commented Feb 10, 2017 •

edited

Loading

Lahrachtdi commented Aug 30, 2017 •

edited

Loading

lehni commented Oct 5, 2017 •

edited

Loading

lehni commented Oct 6, 2017 •

edited

Loading