Comments (12)
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.
Related Issues (20)
- Migrate Promise.from -> Promise.resolve HOT 1
- Cannot read property from mongoDB HOT 3
- Is Express 3 still supported ? HOT 1
- express deprecated res.send(status) Use res.sendStatus(status) instead HOT 2
- how do you use connect-roles with fullstack meanjs HOT 1
- The link in description is broken
- Readme.md HOT 2
- Conditional with handlebars HOT 1
- How to use passport Auth along with this ? HOT 1
- Chaining User.is() checks doesn't work HOT 1
- TypeError: roles.test is not a function HOT 2
- Mongodb built-in roles HOT 4
- Separate rules in modules HOT 2
- How to set roles? HOT 1
- An in-range update of mocha is breaking the build 🚨 HOT 5
- connect-roles is skipping a strategy in my code HOT 2
- Version 10 of node.js has been released
- update permission on database HOT 1
- An in-range update of mocha is breaking the build 🚨 HOT 11
- An in-range update of promise is breaking the build 🚨 HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from connect-roles.