-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow querying with role arrays #12
Conversation
include tests for oop interface
update mocha and add editorconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of whitespace nitpicks and one legitimate suggestion on the README changes.
README.markdown
Outdated
Role and Resource Discovery | ||
--------------------------- | ||
|
||
If you are more of an object-oriented programmer and prefer to use objects | ||
to represent your roles and resources, then you're in luck! Virgen-ACL can | ||
discover roles and resources from your objects so long as your role objects | ||
contain the property `role_id` OR a function `getRoleId()` and your resource | ||
contain the property `role_id` OR a function `getRoleId()` and your resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an extra space. Can't have that.
README.markdown
Outdated
objects contain the property `resource_id` OR a function `getResourceId()`. | ||
Valid value types for role_ids are strings, `null` or arrays of strings. Valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the use of plurals correct here? strings
and arrays
makes it sound like multiple arguments are allowed, but that wouldn't make sense in this case.
lib/acl.js
Outdated
for (var i in roles) | ||
for (var j in resources) | ||
for (var k = this.permissions.length - 1; k >= 0; k--) | ||
for (var k = this.permissions.length -1; k >= 0; k--) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space between the minus and the one.
test/index.js
Outdated
}); | ||
|
||
for (var i in roles) (function(role) { | ||
for (var j in resources) (function(resource) { | ||
for (var k in actions) (function(action) { | ||
if (role == 'user' && resource == 'page' && action == 'view') { | ||
var allowableRole = false; | ||
if( role === 'member' || (Array.isArray(role) && role.indexOf('member') !== -1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space in the wrong place. Sorry about all the space related comments!
test/index.js
Outdated
@@ -374,7 +394,12 @@ require('should'); | |||
for (var i in roles) (function(role) { | |||
for (var j in resources) (function(resource) { | |||
for (var k in actions) (function(action) { | |||
if (role == 'admin') { | |||
var allowableRole = false; | |||
if( role === 'admin' || (Array.isArray(role) && role.indexOf('admin') !== -1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👾
test/index.js
Outdated
if (role == 'guest') { | ||
it('should deny all resources to globally denined role', function() { | ||
var deniableRole = false; | ||
if( role === 'guest' || (Array.isArray(role) && role.indexOf('guest') !== -1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you're doing it on purpose at this point.
test/index.js
Outdated
this.acl.query(role, resource, action, function(err, allowed) { | ||
allowed.should.equal(false); | ||
}); | ||
}); | ||
} else { | ||
it('should allow resources to all other roles', function() { | ||
it(`should allow ${action} of ${resource} to ${role}`, function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should be async.
test/index.js
Outdated
@@ -479,13 +537,13 @@ require('should'); | |||
for (var j in resources) (function(resource) { | |||
for (var k in actions) (function(action) { | |||
if (resource == 'blog') { | |||
it('should deny all roles to globally denined resource', function() { | |||
it(`should deny ${role} to blog`, function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should also be async.
test/index.js
Outdated
this.acl.query(role, resource, action, function(err, allowed) { | ||
allowed.should.equal(false); | ||
}); | ||
}); | ||
} else { | ||
it('should allow roles to all other resources', function() { | ||
it(`should allow ${role} to ${resource}`, function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should also be async.
Closes #10