Making proper use of strictOrigins by megoth · Pull Request #1118 · nodeSolidServer/node-solid-server

megoth · 2019-02-28T16:30:30Z

We had some faulty handling where we returned 403 even if the user wasn't authenticated. (This fixes #1117.)

Some notes

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.
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.
Tried to consolidate the use of getTrustedOrigins in create-app.js, which made a test obsolete

megoth · 2019-02-28T16:35:34Z

I realize the number of changes introduced in test/integration/authentication-oidc-test.js might be a bit too much...

@kjetilk @RubenVerborgh @dmitrizagidulin let me know if you want me to ungroup the tests in that file, it should reduce the number of changes a lot. But it also makes it harder to get an overview of the tests =/ (which had already created some redundant tests)

megoth · 2019-02-28T16:36:29Z

Could also move test/integration/authentication-oidc-with-strict-origins-turned-off-test.js into a separate commit, if wanted; that would also reduce the number of changes a lot.

RubenVerborgh · 2019-02-28T19:59:16Z

-    const agentOrigin = this.agentOrigin ? rdf.sym(this.agentOrigin) : null
-    const trustedOrigins = this.trustedOrigins ? this.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
+    const agentOrigin = this.strictOrigin && this.agentOrigin ? rdf.sym(this.agentOrigin) : null
+    const trustedOrigins = this.strictOrigin && this.trustedOrigins ? this.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null


We do this every time? That might get expensive.

Are you thinking of the use of Array.prototype.map-method and/or the use of ternary operators?

RubenVerborgh · 2019-02-28T20:03:13Z

    // (https://github.com/solid/node-solid-server/pull/835#issuecomment-426429003)
-    if (!argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
+    // We only want to do this check if strictOrigin is set to false, since we trust WebACL to handle origins otherwise
+    // If we don't allow authentication with cookies we can't handle ACLs properly wrt trusted origins


That's not the right explanation. We can handle ACLs properly; with non-cookie auth mechanisms.

The full explanation is:

Authentication cookies are an optimization:
instead of going through the process of
fully validating authentication on every request,
we go through this process once,
and store its successful result in a cookie
that will be reused upon the next request.
However, that cookie can then be sent by any server,
even servers that have not gone through the proper authentication mechanism.
However, if trusted origins are enabled,
then any origin is allowed to take the shortcut route,
since malicious origins will be banned at the ACL checking phase.

I'm having problem wrapping my head around the reasoning for this part of the code, so thank you for the explanation. I think it's a good explanation, so I added it to the PR, hope that's ok.

RubenVerborgh · 2019-02-28T20:04:05Z

-    if (originHost === userHost) return true
-    if (trustedOrigins.includes(origin)) return true
-    return false
+    return !!trustedOrigins.includes(origin)


which by default included the user's own domain?

Apparently not, given the test: is this intended?

I tried to reuse ldp.getTrustedOrigins, which includes the user's own domain if the server is a multiuser server.

I thought we could remove some redundant code, but maybe I should revert this change back, since others might not use ldp.getTrustedOrigins?

Closes #1106.

We had some faulty handling where we returned 403 even if the user wasn't authenticated. (This fixes #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

My previous explanation was wrong

This reverts commit 90dcfe6.

This reverts commit 01d2a35.

megoth · 2019-03-02T22:28:10Z

I just discovered that some tests will fail if run by themselves, but succeed if run as part of running all tests in that file...

An example of this is "ACL with WebID+OIDC over HTTP > Origin > user1 should be able to access test directory". This should actually fail, at least if I understand the logic correctly.

I will try to commit a fix for this soon.

megoth · 2019-03-03T01:15:31Z

Added two more commits that removes the need of ordering in tests running in test/integration/acl-oidc-test.js and test/integration/acl-tls-test.js.

I think these commits could've been extracted into a separate PR for clarity. Let me know if this is desired.

This branch is dependent on https://github.com/solid/acl-check/tree/feature/trusted-app and should be merged after #1118. This should fix #1121

megoth · 2019-03-06T12:12:21Z

@RubenVerborgh I moved some code into the constructor of the ACLChecker, tests verifies it works nicely ^_^

I've fell a bit out of loop on this PR, were there more feedback you had that I should fix?

kjetilk

OK, it looks good. What makes me uneasy is the very extensive changes to authentication-oidc-test.js, since a lot of lines have changed, it had very thorough review before this, so I was thinking that not much should change in there.

I suppose your tooling did many of those changes, @megoth , so perhaps it is just that, but it is hard for me to see what has actually changed there.

megoth · 2019-03-07T11:46:42Z

OK, it looks good. What makes me uneasy is the very extensive changes to authentication-oidc-test.js, since a lot of lines have changed, it had very thorough review before this, so I was thinking that not much should change in there.

I suppose your tooling did many of those changes, @megoth , so perhaps it is just that, but it is hard for me to see what has actually changed there.

It was a deliberate choice, because the tests were badly grouped which made them very hard to read.

But I can revert those changes and we could live with it instead of having this many changes.

kjetilk · 2019-03-07T12:46:44Z

It was a deliberate choice, because the tests were badly grouped which made them very hard to read.

But I can revert those changes and we could live with it instead of having this many changes.

I'm all for cleaning up, but this ended up being a rather large change, and I find it hard to review. So, perhaps the grouping changes could be done in a separate PR? Would that be a lot of work at this point?

megoth · 2019-03-07T16:33:21Z

It was a deliberate choice, because the tests were badly grouped which made them very hard to read.
But I can revert those changes and we could live with it instead of having this many changes.

I'm all for cleaning up, but this ended up being a rather large change, and I find it hard to review. So, perhaps the grouping changes could be done in a separate PR? Would that be a lot of work at this point?

I'm going to try to revert the big changes to authentication-oidc-test.js, so that you only have to review my added tests ^_^

And applying the new tests that were added additionally on the reverted file

This reverts commit 06c5b19.

Trying to limit the number of changes

megoth · 2019-03-07T19:12:16Z

@kjetilk I've reverted a bunch of unnecessary changes, most of the changes are now simply new files. Let me know if you want me to do more 😸

kjetilk · 2019-03-07T23:43:48Z


-          it('should return a 403', () => {
-            expect(response).to.have.property('status', 403)
+          it('should return a 401', () => {


Not terribly important, but as per the comment above the test, I still think 403 is the correct status. How hard would it be to fix?

kjetilk · 2019-03-07T23:46:30Z


-          it('should return a 403', () => {
-            expect(response).to.have.property('status', 403)
+          it('should return a 401', () => {


Again here I would prefer a 403, since it is good to fend off an attacker at once, but it is even less important than the previous comment, since it is most important to just deny them.

kjetilk · 2019-03-07T23:49:13Z

Two small comments inline, the rest looks good, thanks a lot. :-)

Please feel free to submit a separate PR with the cleanup later!

kjetilk

On second thoughts: Even though I would prefer to have a 403 when a log in wouldn't matter, in the light of the problem #1117 that this is intended to fix, doing that would probably add complexity that is not worth it.

Redo #1118

megoth requested review from RubenVerborgh, dmitrizagidulin and kjetilk February 28, 2019 16:30

RubenVerborgh reviewed Feb 28, 2019

View reviewed changes

megoth mentioned this pull request Feb 28, 2019

Lock file during PATCH #1116

Merged

RubenVerborgh and others added 5 commits March 2, 2019 21:51

Add proper-lockfile dependency.

9876274

Lock file during patching.

74e5464

Closes #1106.

Fixed some faulty tests

c8a23a9

Added RubenVerborgh explanation

01853af

My previous explanation was wrong

megoth force-pushed the fix/faulty-403 branch from 8efc6b2 to 01853af Compare March 2, 2019 20:55

megoth added 3 commits March 2, 2019 22:03

Revert "Lock file during patching."

f5d2816

This reverts commit 90dcfe6.

Did a faulty rebase, trying to revert commits

017ca05

Revert "Add proper-lockfile dependency."

7c0d436

This reverts commit 01d2a35.

megoth mentioned this pull request Mar 2, 2019

Tests fail irregularly #1119

Open

megoth added 2 commits March 3, 2019 01:50

Trying to make tests less dependent on order

06c5b19

Removing the need to run tests in order

1998a5b

megoth added a commit that referenced this pull request Mar 3, 2019

Making use of new features in acl-check@trusted-app

2233f13

This branch is dependent on https://github.com/solid/acl-check/tree/feature/trusted-app and should be merged after #1118. This should fix #1121

megoth mentioned this pull request Mar 3, 2019

Making use of new features in acl-check@trusted-app #1122

Closed

Some optimizations

0ece321

megoth requested a review from RubenVerborgh March 6, 2019 12:11

RubenVerborgh approved these changes Mar 6, 2019

View reviewed changes

kjetilk reviewed Mar 7, 2019

View reviewed changes

kjetilk requested review from dmitrizagidulin and removed request for dmitrizagidulin March 7, 2019 11:01

megoth added 5 commits March 7, 2019 19:46

Trying to revert authentication-oidc-test manually

b486abe

And applying the new tests that were added additionally on the reverted file

Fixing alignment

de3b0e0

Revert "Trying to make tests less dependent on order"

4fd4f90

This reverts commit 06c5b19.

Making changes minimal for acl-tls-test.js

7b75a0b

Reverting changes to solid-host

e4cf1e2

Trying to limit the number of changes

kjetilk reviewed Mar 7, 2019

View reviewed changes

kjetilk approved these changes Mar 8, 2019

View reviewed changes

kjetilk merged commit eb6b62b into release/v5.0.0 Mar 8, 2019

kjetilk deleted the fix/faulty-403 branch March 8, 2019 21:30

kjetilk restored the fix/faulty-403 branch March 8, 2019 22:03

This was referenced Mar 9, 2019

Returns 403 instead of 401 for certain requests #1117

Closed

Fix faulty 403 #1137

Closed

This was referenced Mar 12, 2019

[do not merge] Enable trusted apps in dependencies #1138

Closed

Require cross-origin requests to use bearer tokens instead of cookies? solid/solid-spec#144

Open

michielbdejong added a commit to michielbdejong/node-solid-server that referenced this pull request Mar 12, 2019

Redo nodeSolidServer#1118

5980d75

kjetilk added a commit that referenced this pull request Mar 15, 2019

Merge pull request #1140 from michielbdejong/redo-1118

0339003

Redo #1118

Conversation

megoth commented Feb 28, 2019

Some notes

Uh oh!

megoth commented Feb 28, 2019

Uh oh!

megoth commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megoth commented Mar 2, 2019

Uh oh!

megoth commented Mar 3, 2019

Uh oh!

megoth commented Mar 6, 2019

Uh oh!

kjetilk left a comment

Choose a reason for hiding this comment

Uh oh!

megoth commented Mar 7, 2019

Uh oh!

kjetilk commented Mar 7, 2019

Uh oh!

megoth commented Mar 7, 2019

Uh oh!

megoth commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjetilk commented Mar 7, 2019

Uh oh!

kjetilk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants