search

solid-contrib / web-access-control-tests

Tests if a Solid server implements web access control correctly
MIT License
2 stars 5 forks source link

remove Append success with PUT and edit PATCH success from 200 to 2xx #17

Closed bourgeoa closed 3 years ago

bourgeoa commented 3 years ago

This is following the discussion on https://gitter.im/solid/test-suite

Alain Bourgeois
@bourgeoa
janv. 11 12:31

@csarven from the tests I see that update has 2 tests versions :

    append with a PUT to target URL requiring acl:Append with If-Match : etag on target, or acl:Write on parent
    overwrite with a PUT to target URL requiring acl:Write (to target or parent)

Is this correct ? What is the difference between URL exists and if-Match: etag
This append for PUT seems very new to me.
Yvo Brevoort
@ylebre
janv. 11 13:32
that is a weird one - you'd expect it to need 'Write' for that
but because the beginning of the file does not change, Append suffices
it is changed from 'hello' to 'hello world'
so it is treated as if the request only appends ' world' to the file
Sarven Capadisli
@csarven
janv. 11 14:33
PUT is a replace operation. Need Write. Append doesn't cut it.
Need Append/Write access on container in order to allocate the URI under that space.

and a new proposal to edit in tests the PATCH success to allow 2xx for 200 or 201, like in other places in the tests.

ylebre commented 3 years ago

This excerpt from the spec is relevant for this discussion, from http://solid.github.io/web-access-control-spec/#aclappend: ... some implementations may also extend this mode to cover non-overwriting PUTs, ...

My understanding of this tidbit is that al long as the contents of the original is not changed, it is considered an append operation and not an overwrite. I do agree that would be better to remove this test because it states that 'some implementations may ...' - it should not be a requirement to implement this to pass the tests.

bourgeoa commented 3 years ago

Thanks for pointing this. This is not in line with @csarven position. But as you quoted a MAY should not be a requirement to pass the tests.

ylebre commented 3 years ago

Hrm, I failed to point out that the test modification is incorrect: it('Is disallowed with default Read+Append+Control access on parent', async () => { ... continues to put 'hello world' on a resource previously containing 'hello'.

as per spec, the server MAY allow this with Append, so the test should not fail when it is a non-overwriting PUT as described before.