From 6200c850704bc6a8025483b27395087e789c2c80 Mon Sep 17 00:00:00 2001 From: Francisco de Gouveia Date: Sat, 4 Mar 2017 22:06:41 +0100 Subject: [PATCH 1/7] Refactored get method from DataRetrievalRouter to use promise instead of callback --- lib/DataRetrievalRouter.js | 99 +++++++++++++-------------- test/dataretriever.js | 133 +++++++++++++++++++------------------ 2 files changed, 118 insertions(+), 114 deletions(-) diff --git a/lib/DataRetrievalRouter.js b/lib/DataRetrievalRouter.js index 89578e8..23d0d64 100644 --- a/lib/DataRetrievalRouter.js +++ b/lib/DataRetrievalRouter.js @@ -44,9 +44,9 @@ internals.DataRetrievalRouter.prototype.createChild = function (context) { /** * Register a data retriever. * - * * handles - A string or array of strings specifying what this component retrieves (source of data, e.g. 'credentials') - * * retriever - A function which returns data, according to a key. Function signature is (source:string, key:string, context:object) => String - * * options - (optional) A JSON with the following options: + * @param handles - A string or array of strings specifying what this component retrieves (source of data, e.g. 'credentials') + * @param retriever - A function which returns data, according to a key. Function signature is (source:string, key:string, context:object) => String + * @param options - (optional) A JSON with the following options: * * override - When true, overrides existent handler if exists. When false, throws an error when a repeated handler is used. (default: false) **/ internals.DataRetrievalRouter.prototype.register = function (handles, retriever, options) { @@ -91,71 +91,72 @@ internals.DataRetrievalRouter.prototype._register = function (handles, retriever /** * Obtain data from a retriever. * - * * key - Key value from the source (e.g. 'credentials:username') - * * context - (optional) Context object. Contains the request object. - * * callback - Function with the signature (err, result) + * @param key - Key value from the source (e.g. 'credentials:username') + * @param context - (optional) Context object. Contains the request object. + * @returns Promise **/ -internals.DataRetrievalRouter.prototype.get = function (key, context, callback) { +internals.DataRetrievalRouter.prototype.get = function (key, context) { - if (!callback) { - if (context && context instanceof Function) { + return new Promise((resolve, reject) => { - callback = context; - context = null; - } else { + Joi.assert(key, schemas.DataRetrievalRouter_get_key); + let source; + let subkey; - throw new Error('Callback not given'); + if (key.indexOf(':') === -1) { + source = 'credentials'; // keep it backwards compatible + subkey = key; + } + else { + const split_key = key.split(':'); + source = split_key[0]; + subkey = split_key[1]; } - } - - Joi.assert(key, schemas.DataRetrievalRouter_get_key); - let source; - let subkey; - if (key.indexOf(':') === -1) { - source = 'credentials'; // keep it backwards compatible - subkey = key; - } - else { - const split_key = key.split(':'); - source = split_key[0]; - subkey = split_key[1]; - } + Joi.assert(subkey, schemas.DataRetrievalRouter_get_key); + Joi.assert(source, schemas.DataRetrievalRouter_get_source); - Joi.assert(subkey, schemas.DataRetrievalRouter_get_key); - Joi.assert(source, schemas.DataRetrievalRouter_get_source); + const fn = this.retrievers[source]; - const fn = this.retrievers[source]; + if (!fn) { - if (!fn) { + if (!this.parent) { - if (!this.parent) { + return resolve(null); + } - return callback(null, null); + return this.parent.get(key, context || this.context) + .then((result) => resolve(result)) + .catch((err) => reject(err)); } - return this.parent.get(key, context || this.context, callback); - } + if (fn.length > 3) { - if (fn.length > 3) { + // has callback + try { + return fn(source, subkey, context || this.context, (err, value) => { - // has callback - try { - return fn(source, subkey, context || this.context, callback); - } catch(e) { - return callback(e); + if (err) { + return reject(err); + } + + resolve(value); + }); + } catch(e) { + return reject(e); + } } - } - let value; + let value; - try { - value = fn(source, subkey, context || this.context); - } catch(e) { - return callback(e); - } + try { + value = fn(source, subkey, context || this.context); + } catch(e) { + return reject(e); + } - callback(null, value); + resolve(value); + }); }; schemas.DataRetrievalRouter_get_source = Joi.string().min(1); schemas.DataRetrievalRouter_get_key = Joi.string().min(1); diff --git a/test/dataretriever.js b/test/dataretriever.js index f15fd53..7e114d0 100644 --- a/test/dataretriever.js +++ b/test/dataretriever.js @@ -17,7 +17,7 @@ experiment('RBAC internal modular information retrieval', () => { const dataRetriever = new DataRetrievalRouter(); - test('should register a valid retriever', (done) => { + test('should register a valid retriever', () => { const retriever = (source, key, context) => { @@ -26,15 +26,14 @@ experiment('RBAC internal modular information retrieval', () => { dataRetriever.register('test', retriever); - dataRetriever.get('test:x', (err, result) => { + return dataRetriever.get('test:x') + .then((result) => { - expect(err).to.not.exist(); expect(result).to.equal('key-x'); - done(); }); }); - test('should override a valid retriever (single handler)', (done) => { + test('should override a valid retriever (single handler)', () => { const retriever1 = (source, key, context) => { @@ -49,14 +48,11 @@ experiment('RBAC internal modular information retrieval', () => { dataRetriever.register('test-override', retriever1); dataRetriever.register('test-override', retriever2, { override: true }); - dataRetriever.get('test-override:test', (err, result) => { + return dataRetriever.get('test-override:test') + .then((result) => { - expect(err).to.not.exist(); expect(result).to.equal('test-2'); - - done(); }); - }); test('should not override a valid retriever (single handler)', (done) => { @@ -93,31 +89,18 @@ experiment('RBAC internal modular information retrieval', () => { dataRetriever.register(['test-override-multiple-1', 'test-override-multiple-2', 'test-override-multiple-3'], retriever1); dataRetriever.register(['test-override-multiple-2', 'test-override-multiple-4'], retriever2, { override: true }); // test-override-multiple-2 collides - dataRetriever.get('test-override-multiple-1:test', (err, result) => { - - expect(err).to.not.exist(); - expect(result).to.equal('test-1'); - - dataRetriever.get('test-override-multiple-2:test', (err, result) => { - - expect(err).to.not.exist(); - expect(result).to.equal('test-2'); - - - dataRetriever.get('test-override-multiple-3:test', (err, result) => { - - expect(err).to.not.exist(); - expect(result).to.equal('test-1'); - - dataRetriever.get('test-override-multiple-4:test', (err, result) => { - - expect(err).to.not.exist(); - expect(result).to.equal('test-2'); - - done(); - }); - }); - }); + return Promise.all([ + dataRetriever.get('test-override-multiple-1:test'), + dataRetriever.get('test-override-multiple-2:test'), + dataRetriever.get('test-override-multiple-3:test'), + dataRetriever.get('test-override-multiple-4:test') + ]) + .then(([result1, result2, result3, result4]) => { + + expect(result1).to.equal('test-1'); + expect(result2).to.equal('test-2'); + expect(result3).to.equal('test-1'); + expect(result4).to.equal('test-2'); }); }); @@ -139,7 +122,7 @@ experiment('RBAC internal modular information retrieval', () => { done(); }); - test('should register a valid asynchronous retriever', (done) => { + test('should register a valid asynchronous retriever', () => { const retriever = (source, key, context, callback) => { @@ -148,15 +131,14 @@ experiment('RBAC internal modular information retrieval', () => { dataRetriever.register('async-test', retriever); - dataRetriever.get('async-test:x', (err, result) => { + return dataRetriever.get('async-test:x') + .then((result) => { - expect(err).to.not.exist(); expect(result).to.equal('key-x'); - done(); }); }); - test('should use parent asynchronous retriever', (done) => { + test('should use parent asynchronous retriever', () => { const retriever = (source, key, context, callback) => { @@ -167,15 +149,14 @@ experiment('RBAC internal modular information retrieval', () => { const childDataRetriever = dataRetriever.createChild(); - childDataRetriever.get('async-parent-test-1:x', (err, result) => { + return childDataRetriever.get('async-parent-test-1:x') + .then((result) => { - expect(err).to.not.exist(); expect(result).to.equal('key-x'); - done(); }); }); - test('should use parent synchronous retriever', (done) => { + test('should use parent synchronous retriever', () => { const retriever = (source, key, context) => { @@ -186,39 +167,53 @@ experiment('RBAC internal modular information retrieval', () => { const childDataRetriever = dataRetriever.createChild(); - childDataRetriever.get('sync-parent-test-1:x', (err, result) => { + return childDataRetriever.get('sync-parent-test-1:x') + .then((result) => { - expect(err).to.not.exist(); expect(result).to.equal('key-x'); - done(); }); }); - test('should return null if inexistent prefix on child and parent', (done) => { + test('should return null if inexistent prefix on child and parent', () => { const childDataRetriever = dataRetriever.createChild(); - childDataRetriever.get('this-does-not-exist-1:x', (err, result) => { + return childDataRetriever.get('this-does-not-exist-1:x') + .then((result) => { - expect(err).to.not.exist(); expect(result).to.not.exist(); - done(); }); }); - test('should not allow using get with context and without callback', (done) => { + test('should not allow using get with context', () => { + + return dataRetriever.get('get-with-context', {}) + .then((result) => { - expect(dataRetriever.get.bind(null, 'get-with-context-without-callback:x', {})).to.throw(Error); - done() + fail("Should have rejected"); + }) + .catch((err) => { + + expect(err).to.exist(); + expect(err.message).to.exist().and.not.equal("Should have rejected"); + }); }); - test('should not allow using get without context and without callback', (done) => { + test('should not allow using get without context', () => { - expect(dataRetriever.get.bind(null, 'get-with-context-without-callback:x', {})).to.throw(Error); - done() + return dataRetriever.get('get-with-context') + .then((result) => { + + fail("Should have rejected"); + }) + .catch((err) => { + + expect(err).to.exist(); + expect(err.message).to.exist().and.not.equal("Should have rejected"); + }); }); - test('should return err in callback when an error is thrown (sync)', (done) => { + test('should return err in callback when an error is thrown (sync)', () => { const retriever = (source, key, context) => { @@ -227,15 +222,19 @@ experiment('RBAC internal modular information retrieval', () => { dataRetriever.register('sync-test-err-1', retriever); - dataRetriever.get('sync-test-err-1:x', (err, result) => { + return dataRetriever.get('sync-test-err-1:x') + .then((result) => { - expect(err).to.exist(); + fail("Should have rejected"); + }) + .catch((err) => { - done(); + expect(err).to.exist(); + expect(err.message).to.exist().and.not.equal("Should have rejected"); }); }); - test('should return err in callback when an error is thrown (async)', (done) => { + test('should return err in callback when an error is thrown (async)', () => { const retriever = (source, key, context, callback) => { @@ -244,11 +243,15 @@ experiment('RBAC internal modular information retrieval', () => { dataRetriever.register('async-test-err-1', retriever); - dataRetriever.get('async-test-err-1:x', (err, result) => { + return dataRetriever.get('async-test-err-1:x') + .then((result) => { - expect(err).to.exist(); + fail("Should have rejected"); + }) + .catch((err) => { - done(); + expect(err).to.exist(); + expect(err.message).to.exist().and.not.equal("Should have rejected"); }); }); }); From 4327c633cf58f76d6871aa7d3678886708ea2024 Mon Sep 17 00:00:00 2001 From: Francisco de Gouveia Date: Sat, 4 Mar 2017 22:07:43 +0100 Subject: [PATCH 2/7] Implemented retrieval and matching of two target reference keys --- lib/index.js | 53 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/lib/index.js b/lib/index.js index 6c02ddf..4413bfa 100644 --- a/lib/index.js +++ b/lib/index.js @@ -175,38 +175,51 @@ internals.evaluateTargetElement = (dataRetriever, element) => { return (callback) => { - const tasks = []; + const promises = Object.keys(element).map((key) => internals.evaluateTargetElementKey(dataRetriever, element, key)); - for (const key in element) { - - tasks.push(internals.evaluateTargetElementKey(dataRetriever, element, key)); - } - - Async.parallel(tasks, (err, result) => { - - if (err) { - return callback(err); - } + Promise.all(promises) + .then((results) => { // Should all apply (AND) - const nonApplicable = result.filter((value) => !value); + const nonApplicable = results.filter((value) => !value); callback(null, nonApplicable.length === 0); - }); + }) + .catch((err) => callback(err)) }; }; -internals.evaluateTargetElementKey = (dataRetriever, element, key) => { +/** + * If target is defined as: + * { field: "credentials:user" } + * then this definition should be replaced by + * a value from dataRetriever for matching. + * + * @param dataRetriever + * @param definedValue + * @returns Promise + **/ +internals.getTargetValue = (dataRetriever, definedValue) => { - return (callback) => { + if(typeof definedValue === "object") { + if (definedValue.field) { + return dataRetriever.get(definedValue.field); + } + } - dataRetriever.get(key, null, (err, value) => { + return Promise.resolve(definedValue); +}; - const result = internals._targetApplies(element[key], value); +internals.evaluateTargetElementKey = (dataRetriever, element, key) => { - callback(null, !!result); - }); - }; + return Promise.all([ + internals.getTargetValue(dataRetriever, element[key]), + dataRetriever.get(key) + ]) + .then(([targetValue, value]) => { + + return internals._targetApplies(targetValue, value); + }); }; /** From 79218134c4f35cd0db940c31c63cc53c799b7bb4 Mon Sep 17 00:00:00 2001 From: Francisco de Gouveia Date: Sat, 4 Mar 2017 22:08:13 +0100 Subject: [PATCH 3/7] Created tests for field matching --- test/target.js | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/test/target.js b/test/target.js index b01d9a4..2f3cf71 100644 --- a/test/target.js +++ b/test/target.js @@ -165,6 +165,74 @@ experiment('Target unit tests (AND with RegExp)', () => { }); + +experiment('Target unit tests (AND with field agains field matching)', () => { + + const target = { 'credentials:group': 'writer', 'credentials:some-field': { field: 'external:some-field-name' } }; + + // Register mocked data retrievers + const dataRetriever = new DataRetrievalRouter(); + dataRetriever.register('credentials', (source, key, context) => context[key], { override: true }); + + const externalContext = { 'some-field-name': 'some-field-value' }; + dataRetriever.register('external', (source, key, context) => externalContext[key], { override: true }); + + test('should apply (full match)', (done) => { + + const information = { + username: 'user00001', + group: ['writer'], + 'some-field': 'some-field-value' + }; + + Rbac.evaluateTarget(target, dataRetriever.createChild(information), (err, applies) => { + + expect(err).to.not.exist(); + + expect(applies).to.exist().and.to.equal(true); + + done(); + }); + }); + + test('should not apply (partial match)', (done) => { + + const information = { + username: 'user00002', + group: ['writer'], + 'some-field': 'bad-field-value' + }; + + Rbac.evaluateTarget(target, dataRetriever.createChild(information), (err, applies) => { + + expect(err).to.not.exist(); + + expect(applies).to.exist().and.to.equal(false); + + done(); + }); + }); + + test('should not apply (no match)', (done) => { + + const information = { + username: 'user00003', + group: ['reader'], + 'some-field': 'bad-field-value' + }; + + Rbac.evaluateTarget(target, dataRetriever.createChild(information), (err, applies) => { + + expect(err).to.not.exist(); + + expect(applies).to.exist().and.to.equal(false); + + done(); + }); + }); + +}); + experiment('Target unit tests (OR)', () => { const target = [ From a134f66d849cd226620248522c96d033a1d5e4e7 Mon Sep 17 00:00:00 2001 From: Francisco de Gouveia Date: Sat, 4 Mar 2017 22:23:49 +0100 Subject: [PATCH 4/7] Updated documentation with example of field matching --- README.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 9584945..b2e2e27 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,7 @@ const dataRetrieverRouter = new DataRetrievalRouter(); dataRetrieverRouter.register('credentials', (source, key, context) => { // Obtain your value (e.g. from the context) - const value = context[key]; + const value = context.user[key]; return value; }); @@ -77,21 +77,32 @@ Evaluate your policies against a certain context const context = { user: { username: 'francisco', - group: ['admin', 'developer'], - validated: true + group: ['articles:admin', 'articles:developer'], + validated: true, + exampleField1: "test-value" }, connection: { remoteip: '192.168.0.123', remoteport: 90, localip: '192.168.0.2' - localport: 80 + localport: 80, + exampleField2: "test-value" } }; dataRetrieverRouter.setContext(context); const policy = { - target: [{ 'credentials:username': 'francisco' }, { 'credentials:group': /^articles\:.*$/ }], // if username is 'francisco' OR group matches 'articles:*' (using native javascript RegExp) + target: [ + // if username matches 'francisco' OR (exampleField1 matches exampleField2 AND user group matches 'articles:*') + { 'credentials:username': 'francisco' }, + // OR + { + 'credentials:exampleField1': { field: 'connection:exampleField2' } + // AND + 'credentials:group': /^articles\:.*$/ //(using native javascript RegExp) + } + ], apply: 'deny-overrides', // permit, unless one denies rules: [ { From d4795e1abc7824060cb2e8eb092a8f014de1ee09 Mon Sep 17 00:00:00 2001 From: Francisco de Gouveia Date: Sat, 4 Mar 2017 22:28:09 +0100 Subject: [PATCH 5/7] Added node 6 to tests --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 2953708..9181028 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,6 +2,7 @@ language: node_js sudo: false node_js: + - "6" - "5" - "4" From c2792b83a02b5430ecf6113168af03ec35060123 Mon Sep 17 00:00:00 2001 From: Francisco de Gouveia Date: Sat, 4 Mar 2017 22:29:47 +0100 Subject: [PATCH 6/7] Support node 4.x and 5.x --- test/dataretriever.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/dataretriever.js b/test/dataretriever.js index 7e114d0..296e3a2 100644 --- a/test/dataretriever.js +++ b/test/dataretriever.js @@ -95,12 +95,12 @@ experiment('RBAC internal modular information retrieval', () => { dataRetriever.get('test-override-multiple-3:test'), dataRetriever.get('test-override-multiple-4:test') ]) - .then(([result1, result2, result3, result4]) => { + .then((results) => { - expect(result1).to.equal('test-1'); - expect(result2).to.equal('test-2'); - expect(result3).to.equal('test-1'); - expect(result4).to.equal('test-2'); + expect(results[0]).to.equal('test-1'); + expect(results[1]).to.equal('test-2'); + expect(results[2]).to.equal('test-1'); + expect(results[3]).to.equal('test-2'); }); }); From 840a1145d049c46b7399efe41989e26ba0cfcdcc Mon Sep 17 00:00:00 2001 From: Francisco de Gouveia Date: Sat, 4 Mar 2017 22:32:02 +0100 Subject: [PATCH 7/7] Support node 4.x and 5.x --- lib/index.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/index.js b/lib/index.js index 4413bfa..7b6b954 100644 --- a/lib/index.js +++ b/lib/index.js @@ -216,8 +216,10 @@ internals.evaluateTargetElementKey = (dataRetriever, element, key) => { internals.getTargetValue(dataRetriever, element[key]), dataRetriever.get(key) ]) - .then(([targetValue, value]) => { + .then((results) => { + const targetValue = results[0]; + const value = results[1]; return internals._targetApplies(targetValue, value); }); };