From e03b2f3b2cc5a8fafdfa7ccb35faf7e6b92e5ff9 Mon Sep 17 00:00:00 2001 From: Arne Hassel Date: Sat, 26 Jan 2019 23:26:54 +0100 Subject: [PATCH 1/4] Added special handling for root To not break old PODs that have a root ACL that doesn't give READ access to root - will check for the ACL for the representation that is served back for root --- lib/acl-checker.js | 4 +- lib/handlers/allow.js | 11 +++- lib/resource-mapper.js | 19 ++++++ test/integration/authentication-oidc-test.js | 12 ++-- test/integration/special-root-acl-handling.js | 66 +++++++++++++++++++ .../accounts-acl/nicola.localhost/.acl | 1 + .../accounts-acl/nicola.localhost/index.html | 1 + .../nicola.localhost/index.html.acl | 10 +++ .../alice/private-for-owner.txt | 1 + .../alice/private-for-owner.txt.acl | 5 ++ 10 files changed, 121 insertions(+), 9 deletions(-) create mode 100644 test/integration/special-root-acl-handling.js create mode 100644 test/resources/accounts-acl/nicola.localhost/.acl create mode 100644 test/resources/accounts-acl/nicola.localhost/index.html create mode 100644 test/resources/accounts-acl/nicola.localhost/index.html.acl create mode 100644 test/resources/accounts-scenario/alice/private-for-owner.txt create mode 100644 test/resources/accounts-scenario/alice/private-for-owner.txt.acl diff --git a/lib/acl-checker.js b/lib/acl-checker.js index 6e15a0d00..df59b3e6b 100644 --- a/lib/acl-checker.js +++ b/lib/acl-checker.js @@ -114,8 +114,8 @@ class ACLChecker { throw new HTTPError(500, `No ACL found for ${resource}, searched in \n- ${acls.join('\n- ')}`) } const groupUrls = returnAcl.graph - .statementsMatching(null, ACL('agentGroup'), null) - .map(node => node.object.value.split('#')[0]) + .statementsMatching(null, ACL('agentGroup'), null) + .map(node => node.object.value.split('#')[0]) await Promise.all(groupUrls.map(groupUrl => { this.requests[groupUrl] = this.requests[groupUrl] || this.fetch(groupUrl, returnAcl.graph) return this.requests[groupUrl] diff --git a/lib/handlers/allow.js b/lib/handlers/allow.js index b18c2e0a7..a376b61de 100644 --- a/lib/handlers/allow.js +++ b/lib/handlers/allow.js @@ -44,7 +44,8 @@ function allow (mode, checkPermissionsForDirectory) { trustedOrigins.push(ldp.serverUri) } // Obtain and store the ACL of the requested resource - req.acl = ACL.createFromLDPAndRequest(rootUrl + resourcePath, ldp, req) + const resourceUrl = rootUrl + resourcePath + req.acl = ACL.createFromLDPAndRequest(resourceUrl, ldp, req) // Ensure the user has the required permission const userId = req.session.userId @@ -52,6 +53,14 @@ function allow (mode, checkPermissionsForDirectory) { if (isAllowed) { return next() } + if (mode === 'Read' && (resourcePath === '' || resourcePath === '/')) { + const representationUrl = await ldp.resourceMapper.getRepresentationUrlForResource(resourceUrl) + req.acl = ACL.createFromLDPAndRequest(representationUrl, ldp, req) + const representationIsAllowed = await req.acl.can(userId, mode) + if (representationIsAllowed) { + return next() + } + } const error = await req.acl.getError(userId, mode) debug(`${mode} access denied to ${userId || '(none)'}: ${error.status} - ${error.message}`) next(error) diff --git a/lib/resource-mapper.js b/lib/resource-mapper.js index 241996bf6..d9811400e 100644 --- a/lib/resource-mapper.js +++ b/lib/resource-mapper.js @@ -90,6 +90,25 @@ class ResourceMapper { return { path, contentType: contentType || this._defaultContentType } } + async getRepresentationUrlForResource (resourceUrl) { + let fullPath = this.getFullPath(resourceUrl) + let isIndex = fullPath.endsWith('/') + + // Append index filename if the URL ends with a '/' + if (isIndex) { + fullPath += this._indexFilename + } + + // Read all files in the corresponding folder + const filename = fullPath.substr(fullPath.lastIndexOf('/') + 1) + const folder = fullPath.substr(0, fullPath.length - filename.length) + const files = await this._readdir(folder) + + // Find a file with the same name (minus the dollar extension) + let match = (files.find(f => this._removeDollarExtension(f) === filename || (isIndex && f.startsWith(this._indexFilename + '.')))) + return `${resourceUrl}${match || ''}` + } + // Maps a given server file to a URL async mapFileToUrl ({ path, hostname }) { // Determine the URL by chopping off everything after the dollar sign diff --git a/test/integration/authentication-oidc-test.js b/test/integration/authentication-oidc-test.js index ccc2d7d19..54281eec3 100644 --- a/test/integration/authentication-oidc-test.js +++ b/test/integration/authentication-oidc-test.js @@ -164,7 +164,7 @@ describe('Authentication API (OIDC)', () => { describe('with that cookie and a non-matching origin', () => { let response before(done => { - alice.get('/') + alice.get('/private-for-owner.txt') .set('Cookie', cookie) .set('Origin', bobServerUri) .end((err, res) => { @@ -234,7 +234,7 @@ describe('Authentication API (OIDC)', () => { describe('with that cookie and our origin', () => { let response before(done => { - alice.get('/') + alice.get('/private-for-owner.txt') .set('Cookie', cookie) .set('Origin', 'https://some.other.domain.com') .end((err, res) => { @@ -252,7 +252,7 @@ describe('Authentication API (OIDC)', () => { describe('without that cookie but with our origin', () => { let response before(done => { - alice.get('/') + alice.get('/private-for-owner.txt') .set('Origin', aliceServerUri) .end((err, res) => { response = res @@ -324,7 +324,7 @@ describe('Authentication API (OIDC)', () => { describe('without that cookie and a matching origin', () => { let response before(done => { - alice.get('/') + alice.get('/private-for-owner.txt') .set('Origin', bobServerUri) .end((err, res) => { response = res @@ -341,7 +341,7 @@ describe('Authentication API (OIDC)', () => { describe('with that cookie and a non-matching origin', () => { let response before(done => { - alice.get('/') + alice.get('/private-for-owner.txt') .set('Cookie', cookie) .set('Origin', bobServerUri) .end((err, res) => { @@ -360,7 +360,7 @@ describe('Authentication API (OIDC)', () => { let response before(done => { var malcookie = cookie.replace(/connect\.sid=(\S+)/, 'connect.sid=l33th4x0rzp0wn4g3;') - alice.get('/') + alice.get('/private-for-owner.txt') .set('Cookie', malcookie) .set('Origin', bobServerUri) .end((err, res) => { diff --git a/test/integration/special-root-acl-handling.js b/test/integration/special-root-acl-handling.js new file mode 100644 index 000000000..aea464f78 --- /dev/null +++ b/test/integration/special-root-acl-handling.js @@ -0,0 +1,66 @@ +const assert = require('chai').assert +const request = require('request') +const path = require('path') +const { checkDnsSettings, cleanDir } = require('../utils') + +const ldnode = require('../../index') + +const port = 7777 +const serverUri = `https://localhost:${port}` +const root = path.join(__dirname, '../resources/accounts-acl') +const dbPath = path.join(root, 'db') +const configPath = path.join(root, 'config') + +function createOptions (path = '') { + return { + url: `https://nicola.localhost:${port}${path}` + } +} + +describe('Special handling: Root ACL does not give READ access to root', () => { + let ldp, ldpHttpsServer + + before(checkDnsSettings) + + before(done => { + ldp = ldnode.createServer({ + root, + serverUri, + dbPath, + port, + configPath, + sslKey: path.join(__dirname, '../keys/key.pem'), + sslCert: path.join(__dirname, '../keys/cert.pem'), + webid: true, + multiuser: true, + auth: 'oidc', + strictOrigin: true, + host: { serverUri } + }) + ldpHttpsServer = ldp.listen(port, done) + }) + + after(() => { + if (ldpHttpsServer) ldpHttpsServer.close() + cleanDir(root) + }) + + describe('should still grant READ access to everyone because of index.html.acl', () => { + it('for root with /', function (done) { + var options = createOptions('/') + request.get(options, function (error, response, body) { + assert.equal(error, null) + assert.equal(response.statusCode, 200) + done() + }) + }) + it('for root without /', function (done) { + var options = createOptions() + request.get(options, function (error, response, body) { + assert.equal(error, null) + assert.equal(response.statusCode, 200) + done() + }) + }) + }) +}) diff --git a/test/resources/accounts-acl/nicola.localhost/.acl b/test/resources/accounts-acl/nicola.localhost/.acl new file mode 100644 index 000000000..de881069a --- /dev/null +++ b/test/resources/accounts-acl/nicola.localhost/.acl @@ -0,0 +1 @@ +# This ACL does nothing by default diff --git a/test/resources/accounts-acl/nicola.localhost/index.html b/test/resources/accounts-acl/nicola.localhost/index.html new file mode 100644 index 000000000..cb12e071b --- /dev/null +++ b/test/resources/accounts-acl/nicola.localhost/index.html @@ -0,0 +1 @@ +Everyone should get READ access for this file through 
index.html.acl
. diff --git a/test/resources/accounts-acl/nicola.localhost/index.html.acl b/test/resources/accounts-acl/nicola.localhost/index.html.acl new file mode 100644 index 000000000..19f9544f3 --- /dev/null +++ b/test/resources/accounts-acl/nicola.localhost/index.html.acl @@ -0,0 +1,10 @@ +# This file grants everyone READ access to ./index.html + +@prefix acl: . +@prefix foaf: . + +<#public> + a acl:Authorization; + acl:agentClass foaf:Agent; + acl:accessTo <./index.html>; + acl:mode acl:Read. diff --git a/test/resources/accounts-scenario/alice/private-for-owner.txt b/test/resources/accounts-scenario/alice/private-for-owner.txt new file mode 100644 index 000000000..5d39eb781 --- /dev/null +++ b/test/resources/accounts-scenario/alice/private-for-owner.txt @@ -0,0 +1 @@ +protected contents for owner diff --git a/test/resources/accounts-scenario/alice/private-for-owner.txt.acl b/test/resources/accounts-scenario/alice/private-for-owner.txt.acl new file mode 100644 index 000000000..fbc63348b --- /dev/null +++ b/test/resources/accounts-scenario/alice/private-for-owner.txt.acl @@ -0,0 +1,5 @@ +<#Owner> + a ; + <./private-for-owner.txt>; + ; + , , . From 5e62f8cd699314fbe4e6063a4de3cab45e4b33e2 Mon Sep 17 00:00:00 2001 From: Arne Hassel Date: Sat, 26 Jan 2019 23:35:39 +0100 Subject: [PATCH 2/4] Hopefully making things a bit easier to read --- lib/acl-checker.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/acl-checker.js b/lib/acl-checker.js index df59b3e6b..53c41f86d 100644 --- a/lib/acl-checker.js +++ b/lib/acl-checker.js @@ -113,9 +113,8 @@ class ACLChecker { if (!returnAcl) { throw new HTTPError(500, `No ACL found for ${resource}, searched in \n- ${acls.join('\n- ')}`) } - const groupUrls = returnAcl.graph - .statementsMatching(null, ACL('agentGroup'), null) - .map(node => node.object.value.split('#')[0]) + const groupNodes = returnAcl.graph.statementsMatching(null, ACL('agentGroup'), null) + const groupUrls = groupNodes.map(node => node.object.value.split('#')[0]) await Promise.all(groupUrls.map(groupUrl => { this.requests[groupUrl] = this.requests[groupUrl] || this.fetch(groupUrl, returnAcl.graph) return this.requests[groupUrl] From 84e856c335dab299446d16cc11dd05f5c9121ef3 Mon Sep 17 00:00:00 2001 From: Arne Hassel Date: Sat, 26 Jan 2019 23:26:54 +0100 Subject: [PATCH 3/4] Added special handling for root To not break old PODs that have a root ACL that doesn't give READ access to root - will check for the ACL for the representation that is served back for root Hopefully making things a bit easier to read Added comment about the hack --- lib/handlers/allow.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/handlers/allow.js b/lib/handlers/allow.js index a376b61de..774450477 100644 --- a/lib/handlers/allow.js +++ b/lib/handlers/allow.js @@ -54,6 +54,8 @@ function allow (mode, checkPermissionsForDirectory) { return next() } if (mode === 'Read' && (resourcePath === '' || resourcePath === '/')) { + // This is a hack to make NSS check the ACL for representation that is served for root (if any) + // See https://github.com/solid/node-solid-server/issues/1063 for more info const representationUrl = await ldp.resourceMapper.getRepresentationUrlForResource(resourceUrl) req.acl = ACL.createFromLDPAndRequest(representationUrl, ldp, req) const representationIsAllowed = await req.acl.can(userId, mode) From f71e582d852833f5e4321d63d93c6f5cd7d30812 Mon Sep 17 00:00:00 2001 From: Arne Hassel Date: Tue, 29 Jan 2019 12:53:47 +0100 Subject: [PATCH 4/4] Added extra check so that only HTML files are handled with care The hack will only work when it's a HTML file that is returned as the representation for the root resource --- lib/handlers/allow.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/handlers/allow.js b/lib/handlers/allow.js index 774450477..851b93fa7 100644 --- a/lib/handlers/allow.js +++ b/lib/handlers/allow.js @@ -57,10 +57,13 @@ function allow (mode, checkPermissionsForDirectory) { // This is a hack to make NSS check the ACL for representation that is served for root (if any) // See https://github.com/solid/node-solid-server/issues/1063 for more info const representationUrl = await ldp.resourceMapper.getRepresentationUrlForResource(resourceUrl) - req.acl = ACL.createFromLDPAndRequest(representationUrl, ldp, req) - const representationIsAllowed = await req.acl.can(userId, mode) - if (representationIsAllowed) { - return next() + if (representationUrl.endsWith('index.html')) { + // We ONLY want to do this when the representation we return is a HTML file + req.acl = ACL.createFromLDPAndRequest(representationUrl, ldp, req) + const representationIsAllowed = await req.acl.can(userId, mode) + if (representationIsAllowed) { + return next() + } } } const error = await req.acl.getError(userId, mode)