ForbesLindesay / connect-roles

Provides dynamic roles based authorisation for node.js connect and express servers.
749 stars 62 forks source link

Async AuthorisationStrategy? #3

Open avimar opened 11 years ago

avimar commented 11 years ago

I wasn't sure where else to ask this. How do I do an async auth strategy?

e.g. I have a role of editing their own resources -- so I match the credentials to the resource edit request.

But this new case is I want to allow the owner of many sub-accounts to edit the resource. So in that case, I need to do a query to find the owner of that account. (But only if other role-checks fail)

(or maybe I can do this a different way...)

So, is there some way to do async in the AuthorisationStrategy? there's no next(); to not call like in connect middleware...

Thanks!

ForbesLindesay commented 11 years ago

At the moment this only supports synchronous strategies. The reason for this is that I wanted to support things like:

app.get('/home', function (req, res, next) {
  if (req.user.is('admin')) {
    res.render('/home/admin');
  } else {
    res.render('home/user');
  }
});

Which wouldn't work with an async API.

It would be cool to have a similar module that did support async, but I don't think it can sensibly be added to this module.

avimar commented 11 years ago

I see.

This is a bit of a hack.. but how about running a middleware before connect-roles that saves the required information into the req object (and then calls next()). I'll give that a try later today...

ForbesLindesay commented 11 years ago

Yes, I should've commented on that, because this inside the authenticationStrategies refers to the request object, you can load anything you need in advance and put it in the request object. That, for example, is how I tend to load the user itself from the database.

The only downside of doing that it that sometimes you'll be running un-necessary database queries, whereas async authentication strategies would let you run them only when needed.

valerianrossigneux commented 10 years ago

do you have any example of this middleware that would load data into the request object ?

avimar commented 10 years ago

It's just a middleware that gets run first, something like this:

function checkLogin(req, res, next, hash){
            //do some checks!
            req.isAuthenticated=true;
            req.user={
                isAuthenticated:true
                ,username:username
                };
                next();//run this once all the async things are finished.
            }
ForbesLindesay commented 10 years ago

The next version will also support an alternative option whereby all authorisation strategies are allowed to return Promises rather than true/false. This lets them be async, but also means that calls like req.userCan will return a Promise. To enable this, you do:

var ConnectRoles = require('connect-roles');
var roles = new ConnectRoles({async: true});

It's still totally un-tested and un-documented though, so I'm going to leave this open until that gets resolved.

mike-grayhat commented 10 years ago

It works, thank you!

JorgeLGonzalez commented 8 years ago

Any new thoughts on the promise-based solution for async authorisation strategy? Works like a charm for me!

dskrvk commented 7 years ago

Can this be closed now?

mikeatlas commented 7 years ago

Works for me too.

Note that it needs to only return Promises which either resolve(true) or resolve(false), but not to use reject(...):

permission.use('get stuff', req => (
  new Promise((resolve) => {
    if (userCanDoStuff(req)) { return resolve(true); }
    return resolve(false);
  })));
mvolkmann commented 6 years ago

Yes, works great for me also! Can the docs be changed to no longer say that the "async" option is experimental? My use case is to query a database to get the roles of the user which is why I needed the callback function of the "use" method to return a Promise. I would guess this is a common scenario. It would be great to show one example of an async function being passed to the "use" method in the docs.

ForbesLindesay commented 6 years ago

@mvolkmann Yes, if someone adds a test case to cover it.