Skip to content

Commit a4aeb9e

Browse files
megothmegoth
committed
Making proper use of strictOrigins
We had some faulty handling where we returned 403 even if the user wasn't authenticated. (This fixes nodeSolidServer#1117.) 1. I discovered some redundant tests in test/integration/authentication-oidc-test.js, and moved some tests into groups for better overview. This makes for a lot of changes, but I think it's worth it. 2. I duplicated authentication-oidc-test into authentication-oidc-with-strict-origins-turned-off-test to make sure we handle those permutations correctly as well. This required a lot of setup, which are all of the new files in this commit. 3. I tried to consolidate the use of getTrustedOrigins in create-app.js, which made a test obsolete
1 parent 74e5464 commit a4aeb9e

16 files changed

Lines changed: 1559 additions & 240 deletions

‎lib/acl-checker.js‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,17 +56,14 @@ class ACLChecker {
5656
const aclFile = rdf.sym(acl.acl)
5757
const agent = user ? rdf.sym(user) : null
5858
const modes = [ACL(mode)]
59-
const agentOrigin = this.agentOrigin ? rdf.sym(this.agentOrigin) : null
60-
const trustedOrigins = this.trustedOrigins ? this.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
59+
const agentOrigin = this.strictOrigin && this.agentOrigin ? rdf.sym(this.agentOrigin) : null
60+
const trustedOrigins = this.strictOrigin && this.trustedOrigins ? this.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
6161
const accessDenied = aclCheck.accessDenied(acl.graph, resource, directory, aclFile, agent, modes, agentOrigin, trustedOrigins)
62-
if (accessDenied && this.agentOrigin && this.resourceUrl.origin !== this.agentOrigin) {
63-
this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
64-
} else if (accessDenied && user) {
62+
63+
if (accessDenied && user) {
6564
this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
66-
} else if (accessDenied && !user) {
67-
this.messagesCached[cacheKey].push(HTTPError(401, 'Unauthenticated'))
6865
} else if (accessDenied) {
69-
this.messagesCached[cacheKey].push(HTTPError(401, accessDenied))
66+
this.messagesCached[cacheKey].push(HTTPError(401, 'Unauthenticated'))
7067
}
7168
this.aclCached[cacheKey] = Promise.resolve(!accessDenied)
7269
return this.aclCached[cacheKey]

‎lib/create-app.js‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,14 +206,17 @@ function initWebId (argv, app, ldp) {
206206
// without permission by including the credentials set by the Solid server.
207207
app.use((req, res, next) => {
208208
const origin = req.get('origin')
209-
const trustedOrigins = argv.trustedOrigins
209+
const trustedOrigins = ldp.getTrustedOrigins(req)
210210
const userId = req.session.userId
211211
// Exception: allow logout requests from all third-party apps
212212
// such that OIDC client can log out via cookie auth
213213
// TODO: remove this exception when OIDC clients
214214
// use Bearer token to authenticate instead of cookie
215215
// (https://github.com/solid/node-solid-server/pull/835#issuecomment-426429003)
216-
if (!argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
216+
// We only want to do this check if strictOrigin is set to false, since we trust WebACL to handle origins otherwise
217+
// If we don't allow authentication with cookies we can't handle ACLs properly wrt trusted origins
218+
// https://github.com/solid/node-solid-server/issues/1117
219+
if (!argv.strictOrigin && !argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
217220
debug.authentication(`Rejecting session for ${userId} from ${origin}`)
218221
// Destroy session data
219222
delete req.session.userId

‎lib/models/solid-host.js‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,7 @@ class SolidHost {
7777
const serverHost = getHostName(this.serverUri)
7878
if (originHost === serverHost) return true
7979
if (originHost.endsWith('.' + serverHost)) return true
80-
// Allow the user's own domain
81-
const userHost = getHostName(userId)
82-
if (originHost === userHost) return true
83-
if (trustedOrigins.includes(origin)) return true
84-
return false
80+
return !!trustedOrigins.includes(origin)
8581
}
8682

8783
/**

‎test/integration/acl-tls-test.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ describe('ACL with WebID+TLS', function () {
332332

333333
request.head(options, function (error, response, body) {
334334
assert.equal(error, null)
335-
assert.equal(response.statusCode, 403)
335+
assert.equal(response.statusCode, 401)
336336
done()
337337
})
338338
})
@@ -354,7 +354,7 @@ describe('ACL with WebID+TLS', function () {
354354

355355
request.head(options, function (error, response, body) {
356356
assert.equal(error, null)
357-
assert.equal(response.statusCode, 403)
357+
assert.equal(response.statusCode, 401)
358358
done()
359359
})
360360
})
@@ -433,7 +433,7 @@ describe('ACL with WebID+TLS', function () {
433433

434434
request.head(options, function (error, response, body) {
435435
assert.equal(error, null)
436-
assert.equal(response.statusCode, 403)
436+
assert.equal(response.statusCode, 401)
437437
done()
438438
})
439439
})
@@ -455,7 +455,7 @@ describe('ACL with WebID+TLS', function () {
455455

456456
request.head(options, function (error, response, body) {
457457
assert.equal(error, null)
458-
assert.equal(response.statusCode, 403)
458+
assert.equal(response.statusCode, 401)
459459
done()
460460
})
461461
})

0 commit comments

Comments
 (0)