djvirgen / virgen-acl

A simple ACL for node.js
MIT License
74 stars 19 forks source link

synchronous query #4

Closed smhg closed 7 years ago

smhg commented 11 years ago

I'm trying to use virgen-acl inside a (client-side) Handlebars template helper so my templates can contain something like:

{{#can "edit" blog}}<button>Edit</button>{{/can}}

The can helper gets the current user and queries the acl similar to:

acl.query(user, resource, action, function (err, allowed) {
  if (allowed) {
    return options.fn(self);
  }
});

This can not work however since Handlebars helpers can only return synchronous results.

Would you support turning query into a sync method if no callback is specified? I'm happy to contribute.

djvirgen commented 11 years ago

Sync assertions would be a great addition, but I'm concerned about custom assertions that require async calls. In that case, I don't think you'll be able to get your assertion result synchronously, which can make it challenging to use in templates. This could lead to troublesome bugs down the road if a synchronous assertion was updated to make an async call.

Have you considered using promises or query()ing before loading the template?

smhg commented 11 years ago

That is a very good remark. There is no way to enable both features? Obviously you would not be able to use them together, but with some deeper changes to the API it might be possible to keep things separated?

For large client-side (think backbone) apps, it is good to use an ACL to keep things clean. Async checks will hardly ever be used while being able to query in a synchronous way seems quite useful. I understand this is different for the server.

Querying beforehand without promises seems messy. Promises might clean it up a bit, but booleans for each query result would still have to be assigned to templates (no idea yet, but I expect there to be many).

djvirgen commented 11 years ago

This is definitely a challenging problem to solve. Thanks for bringing it up because it really got me thinking about how best to solve it.

It would be possible to implement your idea of returning the assertion if a custom callback isn't registered for that query, but I'm worried that may be too limiting.

I'm assuming your query {{#can 'edit' blog}} will need to be true for the owner of the blog but false for everyone else. IMO, the best way to solve this is with a custom assertion callback -- which wouldn't help in your case.

Another approach would be to dynamically set up ACL rules within the context of the currently logged-in user, which would eliminate the need for the callback in this case but also limit you in other cases. For example, if you need to build an admin interface that lists all of the site's users and their permissions, then you may not get the results you expect from the ACL.

After thinking this through I'm still leaning towards querying the ACL before rendering the view, perhaps even server-side if that works for you. That way when you get your object from the server, it can contain a map of permissions ready for presenting:

GET /user/foo/blog/bar

{
  "title": "Bar Blog",
  "content": "...",
  "resourceId": "blog:bar",
  "permissions": {         // a map of permissions for the requesting user, based on session cookie
    "edit": true,          // user can edit his own blog
    "unpublish": true,     // user can unpublish his own blog
    "report_spam": false   // user cannot report their own blog for spam
  }
}
smhg commented 11 years ago

Just a first partial response: In the example of the edit-blog permission, you are right that it indeed needs to be performed by a custom callback. This however would not really put a requirement on the query method to be async. Without having looked at it well enough, I guess it could loop through all permissions, calling callbacks when needed, and returning a result in an synchronous manner.

smhg commented 11 years ago

Related to the timing of ACL querying: I naturally lean more towards an approach where you query at the time and place where you actually need the information. Creating a list of permissions up front and supplying that to the view might add a few permissions that are actually never evaluated. Of course this does not create tremendous problems, but in the client it is all a workaround because of a use case (async lookups) which is not very common (and probably should be avoided). Again: I understand the focus is on the server and I truly understand your approach and reasoning.

smhg commented 11 years ago

One way I could imagine this to work without throwing everything upside-down:

Actually you could probably just check the callback parameter and handle everything inside the current query method.

nervgh commented 7 years ago

I have a backend on koa@2 which uses async functions (promises) and I'm viewing your package right now. It looks optimistic :+1: The slogan says

Simple in-memory ACL for node.js apps

And I think why does "in-memory ACL" require callbacks? :smirk_cat: Oh, don't worry about that. These are just my minds :smile_cat:

spruce-bruce commented 7 years ago

It uses callbacks because it's possible to do async work in the callbacks. This lets you fetch things from the database yourself to do custom logic to determine if your role really does have access to that resource.

smhg commented 7 years ago

I'm going to close this as I have no more use for this myself. Thanks for your feedback!