Extend ACL with Relation Support

[ritch]

#115 brings up an issue that can only be fixed by extending ACLs with specific support for relations.

In this thread lets discuss the feature and track the work for implementing it.

We should also add example usage of this new feature to the access control example.

@raymondfeng @chandadharap @bajtos

[bajtos]

An idea to consider:

The remoting metadata already has accessType option allowing the method to specify READ/WRITE instead of EXECUTE.

Perhaps we can add accessTarget option that will allow the method to specify the model to use for checking the access control?

For example, Product.prototype.__get__categories can have accessTarget: 'Category'.

define('__get__' + relationName, {
  isStatic: false,
  // ...
  accessType: 'READ',
  accessTarget: relation.modelTo
});

The permission check for GET /api/products/1/categories then must perform two steps:

1. Verify that the caller is allowed to access the parent model instance (Product id 1).
2. Verify that the caller is allowed to execute the operation on the target model (READ Categories).

What is not clear: how to deal with writes, does POST /api/products/1/categories require write permissions for both Product and Category, or is it sufficient to havea READ permission for Product and WRITE permission for Category?

[ritch]

accessTarget seems like exactly what we need. We can just call checkAccess() on the target instead of the "from" model.

Verify that the caller is allowed to access the parent model instance (Product id 1).

Why do we need this? I guess this could be accomplished by calling checkAccess on both the accessTarget and the original parent model.

So to illustrate the check:

/api/products/1 => READ (Product.findById()) Product as $ROLE

/api/products/1/categories =>
  READ (Product.findById()) Product as $ROLE
  and
  READ (Category.find()) Category as $ROLE

What is not clear: how to deal with writes, does POST /api/products/1/categories require write permissions for both Product and Category, or is it sufficient to havea READ permission for Product and WRITE permission for Category?

Hmm... This makes me think that we should treat these methods as sugar and just apply the ACLs as if they were accessed without a relation. If we do this the accessType and accessTarget would be used to form a new AccessContext and a single check would be performed:

/api/products/1/categories => READ (Category.find()) Category as $ROLE

[ritch]

During an offline discussion @raymondfeng brought up an interesting alternative. To pass the remote context around the call stack as part of the options param.

The implementation of this has several parts.

1. injecting the remoteContext into the options argument

remotes.before('**', function(ctx, next) {
  if(theMethodToInvokeHasAnOptionsObject(ctx)) {
    var options = ctx.args.options = ctx.args.options || {};
    options.remoteContext = ctx;
  }

  next();
});

2. check access in all DAO methods by using observers

// add this to each DAO method
if(options && options.remoteContext) {
  // new method based on Model.checkAccess()
  Model.checkAccessForContext({
    method: 'create', // or the method
    modelId: modelId,
    remoteContext: options.remoteContext,
  }, continueWithDaoMehtod);
}

3. ensure options.remoteContext is passed in every DAO call (especially the relation sugar methods)

[ritch]

I'm leaning towards the simpler solution: adding an accessTarget to the remoting metadata and treating __get__categories exactly like Categories.find().

[bajtos]

There is one thing I dislike about options.remoteContext: it's introducing coupling between strong-remoting context object, loopback's checkAccessForContext and loopback-datasource-juggler.

At the moment, juggler is a standalone library that can be used on its own and I'd like to preserve that, as it is a good design decision IMO.

Having said that, I can imagine we can rework this initial proposal to make it more generic and fit better into the standalone juggler. For example, instead of options.remoteContext, we can pass options.principal and expect Models to override Model._authorizeOperation({ operation, instanceId, principal }). Operation = method name, e.g. "create" or "updateOrCreate". instanceId = the id of the affected model instance, undefined for "create".

Now it all depends on how much time we are willing to invest into this. Providing the current user in the options object will allow us to implement acl-based filtering in queries, i.e. the find method can return only the items the user can read. (At the moment, it returns 401 unless the user can read everything.) On the other hand, this solution will need more time to implement compared to the accessTarget alternative.

I am personally fine with both ways.

[bajtos]

Actually, the solution based on options.remoteContext has a major issue: the authentication will be performed by DAO methods, meaning that any strong-remoting hooks will be executed before the user is authorized to perform the operation - see #1191.

In the light of that, I am leaning towards accessTarget too.

[ritch]

There is a problem with accessTarget... it is not always as straightforward as the model at the other end of the relation. Consider include=secretModel. The real accessTarget is dynamic. We can resolve this during checkAccess() but only by coupling the relation methods with checkAccess().

I think passing around an accessContext when a method is invoked remotely is a pretty interesting idea. Its definitely not as easy to implement, but it is a clean way to enforce access control whenever a set of methods are called remotely.

The downside is that we are making remote methods potentially less useful when not called remotely.

[ritch]

I think passing around an accessContext when a method is invoked remotely is a pretty interesting idea. Its definitely not as easy to implement, but it is a clean way to enforce access control whenever a set of methods are called remotely.

Now it all depends on how much time we are willing to invest into this - @bajtos

What makes this hard to implement is the missing options object in the scope remoting methods (eg. here) Since the argument parsing there is already fairly complex it isn't trivial to add support for an options object.

The other tricky part is ensuring we pass the options.accessContext around for every DAO to DAO call. This is less tricky actually. The other parts listed below are actually pretty easy to implement

• injecting the options.accessContext
• checking access by observing Model operations that include a options.accessContext

[ritch]

At the moment, juggler is a standalone library that can be used on its own and I'd like to preserve that, as it is a good design decision IMO.

I don't think we would lose this. If you don't pass the options.accessContext it will continue to work like normal.

[ritch]

Keep in mind... what actually might make this worthwhile is laying the ground work for options.accessContext.transaction. Certain connectors can use this to implement transactions.

So the problem now is that in order to fix something that otherwise seems like a fairly small issue we have to add a huge feature. :(

[ritch]

So after much deliberation @bajtos @raymondfeng and I have agreed on adding a new authorize hook for each remote method.

We'll use this hook to implement a relation specific check to ensure the principal has access to all requested models.

/customers/1/reviews?include=secrets

Will result in these checkAccess calls:

 - Customer.findById(1) => Customer.checkAccess() (from enableAuth())
 - Reviews.find({where: ...}) => Reviews.checkAccess()
 - Secrets.find() => Secrets.checkAccess()

[ritch]

@raymondfeng is adding context propagation for passing around transactions. It looks like this can be used to implement his originial suggestion to enhance the access control checking for relations (and include).

[fabien]

@ritch +1 hope to see this merged soon. Thanks.

[GeoffreyEmery]

+1 on commit soon

[colin-welch]

This looks like a promising solution. What's the status of the implementation these days?

[mohitgoyal707]

Hello everyone. I am facing the similar issue on my app. My product is about to go live within 1 month but recently i noticed flaw in ACL leading to access of sensitive data via inclusion. I can't disable include filter because i am fetching alot of data using the inclusion filter along with access token. Can you please give me a workaround or tell me when can i expect the issue to be resolved?

[pulkitsinghal]

@ritch and @bajtos - how do users of loopback get insight into when issues are realistically expected to be worked on? No doubt you are all very busy and I understand that PRs are welcome but setting expectations must also be key. Does IBM have any plans to help by beefing up either the dev efforts and/or transparency?

[raymondfeng]

@mohit1993 You can set disableInclude per relation. Would that help?

[mohitgoyal707]

On Fri 22 Jul, 2016, 9:11 PM Raymond Feng, [email protected] wrote:

@mohit1993 https://github.com/mohit1993 You can set disableInclude per
relation. Would that help?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1362 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AE_TakMV1VXgcnOATsZbXsHcVSEWHVXuks5qYOMRgaJpZM4EQsKt
.

But i wanted to use inclusion with access token.

Regards
Mohit Goyal

[madianas]

@raymondfeng Using disableInclude does not work, because some dynamics roles need to use the include feature.

Would it be possible to add something similar to ACL, but specific to the "include". Whenever nested include is used it would check each models "inclusion" tag and run. This way each model will decides access by role.

[mitsos1os]

What is the status on this issue??? This should definitely be implemented in next versions... You should no way be able to access a model B without read access just because you have access to model A which is related to model B...

This can be easily reproduced at example repo. If you convert all ACLs to a single deny all you should not be able to access anything. However accessing the user/:id with filter={"include":"projects"} then voila... user projects, although ACL denies...

[eisaqasemi]

here is a workaround which works with one level deep include but not with nested include.

let remotes = server.remotes();
  if (remotes.authorization) {
    let authorization = remotes.authorization;
    remotes.authorization = function (ctx, next) {
      authorization(ctx, function (err) {
        if (err && (err !== 'route')) {
          next(err)
        } else {
          //if request has include filter
          if (ctx.args && ctx.args.filter && ctx.args.filter.include) {

            let model= ctx.method.ctor;

            async.each(
              model.normalizeInclude(ctx.args.filter.include),
              (include,callback)=> {
                let relation;
                if(typeof include === 'string'){
                  relation = include;
                }else if(
                  include.constructor.name === 'Object' &&
                  ( rel = (include.rel || include.relation))
                ){
                  relation = rel;
                }

                let ctxCloned= Object.assign({},ctx);
                ctxCloned.method= remotes._classes[model.modelName].
                findMethodByName('prototype.__get__'+relation);

                authorization(ctxCloned,function (err) {
                  if (err && (err !== 'route')){
                    callback(err);
                  }else{
                    callback();
                  }
                })
              },
              (err)=> {

                if(err){
                  return next(err);
                }
                next();

              });
          // if does not have include filter
          }else{
            next();
          }

        }
      });
    }
  }

[caleuanhopkins]

@mitsos1os I've just found out this can stop the includes bug:

"relations": {
    "projects": {
        ...
            "options": {
                "disableInclude": true
            }
        ...
    }
}

[mitsos1os]

@caleuanhopkins This indeed complies with security, but it simply disables the feature. The best option would be to have the feature but propagate proper permissions to included models...

[bajtos]

With the release of LoopBack 4.0 GA (see the announcement), this module has entered Active LTS mode and no longer accepts new features (see our LTS policy).