If the rule does not return a false in the else, the failure handler never gets called about connect-roles HOT 12 CLOSED

That certainly shouldn't be the case. Looking at the code it's not what will happen. Can you show a working example that actually fails?

from connect-roles.

 roles.use('logged', function(req){
    if(req.user.isAuthenticated){
      return true;
    }
  }); 

roles.use('test', function(req){
//my level is actually is 3 so this will not return true
    if(req.user.level == 1){
      return true;
    }
  });
//if there is no else the req will pass and execute the callback;
app.get('/api/me', roles.is('logged'), roles.can('test'), users.me);

from connect-roles.

See if you can write a failing unit test and submit a pull request?

from connect-roles.

I've created unit tests and I couldn't reproduce the issue: https://github.com/ForbesLindesay/connect-roles/blob/master/test/index.js#L118+L146

If you can add a unit test that fails when it should pass, I'll reopen.

from connect-roles.

well I'm not familiar with any tests, nor mocha, I ran your test and yeah they are not failing but i don't think they are correct.

describe('when the first handler returns `false`', function () {
  it('requests are rejected', function (done) {
    roles.setFailureHandler(function () { done(); });
// here one of the use rules return false just like I said
    roles.use(function (req, action) { assert(action === 'any'); return false; });
    roles.use(function (req, action) { assert(action === 'any'); return true; });
//don't you have to check for different rules, and call them here both?
    roles.is('any')({}, {}, notCalled('next'));
  });
});
describe('when one handler returns `true', function () {
  it('requests are accepted', function (done) { 
// I don't think you have to accept that request, if the first doesn't return true it should redirect to failure
//and again you check only once, for 2 same actions
    roles.setFailureHandler(notCalled('failureHandler'));
    roles.use(function (req, action) { assert(action === 'any'); });
    roles.use(function (req, action) { assert(action === 'any'); return true; });
    roles.is('any')({}, {}, done);
  }

please explain me

from connect-roles.

Yes, we accept that request because one of the handlers returned true and none of them returned false. Consider the meanings:

• true => "accept the request"
• false => "deny the request"
• undefined/null => "not yet sure what to do with the request, ask one of the other handlers"

If none of the handlers know what to do, we should take the safe option and deny the request. The first handler to actually make a decision wins and the remaining handlers are ignored, a bit like middleware in express. Returning anything other than true or false is equivalent to calling next in express.

from connect-roles.

so if i return false where the rule should fail, everrything will be ok, my confusion was provoqued by your examples, nowhere you're returning false, for me that's a little strange for example the case

there is route i want to protect with 2 roles.is

roles.is('logged'), roles.is('moderator').

normally if the user is not logged in, the first route should directly redirect to failure
without need of checking the next rule in the stack

I guess I have to keep in mind to return the false if i don't want to continue testing for other rules

from connect-roles.

Generally I rarely return false in my examples. e.g.

roles.use('logged', function (req) {
  if (req.user && req.user.isAuthenticated) return true;
  //else fall through
})
roles.use(function(req, role) {
  if (req.user && req.user.isInRole(role)) return true;
  //else fall through
})

app.get('/edit', roles.is('logged'), roles.is('moderator'), ...)

In this case the roles.is('logged') check will fail if the user is not logged in, and then no further work will be performed. The failure handler will be called. Both the role handlers will be tested as part of the is('logged') test, because we haven't returned false. This is useful, for example, if there were a role called 'logged'

from connect-roles.

I made some tweaks to make it work as I expect. I didn't make pull request. But i have it in separate branch. after the explanations given, I finally understood how to deal with it.
cause my understanding of not returning true is falsy, it's not a shrodinger cat case. I would like to have pass or not pass, if pass check next , else fail.

https://github.com/talamaska/connect-roles/tree/stop-on-first-failed-rule

from connect-roles.

You can of course use your fork if you like. It defeats the entire purpose of this module though, which is to allow a series of rules to fall through until one of them makes a decision.

from connect-roles.

Using my way you can have also multiple rules, but I check them one by one. for example, is logged, is moderator, can edit skill. and fail on any and not verify other rules. This is big concept difference. You're right that it's changing the purpose of your module. I can change the name of my fork and refer to your repository in the readme and push it to npm, cause I would like to be available from npm install.

from connect-roles.

Btw, your tests are still passing after my changes. Could you please help me with modifying the tests so they match what I have changed in the module? I think only the last one should fail instead of passing. Still cannot get it why you are passing only 'any'?

from connect-roles.