pawangspandey / accesscontrol-middleware

14 stars 6 forks source link

Validating user ownership #4

Closed BlueAccords closed 6 years ago

BlueAccords commented 6 years ago

Currently ownership validation is done by checking these two properties:

checkOwnerShip : true, // optional if false or not provided will check any permission of action
    operands : [
      { source : 'user', key : '_id' },  // means req.user._id (use to check ownership)
      { source : 'params', key : 'userId' } // means req.params.userId (use to check ownership)
    ]

Where checkOwnerShip should be true and the operands should be equal to each other.

My problem is that in typical relational databases, the ownership id of an object is typically separate from the actual id of the object itself. For example if there were two models of User and Comment:

User : {
  id,
  username,
  email
}

and

Comment: {
  id,
  body,
  owner_id
}

The comment's owner is determined by the owner_id rather than its own id. This means that you would need to make a call to the database using params.id.

In the provided example for this library it uses params.userId as the passed in parameter but I think that, typically speaking, you would normally structure the routes in a way where you're passing the model's id itself as the parameter, rather than the model's ownership id.

I'm not sure if this should be considered an issue with this library itself, or maybe its just that I structured my routes in a way that isn't 100% compatible with this library. Solving this would require some database agnostic way of querying a row by the passed in parameter and comparing the resulting ownership id of that row to the user.id.

pawangspandey commented 6 years ago

@BlueAccords, You could write a middleware where you can collect unknown data and assigned it to req object and use that middleware before accesscontrol-middleware. Now you can config the accesscontrol-middleware with available data.

Example if you want to perform delete operation on a comment. You could do like:

getCommetByIdMiddleware(req, res, next) {
// any db query.
req.comment = comment;
next();
}

comment.delete('comment/:commentId',
   passport.authenticate('any-authentication-strategy'), 
   getCommetByIdMiddleware, accessControlMiddleware.check({ 
        resource : 'comment',
        action : 'delete',
        checkOwnerShip : true,
        operands : [
          { source : 'user', key : '_id' },  // req.user._id
          { source : 'comment', key : 'owner_id' } // req.comment.owner_id
       ]
   }),
   deleteController
)
BlueAccords commented 6 years ago

Alright thanks! I wrote my own implementation in a fork so I could check it in one middleware but it's directly reliant on knex.js as the db connection so it can't really be merged or anything.

Either way my problem is solved.