From fea1cee1c49579419704be5615ae169874aad647 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Mon, 2 Jun 2014 13:41:14 -0700 Subject: [PATCH] !fixup only set ctx.accessType when sharedMethod is available --- lib/models/access-context.js | 17 +++++++------- lib/models/acl.js | 43 ++++++++++++++++-------------------- lib/models/model.js | 5 +++-- lib/models/role.js | 6 ++--- test/access-context.test.js | 0 test/acl.test.js | 6 ++++- 6 files changed, 38 insertions(+), 39 deletions(-) delete mode 100644 test/access-context.test.js diff --git a/lib/models/access-context.js b/lib/models/access-context.js index 83883e8f..681c366c 100644 --- a/lib/models/access-context.js +++ b/lib/models/access-context.js @@ -44,7 +44,10 @@ function AccessContext(context) { this.methodNames = []; } - this.accessType = this.model._getAccessTypeForMethod(this.method); + if(this.sharedMethod) { + this.accessType = this.model._getAccessTypeForMethod(this.sharedMethod); + } + this.accessType = context.accessType || AccessContext.ALL; this.accessToken = context.accessToken || AccessToken.ANONYMOUS; @@ -240,8 +243,9 @@ function AccessRequest(model, property, accessType, permission, methodNames) { } /** - * Is the request a wildcard - * @returns {boolean} + * Does the request contain any wildcards? + * + * @returns {Boolean} */ AccessRequest.prototype.isWildcard = function () { return this.model === AccessContext.ALL || @@ -259,12 +263,7 @@ AccessRequest.prototype.exactlyMatches = function(acl) { var matchesModel = acl.model === this.model; var matchesProperty = acl.property === this.property; var matchesMethodName = this.methodNames.indexOf(acl.property) !== -1; - var matchesAccessType = (acl.accessType || '*') === this.accessType; - - debug('matchesModel %s === %s %j', acl.model, this.model, acl.model === this.model); - debug('matchesProperty %s === %s %j', acl.property, this.property, acl.property === this.property); - debug('matchesMethodName %s in %j %j', acl.property, this.methodNames, this.methodNames.indexOf(acl.property) !== -1); - debug('matchesAccessType %s === %s %j', acl.accessType, this.accessType, acl.accessType === this.accessType); + var matchesAccessType = acl.accessType === this.accessType; if(matchesModel && matchesAccessType) { return matchesProperty || matchesMethodName; diff --git a/lib/models/acl.js b/lib/models/acl.js index f5b25340..b29b3e63 100644 --- a/lib/models/acl.js +++ b/lib/models/acl.js @@ -125,12 +125,9 @@ ACL.getMatchingScore = function getMatchingScore(rule, req) { score = score * 4; var val1 = rule[props[i]] || ACL.ALL; var val2 = req[props[i]] || ACL.ALL; + var isMatchingMethodName = props[i] === 'property' && req.methodNames.indexOf(val1) !== -1; - if(props[i] === 'property' && req.methodNames.indexOf(val1) !== -1) { - score += 3; - } - - if (val1 === val2) { + if (val1 === val2 || isMatchingMethodName) { // Exact match score += 3; } else if (val1 === ACL.ALL) { @@ -192,6 +189,16 @@ ACL.getMatchingScore = function getMatchingScore(rule, req) { return score; }; +/** + * Get matching score for the given `AccessRequest`. + * @param {AccessRequest} req The request + * @returns {Number} score + */ + +ACL.prototype.score = function(req) { + return this.constructor.getMatchingScore(this, req); +} + /*! * Resolve permission from the ACLs * @param {Object[]) acls The list of ACLs @@ -208,47 +215,35 @@ ACL.resolvePermission = function resolvePermission(acls, req) { }); var permission = ACL.DEFAULT; var score = 0; - var matchingACL; for (var i = 0; i < acls.length; i++) { score = ACL.getMatchingScore(acls[i], req); if (score < 0) { + // the highest scored ACL did not match break; } if (!req.isWildcard()) { // We should stop from the first match for non-wildcard - debug('Found first match (non-wildcard):'); - matchingACL = acls[i]; - permission = matchingACL.permission; + permission = acls[i].permission; break; } else { if(req.exactlyMatches(acls[i])) { - debug('Exact ACL match:'); - acls[i].debug(); - // We should stop at the exact match - matchingACL = acls[i]; - permission = matchingACL.permission; + permission = acls[i].permission; break; } // For wildcard match, find the strongest permission if(AccessContext.permissionOrder[acls[i].permission] > AccessContext.permissionOrder[permission]) { - matchingACL = acls[i]; - permission = matchingACL.permission; + permission = acls[i].permission; } } } if(debug.enabled) { - if(matchingACL) { - debug('Matching ACL:'); - matchingACL.debug(); - } else { - debug('No matching ACL found!') - } debug('The following ACLs were searched: '); acls.forEach(function(acl) { acl.debug(); + debug('with score:', acl.score(req)); }); } @@ -274,7 +269,7 @@ ACL.getStaticACLs = function getStaticACLs(model, property) { property: acl.property || ACL.ALL, principalType: acl.principalType, principalId: acl.principalId, // TODO: Should it be a name? - accessType: acl.accessType, + accessType: acl.accessType || ACL.ALL, permission: acl.permission })); }); @@ -393,7 +388,7 @@ ACL.checkAccessForContext = function (context, callback) { var propertyQuery = (property === ACL.ALL) ? undefined : {inq: methodNames.concat([ACL.ALL])}; var accessTypeQuery = (accessType === ACL.ALL) ? undefined : {inq: [accessType, ACL.ALL]}; - var req = new AccessRequest(modelName, property, accessType, methodNames); + var req = new AccessRequest(modelName, property, accessType, ACL.DEFAULT, methodNames); var effectiveACLs = []; var staticACLs = this.getStaticACLs(model.modelName, property); diff --git a/lib/models/model.js b/lib/models/model.js index e80cb187..fdeea317 100644 --- a/lib/models/model.js +++ b/lib/models/model.js @@ -155,7 +155,8 @@ Model.checkAccess = function(token, modelId, sharedMethod, callback) { property: sharedMethod.name, method: sharedMethod.name, sharedMethod: sharedMethod, - modelId: modelId + modelId: modelId, + accessType: this._getAccessTypeForMethod(sharedMethod) }, function(err, accessRequest) { if(err) return callback(err); callback(null, accessRequest.isAllowed()); @@ -172,7 +173,7 @@ Model.checkAccess = function(token, modelId, sharedMethod, callback) { Model._getAccessTypeForMethod = function(method) { if(typeof method === 'string') { method = {name: method}; - }40 + } assert( typeof method === 'object', 'method is a required argument and must be a RemoteMethod object' diff --git a/lib/models/role.js b/lib/models/role.js index 94212b75..072b7a76 100644 --- a/lib/models/role.js +++ b/lib/models/role.js @@ -319,13 +319,13 @@ Role.registerResolver(Role.EVERYONE, function (role, context, callback) { * @param {Boolean} isInRole */ Role.isInRole = function (role, context, callback) { - debug('isInRole(): %s', role); - context.debug(); - if (!(context instanceof AccessContext)) { context = new AccessContext(context); } + debug('isInRole(): %s', role); + context.debug(); + var resolver = Role.resolvers[role]; if (resolver) { debug('Custom resolver found for role %s', role); diff --git a/test/access-context.test.js b/test/access-context.test.js deleted file mode 100644 index e69de29b..00000000 diff --git a/test/acl.test.js b/test/acl.test.js index 96155683..c4e7bb8f 100644 --- a/test/acl.test.js +++ b/test/acl.test.js @@ -101,11 +101,15 @@ describe('security ACLs', function () { property: 'find', accessType: 'WRITE' }; + + acls = acls.map(function(a) { return new ACL(a)}); + var perm = ACL.resolvePermission(acls, req); assert.deepEqual(perm, { model: 'account', property: 'find', accessType: 'WRITE', - permission: 'ALLOW' }); + permission: 'ALLOW', + methodNames: []}); }); it("should allow access to models for the given principal by wildcard", function () {