From bf1c4b254f63768b4cbdc8f3837c29215a7fd8a6 Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Fri, 19 Aug 2016 15:23:34 -0400 Subject: [PATCH 1/9] Add docstring to findRule() --- lib/acl-checker.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/acl-checker.js b/lib/acl-checker.js index e386dd760..333a3e648 100644 --- a/lib/acl-checker.js +++ b/lib/acl-checker.js @@ -103,6 +103,19 @@ class ACLChecker { callback) } + /** + * @method findRule + * @param graph {Graph} Parsed RDF graph of current .acl resource + * @param user {String} WebID URI of the user accessing the resource + * @param mode {String} Access mode, e.g. 'Read', 'Write', etc. + * @param resource {String} URI of the resource being accessed + * @param accessType {String} One of `accessTo`, or `default` + * @param acl {String} URI of this current .acl resource + * @param callback {Function} + * @param options {Object} Options hashmap + * @param [options.origin] Request's `Origin:` header + * @param [options.host] Request's host URI (with protocol) + */ findRule (graph, user, mode, resource, accessType, acl, callback, options) { const debug = this.debug if (!graph || graph.length === 0) { From 24fe64f16733cf1f2ba63dfa721ae7cbece7a056 Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Thu, 15 Sep 2016 12:32:35 -0400 Subject: [PATCH 2/9] Refactor acl checker to use solid-permissions lib --- lib/acl-checker.js | 130 +++++++++------------------------------------ lib/ldp.js | 1 - package.json | 1 + 3 files changed, 25 insertions(+), 107 deletions(-) diff --git a/lib/acl-checker.js b/lib/acl-checker.js index 333a3e648..f2060ff6b 100644 --- a/lib/acl-checker.js +++ b/lib/acl-checker.js @@ -2,6 +2,8 @@ const async = require('async') const path = require('path') +const PermissionSet = require('solid-permissions').PermissionSet +const rdf = require('rdflib') const url = require('url') const DEFAULT_ACL_SUFFIX = '.acl' @@ -81,28 +83,6 @@ class ACLChecker { }) } - findAgentClass (graph, user, mode, resource, acl, callback) { - const debug = this.debug - // Agent class statement - var agentClassStatements = graph.match(acl, - 'http://www.w3.org/ns/auth/acl#agentClass') - if (agentClassStatements.length === 0) { - return callback(false) - } - async.some( - agentClassStatements, - function (agentClassTriple, found) { - // Check for FOAF groups - debug('Found agentClass policy') - if (agentClassTriple.object.uri === 'http://xmlns.com/foaf/0.1/Agent') { - debug(mode + ' allowed access as FOAF agent') - return found(true) - } - return found(false) - }, - callback) - } - /** * @method findRule * @param graph {Graph} Parsed RDF graph of current .acl resource @@ -120,101 +100,39 @@ class ACLChecker { const debug = this.debug if (!graph || graph.length === 0) { debug('ACL ' + acl + ' is empty') - return callback(new Error('No policy found')) + return callback(new Error('No policy found - empty ACL')) } - debug('Found policies in ' + acl) - // Check for mode - var statements = this.getMode(graph, mode) - if (mode === 'Append') { - statements = statements.concat(this.getMode(graph, 'Write')) + let isContainer = accessType.startsWith('default') + let aclOptions = { + aclSuffix: this.suffix, + graph: graph, + host: options.host, + origin: options.origin, + rdf: rdf, + strictOrigin: this.strictOrigin } - var self = this - async.some( - statements, - function (statement, done) { - var statementSubject = statement.subject.uri - // Check for origin - var matchOrigin = self.matchOrigin(graph, statementSubject, - options.origin, options.host) - if (!matchOrigin) { - debug('The request does not match the origin') - return done(false) - } - // Check for accessTo/defaultForNew - if (!self.isAcl(resource) || accessType === 'defaultForNew') { - debug('Checking for accessType:' + accessType) - var accesses = self.matchAccessType(graph, statementSubject, accessType, - resource) - if (!accesses) { - debug('Cannot find accessType ' + accessType) - return done(false) - } - } - // Check for Agent - var agentStatements = [] - if (user) { - agentStatements = graph.match( - statementSubject, - 'http://www.w3.org/ns/auth/acl#agent', - user) - } - if (agentStatements.length) { - debug(mode + ' access allowed (as agent) for: ' + user) - return done(true) - } - debug('Inspect agentClass') - // Check for AgentClass - return self.findAgentClass(graph, user, mode, resource, statementSubject, - done) - }, - function (found) { - if (!found) { + let acls = new PermissionSet(resource, acl, isContainer, aclOptions) + acls.checkAccess(resource, user, mode) + .then(hasAccess => { + if (hasAccess) { + debug(`${mode} access permitted to ${user}`) + return callback() + } else { + debug(`${mode} access not permitted to ${user}`) return callback(new Error('Acl found but policy not found')) } - return callback(null) }) - } - - getMode (graph, mode) { - return graph.match( - null, - 'http://www.w3.org/ns/auth/acl#mode', - 'http://www.w3.org/ns/auth/acl#' + mode - ) + .catch(err => { + debug(`${mode} access denied to ${user}`) + debug(err) + return callback(err) + }) } isAcl (resource) { return resource.endsWith(this.suffix) } - matchAccessType (graph, rule, accessType, uri) { - var matches = graph.match( - rule, - 'http://www.w3.org/ns/auth/acl#' + accessType - ) - return matches.some(function (match) { - return uri.startsWith(match.object.uri) - }) - } - - matchOrigin (graph, rule, origin, host) { - // if there is no origin, then the host is the origin - if (this.strictOrigin && !origin) { - return true - } - var origins = graph.match( - rule, - 'http://www.w3.org/ns/auth/acl#origin' - ) - if (origins.length) { - return origins.some(function (triple) { - return triple.object.uri === (origin || host) - }) - } - // return true if origin is not enforced - return !this.strictOrigin - } - static possibleACLs (uri, suffix) { var first = uri.endsWith(suffix) ? uri : uri + suffix var urls = [first] diff --git a/lib/ldp.js b/lib/ldp.js index 4fe0270c6..74031917e 100644 --- a/lib/ldp.js +++ b/lib/ldp.js @@ -260,7 +260,6 @@ LDP.prototype.graph = function (host, reqPath, baseUri, contentType, callback) { var root = ldp.idp ? ldp.root + host + '/' : ldp.root var filename = utils.uriToFilename(reqPath, root) - console.log(filename) async.waterfall([ // Read file diff --git a/package.json b/package.json index 3f87b8258..e379234e9 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,7 @@ "rimraf": "^2.5.0", "run-waterfall": "^1.1.3", "solid-namespace": "^0.1.0", + "solid-permissions": "git://github.com/solid/solid-permissions.git#dz_allows_helpers", "solid-ws": "^0.2.2", "string": "^3.3.0", "uid-safe": "^2.1.1", From 80e0675459a69a2fd25f2a6eb804a5dee05a7e84 Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Thu, 15 Sep 2016 13:54:41 -0400 Subject: [PATCH 3/9] Pass in server's acl uri deriving functions --- lib/acl-checker.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/acl-checker.js b/lib/acl-checker.js index f2060ff6b..459817099 100644 --- a/lib/acl-checker.js +++ b/lib/acl-checker.js @@ -22,7 +22,7 @@ class ACLChecker { var accessType = 'accessTo' var possibleACLs = ACLChecker.possibleACLs(resource, this.suffix) // If this is an ACL, Control mode must be present for any operations - if (this.isAcl(resource)) { + if (this.isAcl(resource, this.suffix)) { mode = 'Control' } var self = this @@ -109,7 +109,9 @@ class ACLChecker { host: options.host, origin: options.origin, rdf: rdf, - strictOrigin: this.strictOrigin + strictOrigin: this.strictOrigin, + isAcl: (uri) => { return this.isAcl(uri) }, + aclUrlFor: (uri) => { return this.aclUrlFor(uri) } } let acls = new PermissionSet(resource, acl, isContainer, aclOptions) acls.checkAccess(resource, user, mode) @@ -129,6 +131,14 @@ class ACLChecker { }) } + aclUrlFor (uri) { + if (this.isAcl(uri)) { + return uri + } else { + return uri + this.suffix + } + } + isAcl (resource) { return resource.endsWith(this.suffix) } From 3b0bc9f39d6751299c4db97863422dcbaa925942 Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Mon, 19 Sep 2016 12:49:37 -0400 Subject: [PATCH 4/9] Rename findRule() to checkAccess(), add comments --- lib/acl-checker.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/acl-checker.js b/lib/acl-checker.js index 459817099..c38927cd7 100644 --- a/lib/acl-checker.js +++ b/lib/acl-checker.js @@ -22,7 +22,7 @@ class ACLChecker { var accessType = 'accessTo' var possibleACLs = ACLChecker.possibleACLs(resource, this.suffix) // If this is an ACL, Control mode must be present for any operations - if (this.isAcl(resource, this.suffix)) { + if (this.isAcl(resource)) { mode = 'Control' } var self = this @@ -46,7 +46,7 @@ class ACLChecker { accessType = 'defaultForNew' return next() } - self.findRule( + self.checkAccess( graph, // The ACL graph user, // The webId of the user mode, // Read/Write/Append @@ -84,7 +84,10 @@ class ACLChecker { } /** - * @method findRule + * Tests whether a graph (parsed .acl resource) allows a given operation + * for a given user. Calls the provided callback with `null` if the user + * has access, otherwise calls it with an error. + * @method checkAccess * @param graph {Graph} Parsed RDF graph of current .acl resource * @param user {String} WebID URI of the user accessing the resource * @param mode {String} Access mode, e.g. 'Read', 'Write', etc. @@ -96,7 +99,7 @@ class ACLChecker { * @param [options.origin] Request's `Origin:` header * @param [options.host] Request's host URI (with protocol) */ - findRule (graph, user, mode, resource, accessType, acl, callback, options) { + checkAccess (graph, user, mode, resource, accessType, acl, callback, options) { const debug = this.debug if (!graph || graph.length === 0) { debug('ACL ' + acl + ' is empty') From cfb71d16f5d5c70b7adc06f8e31d41663861196d Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Mon, 19 Sep 2016 14:22:14 -0400 Subject: [PATCH 5/9] Bump solid permissions dep --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e379234e9..11180e3f4 100644 --- a/package.json +++ b/package.json @@ -62,7 +62,7 @@ "rimraf": "^2.5.0", "run-waterfall": "^1.1.3", "solid-namespace": "^0.1.0", - "solid-permissions": "git://github.com/solid/solid-permissions.git#dz_allows_helpers", + "solid-permissions": "^0.3.0", "solid-ws": "^0.2.2", "string": "^3.3.0", "uid-safe": "^2.1.1", From 3f4a7bdc7ac28e36e739c7baca3931d926f83d43 Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Fri, 23 Sep 2016 12:07:13 -0400 Subject: [PATCH 6/9] Remove invalid comment --- lib/acl-checker.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/acl-checker.js b/lib/acl-checker.js index c38927cd7..aa1f1cbf1 100644 --- a/lib/acl-checker.js +++ b/lib/acl-checker.js @@ -35,13 +35,6 @@ class ACLChecker { // Let's see if there is a file.. self.fetch(acl, function (err, graph) { if (err || !graph || graph.length === 0) { - // TODO - // If no file is found and we want to Control, - // we should not be able to do that! - // Control is only to Read and Write the current file! - // if (mode === 'Control') { - // return next(new Error('You can\'t Control an unexisting file')) - // } if (err) debug('Error: ' + err) accessType = 'defaultForNew' return next() From a831439e4c95c0bc1bd6f43b1cd1ee79cbbb1b92 Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Wed, 28 Sep 2016 14:50:34 -0400 Subject: [PATCH 7/9] Add proxyquire package --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index 11180e3f4..2015829fb 100644 --- a/package.json +++ b/package.json @@ -76,6 +76,7 @@ "hippie": "^0.5.0", "mocha": "^2.2.5", "nock": "^7.0.2", + "proxyquire": "^1.7.10", "rsvp": "^3.1.0", "run-waterfall": "^1.1.3", "sinon": "^1.17.4", From 8930736a0713a99e20802065dd92f6748997ecde Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Wed, 28 Sep 2016 14:50:59 -0400 Subject: [PATCH 8/9] allow handler - format cleanup --- lib/handlers/allow.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/handlers/allow.js b/lib/handlers/allow.js index 982b84753..d2fcaea06 100644 --- a/lib/handlers/allow.js +++ b/lib/handlers/allow.js @@ -31,10 +31,8 @@ function allow (mode) { var reqPath = res && res.locals && res.locals.path ? res.locals.path : req.path - ldp.exists(req.hostname, reqPath, function (err, stat) { - if (reqPath[reqPath.length - 1] !== '/' && - !err && - stat.isDirectory()) { + ldp.exists(req.hostname, reqPath, (err, stat) => { + if (!reqPath.endsWith('/') && !err && stat.isDirectory()) { reqPath += '/' } var options = { From 5b17170c765921db605aa5774d7eaaa4f408a160 Mon Sep 17 00:00:00 2001 From: Dmitri Zagidulin Date: Wed, 28 Sep 2016 14:51:30 -0400 Subject: [PATCH 9/9] Add acl-checker unit tests --- lib/acl-checker.js | 9 +++++-- test/acl-checker.js | 65 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 test/acl-checker.js diff --git a/lib/acl-checker.js b/lib/acl-checker.js index aa1f1cbf1..0ab18c4f7 100644 --- a/lib/acl-checker.js +++ b/lib/acl-checker.js @@ -92,7 +92,8 @@ class ACLChecker { * @param [options.origin] Request's `Origin:` header * @param [options.host] Request's host URI (with protocol) */ - checkAccess (graph, user, mode, resource, accessType, acl, callback, options) { + checkAccess (graph, user, mode, resource, accessType, acl, callback, + options = {}) { const debug = this.debug if (!graph || graph.length === 0) { debug('ACL ' + acl + ' is empty') @@ -136,7 +137,11 @@ class ACLChecker { } isAcl (resource) { - return resource.endsWith(this.suffix) + if (typeof resource === 'string') { + return resource.endsWith(this.suffix) + } else { + return false + } } static possibleACLs (uri, suffix) { diff --git a/test/acl-checker.js b/test/acl-checker.js new file mode 100644 index 000000000..fd80d20b2 --- /dev/null +++ b/test/acl-checker.js @@ -0,0 +1,65 @@ +'use strict' +const proxyquire = require('proxyquire') +const assert = require('chai').assert +const debug = require('../lib/debug').ACL + +class PermissionSetAlwaysGrant { + checkAccess () { + return Promise.resolve(true) + } +} +class PermissionSetNeverGrant { + checkAccess () { + return Promise.resolve(false) + } +} +class PermissionSetAlwaysError { + checkAccess () { + return Promise.reject(new Error('Error thrown during checkAccess()')) + } +} + +describe('ACLChecker unit test', () => { + it('should callback with null on grant success', done => { + let ACLChecker = proxyquire('../lib/acl-checker', { + 'solid-permissions': { PermissionSet: PermissionSetAlwaysGrant } + }) + let graph = {} + let accessType = '' + let user, mode, resource, aclUrl + let acl = new ACLChecker({ debug }) + acl.checkAccess(graph, user, mode, resource, accessType, aclUrl, (err) => { + assert.isUndefined(err, + 'Granted permission should result in an empty callback!') + done() + }) + }) + it('should callback with error on grant failure', done => { + let ACLChecker = proxyquire('../lib/acl-checker', { + 'solid-permissions': { PermissionSet: PermissionSetNeverGrant } + }) + let graph = {} + let accessType = '' + let user, mode, resource, aclUrl + let acl = new ACLChecker({ debug }) + acl.checkAccess(graph, user, mode, resource, accessType, aclUrl, (err) => { + assert.ok(err instanceof Error, + 'Denied permission should result in an error callback!') + done() + }) + }) + it('should callback with error on grant error', done => { + let ACLChecker = proxyquire('../lib/acl-checker', { + 'solid-permissions': { PermissionSet: PermissionSetAlwaysError } + }) + let graph = {} + let accessType = '' + let user, mode, resource, aclUrl + let acl = new ACLChecker({ debug }) + acl.checkAccess(graph, user, mode, resource, accessType, aclUrl, (err) => { + assert.ok(err instanceof Error, + 'Error during checkAccess should result in an error callback!') + done() + }) + }) +})