ForbesLindesay / connect-roles

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

How do you actually check for role with is()? #13

Closed talamaska closed 11 years ago

talamaska commented 11 years ago

I'm not getting it. I see you have to use .use() for defining .can()

app.use('access private page', function (req) {
  if (req.user.role ==== 'moderator') {
    return true;
  }
})

app.get('/private', user.can('access private page'), function (req, res) {
  res.render('private');
});

But how do you define is('admin') Is that the example?

user.use(function (req) {
  if (req.user.role === 'admin') {
    return true;
  }
});

//and later use
app.get('/admin', user.is('admin'), function (req, res) {
  res.render('admin');
});

I think I'm missing something. there should be a way to define the role check. Maybe i have to define like this?

user.use('admin', function (req) {
  if (req.user.role === 'admin') {
    return true;
  }

Is it expected to be recorded as string in req.user object? I have an existing User model where i have level : 1,2,3, instead of role:'admin','moderator'?

user.use('admin', function (req) {
  if (req.user.level=== '3') {
    return true;
  }

Sorry if the answer for you is obvious, i just don't see it.

ForbesLindesay commented 11 years ago

see: https://github.com/ForbesLindesay/connect-roles#rolesusefnreq-action

user.use(function (req, role) {
  if (req.user.role === role) {
    return true;
  }
});

//and later use
app.get('/admin', user.is('admin'), function (req, res) {
  res.render('admin');
});

Alternatively you can use something more complex:

var roles = [null, 'basic', 'moderator', 'admin']
user.use(function (req, role) {
  if (req.user.level=== roles.indexOf(role).toString()) {
    return true;
  }
});

Something like:

user.use('admin', function (req) {
  if (req.user.role === 'admin') {
    return true;
  }
})

would also work of course, but it would only check for the 'admin' role and would not support other roles such as 'moderator'.

How you record the information inside the user object is entirely up to you, you just have to write a function that reconciles that data model with the string representing a role or an action.

It's worth reminding people as well that is and can are just aliases of each other. The only reason they're there is because it's ugly to say user.is('edit page') and it's ugly to say user.can('admin') but both would work just fine.

talamaska commented 11 years ago

OK, but now i have another problem. without your module, my passport has it's data correct

var property = 'user';
console.log(this[property]);
undefined //(if i'm not logged)
//else a user object gotten from my store, remember that password is doing that for you
//so it's function isAuthenticated return 
return (this[property]) ? true : false;

when your modules alter something

var property = 'user';
console.log(this[property]);
//becomes
{ isAuthenticated: false, is: [Function], can: [Function] }
//so
return (this[property]) ? true : false;
// is returning true always, if i continue to rely on req.isAuthenticated() I'm screwed;

You have your own function isAuthenticated which is checking things differently, but you suggested to use altogether with passport, but did nothing to not break it.

I don't understand why are those methods binded to this['user'] you can check that in requests.js in passport.js library.

I'd expect connect-roles to not mess up req.user object, I see this extra for you, but can i turn it off. after all it's just middleware, should not mess up my existing modules

ForbesLindesay commented 11 years ago

If you don't want it to touch the user object, just leave out the app.use(require('connect-roles')) That way it won't ever add the user. Everything will function as normal except you won't get access to req.user.is, req.user.can or user.is and user.can in views.

It doesn't work well if you're only partially committed to using this module. It's intended for use at an application level and not a module level. With this in mind, it's better to consistently have access to authorization functions via req.user than sometimes have them absent. I also feel that req.user.isAuthencitated is much more explicit than req.user. Being explicit is nice in this context because conceptually there is always a user, it's just that the user might not be authenticated.

talamaska commented 11 years ago

For me the same can be said for a 'role' keyword, as you say everyone has it, even the unauthenticated request has a role - isAuthorized : false. I see other people may have issues. It's good that the code is evolving and you are up soon with next major version. We are just asking for more flexibility.

On 09.07.2013, at 19:01, Forbes Lindesay notifications@github.com wrote:

If you don't want it to touch the user object, just leave out the app.use(require('connect-roles')) That way it won't ever add the user. Everything will function as normal except you won't get access to req.user.is, req.user.can or user.is and user.can in views.

It doesn't work well if you're only partially committed to using this module. It's intended for use at an application level and not a module level. With this in mind, it's better to consistently have access to authorization functions via req.user than sometimes have them absent. I also feel that req.user.isAuthencitated is much more explicit than req.user. Being explicit is nice in this context because conceptually there is always a user, it's just that the user might not be authenticated.

— Reply to this email directly or view it on GitHub.