Allow multiple ownership relations #3106

pierreclr · 2017-01-16T13:11:33Z

Description

This PR allows us to resolve multiple belongsTo role

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)

slnode · 2017-01-16T13:11:35Z

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-16T13:11:42Z

Can one of the admins verify this patch?

slnode · 2017-01-16T13:11:42Z

Can one of the admins verify this patch?

slnode · 2017-01-16T13:11:42Z

Can one of the admins verify this patch?

slnode · 2017-01-16T13:11:42Z

Can one of the admins verify this patch?

pierreclr · 2017-01-16T13:20:46Z

Sorry, I see that some points are not passing : One of my comment exceed the 50 characters size but needs to be removed anyway before merging (I guess). Could we integrate this check when passing test in local ? However any help would be appreciated for the rebase action ask by the linter bot

bajtos · 2017-01-16T16:49:33Z

Hi @pierreclr, thank you for the pull request. Don't worry about the git history now, we can fix it together later, when the code is good to be merged.

Are you trying to fix the same issue as #2454? cc @upupzealot

I need you to add unit-tests to verify that your implementation works as intended and also to prevent unintentional regressions in the future.

bajtos · 2017-01-16T16:49:40Z

@slnode ok to test

pierreclr · 2017-01-16T17:01:29Z

Hi @bajtos, I think you're right. I didn't see it (despite actively looking for it :/)

That's the same issue but I choose another implementation (I think having a counter to check if we have finished is quite tricky (see 8a497eb) and prefer to return the denied callback once every possibility has been tried.

However I see that I forgot to remove console.log ... Depending on the implementation you will choose I'll update my PR and write unit tests.

pierreclr · 2017-01-19T09:04:30Z

Hi @bajtos, @upupzealot is not maintaining his PR anymore I added unit tests to avoid unintentional regressions in the future.

Let me know when you plan to implement this feature because I really need to have that going to production soon. Thank you in advance

bajtos · 2017-01-19T12:57:44Z

common/models/role.js

@@ -171,13 +171,6 @@ module.exports = function(Role) {
  Role.isOwner = function isOwner(modelClass, modelId, userId, callback) {
    assert(modelClass, 'Model class is required');
    debug('isOwner(): %s %s userId: %s', modelClass && modelClass.modelName, modelId, userId);
-    // No userId is present
-    if (!userId) {


I think this block should be preserved. If there is no userId provided by the request context, then there is no point in running any checks - they must all fail.

Sorry, done in de8cc15

bajtos · 2017-01-19T13:11:30Z

common/models/role.js

+
+        if (rel.type === 'belongsTo' && isUserClass(rel.modelTo)) {
+          debug('Checking relation %s to %s: %j', r, rel.modelTo.modelName, rel);
+          if (inst[r](processRelatedUser)) return callback(null, true);


inst[r] is async function that may not return immediately, I am surprised this code actually works.

I think we should use async.some() to find whether any of the relation grants the owner role.

async.some( Object.keys(modelClass.relations), function(r, next) { var rel = modelClass.relations[r]; // ... next(null, matches(user.id, userId)); }, function(err, matchFound) { if (err) return callback(err); if (matchFound) return callback(null, matchFound); // handle userId/owner properties });

Indeed, something is wrong here

bajtos · 2017-01-19T13:34:41Z

Checking the userId or owner field for resolving owner role
is now done after fetching belongsTo relation to make possible
to have multiple resolver role

I am concerned about backwards-compatibility of such change.

Right now, when a model instance has userId or owner property, and its value is different from current user id, then isRole returns false, ignoring any relations. In your proposal here, the relations can overwrite the result of userId/owner comparison. I am concerned that there may be existing application that will break after such change.

I am proposing to add a feature flag to control this behaviour, defaulting to the old mode.

The algorithm in isRole should then become:

Check userId/owner properties using the current code
If one of them matches, then return true via callback
If none of them matches:
a. if the new flag is not set, then return false via callback
b. if the new flag is set, then continue to next steps
Iterate through relations to find if any of them grants the ownership

Now the remaining question is where and how to configure the feature flag. Consider the use case where a model has two relations to a User model, but only one of the relations is granting the ownership. For example, a Contract has an owner who can make changes (WRITE), and an auditor who can only view the data (READ). To support this case, one need to tell LoopBack which relations to examine when determining the ownership.

I am proposing to add a new model-level setting, e.g. ownerRelations, with the following values:

true - all relations to User-like models are examined
false - no relations are examined
An array of relation names - only the relations specified in the array are examined

This flag can be read via MyModel.settings.ownerRelations and configured e.g. via common/models/my-model.json.

@raymondfeng @superkhau do you have any opinion how to best design this feature?

pierreclr · 2017-01-19T13:39:52Z

common/models/role.js

+      // to have multiple resolver role
+      var ownerId = inst.userId || inst.owner;
+      // Ensure ownerId exists and is not a function/relation
+      if (ownerId && 'function' !== typeof ownerId) {


@bajtos Concerning your comment on backward compatibility, the fallback is here : I choose to first check the belongsTo relations and then fallback to userId / owner fields. So no problem of backward compatibility in case userId exist without belongsTo relation. However I feel not comfortable with that because it means that you can resolve the owner role without having any belongsTo relation defined (which was already the case)

However I feel not comfortable with that because it means that you can resolve the owner role without having any belongsTo relation defined (which was already the case)

While the scenario may look wrong to us, it is supported by the current LoopBack version, thus there may be existing application relying on it, and therefore we need to preserve it for backwards compatibility.

pierreclr · 2017-01-19T14:57:15Z

@bajtos Considering your comments I'll update my PR ASAP to make it clean

superkhau · 2017-01-19T19:09:40Z

@bajtos Your proposal sounds solid, my thoughts are pretty much the same as yours.

raymondfeng · 2017-01-19T19:16:19Z

@bajtos I would also like to allow model itself to provide isOwner(...) function to take over the check.

upupzealot · 2017-01-21T08:01:42Z

So will this PR be merged into master now? Or should we wait for a isOwner(...) solution mentioned above?

@pierreclr if you need to use this in your production, you may create a bootscript which redefine the isOwner method

pierreclr · 2017-01-21T12:45:34Z

@raymondfeng I don't really see why having an isOwner(...) method on a model (as prototype or static ?). I think this feature is a little out of scope of this PR and I don't see the use case of how can someone be owner of a ressource without belongsTo relation. In addition we already have Role.registerResolver(...) for resolving custom (owner or not) roles ...

bajtos · 2017-01-23T10:26:16Z

I would also like to allow model itself to provide isOwner(...) function to take over the check.

Makes sense.

I don't really see why having an isOwner(...) method on a model (as prototype or static ?). I think this feature is a little out of scope of this PR and I don't see the use case of how can someone be owner of a ressource without belongsTo relation. In addition we already have Role.registerResolver(...) for resolving custom (owner or not) roles ...

I agree adding model-specific isOwner is out of scope of this pull request, let's work in small incremental steps.

@pierreclr could you please rework your patch according to the proposal in my #3106 (comment)?

So will this PR be merged into master now?

@upupzealot The usual rules apply - we will merge it when our concerns were addressed and the code looks good. Perhaps you can work together with @pierreclr to make this happen earlier?

pierreclr · 2017-01-25T08:13:57Z

Hi @bajtos, sorry for the late reply. I can upgrade my PR according to your comment but I'm not familiar with async so I wondered If I can make it with bluebird Promises ? However, when you spoke about adding a new flag, I imagine that you want it at the application level (not model) ? Where can I find meaning example of how to handle that kind of things your way in loopback ? Thank you

bajtos · 2017-01-25T09:43:58Z

I'm not familiar with async so I wondered If I can make it with bluebird Promises ?

Our code base (the part which is published to npmjs) is still callback-based, we cannot use Promises in common, lib and server. I think you should not need to learn all async API, the async.some() method I pointed out in my #3106 (comment) should suffice.

The tests are different, using Promises in tests is the direction we want to take, so feel free to use Promises there! Just please try to preserve existing tests unchanged and use Promises only in the newly added code.

adding a new flag, I imagine that you want it at the application level (not model) ? Where can I find meaning example of how to handle that kind of things your way in loopback ?

See my previous comment #3106 (comment)

I am proposing to add a new model-level setting, e.g. ownerRelations, with the following values:

true - all relations to User-like models are examined

false - no relations are examined

An array of relation names - only the relations specified in the array are examined

This flag can be read via MyModel.settings.ownerRelations and configured e.g. via common/models/my-model.json.

Inside Role.isOwner() method, you can read the flag via modelClass.settings.ownerRelations.

To set the flag in your test model, add it to the third argument of app.registry.createModel, e.g.

app.registry.createModel('Message', {
  name: String,
  // ...
}, {
  relations: {
    // ...
  },
  ownerRelations: true,
  // or
  // ownerRelations: ['user', 'sender']
});

pierreclr · 2017-01-25T17:32:26Z

@bajtos working on it but I can't have modelClass.settings.ownerRelations in Role.isOwner() ...
Do you clean models before defining them to prevent any unauthorized flags ?

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

@bajtos, I created a new PR (#3140) because I restart from scratch and also because I don't know how to change my comparaison branch for PR across forks ..

bajtos · 2017-01-26T14:17:16Z

I created a new PR (#3140) because I restart from scratch and also because I don't know how to change my comparaison branch for PR across forks ..

In general, we prefer to keep the work in the same pull request, as it preserves all comments in a single place. That becomes a bit more difficult when the pull request was submitted from the master branch as you did here, so don't worry about it now. I see that you started to use a feature branch in #3140, that's the right way! 👍

You may find the following article about github and pull requests helpful: http://www.nearform.com/nodecrunch/first-time-with-open-source/

I am closing this pull request in favour of #3140

NEW fetch belongsTo relation first before resolving role

b091ea2

bajtos self-assigned this Jan 16, 2017

pierreclr added 2 commits January 16, 2017 18:02

Remove console.log

39c68af

NEW add unit test for handling multi owners role

0c1e15b

bajtos reviewed Jan 19, 2017

View reviewed changes

bajtos suggested changes Jan 19, 2017

View reviewed changes

bajtos mentioned this pull request Jan 19, 2017

Make things support multi-owner #2454

Closed

bajtos changed the title ~~NEW fetch belongsTo relation first before resolving role~~ Allow multiple ownership relations Jan 19, 2017

pierreclr commented Jan 19, 2017

View reviewed changes

pierreclr added 2 commits January 19, 2017 14:48

FIX deny owner resolve if no userId in request

de8cc15

Remove describe only

8d68b85

bajtos added the feature label Jan 26, 2017

pierreclr mentioned this pull request Jan 26, 2017

Fix OWNER role to handle multiple relations #3140

Merged

bajtos closed this Jan 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple ownership relations #3106

Allow multiple ownership relations #3106

pierreclr commented Jan 16, 2017 •

edited by bajtos

Loading

slnode commented Jan 16, 2017

slnode commented Jan 16, 2017

slnode commented Jan 16, 2017

slnode commented Jan 16, 2017

slnode commented Jan 16, 2017

pierreclr commented Jan 16, 2017 •

edited

Loading

bajtos commented Jan 16, 2017

bajtos commented Jan 16, 2017

pierreclr commented Jan 16, 2017

pierreclr commented Jan 19, 2017

bajtos Jan 19, 2017

pierreclr Jan 19, 2017

bajtos Jan 19, 2017

pierreclr Jan 19, 2017

bajtos commented Jan 19, 2017

pierreclr Jan 19, 2017

bajtos Jan 23, 2017

pierreclr commented Jan 19, 2017

superkhau commented Jan 19, 2017 •

edited

Loading

raymondfeng commented Jan 19, 2017

upupzealot commented Jan 21, 2017 •

edited

Loading

pierreclr commented Jan 21, 2017

bajtos commented Jan 23, 2017

pierreclr commented Jan 25, 2017

bajtos commented Jan 25, 2017

pierreclr commented Jan 25, 2017

pierreclr commented Jan 26, 2017

bajtos commented Jan 26, 2017

Allow multiple ownership relations #3106

Allow multiple ownership relations #3106

Conversation

pierreclr commented Jan 16, 2017 • edited by bajtos Loading

Description

Related issues

slnode commented Jan 16, 2017

slnode commented Jan 16, 2017

slnode commented Jan 16, 2017

slnode commented Jan 16, 2017

slnode commented Jan 16, 2017

pierreclr commented Jan 16, 2017 • edited Loading

bajtos commented Jan 16, 2017

bajtos commented Jan 16, 2017

pierreclr commented Jan 16, 2017

pierreclr commented Jan 19, 2017

bajtos Jan 19, 2017

Choose a reason for hiding this comment

pierreclr Jan 19, 2017

Choose a reason for hiding this comment

bajtos Jan 19, 2017

Choose a reason for hiding this comment

pierreclr Jan 19, 2017

Choose a reason for hiding this comment

bajtos commented Jan 19, 2017

pierreclr Jan 19, 2017

Choose a reason for hiding this comment

bajtos Jan 23, 2017

Choose a reason for hiding this comment

pierreclr commented Jan 19, 2017

superkhau commented Jan 19, 2017 • edited Loading

raymondfeng commented Jan 19, 2017

upupzealot commented Jan 21, 2017 • edited Loading

pierreclr commented Jan 21, 2017

bajtos commented Jan 23, 2017

pierreclr commented Jan 25, 2017

bajtos commented Jan 25, 2017

pierreclr commented Jan 25, 2017

pierreclr commented Jan 26, 2017

bajtos commented Jan 26, 2017

pierreclr commented Jan 16, 2017 •

edited by bajtos

Loading

pierreclr commented Jan 16, 2017 •

edited

Loading

superkhau commented Jan 19, 2017 •

edited

Loading

upupzealot commented Jan 21, 2017 •

edited

Loading