Incorporate a list of allowed origins

[kjetilk]

The idea that I had behind this implementation, is that the newly named Permission class (and I chose to branch of that branch to make it more comprehensible) has a method allowsOriginthat already does the actual checking of the allowed origins. In that respect, the condition that our own origin should be granted access was a special case, and I simply removed that from the logic.

Instead, I decided to use the facilities already in this module to add origins, by having an array that could be passed to the constructor of the PermissionSet class.

This again, could be passed through from NSS.

It seems quite sane, but I am not totally confident about the logic around group ACLs and origin, but I think that is orthogonal to this issue.

This should fix #26 .

[RubenVerborgh]

No apparent issues.

[kjetilk]

So, I wasn't completely confident myself, so I did some more work on tests in nss, eventually ending up with a branch https://github.com/solid/node-solid-server/tree/origin-list-git-perm-org which makes sure NSS runs the test suite with the solid-permissions commits. With that, I have a failing test: https://travis-ci.org/solid/node-solid-server/jobs/423024761#L1587

My understanding is still a bit fluffy, I'm trying to understand whether that test is wrong, or the code has a bug.

I'm thinking the latter is the case, because in the test, the cookie is set, so the user is authenticated and given the authorization rules, they should have access, right?

[RubenVerborgh]

The second test failure bothers me, but I believe @justinwb has it fixed.

[kjetilk]

Yeah, the second and third are also seen in the git dep related branches, but the first is what worries me. What do you make of that, @RubenVerborgh ?

[justinwb]

Yes @kjetilk I can confirm that test failure #2 is resolved in the NSS release candidate that I've got, and #3 is simply a result of #2 failing, so that is resolved as well.

[dmitrizagidulin]

@kjetilk the rename looks great.

The "Allowed Origins" list looks ok, though I do have a question. Does it still check the "strict origin" server-side flag, and when it's disabled, ignore the default list of allowed origins?

[kjetilk]

@kjetilk the rename looks great.

Thanks, then I merged that part.

The "Allowed Origins" list looks ok, though I do have a question. Does it still check the "strict origin" server-side flag, and when it's disabled, ignore the default list of allowed origins?

Yeah, so that part of the logic hasn't been touched: 7d94ffa#diff-288570275da54b1fec61b0b6851061afL469

[dmitrizagidulin]

@kjetilk Got it, sounds great!

[RubenVerborgh]

Does it still check the "strict origin" server-side flag, and when it's disabled, ignore the default list of allowed origins?

Another way to see it would be that "strict origin" now means "only accept the allowed origins". We could have the convention that, if allowed origins are not passed (null / undefined), all origins are allowed. If an array is passed, only these origins are allowed. I don't think it's necessary to keep the separate "strict origin" setting; this unnecessarily ties solid-permissions to the configuration file of node-solid-server.

Such a change would be semver-major, however.

[kjetilk]

Now, we are down to one failing test: https://travis-ci.org/solid/node-solid-server/jobs/423853623#L1586

Anyone around to discuss its implications?

[kjetilk]

Could you please open a separate issue on that, @RubenVerborgh? I'm open to it, as I see your point that solid-permissions is too tied to the config of NSS, but OTOH, allowing all origins could be pretty dangerous, not something you'd want people to do by accident, and so I feel this gives an extra proofing against that.

[kjetilk]

After reorganizing the tests and try to reference them to the spec, I have 4 test failures: https://travis-ci.org/solid/node-solid-server/jobs/423913686#L1586

It looks like there are some bugs, and I don't think all of them are due to latest changes.

[RubenVerborgh]

@kjetilk Hmm, the test failures are obviously not really helpful, but I would guess that all of them are related.

[RubenVerborgh]

Could you please open a separate issue on that, @RubenVerborgh?

#32

allowing all origins could be pretty dangerous, not something you'd want people to do by accident

We could demand an explicit false instead of undefined/null. In any case, can't be the empty array, because that should mean “no trusted origins”.

[kjetilk]

@kjetilk Hmm, the test failures are obviously not really helpful, but I would guess that all of them are related.

They might. But the first thing we should do is to figure out if the tests are valid. If they are, then we can start to work out what bugs we have.

[RubenVerborgh]

Important to know is that, typically, an exception occurring in solid-permissions will lead to a 401 (without warning).

[kjetilk]

Actually, I think I'll be merging this unless someone screams, so that I can merge the NSS origin-list branch too, so that there is at least something running around this.