Skip to content
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

Merged
merged 16 commits into from
Mar 15, 2017
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions README.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,25 @@ Usage
}
});

// supports multiple roles in query
acl.query(["member", "admin"], "blog", "create", function(err, allowed) {
if (allowed) {
// creating allowed!
} else {
// no creating allowed!
}
});

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
Copy link

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.

objects contain the property `resource_id` OR a function `getResourceId()`.
Valid value types for role_ids are strings, `null` or arrays of strings. Valid
Copy link

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.

value types for resource_ids are `null` or strings.
Here's an example of how that might work:

// User class
Expand Down Expand Up @@ -133,7 +144,7 @@ Sometimes you need more complex rules when determining access. Custom
assertions can be provided to perform additional logic on each matching
ACL query:

acl.allow("member", "blog", "edit", function(role, resource, action, result, next) {
acl.allow("member", "blog", "edit", function(err, role, resource, action, result, next) {
// Use next() if unable to determine permission based on provided arguments
if (!(role instanceof User) || !(resource instanceof Blog))
return next();
Expand Down
42 changes: 35 additions & 7 deletions lib/acl.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,38 @@ Acl.prototype.query = function(role, resource, action, done) {
// LIFO
var roles
, resources
, matches = [];
, matches = []
, extractedRole = extractRole(role);

// LIFO loop, starting with specified role/resource and moving up through parents
roles = getParentRoles.call(this, role);
roles = isArray(extractedRole)
? getParentRolesFromArray.call(this, extractedRole)
: getParentRoles.call(this, extractedRole);

resources = getParentResources.call(this, resource);

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--)
Copy link

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.

for (var i in roles)
for (var j in resources)
if (this.permissions[k].match(roles[i] || null, resources[j] || null, action || null)) {
matches.push(this.permissions[k]);
}

var pl = new PermissionList(matches, role, resource, action, done);
var pl = new PermissionList(matches, extractedRole, resource, action, done);
pl.next();
};

// Private
var getParentRolesFromArray = function(role) {
var roles = [];
for (var i in role) {
var parentRoles = getParentRoles.call(this, role[i]);
parentRoles = parentRoles.filter(function (item, pos) { return roles.indexOf(item) < 0 });
roles = roles.concat(parentRoles);
}

return roles;
}

var getParentRole = function(role) {
return this.roles[role] || null;
Expand All @@ -65,7 +79,7 @@ var getParentRoles = function(role) {
};

var getParentResources = function(resource) {
var resources = [resource];
var resources = [];

do {
resources.push(resource);
Expand All @@ -82,4 +96,18 @@ var isArray = Array.isArray || function (vArg) {
return Object.prototype.toString.call(vArg) === "[object Array]";
};

function extractRole(role) {
if (typeof(role) == 'string' || isArray(role)) {
return role;
} else if (null === role) {
return null;
} else if (typeof(role.getRoleId) == 'function') {
return role.getRoleId();
} else if (typeof(role.role_id) == 'string' || isArray(role.role_id)) {
return role.role_id;
} else {
throw "Unable to determine role";
}
};

module.exports = Acl;
16 changes: 1 addition & 15 deletions lib/permission.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ function Permission(role, resource, action, allowed) {

Permission.prototype.match = function(role, resource, action) {
var result = null
, _role = extractRole(role)
, _role = role
, _resource = extractResource(resource);

switch (true) {
Expand Down Expand Up @@ -43,20 +43,6 @@ Permission.prototype.query = function(role, resource, action, done, next) {

// Private

function extractRole(role) {
if (typeof(role) == 'string') {
return role;
} else if (null === role) {
return null;
} else if (typeof(role.getRoleId) == 'function') {
return role.getRoleId();
} else if (typeof(role.role_id) == 'string') {
return role.role_id;
} else {
throw "Unable to determine role";
}
};

function extractResource(resource) {
if (typeof(resource) == 'string') {
return resource;
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
"test": "mocha"
},
"devDependencies": {
"mocha": "~1.12.0",
"should": "~1.2.2"
"mocha": "~3.2.0",
"should": "~11.2.0"
},
"keywords": [
"acl"
Expand Down
78 changes: 68 additions & 10 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,26 @@ require('should');
(function() {
var assert = require('assert')
, Acl = require('../lib').Acl
, roles = ['admin', 'member', 'guest']
, roles = ['admin', 'member', 'guest', ['member', 'admin'], ['member', 'guest'], ['guest', 'member']]
, resources = ['blog', 'page', 'site']
, actions = ['view', 'create', 'edit', 'delete'];

// User class
var User = function(role) {
return {
getRoleId: function() {
return role;
}
};
}

// Resource class
var Resource = function() {
return {
resource_id: 'resource'
};
}

// tests
describe('acl', function() {
beforeEach(function() {
Expand Down Expand Up @@ -107,7 +123,6 @@ require('should');
});

this.acl.query(child, resource, action, function(err, allowed) {
console.log(err, allowed);
allowed.should.equal(true); // child can access resource
done();
});
Expand Down Expand Up @@ -339,13 +354,18 @@ require('should');
describe('custom permissions', function() {
beforeEach(function() {
this.acl = new Acl();
this.acl.allow('user', 'page', 'view');
this.acl.allow('member', 'page', 'view');
});

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)) {
Copy link

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!

allowableRole = true;
}

if (allowableRole && resource == 'page' && action == 'view') {
it('should allow role to resource', function(done) {
this.acl.query(role, resource, action, function(err, allowed) {
assert(allowed == true);
Expand Down Expand Up @@ -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)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👾

allowableRole = true;
}

if (allowableRole) {
it('should allow all resources to globally allowed role', function() {
this.acl.query(role, resource, action, function(err, allowed){
assert(allowed == true);
Expand Down Expand Up @@ -420,6 +445,34 @@ require('should');
})(resources[j]);
})(roles[i]);
});

describe('object oriented interface', function() {
beforeEach(function() {
this.acl = new Acl();
});

it('should support objects with getRoleId()', function(done) {
var user = new User('guest');
var resource = new Resource();

this.acl.allow('guest', 'resource', 'action');
this.acl.query(user, resource, 'action', function(err, allowed) {
allowed.should.equal(true);
done();
})
});

it('should support objects with multiple roles', function(done) {
var user = new User(['blog-admin', 'product-admin']);
var resource = new Resource();

this.acl.allow('product-admin', 'resource', 'action');
this.acl.query(user, resource, 'action', function(err, allowed) {
allowed.should.equal(true);
done();
});
});
});
});

describe('with global allow all', function() {
Expand Down Expand Up @@ -450,14 +503,19 @@ 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 == 'guest') {
it('should deny all resources to globally denined role', function() {
var deniableRole = false;
if( role === 'guest' || (Array.isArray(role) && role.indexOf('guest') !== -1)) {
Copy link

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.

deniableRole = true;
}

if (deniableRole) {
it(`should deny ${action} of ${resource} to guest`, function() {
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() {
Copy link
Owner

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.

this.acl.query(role, resource, action, function(err, allowed) {
allowed.should.equal(true);
});
Expand All @@ -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() {
Copy link
Owner

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.

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() {
Copy link
Owner

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.

this.acl.query(role, resource, action, function(err, allowed) {
allowed.should.equal(true);
});
Expand Down